Issue 10868012: Browser Plugin: New Implementation (Browser Side)

lazyboy

Hey, Charlie: This is the updated browser side patch for browser plugin that Fady was ...

8 years, 4 months ago (2012-08-22 00:31:52 UTC) #1

rjkroege

Can you split this up? I mostly read for nits to save Charlie some trouble ...

8 years, 4 months ago (2012-08-22 21:57:38 UTC) #2

lazyboy

Charlie: Do you want me to split up the guest and embedder parts in to ...

8 years, 4 months ago (2012-08-23 00:45:22 UTC) #3

Charlie Reis

On 2012/08/23 00:45:22, lazyboy wrote: > Charlie: Do you want me to split up the ...

8 years, 4 months ago (2012-08-23 00:52:00 UTC) #4

lazyboy

Yes, you should be able to review it tomorrow morning. (Rob let us know if ...

8 years, 4 months ago (2012-08-23 01:41:51 UTC) #5

Yes, you should be able to review it tomorrow morning. (Rob let us know if
that's not okay).

Also, all of Charlie's comments from original CL
(http://chromiumcodereview.appspot.com/10560022) has been addressed while I
created this CL, except a few, which I've added as comments here.

https://chromiumcodereview.appspot.com/10868012/diff/28/content/browser/brows...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/28/content/browser/brows...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:117:
crash_observer.Wait();
creis 2012/08/13 17:50:14
=========================
I still wonder about the kill behavior here.  A 5 second timeout is going to
lead to a lot of unnecessary kills in practice, but lengthening the timeout is
going to make this test very slow.  Is there any way to let the embedder react
to a notification that the guest has hung?

lazyboy:
========
Not sure if I understand this correctly, do you want to shorten the timeout
somehow or just skip crashing the guest part (but still wait 5s).

https://chromiumcodereview.appspot.com/10868012/diff/28/content/browser/brows...
File content/browser/browser_plugin/browser_plugin_host_embedder_delegate.cc
(right):

https://chromiumcodereview.appspot.com/10868012/diff/28/content/browser/brows...
content/browser/browser_plugin/browser_plugin_host_embedder_delegate.cc:60: //
The SiteInstance of a given guest is based on the fact that it's a guest
creis 2012/08/13 17:50:14
=========================
nit: Please move this comment above the host declaration on line 102.

Also, it's unfortunate we don't have any way to enforce that this is in a
platform app.  Are there things that will break if someone tries to create a
BrowserPlugin elsewhere (e.g., in a WebUI page)?

Also, do we need things to enforce that these guest processes won't share across
apps if the process limit is reached?  (Is there already a bug on file for
this?)

lazyboy:
========
re: will browser plugin work other than platform app (e.g. WebUI)?
I don't know what the class WebUI represent, I'm assuming it's regular html
page, in that case it works on a regular page.

re: do we need things to enforce that these guest processes won't share across
apps if the process limit is reached?  (Is there already a bug on file for
this?)
Should I file bug for this?

https://chromiumcodereview.appspot.com/10868012/diff/28/content/browser/brows...
content/browser/browser_plugin/browser_plugin_host_embedder_delegate.cc:179:
GuestSet guests_to_delete;
creis 2012/08/13 17:50:14
=========================
This looks like it finds all the guests in the frame that navigated.  What about
the subframes of that frame?

e.g., Page A has subframe B, which itself has subframe C.  If B navigates, we
have to destroy any guests present in B or C.

lazyboy:
=========
We won't allow embedding browser tag inside browser tags, not initially at least
(http://crbug.com/141206); I've put a TODO below instead to check this if we
ever
allow nested tags.

Charlie Reis

Sorry, I didn't have enough time to get to this in full today. I've started ...

8 years, 4 months ago (2012-08-24 00:28:32 UTC) #6

Sorry, I didn't have enough time to get to this in full today.  I've started
skimming and it looks better, though the role thing seems unusual to me. 
Anyway, here's a few quick replies, and I'll focus on this review in depth
tomorrow.

https://chromiumcodereview.appspot.com/10868012/diff/28/content/browser/brows...
File content/browser/browser_plugin/browser_plugin_host_embedder_delegate.cc
(right):

https://chromiumcodereview.appspot.com/10868012/diff/28/content/browser/brows...
content/browser/browser_plugin/browser_plugin_host_embedder_delegate.cc:60: //
The SiteInstance of a given guest is based on the fact that it's a guest
On 2012/08/23 01:41:51, lazyboy wrote:
> creis 2012/08/13 17:50:14
> =========================
> nit: Please move this comment above the host declaration on line 102.
> 
> Also, it's unfortunate we don't have any way to enforce that this is in a
> platform app.  Are there things that will break if someone tries to create a
> BrowserPlugin elsewhere (e.g., in a WebUI page)?
> 
> Also, do we need things to enforce that these guest processes won't share
across
> apps if the process limit is reached?  (Is there already a bug on file for
> this?)
> 
> lazyboy:
> ========
> re: will browser plugin work other than platform app (e.g. WebUI)?
> I don't know what the class WebUI represent, I'm assuming it's regular html
> page, in that case it works on a regular page.

WebUI pages are special privileged pages in Chrome, like the New Tab Page and
Settings page.  There has been some discussion of allowing browser plugins
within such a page, and I'm concerned that won't work.

Please note that there currently should be no way to include a browser plugin
inside a regular page.  The computation of site here is assuming that the
browser plugin will only be instantiated inside an app, so that the "host" will
be the app's ID.

> 
> re: do we need things to enforce that these guest processes won't share across
> apps if the process limit is reached?  (Is there already a bug on file for
> this?)
> Should I file bug for this?

No, http://crbug.com/131810 will cover it.

https://chromiumcodereview.appspot.com/10868012/diff/28/content/browser/brows...
content/browser/browser_plugin/browser_plugin_host_embedder_delegate.cc:179:
GuestSet guests_to_delete;
On 2012/08/23 01:41:51, lazyboy wrote:
> creis 2012/08/13 17:50:14
> =========================
> This looks like it finds all the guests in the frame that navigated.  What
about
> the subframes of that frame?
> 
> e.g., Page A has subframe B, which itself has subframe C.  If B navigates, we
> have to destroy any guests present in B or C.
> 
> lazyboy:
> =========
> We won't allow embedding browser tag inside browser tags, not initially at
least
> (http://crbug.com/141206%29; I've put a TODO below instead to check this if we
> ever
> allow nested tags.

Good to know, but that's not quite what I meant.  The embedder page could have
an iframe with a browser tag in it (not a browser tag within a browser tag), and
this code won't clean that up.

https://chromiumcodereview.appspot.com/10868012/diff/14036/content/browser/br...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/14036/content/browser/br...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:71: // This
test loads a guest that has inifinite loop, therefore it hangs the guest
nit: inifinite -> infinite

https://chromiumcodereview.appspot.com/10868012/diff/14036/content/public/bro...
File content/public/browser/notification_types.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/14036/content/public/bro...
content/public/browser/notification_types.h:373:
NOTIFICATION_BROWSER_PLUGIN_GUEST_CRASHED,
These need comments.  Also, we're trying to avoid any new notifications if
possible.  (You'll probably get pushback from John here.)

rjkroege

On 2012/08/24 00:28:32, creis wrote: > Sorry, I didn't have enough time to get to ...

8 years, 4 months ago (2012-08-24 15:17:02 UTC) #7

On 2012/08/24 00:28:32, creis wrote:
> Sorry, I didn't have enough time to get to this in full today.  I've started
> skimming and it looks better, though the role thing seems unusual to me. 

The idea of a WebContents "having a role" was intended to capture the notion
that some WebContents act as guests and some act as embedders, that this can
change (an embedder can become a non-embedder and the converse for example).

Maybe there's a better word / pattern?

I also wanted us to have a way to separate guest and embedder specific
functionality. 


> Anyway, here's a few quick replies, and I'll focus on this review in depth
> tomorrow.
> 
>
https://chromiumcodereview.appspot.com/10868012/diff/28/content/browser/brows...
> File content/browser/browser_plugin/browser_plugin_host_embedder_delegate.cc
> (right):
> 
>
https://chromiumcodereview.appspot.com/10868012/diff/28/content/browser/brows...
> content/browser/browser_plugin/browser_plugin_host_embedder_delegate.cc:60: //
> The SiteInstance of a given guest is based on the fact that it's a guest
> On 2012/08/23 01:41:51, lazyboy wrote:
> > creis 2012/08/13 17:50:14
> > =========================
> > nit: Please move this comment above the host declaration on line 102.
> > 
> > Also, it's unfortunate we don't have any way to enforce that this is in a
> > platform app.  Are there things that will break if someone tries to create a
> > BrowserPlugin elsewhere (e.g., in a WebUI page)?
> > 
> > Also, do we need things to enforce that these guest processes won't share
> across
> > apps if the process limit is reached?  (Is there already a bug on file for
> > this?)
> > 
> > lazyboy:
> > ========
> > re: will browser plugin work other than platform app (e.g. WebUI)?
> > I don't know what the class WebUI represent, I'm assuming it's regular html
> > page, in that case it works on a regular page.
> 
> WebUI pages are special privileged pages in Chrome, like the New Tab Page and
> Settings page.  There has been some discussion of allowing browser plugins
> within such a page, and I'm concerned that won't work.
> 
> Please note that there currently should be no way to include a browser plugin
> inside a regular page.  The computation of site here is assuming that the
> browser plugin will only be instantiated inside an app, so that the "host"
will
> be the app's ID.
> 
> > 
> > re: do we need things to enforce that these guest processes won't share
across
> > apps if the process limit is reached?  (Is there already a bug on file for
> > this?)
> > Should I file bug for this?
> 
> No, http://crbug.com/131810 will cover it.
> 
>
https://chromiumcodereview.appspot.com/10868012/diff/28/content/browser/brows...
> content/browser/browser_plugin/browser_plugin_host_embedder_delegate.cc:179:
> GuestSet guests_to_delete;
> On 2012/08/23 01:41:51, lazyboy wrote:
> > creis 2012/08/13 17:50:14
> > =========================
> > This looks like it finds all the guests in the frame that navigated.  What
> about
> > the subframes of that frame?
> > 
> > e.g., Page A has subframe B, which itself has subframe C.  If B navigates,
we
> > have to destroy any guests present in B or C.
> > 
> > lazyboy:
> > =========
> > We won't allow embedding browser tag inside browser tags, not initially at
> least
> > (http://crbug.com/141206%2529; I've put a TODO below instead to check this
if we
> > ever
> > allow nested tags.
> 
> Good to know, but that's not quite what I meant.  The embedder page could have
> an iframe with a browser tag in it (not a browser tag within a browser tag),
and
> this code won't clean that up.
> 
>
https://chromiumcodereview.appspot.com/10868012/diff/14036/content/browser/br...
> File content/browser/browser_plugin/browser_plugin_host_browsertest.cc
(right):
> 
>
https://chromiumcodereview.appspot.com/10868012/diff/14036/content/browser/br...
> content/browser/browser_plugin/browser_plugin_host_browsertest.cc:71: // This
> test loads a guest that has inifinite loop, therefore it hangs the guest
> nit: inifinite -> infinite
> 
>
https://chromiumcodereview.appspot.com/10868012/diff/14036/content/public/bro...
> File content/public/browser/notification_types.h (right):
> 
>
https://chromiumcodereview.appspot.com/10868012/diff/14036/content/public/bro...
> content/public/browser/notification_types.h:373:
> NOTIFICATION_BROWSER_PLUGIN_GUEST_CRASHED,
> These need comments.  Also, we're trying to avoid any new notifications if
> possible.  (You'll probably get pushback from John here.)

Charlie Reis

On 2012/08/24 15:17:02, rjkroege wrote: > On 2012/08/24 00:28:32, creis wrote: > > Sorry, I ...

8 years, 4 months ago (2012-08-24 17:22:00 UTC) #8

On 2012/08/24 15:17:02, rjkroege wrote:
> On 2012/08/24 00:28:32, creis wrote:
> > Sorry, I didn't have enough time to get to this in full today.  I've started
> > skimming and it looks better, though the role thing seems unusual to me. 
> 
> The idea of a WebContents "having a role" was intended to capture the notion
> that some WebContents act as guests and some act as embedders, that this can
> change (an embedder can become a non-embedder and the converse for example).
> 
> Maybe there's a better word / pattern?
> 
> I also wanted us to have a way to separate guest and embedder specific
> functionality. 

Yeah, I think we might be able to just reorganize things a bit to make it
clearer.  First, "delegates" usually refer to a callback interface that some
other class implements, but here they appear to include state and the actual
logic.  Second, the "role" classes seem like they're just listening to messages
and passing them on to the "delegates," which feels unnecessary.  Each
delegate/role pair could probably be folded into the same class.

I may be missing something, but what about this simpler layout?
1. Merge BrowserPluginHostGuestRole and BrowserPluginHostGuestDelegate into one
class, called BrowserPluginHost.  (This corresponds to the BrowserPlugin host
class in the renderer process, so it nicely fits the Foo and FooHost naming
pattern.)
2. Merge BrowserPluginHostEmbedderRole and BrowserPluginHostEmbedderDelegate
into one class, called BrowserPluginEmbedder.  (This has no corresponding class
in the renderer process, but rather manages the various guests in a given
embedder tab.)
3. Eliminate BrowserPluginHostRole, which doesn't appear to be used for
anything.

Then a WebContents can have a BrowserPluginEmbedder if it becomes an embedder,
allowing it to manage its guests.  And a WebContents can have a
BrowserPluginHost if it itself is a guest.

Does that make sense?  Just want to make sure my understanding is correct.

Thanks,
Charlie

lazyboy

On 2012/08/24 17:22:00, creis wrote: > On 2012/08/24 15:17:02, rjkroege wrote: > > On 2012/08/24 ...

8 years, 4 months ago (2012-08-24 17:38:45 UTC) #9

On 2012/08/24 17:22:00, creis wrote:
> On 2012/08/24 15:17:02, rjkroege wrote:
> > On 2012/08/24 00:28:32, creis wrote:
> > > Sorry, I didn't have enough time to get to this in full today.  I've
started
> > > skimming and it looks better, though the role thing seems unusual to me. 
> > 
> > The idea of a WebContents "having a role" was intended to capture the notion
> > that some WebContents act as guests and some act as embedders, that this can
> > change (an embedder can become a non-embedder and the converse for example).
> > 
> > Maybe there's a better word / pattern?
> > 
> > I also wanted us to have a way to separate guest and embedder specific
> > functionality. 
> 
> Yeah, I think we might be able to just reorganize things a bit to make it
> clearer.  First, "delegates" usually refer to a callback interface that some
> other class implements, but here they appear to include state and the actual
> logic.  Second, the "role" classes seem like they're just listening to
messages
> and passing them on to the "delegates," which feels unnecessary.  Each
> delegate/role pair could probably be folded into the same class.
> 
> I may be missing something, but what about this simpler layout?
> 1. Merge BrowserPluginHostGuestRole and BrowserPluginHostGuestDelegate into
one
> class, called BrowserPluginHost.  (This corresponds to the BrowserPlugin host
> class in the renderer process, so it nicely fits the Foo and FooHost naming
> pattern.)
> 2. Merge BrowserPluginHostEmbedderRole and BrowserPluginHostEmbedderDelegate
> into one class, called BrowserPluginEmbedder.  (This has no corresponding
class
> in the renderer process, but rather manages the various guests in a given
> embedder tab.)
> 3. Eliminate BrowserPluginHostRole, which doesn't appear to be used for
> anything.
> 
> Then a WebContents can have a BrowserPluginEmbedder if it becomes an embedder,
> allowing it to manage its guests.  And a WebContents can have a
> BrowserPluginHost if it itself is a guest.
> 
> Does that make sense?  Just want to make sure my understanding is correct.
> 
> Thanks,
> Charlie

Your understanding is correct.
One other reason we separated the Role/message_listener and Delegate/logic class
is that the earlier one could essentially run as a MessageFilter (esp. for
embedder) to have room for future optimization (reducing thread hops for
events).
Rob can comment more on this.

Fady Samuel

On 2012/08/24 17:38:45, lazyboy wrote: > On 2012/08/24 17:22:00, creis wrote: > > On 2012/08/24 ...

8 years, 4 months ago (2012-08-24 17:43:58 UTC) #10

On 2012/08/24 17:38:45, lazyboy wrote:
> On 2012/08/24 17:22:00, creis wrote:
> > On 2012/08/24 15:17:02, rjkroege wrote:
> > > On 2012/08/24 00:28:32, creis wrote:
> > > > Sorry, I didn't have enough time to get to this in full today.  I've
> started
> > > > skimming and it looks better, though the role thing seems unusual to me.

> > > 
> > > The idea of a WebContents "having a role" was intended to capture the
notion
> > > that some WebContents act as guests and some act as embedders, that this
can
> > > change (an embedder can become a non-embedder and the converse for
example).
> > > 
> > > Maybe there's a better word / pattern?
> > > 
> > > I also wanted us to have a way to separate guest and embedder specific
> > > functionality. 
> > 
> > Yeah, I think we might be able to just reorganize things a bit to make it
> > clearer.  First, "delegates" usually refer to a callback interface that some
> > other class implements, but here they appear to include state and the actual
> > logic.  Second, the "role" classes seem like they're just listening to
> messages
> > and passing them on to the "delegates," which feels unnecessary.  Each
> > delegate/role pair could probably be folded into the same class.
> > 
> > I may be missing something, but what about this simpler layout?
> > 1. Merge BrowserPluginHostGuestRole and BrowserPluginHostGuestDelegate into
> one
> > class, called BrowserPluginHost.  (This corresponds to the BrowserPlugin
host
> > class in the renderer process, so it nicely fits the Foo and FooHost naming
> > pattern.)
> > 2. Merge BrowserPluginHostEmbedderRole and BrowserPluginHostEmbedderDelegate
> > into one class, called BrowserPluginEmbedder.  (This has no corresponding
> class
> > in the renderer process, but rather manages the various guests in a given
> > embedder tab.)
> > 3. Eliminate BrowserPluginHostRole, which doesn't appear to be used for
> > anything.
> > 
> > Then a WebContents can have a BrowserPluginEmbedder if it becomes an
embedder,
> > allowing it to manage its guests.  And a WebContents can have a
> > BrowserPluginHost if it itself is a guest.
> > 
> > Does that make sense?  Just want to make sure my understanding is correct.
> > 
> > Thanks,
> > Charlie
> 
> Your understanding is correct.
> One other reason we separated the Role/message_listener and Delegate/logic
class
> is that the earlier one could essentially run as a MessageFilter (esp. for
> embedder) to have room for future optimization (reducing thread hops for
> events).
> Rob can comment more on this.

Also, BrowserPluginHostGuestRole/EmbedderRole were originally
BrowserPluginHostHelper (RenderViewHostObserver as opposed to
WebContentsObserver).

I originally created BrowserPluginHostHelper in response to some cross process
navigation concerns raised by Charlie about making sure that messages arriving
from one RenderViewHost should be responded to via the same RenderViewHost via a
method parameter.

It seems like these will still apply in the near future, no?
WebContentsObservers and RenderViewHostObservers have different lifetimes and
therefore should be different classes, right?

Charlie Reis

On 2012/08/24 17:43:58, Fady Samuel wrote: > On 2012/08/24 17:38:45, lazyboy wrote: > > On ...

8 years, 4 months ago (2012-08-24 18:59:24 UTC) #11

On 2012/08/24 17:43:58, Fady Samuel wrote:
> On 2012/08/24 17:38:45, lazyboy wrote:
> > On 2012/08/24 17:22:00, creis wrote:
> > > On 2012/08/24 15:17:02, rjkroege wrote:
> > > > On 2012/08/24 00:28:32, creis wrote:
> > > > > Sorry, I didn't have enough time to get to this in full today.  I've
> > started
> > > > > skimming and it looks better, though the role thing seems unusual to
me.
> 
> > > > 
> > > > The idea of a WebContents "having a role" was intended to capture the
> notion
> > > > that some WebContents act as guests and some act as embedders, that this
> can
> > > > change (an embedder can become a non-embedder and the converse for
> example).
> > > > 
> > > > Maybe there's a better word / pattern?
> > > > 
> > > > I also wanted us to have a way to separate guest and embedder specific
> > > > functionality. 
> > > 
> > > Yeah, I think we might be able to just reorganize things a bit to make it
> > > clearer.  First, "delegates" usually refer to a callback interface that
some
> > > other class implements, but here they appear to include state and the
actual
> > > logic.  Second, the "role" classes seem like they're just listening to
> > messages
> > > and passing them on to the "delegates," which feels unnecessary.  Each
> > > delegate/role pair could probably be folded into the same class.
> > > 
> > > I may be missing something, but what about this simpler layout?
> > > 1. Merge BrowserPluginHostGuestRole and BrowserPluginHostGuestDelegate
into
> > one
> > > class, called BrowserPluginHost.  (This corresponds to the BrowserPlugin
> host
> > > class in the renderer process, so it nicely fits the Foo and FooHost
naming
> > > pattern.)
> > > 2. Merge BrowserPluginHostEmbedderRole and
BrowserPluginHostEmbedderDelegate
> > > into one class, called BrowserPluginEmbedder.  (This has no corresponding
> > class
> > > in the renderer process, but rather manages the various guests in a given
> > > embedder tab.)
> > > 3. Eliminate BrowserPluginHostRole, which doesn't appear to be used for
> > > anything.
> > > 
> > > Then a WebContents can have a BrowserPluginEmbedder if it becomes an
> embedder,
> > > allowing it to manage its guests.  And a WebContents can have a
> > > BrowserPluginHost if it itself is a guest.
> > > 
> > > Does that make sense?  Just want to make sure my understanding is correct.
> > > 
> > > Thanks,
> > > Charlie
> > 
> > Your understanding is correct.
> > One other reason we separated the Role/message_listener and Delegate/logic
> class
> > is that the earlier one could essentially run as a MessageFilter (esp. for
> > embedder) to have room for future optimization (reducing thread hops for
> > events).
> > Rob can comment more on this.
> 
> Also, BrowserPluginHostGuestRole/EmbedderRole were originally
> BrowserPluginHostHelper (RenderViewHostObserver as opposed to
> WebContentsObserver).
> 
> I originally created BrowserPluginHostHelper in response to some cross process
> navigation concerns raised by Charlie about making sure that messages arriving
> from one RenderViewHost should be responded to via the same RenderViewHost via
a
> method parameter.
> 
> It seems like these will still apply in the near future, no?
> WebContentsObservers and RenderViewHostObservers have different lifetimes and
> therefore should be different classes, right?

Oh, I missed that these classes were different types of observers.  But it
doesn't work as is, either, right?  The "role" class is the
RenderViewHostObserver, but it's stored on WebContents.

I suppose we could have BrowserPluginHostHelper and BrowserPluginEmbedderHelper
as RenderViewHostObservers, which probably boils down to a rename (and dropping
BrowserPluginHostRole)?  It certainly needs to be clearer why these classes are
split (i.e., that the RenderViewHost may change over time).

lazyboy

On 2012/08/24 18:59:24, creis wrote: > On 2012/08/24 17:43:58, Fady Samuel wrote: > > On ...

8 years, 4 months ago (2012-08-24 19:54:59 UTC) #12

On 2012/08/24 18:59:24, creis wrote:
> On 2012/08/24 17:43:58, Fady Samuel wrote:
> > On 2012/08/24 17:38:45, lazyboy wrote:
> > > On 2012/08/24 17:22:00, creis wrote:
> > > > On 2012/08/24 15:17:02, rjkroege wrote:
> > > > > On 2012/08/24 00:28:32, creis wrote:
> > > > > > Sorry, I didn't have enough time to get to this in full today.  I've
> > > started
> > > > > > skimming and it looks better, though the role thing seems unusual to
> me.
> > 
> > > > > 
> > > > > The idea of a WebContents "having a role" was intended to capture the
> > notion
> > > > > that some WebContents act as guests and some act as embedders, that
this
> > can
> > > > > change (an embedder can become a non-embedder and the converse for
> > example).
> > > > > 
> > > > > Maybe there's a better word / pattern?
> > > > > 
> > > > > I also wanted us to have a way to separate guest and embedder specific
> > > > > functionality. 
> > > > 
> > > > Yeah, I think we might be able to just reorganize things a bit to make
it
> > > > clearer.  First, "delegates" usually refer to a callback interface that
> some
> > > > other class implements, but here they appear to include state and the
> actual
> > > > logic.  Second, the "role" classes seem like they're just listening to
> > > messages
> > > > and passing them on to the "delegates," which feels unnecessary.  Each
> > > > delegate/role pair could probably be folded into the same class.
> > > > 
> > > > I may be missing something, but what about this simpler layout?
> > > > 1. Merge BrowserPluginHostGuestRole and BrowserPluginHostGuestDelegate
> into
> > > one
> > > > class, called BrowserPluginHost.  (This corresponds to the BrowserPlugin
> > host
> > > > class in the renderer process, so it nicely fits the Foo and FooHost
> naming
> > > > pattern.)
> > > > 2. Merge BrowserPluginHostEmbedderRole and
> BrowserPluginHostEmbedderDelegate
> > > > into one class, called BrowserPluginEmbedder.  (This has no
corresponding
> > > class
> > > > in the renderer process, but rather manages the various guests in a
given
> > > > embedder tab.)
> > > > 3. Eliminate BrowserPluginHostRole, which doesn't appear to be used for
> > > > anything.
> > > > 
> > > > Then a WebContents can have a BrowserPluginEmbedder if it becomes an
> > embedder,
> > > > allowing it to manage its guests.  And a WebContents can have a
> > > > BrowserPluginHost if it itself is a guest.
> > > > 
> > > > Does that make sense?  Just want to make sure my understanding is
correct.
> > > > 
> > > > Thanks,
> > > > Charlie
> > > 
> > > Your understanding is correct.
> > > One other reason we separated the Role/message_listener and Delegate/logic
> > class
> > > is that the earlier one could essentially run as a MessageFilter (esp. for
> > > embedder) to have room for future optimization (reducing thread hops for
> > > events).
> > > Rob can comment more on this.
> > 
> > Also, BrowserPluginHostGuestRole/EmbedderRole were originally
> > BrowserPluginHostHelper (RenderViewHostObserver as opposed to
> > WebContentsObserver).
> > 
> > I originally created BrowserPluginHostHelper in response to some cross
process
> > navigation concerns raised by Charlie about making sure that messages
arriving
> > from one RenderViewHost should be responded to via the same RenderViewHost
via
> a
> > method parameter.
> > 
> > It seems like these will still apply in the near future, no?
> > WebContentsObservers and RenderViewHostObservers have different lifetimes
and
> > therefore should be different classes, right?
> 
> Oh, I missed that these classes were different types of observers.  But it
> doesn't work as is, either, right?  The "role" class is the
> RenderViewHostObserver, but it's stored on WebContents.
> 
> I suppose we could have BrowserPluginHostHelper and
BrowserPluginEmbedderHelper
> as RenderViewHostObservers, which probably boils down to a rename (and
dropping
> BrowserPluginHostRole)?  It certainly needs to be clearer why these classes
are
> split (i.e., that the RenderViewHost may change over time).

Okay, so to recap (and one name question):

rename BrowserPluginHostGuestRole -> BrowserPluginHostHelper : RVHObserver,
NotificationObserver
rename BrowserPluginHostGuestDelegate -> BrowserPluginHost : WCObserver


rename BrowserPluginHostEmbedderRole -> BrowserPluginHostEmbedderHelper :
RVHObserver, NotificationObserver
rename BrowserPluginHostEmbedderDelegate -> BrowserPluginEmbedderHost :
WCObserver, WCDelegate
(*** or BrowserPluginHostEmbedder?, don't we need *Host* in the class name b/c
it's on browser/host side? or we should just drop it because there's no
corresponding renderer counterpart?)


since *Host-s are WCObserver, store them in WebContents, not the helpers.

no common functionality betwn BrowserPluginHost and BrowserPluginEmbedder
exists, so remove BrowserPluginHostRole.

I'll be making the changes today if this sounds good, thanks.

Fady Samuel

On 2012/08/24 19:54:59, lazyboy wrote: > On 2012/08/24 18:59:24, creis wrote: > > On 2012/08/24 ...

8 years, 4 months ago (2012-08-24 20:27:44 UTC) #13

On 2012/08/24 19:54:59, lazyboy wrote:
> On 2012/08/24 18:59:24, creis wrote:
> > On 2012/08/24 17:43:58, Fady Samuel wrote:
> > > On 2012/08/24 17:38:45, lazyboy wrote:
> > > > On 2012/08/24 17:22:00, creis wrote:
> > > > > On 2012/08/24 15:17:02, rjkroege wrote:
> > > > > > On 2012/08/24 00:28:32, creis wrote:
> > > > > > > Sorry, I didn't have enough time to get to this in full today. 
I've
> > > > started
> > > > > > > skimming and it looks better, though the role thing seems unusual
to
> > me.
> > > 
> > > > > > 
> > > > > > The idea of a WebContents "having a role" was intended to capture
the
> > > notion
> > > > > > that some WebContents act as guests and some act as embedders, that
> this
> > > can
> > > > > > change (an embedder can become a non-embedder and the converse for
> > > example).
> > > > > > 
> > > > > > Maybe there's a better word / pattern?
> > > > > > 
> > > > > > I also wanted us to have a way to separate guest and embedder
specific
> > > > > > functionality. 
> > > > > 
> > > > > Yeah, I think we might be able to just reorganize things a bit to make
> it
> > > > > clearer.  First, "delegates" usually refer to a callback interface
that
> > some
> > > > > other class implements, but here they appear to include state and the
> > actual
> > > > > logic.  Second, the "role" classes seem like they're just listening to
> > > > messages
> > > > > and passing them on to the "delegates," which feels unnecessary.  Each
> > > > > delegate/role pair could probably be folded into the same class.
> > > > > 
> > > > > I may be missing something, but what about this simpler layout?
> > > > > 1. Merge BrowserPluginHostGuestRole and BrowserPluginHostGuestDelegate
> > into
> > > > one
> > > > > class, called BrowserPluginHost.  (This corresponds to the
BrowserPlugin
> > > host
> > > > > class in the renderer process, so it nicely fits the Foo and FooHost
> > naming
> > > > > pattern.)
> > > > > 2. Merge BrowserPluginHostEmbedderRole and
> > BrowserPluginHostEmbedderDelegate
> > > > > into one class, called BrowserPluginEmbedder.  (This has no
> corresponding
> > > > class
> > > > > in the renderer process, but rather manages the various guests in a
> given
> > > > > embedder tab.)
> > > > > 3. Eliminate BrowserPluginHostRole, which doesn't appear to be used
for
> > > > > anything.
> > > > > 
> > > > > Then a WebContents can have a BrowserPluginEmbedder if it becomes an
> > > embedder,
> > > > > allowing it to manage its guests.  And a WebContents can have a
> > > > > BrowserPluginHost if it itself is a guest.
> > > > > 
> > > > > Does that make sense?  Just want to make sure my understanding is
> correct.
> > > > > 
> > > > > Thanks,
> > > > > Charlie
> > > > 
> > > > Your understanding is correct.
> > > > One other reason we separated the Role/message_listener and
Delegate/logic
> > > class
> > > > is that the earlier one could essentially run as a MessageFilter (esp.
for
> > > > embedder) to have room for future optimization (reducing thread hops for
> > > > events).
> > > > Rob can comment more on this.
> > > 
> > > Also, BrowserPluginHostGuestRole/EmbedderRole were originally
> > > BrowserPluginHostHelper (RenderViewHostObserver as opposed to
> > > WebContentsObserver).
> > > 
> > > I originally created BrowserPluginHostHelper in response to some cross
> process
> > > navigation concerns raised by Charlie about making sure that messages
> arriving
> > > from one RenderViewHost should be responded to via the same RenderViewHost
> via
> > a
> > > method parameter.
> > > 
> > > It seems like these will still apply in the near future, no?
> > > WebContentsObservers and RenderViewHostObservers have different lifetimes
> and
> > > therefore should be different classes, right?
> > 
> > Oh, I missed that these classes were different types of observers.  But it
> > doesn't work as is, either, right?  The "role" class is the
> > RenderViewHostObserver, but it's stored on WebContents.
> > 
> > I suppose we could have BrowserPluginHostHelper and
> BrowserPluginEmbedderHelper
> > as RenderViewHostObservers, which probably boils down to a rename (and
> dropping
> > BrowserPluginHostRole)?  It certainly needs to be clearer why these classes
> are
> > split (i.e., that the RenderViewHost may change over time).
> 
> Okay, so to recap (and one name question):
> 
> rename BrowserPluginHostGuestRole -> BrowserPluginHostHelper : RVHObserver,
> NotificationObserver
> rename BrowserPluginHostGuestDelegate -> BrowserPluginHost : WCObserver
> 
> 
> rename BrowserPluginHostEmbedderRole -> BrowserPluginHostEmbedderHelper :
> RVHObserver, NotificationObserver
> rename BrowserPluginHostEmbedderDelegate -> BrowserPluginEmbedderHost :
> WCObserver, WCDelegate
> (*** or BrowserPluginHostEmbedder?, don't we need *Host* in the class name b/c
> it's on browser/host side? or we should just drop it because there's no
> corresponding renderer counterpart?)
> 
> 
> since *Host-s are WCObserver, store them in WebContents, not the helpers.
> 
> no common functionality betwn BrowserPluginHost and BrowserPluginEmbedder
> exists, so remove BrowserPluginHostRole.
> 
> I'll be making the changes today if this sounds good, thanks.

This doesn't match the typical naming schemes.

rename BrowserPluginHostEmbedderRole -> BrowserPluginHostHelper :
RVHObserver, NotificationObserver

BrowserPluginHostEmbedderDelegate -> BrowserPluginHost :
WCObserver, WCDelegate


rename BrowserPluginHostGuestRole -> BrowserPluginHostGuestHelper : RVHObserver,
NotificationObserver

rename BrowserPluginHostGuestDelegate -> BrowserPluginHostGuest : WCObserver

Whether we call this a GuestHost or a HostGuest, both are ugly, but at least
they better describe what they do.

Charlie Reis

On 2012/08/24 20:27:44, Fady Samuel wrote: > On 2012/08/24 19:54:59, lazyboy wrote: > > Okay, ...

8 years, 4 months ago (2012-08-24 21:50:29 UTC) #14

lazyboy

On 2012/08/24 21:50:29, creis wrote: > On 2012/08/24 20:27:44, Fady Samuel wrote: > > On ...

8 years, 4 months ago (2012-08-25 01:32:18 UTC) #15

On 2012/08/24 21:50:29, creis wrote:
> On 2012/08/24 20:27:44, Fady Samuel wrote:
> > On 2012/08/24 19:54:59, lazyboy wrote:
> > > Okay, so to recap (and one name question):
> > > 
> > > rename BrowserPluginHostGuestRole -> BrowserPluginHostHelper :
RVHObserver,
> > > NotificationObserver
> > > rename BrowserPluginHostGuestDelegate -> BrowserPluginHost : WCObserver
> > > 
> > > 
> > > rename BrowserPluginHostEmbedderRole -> BrowserPluginHostEmbedderHelper :
> > > RVHObserver, NotificationObserver
> > > rename BrowserPluginHostEmbedderDelegate -> BrowserPluginEmbedderHost :
> > > WCObserver, WCDelegate
> > > (*** or BrowserPluginHostEmbedder?, don't we need *Host* in the class name
> b/c
> > > it's on browser/host side? or we should just drop it because there's no
> > > corresponding renderer counterpart?)
> > > 
> > > 
> > > since *Host-s are WCObserver, store them in WebContents, not the helpers.
> > > 
> > > no common functionality betwn BrowserPluginHost and BrowserPluginEmbedder
> > > exists, so remove BrowserPluginHostRole.
> > > 
> > > I'll be making the changes today if this sounds good, thanks.
> > 
> > This doesn't match the typical naming schemes.
> > 
> > rename BrowserPluginHostEmbedderRole -> BrowserPluginHostHelper :
> > RVHObserver, NotificationObserver
> > 
> > BrowserPluginHostEmbedderDelegate -> BrowserPluginHost :
> > WCObserver, WCDelegate
> > 
> > 
> > rename BrowserPluginHostGuestRole -> BrowserPluginHostGuestHelper :
> RVHObserver,
> > NotificationObserver
> > 
> > rename BrowserPluginHostGuestDelegate -> BrowserPluginHostGuest : WCObserver
> > 
> > Whether we call this a GuestHost or a HostGuest, both are ugly, but at least
> > they better describe what they do.
> 
> No, that's not what I meant.  I'm not on the same page as you for what these
> classes represent.  I'll have to take another look at the design doc, so
please
> hold off renaming things for the moment.

As we discussed offline, I've done the renaming, Please take a look. Thanks.

lazyboy

... forgot to publish these. http://chromiumcodereview.appspot.com/10868012/diff/14036/content/browser/browser_plugin/browser_plugin_host_browsertest.cc File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right): http://chromiumcodereview.appspot.com/10868012/diff/14036/content/browser/browser_plugin/browser_plugin_host_browsertest.cc#newcode71 content/browser/browser_plugin/browser_plugin_host_browsertest.cc:71: // This test loads ...

8 years, 4 months ago (2012-08-25 01:32:51 UTC) #16

Fady Samuel

http://chromiumcodereview.appspot.com/10868012/diff/3027/content/browser/browser_plugin/browser_plugin_embedder.cc File content/browser/browser_plugin/browser_plugin_embedder.cc (right): http://chromiumcodereview.appspot.com/10868012/diff/3027/content/browser/browser_plugin/browser_plugin_embedder.cc#newcode30 content/browser/browser_plugin/browser_plugin_embedder.cc:30: new BrowserPluginEmbedderHelper(this, web_contents, render_view_host); The helpers are not being ...

8 years, 4 months ago (2012-08-25 05:28:32 UTC) #17

lazyboy

https://chromiumcodereview.appspot.com/10868012/diff/3027/content/browser/browser_plugin/browser_plugin_guest.cc File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://chromiumcodereview.appspot.com/10868012/diff/3027/content/browser/browser_plugin/browser_plugin_guest.cc#newcode39 content/browser/browser_plugin/browser_plugin_guest.cc:39: new BrowserPluginGuestHelper(this, render_view_host); On 2012/08/25 05:28:32, Fady Samuel wrote: ...

8 years, 4 months ago (2012-08-25 05:57:08 UTC) #18

Fady Samuel

On 2012/08/25 05:57:08, lazyboy wrote: > https://chromiumcodereview.appspot.com/10868012/diff/3027/content/browser/browser_plugin/browser_plugin_guest.cc > File content/browser/browser_plugin/browser_plugin_guest.cc (right): > > https://chromiumcodereview.appspot.com/10868012/diff/3027/content/browser/browser_plugin/browser_plugin_guest.cc#newcode39 > ...

8 years, 4 months ago (2012-08-25 08:39:39 UTC) #19

Fady Samuel

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plugin/browser_plugin_embedder_helper.cc File content/browser/browser_plugin/browser_plugin_embedder_helper.cc (right): http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plugin/browser_plugin_embedder_helper.cc#newcode27 content/browser/browser_plugin/browser_plugin_embedder_helper.cc:27: WebContentsImpl* web_contents, It seems the only reason BrowserPluginEmbedderHelper needs ...

8 years, 3 months ago (2012-08-27 15:35:00 UTC) #20

lazyboy

https://chromiumcodereview.appspot.com/10868012/diff/3027/content/browser/browser_plugin/browser_plugin_guest.cc File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://chromiumcodereview.appspot.com/10868012/diff/3027/content/browser/browser_plugin/browser_plugin_guest.cc#newcode39 content/browser/browser_plugin/browser_plugin_guest.cc:39: new BrowserPluginGuestHelper(this, render_view_host); On 2012/08/25 05:57:09, lazyboy wrote: > ...

8 years, 3 months ago (2012-08-27 15:42:51 UTC) #21

Charlie Reis

On 2012/08/27 15:42:51, lazyboy wrote: > https://chromiumcodereview.appspot.com/10868012/diff/3027/content/browser/browser_plugin/browser_plugin_guest.cc > File content/browser/browser_plugin/browser_plugin_guest.cc (right): > > https://chromiumcodereview.appspot.com/10868012/diff/3027/content/browser/browser_plugin/browser_plugin_guest.cc#newcode39 > ...

8 years, 3 months ago (2012-08-27 18:36:08 UTC) #22

On 2012/08/27 15:42:51, lazyboy wrote:
>
https://chromiumcodereview.appspot.com/10868012/diff/3027/content/browser/bro...
> File content/browser/browser_plugin/browser_plugin_guest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10868012/diff/3027/content/browser/bro...
> content/browser/browser_plugin/browser_plugin_guest.cc:39: new
> BrowserPluginGuestHelper(this, render_view_host);
> On 2012/08/25 05:57:09, lazyboy wrote:
> > On 2012/08/25 05:28:32, Fady Samuel wrote:
> > > 
> > > The lifetime of the helper is not equal to the lifetime of the
> > > BrowserPluginGuest. The helper should be created on
> > > NOTIFICATION_WEB_CONTENTS_RENDER_VIEW_HOST_CREATED as before, see
> > > http://codereview.chromium.org/10560022/patch/59102/53234
> > > 
> > > We need to create a new helper every time we get a new RenderViewHost
> > > (cross-process navigation for example).
> > 
> > Ah, that's why it was like that, I see.
> > Now the problem is, at the time BrowserPluginGuest gets created, we
> would/might
> > already have the render_view_host created (since we don't create
> > BrowserPluginGuest at the very beginning of WebContentsImpl anymore) and we
> > might not get such notification (at least the first RVH creation one) at
all?
> 
> So my assumption was:
> embedder loads guest http://a.com, creates guest, gweb_contents, g_RVH
> embedder navigates the same guest to http://b.com, modifies existing
gweb_contents to
> LoadURL http://b.com, *this would create a new RVH other than g_RVH?
> 
> I don't see any notification saying RENDER_VIEW_HOST_CREATED in this case
(both
> in this code and your old patch)? When does a web_contents create (another)
new
> RVH?

We currently have it set up so that guest WebContents never do a cross-process
navigation.  It will not create a new RVH.

That may change in the future, though.  We probably shouldn't worry about
infrastructure for it until it's needed, though.

On the other hand, I think we do need to know if the embedder switches to a
different RVH.  It's not needed for AppsV2 (where there are no navigations at
all), but it is needed if browser plugins can be used in WebUI pages.  For
example, the NTP could have a browser plugin, then you might navigate to the
settings page in a different process, which also might have a browser plugin.

Charlie Reis

Thanks, this is much easier to follow with the new names. Comments inline. https://chromiumcodereview.appspot.com/10868012/diff/3027/content/browser/browser_plugin/browser_plugin_embedder.cc File ...

8 years, 3 months ago (2012-08-27 20:23:59 UTC) #23

lazyboy

*I am still working on making unit tests work without NOTIFICATION_* (having linker errors on ...

8 years, 3 months ago (2012-08-28 19:07:14 UTC) #24

*I am still working on making unit tests work without NOTIFICATION_* (having
linker errors on unrelated issues :()

recap/issues_to_resolve for this round:

1. Issue when RVH switches in a WC to a different one for embedder. I've not
made any change for this, have more questions. (This relates to both creis and
fsamuel's comments)

2. re: Killing process when guest hangs on input, rjkroege@ to follow up more.

3. There are several places I would require fsamuel@ to answer, please help!
(Search for word 'Fady' in comments).

Thanks

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_embedder.cc:48:
DCHECK(guests_by_instance_id_.find(instance_id) ==
On 2012/08/27 20:23:59, creis wrote:
> nit: DCHECK_EQ
> 
> Should we make this a CHECK_EQ?
Isn't CHECK more aggressive for this case?
Also, _EQ is giving errors since it cannot resolve default operator= for
map::iterator :S

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_embedder.cc:91:
guest->web_contents()->SetDelegate(guest);
On 2012/08/27 20:23:59, creis wrote:
> Do we need to set this again if the guest already existed, or should we move
it
> above the else block?

Moved.
Also this is a bit weird this part has guest_web_contents and
guest->web_contents() in many places; Fady, wouldn't they refer to the same
thing?

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_embedder.cc:94:
PAGE_TRANSITION_AUTO_SUBFRAME,
On 2012/08/27 20:23:59, creis wrote:
> I like that we're classifying this as a subframe load.  Is that going to
confuse
> anything in the guest, since it thinks it's loading a top-level page?

I'm not aware of the differences. Only related difference I have seen is
InfoBars don't show up (which is a good thing for us, if that's related);
I believe if the more we can treat WebContents transparently (whether or not
they are guest) the better.
Do you want me to add a note/todo?

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_embedder.cc:172: // Clean-up
guests that lie in the frame that we're navigating.
On 2012/08/27 20:23:59, creis wrote:
> nit: Clean up

Done.

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_embedder.cc:176: it !=
guests_.end(); ++it) {
On 2012/08/27 20:23:59, creis wrote:
> nit: Wrong indent.  Same below.

I somehow broke this thinking for statements are not function calls, it should
indent 4 sp.
Fixed.

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_embedder.cc:180: // care of
subframes inside this frame as well.
On 2012/08/27 20:23:59, creis wrote:
> This is not sufficient.
> 
> If the embedder has a tree of iframes, we also have to delete guests from all
of
> the children of the frame that navigated.

Wouldn't that iframe's embedder take care of that?
If not, then I think it's better to make that instead of recursively deleting
from here.

Also would we allow embedding guests inside a platform app's iframe? (Does that
have a compelling use case?)

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_embedder.cc:204: it !=
guests_.end(); ++it) {
On 2012/08/27 20:23:59, creis wrote:
> nit: Wrong indent.

Done.

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_embedder.h:39: RenderViewHost*
render_view_host);
On 2012/08/27 20:23:59, creis wrote:
> This suggests that we'll need to update our RVH if |web_contents| does a
> cross-process navigation.
Or create replace with a new embedder and attach that one to web_contents.
How do you want me to address this comment?

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_embedder.h:45: // Navigates to a
guest (new or existing).
On 2012/08/27 20:23:59, creis wrote:
> nit: s/to/in/
> 
> Also, please add a blank line above this, since they're not related.

Done.

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_embedder.h:75: // Called when
visiblity of web_contents changes, the embedder would show/hide
On 2012/08/27 20:23:59, creis wrote:
> nit: so the embedder will

Done.

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_embedder.h:78: // Handles input
event sent from the BrowserPlugin (renderer) by passing it to
On 2012/08/27 20:23:59, creis wrote:
> nit: events / passing them
> 
> Also, let's clarify (renderer) as (embedder's renderer process)

Done.

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_embedder.h:88: // Used to monitor
frame navigation to cleanup guests when a frame navigates
On 2012/08/27 20:23:59, creis wrote:
> nit: clean up

Done.

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_embedder.h:99: GuestMap guests_;
On 2012/08/27 20:23:59, creis wrote:
> It's not clear what this map is, especially since it maps to an int64 while
the
> one below maps from an int.  Please improve the name and add a comment.
Renamed the instance to guest_web_contents_containe_, typedef to
GuestWebContentsMap; added comments.

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_embedder_helper.cc (right):

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_embedder_helper.cc:27:
WebContentsImpl* web_contents,
On 2012/08/27 15:35:00, Fady Samuel wrote:
> It seems the only reason BrowserPluginEmbedderHelper needs to know about
> WebContents is for the visibility notification? How about moving that code to
> BrowserPluginEmbedder?

Moved.
Done.

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_guest.cc:39: new
BrowserPluginGuestHelper(this, render_view_host);
On 2012/08/27 20:23:59, creis wrote:
> On 2012/08/27 15:42:51, lazyboy wrote:
> > On 2012/08/25 05:57:09, lazyboy wrote:
> > > On 2012/08/25 05:28:32, Fady Samuel wrote:
> > > > 
> > > > The lifetime of the helper is not equal to the lifetime of the
> > > > BrowserPluginGuest. The helper should be created on
> > > > NOTIFICATION_WEB_CONTENTS_RENDER_VIEW_HOST_CREATED as before, see
> > > > http://codereview.chromium.org/10560022/patch/59102/53234
> > > > 
> > > > We need to create a new helper every time we get a new RenderViewHost
> > > > (cross-process navigation for example).
> > > 
> > > Ah, that's why it was like that, I see.
> > > Now the problem is, at the time BrowserPluginGuest gets created, we
> > would/might
> > > already have the render_view_host created (since we don't create
> > > BrowserPluginGuest at the very beginning of WebContentsImpl anymore) and
we
> > > might not get such notification (at least the first RVH creation one) at
> all?
> > 
> > So my assumption was:
> > embedder loads guest http://a.com, creates guest, gweb_contents, g_RVH
> > embedder navigates the same guest to http://b.com, modifies existing
> gweb_contents to
> > LoadURL http://b.com, *this would create a new RVH other than g_RVH?
> > 
> > I don't see any notification saying RENDER_VIEW_HOST_CREATED in this case
> (both
> > in this code and your old patch)? When does a web_contents create (another)
> new
> > RVH?
> 
> As I mentioned separately, we don't yet do any cross-process swaps for
> navigations in guests.  It might happen in the future, but I'm more concerned
> about this on the embedder side than the guest side for now.

When we switch to new RVH (for embedder case), would we not get a RenderViewGone
call in BrowserPluginEmbedder? Right now we DestroyGuests() from there, but
maybe we can just remove the whole embedder instance from WebContents, that way
next time we get a browser_plugin from renderer process, we would create a new
one (with correct RVH).

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_guest.cc:56:
base::KillProcess(process_handle, RESULT_CODE_HUNG, false);
On 2012/08/27 20:23:59, creis wrote:
> I don't think we've resolved this yet.  I understand that we don't want the
> guest to block the embedder, but automatically killing the guest if it goes
> unresponsive for a bit isn't a good long term plan.
> 
> Darin suggested treating the guest as a black hole for input events so that we
> don't block on it, or we could let them bubble up asynchronously (at the risk
of
> some odd behavior).
> 
> Are either of those feasible?  At the very least, we need a TODO and a heads
up
> to the stability team that this could cause kills, but I'd like to avoid
having
> the kill here if possible.

I talked to rjkroege@ abt this, it seems:
a. would make a page's event handling behave significantly differently if it is
an embedder.
b. it's very hard to make them async (webkit related).
Rob will comment more.

Also which part of killing is the concern (to educate myself): would it report
more kills from chrome in general and reduce some stability score of chrome?

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_guest.cc:212: // TODO(creis,
fsamuel): Ensure this is safe/secure.
On 2012/08/27 20:23:59, creis wrote:
> What is this referring to?

I have no context on this one, it's from Fady's CL, Fady can you comment?
Thanks.

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_guest.cc:238: // TODO(lazyboy):
Rename notification variable if this stays.
On 2012/08/27 20:23:59, creis wrote:
> nit: Wrong indent.

Done.

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_guest.h:37: // the guest role,
implements guest specific overrides for ViewHostMsg_*
On 2012/08/27 20:23:59, creis wrote:
> nit: the guest role and implements

Done.

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_guest.h:75: // browser plugin
{renderer side of the embedder}).
On 2012/08/27 20:23:59, creis wrote:
> nit: Just used nested parens.

Done.

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_guest.h:87: // Handles input event
ack so it's sent to browser plugin host (via embedder)
On 2012/08/27 20:23:59, creis wrote:
> nit: acks so they're sent

Done.

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_guest.h:94: // Used to monitor
frame navigation to cleanup itself up.
On 2012/08/27 20:23:59, creis wrote:
> nit: clean
> 
> (Why does the guest need cleaning up here?  It doesn't seem to match what the
> implementation is doing, so perhaps the comment is stale?)

Right, cleanup is done on embedder's version of DidCommitProvisional..., removed
comment.

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_guest_helper.h (right):

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_guest_helper.h:36: // delegate,
this class exists so we have separation of messages it handles,
On 2012/08/27 20:23:59, creis wrote:
> nit: delegate.  This class
> (or use a semicolon)

Done.

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:79:
shell()->web_contents());
On 2012/08/27 20:23:59, creis wrote:
> nit: Wrong indent.

Done.

http://codereview.chromium.org/10868012/diff/3027/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

http://codereview.chromium.org/10868012/diff/3027/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:2199: // NavigateGuest message
in WebContents via IO thread (by capturing it via
On 2012/08/27 15:35:00, Fady Samuel wrote:
> Why handle NavigateGuest on the I/O thread?

If we can do this in MessageFilter (running in IO) then we can install or not
install the filter based on current mode of the web_contents, (e.g. no such
filter if we are not in platform apps), that way the messages don't arrive at
all in WebContents; maybe this is not necessary, wdyt? (see comment below too
please).

http://codereview.chromium.org/10868012/diff/3027/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:2203: // 'NavigateGuest'
message).
On 2012/08/27 20:23:59, creis wrote:
> I agree, this sounds fragile.  Maybe it's better for the guest to first
> initialize the BrowserPluginEmbedder (on the UI thread) before sending
> NavigateGuest, and only handle NavigateGuest in the BrowserPluginEmbedder?
> 
> (Or, as Fady asks, why make it possible to process this on the IO thread?)

(See comments above too), it sounds like it's better to keep it on UI thread?
I can remove the todo.

http://codereview.chromium.org/10868012/diff/3027/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:3184:
content::BrowserPluginGuest* WebContentsImpl::AddBrowserPluginGuest(
On 2012/08/27 15:35:00, Fady Samuel wrote:
> I think SetBrowserPluginGuest might be a better name because a WebContents is
> either a guest or its not, it does not keep track of more than one guest.

Done.

http://codereview.chromium.org/10868012/diff/3027/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.h (right):

http://codereview.chromium.org/10868012/diff/3027/content/browser/web_content...
content/browser/web_contents/web_contents_impl.h:812: // Browser plugin embedder
for WebContents.
On 2012/08/27 20:23:59, creis wrote:
> These comments are redundant with the class name.  Maybe add more useful info,
> like "Manages the embedder state for browser plugins, if this WebContents is
an
> embedder.  Null otherwise."

Done.

http://codereview.chromium.org/10868012/diff/3027/content/renderer/browser_pl...
File content/renderer/browser_plugin/browser_plugin.cc (right):

http://codereview.chromium.org/10868012/diff/3027/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:334: // send UpdateRect
messages to the host.
On 2012/08/27 20:23:59, creis wrote:
> Can we determine if this is correct or not, rather than committing with the
> TODO?

Fady can you asnwer this one?:
I've added this so empty src doesn't trigger UpdateRect to browser process.
Initially on page when there's plugin with src="", we don't send any
NavigateGuest_Msg and the browser side yet doesn't have any browser plugin
embedder. Now if we send UpdateRect_Msg from here it will hang since we won't
receive any reply (no browser_plugin_embedder message handler exists).

http://codereview.chromium.org/10868012/diff/3027/content/renderer/renderer_m...
File content/renderer/renderer_main.cc (right):

http://codereview.chromium.org/10868012/diff/3027/content/renderer/renderer_m...
content/renderer/renderer_main.cc:217: new content::BrowserPluginManagerImpl();
On 2012/08/27 20:23:59, creis wrote:
> Why is this necessary, but wasn't before?  Just a bug?

I believe this is what triggers the renderer side to turn on/listen to messages
(copied from Fady's CL).

Charlie Reis

Ok. Answered a few questions and added a few nits, but mainly waiting on resolutions ...

8 years, 3 months ago (2012-08-29 00:11:04 UTC) #25

Ok.  Answered a few questions and added a few nits, but mainly waiting on
resolutions for the things below.

On 2012/08/28 19:07:14, lazyboy wrote:
> *I am still working on making unit tests work without NOTIFICATION_* (having
> linker errors on unrelated issues :()
> 
> recap/issues_to_resolve for this round:
> 
> 1. Issue when RVH switches in a WC to a different one for embedder. I've not
> made any change for this, have more questions. (This relates to both creis and
> fsamuel's comments)

Hope you have enough to fix this now.

> 2. re: Killing process when guest hangs on input, rjkroege@ to follow up more.

Waiting to hear a plan here.

> 3. There are several places I would require fsamuel@ to answer, please help!
> (Search for word 'Fady' in comments).
> 
> Thanks

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_embedder.h:39: RenderViewHost*
render_view_host);
On 2012/08/28 19:07:14, lazyboy wrote:
> On 2012/08/27 20:23:59, creis wrote:
> > This suggests that we'll need to update our RVH if |web_contents| does a
> > cross-process navigation.
> Or create replace with a new embedder and attach that one to web_contents.
> How do you want me to address this comment?

Since we don't really need to preserve anything when you navigate away and back,
I like your idea of just destroying the BrowserPluginEmbedder and creating a new
one.  Keep in mind that we may not get a RenderViewGone when swapping RVHs
(e.g., the other process may still be alive).  I'm guessing we'll have to handle
both swaps (e.g., something like NOTIFICATION_WEB_CONTENTS_SWAPPED) and
RenderViewGone.

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_guest.cc:39: new
BrowserPluginGuestHelper(this, render_view_host);
On 2012/08/28 19:07:14, lazyboy wrote:
> On 2012/08/27 20:23:59, creis wrote:
> > On 2012/08/27 15:42:51, lazyboy wrote:
> > > On 2012/08/25 05:57:09, lazyboy wrote:
> > > > On 2012/08/25 05:28:32, Fady Samuel wrote:
> > > > > 
> > > > > The lifetime of the helper is not equal to the lifetime of the
> > > > > BrowserPluginGuest. The helper should be created on
> > > > > NOTIFICATION_WEB_CONTENTS_RENDER_VIEW_HOST_CREATED as before, see
> > > > > http://codereview.chromium.org/10560022/patch/59102/53234
> > > > > 
> > > > > We need to create a new helper every time we get a new RenderViewHost
> > > > > (cross-process navigation for example).
> > > > 
> > > > Ah, that's why it was like that, I see.
> > > > Now the problem is, at the time BrowserPluginGuest gets created, we
> > > would/might
> > > > already have the render_view_host created (since we don't create
> > > > BrowserPluginGuest at the very beginning of WebContentsImpl anymore) and
> we
> > > > might not get such notification (at least the first RVH creation one) at
> > all?
> > > 
> > > So my assumption was:
> > > embedder loads guest http://a.com, creates guest, gweb_contents, g_RVH
> > > embedder navigates the same guest to http://b.com, modifies existing
> > gweb_contents to
> > > LoadURL http://b.com, *this would create a new RVH other than g_RVH?
> > > 
> > > I don't see any notification saying RENDER_VIEW_HOST_CREATED in this case
> > (both
> > > in this code and your old patch)? When does a web_contents create
(another)
> > new
> > > RVH?
> > 
> > As I mentioned separately, we don't yet do any cross-process swaps for
> > navigations in guests.  It might happen in the future, but I'm more
concerned
> > about this on the embedder side than the guest side for now.
> 
> When we switch to new RVH (for embedder case), would we not get a
RenderViewGone
> call in BrowserPluginEmbedder? Right now we DestroyGuests() from there, but
> maybe we can just remove the whole embedder instance from WebContents, that
way
> next time we get a browser_plugin from renderer process, we would create a new
> one (with correct RVH).

Sounds like you're talking about the embedder here, so let's move that
discussion to BrowserPluginEmbedder.

For BrowserPluginGuest, we'll need a CHECK or DCHECK somewhere to ensure that we
don't swap RVHs.  Maybe that's something like listening for
NOTIFICATION_WEB_CONTENTS_SWAPPED?

http://codereview.chromium.org/10868012/diff/3027/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

http://codereview.chromium.org/10868012/diff/3027/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:2199: // NavigateGuest message
in WebContents via IO thread (by capturing it via
On 2012/08/28 19:07:14, lazyboy wrote:
> On 2012/08/27 15:35:00, Fady Samuel wrote:
> > Why handle NavigateGuest on the I/O thread?
> 
> If we can do this in MessageFilter (running in IO) then we can install or not
> install the filter based on current mode of the web_contents, (e.g. no such
> filter if we are not in platform apps), that way the messages don't arrive at
> all in WebContents; maybe this is not necessary, wdyt? (see comment below too
> please).

Yeah, a race is worse than an unnecessary filter, in my opinion.

http://codereview.chromium.org/10868012/diff/17001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

http://codereview.chromium.org/10868012/diff/17001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.h:107: void
WebContentsVisiblitlyChanged(bool visible);
nit: visibility is spelled wrong (also in the comment above).

http://codereview.chromium.org/10868012/diff/17001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

http://codereview.chromium.org/10868012/diff/17001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:238: // TODO(lazyboy):
Rename notification variable if this stays.
This indent is wrong again.  :)

Before, I meant the comment was indented one space too far.  Now, everything
needs another two spaces.  (Four space indent for unfinished lines.)

http://codereview.chromium.org/10868012/diff/17001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

http://codereview.chromium.org/10868012/diff/17001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.h:107: // An identifier that
uniquely identifies a browser plugin container within an
What's a browser plugin container?  Do you mean a guest?

http://codereview.chromium.org/10868012/diff/17001/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.cc (right):

http://codereview.chromium.org/10868012/diff/17001/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.cc:3195:
content::BrowserPluginEmbedder* WebContentsImpl::
nit: Please move the WebContentsImpl:: part to the next line, to avoid breaking
on ::.

http://codereview.chromium.org/10868012/diff/17001/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.h (right):

http://codereview.chromium.org/10868012/diff/17001/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.h:159: // Returns guest browser
plugin object, NULL if this WebContents is not a
nit: or NULL (same below)

http://codereview.chromium.org/10868012/diff/17001/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.h:813: // embedder, NULL
otherwise.
nit: semicolon (here and below).

Fady Samuel

Hi Istiaque, I believe I've answered your questions. Please email me if you have any ...

8 years, 3 months ago (2012-08-29 08:16:20 UTC) #26

Hi Istiaque,

I believe I've answered your questions. Please email me if you have any more.
Thanks!

Fady

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_embedder.cc:91:
guest->web_contents()->SetDelegate(guest);
On 2012/08/28 19:07:14, lazyboy wrote:
> On 2012/08/27 20:23:59, creis wrote:
> > Do we need to set this again if the guest already existed, or should we move
> it
> > above the else block?
> 
> Moved.
> Also this is a bit weird this part has guest_web_contents and
> guest->web_contents() in many places; Fady, wouldn't they refer to the same
> thing?

Yea, it doesn't look like the local variable guest_web_contents is necessary any
more! Thanks!

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_guest.cc:212: // TODO(creis,
fsamuel): Ensure this is safe/secure.
On 2012/08/28 19:07:14, lazyboy wrote:
> On 2012/08/27 20:23:59, creis wrote:
> > What is this referring to?
> 
> I have no context on this one, it's from Fady's CL, Fady can you comment?
> Thanks.

At some point Charlie expressed a concern that passing the guest's URL to the
embedder might be unsafe. I'm really not sure. Charlie, if this code looks OK to
you, I'm OK with removing this TODO.

http://codereview.chromium.org/10868012/diff/3027/content/renderer/browser_pl...
File content/renderer/browser_plugin/browser_plugin.cc (right):

http://codereview.chromium.org/10868012/diff/3027/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:334: // send UpdateRect
messages to the host.
On 2012/08/28 19:07:14, lazyboy wrote:
> On 2012/08/27 20:23:59, creis wrote:
> > Can we determine if this is correct or not, rather than committing with the
> > TODO?
> 
> Fady can you asnwer this one?:
> I've added this so empty src doesn't trigger UpdateRect to browser process.
> Initially on page when there's plugin with src="", we don't send any
> NavigateGuest_Msg and the browser side yet doesn't have any browser plugin
> embedder. Now if we send UpdateRect_Msg from here it will hang since we won't
> receive any reply (no browser_plugin_embedder message handler exists).

So when the src attribute is not set, we may or may not have a
BrowserPluginGuest. If we have a BrowserPluginGuest, its WebContents doesn't
need to be resized because we're not doing anything with it. If we don't have a
BrowserPluginGuest, we really shouldn't be creating one just to resize it. This
seems reasonable to me.

A point Sean made a couple of weeks ago that may be worth considering is what
happens if we navigate to a page (say http://www.google.com) and then set
src="". Should we keep a BrowserPluginGuest, and the old RVH lingering around
while src=""? Certainly, js might still be running on that old page, and it
might be sending updated pixels to the BrowserPlugin, which we're discarding.
This seems like a waste of cycles? Perhaps setting src="" should swap out the
guest RenderView?

Another concern (I can't remember if I tested this or not, it might be worth
writing a test for this) is the following:

1. set src="http://www.google.com" size (640, 480)
2. set src=""
3. set size (400, 400)
4. Navigate to "http://www.chromium.org"

Will chromium.org layout in the correct (400, 400) size?

I think I fixed this right before vacation (NavigateGuest takes a size now
right?) but could you please verify this?

http://codereview.chromium.org/10868012/diff/3027/content/renderer/renderer_m...
File content/renderer/renderer_main.cc (right):

http://codereview.chromium.org/10868012/diff/3027/content/renderer/renderer_m...
content/renderer/renderer_main.cc:217: new content::BrowserPluginManagerImpl();
On 2012/08/28 19:07:14, lazyboy wrote:
> On 2012/08/27 20:23:59, creis wrote:
> > Why is this necessary, but wasn't before?  Just a bug?
> 
> I believe this is what triggers the renderer side to turn on/listen to
messages
> (copied from Fady's CL).

Yup, there was no BrowserPluginManager instantiated in the renderer side CL (we
only created one in the test).

Fady Samuel

One small nit. BTW, very cool factory implementation! http://codereview.chromium.org/10868012/diff/18029/content/browser/web_contents/web_contents_impl.cc File content/browser/web_contents/web_contents_impl.cc (right): http://codereview.chromium.org/10868012/diff/18029/content/browser/web_contents/web_contents_impl.cc#newcode2205 content/browser/web_contents/web_contents_impl.cc:2205: content::BrowserPluginEmbedder::CreateInstance(this, ...

8 years, 3 months ago (2012-08-29 08:29:14 UTC) #27

Fady Samuel

Sorry, one more other nit, it looks like you're using more than one MessageLoopRunner. Is ...

8 years, 3 months ago (2012-08-29 08:53:07 UTC) #28

Charlie Reis

(I still have some unaddressed comments in patch set 6, too, which I'm guessing you ...

8 years, 3 months ago (2012-08-29 18:08:11 UTC) #29

(I still have some unaddressed comments in patch set 6, too, which I'm guessing
you just haven't gotten to yet.  Let me know when it's ready for another pass.)

Thanks!
Charlie

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_guest.cc:212: // TODO(creis,
fsamuel): Ensure this is safe/secure.
On 2012/08/29 08:16:20, Fady Samuel wrote:
> On 2012/08/28 19:07:14, lazyboy wrote:
> > On 2012/08/27 20:23:59, creis wrote:
> > > What is this referring to?
> > 
> > I have no context on this one, it's from Fady's CL, Fady can you comment?
> > Thanks.
> 
> At some point Charlie expressed a concern that passing the guest's URL to the
> embedder might be unsafe. I'm really not sure. Charlie, if this code looks OK
to
> you, I'm OK with removing this TODO.

I can't find the context for this.  Now that we assume the embedder has control
over the guest's content, I don't see the risk of providing the guest's URL to
the embedder.  I think we can remove this TODO.

http://codereview.chromium.org/10868012/diff/3027/content/renderer/browser_pl...
File content/renderer/browser_plugin/browser_plugin.cc (right):

http://codereview.chromium.org/10868012/diff/3027/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:334: // send UpdateRect
messages to the host.
On 2012/08/29 08:16:20, Fady Samuel wrote:
> On 2012/08/28 19:07:14, lazyboy wrote:
> > On 2012/08/27 20:23:59, creis wrote:
> > > Can we determine if this is correct or not, rather than committing with
the
> > > TODO?
> > 
> > Fady can you asnwer this one?:
> > I've added this so empty src doesn't trigger UpdateRect to browser process.
> > Initially on page when there's plugin with src="", we don't send any
> > NavigateGuest_Msg and the browser side yet doesn't have any browser plugin
> > embedder. Now if we send UpdateRect_Msg from here it will hang since we
won't
> > receive any reply (no browser_plugin_embedder message handler exists).
> 
> So when the src attribute is not set, we may or may not have a
> BrowserPluginGuest. If we have a BrowserPluginGuest, its WebContents doesn't
> need to be resized because we're not doing anything with it. If we don't have
a
> BrowserPluginGuest, we really shouldn't be creating one just to resize it.
This
> seems reasonable to me.
> 
> A point Sean made a couple of weeks ago that may be worth considering is what
> happens if we navigate to a page (say http://www.google.com) and then set
> src="". Should we keep a BrowserPluginGuest, and the old RVH lingering around
> while src=""? Certainly, js might still be running on that old page, and it
> might be sending updated pixels to the BrowserPlugin, which we're discarding.
> This seems like a waste of cycles? Perhaps setting src="" should swap out the
> guest RenderView?
> 
> Another concern (I can't remember if I tested this or not, it might be worth
> writing a test for this) is the following:
> 
> 1. set src="http://www.google.com" size (640, 480)
> 2. set src=""
> 3. set size (400, 400)
> 4. Navigate to "http://www.chromium.org"
> 
> Will http://chromium.org layout in the correct (400, 400) size?
> 
> I think I fixed this right before vacation (NavigateGuest takes a size now
> right?) but could you please verify this?

As for Sean's question, I think manually assigning src to "" should cause a
navigation to about:blank, not swap out the guest.  (A swapped out RVH should
never be visible.)

lazyboy

1. So I tried deleting browser_plugin_embedder_ when we create a new RVH in WebContents so ...

8 years, 3 months ago (2012-08-29 21:24:44 UTC) #30

1. So I tried deleting browser_plugin_embedder_ when we create a new RVH in
WebContents so we have new embedder for different RVH, but no luck.

This is what I tried:
The place where WebContentsImpl sends
Nofity(WEB_CONTENTS_RENDER_VIEW_HOST_CREATED) and Notify(WEB_CONTENTS_SWAPPED),
I check if browser_plugin_embedder_ corresponds to current RVH in WC, if not, I
just delete it (so the next NavigateGuest message from the new page would create
correct browser_plugin_embedder_ in the WC).

Seems my assumption around RVH creation is flawed, I don't see what I'm
expecting.

I see WEB_CONTENTS_SWAPPED only if I press back button in browser;
(NTP->a.com->press back).
Unexpected of all was: if I'm in a.com with an embedder, and there is a link <a
href="b.com"/>, clicking on it doesn't even create a new RVH.

Can you give me hints how to achieve this?

2. re: Events, I asked Rob to comment (his main comment was it would be possible
if webkit events were possible to make reentrant), he is flying to MTV at this
moment, hopefully will respond soon.

3. The CL is updated with tests not using NOTIFICATION_ anymore (turned out to
be a bigger change).

4. Also can you suggest other reviewer to continue reviewing this?

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

http://codereview.chromium.org/10868012/diff/3027/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_guest.cc:212: // TODO(creis,
fsamuel): Ensure this is safe/secure.
On 2012/08/29 18:08:12, creis wrote:
> On 2012/08/29 08:16:20, Fady Samuel wrote:
> > On 2012/08/28 19:07:14, lazyboy wrote:
> > > On 2012/08/27 20:23:59, creis wrote:
> > > > What is this referring to?
> > > 
> > > I have no context on this one, it's from Fady's CL, Fady can you comment?
> > > Thanks.
> > 
> > At some point Charlie expressed a concern that passing the guest's URL to
the
> > embedder might be unsafe. I'm really not sure. Charlie, if this code looks
OK
> to
> > you, I'm OK with removing this TODO.
> 
> I can't find the context for this.  Now that we assume the embedder has
control
> over the guest's content, I don't see the risk of providing the guest's URL to
> the embedder.  I think we can remove this TODO.

Removed.

http://codereview.chromium.org/10868012/diff/17001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

http://codereview.chromium.org/10868012/diff/17001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.h:107: void
WebContentsVisiblitlyChanged(bool visible);
On 2012/08/29 00:11:04, creis wrote:
> nit: visibility is spelled wrong (also in the comment above).

Done.

http://codereview.chromium.org/10868012/diff/17001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

http://codereview.chromium.org/10868012/diff/17001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:238: // TODO(lazyboy):
Rename notification variable if this stays.
On 2012/08/29 00:11:04, creis wrote:
> This indent is wrong again.  :)
> 
> Before, I meant the comment was indented one space too far.  Now, everything
> needs another two spaces.  (Four space indent for unfinished lines.)

Sorry about that, I guess I was paying less attention to this file since I was
in the process of reverting this.
Reverted.

http://codereview.chromium.org/10868012/diff/17001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

http://codereview.chromium.org/10868012/diff/17001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.h:107: // An identifier that
uniquely identifies a browser plugin container within an
On 2012/08/29 00:11:04, creis wrote:
> What's a browser plugin container?  Do you mean a guest?
Right, stale comments, fixed.

http://codereview.chromium.org/10868012/diff/17001/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.h (right):

http://codereview.chromium.org/10868012/diff/17001/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.h:159: // Returns guest browser
plugin object, NULL if this WebContents is not a
On 2012/08/29 00:11:04, creis wrote:
> nit: or NULL (same below)

Done.

http://codereview.chromium.org/10868012/diff/17001/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.h:813: // embedder, NULL
otherwise.
On 2012/08/29 00:11:04, creis wrote:
> nit: semicolon (here and below).

Done.

http://codereview.chromium.org/10868012/diff/18029/content/browser/browser_pl...
File content/browser/browser_plugin/test_browser_plugin_embedder.cc (right):

http://codereview.chromium.org/10868012/diff/18029/content/browser/browser_pl...
content/browser/browser_plugin/test_browser_plugin_embedder.cc:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
On 2012/08/29 08:53:07, Fady Samuel wrote:
> I believe the typical naming scheme would MockBrowserPluginEmbedder. 
So these are not mock objects, they are decorated versions of actual objects,
wouldn't naming them MockFoo be confusing?

http://codereview.chromium.org/10868012/diff/18029/content/browser/browser_pl...
File content/browser/browser_plugin/test_browser_plugin_guest.h (right):

http://codereview.chromium.org/10868012/diff/18029/content/browser/browser_pl...
content/browser/browser_plugin/test_browser_plugin_guest.h:46:
scoped_refptr<MessageLoopRunner> send_message_loop_runner_;
On 2012/08/29 08:53:07, Fady Samuel wrote:
> Do we need to use two message loop runners?

Using two message loop is clearer and error-free since, the condition of one
that stops the messaage loop might interfere with the other (getting update rect
while waiting for crash would then require another variable to track that it is
waiting for crash and should not stop the message loop), I thought this one is
clearer, let me know if you think using one message loop is better.

http://codereview.chromium.org/10868012/diff/18029/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.cc (right):

http://codereview.chromium.org/10868012/diff/18029/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.cc:2205:
content::BrowserPluginEmbedder::CreateInstance(this,
On 2012/08/29 08:29:14, Fady Samuel wrote:
> The typical naming for these factory methods is just "Create" rather than
> "CreateInstance".

Done.

lazyboy

(missed one comment) http://chromiumcodereview.appspot.com/10868012/diff/17001/content/browser/web_contents/web_contents_impl.cc File content/browser/web_contents/web_contents_impl.cc (right): http://chromiumcodereview.appspot.com/10868012/diff/17001/content/browser/web_contents/web_contents_impl.cc#newcode3195 content/browser/web_contents/web_contents_impl.cc:3195: content::BrowserPluginEmbedder* WebContentsImpl:: On 2012/08/29 00:11:04, creis ...

8 years, 3 months ago (2012-08-29 21:37:25 UTC) #31

Charlie Reis

Thanks. Things are looking better, and I'm happy with this landing once we've resolved the ...

8 years, 3 months ago (2012-08-30 01:01:51 UTC) #32

Thanks.  Things are looking better, and I'm happy with this landing once we've
resolved the remaining questions.  A few responses below.

On 2012/08/29 21:24:44, lazyboy wrote:
> 1. So I tried deleting browser_plugin_embedder_ when we create a new RVH in
> WebContents so we have new embedder for different RVH, but no luck.
> 
> This is what I tried:
> The place where WebContentsImpl sends
> Nofity(WEB_CONTENTS_RENDER_VIEW_HOST_CREATED) and
Notify(WEB_CONTENTS_SWAPPED),
> I check if browser_plugin_embedder_ corresponds to current RVH in WC, if not,
I
> just delete it (so the next NavigateGuest message from the new page would
create
> correct browser_plugin_embedder_ in the WC).
> 
> Seems my assumption around RVH creation is flawed, I don't see what I'm
> expecting.
> 
> I see WEB_CONTENTS_SWAPPED only if I press back button in browser;
> (NTP->a.com->press back).
> Unexpected of all was: if I'm in http://a.com with an embedder, and there is a
link <a
> href="b.com"/>, clicking on it doesn't even create a new RVH.
> 
> Can you give me hints how to achieve this?

That's all expected behavior.  We don't swap processes on renderer-initiated
cross-site navigations, for lots of compatibility reasons.  (It's a long term
goal to be able to do that.)  Gory details in the "Caveats" section of the
Process Models documentation:
http://dev.chromium.org/developers/design-documents/process-models

You'll see process swaps if you type a cross-site URL into the omnibox, or if
you make any kind of navigation across different types of pages (like normal
page <-> NTP).

> 2. re: Events, I asked Rob to comment (his main comment was it would be
possible
> if webkit events were possible to make reentrant), he is flying to MTV at this
> moment, hopefully will respond soon.

Hmm, no response and I'm heading out soon.  Please see what you can do to avoid
this, but worst case, file a bug to find an approach that doesn't automatically
kill renderers and include a TODO comment with the bug number.

> 3. The CL is updated with tests not using NOTIFICATION_ anymore (turned out to
> be a bigger change).

Great.

> 4. Also can you suggest other reviewer to continue reviewing this?

I'm adding Albert Wong as a reviewer, since he's generally aware of what we're
doing and knows this area of code well.  He can pull in additional reviewers as
needed.

Albert, the remaining issues I'm aware of are:
 - Avoiding automatic kills of guests after 5 seconds
 - Ensuring BPE is cleared after RVH swaps
 - Ensuring guests from subframes are cleaned up if their parent frame navigates
or is detached
 - Navigating to about:blank on src=""
 - Avoiding race in WebContentsImpl::OnBrowserPluginNavigateGuest (see TODO
discussion)

Thanks all.  I'll be away from email until September 12th.  Hope you're able to
wrap it up!

Charlie

lazyboy

This should be good to be reviewed again. Albert can you take a look, thanks! ...

8 years, 3 months ago (2012-08-31 00:38:04 UTC) #33

This should be good to be reviewed again.
Albert can you take a look, thanks!

Handled all but one issues:
 - Avoiding automatic kills of guests after 5 seconds
*not* Done, related to input event replies waiting from rjkroege@
 - Ensuring BPE is cleared after RVH swaps
Done, see web_contents_impl changes.
 - Ensuring guests from subframes are cleaned up if their parent frame navigates
or is detached
Done (I believe so), debugged few cases, guests seems to be properly cleaning
up, but I might have missed other cases. More in
BrowserPluginEmbedder.DidCommitProvisionalLoadForFrame.
 - Navigating to about:blank on src=""
Done.
 - Avoiding race in WebContentsImpl::OnBrowserPluginNavigateGuest (see TODO
discussion)
Done. This is not a race in the current code, it would be if we implemented the
TODO, since none of creis/fsamuel thinks the TODO is worth the complexity, I've
removed the TODO.

https://chromiumcodereview.appspot.com/10868012/diff/3027/content/browser/bro...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/3027/content/browser/bro...
content/browser/browser_plugin/browser_plugin_embedder.cc:91:
guest->web_contents()->SetDelegate(guest);
On 2012/08/29 08:16:20, Fady Samuel wrote:
> On 2012/08/28 19:07:14, lazyboy wrote:
> > On 2012/08/27 20:23:59, creis wrote:
> > > Do we need to set this again if the guest already existed, or should we
move
> > it
> > > above the else block?
> > 
> > Moved.
> > Also this is a bit weird this part has guest_web_contents and
> > guest->web_contents() in many places; Fady, wouldn't they refer to the same
> > thing?
> 
> Yea, it doesn't look like the local variable guest_web_contents is necessary
any
> more! Thanks!

Done.

https://chromiumcodereview.appspot.com/10868012/diff/3027/content/browser/web...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/3027/content/browser/web...
content/browser/web_contents/web_contents_impl.cc:2199: // NavigateGuest message
in WebContents via IO thread (by capturing it via
On 2012/08/29 00:11:04, creis wrote:
> On 2012/08/28 19:07:14, lazyboy wrote:
> > On 2012/08/27 15:35:00, Fady Samuel wrote:
> > > Why handle NavigateGuest on the I/O thread?
> > 
> > If we can do this in MessageFilter (running in IO) then we can install or
not
> > install the filter based on current mode of the web_contents, (e.g. no such
> > filter if we are not in platform apps), that way the messages don't arrive
at
> > all in WebContents; maybe this is not necessary, wdyt? (see comment below
too
> > please).
> 
> Yeah, a race is worse than an unnecessary filter, in my opinion.

I've removed the todo since moving this to a filter will introduce race
condition.

https://chromiumcodereview.appspot.com/10868012/diff/3027/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/3027/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:334: // send UpdateRect
messages to the host.
On 2012/08/29 18:08:12, creis wrote:
> On 2012/08/29 08:16:20, Fady Samuel wrote:
> > On 2012/08/28 19:07:14, lazyboy wrote:
> > > On 2012/08/27 20:23:59, creis wrote:
> > > > Can we determine if this is correct or not, rather than committing with
> the
> > > > TODO?
> > > 
> > > Fady can you asnwer this one?:
> > > I've added this so empty src doesn't trigger UpdateRect to browser
process.
> > > Initially on page when there's plugin with src="", we don't send any
> > > NavigateGuest_Msg and the browser side yet doesn't have any browser plugin
> > > embedder. Now if we send UpdateRect_Msg from here it will hang since we
> won't
> > > receive any reply (no browser_plugin_embedder message handler exists).
> > 
> > So when the src attribute is not set, we may or may not have a
> > BrowserPluginGuest. If we have a BrowserPluginGuest, its WebContents doesn't
> > need to be resized because we're not doing anything with it. If we don't
have
> a
> > BrowserPluginGuest, we really shouldn't be creating one just to resize it.
> This
> > seems reasonable to me.
> > 
> > A point Sean made a couple of weeks ago that may be worth considering is
what
> > happens if we navigate to a page (say http://www.google.com) and then set
> > src="". Should we keep a BrowserPluginGuest, and the old RVH lingering
around
> > while src=""? Certainly, js might still be running on that old page, and it
> > might be sending updated pixels to the BrowserPlugin, which we're
discarding.
> > This seems like a waste of cycles? Perhaps setting src="" should swap out
the
> > guest RenderView?
> > 
> > Another concern (I can't remember if I tested this or not, it might be worth
> > writing a test for this) is the following:
> > 
> > 1. set src="http://www.google.com" size (640, 480)
> > 2. set src=""
> > 3. set size (400, 400)
> > 4. Navigate to "http://www.chromium.org"
> > 
> > Will http://chromium.org layout in the correct (400, 400) size?
> > 
> > I think I fixed this right before vacation (NavigateGuest takes a size now
> > right?) but could you please verify this?
> 
> As for Sean's question, I think manually assigning src to "" should cause a
> navigation to about:blank, not swap out the guest.  (A swapped out RVH should
> never be visible.)

I've resolved this issue.

Ignoring everything when the src="" is not sufficient since Fady's case won't be
handled properly (because if have special empty src case handling in
BrowserPluginGuest::SetSrcAttribute(..)

Right now I ignore only initial empty src attributes; All other src attributes
(including empty ones) after a proper navigate (/w non empty src) are always
sent to embedder.

Tested with the test case scenario mentioned by Fady, works well now. Also
checked without the step 3 (to make sure resize isn't forcing some kind of
painting).

https://chromiumcodereview.appspot.com/10868012/diff/37004/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/37004/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:50: // Destroy guests
that are managed by the current embedder, for more details
We are destroying all guests in destructor now, this should take care of one
case of cleaning up guests inside embedder's frame (see note in
DidCommitProvisionalLoadForFrame)

awong

FYI, I'm actually on vacation today, and then monday is a US holiday so I ...

8 years, 3 months ago (2012-08-31 18:08:08 UTC) #34

lazyboy

Addressing nits... re: which thread this classes should be used on: If I understand your ...

8 years, 3 months ago (2012-08-31 18:49:18 UTC) #35

awong

I've made a pass through the code and I think I understand it well enough ...

8 years, 3 months ago (2012-09-06 00:23:27 UTC) #36

lazyboy

I've run cpplint.py on all the new files. Deferring to fsamuel@ for question/comment about RWHImpl::ResetFlags(). ...

8 years, 3 months ago (2012-09-06 04:33:53 UTC) #37

awong

I think I'm getting closer to feeling comfortable with this, but still need another pass ...

8 years, 3 months ago (2012-09-06 19:55:26 UTC) #38

I think I'm getting closer to feeling comfortable with this, but still need
another pass (sorry...this is a lot of code to ingest at once).

Added some more comments.

One high level piece of feedback is that there the class documentation is
missing details on how they are supposed to interact with each other.  This
system is complicated enough that I would have liked to see 2 things:
  1) On some class (Probably BrowserPluginGuest) a large block of documentation
explaining how all these classes interact, who owns what, which main messages we
care about, and what invariants are guiding this design. I know there's a design
doc, but having it baked into the code is preferred. Example of such interaction
documentation would be stuff like:

   webkit/media/webmediaplayer_impl.h
   content/public/browser/site_instance.h

Then at various points where you introduce complex concepts into big classes
(eg., the fact that a WebContents can be a guest, an embedder, or none) you can
just put a comment saying "For more info, see comment above XXX class".

2) Each class needs to have slightly more documentation in its class comment
explaining its lifetime, which thread its associated with, and any other
subtleties with messages it expects to receive when, etc.

More later.

https://chromiumcodereview.appspot.com/10868012/diff/47001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/47001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:162:
guest_web_contents_container_.begin();
On 2012/09/06 04:33:53, lazyboy wrote:
> On 2012/09/06 00:23:27, awong wrote:
> > line continuations should be 4 spaces indented from previous indent level.
> > 
> > Though maybe just use STLDeleteContainerPairFirstPointers() from
> > base/stl_util.h?
> 
> Done.
>  
> Is the style guide for "for (...)" indentation is 4 spaces or matching "("?,
> i.e.:
> for (i = 0;
>     i < 100;...
> or
> for (i = 0;
>      i < 100;...
> ?

The latter.  What I meant was that the guest_web_contents_container_.begin()
should be 4 spaces further in.  See new comment in
DidCommitProvisionLoadForFrame.

Sorry for being unclear.

https://chromiumcodereview.appspot.com/10868012/diff/47001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/47001/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:70:
base::KillProcess(process_handle, RESULT_CODE_HUNG, false);
On 2012/09/06 04:33:53, lazyboy wrote:
> On 2012/09/06 00:23:27, awong wrote:
> > Charlie mentioned that 5-second automatic kill is undesirable behavior.  Is
> > there a BUG that should be included in a TODO here for finding a better
> > behavior?
> 
> This one is related to guest's input event hanging embedder, from what I know
> it's not possible to do without making webkit input events reentrant. Let's
get
> back to this when you have done next detailed pass, I can add a bug as Charlie
> suggested in that case. 

Right. Let's file a bug then and also add a comment (probably where you set the
input even hang timer) that explains why we are must do a full process kill
here.  BTW, do we need to track stats for how often we get a kill cause of an
input event?

https://chromiumcodereview.appspot.com/10868012/diff/47001/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:163:
message->WriteData(input_buffer.get(), event.size);
On 2012/09/06 04:33:53, lazyboy wrote:
> On 2012/09/06 00:23:27, awong wrote:
> > To make this all consistent, maybe use input_event and input_event->size?
> 
> Sorry did you mean to use message->WriteData(input_event, input_event->size)?
> Pickle::WriteData requires first param to be char*.

Gotcha. Makes sense.

https://chromiumcodereview.appspot.com/10868012/diff/47001/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:172:
base::TimeDelta::FromMilliseconds(kGuestHangTimeout));
On 2012/09/06 04:33:53, lazyboy wrote:
> On 2012/09/06 00:23:27, awong wrote:
> > If a single guest doesn't respond, won't this kill the whole process? Are
> there
> > cases where a single guest might be slow to respond, but the rest of the
> process
> > is still responsive?
> 
> If a guest's hang monitor times out, won't that make RenderereUnresponsive()
> only for that guest's RenderViewHost? By 'reset of the process' do you mean
> other guests in the same embedder?

What I specifically meant was guests in the same renderer process. I think for
browser tag this is restricted to your case of others guests in the same
embedder.  This is related to the 5-second kill on hung input that we were
talking about earlier.

https://chromiumcodereview.appspot.com/10868012/diff/47001/content/browser/we...
File content/browser/web_contents/web_contents_impl.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/47001/content/browser/we...
content/browser/web_contents/web_contents_impl.h:166: // Sets guest to this
WebContents embedder.
On 2012/09/06 04:33:53, lazyboy wrote:
> On 2012/09/06 00:23:27, awong wrote:
> > I expect a SetXXX() function to take an object that gets put into a member
> > variable and to be symmetrical with the GetXXX functions.
> > 
> > But this function seems to be creating the guest.  Is it more like
> > AssignBrowserPluingGuest?
> > 
> > The comment also doesn't quite explain the functionality. BTW, is it okay to
> > call this function twice?
> 
> Renamed the function.
> There should be no side effect calling it > 1. We don't expect it to be called
> more than once for a given web_contents though.

Ah, I see.  Though looking at this more, this seems like it should be a
construction. A WebContents never switches between being a guest, and being
{normal,embedder}.

What about making a WebContents::CreateGuest() constructor?  You wouldn't even
need the base_web_contents and could move the messy custom SiteInstance creation
logic into there so you just take the host's GURL.

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:209: // points to a
page that has an iframe. Call the iframe's WebCotnents WC2. Now
nit: WebCtnents -> WebContents

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:227:
guest_web_contents_container_.begin();
guest_web_contents_container_.begin() should be indented 4 more spaces because
it is a continuation of the initializer statement portion of the for loop.

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:90: // factor.
If the scale factor is unequal (I presume this can happen if the guest navigates
to a domain that has a different host zoom setting?) won't this leave the guest
with a stale image?

Should be a bug?

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:106: // and
eventually gets killed.
Doesn't this guarantee a 5 second wait in the browser test? If so, is there a
way we can override the hang time in tests ideally based on the test_timeouts.h?
5s is a long time in a test...

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
File content/browser/browser_plugin/browser_plugin_host_factory.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
content/browser/browser_plugin/browser_plugin_host_factory.h:32: RenderViewHost*
render_view_host) = 0;
Nice catch on the virtual destructor :)

nit: add a newline above private:

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/we...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/we...
content/browser/web_contents/web_contents_impl.cc:2463:
RemoveSwappedOutBrowserPluginEmbedder();
Can we add a test asserting this behavior, both that a non-swapping load doesn't
change the browser_plugin_embedder_ and that a swapping load does change it.

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/we...
content/browser/web_contents/web_contents_impl.cc:2511:
!browser_plugin_embedder_->IsForRenderViewHost(GetRenderViewHost())) {
Is there a case where you get a swapped signal, have a previous
browser_plugin_embedder_, and have it be the same RVH?  What's the sequence?
browser_plugin_embedder_ somehow gets created before the provisional load of the
top-level frame commits?

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:91: void
BrowserPlugin::SetSrcAttribute(const std::string& src) {
On Charlie's comment that on src.empty(), we should navigate to about:blank,
what's the expected behavior.

if someone does in JS

  guest.src = "";

and then later does

  alert(guest.src);

Should we get:
  1) alert with "", and the embedder has no guest.
  2) alert with "", and the guest is rendering about:blank behind the scenes?
  3) alert with "about:blank", and the guest is rendering about:blank?

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:103: // such a message,
subsequent SetSrcAttribute() calls  must always send
nit: extra space after calls

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:107: // non-empty value).
Awesome. That's way more clear :)

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:339: old_height ==
window_rect.height)
use braces if you have a multi-line conditional

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:341: // Don't send anything to
embedder until an actual navigation occurs, since
nit: remove comment.

Also, I think it we should callout that the plugin's geometry still should be
updated.  Maybe phrase it like:

"Until an actual navigation occurs, there is no browser side embedder present to
notify about geometry updates. In this case, after we've updated the
BrowserPlugin's state we are done and can return immediately."

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/b...
content/renderer/browser_plugin/browser_plugin_browsertest.cc:73:
TEST_F(BrowserPluginTest, InitialResize) {
Do we need to test resize being preserved when src=""?

Fady Samuel

http://codereview.chromium.org/10868012/diff/38062/content/browser/browser_plugin/browser_plugin_guest.cc File content/browser/browser_plugin/browser_plugin_guest.cc (right): http://codereview.chromium.org/10868012/diff/38062/content/browser/browser_plugin/browser_plugin_guest.cc#newcode90 content/browser/browser_plugin/browser_plugin_guest.cc:90: // factor. On 2012/09/06 19:55:26, awong wrote: > If ...

8 years, 3 months ago (2012-09-06 20:56:10 UTC) #39

Fady Samuel

http://codereview.chromium.org/10868012/diff/38062/content/renderer/browser_plugin/browser_plugin.cc File content/renderer/browser_plugin/browser_plugin.cc (right): http://codereview.chromium.org/10868012/diff/38062/content/renderer/browser_plugin/browser_plugin.cc#newcode341 content/renderer/browser_plugin/browser_plugin.cc:341: // Don't send anything to embedder until an actual ...

8 years, 3 months ago (2012-09-06 21:08:15 UTC) #40

lazyboy

I've added class level documentations for BrowserPluginGuest and BrowserPluginEmbedder. I'm not sure if we need ...

8 years, 3 months ago (2012-09-07 19:32:20 UTC) #41

lazyboy

(publishing drafts) https://chromiumcodereview.appspot.com/10868012/diff/47001/content/browser/browser_plugin/browser_plugin_embedder.cc File content/browser/browser_plugin/browser_plugin_embedder.cc (right): https://chromiumcodereview.appspot.com/10868012/diff/47001/content/browser/browser_plugin/browser_plugin_embedder.cc#newcode162 content/browser/browser_plugin/browser_plugin_embedder.cc:162: guest_web_contents_container_.begin(); On 2012/09/06 19:55:26, awong wrote: > ...

8 years, 3 months ago (2012-09-07 19:33:19 UTC) #42

(publishing drafts)

https://chromiumcodereview.appspot.com/10868012/diff/47001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/47001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:162:
guest_web_contents_container_.begin();
On 2012/09/06 19:55:26, awong wrote:
> On 2012/09/06 04:33:53, lazyboy wrote:
> > On 2012/09/06 00:23:27, awong wrote:
> > > line continuations should be 4 spaces indented from previous indent level.
> > > 
> > > Though maybe just use STLDeleteContainerPairFirstPointers() from
> > > base/stl_util.h?
> > 
> > Done.
> >  
> > Is the style guide for "for (...)" indentation is 4 spaces or matching "("?,
> > i.e.:
> > for (i = 0;
> >     i < 100;...
> > or
> > for (i = 0;
> >      i < 100;...
> > ?
> 
> The latter.  What I meant was that the guest_web_contents_container_.begin()
> should be 4 spaces further in.  See new comment in
> DidCommitProvisionLoadForFrame.
> 
> Sorry for being unclear.

Ok.

https://chromiumcodereview.appspot.com/10868012/diff/47001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/47001/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:70:
base::KillProcess(process_handle, RESULT_CODE_HUNG, false);
On 2012/09/06 19:55:26, awong wrote:
> On 2012/09/06 04:33:53, lazyboy wrote:
> > On 2012/09/06 00:23:27, awong wrote:
> > > Charlie mentioned that 5-second automatic kill is undesirable behavior. 
Is
> > > there a BUG that should be included in a TODO here for finding a better
> > > behavior?
> > 
> > This one is related to guest's input event hanging embedder, from what I
know
> > it's not possible to do without making webkit input events reentrant. Let's
> get
> > back to this when you have done next detailed pass, I can add a bug as
Charlie
> > suggested in that case. 
> 
> Right. Let's file a bug then and also add a comment (probably where you set
the
> input even hang timer) that explains why we are must do a full process kill
> here.  BTW, do we need to track stats for how often we get a kill cause of an
> input event?
Filed crbug.com/147272.
At this moment, each guests are getting their own process in an embedder (Fady
says this will change), therefore I was not seeing that all guests were getting
killed. Noted in the bug.
I am not familiar with the stats tracking, is this done via UMA (I don't know
much about UMA either).

https://chromiumcodereview.appspot.com/10868012/diff/47001/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:172:
base::TimeDelta::FromMilliseconds(kGuestHangTimeout));
On 2012/09/06 19:55:26, awong wrote:
> On 2012/09/06 04:33:53, lazyboy wrote:
> > On 2012/09/06 00:23:27, awong wrote:
> > > If a single guest doesn't respond, won't this kill the whole process? Are
> > there
> > > cases where a single guest might be slow to respond, but the rest of the
> > process
> > > is still responsive?
> > 
> > If a guest's hang monitor times out, won't that make RenderereUnresponsive()
> > only for that guest's RenderViewHost? By 'reset of the process' do you mean
> > other guests in the same embedder?
> 
> What I specifically meant was guests in the same renderer process. I think for
> browser tag this is restricted to your case of others guests in the same
> embedder.  This is related to the 5-second kill on hung input that we were
> talking about earlier.

See the comment above, the bug will cover this.
Also note that I missed this case since my tests were showing otherwise; now I
know why it was not killing all the guests in the embedder: each guests are
getting their own processes and therefore killing one isn't affecting the other.

https://chromiumcodereview.appspot.com/10868012/diff/47001/content/browser/we...
File content/browser/web_contents/web_contents_impl.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/47001/content/browser/we...
content/browser/web_contents/web_contents_impl.h:166: // Sets guest to this
WebContents embedder.
On 2012/09/06 19:55:26, awong wrote:
> On 2012/09/06 04:33:53, lazyboy wrote:
> > On 2012/09/06 00:23:27, awong wrote:
> > > I expect a SetXXX() function to take an object that gets put into a member
> > > variable and to be symmetrical with the GetXXX functions.
> > > 
> > > But this function seems to be creating the guest.  Is it more like
> > > AssignBrowserPluingGuest?
> > > 
> > > The comment also doesn't quite explain the functionality. BTW, is it okay
to
> > > call this function twice?
> > 
> > Renamed the function.
> > There should be no side effect calling it > 1. We don't expect it to be
called
> > more than once for a given web_contents though.
> 
> Ah, I see.  Though looking at this more, this seems like it should be a
> construction. A WebContents never switches between being a guest, and being
> {normal,embedder}.
Embedder and guest can be seen as roles A WC can play, it can have none, any or
both role in theory.
A guest can be an embedder that way, although for first cut we will disable
recursive embedding.
> 
> What about making a WebContents::CreateGuest() constructor?  You wouldn't even
> need the base_web_contents and could move the messy custom SiteInstance
creation
> logic into there so you just take the host's GURL.

+1, So to be clear, we would have:
gwc = WebContentsImpl* WebContentsImpl::CreateGuest(
    BrowserContext*, string& url, int guest_instance_id);
remove function WebContentsImpl::Assign/SetBrowserPluginGuest()
work with gwc->GetBrowserPluginGuest() in embedder.
?
I can do the change.

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:209: // points to a
page that has an iframe. Call the iframe's WebCotnents WC2. Now
On 2012/09/06 19:55:26, awong wrote:
> nit: WebCtnents -> WebContents

Done.

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:227:
guest_web_contents_container_.begin();
On 2012/09/06 19:55:26, awong wrote:
> guest_web_contents_container_.begin() should be indented 4 more spaces because
> it is a continuation of the initializer statement portion of the for loop.

And the second statement in for loop indents with ( of for.
Done.

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:106: // and
eventually gets killed.
On 2012/09/06 19:55:26, awong wrote:
> Doesn't this guarantee a 5 second wait in the browser test? If so, is there a
> way we can override the hang time in tests ideally based on the
test_timeouts.h?
> 5s is a long time in a test...
Unfortunately yes. I will be looking at test_timeouts.h to see if I can avoid
this.

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
File content/browser/browser_plugin/browser_plugin_host_factory.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
content/browser/browser_plugin/browser_plugin_host_factory.h:32: RenderViewHost*
render_view_host) = 0;
On 2012/09/06 19:55:26, awong wrote:
> Nice catch on the virtual destructor :)
> 
> nit: add a newline above private:

Need to make it protected and need providing blank impl. (didn't compile
browsertest on last iter)
Done.

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/we...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/we...
content/browser/web_contents/web_contents_impl.cc:2463:
RemoveSwappedOutBrowserPluginEmbedder();
On 2012/09/06 19:55:26, awong wrote:
> Can we add a test asserting this behavior, both that a non-swapping load
doesn't
> change the browser_plugin_embedder_ and that a swapping load does change it.
Will this be a similar browsertest that we currently have?
(running by you to verify):
1. Load a page with guest, so we have browser_plugin_embedder_ in WC
2. click on some link on the page (no swap),  ensure same
browser_plugin_embedder_ is present in WC
3. type something in url bar (so we have swapped out RVH) , ensure
browser_plugin_embedder_ is reset to NULL.

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/we...
content/browser/web_contents/web_contents_impl.cc:2511:
!browser_plugin_embedder_->IsForRenderViewHost(GetRenderViewHost())) {
On 2012/09/06 19:55:26, awong wrote:
> Is there a case where you get a swapped signal, have a previous
> browser_plugin_embedder_, and have it be the same RVH?  What's the sequence?
> browser_plugin_embedder_ somehow gets created before the provisional load of
the
> top-level frame commits?

This is unnecessary it seems (I had way less idea about swapping at the time of
writing this). I always seem to get different RVH when I get a swap call,
Removed.

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:91: void
BrowserPlugin::SetSrcAttribute(const std::string& src) {
On 2012/09/06 19:55:26, awong wrote:
> On Charlie's comment that on src.empty(), we should navigate to about:blank,
> what's the expected behavior.
> 
> if someone does in JS
> 
>   guest.src = "";
> 
> and then later does
> 
>   alert(guest.src);
> 
> Should we get:
>   1) alert with "", and the embedder has no guest.
>   2) alert with "", and the guest is rendering about:blank behind the scenes?
>   3) alert with "about:blank", and the guest is rendering about:blank?

