[headless] AddWindowTreeClient

[headless] Fix crashing during window.open.

* Add WindowTreeClient.
* Register new WebContents in Embedder API.

BUG=603882
R=skyostil@chromium.org

Committed: https://crrev.com/b6d2d1fcb63abb91d74feb9e0de731e39d83ce55
Cr-Commit-Position: refs/heads/master@{#388775}

[altimin, 2016-04-15 15:22:06]

Description was changed from

==========
[headless] AddWindowTreeClient

BUG=603882
R=skyostil
==========

to

==========
[headless] AddWindowTreeClient

BUG=603882
R=skyostil
==========

[altimin, 2016-04-15 15:22:30]

Description was changed from

==========
[headless] AddWindowTreeClient

BUG=603882
R=skyostil
==========

to

==========
[headless] AddWindowTreeClient

BUG=603882
R=skyostil@chromium.org
==========

[altimin, 2016-04-15 15:22:30]

altimin@chromium.org changed reviewers:
+ skyostil@chromium.org
- skyostil@google.com

[Sami, 2016-04-15 15:45:52]

Code looks great. Would you mind adding a test which does a window.open to make
sure this keeps working?

[altimin, 2016-04-20 16:03:02]

On 2016/04/15 15:45:52, Sami wrote:
> Code looks great. Would you mind adding a test which does a window.open to
make
> sure this keeps working?

PTAL

[Sami, 2016-04-20 16:38:50]

Thanks for the tests. Few miscellaneous comments.

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_impl.h (right):

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_browser_impl.h:73:
std::unordered_map<HeadlessWebContents*, std::unique_ptr<HeadlessWebContents>>
Looks like this could just be an unordered_set<>?

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/browser/he...
File headless/lib/browser/headless_web_contents_impl.h (right):

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_web_contents_impl.h:41: static
std::unique_ptr<HeadlessWebContentsImpl> FromWebContents(
Maybe call this CreateFromWebContents() since this is a constructor too.

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_web_contents_impl.h:69: HeadlessBrowserImpl*
browser_;
// Not owned.

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/browser/he...
File headless/lib/browser/headless_window_tree_client.h (right):

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_window_tree_client.h:23: aura::Window*
root_window_;
// Not owned.

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/headless_b...
File headless/lib/headless_browser_browsertest.cc (right):

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/headless_b...
headless/lib/headless_browser_browsertest.cc:22:
EXPECT_EQ(browser()->GetAllWebContents().size(), static_cast<size_t>(1));
EXPECT_EQ(expected, actual)  (here and elsewhere)

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/headless_b...
headless/lib/headless_browser_browsertest.cc:70: // TODO(headless-dev):
Currently this construction does not serve hello.html
TODO(altimin) since this mailing list style isn't really widely used (doesn't
mean you actually have to fix this :)

https://codereview.chromium.org/1891843004/diff/20001/headless/public/headles...
File headless/public/headless_browser.h (right):

https://codereview.chromium.org/1891843004/diff/20001/headless/public/headles...
headless/public/headless_browser.h:39: // physical pixels.
Could you mention that the browser owns the returned object and that the client
can call Close() to dispose of it.

https://codereview.chromium.org/1891843004/diff/20001/headless/public/headles...
File headless/public/headless_web_contents.h (right):

https://codereview.chromium.org/1891843004/diff/20001/headless/public/headles...
headless/public/headless_web_contents.h:49: virtual bool IsDirectlyCreated()
const = 0;
I would suggest leaving this out, since the client can easily keep track of
which WebContents they created themselves.

[Sami, 2016-04-20 16:38:50]

Thanks for the tests. Few miscellaneous comments.

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_impl.h (right):

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_browser_impl.h:73:
std::unordered_map<HeadlessWebContents*, std::unique_ptr<HeadlessWebContents>>
Looks like this could just be an unordered_set<>?

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/browser/he...
File headless/lib/browser/headless_web_contents_impl.h (right):

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_web_contents_impl.h:41: static
std::unique_ptr<HeadlessWebContentsImpl> FromWebContents(
Maybe call this CreateFromWebContents() since this is a constructor too.

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_web_contents_impl.h:69: HeadlessBrowserImpl*
browser_;
// Not owned.

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/browser/he...
File headless/lib/browser/headless_window_tree_client.h (right):

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_window_tree_client.h:23: aura::Window*
root_window_;
// Not owned.

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/headless_b...
File headless/lib/headless_browser_browsertest.cc (right):

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/headless_b...
headless/lib/headless_browser_browsertest.cc:22:
EXPECT_EQ(browser()->GetAllWebContents().size(), static_cast<size_t>(1));
EXPECT_EQ(expected, actual)  (here and elsewhere)

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/headless_b...
headless/lib/headless_browser_browsertest.cc:70: // TODO(headless-dev):
Currently this construction does not serve hello.html
TODO(altimin) since this mailing list style isn't really widely used (doesn't
mean you actually have to fix this :)

https://codereview.chromium.org/1891843004/diff/20001/headless/public/headles...
File headless/public/headless_browser.h (right):

https://codereview.chromium.org/1891843004/diff/20001/headless/public/headles...
headless/public/headless_browser.h:39: // physical pixels.
Could you mention that the browser owns the returned object and that the client
can call Close() to dispose of it.

https://codereview.chromium.org/1891843004/diff/20001/headless/public/headles...
File headless/public/headless_web_contents.h (right):

https://codereview.chromium.org/1891843004/diff/20001/headless/public/headles...
headless/public/headless_web_contents.h:49: virtual bool IsDirectlyCreated()
const = 0;
I would suggest leaving this out, since the client can easily keep track of
which WebContents they created themselves.

[altimin, 2016-04-20 17:35:26]

PTAL

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_impl.h (right):

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_browser_impl.h:73:
std::unordered_map<HeadlessWebContents*, std::unique_ptr<HeadlessWebContents>>
On 2016/04/20 16:38:50, Sami wrote:
> Looks like this could just be an unordered_set<>?

The problem here is that you can't really use find() with move-only types in STL
containers.

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/browser/he...
File headless/lib/browser/headless_web_contents_impl.h (right):

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_web_contents_impl.h:41: static
std::unique_ptr<HeadlessWebContentsImpl> FromWebContents(
On 2016/04/20 16:38:50, Sami wrote:
> Maybe call this CreateFromWebContents() since this is a constructor too.

Done.

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_web_contents_impl.h:69: HeadlessBrowserImpl*
browser_;
On 2016/04/20 16:38:50, Sami wrote:
> // Not owned.

Done.

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/browser/he...
File headless/lib/browser/headless_window_tree_client.h (right):

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_window_tree_client.h:23: aura::Window*
root_window_;
On 2016/04/20 16:38:50, Sami wrote:
> // Not owned.

Done.

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/headless_b...
File headless/lib/headless_browser_browsertest.cc (right):

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/headless_b...
headless/lib/headless_browser_browsertest.cc:22:
EXPECT_EQ(browser()->GetAllWebContents().size(), static_cast<size_t>(1));
On 2016/04/20 16:38:50, Sami wrote:
> EXPECT_EQ(expected, actual)  (here and elsewhere)

Done.

https://codereview.chromium.org/1891843004/diff/20001/headless/lib/headless_b...
headless/lib/headless_browser_browsertest.cc:70: // TODO(headless-dev):
Currently this construction does not serve hello.html
On 2016/04/20 16:38:50, Sami wrote:
> TODO(altimin) since this mailing list style isn't really widely used (doesn't
> mean you actually have to fix this :)

Done.

https://codereview.chromium.org/1891843004/diff/20001/headless/public/headles...
File headless/public/headless_browser.h (right):

https://codereview.chromium.org/1891843004/diff/20001/headless/public/headles...
headless/public/headless_browser.h:39: // physical pixels.
On 2016/04/20 16:38:50, Sami wrote:
> Could you mention that the browser owns the returned object and that the
client
> can call Close() to dispose of it.

Done.

https://codereview.chromium.org/1891843004/diff/20001/headless/public/headles...
File headless/public/headless_web_contents.h (right):

https://codereview.chromium.org/1891843004/diff/20001/headless/public/headles...
headless/public/headless_web_contents.h:49: virtual bool IsDirectlyCreated()
const = 0;
On 2016/04/20 16:38:50, Sami wrote:
> I would suggest leaving this out, since the client can easily keep track of
> which WebContents they created themselves.

Done.

[Sami, 2016-04-20 17:42:57]

Thanks, lgtm with one comment. Also, please mention in the change description
how this changes the ownership model for WebContents.

https://codereview.chromium.org/1891843004/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_impl.cc (left):

https://codereview.chromium.org/1891843004/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_browser_impl.cc:40: // We require the user to pass
in an initial URL to ensure that the renderer
Any reason to remove this comment? I think it's still useful, although
s/WebContentsReady/DevToolsTargetReady/ :)

