Initialize BrowserPluginGuest to attach to the View hierarchy

Initialize BrowserPluginGuest to attach to the View hierarchy

This CL makes sure that a BrowserPluginGuest is attached to its owner's View hierarchy on creation.

This allows the guest WebContents to navigate without needing to be attached to a container, as if it was a worker with a DOM.

This CL will also ACK the last CompositorFrameSwapped message from the guest on attachment if it hasn't been ACK already so that the guest continues to produce frames once it's attached to a new container.

BUG=434226
Committed: https://crrev.com/7310a42645ba9f0724a8abe38194cc0064d7e7f9
Cr-Commit-Position: refs/heads/master@{#306983}

[Fady Samuel, 2014-12-03 17:48:16]

fsamuel@chromium.org changed reviewers:
+ lazyboy@chromium.org

[Fady Samuel, 2014-12-04 01:40:15]

PTAL Istiaque. GuestViews don't get attached to the view hierarchy if they don't
have an embedder.

[Fady Samuel, 2014-12-04 01:40:39]

On 2014/12/04 01:40:15, Fady Samuel wrote:
> PTAL Istiaque. GuestViews don't get attached to the view hierarchy if they
don't
> have an embedder.

By default that is, WTFrame will be the exception in a future CL.

[lazyboy, 2014-12-04 16:51:13]

https://chromiumcodereview.appspot.com/769363002/diff/40001/content/browser/b...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/769363002/diff/40001/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.cc:108: if
(!delegate_->CanRunInDetachedState())
Add a todo saying we should guard this with WTFrame flag once we have it.

https://chromiumcodereview.appspot.com/769363002/diff/40001/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.cc:537: if (pending_frame_)
{
Add a not when this can happen.

https://chromiumcodereview.appspot.com/769363002/diff/40001/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.cc:709: int /* unused */,
nit: This is not consistent in this file, revert this change, there are lots of
function that do not care about bpid param and do not specify /* unused */

https://chromiumcodereview.appspot.com/769363002/diff/40001/content/browser/b...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/769363002/diff/40001/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.h:378:
scoped_ptr<FrameMsg_CompositorFrameSwapped_Params> pending_frame_;
Describe why pending frame needs to be stored, i.e. the Attach/Detach scenario.

https://chromiumcodereview.appspot.com/769363002/diff/40001/content/browser/w...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/769363002/diff/40001/content/browser/w...
content/browser/web_contents/web_contents_impl.cc:1200: if
(browser_plugin_guest_)
You need to run Init() after |view_->CreateView()| has been called right? Can
you add comment about that here? If not then we could have pulled this up where
|view_| gets created.

https://chromiumcodereview.appspot.com/769363002/diff/40001/content/public/br...
File content/public/browser/browser_plugin_guest_delegate.h (right):

https://chromiumcodereview.appspot.com/769363002/diff/40001/content/public/br...
content/public/browser/browser_plugin_guest_delegate.h:41: virtual bool
CanRunInDetachedState() const;
Explain "detached" a bit more here.

[Fady Samuel, 2014-12-04 20:54:44]

PTAL.

https://codereview.chromium.org/769363002/diff/40001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/769363002/diff/40001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:108: if
(!delegate_->CanRunInDetachedState())
On 2014/12/04 16:51:13, lazyboy wrote:
> Add a todo saying we should guard this with WTFrame flag once we have it.

Done.

https://codereview.chromium.org/769363002/diff/40001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:537: if (pending_frame_)
{
On 2014/12/04 16:51:12, lazyboy wrote:
> Add a not when this can happen.

Done.

https://codereview.chromium.org/769363002/diff/40001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:709: int /* unused */,
On 2014/12/04 16:51:13, lazyboy wrote:
> nit: This is not consistent in this file, revert this change, there are lots
of
> function that do not care about bpid param and do not specify /* unused */

Done.

https://codereview.chromium.org/769363002/diff/40001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/769363002/diff/40001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.h:378:
scoped_ptr<FrameMsg_CompositorFrameSwapped_Params> pending_frame_;
On 2014/12/04 16:51:13, lazyboy wrote:
> Describe why pending frame needs to be stored, i.e. the Attach/Detach
scenario.

Done.

https://codereview.chromium.org/769363002/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/769363002/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1200: if
(browser_plugin_guest_)
On 2014/12/04 16:51:13, lazyboy wrote:
> You need to run Init() after |view_->CreateView()| has been called right? Can
> you add comment about that here? If not then we could have pulled this up
where
> |view_| gets created.

Done.

https://codereview.chromium.org/769363002/diff/40001/content/public/browser/b...
File content/public/browser/browser_plugin_guest_delegate.h (right):

https://codereview.chromium.org/769363002/diff/40001/content/public/browser/b...
content/public/browser/browser_plugin_guest_delegate.h:41: virtual bool
CanRunInDetachedState() const;
On 2014/12/04 16:51:13, lazyboy wrote:
> Explain "detached" a bit more here.

Done.

[lazyboy, 2014-12-04 20:58:46]

lgtm

https://codereview.chromium.org/769363002/diff/60001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/769363002/diff/60001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:216: OnSetFocus(0,
focused_);
s/0/browser_plugin::kInstanceIDNone

[Fady Samuel, 2014-12-04 21:30:08]

fsamuel@chromium.org changed reviewers:
+ creis@chromium.org

[Fady Samuel, 2014-12-04 21:30:08]

Thanks Istiaque!

Charlie, could I please get content/ OWNER review?

[Charlie Reis, 2014-12-04 22:16:52]

Just a few questions and nits below.

Since this is non-trivial, I think it'd be worth having a test for this if we
can, showing how a detached guest is supposed to behave.

https://codereview.chromium.org/769363002/diff/80001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/769363002/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:103: void
BrowserPluginGuest::Init() {
Style nit: Should generally have the same order of functions in .h and .cc.  No
need to change existing code, but good to follow for new code.

https://codereview.chromium.org/769363002/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:108: // TODO(fsamuel):
This should be behind a command line flag once we introduce
Can elaborate which part of "this" you're referring to?  I'm not sure I
understand what's possible now and what will be possible soon.

https://codereview.chromium.org/769363002/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:261: // The instance ID
does not matter here.
Can the parameter be removed from OnResizeGuest?  Otherwise please explain a bit
further.

https://codereview.chromium.org/769363002/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:344:
pending_frame_.reset(new FrameMsg_CompositorFrameSwapped_Params());
Maybe this is better named last_pending_frame_?

https://codereview.chromium.org/769363002/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:539: // If a guest is in
the process of detaching from one container and attaching
nit: Remove "in the process of"

https://codereview.chromium.org/769363002/diff/80001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/769363002/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.h:49: struct
FrameMsg_CompositorFrameSwapped_Params;
Style nit: I think the ifdef'd structs are supposed to come in a separate block,
rather than alphabetized in the main block.  See the "Exception" note in:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Ord...

https://codereview.chromium.org/769363002/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.h:130: void Init();
Perhaps this should be closer to Create?  Can you add a comment about when it is
expected to be called?

[Fady Samuel, 2014-12-05 00:56:30]

There are no consumers of this new functionality upstream yet. Once the Worker
Thread Frame prototype is ready to upstream, I'll add tests specific for it.
It'll be the only GuestView type that can operate without a container. The
intent is it can function as a shared worker with a DOM that can be attached to
a container where it can composite its DOM onto its embedder.

https://codereview.chromium.org/769363002/diff/80001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/769363002/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:103: void
BrowserPluginGuest::Init() {
On 2014/12/04 22:16:52, Charlie Reis wrote:
> Style nit: Should generally have the same order of functions in .h and .cc. 
No
> need to change existing code, but good to follow for new code.

Done.

https://codereview.chromium.org/769363002/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:108: // TODO(fsamuel):
This should be behind a command line flag once we introduce
On 2014/12/04 22:16:52, Charlie Reis wrote:
> Can elaborate which part of "this" you're referring to?  I'm not sure I
> understand what's possible now and what will be possible soon.

Done.

https://codereview.chromium.org/769363002/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:261: // The instance ID
does not matter here.
On 2014/12/04 22:16:52, Charlie Reis wrote:
> Can the parameter be removed from OnResizeGuest?  Otherwise please explain a
bit
> further.

Done.

https://codereview.chromium.org/769363002/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:344:
pending_frame_.reset(new FrameMsg_CompositorFrameSwapped_Params());
On 2014/12/04 22:16:52, Charlie Reis wrote:
> Maybe this is better named last_pending_frame_?

Done.

https://codereview.chromium.org/769363002/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:539: // If a guest is in
the process of detaching from one container and attaching
On 2014/12/04 22:16:52, Charlie Reis wrote:
> nit: Remove "in the process of"

Done.

https://codereview.chromium.org/769363002/diff/80001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/769363002/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.h:49: struct
FrameMsg_CompositorFrameSwapped_Params;
On 2014/12/04 22:16:52, Charlie Reis wrote:
> Style nit: I think the ifdef'd structs are supposed to come in a separate
block,
> rather than alphabetized in the main block.  See the "Exception" note in:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Ord...

Done.

https://codereview.chromium.org/769363002/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.h:130: void Init();
On 2014/12/04 22:16:52, Charlie Reis wrote:
> Perhaps this should be closer to Create?  Can you add a comment about when it
is
> expected to be called?

I moved it right below the constructor.

[Charlie Reis, 2014-12-05 02:08:29]

On 2014/12/05 00:56:30, Fady Samuel wrote:
> There are no consumers of this new functionality upstream yet. Once the Worker
> Thread Frame prototype is ready to upstream, I'll add tests specific for it.
> It'll be the only GuestView type that can operate without a container. The
> intent is it can function as a shared worker with a DOM that can be attached
to
> a container where it can composite its DOM onto its embedder.

I was suggesting creating a consumer of it in a test class, to show the
expectations for these APIs within the content/ module.  Otherwise we have
content/ logic that is only tested outside content/, which isn't a great
situation.

I'm not sure how much test code overhead that requires, though, so I'll grant my
LGTM now.  If it's not too difficult, I think it'd be worthwhile to add. 
Otherwise, feel free to proceed.

[Fady Samuel, 2014-12-05 02:17:27]

On 2014/12/05 02:08:29, Charlie Reis wrote:
> On 2014/12/05 00:56:30, Fady Samuel wrote:
> > There are no consumers of this new functionality upstream yet. Once the
Worker
> > Thread Frame prototype is ready to upstream, I'll add tests specific for it.
> > It'll be the only GuestView type that can operate without a container. The
> > intent is it can function as a shared worker with a DOM that can be attached
> to
> > a container where it can composite its DOM onto its embedder.
> 
> I was suggesting creating a consumer of it in a test class, to show the
> expectations for these APIs within the content/ module.  Otherwise we have
> content/ logic that is only tested outside content/, which isn't a great
> situation.
> 
> I'm not sure how much test code overhead that requires, though, so I'll grant
my
> LGTM now.  If it's not too difficult, I think it'd be worthwhile to add. 
> Otherwise, feel free to proceed.

It's hard to test BrowserPlugin inside content now because it gets its
WebContents from outside of content. It can be done but it needs a
MockBrowserPluginGuestDelegate, which we don't have yet. Since that's likely
several hundred lines of code, I'll skip an in content module test for now, and
revisit once we have cycles to pursue this project further. 

Right now, WTFrame is still an exploratory project, and this change exists
merely to allow us to continue to explore that space and chat about API, process
and threading models. This code path is not meant to be production quality code
by any means so that's why it's off by default.

[Fady Samuel, 2014-12-05 02:18:17]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-12-05 02:19:35]

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

[commit-bot: I haz the power, 2014-12-05 02:46:48]

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

[commit-bot: I haz the power, 2014-12-05 02:46:49]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Fady Samuel, 2014-12-05 02:50:06]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-12-05 02:51:35]

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

[commit-bot: I haz the power, 2014-12-05 03:13:50]

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

[commit-bot: I haz the power, 2014-12-05 03:13:51]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Fady Samuel, 2014-12-05 04:21:12]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-12-05 04:21:27]

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

[commit-bot: I haz the power, 2014-12-05 05:06:54]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-12-05 05:07:39]

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