open multple tabs from CLI

headless/app/headless_shell.cc

Index: headless/app/headless_shell.cc
diff --git a/headless/app/headless_shell.cc b/headless/app/headless_shell.cc
index 530a7c5dc3498e2d3b19d99b12c8ce6da043d47a..38eebb00397df32d5b2bd59eb2c2cf37f6608fc9 100644
--- a/headless/app/headless_shell.cc
+++ b/headless/app/headless_shell.cc
@@ -104,17 +104,24 @@ class HeadlessShell : public HeadlessWebContents::Observer,
     base::CommandLine::StringVector args =
         base::CommandLine::ForCurrentProcess()->GetArgs();
 
-    // TODO(alexclarke): Should we navigate to about:blank first if using
-    // virtual time?
-    if (!args.empty() && !args[0].empty())
-      builder.SetInitialURL(GURL(args[0]));
-
-    web_contents_ = builder.Build();
-    if (!web_contents_) {
-      LOG(ERROR) << "Navigation failed";
+    if (!MultiTabsCheckPassed(args.size())) {

[Eric Seckler, 2017/01/10 17:42:30]

nit: Can we move this to HeadlessShellMain() before the browser is created?

[jzfeng, 2017/01/11 12:00:27]

Good point. Done.

       browser_->Shutdown();
       return;
     }
+    // TODO(alexclarke): Should we navigate to about:blank first if using
+    // virtual time?
+    if (args.empty()) {
+      args.push_back("about:blank");
+    }
+    for (auto it = args.rbegin(); it != args.rend(); ++it) {
+      url_ = GURL(*it);

[Eric Seckler, 2017/01/10 17:42:30]

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.

[jzfeng, 2017/01/11 12:00:27]

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.

+      web_contents_ = builder.SetInitialURL(url_).Build();
+      if (!web_contents_) {
+        LOG(ERROR) << "Navigation to " << url_ << " failed";
+        browser_->Shutdown();
+        return;
+      }
+    }
     web_contents_->AddObserver(this);

[Eric Seckler, 2017/01/10 17:42:30]

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 :)

   }
 
@@ -386,6 +393,28 @@ class HeadlessShell : public HeadlessWebContents::Observer,
     return command_line.HasSwitch(::switches::kRemoteDebuggingPort);
   }
 
+  bool MultiTabsCheckPassed(const int tab_number) const {

[Sami, 2017/01/11 11:32:37]

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 :)

[jzfeng, 2017/01/11 12:00:27]

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!

[alex clarke (OOO till 29th), 2017/01/11 12:03:39]

One rule of thumb, any type of integer or floating point number shouldn't be
const.

Const is mostly useful for pointers and reference.

+    if (tab_number < 2 || RemoteDebuggingEnabled()) {
+      return true;
+    }
+    const base::CommandLine& command_line =
+        *base::CommandLine::ForCurrentProcess();
+    if (command_line.HasSwitch(switches::kDumpDom)) {

[Eric Seckler, 2017/01/10 17:42:30]

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.

[jzfeng, 2017/01/11 12:00:27]

Done. 

+      LOG(ERROR) << "Dump dom is not supported for multiple tabs.";

[Sami, 2017/01/11 11:32:37]

nit: s/dom/DOM/

[jzfeng, 2017/01/11 12:00:27]

Done.

+      return false;
+    }
+    if (command_line.HasSwitch(switches::kRepl)) {
+      LOG(ERROR) << "Evaluate Javascript is not supported for multiple tabs.";
+      return false;
+    }
+    if (command_line.HasSwitch(switches::kScreenshot)) {
+      // TODO(jzfeng): Add support for multiple tabs in the future.
+      LOG(ERROR) << "Capture screenshot is not supported for multiple tabs.";
+      return false;
+    }
+    return true;
+  }
+
  private:
   GURL url_;
   HeadlessBrowser* browser_;  // Not owned.