Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11615021:
Adopt inspector protocol changes. (Closed)
|
Created:
8 years ago by eustas Modified:
7 years, 11 months ago CC:
chromium-reviews, dennis_jeffrey, anantha, dyu1 Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdopt inspector protocol changes.
Add backward compatibility to pyautolib.
See https://bugs.webkit.org/show_bug.cgi?id=104545
BUG=166160
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177677
Patch Set 1 #
Total comments: 9
Patch Set 2 : Take-2 #
Total comments: 2
Patch Set 3 : Fixed nit #Patch Set 4 : Rebased #Patch Set 5 : Updated target Webkit revision #Messages
Total messages: 16 (0 generated)
LGTM, assuming you've tested this with chromium < 25 and chromium-with-the-dev-tools-changes. (The version getting API is present in both, right?)
On 2012/12/19 14:51:21, marja wrote: > LGTM, assuming you've tested this with chromium < 25 and > chromium-with-the-dev-tools-changes. (The version getting API is present in > both, right?) Tested on M24 - works. Tested on tip of tree (with b/104545 patch) - works. JFYI: On tip of tree leak finder reports "No leaks found" for test page.
https://codereview.chromium.org/11615021/diff/1/chrome/test/pyautolib/remote_... File chrome/test/pyautolib/remote_inspector_client.py (right): https://codereview.chromium.org/11615021/diff/1/chrome/test/pyautolib/remote_... chrome/test/pyautolib/remote_inspector_client.py:285: endpoint: The base URL to connent to. may be rename it to url? https://codereview.chromium.org/11615021/diff/1/chrome/test/pyautolib/remote_... chrome/test/pyautolib/remote_inspector_client.py:522: # TODO(eustas): Remove this case after M25 is released. This probably should be removed after M26 as M25 has already branched. https://codereview.chromium.org/11615021/diff/1/chrome/test/pyautolib/remote_... chrome/test/pyautolib/remote_inspector_client.py:881: # TODO(eustas): Remove this hack after M25 is released. Again "... after M26..." https://codereview.chromium.org/11615021/diff/1/chrome/test/pyautolib/remote_... chrome/test/pyautolib/remote_inspector_client.py:1275: elif version['major'] == major and version['minor'] < minor: version['minor'] < minor -> version['minor'] <= minor
https://codereview.chromium.org/11615021/diff/1/chrome/test/pyautolib/remote_... File chrome/test/pyautolib/remote_inspector_client.py (right): https://codereview.chromium.org/11615021/diff/1/chrome/test/pyautolib/remote_... chrome/test/pyautolib/remote_inspector_client.py:285: endpoint: The base URL to connent to. On 2012/12/20 08:04:48, Yury Semikhatsky wrote: > may be rename it to url? Done. https://codereview.chromium.org/11615021/diff/1/chrome/test/pyautolib/remote_... chrome/test/pyautolib/remote_inspector_client.py:522: # TODO(eustas): Remove this case after M25 is released. On 2012/12/20 08:04:48, Yury Semikhatsky wrote: > This probably should be removed after M26 as M25 has already branched. Done. https://codereview.chromium.org/11615021/diff/1/chrome/test/pyautolib/remote_... chrome/test/pyautolib/remote_inspector_client.py:881: # TODO(eustas): Remove this hack after M25 is released. On 2012/12/20 08:04:48, Yury Semikhatsky wrote: > Again "... after M26..." Done. https://codereview.chromium.org/11615021/diff/1/chrome/test/pyautolib/remote_... chrome/test/pyautolib/remote_inspector_client.py:1275: elif version['major'] == major and version['minor'] < minor: On 2012/12/20 08:04:48, Yury Semikhatsky wrote: > version['minor'] < minor -> version['minor'] <= minor Not older than == same or newer. So if minor and major version are matched, result should be true. If we place "<=" here result will be false...
https://codereview.chromium.org/11615021/diff/1/chrome/test/pyautolib/remote_... File chrome/test/pyautolib/remote_inspector_client.py (right): https://codereview.chromium.org/11615021/diff/1/chrome/test/pyautolib/remote_... chrome/test/pyautolib/remote_inspector_client.py:1275: elif version['major'] == major and version['minor'] < minor: On 2012/12/20 08:27:30, eustas.ru wrote: > On 2012/12/20 08:04:48, Yury Semikhatsky wrote: > > version['minor'] < minor -> version['minor'] <= minor > > Not older than == same or newer. > > So if minor and major version are matched, result should be true. > > If we place "<=" here result will be false... You are right.
lgtm
You'll need OWNERS stamp for this change.
LGTM with a nit to consider before submitting. https://codereview.chromium.org/11615021/diff/4003/chrome/test/pyautolib/remo... File chrome/test/pyautolib/remote_inspector_client.py (right): https://codereview.chromium.org/11615021/diff/4003/chrome/test/pyautolib/remo... chrome/test/pyautolib/remote_inspector_client.py:1268: version = self._webkit_version will this work as expected if self._webkit_version hasn't yet been defined at all, or we will get a runtime exception saying that this variable hasn't been defined? Maybe instead we could do something like: if not hasattr(self, '_webkit_version'): raise RuntimeError...
Fixed nit. https://codereview.chromium.org/11615021/diff/4003/chrome/test/pyautolib/remo... File chrome/test/pyautolib/remote_inspector_client.py (right): https://codereview.chromium.org/11615021/diff/4003/chrome/test/pyautolib/remo... chrome/test/pyautolib/remote_inspector_client.py:1268: version = self._webkit_version On 2012/12/20 18:22:49, dennis_jeffrey wrote: > will this work as expected if self._webkit_version hasn't yet been defined at > all, or we will get a runtime exception saying that this variable hasn't been > defined? > > Maybe instead we could do something like: > > if not hasattr(self, '_webkit_version'): > raise RuntimeError... Done.
On 2012/12/20 07:54:30, eustas.ru wrote: > On 2012/12/19 14:51:21, marja wrote: > > LGTM, assuming you've tested this with chromium < 25 and > > chromium-with-the-dev-tools-changes. (The version getting API is present in > > both, right?) > > Tested on M24 - works. > Tested on tip of tree (with b/104545 patch) - works. > > JFYI: On tip of tree leak finder reports "No leaks found" for test page. Hmm? This sounds bad, sounds like it does *not* work for tip of tree. Has the heap snapshot format changed again, or what might this be about? What's b/104545 patch?
On 2013/01/07 08:51:45, marja wrote: > On 2012/12/20 07:54:30, eustas.ru wrote: > > On 2012/12/19 14:51:21, marja wrote: > > > LGTM, assuming you've tested this with chromium < 25 and > > > chromium-with-the-dev-tools-changes. (The version getting API is present in > > > both, right?) > > > > Tested on M24 - works. > > Tested on tip of tree (with b/104545 patch) - works. > > > > JFYI: On tip of tree leak finder reports "No leaks found" for test page. > > Hmm? This sounds bad, sounds like it does *not* work for tip of tree. Has the > heap snapshot format changed again, or what might this be about? > > What's b/104545 patch? ... I had a quick look at this; seems that jsleakcheck breaking on tip of tree is not caused by any remote inspector protocol changes or such. So feel free to go on with this change. Thanks for the heads-up!
Ping! Shouldn't this get landed now that the WebKit side changes were landed, too? (This shouldn't break anything even if this CL goes in Chrome before the WebKit changes, since this works with the old and the new version; right?)
There is some set of revisions where automation will not work - all revisions of Webkit version 537.27 before patch that changed API. On tip of tree this should work well. On 2013/01/18 09:30:02, marja wrote: > Ping! Shouldn't this get landed now that the WebKit side changes were landed, > too? > > (This shouldn't break anything even if this CL goes in Chrome before the WebKit > changes, since this works with the old and the new version; right?)
But this CL is still needed for unbreaking remote_inspector_client.py, right? What are your plans for landing this? (Am I missing something?)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eustas@chromium.org/11615021/20001
Message was sent while issue was closed.
Change committed as 177677 |
