Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(5)

Issue 2743013002: Add webdriver endpoint to send custom debugger commands (Closed)

Created:
3 years, 9 months ago by em
Modified:
3 years, 7 months ago
Reviewers:
johnchen, stgao
CC:
chromium-reviews, samuong+watch_chromium.org, stgao, samuong
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Add webdriver endpoint to send custom debugger commands Since its introduction in Canary I became a big fan of the CSS rule usage tracker and I'm looking forward to its introduction in Chrome stable. I also thought it would be a great idea to run it periodically as part of the e2e tests at work to collect the (un)used CSS rules instead of during manual testing. I then decided to implement a POC [1] to run the CSS tracker with Protractor. This of course required some changes in Chromedriver, Protractor and WebDriver JS Extender. In particular, I had to change the Chromedriver so that I could send the commands to start and stop the CSS rule usage tracking (CSS.startRuleUsageTracking/CSS.stopRuleUsageTracking) to the remote debugger. I thought of 3 possible implementations: 1) as part of the performance logging [2]; 2) as a new endpoint of the Chromedriver to start/stop the CSS rule usage tracking; 3) as a new endpoint of the Chromedriver to send any command directly to the debugger, hence opening up the road to a whole series of tools able to send commands directly to the debugger through the Chromedriver. Of these 3 options, I implemented option 2 and 3 in this change. If this change is acceptable, I'll proceed with the tests and fixing the linter issues. Thanks, EM. [1] https://github.com/ventuno/css-usage-recorder/tree/ftr-css-recording [2] https://sites.google.com/a/chromium.org/chromedriver/logging/performance-log BUG= Review-Url: https://codereview.chromium.org/2743013002 Cr-Commit-Position: refs/heads/master@{#469275} Committed: https://chromium.googlesource.com/chromium/src/+/711de0efbb675bd2a4a28ec47c9194d8e498e600

Patch Set 1 #

Patch Set 2 : Adding name to AUTHORS file #

Patch Set 3 : Removing start/stopcssruleusage cmds (et al). #

Patch Set 4 : Remove CssTracker #

Patch Set 5 : Remove CssTracker - contd #

Patch Set 6 : New DevToolsEventsLogger #

Total comments: 4

Patch Set 7 : Addressing initial review comments #

Patch Set 8 : Adding e2e test #

Patch Set 9 : Adding e2e test #

Total comments: 10

Patch Set 10 : Addressing review comments #

Patch Set 11 : Update test file #

Patch Set 12 : Use std::move instead of DeepCopy #

Total comments: 10

Patch Set 13 : Rebasing and applying recommended cosmetic changes #

Patch Set 14 : Fixing indentation #

Total comments: 6

Patch Set 15 : Indentation and style changes #

Total comments: 2

Patch Set 16 : Fix more indentation issues #

Patch Set 17 : Adding stub, quick code reorg #

Unified diffs Side-by-side diffs Delta from patch set Stats (+285 lines, -4 lines) Patch
M AUTHORS View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -0 lines 0 comments Download
M chrome/test/chromedriver/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/test/chromedriver/capabilities.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/test/chromedriver/capabilities.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +25 lines, -0 lines 0 comments Download
M chrome/test/chromedriver/chrome/stub_web_view.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/test/chromedriver/chrome/stub_web_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +12 lines, -0 lines 0 comments Download
M chrome/test/chromedriver/chrome/web_view.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +10 lines, -0 lines 0 comments Download
M chrome/test/chromedriver/chrome/web_view_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/test/chromedriver/chrome/web_view_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +17 lines, -0 lines 0 comments Download
M chrome/test/chromedriver/client/chromedriver.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 4 chunks +16 lines, -2 lines 0 comments Download
M chrome/test/chromedriver/client/command_executor.py View 1 2 3 4 5 6 7 8 9 10 1 chunk +4 lines, -0 lines 0 comments Download
A chrome/test/chromedriver/devtools_events_logger.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +47 lines, -0 lines 0 comments Download
A chrome/test/chromedriver/devtools_events_logger.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +43 lines, -0 lines 0 comments Download
M chrome/test/chromedriver/logging.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M chrome/test/chromedriver/logging.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +8 lines, -0 lines 0 comments Download
M chrome/test/chromedriver/server/http_handler.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +9 lines, -0 lines 0 comments Download
M chrome/test/chromedriver/test/run_py_tests.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 4 chunks +33 lines, -2 lines 0 comments Download
M chrome/test/chromedriver/window_commands.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +12 lines, -0 lines 0 comments Download
M chrome/test/chromedriver/window_commands.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +32 lines, -0 lines 0 comments Download

