Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2609843002:
open multple tabs from CLI (Closed)
Descriptionopen multple tabs from CLI
BUG=626873
TEST=headless_shell --remote-debugging-port=9222 https://www.google.com https://www.chromium.org https://cs.chromium.org
Review-Url: https://codereview.chromium.org/2609843002
Cr-Commit-Position: refs/heads/master@{#442899}
Committed: https://chromium.googlesource.com/chromium/src/+/14851efa455daad231261a89b13a8e588ff6ad5b
Patch Set 1 #Patch Set 2 : add one more space #Patch Set 3 : only add observer for the first url #Patch Set 4 : only store one web_contents #Patch Set 5 : revert one change #Patch Set 6 : add flag check #
Total comments: 12
Patch Set 7 : check the commandline in HeadlessShellMain #Patch Set 8 : minor fix #Patch Set 9 : minor fix #
Total comments: 6
Patch Set 10 : remove brackets #Messages
Total messages: 29 (15 generated)
The CQ bit was checked by jzfeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== open multple tabs from CLI BUG=626873 ========== to ========== open multple tabs from CLI BUG=626873 TEST=headless_shell --remote-debugging-port=9222 https://www.google.com https://www.chromium.org https://cs.chromium.org ==========
jzfeng@chromium.org changed reviewers: + alexclarke@chromium.org, skyostil@chromium.org
The CQ bit was checked by jzfeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the patch! Looks like you noticed some of the shell's features don't really make sense when you have multiple tabs open. I'm thinking we should add code to check if you're trying to, say, take a screenshot while also opening multiple tabs and bail out with an error message. Let's do this for all the necessary flags in headless/switches.cc? (Taking multiple screenshots seems like a good feature in itself but we can leave a TODO for that for now.)
skyostil@chromium.org changed reviewers: + eseckler@chromium.org
On 2017/01/09 16:43:03, Sami wrote: Added the checking logic. PTAL. I have a related question: Are the shell's features (dump dom, capture screenshot etc.) already used by some actual users, or they are a demonstration of how to use the HeadlessDevToolsClient API? Thanks! Jianzhou
Thanks, Jianzhou! On 2017/01/10 12:01:17, jzfeng wrote: > I have a related question: Are the shell's features (dump dom, capture > screenshot etc.) already used by some actual users, or they are a demonstration > of how to use the HeadlessDevToolsClient API? There are quite a few people that use headless_shell to take screenshots and capture dom etc. Have a look on the headless-dev mailing list archive if you're interested :) I'm not sure how many of them use the shell's command line options for those features, though, versus using --remote-debugging-port and sending the DevTools commands themselves. The latter is probably the more useful thing if you're not just playing with the shell. In that sense - yes, those command-line flags are primarily for demo purposes at the moment :) https://codereview.chromium.org/2609843002/diff/100001/headless/app/headless_... File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2609843002/diff/100001/headless/app/headless_... headless/app/headless_shell.cc:107: if (!MultiTabsCheckPassed(args.size())) { nit: Can we move this to HeadlessShellMain() before the browser is created? https://codereview.chromium.org/2609843002/diff/100001/headless/app/headless_... headless/app/headless_shell.cc:117: url_ = GURL(*it); Good spot, seems like we didn't set url_ correctly before! Small nit: This is a trade-off between performance/LOC and readability, but I'd avoid overriding url_ and web_contents_ during iteration. It makes it a little less intuitive what actually ends up in these fields. As an alternative, I'd suggest to set them in the first iteration, and that way also get rid of the reverse iteration, e.g. for (const string& arg : args) { GURL url(arg); HeadlessWebContents* web_contents = builder.SetInitialURL(url).Build(); if (!web_contents) { [...] } if (!web_contents_) { // TODO(jzfeng): Support observing multiple targets. url_ = url; web_contents_ = web_contents; web_contents_->AddObserver(this); } } It's a personal choice, though, so you're free to keep what you have. https://codereview.chromium.org/2609843002/diff/100001/headless/app/headless_... headless/app/headless_shell.cc:125: web_contents_->AddObserver(this); I think this is a rather temporary solution, since it means that we detect only crashes from target #1, and if we detect one, we kill the others as well. Maybe we can add a TODO here to support observing multiple targets :) https://codereview.chromium.org/2609843002/diff/100001/headless/app/headless_... headless/app/headless_shell.cc:402: if (command_line.HasSwitch(switches::kDumpDom)) { Actually, I think at the moment, we kinda require remote debugging to be enabled for multiple tabs to make any sense. Otherwise, we immediately shutdown during OnPageReady() of the first tab. The flags you're checking for here don't work when ::switches::kRemoteDebuggingPort is set, irrespective of multi-tab or single-tab. It would probably make more sense to check for these conditions. i.e. if multi-tab, require kRemoteDebuggingPort and, more generally, if kRemoteDebuggingPort, then disallow all these switches. Does that make sense? :) Btw, feel free to additionally check for switches::kVirtualTimeBudget and switches::kTimeout, since they're not supported with remote debugging / multi tabs either.
I think we're still figuring out exactly what role Headless Shell will fill in the future. It started off as a demonstrator of headless API features, but I'm now wondering if we should migrate all of that to, e.g., a devtools-based NPM package. For now, let's make sure the currently supported features keep working. https://codereview.chromium.org/2609843002/diff/100001/headless/app/headless_... File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2609843002/diff/100001/headless/app/headless_... headless/app/headless_shell.cc:396: bool MultiTabsCheckPassed(const int tab_number) const { nit: I'm not sure what the method name is saying. How about something like ValidateSwitchesForMultipleTabs (and then only call this when we have more than one tab)? nit: we generally don't use const for arguments like this. (I notice there's one instance just above this but that slipped past review :) https://codereview.chromium.org/2609843002/diff/100001/headless/app/headless_... headless/app/headless_shell.cc:403: LOG(ERROR) << "Dump dom is not supported for multiple tabs."; nit: s/dom/DOM/
On 2017/01/11 11:32:37, Sami wrote: > I think we're still figuring out exactly what role Headless Shell will fill in > the future. It started off as a demonstrator of headless API features, but I'm > now wondering if we should migrate all of that to, e.g., a devtools-based NPM > package. For now, let's make sure the currently supported features keep working. > > https://codereview.chromium.org/2609843002/diff/100001/headless/app/headless_... > File headless/app/headless_shell.cc (right): > > https://codereview.chromium.org/2609843002/diff/100001/headless/app/headless_... > headless/app/headless_shell.cc:396: bool MultiTabsCheckPassed(const int > tab_number) const { > nit: I'm not sure what the method name is saying. How about something like > ValidateSwitchesForMultipleTabs (and then only call this when we have more than > one tab)? > > nit: we generally don't use const for arguments like this. (I notice there's one > instance just above this but that slipped past review :) > > https://codereview.chromium.org/2609843002/diff/100001/headless/app/headless_... > headless/app/headless_shell.cc:403: LOG(ERROR) << "Dump dom is not supported for > multiple tabs."; > nit: s/dom/DOM/ Hi all, Thanks for the explanation and comments! I modified the code accordingly. I may misunderstood something, since I'm still new to the project :) PTAL.
https://codereview.chromium.org/2609843002/diff/100001/headless/app/headless_... File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2609843002/diff/100001/headless/app/headless_... headless/app/headless_shell.cc:107: if (!MultiTabsCheckPassed(args.size())) { On 2017/01/10 17:42:30, Eric Seckler wrote: > nit: Can we move this to HeadlessShellMain() before the browser is created? Good point. Done. https://codereview.chromium.org/2609843002/diff/100001/headless/app/headless_... headless/app/headless_shell.cc:117: url_ = GURL(*it); On 2017/01/10 17:42:30, Eric Seckler wrote: > Good spot, seems like we didn't set url_ correctly before! > > Small nit: This is a trade-off between performance/LOC and readability, but I'd > avoid overriding url_ and web_contents_ during iteration. It makes it a little > less intuitive what actually ends up in these fields. As an alternative, I'd > suggest to set them in the first iteration, and that way also get rid of the > reverse iteration, e.g. > > for (const string& arg : args) { > GURL url(arg); > HeadlessWebContents* web_contents = builder.SetInitialURL(url).Build(); > if (!web_contents) { > [...] > } > > if (!web_contents_) { > // TODO(jzfeng): Support observing multiple targets. > url_ = url; > web_contents_ = web_contents; > web_contents_->AddObserver(this); > } > } > > It's a personal choice, though, so you're free to keep what you have. By experiment, I found the tab order on the Inspectable WebContents page is the reverse of the order of builder.SetInitialURL(url).Build(). That is the main reason I'm using the reverse iteration. So I keep the reverse iteration and modified the code as suggested. https://codereview.chromium.org/2609843002/diff/100001/headless/app/headless_... headless/app/headless_shell.cc:396: bool MultiTabsCheckPassed(const int tab_number) const { On 2017/01/11 11:32:37, Sami wrote: > nit: I'm not sure what the method name is saying. How about something like > ValidateSwitchesForMultipleTabs (and then only call this when we have more than > one tab)? > > nit: we generally don't use const for arguments like this. (I notice there's one > instance just above this but that slipped past review :) I moved this function to HeadlessShellMain and change the name to be ValidateCommandLine, hopefully it is easier to understand. I'm new to c++ code style, could you explain a bit, under which case, we don't use const? Thanks! https://codereview.chromium.org/2609843002/diff/100001/headless/app/headless_... headless/app/headless_shell.cc:402: if (command_line.HasSwitch(switches::kDumpDom)) { On 2017/01/10 17:42:30, Eric Seckler wrote: > Actually, I think at the moment, we kinda require remote debugging to be enabled > for multiple tabs to make any sense. Otherwise, we immediately shutdown during > OnPageReady() of the first tab. > > The flags you're checking for here don't work when > ::switches::kRemoteDebuggingPort is set, irrespective of multi-tab or > single-tab. > > It would probably make more sense to check for these conditions. i.e. if > multi-tab, require kRemoteDebuggingPort and, more generally, if > kRemoteDebuggingPort, then disallow all these switches. Does that make sense? :) > > Btw, feel free to additionally check for switches::kVirtualTimeBudget and > switches::kTimeout, since they're not supported with remote debugging / multi > tabs either. Done. https://codereview.chromium.org/2609843002/diff/100001/headless/app/headless_... headless/app/headless_shell.cc:403: LOG(ERROR) << "Dump dom is not supported for multiple tabs."; On 2017/01/11 11:32:37, Sami wrote: > nit: s/dom/DOM/ Done.
https://codereview.chromium.org/2609843002/diff/100001/headless/app/headless_... File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2609843002/diff/100001/headless/app/headless_... headless/app/headless_shell.cc:396: bool MultiTabsCheckPassed(const int tab_number) const { On 2017/01/11 12:00:27, jzfeng wrote: > On 2017/01/11 11:32:37, Sami wrote: > > nit: I'm not sure what the method name is saying. How about something like > > ValidateSwitchesForMultipleTabs (and then only call this when we have more > than > > one tab)? > > > > nit: we generally don't use const for arguments like this. (I notice there's > one > > instance just above this but that slipped past review :) > > I moved this function to HeadlessShellMain and change the name to be > ValidateCommandLine, hopefully it is easier to understand. > > I'm new to c++ code style, could you explain a bit, under which case, we don't > use const? Thanks! One rule of thumb, any type of integer or floating point number shouldn't be const. Const is mostly useful for pointers and reference.
Thanks! https://codereview.chromium.org/2609843002/diff/160001/headless/app/headless_... File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2609843002/diff/160001/headless/app/headless_... headless/app/headless_shell.cc:411: if (command_line.GetArgs().size() <= 1) Looks like this should be before the previous if statement?
lgtm https://codereview.chromium.org/2609843002/diff/160001/headless/app/headless_... File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2609843002/diff/160001/headless/app/headless_... headless/app/headless_shell.cc:411: if (command_line.GetArgs().size() <= 1) On 2017/01/11 13:14:28, Sami wrote: > Looks like this should be before the previous if statement? Oops, I misread, ignore me :)
lgtm with two very tiny nits as well! :) https://codereview.chromium.org/2609843002/diff/160001/headless/app/headless_... File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2609843002/diff/160001/headless/app/headless_... headless/app/headless_shell.cc:109: if (args.empty()) { nit: get rid of brackets, i.e. if (args.empty()) args.push_back("about:blank"); (We don't use brackets if the body & condition are both only a single line.) https://codereview.chromium.org/2609843002/diff/160001/headless/app/headless_... headless/app/headless_shell.cc:451: if (!ValidateCommandLine(command_line)) { nit: same here (no brackets).
https://codereview.chromium.org/2609843002/diff/160001/headless/app/headless_... File headless/app/headless_shell.cc (right): https://codereview.chromium.org/2609843002/diff/160001/headless/app/headless_... headless/app/headless_shell.cc:109: if (args.empty()) { On 2017/01/11 13:39:20, Eric Seckler wrote: > nit: get rid of brackets, i.e. > > if (args.empty()) > args.push_back("about:blank"); > > (We don't use brackets if the body & condition are both only a single line.) Thanks for the tip! Done. https://codereview.chromium.org/2609843002/diff/160001/headless/app/headless_... headless/app/headless_shell.cc:451: if (!ValidateCommandLine(command_line)) { On 2017/01/11 13:39:20, Eric Seckler wrote: > nit: same here (no brackets). Done.
The CQ bit was checked by jzfeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, eseckler@chromium.org Link to the patchset: https://codereview.chromium.org/2609843002/#ps180001 (title: "remove brackets")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1484143066996470,
"parent_rev": "dffd09b3dc86444afa5b91f90a1267bc7d133839", "commit_rev":
"14851efa455daad231261a89b13a8e588ff6ad5b"}
Message was sent while issue was closed.
Description was changed from ========== open multple tabs from CLI BUG=626873 TEST=headless_shell --remote-debugging-port=9222 https://www.google.com https://www.chromium.org https://cs.chromium.org ========== to ========== open multple tabs from CLI BUG=626873 TEST=headless_shell --remote-debugging-port=9222 https://www.google.com https://www.chromium.org https://cs.chromium.org Review-Url: https://codereview.chromium.org/2609843002 Cr-Commit-Position: refs/heads/master@{#442899} Committed: https://chromium.googlesource.com/chromium/src/+/14851efa455daad231261a89b13a... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/14851efa455daad231261a89b13a... |