If we haven't loaded a guest at all in |guest|, we'd get alert with ="",
embedder has no guest.
But say we loaded a |guest| with src="something" (and we have a guest) and later
set src="", we'd get alert with "", the WebContents will load with "" as src,
and js will alert with "".

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:103: // such a message,
subsequent SetSrcAttribute() calls  must always send
On 2012/09/06 19:55:26, awong wrote:
> nit: extra space after calls

Done.

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:107: // non-empty value).
On 2012/09/06 19:55:26, awong wrote:
> Awesome. That's way more clear :)

Done.

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:339: old_height ==
window_rect.height)
On 2012/09/06 19:55:26, awong wrote:
> use braces if you have a multi-line conditional 

Done.

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:341: // Don't send anything to
embedder until an actual navigation occurs, since
On 2012/09/06 19:55:26, awong wrote:
> nit: remove comment.
> 
> Also, I think it we should callout that the plugin's geometry still should be
> updated.  Maybe phrase it like:
> 
> "Until an actual navigation occurs, there is no browser side embedder present
to
> notify about geometry updates. In this case, after we've updated the
> BrowserPlugin's state we are done and can return immediately."

Done.

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:341: // Don't send anything to
embedder until an actual navigation occurs, since
On 2012/09/06 21:08:15, Fady Samuel wrote:
> On 2012/09/06 19:55:26, awong wrote:
> > nit: remove comment.
> > 
> > Also, I think it we should callout that the plugin's geometry still should
be
> > updated.  Maybe phrase it like:
> > 
> > "Until an actual navigation occurs, there is no browser side embedder
present
> to
> > notify about geometry updates. In this case, after we've updated the
> > BrowserPlugin's state we are done and can return immediately."
> 
> As discussed offline, please also verify that we're actually navigation away
> from the current page when src is set to "". 