[altimin, 2016-04-20 18:11:57]

FYI

https://codereview.chromium.org/1891843004/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_impl.cc (left):

https://codereview.chromium.org/1891843004/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_browser_impl.cc:40: // We require the user to pass
in an initial URL to ensure that the renderer
On 2016/04/20 17:42:57, Sami wrote:
> Any reason to remove this comment? I think it's still useful, although
> s/WebContentsReady/DevToolsTargetReady/ :)

Done.

[altimin, 2016-04-20 18:12:29]

The CQ bit was checked by altimin@chromium.org

[altimin, 2016-04-20 18:12:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/1891843004/#ps80001
(title: "Nits")

[commit-bot: I haz the power, 2016-04-20 18:12:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1891843004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1891843004/80001

[commit-bot: I haz the power, 2016-04-20 18:15:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-20 18:15:28]

Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sami, 2016-04-21 09:42:50]

(Please update the patch description before landing too)

[altimin, 2016-04-21 14:25:20]

Description was changed from

==========
[headless] AddWindowTreeClient

BUG=603882
R=skyostil@chromium.org
==========

to

==========
[headless] Fix crashing during window.open.

* Add WindowTreeClient.
* Register new WebContents in Embedder API.

BUG=603882
R=skyostil@chromium.org
==========

[altimin, 2016-04-21 14:25:29]

The CQ bit was checked by altimin@chromium.org

[altimin, 2016-04-21 14:25:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/1891843004/#ps100001
(title: "Rebase to latest master")

[commit-bot: I haz the power, 2016-04-21 14:25:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1891843004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1891843004/100001

[commit-bot: I haz the power, 2016-04-21 14:35:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-21 14:35:02]

Try jobs failed on following builders:
  win_clang on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[altimin, 2016-04-21 14:47:14]

The CQ bit was checked by altimin@chromium.org

[commit-bot: I haz the power, 2016-04-21 14:47:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1891843004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1891843004/100001

[commit-bot: I haz the power, 2016-04-21 15:23:34]

Description was changed from

==========
[headless] Fix crashing during window.open.

* Add WindowTreeClient.
* Register new WebContents in Embedder API.

BUG=603882
R=skyostil@chromium.org
==========

to

==========
[headless] Fix crashing during window.open.

* Add WindowTreeClient.
* Register new WebContents in Embedder API.

BUG=603882
R=skyostil@chromium.org
==========

[commit-bot: I haz the power, 2016-04-21 15:23:35]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-22 19:34:47]

Description was changed from

==========
[headless] Fix crashing during window.open.

* Add WindowTreeClient.
* Register new WebContents in Embedder API.

BUG=603882
R=skyostil@chromium.org
==========

to

==========
[headless] Fix crashing during window.open.

* Add WindowTreeClient.
* Register new WebContents in Embedder API.

BUG=603882
R=skyostil@chromium.org

Committed: https://crrev.com/b6d2d1fcb63abb91d74feb9e0de731e39d83ce55
Cr-Commit-Position: refs/heads/master@{#388775}
==========

[commit-bot: I haz the power, 2016-04-22 19:34:48]

Patchset 6 (id:??) landed as
https://crrev.com/b6d2d1fcb63abb91d74feb9e0de731e39d83ce55
Cr-Commit-Position: refs/heads/master@{#388775}