Messages

Total messages: 80 (24 generated)
em
3 years, 9 months ago (2017-03-10 15:46:21 UTC) #2
em
3 years, 9 months ago (2017-03-10 16:35:05 UTC) #5
em
3 years, 9 months ago (2017-03-14 02:18:15 UTC) #9
samuong
I should probably be the one doing the review for this, so I've removed everyone ...
3 years, 9 months ago (2017-03-14 20:48:11 UTC) #11
em
Hello Sam, thanks for your message. There's no design document or proposal for this, everything ...
3 years, 9 months ago (2017-03-15 01:28:09 UTC) #12
samuong
We've tended to use "chromium" as the vendor prefix, rather than "c". It would be ...
3 years, 9 months ago (2017-03-15 17:34:04 UTC) #13
em
Hello Sam, I updated the code to remove the CSS.start/stopRuleUsageTracker specific commands and kept only ...
3 years, 9 months ago (2017-03-16 06:25:31 UTC) #14
samuong
I'm thinking that it should be more of a general EventTracker or EventLogger, rather than ...
3 years, 9 months ago (2017-03-16 17:52:07 UTC) #15
em
Thanks Sam! It makes complete sense to me this way. I'll create a DevToolsEventsTracker that ...
3 years, 9 months ago (2017-03-17 05:33:10 UTC) #16
em
Sorry for the double message (please take a look at my previous one as well), ...
3 years, 9 months ago (2017-03-17 14:32:33 UTC) #17
samuong
On 2017/03/17 14:32:33, em wrote: > Sorry for the double message (please take a look ...
3 years, 9 months ago (2017-03-20 18:02:48 UTC) #18
samuong
One more thing: please also consider how this will work when there are multiple tabs/windows. ...
3 years, 9 months ago (2017-03-21 18:12:18 UTC) #19
em
Thanks, Sam. I'm currently working on the part that gets the list of events and ...
3 years, 9 months ago (2017-03-23 07:09:36 UTC) #20
samuong
Hmm, if it needs that much work, then it's already quite complicated. My main concern ...
3 years, 9 months ago (2017-03-23 21:12:50 UTC) #21
em
I also have an alternative solution which would reduce the complexity a bit, but we ...
3 years, 9 months ago (2017-03-23 21:59:56 UTC) #22
samuong
I can see that makes things simpler, which is nice, but as I said the ...
3 years, 9 months ago (2017-03-23 22:35:39 UTC) #23
em
Cool, we agree on having the DevToolsEventsLogger being in charge of logging alone and not ...
3 years, 9 months ago (2017-03-23 22:40:35 UTC) #24
em
Here's a summary of the latest changes: * Create a new log type: "devtools"; * ...
3 years, 9 months ago (2017-03-25 05:05:28 UTC) #25
samuong
Generally I think this looks good. Just a few initial comments. https://codereview.chromium.org/2743013002/diff/100001/chrome/test/chromedriver/base_logger.cc File chrome/test/chromedriver/base_logger.cc (right): ...
3 years, 8 months ago (2017-03-27 18:42:10 UTC) #26
em
However, this was the data posted to the server: xsrf_token: c8eebc2ed27a782a94e25521d71622ff add_as_reviewer: 0 message_only: 1 ...
3 years, 8 months ago (2017-03-27 21:20:14 UTC) #27
samuong
The PerformanceLogger is a BrowserWideDevToolsClient, which means that it might be listening to events from ...
3 years, 8 months ago (2017-03-27 21:32:58 UTC) #28
em
I added the webview information based on your earlier comment: > One more thing: please ...
3 years, 8 months ago (2017-03-27 21:39:32 UTC) #29
samuong
The difference is that PerformanceLogger::subscribes_to_browser returns true, which means that it is subscribed to the ...
3 years, 8 months ago (2017-03-27 22:00:32 UTC) #30
em
Makes sense, so: no webview property in the log message returned by the DevToolsEventsLogger. Is ...
3 years, 8 months ago (2017-03-27 22:05:54 UTC) #31
samuong
sounds good to me
3 years, 8 months ago (2017-03-27 22:07:43 UTC) #32
em
Thanks Sam. I applied the changes you recommended: * Removed BaseLogger class; * Updated variable ...
3 years, 8 months ago (2017-03-28 03:41:21 UTC) #33
samuong
It's really important that we have automated tests running on our buildbots so that we ...
3 years, 8 months ago (2017-03-28 23:11:45 UTC) #34
em
Sure thing. I was wondering if you had some sort of e2e testing already in ...
3 years, 8 months ago (2017-03-28 23:26:49 UTC) #35
em
In the latest patch set: * Changed chromedriver.py and command_executor.py to: * support the 'devToolsEventsToLog' ...
3 years, 8 months ago (2017-03-29 04:41:48 UTC) #36
em
Hi Sam, I just wanted to double check if there's any other code change you're ...
3 years, 8 months ago (2017-04-04 02:00:41 UTC) #37
em
Hi Sam, just a gentle ping: is there any update on this?
3 years, 8 months ago (2017-04-19 16:43:49 UTC) #38
em
Hi stgao, this issue has been open for some time and I didn't get any ...
3 years, 8 months ago (2017-04-22 07:05:59 UTC) #39
stgao
johnchen@, would you mind helping review this CL?
3 years, 8 months ago (2017-04-24 17:30:14 UTC) #41
em
Also including ananthak as recommended by samuong.
3 years, 8 months ago (2017-04-24 18:32:20 UTC) #44
johnchen
Hi em, sorry for the delay. I'll review this within a day or two. Thanks.
3 years, 8 months ago (2017-04-24 18:36:09 UTC) #45
johnchen
The overall structure of the code looks fine. I've made a few comments about specific ...
3 years, 8 months ago (2017-04-26 05:17:47 UTC) #47
em
Thanks for the review John. I addressed all of your comments. There's just one issue ...
3 years, 7 months ago (2017-04-27 05:02:09 UTC) #48
johnchen
On 2017/04/27 05:02:09, em wrote: > https://codereview.chromium.org/2743013002/diff/160001/chrome/test/chromedriver/chrome/web_view_impl.cc > File chrome/test/chromedriver/chrome/web_view_impl.cc (right): > > https://codereview.chromium.org/2743013002/diff/160001/chrome/test/chromedriver/chrome/web_view_impl.cc#newcode206 > ...
3 years, 7 months ago (2017-04-27 15:33:48 UTC) #49
em
Thanks John, I'm now using std::move instead of DeepCopy as recommended. Please advise on the ...
3 years, 7 months ago (2017-04-28 02:21:18 UTC) #50
johnchen
Hi EM, Thanks for making all the updates. I don't see any more real issues ...
3 years, 7 months ago (2017-04-28 21:59:32 UTC) #51
em
Thanks again John, I rebased the branch (this required a few changes to logging.cc and ...
3 years, 7 months ago (2017-04-29 03:07:03 UTC) #52
johnchen
Ernesto: Thanks for making the changes. LGTM. Shuotao: Could you please take a look?
3 years, 7 months ago (2017-05-01 00:21:01 UTC) #53
johnchen
3 years, 7 months ago (2017-05-01 00:21:23 UTC) #55
stgao
code style lgtm expect those I pointed out. https://codereview.chromium.org/2743013002/diff/260001/chrome/test/chromedriver/capabilities.cc File chrome/test/chromedriver/capabilities.cc (right): https://codereview.chromium.org/2743013002/diff/260001/chrome/test/chromedriver/capabilities.cc#newcode404 chrome/test/chromedriver/capabilities.cc:404: Capabilities* ...
3 years, 7 months ago (2017-05-03 20:24:44 UTC) #56
em
Thanks a lot stgao/johnchen. I made all the requested changes. Additionally I adjusted some of ...
3 years, 7 months ago (2017-05-04 01:11:22 UTC) #57
johnchen
Ernesto: Thanks for making the changes. LGTM except for one formatting issue. Please fix that ...
3 years, 7 months ago (2017-05-04 01:50:23 UTC) #58
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2743013002/300001
3 years, 7 months ago (2017-05-04 02:01:06 UTC) #61
em
Thanks jonhchen :-). https://codereview.chromium.org/2743013002/diff/280001/chrome/test/chromedriver/logging.cc File chrome/test/chromedriver/logging.cc (right): https://codereview.chromium.org/2743013002/diff/280001/chrome/test/chromedriver/logging.cc#newcode312 chrome/test/chromedriver/logging.cc:312: base::MakeUnique<DevToolsEventsLogger>( On 2017/05/04 01:50:22, johnchen wrote: ...
3 years, 7 months ago (2017-05-04 02:01:32 UTC) #62
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_tsan_rel_ng/builds/66638)
3 years, 7 months ago (2017-05-04 02:12:44 UTC) #64
em
jonhchen/stgao, tests were failing, apparently because I didn't stub the SendCommand/SendCommandAndGetResults method in the StubWebView ...
3 years, 7 months ago (2017-05-04 02:43:52 UTC) #65
johnchen
LGTM. Please retry the commit when you're ready. (For future reference, it's generally a good ...
3 years, 7 months ago (2017-05-04 04:00:21 UTC) #68
em
Thanks John. I'm currently running a dry run: everything is green so far. One more ...
3 years, 7 months ago (2017-05-04 04:08:00 UTC) #69
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2743013002/320001
3 years, 7 months ago (2017-05-04 05:20:18 UTC) #74
commit-bot: I haz the power
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/chromium/src/+/711de0efbb675bd2a4a28ec47c9194d8e498e600
3 years, 7 months ago (2017-05-04 05:27:10 UTC) #78
johnchen
Congratulations! Your changes have been committed into the official Chromium Git repository. Here are answers ...
3 years, 7 months ago (2017-05-04 06:13:32 UTC) #79
em
3 years, 7 months ago (2017-05-04 18:04:00 UTC) #80
Message was sent while issue was closed.
Thanks John!

As far as the documentation goes, we should add a new row in the chromeOptions
table [1]:

====
Name: devToolsEventsToLog;
Type: list of strings;
Default: N/A;
Description: An optional list of DevTools events to log. In order for this to
work the "devtools" log type should be enabled in the loggingPrefs [2] object.
The list of possible events to log is available in the Chrome DevTools Protocol
API Docs [3]. In order for the events to be logged the corresponding domain
should be enabled as well. You can make sure a domain is enabled by sending a
custom command to the ChromeDriver using the
"/session/:sessionId/chromium/send_command" endpoint. For example, to log the
"CSS.styleSheetAdded" event, the "CSS" domain needs to be enabled by sending the
"CSS.enable" command to the ChromeDriver.
====

[1]
https://sites.google.com/a/chromium.org/chromedriver/capabilities#TOC-chromeO...
[2]
https://sites.google.com/a/chromium.org/chromedriver/capabilities#TOC-logging...
[3] https://chromedevtools.github.io/devtools-protocol/

Powered by Google App Engine
This is Rietveld 408576698