I've tested with
1. set guest src to a infinite loop (for(;;);) page
2. after 3 seconds set the src to an empty page (src="")
3. send input to the guest and observe it doesn't crash.
So, we do navigate away from the current page, however, I can't verify this way
if the old guest is still lying around for some reason (no way to send any js
message from guest to embedder to track it), but would be possible to do it in a
browsertest (Fady volunteered to write one :))

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/b...
content/renderer/browser_plugin/browser_plugin_browsertest.cc:73:
TEST_F(BrowserPluginTest, InitialResize) {
On 2012/09/06 19:55:26, awong wrote:
> Do we need to test resize being preserved when src=""?

If I understand this one correctly: since we got an update rect and
resize_pending_ = false happened at the end, this means the size is correct
(640x480).
Fady can comment since he wrote this one.

Fady Samuel

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/browser_plugin/browser_plugin_browsertest.cc File content/renderer/browser_plugin/browser_plugin_browsertest.cc (right): https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/browser_plugin/browser_plugin_browsertest.cc#newcode73 content/renderer/browser_plugin/browser_plugin_browsertest.cc:73: TEST_F(BrowserPluginTest, InitialResize) { On 2012/09/07 19:33:19, lazyboy wrote: > ...

8 years, 3 months ago (2012-09-07 19:37:52 UTC) #43

awong

On 2012/09/07 19:37:52, Fady Samuel wrote: > https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/browser_plugin/browser_plugin_browsertest.cc > File content/renderer/browser_plugin/browser_plugin_browsertest.cc (right): > > https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/browser_plugin/browser_plugin_browsertest.cc#newcode73 ...

8 years, 3 months ago (2012-09-07 20:03:19 UTC) #44

Fady Samuel

> Yes, I think so. The invariant that you guys were stating previous was, if ...

8 years, 3 months ago (2012-09-07 20:10:34 UTC) #45

awong

On Fri, Sep 7, 2012 at 1:10 PM, <fsamuel@chromium.org> wrote: > > Yes, I think ...

8 years, 3 months ago (2012-09-07 20:13:30 UTC) #46

awong

Sending responses to question from last round of comments. Still doing final validation of the ...

8 years, 3 months ago (2012-09-07 20:51:03 UTC) #47

Sending responses to question from last round of comments.

Still doing final validation of the points Charlie raised + checking
unittests...unfortunately that's probably 3-4 hours away.

http://codereview.chromium.org/10868012/diff/47001/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.h (right):

http://codereview.chromium.org/10868012/diff/47001/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.h:166: // Sets guest to this
WebContents embedder.
On 2012/09/07 19:33:19, lazyboy wrote:
> On 2012/09/06 19:55:26, awong wrote:
> > On 2012/09/06 04:33:53, lazyboy wrote:
> > > On 2012/09/06 00:23:27, awong wrote:
> > > > I expect a SetXXX() function to take an object that gets put into a
member
> > > > variable and to be symmetrical with the GetXXX functions.
> > > > 
> > > > But this function seems to be creating the guest.  Is it more like
> > > > AssignBrowserPluingGuest?
> > > > 
> > > > The comment also doesn't quite explain the functionality. BTW, is it
okay
> to
> > > > call this function twice?
> > > 
> > > Renamed the function.
> > > There should be no side effect calling it > 1. We don't expect it to be
> called
> > > more than once for a given web_contents though.
> > 
> > Ah, I see.  Though looking at this more, this seems like it should be a
> > construction. A WebContents never switches between being a guest, and being
> > {normal,embedder}.
> Embedder and guest can be seen as roles A WC can play, it can have none, any
or
> both role in theory.
> A guest can be an embedder that way, although for first cut we will disable
> recursive embedding.
> > 
> > What about making a WebContents::CreateGuest() constructor?  You wouldn't
even
> > need the base_web_contents and could move the messy custom SiteInstance
> creation
> > logic into there so you just take the host's GURL.
> 
> +1, So to be clear, we would have:
> gwc = WebContentsImpl* WebContentsImpl::CreateGuest(
>     BrowserContext*, string& url, int guest_instance_id);
> remove function WebContentsImpl::Assign/SetBrowserPluginGuest()
> work with gwc->GetBrowserPluginGuest() in embedder.
> ?
> I can do the change.

Yep. I think constructing the right state from the get go makes a class of
possible problems impossible. :)

http://codereview.chromium.org/10868012/diff/38062/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

http://codereview.chromium.org/10868012/diff/38062/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:90: // factor.
On 2012/09/06 20:56:10, Fady Samuel wrote:
> On 2012/09/06 19:55:26, awong wrote:
> > If the scale factor is unequal (I presume this can happen if the guest
> navigates
> > to a domain that has a different host zoom setting?) won't this leave the
> guest
> > with a stale image?
> > 
> > Should be a bug?
> 
> Scaling has nothing to do with zooming.
> 
> Scaling is used on high-DPI screens (like retina display MacBook Pros).
> 
> Scaling can change if we move a window from a high-DPI screen to a low-DPI
> screen or vice versa.

Ah. So should this really be a DCHECK?

I'm trying to understand what then this clause is skipped and what the
consequences of that are.

http://codereview.chromium.org/10868012/diff/38062/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.cc (right):

http://codereview.chromium.org/10868012/diff/38062/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.cc:2463:
RemoveSwappedOutBrowserPluginEmbedder();
On 2012/09/07 19:33:19, lazyboy wrote:
> On 2012/09/06 19:55:26, awong wrote:
> > Can we add a test asserting this behavior, both that a non-swapping load
> doesn't
> > change the browser_plugin_embedder_ and that a swapping load does change it.
> Will this be a similar browsertest that we currently have?
> (running by you to verify):
> 1. Load a page with guest, so we have browser_plugin_embedder_ in WC
> 2. click on some link on the page (no swap),  ensure same
> browser_plugin_embedder_ is present in WC
> 3. type something in url bar (so we have swapped out RVH) , ensure
> browser_plugin_embedder_ is reset to NULL.

Yes, something like that.  Is there already a test that covers this?

http://codereview.chromium.org/10868012/diff/38062/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (right):

http://codereview.chromium.org/10868012/diff/38062/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:91: void
BrowserPlugin::SetSrcAttribute(const std::string& src) {
On 2012/09/07 19:33:19, lazyboy wrote:
> On 2012/09/06 19:55:26, awong wrote:
> > On Charlie's comment that on src.empty(), we should navigate to about:blank,
> > what's the expected behavior.
> > 
> > if someone does in JS
> > 
> >   guest.src = "";
> > 
> > and then later does
> > 
> >   alert(guest.src);
> > 
> > Should we get:
> >   1) alert with "", and the embedder has no guest.
> >   2) alert with "", and the guest is rendering about:blank behind the
scenes?
> >   3) alert with "about:blank", and the guest is rendering about:blank?
> 
> If we haven't loaded a guest at all in |guest|, we'd get alert with ="",
> embedder has no guest.
> But say we loaded a |guest| with src="something" (and we have a guest) and
later
> set src="", we'd get alert with "", the WebContents will load with "" as src,
> and js will alert with "".

If we navigate the existing WebContents to "", what gets rendered?

Charlie's specific comment was:

"As for Sean's question, I think manually assigning src to "" should cause a
navigation to about:blank, not swap out the guest.  (A swapped out RVH should
never be visible.)"

I'm trying to understand if this addresses his concern...

http://codereview.chromium.org/10868012/diff/42040/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

http://codereview.chromium.org/10868012/diff/42040/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.h:6: // In the beginning
when a renderer sees one ore more guests (BrowserPlugin
ore -> or

http://codereview.chromium.org/10868012/diff/42040/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.h:7: // instance(s)) and
there is a request to navigate to them, the WebContents of
of -> for

http://codereview.chromium.org/10868012/diff/42040/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.h:8: // that renderer
creates a BrowserPluginEmbedder for it, which in turn manages
Do you mean "itself" (aka, the WebContents)?

Breaking the second clause into another will be more clear.

The BrowserPluginEmbedder, in turn, manages a set of BrowserPluginGuests -- one
BrowserPluginGuest for each guest in the embedding WebContents.

http://codereview.chromium.org/10868012/diff/42040/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.h:9: // the guest(s). It
routes any messages directed to a guest from the renderer
Same question about "It" :(

Because the previous sentence had so many objects/nouns, it's hard to know.

http://englishplus.com/grammar/00000030.htm

http://codereview.chromium.org/10868012/diff/42040/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

http://codereview.chromium.org/10868012/diff/42040/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:180: //
TODO(fsamuel,lazyboy): Find a way to get rid of guest process kill
Please add the bug url into the TODO.

http://codereview.chromium.org/10868012/diff/42040/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

http://codereview.chromium.org/10868012/diff/42040/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.h:5: // A BrowserPluginGuest
represents the browser side of browser <--> renderer
This comment block could use some paragraphs breaks (with newlines between
paragraphs).

http://codereview.chromium.org/10868012/diff/42040/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.h:6: // communication (a
browser plugin (which is a WebPlugin) is on the renderer
Make the parenthetical statement its own sentence.  Nested parenthesis == new
sentence ;)

Also clarify that this is browser <--> guest renderer, and that a "guest"
renderer is a <browser> tag.

The target audience will be maintainers who might know chrome, but know
BrowserPlugin so assume that knowledge level.

http://codereview.chromium.org/10868012/diff/42040/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.h:9: // object receives
messages (RenderViewHostMsg_*) directed at the browser plugin
Is there a way to know which RenderViewHostMsg_* we want to listen for
specifically?  Do we just want to grab each message that has analogous handler
in WebPlugin?

Stating the constraint would be useful for future maintainers who want to modify
or validate the implementation.

http://codereview.chromium.org/10868012/diff/42040/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.h:15: // It knows about its
embedder process, communication to renderer happens
s/It/BrowserPluginGuest/

Otherwise the antecedent is very ambiguous.

http://codereview.chromium.org/10868012/diff/42040/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.h:18: // to the
BrowserPlugin, its a WebContentsDelegate and WebContentsObserver for
, its -> . It is

http://codereview.chromium.org/10868012/diff/42040/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.h:19: // it.
"it" twice in one sentence referring to 2 different things == ambiguous
antecedent again :)

Refer to both by their full names to be completely clear.

http://codereview.chromium.org/10868012/diff/42040/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.h:56: // messages.
Newline between paragraphs.  Please fix here and in other comment blocks.

http://codereview.chromium.org/10868012/diff/42040/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.h (right):

http://codereview.chromium.org/10868012/diff/42040/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.h:399: // Resets state
variables related to tracking pending size and painting.
Can we expand on when/why you would call this?

lazyboy

Pending from this round (if I haven't missed others): - Adding a test to check ...

8 years, 3 months ago (2012-09-08 02:12:21 UTC) #48

Pending from this round (if I haven't missed others):

- Adding a test to check if WC->browser_plugin_embedder_ gets reset properly
after we have RVH swap.

- Couple questions from Albert is redirected to Fady.

I'll be working on adding the test in the mean time.
Thanks.

https://chromiumcodereview.appspot.com/10868012/diff/47001/content/browser/we...
File content/browser/web_contents/web_contents_impl.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/47001/content/browser/we...
content/browser/web_contents/web_contents_impl.h:166: // Sets guest to this
WebContents embedder.
On 2012/09/07 20:51:03, awong wrote:
> On 2012/09/07 19:33:19, lazyboy wrote:
> > On 2012/09/06 19:55:26, awong wrote:
> > > On 2012/09/06 04:33:53, lazyboy wrote:
> > > > On 2012/09/06 00:23:27, awong wrote:
> > > > > I expect a SetXXX() function to take an object that gets put into a
> member
> > > > > variable and to be symmetrical with the GetXXX functions.
> > > > > 
> > > > > But this function seems to be creating the guest.  Is it more like
> > > > > AssignBrowserPluingGuest?
> > > > > 
> > > > > The comment also doesn't quite explain the functionality. BTW, is it
> okay
> > to
> > > > > call this function twice?
> > > > 
> > > > Renamed the function.
> > > > There should be no side effect calling it > 1. We don't expect it to be
> > called
> > > > more than once for a given web_contents though.
> > > 
> > > Ah, I see.  Though looking at this more, this seems like it should be a
> > > construction. A WebContents never switches between being a guest, and
being
> > > {normal,embedder}.
> > Embedder and guest can be seen as roles A WC can play, it can have none, any
> or
> > both role in theory.
> > A guest can be an embedder that way, although for first cut we will disable
> > recursive embedding.
> > > 
> > > What about making a WebContents::CreateGuest() constructor?  You wouldn't
> even
> > > need the base_web_contents and could move the messy custom SiteInstance
> > creation
> > > logic into there so you just take the host's GURL.
> > 
> > +1, So to be clear, we would have:
> > gwc = WebContentsImpl* WebContentsImpl::CreateGuest(
> >     BrowserContext*, string& url, int guest_instance_id);
> > remove function WebContentsImpl::Assign/SetBrowserPluginGuest()
> > work with gwc->GetBrowserPluginGuest() in embedder.
> > ?
> > I can do the change.
> 
> Yep. I think constructing the right state from the get go makes a class of
> possible problems impossible. :)

Done.

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:90: // factor.
Fady to comment on this one.

On 2012/09/07 20:51:03, awong wrote:
> On 2012/09/06 20:56:10, Fady Samuel wrote:
> > On 2012/09/06 19:55:26, awong wrote:
> > > If the scale factor is unequal (I presume this can happen if the guest
> > navigates
> > > to a domain that has a different host zoom setting?) won't this leave the
> > guest
> > > with a stale image?
> > > 
> > > Should be a bug?
> > 
> > Scaling has nothing to do with zooming.
> > 
> > Scaling is used on high-DPI screens (like retina display MacBook Pros).
> > 
> > Scaling can change if we move a window from a high-DPI screen to a low-DPI
> > screen or vice versa.
> 
> Ah. So should this really be a DCHECK?
> 
> I'm trying to understand what then this clause is skipped and what the
> consequences of that are.

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:106: // and
eventually gets killed.
I am not totally clear on the point of using a particular timeout value from
TestTimeouts, also waiting for your answer on this one.

On 2012/09/07 19:33:19, lazyboy wrote:
> On 2012/09/06 19:55:26, awong wrote:
> > Doesn't this guarantee a 5 second wait in the browser test? If so, is there
a
> > way we can override the hang time in tests ideally based on the
> test_timeouts.h?
> > 5s is a long time in a test...
> Unfortunately yes. I will be looking at test_timeouts.h to see if I can avoid
> this.

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/we...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/we...
content/browser/web_contents/web_contents_impl.cc:2463:
RemoveSwappedOutBrowserPluginEmbedder();
No there isn't. I'll be adding one soon.

On 2012/09/07 20:51:03, awong wrote:
> On 2012/09/07 19:33:19, lazyboy wrote:
> > On 2012/09/06 19:55:26, awong wrote:
> > > Can we add a test asserting this behavior, both that a non-swapping load
> > doesn't
> > > change the browser_plugin_embedder_ and that a swapping load does change
it.
> > Will this be a similar browsertest that we currently have?
> > (running by you to verify):
> > 1. Load a page with guest, so we have browser_plugin_embedder_ in WC
> > 2. click on some link on the page (no swap),  ensure same
> > browser_plugin_embedder_ is present in WC
> > 3. type something in url bar (so we have swapped out RVH) , ensure
> > browser_plugin_embedder_ is reset to NULL.
> 
> Yes, something like that.  Is there already a test that covers this?

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:91: void
BrowserPlugin::SetSrcAttribute(const std::string& src) {
If we navigate the existing WebContents to "", what gets rendered?
-> white rectangle, see BrowserPlugin::paint()

I think manually assigning src to "" should cause a
navigation to about:blank, not swap out the guest. 
-> a. If we have src="" in the beginning, we don't have any guest (browser
side); b. If we have src='something', we have a guest GG (browser side), now if
we set src='', we still have GG, but its WC now did LoadUrl(''). I think the
concern was if we swap out GG, which we are not doing. (Again, if I understand
the concern correctly).

On 2012/09/07 20:51:03, awong wrote:
> On 2012/09/07 19:33:19, lazyboy wrote:
> > On 2012/09/06 19:55:26, awong wrote:
> > > On Charlie's comment that on src.empty(), we should navigate to
about:blank,
> > > what's the expected behavior.
> > > 
> > > if someone does in JS
> > > 
> > >   guest.src = "";
> > > 
> > > and then later does
> > > 
> > >   alert(guest.src);
> > > 
> > > Should we get:
> > >   1) alert with "", and the embedder has no guest.
> > >   2) alert with "", and the guest is rendering about:blank behind the
> scenes?
> > >   3) alert with "about:blank", and the guest is rendering about:blank?
> > 
> > If we haven't loaded a guest at all in |guest|, we'd get alert with ="",
> > embedder has no guest.
> > But say we loaded a |guest| with src="something" (and we have a guest) and
> later
> > set src="", we'd get alert with "", the WebContents will load with "" as
src,
> > and js will alert with "".
> 
> If we navigate the existing WebContents to "", what gets rendered?
> 
> Charlie's specific comment was:
> 
> "As for Sean's question, I think manually assigning src to "" should cause a
> navigation to about:blank, not swap out the guest.  (A swapped out RVH should
> never be visible.)"
> 
> I'm trying to understand if this addresses his concern...

https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.h:6: // In the beginning
when a renderer sees one ore more guests (BrowserPlugin
On 2012/09/07 20:51:03, awong wrote:
> ore -> or

Done.

https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.h:7: // instance(s)) and
there is a request to navigate to them, the WebContents of
On 2012/09/07 20:51:03, awong wrote:
> of -> for

Done.

https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.h:8: // that renderer
creates a BrowserPluginEmbedder for it, which in turn manages
On 2012/09/07 20:51:03, awong wrote:
> Do you mean "itself" (aka, the WebContents)?
> 
> Breaking the second clause into another will be more clear.
> 
> The BrowserPluginEmbedder, in turn, manages a set of BrowserPluginGuests --
one
> BrowserPluginGuest for each guest in the embedding WebContents.

Done.

https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.h:9: // the guest(s). It
routes any messages directed to a guest from the renderer
On 2012/09/07 20:51:03, awong wrote:
> Same question about "It" :(
> 
> Because the previous sentence had so many objects/nouns, it's hard to know.
> 
> http://englishplus.com/grammar/00000030.htm

Done.

https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:180: //
TODO(fsamuel,lazyboy): Find a way to get rid of guest process kill
On 2012/09/07 20:51:03, awong wrote:
> Please add the bug url into the TODO.

Forgot adding this one after creating the bug.
Done.

https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:5: // A BrowserPluginGuest
represents the browser side of browser <--> renderer
On 2012/09/07 20:51:03, awong wrote:
> This comment block could use some paragraphs breaks (with newlines between
> paragraphs).

Done.

https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:6: // communication (a
browser plugin (which is a WebPlugin) is on the renderer
On 2012/09/07 20:51:03, awong wrote:
> Make the parenthetical statement its own sentence.  Nested parenthesis == new
> sentence ;)
> 
> Also clarify that this is browser <--> guest renderer, and that a "guest"
> renderer is a <browser> tag.
> 
> The target audience will be maintainers who might know chrome, but know
> BrowserPlugin so assume that knowledge level.

Done.

https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:9: // object receives
messages (RenderViewHostMsg_*) directed at the browser plugin
On 2012/09/07 20:51:03, awong wrote:
> Is there a way to know which RenderViewHostMsg_* we want to listen for
> specifically?  Do we just want to grab each message that has analogous handler
> in WebPlugin?
> 
> Stating the constraint would be useful for future maintainers who want to
modify
> or validate the implementation.

Fady can you answer this one? Thanks.

https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:15: // It knows about its
embedder process, communication to renderer happens
On 2012/09/07 20:51:03, awong wrote:
> s/It/BrowserPluginGuest/
> 
> Otherwise the antecedent is very ambiguous.

Done.

https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:18: // to the
BrowserPlugin, its a WebContentsDelegate and WebContentsObserver for
On 2012/09/07 20:51:03, awong wrote:
> , its -> . It is

Done.

https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:19: // it.
On 2012/09/07 20:51:03, awong wrote:
> "it" twice in one sentence referring to 2 different things == ambiguous
> antecedent again :)
> 
> Refer to both by their full names to be completely clear.

Done.

https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:56: // messages.
On 2012/09/07 20:51:03, awong wrote:
> Newline between paragraphs.  Please fix here and in other comment blocks.

Done.

https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/re...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/re...
content/browser/renderer_host/render_widget_host_impl.h:399: // Resets state
variables related to tracking pending size and painting.
On 2012/09/07 20:51:03, awong wrote:
> Can we expand on when/why you would call this?
Added comments.

lazyboy

On 2012/09/07 19:37:52, Fady Samuel wrote: > https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/browser_plugin/browser_plugin_browsertest.cc > File content/renderer/browser_plugin/browser_plugin_browsertest.cc (right): > > https://chromiumcodereview.appspot.com/10868012/diff/38062/content/renderer/browser_plugin/browser_plugin_browsertest.cc#newcode73 ...

8 years, 3 months ago (2012-09-08 02:15:13 UTC) #49

awong

I've done another round and added a number more comments. I think I understand the ...

8 years, 3 months ago (2012-09-09 18:08:09 UTC) #50

Fady Samuel

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/browser_plugin/browser_plugin_guest.cc File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/browser_plugin/browser_plugin_guest.cc#newcode90 content/browser/browser_plugin/browser_plugin_guest.cc:90: // factor. On 2012/09/08 02:12:22, lazyboy wrote: > Fady ...

8 years, 3 months ago (2012-09-10 16:24:10 UTC) #51

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:90: // factor.
On 2012/09/08 02:12:22, lazyboy wrote:
> Fady to comment on this one.
> 
> On 2012/09/07 20:51:03, awong wrote:
> > On 2012/09/06 20:56:10, Fady Samuel wrote:
> > > On 2012/09/06 19:55:26, awong wrote:
> > > > If the scale factor is unequal (I presume this can happen if the guest
> > > navigates
> > > > to a domain that has a different host zoom setting?) won't this leave
the
> > > guest
> > > > with a stale image?
> > > > 
> > > > Should be a bug?
> > > 
> > > Scaling has nothing to do with zooming.
> > > 
> > > Scaling is used on high-DPI screens (like retina display MacBook Pros).
> > > 
> > > Scaling can change if we move a window from a high-DPI screen to a low-DPI
> > > screen or vice versa.
> > 
> > Ah. So should this really be a DCHECK?
> > 
> > I'm trying to understand what then this clause is skipped and what the
> > consequences of that are.
> 

No, this shouldn't be a DCHECK. When we change the DPI on ChromeOS, the UI's DPI
changes, and it tells the WebContents to change its DPI. This is an async
process, and for a small period of time (milliseconds) the guest's WebContents
may be renderered at a different DPI than the embedder's.

https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:9: // object receives
messages (RenderViewHostMsg_*) directed at the browser plugin
On 2012/09/08 02:12:22, lazyboy wrote:
> On 2012/09/07 20:51:03, awong wrote:
> > Is there a way to know which RenderViewHostMsg_* we want to listen for
> > specifically?  Do we just want to grab each message that has analogous
handler
> > in WebPlugin?
> > 
> > Stating the constraint would be useful for future maintainers who want to
> modify
> > or validate the implementation.
> 
> Fady can you answer this one? Thanks.

Firstly, I believe that should say ViewHostMsg_* rather than
RenderViewHostMsg_*.

Secondly, I don't have a simple answer to this question.

Basically, anything the embedder might be interested in knowing or modifying
about the guest should be listened for here.

lazyboy

re tests: Fady and I discussed about these tests (#1 is very compelling), I have ...

8 years, 3 months ago (2012-09-10 16:30:17 UTC) #52

re tests:
Fady and I discussed about these tests (#1 is very compelling), I have added
notes for the current status and plans for the ones we'd like to have in the
current CL (#3, #6) since they are expected to be easy to write.
For the rest I will create bug and we will add them once this CL gets submitted.
Let me know if that sounds good to you.

(1) Verify guest cleanup (including subfames and subframe navigations)
(2) resize propagates
-> I have a pending CL that does aggressive resize testing (send a lot of
resizes from js and see the final size is correct or not), I will resume
(completely outdated by now) when I'm done with this CL.
(3) Input events propagate (including raw inputs?)
-> I will add a browsertest for this in the current CL.
(4) input hang trigger auto kill.
-> Covered already. BrowserPluginHost.NavigateGuest()
(4b) plugin destruction on input hang doesn't hang embedder.
(5) Visibility propagation
(6) Focus propagation
-> Fady expects to write this one when he has a chance for current CL.
(7) Initial guest Navigation
-> Covered already. BrowserPluginHost.NavigateGuest()
(8) Guest Navigation from src=""
-> Probably covered already in BrowserPluginHost.NavigateAfterResize(), although
this is not an explicit test for it.
(9) Guest Navigation to src="" (check that we have the right guest rvh)
(10) Drawing when damage region doesn't match?
-> Fady: This might be easy as adding a check to an existing test in render side
browsertest, he will look into it.
(11) Embedder navigation. Test we have indeed detached the guest correctly and
swapped out the RVH.
-> I cannot seem to trigger RVH swap from tests, sent an email to chromium-dev.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:49: new
BrowserPluginEmbedderHelper(this, render_view_host);
On 2012/09/09 18:08:09, awong wrote:
> Comment about ownership.

Done.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:98: // than the URL
that the guest is being navigated to.
On 2012/09/09 18:08:09, awong wrote:
> This comment seems to make more sense in CreateGuest.

Hmmm, I also have this comment in CreateGuest, removed from here.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:99: std::string host =
render_view_host->GetSiteInstance()->GetSite().host();
On 2012/09/09 18:08:09, awong wrote:
> const std::string& host

Done.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:157:
guest_web_contents_container_.end());
On 2012/09/09 18:08:09, awong wrote:
> Does this need to call guest->Destroy()?

guest->Destroy() would actually do the same thing, delete their web_contents().
Maybe
foreach(a in guests_by_instance_id_)
  a->Destroy();
guest_web_contents_container_.clear(), guests_by_instance_id_.clear();

is better implementation for this method?

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:184: guest->Destroy();
On 2012/09/09 18:08:09, awong wrote:
> Do we need to delete guest here?
> 
> Maybe we should have a map of linked_ptrs?


guest->Destroy() would delete the web_contents() associated with the guest,
since the guest is a WCObserver, it would delete the guest as well.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:216: // eventually
destroys G1 since G1 is an observer for WC2.
On 2012/09/09 18:08:09, awong wrote:
> Add a line explaining that the following code is repsonsible for deleting the
> top-level frame which will eventually invoke one of the two cases below
covering
> all guests in the whole frame tree.

Done.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:220:
guest_web_contents_container_.begin();
On 2012/09/09 18:08:09, awong wrote:
> indentation. off by 5 spaces.

Ah, hit this case, what happens in this case when the expressions in for() is
long (I couldn't seem to find it in style guide):

for (my_long_var ... =
    this_is_continuation_4_space_indentation;
     second_expression_regular_(_indent; third...)
or just indent everything 4 spaces or 5 spaces?

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:229: }
On 2012/09/09 18:08:09, awong wrote:
> Shouldn't this just call DestroyGuests()?
> 
> Also, this code is nlogn, though I'm guessing we don't have enough guests to
> care.

This got broken between patch #12 and #13 it seems, fixed, guests_to_delete_
does not contain all the ids in this embedder, only the ones belonging to
|frame_id|

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:244:
guest_web_contents_container_.begin();
On 2012/09/09 18:08:09, awong wrote:
> indentation

See question above, thanks.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.h:51: typedef
std::map<WebContents*, int64> GuestWebContentsMap;
On 2012/09/09 18:08:09, awong wrote:
> Move these typedefs into the private section of BrowserPluginEmbedder.

Done.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.h:93: virtual void
AddGuest(int instance_id,
On 2012/09/09 18:08:09, awong wrote:
> Take a scoped_ptr<BrowserPluginGuest> as the argument to indicate ownership
> transfer?

Hmmm, can you explain a bit more? AddGuest is a convenience method to insert the
guest object ptr in a container here.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.h:118: //
WebContentsObserver implementation.
On 2012/09/09 18:08:09, awong wrote:
> Interface implementations should retain the same access privileges as what
they
> are overriding.

Done.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder_helper.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder_helper.h:35: // will be
handled.
On 2012/09/09 18:08:09, awong wrote:
> Please describe ownership, lifetime, and thread assumptions.

Done.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:30: const int
kGuestHangTimeout = 5000;
On 2012/09/09 18:08:09, awong wrote:
> Rename this with units.
> 
> kGuestHangTimeoutMs

Done.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:163:
message->WriteBool(false);
On 2012/09/09 18:08:09, awong wrote:
> Why do we extend the pickle size for RawKeyDown event with a constant "false"
> but not for normal ones?  Shouldn't the IPC message tuple be the same arity
for
> all messages going to one handler?
The RawKeyDown parameter seems to be optional:
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/common/view_messag...

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:167: // embedder doesn't
hang.
On 2012/09/09 18:08:09, awong wrote:
> Why isn't this caught by the RenderViewGone() handler below? Add comment to
> clarify?
The comment is misleading since the word 'crash' is ambiguous and collides with
guest_crashed, reworked the comment a bit.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:174:
DCHECK(!pending_input_event_reply_.get());
On 2012/09/09 18:08:09, awong wrote:
> Shouldn't this DCHECK go somewhere before the guest_rvh->Send() call, maybe at
> the top of this function?

Done.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:196:
guest_rvh->StopHangMonitorTimeout();
On 2012/09/09 18:08:09, awong wrote:
> Let's stop the hangout monitor first so the exit/entry of the 2 async
callsites
> match up.

Done.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:220:
DCHECK(guest_web_contents);
On 2012/09/09 18:08:09, awong wrote:
> When can guest_web_contents be NULL for this class?
> 
> If we want assert this invariant, shouldn't we just DCHECK in the constructor?

Moved to constructor.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest_helper.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest_helper.cc:53:
render_widget_host->ResetSizeAndRepaintPendingFlags();
On 2012/09/09 18:08:09, awong wrote:
> If this whole object is really meant to be a passthrough that you can
> instantiate on the IO thread, should the call to
> ResetSizeAndRepaintPendingFlags() be inside guest_->UpdateRect()?

Done.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest_helper.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest_helper.h:38: // special
handling, which can be move to a message filter (IPC thread) for
On 2012/09/09 18:08:09, awong wrote:
> How likely is this future optimization? If we don't have concrete plans to
> actually do this soon, speculatively adding such a class just adds more
> maintenance burden cause you have to handle more delegates.
> 
> If we keep this class, add a comment about the expected lifetime of this
object
> (who owns it, does how we know guest outlives it, etc.)

This optimization is something we would like to have soon I believe, specially
for input events since we have unnecessary thread hops.

Added comment about expected lifetime.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest_helper.h:56: virtual bool
OnMessageReceived(const IPC::Message& message) OVERRIDE;
On 2012/09/09 18:08:09, awong wrote:
> Please go through all the classes and move overrides out of private.
> 
> The reason I'm so picky about this is cause you're actually really protecting
> access to the method.  People modifying it in the future may incorrectly
assume
> that since it's private, it's only usable by this class which is wrong. 
> Example:
> 
> BrowserPluginGuest* r = new  BrowserPluginGuestHelper();
> r->OnMessageReceved();  // compile failure.
> static_cast<RenderViewHostObserver*>(r)->OnMessageReceved(); // works on
> upcast??
> 
> this is just odd.

Fixed.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:32: const
char* kHTMLForGuest = "data:text/html,hello world";
On 2012/09/09 18:08:09, awong wrote:
> Isn't this technically malformed HTML?

Added <html>... is this what you mean?

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:170: // sample
guest. In the end we verify that the correct size has been set.
On 2012/09/09 18:08:09, awong wrote:
> Thanks for adding this!
Great.
Done.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
File content/browser/browser_plugin/test_browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/test_browser_plugin_embedder.cc:20:
guest_count_(0) {
On 2012/09/09 18:08:09, awong wrote:
> What is guest_count_ used for?

Removed, unused now.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/we...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/we...
content/browser/web_contents/web_contents_impl.cc:337: guest_instance_id);
On 2012/09/09 18:08:09, awong wrote:
> You can also just do new_contents->browser_plugin_guest_(....); right here I
> think.
Ah
Interesting, done and removed AssignBrowserPluginGuest()

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/renderer/b...
content/renderer/browser_plugin/browser_plugin_browsertest.cc:33: }
On 2012/09/09 18:08:09, awong wrote:
> closing namespace comment.

Done.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/renderer/b...
content/renderer/browser_plugin/browser_plugin_browsertest.cc:33: }
On 2012/09/09 18:08:09, awong wrote:
> closing namespace comment.

Done.

lazyboy

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/browser_plugin/browser_plugin_embedder.h File content/browser/browser_plugin/browser_plugin_embedder.h (right): https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/browser_plugin/browser_plugin_embedder.h#newcode51 content/browser/browser_plugin/browser_plugin_embedder.h:51: typedef std::map<WebContents*, int64> GuestWebContentsMap; On 2012/09/10 16:30:17, lazyboy wrote: ...

8 years, 3 months ago (2012-09-10 18:02:47 UTC) #53

awong

Looking pretty close now. Main 2 comments are about using pointers to WebContents instead of ...

8 years, 3 months ago (2012-09-10 19:21:03 UTC) #54

Looking pretty close now. Main 2 comments are about using pointers to
WebContents instead of Guest in the embedder, and then about not hanging 5
seconds for each hang test.

Regarding the unittests, you guys have a better sense for which cases are useful
coverage and not so do whatever you think makes sense there. :)

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:90: // factor.
On 2012/09/10 16:24:11, Fady Samuel wrote:
> On 2012/09/08 02:12:22, lazyboy wrote:
> > Fady to comment on this one.
> > 
> > On 2012/09/07 20:51:03, awong wrote:
> > > On 2012/09/06 20:56:10, Fady Samuel wrote:
> > > > On 2012/09/06 19:55:26, awong wrote:
> > > > > If the scale factor is unequal (I presume this can happen if the guest
> > > > navigates
> > > > > to a domain that has a different host zoom setting?) won't this leave
> the
> > > > guest
> > > > > with a stale image?
> > > > > 
> > > > > Should be a bug?
> > > > 
> > > > Scaling has nothing to do with zooming.
> > > > 
> > > > Scaling is used on high-DPI screens (like retina display MacBook Pros).
> > > > 
> > > > Scaling can change if we move a window from a high-DPI screen to a
low-DPI
> > > > screen or vice versa.
> > > 
> > > Ah. So should this really be a DCHECK?
> > > 
> > > I'm trying to understand what then this clause is skipped and what the
> > > consequences of that are.
> > 
> 
> No, this shouldn't be a DCHECK. When we change the DPI on ChromeOS, the UI's
DPI
> changes, and it tells the WebContents to change its DPI. This is an async
> process, and for a small period of time (milliseconds) the guest's WebContents
> may be renderered at a different DPI than the embedder's.

Ah hah! That's the piece I was missing.  Can we add a line saying that this can
happen due to asynchronous updates of the DPI on a resolution change?

https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:9: // object receives
messages (RenderViewHostMsg_*) directed at the browser plugin
On 2012/09/10 16:24:11, Fady Samuel wrote:
> On 2012/09/08 02:12:22, lazyboy wrote:
> > On 2012/09/07 20:51:03, awong wrote:
> > > Is there a way to know which RenderViewHostMsg_* we want to listen for
> > > specifically?  Do we just want to grab each message that has analogous
> handler
> > > in WebPlugin?
> > > 
> > > Stating the constraint would be useful for future maintainers who want to
> > modify
> > > or validate the implementation.
> > 
> > Fady can you answer this one? Thanks.
> 
> Firstly, I believe that should say ViewHostMsg_* rather than
> RenderViewHostMsg_*.
> 
> Secondly, I don't have a simple answer to this question.
> 
> Basically, anything the embedder might be interested in knowing or modifying
> about the guest should be listened for here. 

Yeah...you're right. feel free to ignore my comment.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:98: // than the URL
that the guest is being navigated to.
On 2012/09/10 16:30:17, lazyboy wrote:
> On 2012/09/09 18:08:09, awong wrote:
> > This comment seems to make more sense in CreateGuest.
> 
> Hmmm, I also have this comment in CreateGuest, removed from here.

Still seems to be here...

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:184: guest->Destroy();
On 2012/09/10 16:30:17, lazyboy wrote:
> On 2012/09/09 18:08:09, awong wrote:
> > Do we need to delete guest here?
> > 
> > Maybe we should have a map of linked_ptrs?
> 
> 
> guest->Destroy() would delete the web_contents() associated with the guest,
> since the guest is a WCObserver, it would delete the guest as well.

Ah, I see now.  It's weird to have 2 mechanisms for managing destruction of the
same set of pointers.  Makes tracking ownership hard. Also, in the future, if
someone wanted to add some behavior on destruction of a guest, there's a higher
chance that you'd 

AFAICT, this is only place that class guest->Destroy().  Maybe we should
completely remove the Destroy() method and just delete guest_web_contents
manually?

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:220:
guest_web_contents_container_.begin();
On 2012/09/10 16:30:17, lazyboy wrote:
> On 2012/09/09 18:08:09, awong wrote:
> > indentation. off by 5 spaces.
> 
> Ah, hit this case, what happens in this case when the expressions in for() is
> long (I couldn't seem to find it in style guide):
> 
> for (my_long_var ... =
>     this_is_continuation_4_space_indentation;
>      second_expression_regular_(_indent; third...)
> or just indent everything 4 spaces or 5 spaces?

Yeah, it's confusing.  My understanding of the style guide is it should be:

 for (my_long_var ... =
          this_is_continuation_4_space_indentation;
      second_expression_regular_(_indent; third...)

Since, without a line break, the next line is supposed to line up with the "m"
in "my_long_var", when you do the line continuation you are supposed to indent 4
more spaces from "where you should have been."

BTW, if the style guide doesn't state anything, the rule is to look at the
codebase for the prevailing pattern.  Ofcourse, having just written that, I went
and tried searching for

   for\ \(.*\=$ lang:^c++$

on

   http://code.google.com/p/chromium/source/search

and consistency on this is...lacking.  Google3 is a little bit more consistent
so I would indent 4 more spaces.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.h:51: typedef
std::map<WebContents*, int64> GuestWebContentsMap;
On 2012/09/10 18:02:47, lazyboy wrote:
> On 2012/09/10 16:30:17, lazyboy wrote:
> > On 2012/09/09 18:08:09, awong wrote:
> > > Move these typedefs into the private section of BrowserPluginEmbedder.
> > 
> > Done.
> 
> Had to move one back since the browsertest needs it.

Can we at put them in the public section of BrowserPluginEmbedder then?  Or
maybe just friend the test?  That reduces pollution of the content namespace.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.h:93: virtual void
AddGuest(int instance_id,
On 2012/09/10 16:30:17, lazyboy wrote:
> On 2012/09/09 18:08:09, awong wrote:
> > Take a scoped_ptr<BrowserPluginGuest> as the argument to indicate ownership
> > transfer?
> 
> Hmmm, can you explain a bit more? AddGuest is a convenience method to insert
the
> guest object ptr in a container here.

Sorry, I misunderstood the lifetime and though that the embedder deleted the
guest directly. Instead, the embedder actually only owns the guest_web_contents.

This brings up another question.  What if you kept 

  instance_id -> guest_web_contents
  guest_web_contents -> frame_id

Then in GetGuestByInstanceID(), you get the guest via the WebContents pointer?

The advantage here is BrowserPluginEmbedder only holds pointers to things it
owns (the web contents) and the relationship of objects becomes more tree-like.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:32: const
char* kHTMLForGuest = "data:text/html,hello world";
On 2012/09/10 16:30:17, lazyboy wrote:
> On 2012/09/09 18:08:09, awong wrote:
> > Isn't this technically malformed HTML?
> 
> Added <html>... is this what you mean?

Yep! Thanks!

https://chromiumcodereview.appspot.com/10868012/diff/48012/content/browser/br...
File content/browser/browser_plugin/test_browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/48012/content/browser/br...
content/browser/browser_plugin/test_browser_plugin_guest.cc:100:
crash_message_loop_runner_->Run();
Per mail from phadjdan@, sounds like the right thing to do is set the hang time
to TestTimeouts::tiny_timeout().

Then make it so this (and any other WaitForXXX event) doesn't wait indefinitely.
 Instead, make it wait for TestTimeouts::action_timeout().

For this code, I would post a delayed task with crash_message_loop_runner_'s
QuitClosure(), and possibly set a flag that tells if this timedout.  So maybe

// Come up with a better name.
class Foo {
 public:
  Foo() : timed_out_(false) {}

  void OnTimeout(const Closure& done) {
    timed_out_ = true;
    done->Run();
  }

  bool timed_out() { return timed_out_; }

 private:
  bool timed_out_;
}

bool TestBrowserPluginGuest::WaitForCrashed() {
  // Check if we already observed a guest crash, return immediately if so.
  if (crash_observed_)
    return;
  Foo f;
  WeakPtrFactory<Foo> weak_factory(&f);  // So the task cancels on function
exit.

  crash_message_loop_runner_ = new MessageLoopRunner();
  MessageLoop::current()->PostDelayedTask(
     FROM_HERE, 
     Bind(&Foo::OnTimeout, weak_factor.GetWeakPtr(),
          crash_message_loop_runner_.QuitClosure().
     TestTimeouts::action_timeout());
  crash_message_loop_runner_->Run();

  return f.timed_out;
}

lazyboy

Main two concerns are addressed: a. removing indefinitely running tests while waiting + hanging 5s ...

8 years, 3 months ago (2012-09-10 23:40:56 UTC) #55

Main two concerns are addressed: a. removing indefinitely running tests while
waiting + hanging 5s b. remove storing BPGuest* in embedder, storing
WebContents* instead.

My comments/questions:
a. kGuestHangTimeoutMs overriding in test is ugly, any better way to do this
kind of thing?
b. The Map typedef name in BPGuest might (or might not) be off since we're
storing different things, we could probably use better typedef and/or member
names.
c. TestTimeoutTracker (or whatever we end up calling it) seems better to be
moved in some common testing package in future if we find it useful for other
tests.

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:90: // factor.
On 2012/09/10 19:21:07, awong wrote:
> On 2012/09/10 16:24:11, Fady Samuel wrote:
> > On 2012/09/08 02:12:22, lazyboy wrote:
> > > Fady to comment on this one.
> > > 
> > > On 2012/09/07 20:51:03, awong wrote:
> > > > On 2012/09/06 20:56:10, Fady Samuel wrote:
> > > > > On 2012/09/06 19:55:26, awong wrote:
> > > > > > If the scale factor is unequal (I presume this can happen if the
guest
> > > > > navigates
> > > > > > to a domain that has a different host zoom setting?) won't this
leave
> > the
> > > > > guest
> > > > > > with a stale image?
> > > > > > 
> > > > > > Should be a bug?
> > > > > 
> > > > > Scaling has nothing to do with zooming.
> > > > > 
> > > > > Scaling is used on high-DPI screens (like retina display MacBook
Pros).
> > > > > 
> > > > > Scaling can change if we move a window from a high-DPI screen to a
> low-DPI
> > > > > screen or vice versa.
> > > > 
> > > > Ah. So should this really be a DCHECK?
> > > > 
> > > > I'm trying to understand what then this clause is skipped and what the
> > > > consequences of that are.
> > > 
> > 
> > No, this shouldn't be a DCHECK. When we change the DPI on ChromeOS, the UI's
> DPI
> > changes, and it tells the WebContents to change its DPI. This is an async
> > process, and for a small period of time (milliseconds) the guest's
WebContents
> > may be renderered at a different DPI than the embedder's.
> 
> Ah hah! That's the piece I was missing.  Can we add a line saying that this
can
> happen due to asynchronous updates of the DPI on a resolution change?

Added the line.
Done.

https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:9: // object receives
messages (RenderViewHostMsg_*) directed at the browser plugin
On 2012/09/10 19:21:07, awong wrote:
> On 2012/09/10 16:24:11, Fady Samuel wrote:
> > On 2012/09/08 02:12:22, lazyboy wrote:
> > > On 2012/09/07 20:51:03, awong wrote:
> > > > Is there a way to know which RenderViewHostMsg_* we want to listen for
> > > > specifically?  Do we just want to grab each message that has analogous
> > handler
> > > > in WebPlugin?
> > > > 
> > > > Stating the constraint would be useful for future maintainers who want
to
> > > modify
> > > > or validate the implementation.
> > > 
> > > Fady can you answer this one? Thanks.
> > 
> > Firstly, I believe that should say ViewHostMsg_* rather than
> > RenderViewHostMsg_*.
> > 
> > Secondly, I don't have a simple answer to this question.
> > 
> > Basically, anything the embedder might be interested in knowing or modifying
> > about the guest should be listened for here. 
> 
> Yeah...you're right. feel free to ignore my comment.

Fixed the note here saying ViewHostMsg_* instead of RenderViewHostMsg*_; also
added the comment saying we intercept anything that is interesting.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:98: // than the URL
that the guest is being navigated to.
On 2012/09/10 19:21:07, awong wrote:
> On 2012/09/10 16:30:17, lazyboy wrote:
> > On 2012/09/09 18:08:09, awong wrote:
> > > This comment seems to make more sense in CreateGuest.
> > 
> > Hmmm, I also have this comment in CreateGuest, removed from here.
> 
> Still seems to be here...

Missed it somehow :(
Done.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:184: guest->Destroy();
On 2012/09/10 19:21:07, awong wrote:
> On 2012/09/10 16:30:17, lazyboy wrote:
> > On 2012/09/09 18:08:09, awong wrote:
> > > Do we need to delete guest here?
> > > 
> > > Maybe we should have a map of linked_ptrs?
> > 
> > 
> > guest->Destroy() would delete the web_contents() associated with the guest,
> > since the guest is a WCObserver, it would delete the guest as well.
> 
> Ah, I see now.  It's weird to have 2 mechanisms for managing destruction of
the
> same set of pointers.  Makes tracking ownership hard. Also, in the future, if
> someone wanted to add some behavior on destruction of a guest, there's a
higher
> chance that you'd 
> 
> AFAICT, this is only place that class guest->Destroy().  Maybe we should
> completely remove the Destroy() method and just delete guest_web_contents
> manually?

Agreed, I actually thought of destroying the guests in a consistent manner, but
forgot. (Although I was thinking the otherway, always calling Destroy())
Done.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:220:
guest_web_contents_container_.begin();
On 2012/09/10 19:21:07, awong wrote:
> On 2012/09/10 16:30:17, lazyboy wrote:
> > On 2012/09/09 18:08:09, awong wrote:
> > > indentation. off by 5 spaces.
> > 
> > Ah, hit this case, what happens in this case when the expressions in for()
is
> > long (I couldn't seem to find it in style guide):
> > 
> > for (my_long_var ... =
> >     this_is_continuation_4_space_indentation;
> >      second_expression_regular_(_indent; third...)
> > or just indent everything 4 spaces or 5 spaces?
> 
> Yeah, it's confusing.  My understanding of the style guide is it should be:
> 
>  for (my_long_var ... =
>           this_is_continuation_4_space_indentation;
>       second_expression_regular_(_indent; third...)
> 
> Since, without a line break, the next line is supposed to line up with the "m"
> in "my_long_var", when you do the line continuation you are supposed to indent
4
> more spaces from "where you should have been."
> 
> BTW, if the style guide doesn't state anything, the rule is to look at the
> codebase for the prevailing pattern.  Ofcourse, having just written that, I
went
> and tried searching for
> 
>    for\ \(.*\=$ lang:^c++$
> 
> on
> 
>    http://code.google.com/p/chromium/source/search
> 
> and consistency on this is...lacking.  Google3 is a little bit more consistent
> so I would indent 4 more spaces.

Somehow I was always thinking of indenting 4 sp from the beginning of the line
:S.
Done.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.h:51: typedef
std::map<WebContents*, int64> GuestWebContentsMap;
On 2012/09/10 19:21:07, awong wrote:
> On 2012/09/10 18:02:47, lazyboy wrote:
> > On 2012/09/10 16:30:17, lazyboy wrote:
> > > On 2012/09/09 18:08:09, awong wrote:
> > > > Move these typedefs into the private section of BrowserPluginEmbedder.
> > > 
> > > Done.
> > 
> > Had to move one back since the browsertest needs it.
> 
> Can we at put them in the public section of BrowserPluginEmbedder then?  Or
> maybe just friend the test?  That reduces pollution of the content namespace.

Moved to public section.
*TEST_F method doesn't run on the context of the BrowserPluginHostTest, so
friending BrowserPluginHostTest doesn't seem to help.
Done.

https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.h:93: virtual void
AddGuest(int instance_id,
On 2012/09/10 19:21:07, awong wrote:
> On 2012/09/10 16:30:17, lazyboy wrote:
> > On 2012/09/09 18:08:09, awong wrote:
> > > Take a scoped_ptr<BrowserPluginGuest> as the argument to indicate
ownership
> > > transfer?
> > 
> > Hmmm, can you explain a bit more? AddGuest is a convenience method to insert
> the
> > guest object ptr in a container here.
> 
> Sorry, I misunderstood the lifetime and though that the embedder deleted the
> guest directly. Instead, the embedder actually only owns the
guest_web_contents.
> 
> This brings up another question.  What if you kept 
> 
>   instance_id -> guest_web_contents
>   guest_web_contents -> frame_id
> 
> Then in GetGuestByInstanceID(), you get the guest via the WebContents pointer?
> 
> The advantage here is BrowserPluginEmbedder only holds pointers to things it
> owns (the web contents) and the relationship of objects becomes more
tree-like.

Done.

https://chromiumcodereview.appspot.com/10868012/diff/48012/content/browser/br...
File content/browser/browser_plugin/test_browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/48012/content/browser/br...
content/browser/browser_plugin/test_browser_plugin_guest.cc:100:
crash_message_loop_runner_->Run();
On 2012/09/10 19:21:07, awong wrote:
> Per mail from phadjdan@, sounds like the right thing to do is set the hang
time
> to TestTimeouts::tiny_timeout().
> 
> Then make it so this (and any other WaitForXXX event) doesn't wait
indefinitely.
>  Instead, make it wait for TestTimeouts::action_timeout().
> 
> For this code, I would post a delayed task with crash_message_loop_runner_'s
> QuitClosure(), and possibly set a flag that tells if this timedout.  So maybe
> 
> // Come up with a better name.
> class Foo {
>  public:
>   Foo() : timed_out_(false) {}
> 
>   void OnTimeout(const Closure& done) {
>     timed_out_ = true;
>     done->Run();
>   }
> 
>   bool timed_out() { return timed_out_; }
> 
>  private:
>   bool timed_out_;
> }
> 
> bool TestBrowserPluginGuest::WaitForCrashed() {
>   // Check if we already observed a guest crash, return immediately if so.
>   if (crash_observed_)
>     return;
>   Foo f;
>   WeakPtrFactory<Foo> weak_factory(&f);  // So the task cancels on function
> exit.
> 
>   crash_message_loop_runner_ = new MessageLoopRunner();
>   MessageLoop::current()->PostDelayedTask(
>      FROM_HERE, 
>      Bind(&Foo::OnTimeout, weak_factor.GetWeakPtr(),
>           crash_message_loop_runner_.QuitClosure().
>      TestTimeouts::action_timeout());
>   crash_message_loop_runner_->Run();
> 
>   return f.timed_out;
> }

Using tiny_timeout() for guest crash timeout.
I've changed all the WaitForXXX methods to do use action_timeout() timeout; I am
still learning about WeakPtr, pretty much copied the code you showed here for
now =P

awong

LGTM w/ nits Ship it! Thanks for enduring through all the review comments and especially ...

8 years, 3 months ago (2012-09-10 23:57:31 UTC) #56

awong

Forgot to respond to comments. Responses inline. On Mon, Sep 10, 2012 at 4:40 PM, ...

8 years, 3 months ago (2012-09-11 00:10:13 UTC) #57

Forgot to respond to comments. Responses inline.

On Mon, Sep 10, 2012 at 4:40 PM, <lazyboy@chromium.org> wrote:

> Main two concerns are addressed: a. removing indefinitely running tests
> while
> waiting + hanging 5s b. remove storing BPGuest* in embedder, storing
> WebContents* instead.
>
> My comments/questions:
> a. kGuestHangTimeoutMs overriding in test is ugly, any better way to do
> this
> kind of thing?
>

I think what you have (storing a member variable) is probably the best.
 Though I just skimmed through your code again and I don't see wher eyou're
overriding this value to be the tiny timeout.  Where is that happening? I
would have expected to see a SetHangTimeoutForTest() function or similar.


> b. The Map typedef name in BPGuest might (or might not) be off since we're
> storing different things, we could probably use better typedef and/or
> member
> names.
>

Name looks okay for for now.


> c. TestTimeoutTracker (or whatever we end up calling it) seems better to be
> moved in some common testing package in future if we find it useful for
> other
> tests.


On the otherhand, we rarely write test that check for hangs.  So this might
be less generally used.  I think leaving it here for now is okay.  It's not
all that complicated of a class.



> https://chromiumcodereview.**appspot.com/10868012/diff/**
>
38062/content/browser/browser_**plugin/browser_plugin_guest.cc<https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/browser_plugin/browser_plugin_guest.cc>
> File content/browser/browser_**plugin/browser_plugin_guest.cc (right):
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
>
38062/content/browser/browser_**plugin/browser_plugin_guest.**cc#newcode90<https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/browser_plugin/browser_plugin_guest.cc#newcode90>
> content/browser/browser_**plugin/browser_plugin_guest.**cc:90: // factor.
> On 2012/09/10 19:21:07, awong wrote:
>
>> On 2012/09/10 16:24:11, Fady Samuel wrote:
>> > On 2012/09/08 02:12:22, lazyboy wrote:
>> > > Fady to comment on this one.
>> > >
>> > > On 2012/09/07 20:51:03, awong wrote:
>> > > > On 2012/09/06 20:56:10, Fady Samuel wrote:
>> > > > > On 2012/09/06 19:55:26, awong wrote:
>> > > > > > If the scale factor is unequal (I presume this can happen if
>>
> the guest
>
>> > > > > navigates
>> > > > > > to a domain that has a different host zoom setting?) won't
>>
> this leave
>
>> > the
>> > > > > guest
>> > > > > > with a stale image?
>> > > > > >
>> > > > > > Should be a bug?
>> > > > >
>> > > > > Scaling has nothing to do with zooming.
>> > > > >
>> > > > > Scaling is used on high-DPI screens (like retina display
>>
> MacBook Pros).
>
>> > > > >
>> > > > > Scaling can change if we move a window from a high-DPI screen
>>
> to a
>
>> low-DPI
>> > > > > screen or vice versa.
>> > > >
>> > > > Ah. So should this really be a DCHECK?
>> > > >
>> > > > I'm trying to understand what then this clause is skipped and
>>
> what the
>
>> > > > consequences of that are.
>> > >
>> >
>> > No, this shouldn't be a DCHECK. When we change the DPI on ChromeOS,
>>
> the UI's
>
>> DPI
>> > changes, and it tells the WebContents to change its DPI. This is an
>>
> async
>
>> > process, and for a small period of time (milliseconds) the guest's
>>
> WebContents
>
>> > may be renderered at a different DPI than the embedder's.
>>
>
>  Ah hah! That's the piece I was missing.  Can we add a line saying that
>>
> this can
>
>> happen due to asynchronous updates of the DPI on a resolution change?
>>
>
> Added the line.
> Done.
>
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
>
42040/content/browser/browser_**plugin/browser_plugin_guest.h<https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/browser_plugin/browser_plugin_guest.h>
> File content/browser/browser_**plugin/browser_plugin_guest.h (right):
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
>
42040/content/browser/browser_**plugin/browser_plugin_guest.h#**newcode9<https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/browser_plugin/browser_plugin_guest.h#newcode9>
> content/browser/browser_**plugin/browser_plugin_guest.h:**9: // object
> receives messages (RenderViewHostMsg_*) directed at the browser plugin
> On 2012/09/10 19:21:07, awong wrote:
>
>> On 2012/09/10 16:24:11, Fady Samuel wrote:
>> > On 2012/09/08 02:12:22, lazyboy wrote:
>> > > On 2012/09/07 20:51:03, awong wrote:
>> > > > Is there a way to know which RenderViewHostMsg_* we want to
>>
> listen for
>
>> > > > specifically?  Do we just want to grab each message that has
>>
> analogous
>
>> > handler
>> > > > in WebPlugin?
>> > > >
>> > > > Stating the constraint would be useful for future maintainers
>>
> who want to
>
>> > > modify
>> > > > or validate the implementation.
>> > >
>> > > Fady can you answer this one? Thanks.
>> >
>> > Firstly, I believe that should say ViewHostMsg_* rather than
>> > RenderViewHostMsg_*.
>> >
>> > Secondly, I don't have a simple answer to this question.
>> >
>> > Basically, anything the embedder might be interested in knowing or
>>
> modifying
>
>> > about the guest should be listened for here.
>>
>
>  Yeah...you're right. feel free to ignore my comment.
>>
>
> Fixed the note here saying ViewHostMsg_* instead of RenderViewHostMsg*_;
> also added the comment saying we intercept anything that is interesting.
>
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
>
41043/content/browser/browser_**plugin/browser_plugin_**embedder.cc<https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/browser_plugin/browser_plugin_embedder.cc>
> File content/browser/browser_**plugin/browser_plugin_**embedder.cc
> (right):
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
> 41043/content/browser/browser_**plugin/browser_plugin_**
>
embedder.cc#newcode98<https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/browser_plugin/browser_plugin_embedder.cc#newcode98>
> content/browser/browser_**plugin/browser_plugin_**embedder.cc:98: // than
> the URL that the guest is being navigated to.
> On 2012/09/10 19:21:07, awong wrote:
>
>> On 2012/09/10 16:30:17, lazyboy wrote:
>> > On 2012/09/09 18:08:09, awong wrote:
>> > > This comment seems to make more sense in CreateGuest.
>> >
>> > Hmmm, I also have this comment in CreateGuest, removed from here.
>>
>
>  Still seems to be here...
>>
>
> Missed it somehow :(
> Done.
>
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
> 41043/content/browser/browser_**plugin/browser_plugin_**
>
embedder.cc#newcode184<https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/browser_plugin/browser_plugin_embedder.cc#newcode184>
> content/browser/browser_**plugin/browser_plugin_**embedder.cc:184:
> guest->Destroy();
> On 2012/09/10 19:21:07, awong wrote:
>
>> On 2012/09/10 16:30:17, lazyboy wrote:
>> > On 2012/09/09 18:08:09, awong wrote:
>> > > Do we need to delete guest here?
>> > >
>> > > Maybe we should have a map of linked_ptrs?
>> >
>> >
>> > guest->Destroy() would delete the web_contents() associated with the
>>
> guest,
>
>> > since the guest is a WCObserver, it would delete the guest as well.
>>
>
>  Ah, I see now.  It's weird to have 2 mechanisms for managing
>>
> destruction of the
>
>> same set of pointers.  Makes tracking ownership hard. Also, in the
>>
> future, if
>
>> someone wanted to add some behavior on destruction of a guest, there's
>>
> a higher
>
>> chance that you'd
>>
>
>  AFAICT, this is only place that class guest->Destroy().  Maybe we
>>
> should
>
>> completely remove the Destroy() method and just delete
>>
> guest_web_contents
>
>> manually?
>>
>
> Agreed, I actually thought of destroying the guests in a consistent
> manner, but forgot. (Although I was thinking the otherway, always
> calling Destroy())
> Done.
>
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
> 41043/content/browser/browser_**plugin/browser_plugin_**
>
embedder.cc#newcode220<https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/browser_plugin/browser_plugin_embedder.cc#newcode220>
> content/browser/browser_**plugin/browser_plugin_**embedder.cc:220:
> guest_web_contents_container_.**begin();
> On 2012/09/10 19:21:07, awong wrote:
>
>> On 2012/09/10 16:30:17, lazyboy wrote:
>> > On 2012/09/09 18:08:09, awong wrote:
>> > > indentation. off by 5 spaces.
>> >
>> > Ah, hit this case, what happens in this case when the expressions in
>>
> for() is
>
>> > long (I couldn't seem to find it in style guide):
>> >
>> > for (my_long_var ... =
>> >     this_is_continuation_4_space_**indentation;
>> >      second_expression_regular_(_**indent; third...)
>> > or just indent everything 4 spaces or 5 spaces?
>>
>
>  Yeah, it's confusing.  My understanding of the style guide is it
>>
> should be:
>
>    for (my_long_var ... =
>>            this_is_continuation_4_space_**indentation;
>>        second_expression_regular_(_**indent; third...)
>>
>
>  Since, without a line break, the next line is supposed to line up with
>>
> the "m"
>
>> in "my_long_var", when you do the line continuation you are supposed
>>
> to indent 4
>
>> more spaces from "where you should have been."
>>
>
>  BTW, if the style guide doesn't state anything, the rule is to look at
>>
> the
>
>> codebase for the prevailing pattern.  Ofcourse, having just written
>>
> that, I went
>
>> and tried searching for
>>
>
>      for\ \(.*\=$ lang:^c++$
>>
>
>  on
>>
>
>     
http://code.google.com/p/**chromium/source/search<http://code.google.com/p/ch...
>>
>
>  and consistency on this is...lacking.  Google3 is a little bit more
>>
> consistent
>
>> so I would indent 4 more spaces.
>>
>
> Somehow I was always thinking of indenting 4 sp from the beginning of
> the line :S.
> Done.
>
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
>
41043/content/browser/browser_**plugin/browser_plugin_**embedder.h<https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/browser_plugin/browser_plugin_embedder.h>
> File content/browser/browser_**plugin/browser_plugin_**embedder.h (right):
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
> 41043/content/browser/browser_**plugin/browser_plugin_**
>
embedder.h#newcode51<https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/browser_plugin/browser_plugin_embedder.h#newcode51>
> content/browser/browser_**plugin/browser_plugin_**embedder.h:51: typedef
> std::map<WebContents*, int64> GuestWebContentsMap;
> On 2012/09/10 19:21:07, awong wrote:
>
>> On 2012/09/10 18:02:47, lazyboy wrote:
>> > On 2012/09/10 16:30:17, lazyboy wrote:
>> > > On 2012/09/09 18:08:09, awong wrote:
>> > > > Move these typedefs into the private section of
>>
> BrowserPluginEmbedder.
>
>> > >
>> > > Done.
>> >
>> > Had to move one back since the browsertest needs it.
>>
>
>  Can we at put them in the public section of BrowserPluginEmbedder
>>
> then?  Or
>
>> maybe just friend the test?  That reduces pollution of the content
>>
> namespace.
>
> Moved to public section.
> *TEST_F method doesn't run on the context of the BrowserPluginHostTest,
> so friending BrowserPluginHostTest doesn't seem to help.
> Done.
>
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
> 41043/content/browser/browser_**plugin/browser_plugin_**
>
embedder.h#newcode93<https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/browser_plugin/browser_plugin_embedder.h#newcode93>
> content/browser/browser_**plugin/browser_plugin_**embedder.h:93: virtual
> void AddGuest(int instance_id,
> On 2012/09/10 19:21:07, awong wrote:
>
>> On 2012/09/10 16:30:17, lazyboy wrote:
>> > On 2012/09/09 18:08:09, awong wrote:
>> > > Take a scoped_ptr<BrowserPluginGuest> as the argument to indicate
>>
> ownership
>
>> > > transfer?
>> >
>> > Hmmm, can you explain a bit more? AddGuest is a convenience method
>>
> to insert
>
>> the
>> > guest object ptr in a container here.
>>
>
>  Sorry, I misunderstood the lifetime and though that the embedder
>>
> deleted the
>
>> guest directly. Instead, the embedder actually only owns the
>>
> guest_web_contents.
>
>  This brings up another question.  What if you kept
>>
>
>     instance_id -> guest_web_contents
>>    guest_web_contents -> frame_id
>>
>
>  Then in GetGuestByInstanceID(), you get the guest via the WebContents
>>
> pointer?
>
>  The advantage here is BrowserPluginEmbedder only holds pointers to
>>
> things it
>
>> owns (the web contents) and the relationship of objects becomes more
>>
> tree-like.
>
> Done.
>
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
>
48012/content/browser/browser_**plugin/test_browser_plugin_**guest.cc<https://chromiumcodereview.appspot.com/10868012/diff/48012/content/browser/browser_plugin/test_browser_plugin_guest.cc>
> File content/browser/browser_**plugin/test_browser_plugin_**guest.cc
> (right):
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
> 48012/content/browser/browser_**plugin/test_browser_plugin_**
>
guest.cc#newcode100<https://chromiumcodereview.appspot.com/10868012/diff/48012/content/browser/browser_plugin/test_browser_plugin_guest.cc#newcode100>
> content/browser/browser_**plugin/test_browser_plugin_**guest.cc:100:
> crash_message_loop_runner_->**Run();
> On 2012/09/10 19:21:07, awong wrote:
>
>> Per mail from phadjdan@, sounds like the right thing to do is set the
>>
> hang time
>
>> to TestTimeouts::tiny_timeout().
>>
>
>  Then make it so this (and any other WaitForXXX event) doesn't wait
>>
> indefinitely.
>
>>   Instead, make it wait for TestTimeouts::action_timeout()**.
>>
>
>  For this code, I would post a delayed task with
>>
> crash_message_loop_runner_'s
>
>> QuitClosure(), and possibly set a flag that tells if this timedout.
>>
> So maybe
>
>  // Come up with a better name.
>> class Foo {
>>   public:
>>    Foo() : timed_out_(false) {}
>>
>
>     void OnTimeout(const Closure& done) {
>>      timed_out_ = true;
>>      done->Run();
>>    }
>>
>
>     bool timed_out() { return timed_out_; }
>>
>
>    private:
>>    bool timed_out_;
>> }
>>
>
>  bool TestBrowserPluginGuest::**WaitForCrashed() {
>>    // Check if we already observed a guest crash, return immediately if
>>
> so.
>
>>    if (crash_observed_)
>>      return;
>>    Foo f;
>>    WeakPtrFactory<Foo> weak_factory(&f);  // So the task cancels on
>>
> function
>
>> exit.
>>
>
>     crash_message_loop_runner_ = new MessageLoopRunner();
>>    MessageLoop::current()->**PostDelayedTask(
>>       FROM_HERE,
>>       Bind(&Foo::OnTimeout, weak_factor.GetWeakPtr(),
>>            crash_message_loop_runner_.**QuitClosure().
>>       TestTimeouts::action_timeout()**);
>>    crash_message_loop_runner_->**Run();
>>
>
>     return f.timed_out;
>> }
>>
>
> Using tiny_timeout() for guest crash timeout.
> I've changed all the WaitForXXX methods to do use action_timeout()
> timeout; I am still learning about WeakPtr, pretty much copied the code
> you showed here for now =P
>
>
https://chromiumcodereview.**appspot.com/10868012/<https://chromiumcodereview...
>

awong

On Mon, Sep 10, 2012 at 5:09 PM, Albert J. Wong (王重傑) <ajwong@chromium.org>wrote: > Forgot ...

8 years, 3 months ago (2012-09-11 00:13:43 UTC) #58

On Mon, Sep 10, 2012 at 5:09 PM, Albert J. Wong (王重傑)
<ajwong@chromium.org>wrote:

> Forgot to respond to comments. Responses inline.
>
> On Mon, Sep 10, 2012 at 4:40 PM, <lazyboy@chromium.org> wrote:
>
>> Main two concerns are addressed: a. removing indefinitely running tests
>> while
>> waiting + hanging 5s b. remove storing BPGuest* in embedder, storing
>> WebContents* instead.
>>
>> My comments/questions:
>> a. kGuestHangTimeoutMs overriding in test is ugly, any better way to do
>> this
>> kind of thing?
>>
>
> I think what you have (storing a member variable) is probably the best.
>  Though I just skimmed through your code again and I don't see wher eyou're
> overriding this value to be the tiny timeout.  Where is that happening? I
> would have expected to see a SetHangTimeoutForTest() function or similar.
>

Blah...skimming too fast.  I found it.  My only suggestion would be to add
a function like I said (SetGuestHangTimeoutForTest(base::TimeDelta
timeout)) and call that from TestBrowserPluginGuest() rather than directly
reaching into the private variable.


>
>> b. The Map typedef name in BPGuest might (or might not) be off since we're
>> storing different things, we could probably use better typedef and/or
>> member
>> names.
>>
>
> Name looks okay for for now.
>
>
>> c. TestTimeoutTracker (or whatever we end up calling it) seems better to
>> be
>> moved in some common testing package in future if we find it useful for
>> other
>> tests.
>
>
> On the otherhand, we rarely write test that check for hangs.  So this
> might be less generally used.  I think leaving it here for now is okay.
>  It's not all that complicated of a class.
>
>
>
>> https://chromiumcodereview.**appspot.com/10868012/diff/**
>>
38062/content/browser/browser_**plugin/browser_plugin_guest.cc<https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/browser_plugin/browser_plugin_guest.cc>
>> File content/browser/browser_**plugin/browser_plugin_guest.cc (right):
>>
>> https://chromiumcodereview.**appspot.com/10868012/diff/**
>> 38062/content/browser/browser_**plugin/browser_plugin_guest.**
>>
cc#newcode90<https://chromiumcodereview.appspot.com/10868012/diff/38062/content/browser/browser_plugin/browser_plugin_guest.cc#newcode90>
>> content/browser/browser_**plugin/browser_plugin_guest.**cc:90: // factor.
>> On 2012/09/10 19:21:07, awong wrote:
>>
>>> On 2012/09/10 16:24:11, Fady Samuel wrote:
>>> > On 2012/09/08 02:12:22, lazyboy wrote:
>>> > > Fady to comment on this one.
>>> > >
>>> > > On 2012/09/07 20:51:03, awong wrote:
>>> > > > On 2012/09/06 20:56:10, Fady Samuel wrote:
>>> > > > > On 2012/09/06 19:55:26, awong wrote:
>>> > > > > > If the scale factor is unequal (I presume this can happen if
>>>
>> the guest
>>
>>> > > > > navigates
>>> > > > > > to a domain that has a different host zoom setting?) won't
>>>
>> this leave
>>
>>> > the
>>> > > > > guest
>>> > > > > > with a stale image?
>>> > > > > >
>>> > > > > > Should be a bug?
>>> > > > >
>>> > > > > Scaling has nothing to do with zooming.
>>> > > > >
>>> > > > > Scaling is used on high-DPI screens (like retina display
>>>
>> MacBook Pros).
>>
>>> > > > >
>>> > > > > Scaling can change if we move a window from a high-DPI screen
>>>
>> to a
>>
>>> low-DPI
>>> > > > > screen or vice versa.
>>> > > >
>>> > > > Ah. So should this really be a DCHECK?
>>> > > >
>>> > > > I'm trying to understand what then this clause is skipped and
>>>
>> what the
>>
>>> > > > consequences of that are.
>>> > >
>>> >
>>> > No, this shouldn't be a DCHECK. When we change the DPI on ChromeOS,
>>>
>> the UI's
>>
>>> DPI
>>> > changes, and it tells the WebContents to change its DPI. This is an
>>>
>> async
>>
>>> > process, and for a small period of time (milliseconds) the guest's
>>>
>> WebContents
>>
>>> > may be renderered at a different DPI than the embedder's.
>>>
>>
>>  Ah hah! That's the piece I was missing.  Can we add a line saying that
>>>
>> this can
>>
>>> happen due to asynchronous updates of the DPI on a resolution change?
>>>
>>
>> Added the line.
>> Done.
>>
>>
>> https://chromiumcodereview.**appspot.com/10868012/diff/**
>>
42040/content/browser/browser_**plugin/browser_plugin_guest.h<https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/browser_plugin/browser_plugin_guest.h>
>> File content/browser/browser_**plugin/browser_plugin_guest.h (right):
>>
>> https://chromiumcodereview.**appspot.com/10868012/diff/**
>>
42040/content/browser/browser_**plugin/browser_plugin_guest.h#**newcode9<https://chromiumcodereview.appspot.com/10868012/diff/42040/content/browser/browser_plugin/browser_plugin_guest.h#newcode9>
>> content/browser/browser_**plugin/browser_plugin_guest.h:**9: // object
>> receives messages (RenderViewHostMsg_*) directed at the browser plugin
>> On 2012/09/10 19:21:07, awong wrote:
>>
>>> On 2012/09/10 16:24:11, Fady Samuel wrote:
>>> > On 2012/09/08 02:12:22, lazyboy wrote:
>>> > > On 2012/09/07 20:51:03, awong wrote:
>>> > > > Is there a way to know which RenderViewHostMsg_* we want to
>>>
>> listen for
>>
>>> > > > specifically?  Do we just want to grab each message that has
>>>
>> analogous
>>
>>> > handler
>>> > > > in WebPlugin?
>>> > > >
>>> > > > Stating the constraint would be useful for future maintainers
>>>
>> who want to
>>
>>> > > modify
>>> > > > or validate the implementation.
>>> > >
>>> > > Fady can you answer this one? Thanks.
>>> >
>>> > Firstly, I believe that should say ViewHostMsg_* rather than
>>> > RenderViewHostMsg_*.
>>> >
>>> > Secondly, I don't have a simple answer to this question.
>>> >
>>> > Basically, anything the embedder might be interested in knowing or
>>>
>> modifying
>>
>>> > about the guest should be listened for here.
>>>
>>
>>  Yeah...you're right. feel free to ignore my comment.
>>>
>>
>> Fixed the note here saying ViewHostMsg_* instead of RenderViewHostMsg*_;
>> also added the comment saying we intercept anything that is interesting.
>>
>>
>> https://chromiumcodereview.**appspot.com/10868012/diff/**
>>
41043/content/browser/browser_**plugin/browser_plugin_**embedder.cc<https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/browser_plugin/browser_plugin_embedder.cc>
>> File content/browser/browser_**plugin/browser_plugin_**embedder.cc
>> (right):
>>
>> https://chromiumcodereview.**appspot.com/10868012/diff/**
>> 41043/content/browser/browser_**plugin/browser_plugin_**
>>
embedder.cc#newcode98<https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/browser_plugin/browser_plugin_embedder.cc#newcode98>
>> content/browser/browser_**plugin/browser_plugin_**embedder.cc:98: // than
>> the URL that the guest is being navigated to.
>> On 2012/09/10 19:21:07, awong wrote:
>>
>>> On 2012/09/10 16:30:17, lazyboy wrote:
>>> > On 2012/09/09 18:08:09, awong wrote:
>>> > > This comment seems to make more sense in CreateGuest.
>>> >
>>> > Hmmm, I also have this comment in CreateGuest, removed from here.
>>>
>>
>>  Still seems to be here...
>>>
>>
>> Missed it somehow :(
>> Done.
>>
>>
>> https://chromiumcodereview.**appspot.com/10868012/diff/**
>> 41043/content/browser/browser_**plugin/browser_plugin_**
>>
embedder.cc#newcode184<https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/browser_plugin/browser_plugin_embedder.cc#newcode184>
>> content/browser/browser_**plugin/browser_plugin_**embedder.cc:184:
>> guest->Destroy();
>> On 2012/09/10 19:21:07, awong wrote:
>>
>>> On 2012/09/10 16:30:17, lazyboy wrote:
>>> > On 2012/09/09 18:08:09, awong wrote:
>>> > > Do we need to delete guest here?
>>> > >
>>> > > Maybe we should have a map of linked_ptrs?
>>> >
>>> >
>>> > guest->Destroy() would delete the web_contents() associated with the
>>>
>> guest,
>>
>>> > since the guest is a WCObserver, it would delete the guest as well.
>>>
>>
>>  Ah, I see now.  It's weird to have 2 mechanisms for managing
>>>
>> destruction of the
>>
>>> same set of pointers.  Makes tracking ownership hard. Also, in the
>>>
>> future, if
>>
>>> someone wanted to add some behavior on destruction of a guest, there's
>>>
>> a higher
>>
>>> chance that you'd
>>>
>>
>>  AFAICT, this is only place that class guest->Destroy().  Maybe we
>>>
>> should
>>
>>> completely remove the Destroy() method and just delete
>>>
>> guest_web_contents
>>
>>> manually?
>>>
>>
>> Agreed, I actually thought of destroying the guests in a consistent
>> manner, but forgot. (Although I was thinking the otherway, always
>> calling Destroy())
>> Done.
>>
>>
>> https://chromiumcodereview.**appspot.com/10868012/diff/**
>> 41043/content/browser/browser_**plugin/browser_plugin_**
>>
embedder.cc#newcode220<https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/browser_plugin/browser_plugin_embedder.cc#newcode220>
>> content/browser/browser_**plugin/browser_plugin_**embedder.cc:220:
>> guest_web_contents_container_.**begin();
>> On 2012/09/10 19:21:07, awong wrote:
>>
>>> On 2012/09/10 16:30:17, lazyboy wrote:
>>> > On 2012/09/09 18:08:09, awong wrote:
>>> > > indentation. off by 5 spaces.
>>> >
>>> > Ah, hit this case, what happens in this case when the expressions in
>>>
>> for() is
>>
>>> > long (I couldn't seem to find it in style guide):
>>> >
>>> > for (my_long_var ... =
>>> >     this_is_continuation_4_space_**indentation;
>>> >      second_expression_regular_(_**indent; third...)
>>> > or just indent everything 4 spaces or 5 spaces?
>>>
>>
>>  Yeah, it's confusing.  My understanding of the style guide is it
>>>
>> should be:
>>
>>    for (my_long_var ... =
>>>            this_is_continuation_4_space_**indentation;
>>>        second_expression_regular_(_**indent; third...)
>>>
>>
>>  Since, without a line break, the next line is supposed to line up with
>>>
>> the "m"
>>
>>> in "my_long_var", when you do the line continuation you are supposed
>>>
>> to indent 4
>>
>>> more spaces from "where you should have been."
>>>
>>
>>  BTW, if the style guide doesn't state anything, the rule is to look at
>>>
>> the
>>
>>> codebase for the prevailing pattern.  Ofcourse, having just written
>>>
>> that, I went
>>
>>> and tried searching for
>>>
>>
>>      for\ \(.*\=$ lang:^c++$
>>>
>>
>>  on
>>>
>>
>>     
http://code.google.com/p/**chromium/source/search<http://code.google.com/p/ch...
>>>
>>
>>  and consistency on this is...lacking.  Google3 is a little bit more
>>>
>> consistent
>>
>>> so I would indent 4 more spaces.
>>>
>>
>> Somehow I was always thinking of indenting 4 sp from the beginning of
>> the line :S.
>> Done.
>>
>>
>> https://chromiumcodereview.**appspot.com/10868012/diff/**
>>
41043/content/browser/browser_**plugin/browser_plugin_**embedder.h<https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/browser_plugin/browser_plugin_embedder.h>
>> File content/browser/browser_**plugin/browser_plugin_**embedder.h
>> (right):
>>
>> https://chromiumcodereview.**appspot.com/10868012/diff/**
>> 41043/content/browser/browser_**plugin/browser_plugin_**
>>
embedder.h#newcode51<https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/browser_plugin/browser_plugin_embedder.h#newcode51>
>> content/browser/browser_**plugin/browser_plugin_**embedder.h:51: typedef
>> std::map<WebContents*, int64> GuestWebContentsMap;
>> On 2012/09/10 19:21:07, awong wrote:
>>
>>> On 2012/09/10 18:02:47, lazyboy wrote:
>>> > On 2012/09/10 16:30:17, lazyboy wrote:
>>> > > On 2012/09/09 18:08:09, awong wrote:
>>> > > > Move these typedefs into the private section of
>>>
>> BrowserPluginEmbedder.
>>
>>> > >
>>> > > Done.
>>> >
>>> > Had to move one back since the browsertest needs it.
>>>
>>
>>  Can we at put them in the public section of BrowserPluginEmbedder
>>>
>> then?  Or
>>
>>> maybe just friend the test?  That reduces pollution of the content
>>>
>> namespace.
>>
>> Moved to public section.
>> *TEST_F method doesn't run on the context of the BrowserPluginHostTest,
>> so friending BrowserPluginHostTest doesn't seem to help.
>> Done.
>>
>>
>> https://chromiumcodereview.**appspot.com/10868012/diff/**
>> 41043/content/browser/browser_**plugin/browser_plugin_**
>>
embedder.h#newcode93<https://chromiumcodereview.appspot.com/10868012/diff/41043/content/browser/browser_plugin/browser_plugin_embedder.h#newcode93>
>> content/browser/browser_**plugin/browser_plugin_**embedder.h:93: virtual
>> void AddGuest(int instance_id,
>> On 2012/09/10 19:21:07, awong wrote:
>>
>>> On 2012/09/10 16:30:17, lazyboy wrote:
>>> > On 2012/09/09 18:08:09, awong wrote:
>>> > > Take a scoped_ptr<BrowserPluginGuest> as the argument to indicate
>>>
>> ownership
>>
>>> > > transfer?
>>> >
>>> > Hmmm, can you explain a bit more? AddGuest is a convenience method
>>>
>> to insert
>>
>>> the
>>> > guest object ptr in a container here.
>>>
>>
>>  Sorry, I misunderstood the lifetime and though that the embedder
>>>
>> deleted the
>>
>>> guest directly. Instead, the embedder actually only owns the
>>>
>> guest_web_contents.
>>
>>  This brings up another question.  What if you kept
>>>
>>
>>     instance_id -> guest_web_contents
>>>    guest_web_contents -> frame_id
>>>
>>
>>  Then in GetGuestByInstanceID(), you get the guest via the WebContents
>>>
>> pointer?
>>
>>  The advantage here is BrowserPluginEmbedder only holds pointers to
>>>
>> things it
>>
>>> owns (the web contents) and the relationship of objects becomes more
>>>
>> tree-like.
>>
>> Done.
>>
>>
>> https://chromiumcodereview.**appspot.com/10868012/diff/**
>>
48012/content/browser/browser_**plugin/test_browser_plugin_**guest.cc<https://chromiumcodereview.appspot.com/10868012/diff/48012/content/browser/browser_plugin/test_browser_plugin_guest.cc>
>> File content/browser/browser_**plugin/test_browser_plugin_**guest.cc
>> (right):
>>
>> https://chromiumcodereview.**appspot.com/10868012/diff/**
>> 48012/content/browser/browser_**plugin/test_browser_plugin_**
>>
guest.cc#newcode100<https://chromiumcodereview.appspot.com/10868012/diff/48012/content/browser/browser_plugin/test_browser_plugin_guest.cc#newcode100>
>> content/browser/browser_**plugin/test_browser_plugin_**guest.cc:100:
>> crash_message_loop_runner_->**Run();
>> On 2012/09/10 19:21:07, awong wrote:
>>
>>> Per mail from phadjdan@, sounds like the right thing to do is set the
>>>
>> hang time
>>
>>> to TestTimeouts::tiny_timeout().
>>>
>>
>>  Then make it so this (and any other WaitForXXX event) doesn't wait
>>>
>> indefinitely.
>>
>>>   Instead, make it wait for TestTimeouts::action_timeout()**.
>>>
>>
>>  For this code, I would post a delayed task with
>>>
>> crash_message_loop_runner_'s
>>
>>> QuitClosure(), and possibly set a flag that tells if this timedout.
>>>
>> So maybe
>>
>>  // Come up with a better name.
>>> class Foo {
>>>   public:
>>>    Foo() : timed_out_(false) {}
>>>
>>
>>     void OnTimeout(const Closure& done) {
>>>      timed_out_ = true;
>>>      done->Run();
>>>    }
>>>
>>
>>     bool timed_out() { return timed_out_; }
>>>
>>
>>    private:
>>>    bool timed_out_;
>>> }
>>>
>>
>>  bool TestBrowserPluginGuest::**WaitForCrashed() {
>>>    // Check if we already observed a guest crash, return immediately if
>>>
>> so.
>>
>>>    if (crash_observed_)
>>>      return;
>>>    Foo f;
>>>    WeakPtrFactory<Foo> weak_factory(&f);  // So the task cancels on
>>>
>> function
>>
>>> exit.
>>>
>>
>>     crash_message_loop_runner_ = new MessageLoopRunner();
>>>    MessageLoop::current()->**PostDelayedTask(
>>>       FROM_HERE,
>>>       Bind(&Foo::OnTimeout, weak_factor.GetWeakPtr(),
>>>            crash_message_loop_runner_.**QuitClosure().
>>>       TestTimeouts::action_timeout()**);
>>>    crash_message_loop_runner_->**Run();
>>>
>>
>>     return f.timed_out;
>>> }
>>>
>>
>> Using tiny_timeout() for guest crash timeout.
>> I've changed all the WaitForXXX methods to do use action_timeout()
>> timeout; I am still learning about WeakPtr, pretty much copied the code
>> you showed here for now =P
>>
>>
https://chromiumcodereview.**appspot.com/10868012/<https://chromiumcodereview...
>>
>
>

lazyboy

Thanks a lot for the review! Since Charlie is returning tomorrow, he probably can give ...

8 years, 3 months ago (2012-09-11 19:58:38 UTC) #59

jam

(Didn't review this, just looked at content/public) https://chromiumcodereview.appspot.com/10868012/diff/48015/content/public/browser/web_contents.h File content/public/browser/web_contents.h (right): https://chromiumcodereview.appspot.com/10868012/diff/48015/content/public/browser/web_contents.h#newcode81 content/public/browser/web_contents.h:81: CONTENT_EXPORT static ...

8 years, 3 months ago (2012-09-11 20:10:03 UTC) #60

lazyboy

https://chromiumcodereview.appspot.com/10868012/diff/48015/content/public/browser/web_contents.h File content/public/browser/web_contents.h (right): https://chromiumcodereview.appspot.com/10868012/diff/48015/content/public/browser/web_contents.h#newcode81 content/public/browser/web_contents.h:81: CONTENT_EXPORT static WebContents* CreateGuest( On 2012/09/11 20:10:03, John Abd-El-Malek ...

8 years, 3 months ago (2012-09-11 20:50:53 UTC) #61

lazyboy

Hey Charlie, I need content owner LG for the CL. Unreviewed changes after Albert's LG ...

8 years, 3 months ago (2012-09-12 18:58:13 UTC) #62

Charlie Reis

Glad to see you all made a lot of progress. A few nits, plus some ...

8 years, 3 months ago (2012-09-13 00:51:43 UTC) #63

Fady Samuel

http://codereview.chromium.org/10868012/diff/38076/content/browser/browser_plugin/browser_plugin_guest.cc File content/browser/browser_plugin/browser_plugin_guest.cc (right): http://codereview.chromium.org/10868012/diff/38076/content/browser/browser_plugin/browser_plugin_guest.cc#newcode193 content/browser/browser_plugin/browser_plugin_guest.cc:193: // TODO(fsamuel): What do we need to do here? ...

8 years, 3 months ago (2012-09-13 14:17:18 UTC) #64

http://codereview.chromium.org/10868012/diff/38076/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

http://codereview.chromium.org/10868012/diff/38076/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:193: // TODO(fsamuel):
What do we need to do here? This is for keyboard shortcuts.
On 2012/09/13 00:51:44, creis wrote:
> Can this TODO be resolved?  (I can't tell from the comment whether something
is
> broken here or not.)

Really at the time (and now), I'm not really sure I understand what this bool
does.  RenderWidgetHostImpl::ForwardInputEvent suggests that this is related to
keyboard shortcuts.

I've verified that Ctrl+C on Linux inside a guest works just fine. Again,
something may be broken, but I don't know what yet :P

I will investigate further.

http://codereview.chromium.org/10868012/diff/38076/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_impl.cc (right):

http://codereview.chromium.org/10868012/diff/38076/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_impl.cc:831: // Allow
BrowserPluginHostMsg_* sync messages to run on the UI thread.
On 2012/09/13 00:51:44, creis wrote:
> This doesn't sound safe.  Why is it ok to process sync messages on the UI
thread
> here, if we've gone out of our way to prevent it otherwise?

I've copied-and-pasted BrowserMessageFilter::CheckCanDispatchOnUI. My
understanding is 1. platform apps will not support windowed plugins (asked in an
email thread for confirmation). 2. Browser Plugin messages will not participate
in this deadlock cycle in the comment below.

John or someone else might be able to validate my two assumptions/claims above.

bool BrowserMessageFilter::CheckCanDispatchOnUI(const IPC::Message& message,
                                                IPC::Sender* sender) {
#if defined(OS_WIN) && !defined(USE_AURA)
  // On Windows there's a potential deadlock with sync messsages going in
  // a circle from browser -> plugin -> renderer -> browser.
  // On Linux we can avoid this by avoiding sync messages from browser->plugin.
  // On Mac we avoid this by not supporting windowed plugins.
  if (message.is_sync() && !message.is_caller_pumping_messages()) {
    // NOTE: IF YOU HIT THIS ASSERT, THE SOLUTION IS ALMOST NEVER TO RUN A
    // NESTED MESSAGE LOOP IN THE RENDERER!!!
    // That introduces reentrancy which causes hard to track bugs.  You should
    // find a way to either turn this into an asynchronous message, or one
    // that can be answered on the IO thread.
    NOTREACHED() << "Can't send sync messages to UI thread without pumping "
        "messages in the renderer or else deadlocks can occur if the page "
        "has windowed plugins! (message type " << message.type() << ")";
    IPC::Message* reply = IPC::SyncMessage::GenerateReply(&message);
    reply->set_reply_error();
    sender->Send(reply);
    return false;
  }
#endif
  return true;
}

lazyboy

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/browser_plugin/browser_plugin_embedder.cc File content/browser/browser_plugin/browser_plugin_embedder.cc (right): https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/browser_plugin/browser_plugin_embedder.cc#newcode54 content/browser/browser_plugin/browser_plugin_embedder.cc:54: // Destroy guests that are managed by the current ...

8 years, 3 months ago (2012-09-13 15:56:23 UTC) #65

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:54: // Destroy guests
that are managed by the current embedder, for more details
On 2012/09/13 00:51:44, creis wrote:
> nit: These are two separate sentences.  (There have been a lot of run-on
> sentences like this in this review, so please keep an eye out for them.)

Ok.
Done.

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:212: // an embedder as
well, call it E2. Now when
On 2012/09/13 00:51:44, creis wrote:
> Is this possible?  I didn't think we could support guests within guests yet.
We don't forcefully disallow it yet, but we will disallow it.
See comments below since we plan to remove this function.
(with the following pluginDestroyed(), we would cover guest inside guest too if
we had it).

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:223: // won't be
cleaned up with the code below; However since WC2 goes away, this
On 2012/09/13 00:51:44, creis wrote:
> How does WC2 go away?  There's no code that I know of that would delete the
> guest's WebContents in this case, nor in the case that the top-level page
> deletes the iframe.
> 
> If there is code for deleting a guest's WebContents when its embedder
navigates
> or destroys it, then I don't see why the code below is needed.

I talked to Fady about this part of the code: it seems we don't need this bit of
code anymore.

(WebPlugin's pluginDestroyed() seems to be good enough for handling deletions of
guests)

The fact that I saw WC2 go away was because of BrowserPlugin.pluginDestroyed()
call from renderer (which I wasn't aware of, and Fady says it was added after
this function was added), which removes the BrowserPluginGuest, that deletes WC
associated with the plugin in browser side; Therefore we don't need any special
handling for deleting guests here.
Fady/Charlie: can you think of any other cases we might need this code for?

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.h:5: // A
BrowserPluginEmbedder has a list of guest it manages.
On 2012/09/13 00:51:44, creis wrote:
> nit: guests

Done.

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.h:10: //
BrowserPluginGuest for each guest in the embedding WebContents.
On 2012/09/13 00:51:44, creis wrote:
> Can you somehow mention that each guest has its own WebContents, which its
> BrowserPluginGuest is associated with?  That isn't obvious from this
> description.

Done.

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.h:16: // embedder frame
navigates away to a different page.
On 2012/09/13 00:51:44, creis wrote:
> ...or deletes the guests from the existing page?  (Do we handle that case
> properly as well?)
Done.
DidCommitProvisionalLoadForFrame(), I believe this gets covered by
pluginDestroyed()

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:18: // for messages like
handleInputEvent, updateGeometry. Such messages gets
On 2012/09/13 00:51:44, creis wrote:
> nit: gets -> get

Done.

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:22: // BrowserPluginGuest
knows about its embedder process, communication to
On 2012/09/13 00:51:44, creis wrote:
> nit: run-on sentence

Done.

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:145: // Overriden in
tests.
On 2012/09/13 00:51:44, creis wrote:
> nit: Overridden
> (Same below)

Done.

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest_helper.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest_helper.h:38: // special
handling, which can be move to a message filter (IPC thread) for
On 2012/09/13 00:51:44, creis wrote:
> nit: move -> moved

Done.

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/br...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/br...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:102:
BrowserPluginGuest* ret = new TestBrowserPluginGuest(instance_id,
On 2012/09/13 00:51:44, creis wrote:
> nit: ret -> guest
> (ret is not a useful variable name.)

Done.

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/br...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:325: // a
RenderViewHost swap in the web_contents. We verify if the embedder in the
On 2012/09/13 00:51:44, creis wrote:
> nit: verify if -> verify that

Done.

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/br...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:373: // 2.
Navigate to a URL in https, so we trigger a RenderViewHost swap.
On 2012/09/13 00:51:44, creis wrote:
> nit: Move the test_https_url lines below this line, since they're part of part
> "2".

Done.

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/we...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/we...
content/browser/web_contents/web_contents_impl.cc:447: // This makes
|new_contents| to have a guest role.
On 2012/09/13 00:51:44, creis wrote:
> nit: This makes |new_contents| act as a guest.

Done.

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/we...
content/browser/web_contents/web_contents_impl.cc:2520: // RenderViewHost is
attached to this WebContents (for security concerns).
On 2012/09/13 00:51:44, creis wrote:
> What security concerns?  I suggest dropping that phrase unless you can
elaborate
> on it.
Remove the security concerns part from the comment.
My assumption was: we don't want the same embedder to keep talking to the
changed renderrer process.

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:110: // non-empty value).
On 2012/09/13 00:51:44, creis wrote:
> I don't think we can just tell the guest to navigate to "".  It should either
be
> a no-op or navigate to "about:blank".
I sent you a proposal for handling empty src in email, let's discuss it further
there.

jam

(saw the test cause of the cr-dev thread and noticed this) which part of the ...

8 years, 3 months ago (2012-09-13 16:07:56 UTC) #66

Fady Samuel

https://chromiumcodereview.appspot.com/10868012/diff/34062/content/browser/browser_plugin/browser_plugin_host_browsertest.cc File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right): https://chromiumcodereview.appspot.com/10868012/diff/34062/content/browser/browser_plugin/browser_plugin_host_browsertest.cc#newcode494 content/browser/browser_plugin/browser_plugin_host_browsertest.cc:494: embedder_web_contents->WasHidden(); I forgot to add an ASSERT_TRUE here.

8 years, 3 months ago (2012-09-13 16:35:26 UTC) #67

Fady Samuel

https://chromiumcodereview.appspot.com/10868012/diff/34062/content/browser/browser_plugin/browser_plugin_host_browsertest.cc File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right): https://chromiumcodereview.appspot.com/10868012/diff/34062/content/browser/browser_plugin/browser_plugin_host_browsertest.cc#newcode494 content/browser/browser_plugin/browser_plugin_host_browsertest.cc:494: embedder_web_contents->WasHidden(); On 2012/09/13 16:35:26, Fady Samuel wrote: > I ...

8 years, 3 months ago (2012-09-13 16:47:47 UTC) #68

Charlie Reis

http://codereview.chromium.org/10868012/diff/38076/content/browser/browser_plugin/browser_plugin_embedder.cc File content/browser/browser_plugin/browser_plugin_embedder.cc (right): http://codereview.chromium.org/10868012/diff/38076/content/browser/browser_plugin/browser_plugin_embedder.cc#newcode223 content/browser/browser_plugin/browser_plugin_embedder.cc:223: // won't be cleaned up with the code below; ...

8 years, 3 months ago (2012-09-13 16:55:30 UTC) #69

http://codereview.chromium.org/10868012/diff/38076/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

http://codereview.chromium.org/10868012/diff/38076/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.cc:223: // won't be
cleaned up with the code below; However since WC2 goes away, this
On 2012/09/13 15:56:23, lazyboy wrote:
> On 2012/09/13 00:51:44, creis wrote:
> > How does WC2 go away?  There's no code that I know of that would delete the
> > guest's WebContents in this case, nor in the case that the top-level page
> > deletes the iframe.
> > 
> > If there is code for deleting a guest's WebContents when its embedder
> navigates
> > or destroys it, then I don't see why the code below is needed.
> 
> I talked to Fady about this part of the code: it seems we don't need this bit
of
> code anymore.
> 
> (WebPlugin's pluginDestroyed() seems to be good enough for handling deletions
of
> guests)
> 
> The fact that I saw WC2 go away was because of BrowserPlugin.pluginDestroyed()
> call from renderer (which I wasn't aware of, and Fady says it was added after
> this function was added), which removes the BrowserPluginGuest, that deletes
WC
> associated with the plugin in browser side; Therefore we don't need any
special
> handling for deleting guests here.
> Fady/Charlie: can you think of any other cases we might need this code for?

Great!  I'm not aware of another need for this code if
BrowserPlugin.pluginDestroyed() is cleaning up the guest's WebContents and
BrowserPluginGuest.  Feel free to remove unless Fady thinks otherwise.  Thanks.

http://codereview.chromium.org/10868012/diff/38076/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

http://codereview.chromium.org/10868012/diff/38076/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:193: // TODO(fsamuel):
What do we need to do here? This is for keyboard shortcuts.
On 2012/09/13 14:17:18, Fady Samuel wrote:
> On 2012/09/13 00:51:44, creis wrote:
> > Can this TODO be resolved?  (I can't tell from the comment whether something
> is
> > broken here or not.)
> 
> Really at the time (and now), I'm not really sure I understand what this bool
> does.  RenderWidgetHostImpl::ForwardInputEvent suggests that this is related
to
> keyboard shortcuts.
> 
> I've verified that Ctrl+C on Linux inside a guest works just fine. Again,
> something may be broken, but I don't know what yet :P
> 
> I will investigate further.

Ok, thanks.  I'm ok with the TODO, but please update it if you learn anything.

http://codereview.chromium.org/10868012/diff/38076/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_impl.cc (right):

http://codereview.chromium.org/10868012/diff/38076/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_impl.cc:831: // Allow
BrowserPluginHostMsg_* sync messages to run on the UI thread.
On 2012/09/13 14:17:18, Fady Samuel wrote:
> On 2012/09/13 00:51:44, creis wrote:
> > This doesn't sound safe.  Why is it ok to process sync messages on the UI
> thread
> > here, if we've gone out of our way to prevent it otherwise?
> 
> I've copied-and-pasted BrowserMessageFilter::CheckCanDispatchOnUI. My
> understanding is 1. platform apps will not support windowed plugins (asked in
an
> email thread for confirmation). 2. Browser Plugin messages will not
participate
> in this deadlock cycle in the comment below.
> 
> John or someone else might be able to validate my two assumptions/claims
above.

Ok.  Let's put that justification in the comment above so that it's clear why we
think this is safe.

Fady Samuel

http://codereview.chromium.org/10868012/diff/38076/content/browser/browser_plugin/browser_plugin_embedder.cc File content/browser/browser_plugin/browser_plugin_embedder.cc (right): http://codereview.chromium.org/10868012/diff/38076/content/browser/browser_plugin/browser_plugin_embedder.cc#newcode223 content/browser/browser_plugin/browser_plugin_embedder.cc:223: // won't be cleaned up with the code below; ...

8 years, 3 months ago (2012-09-13 16:59:56 UTC) #70

lazyboy

Added the empty src handling. Removed cleaning up in embedder's DidCommitProvisionalLoad. Added comment for running ...

8 years, 3 months ago (2012-09-13 18:52:49 UTC) #71

Added the empty src handling.
Removed cleaning up in embedder's DidCommitProvisionalLoad.
Added comment for running browser_plugin_host messages in UI.

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:223: // won't be
cleaned up with the code below; However since WC2 goes away, this
On 2012/09/13 16:59:57, Fady Samuel wrote:
> On 2012/09/13 16:55:30, creis wrote:
> > On 2012/09/13 15:56:23, lazyboy wrote:
> > > On 2012/09/13 00:51:44, creis wrote:
> > > > How does WC2 go away?  There's no code that I know of that would delete
> the
> > > > guest's WebContents in this case, nor in the case that the top-level
page
> > > > deletes the iframe.
> > > > 
> > > > If there is code for deleting a guest's WebContents when its embedder
> > > navigates
> > > > or destroys it, then I don't see why the code below is needed.
> > > 
> > > I talked to Fady about this part of the code: it seems we don't need this
> bit
> > of
> > > code anymore.
> > > 
> > > (WebPlugin's pluginDestroyed() seems to be good enough for handling
> deletions
> > of
> > > guests)
> > > 
> > > The fact that I saw WC2 go away was because of
> BrowserPlugin.pluginDestroyed()
> > > call from renderer (which I wasn't aware of, and Fady says it was added
> after
> > > this function was added), which removes the BrowserPluginGuest, that
deletes
> > WC
> > > associated with the plugin in browser side; Therefore we don't need any
> > special
> > > handling for deleting guests here.
> > > Fady/Charlie: can you think of any other cases we might need this code
for?
> > 
> > Great!  I'm not aware of another need for this code if
> > BrowserPlugin.pluginDestroyed() is cleaning up the guest's WebContents and
> > BrowserPluginGuest.  Feel free to remove unless Fady thinks otherwise. 
> Thanks.
> 
> I don't believe this code is necessary any more. Please remove it.
Removed.

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/re...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/38076/content/browser/re...
content/browser/renderer_host/render_view_host_impl.cc:831: // Allow
BrowserPluginHostMsg_* sync messages to run on the UI thread.
On 2012/09/13 16:55:30, creis wrote:
> On 2012/09/13 14:17:18, Fady Samuel wrote:
> > On 2012/09/13 00:51:44, creis wrote:
> > > This doesn't sound safe.  Why is it ok to process sync messages on the UI
> > thread
> > > here, if we've gone out of our way to prevent it otherwise?
> > 
> > I've copied-and-pasted BrowserMessageFilter::CheckCanDispatchOnUI. My
> > understanding is 1. platform apps will not support windowed plugins (asked
in
> an
> > email thread for confirmation). 2. Browser Plugin messages will not
> participate
> > in this deadlock cycle in the comment below.
> > 
> > John or someone else might be able to validate my two assumptions/claims
> above.
> 
> Ok.  Let's put that justification in the comment above so that it's clear why
we
> think this is safe.

Done.

Charlie Reis

LGTM, once you fix any remaining test flakiness (and address the comments below). Please wait ...

8 years, 3 months ago (2012-09-13 22:46:25 UTC) #72

lazyboy

I will be waiting for Darin's approval on the email thread before proceeding. Thanks for ...

8 years, 3 months ago (2012-09-14 19:17:29 UTC) #73

lazyboy

OK Darin said this should be good to land. I still need lgtm for one ...

8 years, 3 months ago (2012-09-15 01:31:23 UTC) #74

awong

John's out on vacation... On Fri, Sep 14, 2012 at 6:31 PM, <lazyboy@chromium.org> wrote: > ...

8 years, 3 months ago (2012-09-15 01:32:45 UTC) #75

John's out on vacation...

On Fri, Sep 14, 2012 at 6:31 PM, <lazyboy@chromium.org> wrote:

> OK Darin said this should be good to land.
> I still need lgtm for one change in chrome/renderer/
> John, could you take a look at the change there, thanks!
>
>
> On 2012/09/14 19:17:29, lazyboy wrote:
>
>> I will be waiting for Darin's approval on the email thread before
>> proceeding.
>> Thanks for the review!
>>
>
>
> http://codereview.chromium.**org/10868012/diff/34062/**
>
content/browser/browser_**plugin/browser_plugin_host_**browsertest.cc<http://codereview.chromium.org/10868012/diff/34062/content/browser/browser_plugin/browser_plugin_host_browsertest.cc>
>
>> File content/browser/browser_**plugin/browser_plugin_host_**
>> browsertest.cc
>>
> (right):
>
>
> http://codereview.chromium.**org/10868012/diff/34062/**
> content/browser/browser_**plugin/browser_plugin_host_**
>
browsertest.cc#newcode208<http://codereview.chromium.org/10868012/diff/34062/content/browser/browser_plugin/browser_plugin_host_browsertest.cc#newcode208>
>
>> content/browser/browser_**plugin/browser_plugin_host_**browsertest.cc:208:
>> // Send
>>
> a
>
>> mouse event to the guest.
>> On 2012/09/13 16:07:56, John Abd-El-Malek wrote:
>> > can you make the plugin take 100% and use content::SimulateMouseClick
>>
> instead
>
>> of
>> > this?
>>
>
>  Done.
>>
>
>
> http://codereview.chromium.**org/10868012/diff/48026/**
>
content/browser/renderer_host/**render_view_host_impl.cc<http://codereview.chromium.org/10868012/diff/48026/content/browser/renderer_host/render_view_host_impl.cc>
>
>> File content/browser/renderer_host/**render_view_host_impl.cc (right):
>>
>
>
> http://codereview.chromium.**org/10868012/diff/48026/**
>
content/browser/renderer_host/**render_view_host_impl.cc#**newcode836<http://codereview.chromium.org/10868012/diff/48026/content/browser/renderer_host/render_view_host_impl.cc#newcode836>
>
>> content/browser/renderer_host/**render_view_host_impl.cc:836: // need to
>> be
>> changed to async messages.
>> On 2012/09/13 22:46:25, creis wrote:
>> > Please add a TODO referencing http://crbug.com/149063 to remove this
>> and
>>
> make
>
>> > these messages async, since this should only be temporary.
>>
>
>  Done.
>>
>
>
> http://codereview.chromium.**org/10868012/diff/48026/**
>
content/browser/web_contents/**web_contents_impl.h<http://codereview.chromium.org/10868012/diff/48026/content/browser/web_contents/web_contents_impl.h>
>
>> File content/browser/web_contents/**web_contents_impl.h (right):
>>
>
>
> http://codereview.chromium.**org/10868012/diff/48026/**
>
content/browser/web_contents/**web_contents_impl.h#newcode680<http://codereview.chromium.org/10868012/diff/48026/content/browser/web_contents/web_contents_impl.h#newcode680>
>
>> content/browser/web_contents/**web_contents_impl.h:680: void
>> RemoveSwappedOutBrowserPluginE**mbedder();
>> On 2012/09/13 22:46:25, creis wrote:
>> > Let's remove "SwappedOut" from the name, since we might think of other
>> > circumstances we want to call this.  (e.g., It would be safe to call
>> this if
>> the
>> > only guest were removed from the DOM.)
>>
>
>  Done.
>>
>
>
>
>
https://chromiumcodereview.**appspot.com/10868012/<https://chromiumcodereview...
>

jam

mostly nits, except for test_timeout_tracker.h which I think will cause a lot of pain (I'm ...

8 years, 3 months ago (2012-09-17 17:47:52 UTC) #76

jam

(btw I only looked for nits, Albert/Charlie did the real review so I leave it ...

8 years, 3 months ago (2012-09-17 17:48:19 UTC) #77

lazyboy

Albert: John is concerned that TestTimeoutTracker might cause (future) flakiness, can we revert this class ...

8 years, 3 months ago (2012-09-17 20:22:24 UTC) #78

Albert: John is concerned that TestTimeoutTracker might cause (future)
flakiness, can we revert this class and use MessageLoopRunner->Run() to wait
indefinitely instead?
Thanks.

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:11: #include
"content/browser/browser_plugin/browser_plugin_host_factory.h"
On 2012/09/17 17:47:52, John Abd-El-Malek wrote:
> this is included in the header. please go through all your includes in this
> changelist and make sure the headers include only what they need, and there's
no
> duplication between the header and cc file
Hmmm, I was somehow counting on cpplint.py for these cases.
Done.

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:18: #include
"content/public/browser/render_view_host.h"
On 2012/09/17 17:47:52, John Abd-El-Malek wrote:
> don't include public headers when you're including the implementation one

Done.

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:32: #include <string>
On 2012/09/17 17:47:52, John Abd-El-Malek wrote:
> nit: order. also, including string is really unnecessary...

Done.

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:39: #include
"content/public/browser/render_view_host_observer.h"
On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
> nit: why?
Removed (this class used to be RVHObserver).

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:42: #include
"ipc/ipc_channel_handle.h"
On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
> nit: why?

Removed.

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:43: #include
"ipc/ipc_sync_message.h"
On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
> why?

Removed.

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:44: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebInputEvent.h"
On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
> forward declare instead of including?

Done.

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:47: #include
"ui/gfx/size.h"
On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
> not needed since you include rect

Done.
Doesn't this contradict style rule: "Do not depend on the symbol being brought
in transitively via headers not directly included."

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:51: class Size;
On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
> not needed since you include the header.

Done.

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:83: void
SetGuestHangTimeoutForTesting(const base::TimeDelta& timeout) {
On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
> nit: since this is inline: set_guest_hang_timeout_for_testing()

Done.

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:108: void
set_embedder_render_process_host(
On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
> to make this header easier to read, can you:
> 
> -put all the setters/getters together for one block (with the _for_testing
ones
> at the end)
> -put all the virtual methods that are overridden for tests in one block

Done.

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:154: virtual void
SendMessageToEmbedder(IPC::Message*);
On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
> nit: chrome style is to always list the parameter name

Done.

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest_helper.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest_helper.h:8: #include
<string>
On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
> nit: skip including <string> since this is very basic that's included by many
of
> your headers already (i.e. this is like not including basictypes.h)
Removed them all wherever I could, but cannot move this one now since the other
includes are not pulling it in amy more.
Done.

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest_helper.h:14: #include
"content/public/browser/web_contents_observer.h"
On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
> not needed

Done.

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest_helper.h:20: #include
"webkit/glue/webcursor.h"
On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
> do you really need to include this instead of forward declaring?
> 
> also you don't need all these includes

Done.

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/br...
File content/browser/browser_plugin/test_timeout_tracker.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/br...
content/browser/browser_plugin/test_timeout_tracker.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
> are you sure you really want this class? this already caused flakiness
> elsewhere, and I worry that waiting for the action timeout for some things
> (instead of waiting indefinitely) will similarly cause you flaky tests.
> 
> there's a good reason we generally don't wait for a period of time, but
instead
> wait for a notification.

Albert, can you comment on this one.
I guess we don't need this anymore since we're already overriding the 5s timeout
for guest crashed to small value in tests.
Ty.

awong

On Mon, Sep 17, 2012 at 1:22 PM, <lazyboy@chromium.org> wrote: > Albert: John is concerned ...

8 years, 3 months ago (2012-09-17 20:39:25 UTC) #79

On Mon, Sep 17, 2012 at 1:22 PM, <lazyboy@chromium.org> wrote:

> Albert: John is concerned that TestTimeoutTracker might cause (future)
> flakiness, can we revert this class and use MessageLoopRunner->Run() to
> wait
> indefinitely instead?
> Thanks.


I think this test is prone to be flaky however we slice it because we are
using wallclock values for testing a hang timer.  Go ahead and remove it.
If the infinite hang becomes an issue, we can revisit.



>  https://chromiumcodereview.**appspot.com/10868012/diff/**
>
50042/content/browser/browser_**plugin/browser_plugin_guest.cc<https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest.cc>
> File content/browser/browser_**plugin/browser_plugin_guest.cc (right):
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
>
50042/content/browser/browser_**plugin/browser_plugin_guest.**cc#newcode11<https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest.cc#newcode11>
> content/browser/browser_**plugin/browser_plugin_guest.**cc:11: #include
> "content/browser/browser_**plugin/browser_plugin_host_**factory.h"
> On 2012/09/17 17:47:52, John Abd-El-Malek wrote:
>
>> this is included in the header. please go through all your includes in
>>
> this
>
>> changelist and make sure the headers include only what they need, and
>>
> there's no
>
>> duplication between the header and cc file
>>
> Hmmm, I was somehow counting on cpplint.py for these cases.
> Done.
>
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
>
50042/content/browser/browser_**plugin/browser_plugin_guest.**cc#newcode18<https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest.cc#newcode18>
> content/browser/browser_**plugin/browser_plugin_guest.**cc:18: #include
> "content/public/browser/**render_view_host.h"
> On 2012/09/17 17:47:52, John Abd-El-Malek wrote:
>
>> don't include public headers when you're including the implementation
>>
> one
>
> Done.
>
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
>
50042/content/browser/browser_**plugin/browser_plugin_guest.h<https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest.h>
> File content/browser/browser_**plugin/browser_plugin_guest.h (right):
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
>
50042/content/browser/browser_**plugin/browser_plugin_guest.h#**newcode32<https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest.h#newcode32>
> content/browser/browser_**plugin/browser_plugin_guest.h:**32: #include
> <string>
> On 2012/09/17 17:47:52, John Abd-El-Malek wrote:
>
>> nit: order. also, including string is really unnecessary...
>>
>
> Done.
>
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
>
50042/content/browser/browser_**plugin/browser_plugin_guest.h#**newcode39<https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest.h#newcode39>
> content/browser/browser_**plugin/browser_plugin_guest.h:**39: #include
> "content/public/browser/**render_view_host_observer.h"
> On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
>
>> nit: why?
>>
> Removed (this class used to be RVHObserver).
>
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
>
50042/content/browser/browser_**plugin/browser_plugin_guest.h#**newcode42<https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest.h#newcode42>
> content/browser/browser_**plugin/browser_plugin_guest.h:**42: #include
> "ipc/ipc_channel_handle.h"
> On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
>
>> nit: why?
>>
>
> Removed.
>
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
>
50042/content/browser/browser_**plugin/browser_plugin_guest.h#**newcode43<https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest.h#newcode43>
> content/browser/browser_**plugin/browser_plugin_guest.h:**43: #include
> "ipc/ipc_sync_message.h"
> On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
>
>> why?
>>
>
> Removed.
>
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
>
50042/content/browser/browser_**plugin/browser_plugin_guest.h#**newcode44<https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest.h#newcode44>
> content/browser/browser_**plugin/browser_plugin_guest.h:**44: #include
> "third_party/WebKit/Source/**WebKit/chromium/public/**WebInputEvent.h"
> On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
>
>> forward declare instead of including?
>>
>
> Done.
>
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
>
50042/content/browser/browser_**plugin/browser_plugin_guest.h#**newcode47<https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest.h#newcode47>
> content/browser/browser_**plugin/browser_plugin_guest.h:**47: #include
> "ui/gfx/size.h"
> On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
>
>> not needed since you include rect
>>
>
> Done.
> Doesn't this contradict style rule: "Do not depend on the symbol being
> brought in transitively via headers not directly included."
>
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
>
50042/content/browser/browser_**plugin/browser_plugin_guest.h#**newcode51<https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest.h#newcode51>
> content/browser/browser_**plugin/browser_plugin_guest.h:**51: class Size;
> On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
>
>> not needed since you include the header.
>>
>
> Done.
>
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
>
50042/content/browser/browser_**plugin/browser_plugin_guest.h#**newcode83<https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest.h#newcode83>
> content/browser/browser_**plugin/browser_plugin_guest.h:**83: void
> SetGuestHangTimeoutForTesting(**const base::TimeDelta& timeout) {
> On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
>
>> nit: since this is inline: set_guest_hang_timeout_for_**testing()
>>
>
> Done.
>
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
>
50042/content/browser/browser_**plugin/browser_plugin_guest.h#**newcode108<https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest.h#newcode108>
> content/browser/browser_**plugin/browser_plugin_guest.h:**108: void
> set_embedder_render_process_**host(
> On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
>
>> to make this header easier to read, can you:
>>
>
>  -put all the setters/getters together for one block (with the
>>
> _for_testing ones
>
>> at the end)
>> -put all the virtual methods that are overridden for tests in one
>>
> block
>
> Done.
>
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
>
50042/content/browser/browser_**plugin/browser_plugin_guest.h#**newcode154<https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest.h#newcode154>
> content/browser/browser_**plugin/browser_plugin_guest.h:**154: virtual
> void
> SendMessageToEmbedder(IPC::**Message*);
> On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
>
>> nit: chrome style is to always list the parameter name
>>
>
> Done.
>
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
>
50042/content/browser/browser_**plugin/browser_plugin_guest_**helper.h<https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest_helper.h>
> File content/browser/browser_**plugin/browser_plugin_guest_**helper.h
> (right):
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
> 50042/content/browser/browser_**plugin/browser_plugin_guest_**
>
helper.h#newcode8<https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest_helper.h#newcode8>
> content/browser/browser_**plugin/browser_plugin_guest_**helper.h:8:
> #include
> <string>
> On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
>
>> nit: skip including <string> since this is very basic that's included
>>
> by many of
>
>> your headers already (i.e. this is like not including basictypes.h)
>>
> Removed them all wherever I could, but cannot move this one now since
> the other includes are not pulling it in amy more.
> Done.
>
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
> 50042/content/browser/browser_**plugin/browser_plugin_guest_**
>
helper.h#newcode14<https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest_helper.h#newcode14>
> content/browser/browser_**plugin/browser_plugin_guest_**helper.h:14:
> #include "content/public/browser/web_**contents_observer.h"
> On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
>
>> not needed
>>
>
> Done.
>
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
> 50042/content/browser/browser_**plugin/browser_plugin_guest_**
>
helper.h#newcode20<https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest_helper.h#newcode20>
> content/browser/browser_**plugin/browser_plugin_guest_**helper.h:20:
> #include "webkit/glue/webcursor.h"
> On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
>
>> do you really need to include this instead of forward declaring?
>>
>
>  also you don't need all these includes
>>
>
> Done.
>
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
>
50042/content/browser/browser_**plugin/test_timeout_tracker.h<https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/test_timeout_tracker.h>
> File content/browser/browser_**plugin/test_timeout_tracker.h (right):
>
> https://chromiumcodereview.**appspot.com/10868012/diff/**
>
50042/content/browser/browser_**plugin/test_timeout_tracker.h#**newcode1<https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/test_timeout_tracker.h#newcode1>
> content/browser/browser_**plugin/test_timeout_tracker.h:**1: // Copyright
> (c) 2012 The Chromium Authors. All rights reserved.
> On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
>
>> are you sure you really want this class? this already caused flakiness
>> elsewhere, and I worry that waiting for the action timeout for some
>>
> things
>
>> (instead of waiting indefinitely) will similarly cause you flaky
>>
> tests.
>
>  there's a good reason we generally don't wait for a period of time,
>>
> but instead
>
>> wait for a notification.
>>
>
> Albert, can you comment on this one.
> I guess we don't need this anymore since we're already overriding the 5s
> timeout for guest crashed to small value in tests.
> Ty.
>
>
https://chromiumcodereview.**appspot.com/10868012/<https://chromiumcodereview...
>

jam

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest.cc File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest.cc#newcode11 content/browser/browser_plugin/browser_plugin_guest.cc:11: #include "content/browser/browser_plugin/browser_plugin_host_factory.h" On 2012/09/17 20:22:24, lazyboy wrote: > On ...

8 years, 3 months ago (2012-09-17 20:47:57 UTC) #80

lazyboy

Removed TestTimeoutTracker class, PTAL. https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest.cc File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest.cc#newcode11 content/browser/browser_plugin/browser_plugin_guest.cc:11: #include "content/browser/browser_plugin/browser_plugin_host_factory.h" On 2012/09/17 20:47:57, ...

8 years, 3 months ago (2012-09-17 21:07:54 UTC) #81

jam

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest.h File content/browser/browser_plugin/browser_plugin_guest.h (right): https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest.h#newcode108 content/browser/browser_plugin/browser_plugin_guest.h:108: void set_embedder_render_process_host( On 2012/09/17 20:22:24, lazyboy wrote: > On ...

8 years, 3 months ago (2012-09-17 22:13:36 UTC) #82

lazyboy

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest.h File content/browser/browser_plugin/browser_plugin_guest.h (right): https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest.h#newcode108 content/browser/browser_plugin/browser_plugin_guest.h:108: void set_embedder_render_process_host( On 2012/09/17 22:13:36, John Abd-El-Malek wrote: > ...

8 years, 3 months ago (2012-09-17 22:37:48 UTC) #83

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:108: void
set_embedder_render_process_host(
On 2012/09/17 22:13:36, John Abd-El-Malek wrote:
> On 2012/09/17 20:22:24, lazyboy wrote:
> > On 2012/09/17 17:47:53, John Abd-El-Malek wrote:
> > > to make this header easier to read, can you:
> > > 
> > > -put all the setters/getters together for one block (with the _for_testing
> > ones
> > > at the end)
> > > -put all the virtual methods that are overridden for tests in one block
> > 
> > Done.
> 
> doesn't look like it?

Sorry, not sure if I follow the first one: this function and the next 6 are
getters/setters.
Did you mean to put SetDamageBuffer out of the group since this is not inline
setter?

I misread the second one. Fixed.

https://chromiumcodereview.appspot.com/10868012/diff/48065/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/10868012/diff/48065/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:23: #include
"ipc/ipc_sync_message.h"
On 2012/09/17 22:13:36, John Abd-El-Malek wrote:
> are you sure you need this? it's very rare that someone needs to include this.
> if you're dealing with messages, then you would include the foo_messages.h
file

Don't seem to need this.
Done.

https://chromiumcodereview.appspot.com/10868012/diff/48065/content/browser/br...
File content/browser/browser_plugin/test_browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/10868012/diff/48065/content/browser/br...
content/browser/browser_plugin/test_browser_plugin_guest.h:17: class Message;
On 2012/09/17 22:13:36, John Abd-El-Malek wrote:
> not needed by definition since it's used in an overridden function

Done.

jam

lgtm https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest.h File content/browser/browser_plugin/browser_plugin_guest.h (right): https://chromiumcodereview.appspot.com/10868012/diff/50042/content/browser/browser_plugin/browser_plugin_guest.h#newcode108 content/browser/browser_plugin/browser_plugin_guest.h:108: void set_embedder_render_process_host( On 2012/09/17 22:37:48, lazyboy wrote: > ...

8 years, 3 months ago (2012-09-17 23:08:55 UTC) #84

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lazyboy@chromium.org/10868012/61001

8 years, 3 months ago (2012-09-18 19:13:23 UTC) #85

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lazyboy@chromium.org/10868012/61001

8 years, 3 months ago (2012-09-19 20:40:00 UTC) #86

Charlie Reis

http://codereview.chromium.org/10868012/diff/61001/content/browser/browser_plugin/browser_plugin_embedder.h File content/browser/browser_plugin/browser_plugin_embedder.h (right): http://codereview.chromium.org/10868012/diff/61001/content/browser/browser_plugin/browser_plugin_embedder.h#newcode87 content/browser/browser_plugin/browser_plugin_embedder.h:87: friend class BrowserPluginEmbedderHelper; Oh, I missed this. We generally ...

8 years, 3 months ago (2012-09-19 21:51:02 UTC) #87

Fady Samuel

On 2012/09/19 21:51:02, creis wrote: > http://codereview.chromium.org/10868012/diff/61001/content/browser/browser_plugin/browser_plugin_embedder.h > File content/browser/browser_plugin/browser_plugin_embedder.h (right): > > http://codereview.chromium.org/10868012/diff/61001/content/browser/browser_plugin/browser_plugin_embedder.h#newcode87 > ...

8 years, 3 months ago (2012-09-19 21:53:52 UTC) #88

Charlie Reis

On 2012/09/19 21:53:52, Fady Samuel wrote: http://codereview.chromium.org/10868012/diff/61001/content/browser/browser_plugin/browser_plugin_embedder.h#newcode87 > > content/browser/browser_plugin/browser_plugin_embedder.h:87: friend class > > BrowserPluginEmbedderHelper; ...

8 years, 3 months ago (2012-09-19 21:56:52 UTC) #89

lazyboy

http://codereview.chromium.org/10868012/diff/61001/content/browser/browser_plugin/browser_plugin_embedder.h File content/browser/browser_plugin/browser_plugin_embedder.h (right): http://codereview.chromium.org/10868012/diff/61001/content/browser/browser_plugin/browser_plugin_embedder.h#newcode87 content/browser/browser_plugin/browser_plugin_embedder.h:87: friend class BrowserPluginEmbedderHelper; On 2012/09/19 21:51:02, creis wrote: > ...

8 years, 3 months ago (2012-09-19 22:11:14 UTC) #90

Charlie Reis

On 2012/09/19 22:11:14, lazyboy wrote: > http://codereview.chromium.org/10868012/diff/61001/content/browser/browser_plugin/browser_plugin_embedder.h > File content/browser/browser_plugin/browser_plugin_embedder.h (right): > > http://codereview.chromium.org/10868012/diff/61001/content/browser/browser_plugin/browser_plugin_embedder.h#newcode87 > ...

8 years, 3 months ago (2012-09-19 22:13:10 UTC) #91

lazyboy

Hey Charlie, the CL got reverted, I've fixed the issue (we used static initializer from ...

8 years, 3 months ago (2012-09-20 00:55:16 UTC) #93

lazyboy

Reverting the removal of friend class in this CL for submitting now, sending you the ...

8 years, 3 months ago (2012-09-20 13:38:45 UTC) #94

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lazyboy@chromium.org/10868012/80001

8 years, 3 months ago (2012-09-20 13:39:38 UTC) #95

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lazyboy@chromium.org/10868012/79003

8 years, 3 months ago (2012-09-20 19:14:36 UTC) #98

Change committed as 157808

Issue 10868012: Browser Plugin: New Implementation (Browser Side) (Closed)

Description

Patch Set 1 #

Patch Set 2 : Fixed tests. Also fixed a case where UpdateRect was being sent from bp renderer before NavigateGues… #

Patch Set 3 : Address CL comments. #

Patch Set 4 : Address two nits from original CL's comments by creis #

Patch Set 5 : Rename class as we agreed upon, keep correct type of instances (guest/embedder, not helpers) inside… #

Patch Set 6 : Address CL comments. #

Patch Set 7 : Rewrite test to not use NOTIFICATION_* #

Patch Set 8 : Address nits (only). #

Patch Set 9 : Address 3 issues: Cleaning up embedder on RVH swap, empty src handling fix, subframe's guest cleanu… #

Patch Set 10 : Sync to ToTT #

Patch Set 11 : Fix incorrect sync+rebase. #

Patch Set 12 : Fix mac build + Address nits from awong@ #

Patch Set 13 : sync + Address Albert's comments. #

Patch Set 14 : 1. Address comment 2. Fix browsertest target 3. Ignore input evt when guest is dead. #

Patch Set 15 : Address Albert's comments. #

Patch Set 16 : Clean up BrowserPluginHostTest, remove unused var, use constants for str. #

Patch Set 17 : Address comments from Albert. #

Patch Set 18 : Sync + fix test target. #

Patch Set 19 : Rework tests to use timeouts instead of waiting indefinitely, not store any Guest* in embedder. #

Patch Set 20 : Merge Fady's patch for focus test + Address comments. #

Patch Set 21 : Remove WebContents::CreateGuest from content api to WebContentsImpl #

Patch Set 22 : Add RVH swap tests + @tott #

Patch Set 23 : integrate windows fix by Fady (original cl #10910228) #

Patch Set 24 : Fix windows bot test failures + WasResized test, both from fsamuel@ #

Patch Set 25 : Address nits. #

Patch Set 26 : Empty src fix + remove unnecessary clean up for guests + comment for allowing sync bphost messages … #

Patch Set 27 : IGNORE_THIS_PATCH: patch for checking trybot flakiness. #

Patch Set 28 : @tott + Address comments + fix win_rel trybot flakiness #

Patch Set 29 : Address John's comments, except timers in test, deferring to Albert. #

Patch Set 30 : Remove TestTimeoutTracker and wait indefinitely in tests instead, fix one include. #

Patch Set 31 : Address review comments. #

Patch Set 32 : @tott #

Patch Set 33 : Fix build breakage (const int vs const TimeDelta) + remove friend classes #

Patch Set 34 : Revert the friend class change patch for submitting. #

Patch Set 35 : Disable BrowserPluginHostTest.NavigateGuest on win, flaking. #

Messages