Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: update v8_inspector #7385

Closed
wants to merge 1 commit into from
Closed

Conversation

targos
Copy link
Member

@targos targos commented Jun 23, 2016

Checklist
  • make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
  • the commit message follows commit guidelines
Description of change

Pick up latest commit of v8_inspector and inspector_protocol.
This brings back compatibility with V8 5.1.

R= @ofrobots

Pick up latest commit of v8_inspector and inspector_protocol.
This brings back compatibility with V8 5.1.
@targos targos added the inspector Issues and PRs related to the V8 inspector protocol label Jun 23, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Jun 23, 2016

LGTM

@mscdex
Copy link
Contributor

mscdex commented Jun 23, 2016

Some questions/notes:

  1. What's with the whitespace additions?
  2. The associated comment for the #endif needs to be updated.
  3. Why is this labeled dont-land-on-v6.x? /cc @evanlucas

@targos
Copy link
Member Author

targos commented Jun 23, 2016

  1. The whitespaces are in the original files
  2. I agree with you and that's the change I proposed but another CL was merged before mine without this update (diff)

@ofrobots
Copy link
Contributor

  1. This is an update of the dependencies; and the whitespace exists upstream. It shows up as a diff in this patch because we have a process to land PRs with --whitespace=fix. This will show up in future PRs too. Perhaps we should land deps-only PRs without whitespace fixup.
  2. @targos: Sorry for the miscommunication. I think you should update your CL to fix the comment and still land it upstream. However, I do not think we should have to wait for the comment update before landing this PR. The comment will be picked up in the next roll of the dependency.

@evanlucas
Copy link
Contributor

@mscdex I marked it dont-land-on-v6.x because the original pr has not landed on v6.x and it makes it easier to reason about commits to cherry-pick for releases. That being said, I am pushing for the next v6.x release to include it and would then remove the label.

@ofrobots
Copy link
Contributor

@targos
Copy link
Member Author

targos commented Jun 27, 2016

@ofrobots thanks. Maybe you could help with https://codereview.chromium.org/2084343002/. The buildbot didn't use the email address I want.

@targos
Copy link
Member Author

targos commented Jun 28, 2016

landed in ecd639a

@targos targos closed this Jun 28, 2016
targos added a commit that referenced this pull request Jun 28, 2016
Pick up latest commit of v8_inspector and inspector_protocol.
This brings back compatibility with V8 5.1.

PR-URL: #7385
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@targos targos deleted the update-v8-inspector branch June 28, 2016 10:53
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
Pick up latest commit of v8_inspector and inspector_protocol.
This brings back compatibility with V8 5.1.

PR-URL: #7385
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@Fishrock123 Fishrock123 mentioned this pull request Jul 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants