browser-plugin: Allow accepting drag-n-drop events.

browser-plugin: Allow accepting drag-n-drop events.

This allows dragging content from within the embedder (or other windows) into
the browser-tag plugin.

BUG=120264
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=161457

[sadrul, 2012-10-09 23:09:42]

creis@ Please review.

fsamuel@, lazyboy@ FYI (especially note the
BrowserPluginGuest::set_embedder_render_view_host change and the note below)

http://codereview.chromium.org/11088043/diff/1/content/browser/browser_plugin...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

http://codereview.chromium.org/11088043/diff/1/content/browser/browser_plugin...
content/browser/browser_plugin/browser_plugin_embedder.cc:98:
guest->set_embedder_render_view_host(render_view_host);
I believe this is safe because when a RenderViewHostImpl dies, it calls
delegate::RenderViewDeleted from its destructor, which is 
WebContentsImpl::RenderViewDeleted, which triggers
BrowserPluginEmbedder::DestroyGuests, which kills |guest|.

The same is the case for when the RenderViewHost receives a  RenderViewGone
message.

So from what I can see, at no point in |guest|s lifetime can there be a state
when the embedder RenderViewHost it points to is invalid.

[Fady Samuel, 2012-10-09 23:18:53]

http://codereview.chromium.org/11088043/diff/1/content/browser/browser_plugin...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

http://codereview.chromium.org/11088043/diff/1/content/browser/browser_plugin...
content/browser/browser_plugin/browser_plugin_embedder.cc:98:
guest->set_embedder_render_view_host(render_view_host);
On 2012/10/09 23:09:42, sadrul wrote:
> I believe this is safe because when a RenderViewHostImpl dies, it calls
> delegate::RenderViewDeleted from its destructor, which is 
> WebContentsImpl::RenderViewDeleted, which triggers
> BrowserPluginEmbedder::DestroyGuests, which kills |guest|.
> 
> The same is the case for when the RenderViewHost receives a  RenderViewGone
> message.
> 
> So from what I can see, at no point in |guest|s lifetime can there be a state
> when the embedder RenderViewHost it points to is invalid.

Note: I was planning on deleting that line because it actually introduces a bug
with my postMessage patch where reload stops working because we end up
destroying all guests after we reload if a swapped out guest lives in the
embedder process destroys itself.

In a nutshell, this breaks reload. I'm going to retrace the stack for this
particular bug tomorrow and send you an email with more information because my
memory is a bit hazy on the details.

[sadrul, 2012-10-10 18:43:22]

creis@ ping :)

http://codereview.chromium.org/11088043/diff/1/content/browser/browser_plugin...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

http://codereview.chromium.org/11088043/diff/1/content/browser/browser_plugin...
content/browser/browser_plugin/browser_plugin_embedder.cc:98:
guest->set_embedder_render_view_host(render_view_host);
On 2012/10/09 23:18:53, Fady Samuel wrote:
> On 2012/10/09 23:09:42, sadrul wrote:
> > I believe this is safe because when a RenderViewHostImpl dies, it calls
> > delegate::RenderViewDeleted from its destructor, which is 
> > WebContentsImpl::RenderViewDeleted, which triggers
> > BrowserPluginEmbedder::DestroyGuests, which kills |guest|.
> > 
> > The same is the case for when the RenderViewHost receives a  RenderViewGone
> > message.
> > 
> > So from what I can see, at no point in |guest|s lifetime can there be a
state
> > when the embedder RenderViewHost it points to is invalid.
> 
> Note: I was planning on deleting that line because it actually introduces a
bug
> with my postMessage patch where reload stops working because we end up
> destroying all guests after we reload if a swapped out guest lives in the
> embedder process destroys itself.
> 
> In a nutshell, this breaks reload. I'm going to retrace the stack for this
> particular bug tomorrow and send you an email with more information because my
> memory is a bit hazy on the details.

You didn't mean this CL breaks reload, right? Because reloading the embedder or
the guest seems to work OK.

[Fady Samuel, 2012-10-10 18:45:35]

On 2012/10/10 18:43:22, sadrul wrote:
> creis@ ping :)
> 
>
http://codereview.chromium.org/11088043/diff/1/content/browser/browser_plugin...
> File content/browser/browser_plugin/browser_plugin_embedder.cc (right):
> 
>
http://codereview.chromium.org/11088043/diff/1/content/browser/browser_plugin...
> content/browser/browser_plugin/browser_plugin_embedder.cc:98:
> guest->set_embedder_render_view_host(render_view_host);
> On 2012/10/09 23:18:53, Fady Samuel wrote:
> > On 2012/10/09 23:09:42, sadrul wrote:
> > > I believe this is safe because when a RenderViewHostImpl dies, it calls
> > > delegate::RenderViewDeleted from its destructor, which is 
> > > WebContentsImpl::RenderViewDeleted, which triggers
> > > BrowserPluginEmbedder::DestroyGuests, which kills |guest|.
> > > 
> > > The same is the case for when the RenderViewHost receives a 
RenderViewGone
> > > message.
> > > 
> > > So from what I can see, at no point in |guest|s lifetime can there be a
> state
> > > when the embedder RenderViewHost it points to is invalid.
> > 
> > Note: I was planning on deleting that line because it actually introduces a
> bug
> > with my postMessage patch where reload stops working because we end up
> > destroying all guests after we reload if a swapped out guest lives in the
> > embedder process destroys itself.
> > 
> > In a nutshell, this breaks reload. I'm going to retrace the stack for this
> > particular bug tomorrow and send you an email with more information because
my
> > memory is a bit hazy on the details.
> 
> You didn't mean this CL breaks reload, right? Because reloading the embedder
or
> the guest seems to work OK.

Ignore what I said. At the moment, your patch is fine. Things might be messier
once postMessage lands. I'll deal with that in my CL. Thanks.

[Charlie Reis, 2012-10-10 19:04:32]

In general, this looks ok.  I don't know drag n drop code at all, though, so you
may want some more experienced eyes on that aspect of it.  One question below.

http://codereview.chromium.org/11088043/diff/1/content/browser/browser_plugin...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

http://codereview.chromium.org/11088043/diff/1/content/browser/browser_plugin...
content/browser/browser_plugin/browser_plugin_embedder.cc:98:
guest->set_embedder_render_view_host(render_view_host);
On 2012/10/09 23:09:42, sadrul wrote:
> I believe this is safe because when a RenderViewHostImpl dies, it calls
> delegate::RenderViewDeleted from its destructor, which is 
> WebContentsImpl::RenderViewDeleted, which triggers
> BrowserPluginEmbedder::DestroyGuests, which kills |guest|.
> 
> The same is the case for when the RenderViewHost receives a  RenderViewGone
> message.
> 
> So from what I can see, at no point in |guest|s lifetime can there be a state
> when the embedder RenderViewHost it points to is invalid.

I don't see a big problem with telling the guest what the embedder's
RenderViewHost is, since any change to the embedder's RenderViewHost will
definitely get rid of its guests.

It is a bit odd to me that the BrowserPluginGuest doesn't know it's embedder,
though.  I guess that hasn't been needed, but that would let you find the
embedder's current RenderViewHost and RenderProcessHost without having to store
them separately.  Fady, is there a reason not to do that?

[sadrul, 2012-10-10 19:16:32]

+jam@ Would you mind reviewing this change?

[jam, 2012-10-10 20:14:12]

On 2012/10/10 19:16:32, sadrul wrote:
> +jam@ Would you mind reviewing this change?

I'm not familiar with this code either, so I'll defer to creis.

[jam, 2012-10-10 20:14:46]

https://codereview.chromium.org/11088043/diff/1/content/common/browser_plugin...
File content/common/browser_plugin_messages.h (right):

https://codereview.chromium.org/11088043/diff/1/content/common/browser_plugin...
content/common/browser_plugin_messages.h:133: WebKit::WebDragStatus,
nit: see the convention in this file of putting the parameter name inside
comments for every parameter

[sadrul, 2012-10-10 20:23:09]

Thanks!

http://codereview.chromium.org/11088043/diff/1/content/common/browser_plugin_...
File content/common/browser_plugin_messages.h (right):

http://codereview.chromium.org/11088043/diff/1/content/common/browser_plugin_...
content/common/browser_plugin_messages.h:133: WebKit::WebDragStatus,
On 2012/10/10 20:14:46, John Abd-El-Malek wrote:
> nit: see the convention in this file of putting the parameter name inside
> comments for every parameter

Fixed.

[sadrul, 2012-10-10 21:12:58]

+sky@ Perhaps you could review this change (or suggest who could)?

[sky, 2012-10-10 21:34:53]

John is a better reviewer for this.

[sadrul, 2012-10-11 17:30:16]

Removing jam@ from reviewers list :)

I have added a test, and the trybots are excited about it. Perhaps that placates
your concerns a bit? :)

[Fady Samuel, 2012-10-11 17:57:09]

http://codereview.chromium.org/11088043/diff/12002/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

http://codereview.chromium.org/11088043/diff/12002/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:724:
IN_PROC_BROWSER_TEST_F(BrowserPluginHostTest, AcceptDragEvents) {
What's happening here is not obvious to me. Please comment on how this test
works and what's happening at each major step of the test. What are you
verifying here?

[Charlie Reis, 2012-10-11 18:02:52]

I appreciate the test, and I've looked over it again.  I'm happy to give it an
owner's approval (after the comment below), but my concern still stands.  I
understand the browser plugin part of this but I wouldn't know if something is
missing from the drag n drop part.

dcheng@, it looks like you've worked in that area of code before.  Can you see
if the drag n drop code here looks reasonable?  If so, I'll approve.  Thanks!

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

http://codereview.chromium.org/11088043/diff/12002/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:175: if
(embedder_render_view_host_) {
I thought it wasn't possible for this to be invalid.  Maybe it's better to let
it crash on a null deref below if that assumption is incorrect, so that we can
track it down and fix it.  Otherwise we'll never know that the drag cursor is
broken.

[sadrul, 2012-10-11 18:31:18]

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

http://codereview.chromium.org/11088043/diff/12002/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:175: if
(embedder_render_view_host_) {
On 2012/10/11 18:02:52, creis wrote:
> I thought it wasn't possible for this to be invalid.  Maybe it's better to let
> it crash on a null deref below if that assumption is incorrect, so that we can
> track it down and fix it.  Otherwise we'll never know that the drag cursor is
> broken.

Sounds good. Done.

http://codereview.chromium.org/11088043/diff/12002/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

http://codereview.chromium.org/11088043/diff/12002/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:724:
IN_PROC_BROWSER_TEST_F(BrowserPluginHostTest, AcceptDragEvents) {
On 2012/10/11 17:57:09, Fady Samuel wrote:
> What's happening here is not obvious to me. Please comment on how this test
> works and what's happening at each major step of the test. What are you
> verifying here?

Added comments (and simplified the test a bit too).

[dcheng, 2012-10-11 20:11:51]

The drag and drop portion looks OK (sorry I had to take a look at the WebKit
implementation as well). I have a few comments on other stuff though.

http://codereview.chromium.org/11088043/diff/14002/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

http://codereview.chromium.org/11088043/diff/14002/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:727: const
char* kEmbedderURL = "files/browser_plugin_dragging.html";
Nit: const char[] is preferred to const char* const. It seems like it's already
pretty inconsistent in this file though (see lines 39, 46, 60, and 65).

http://codereview.chromium.org/11088043/diff/14002/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:735:
base::Value* v = rvh->ExecuteJavascriptAndGetValue(string16(),
Don't we need to delete the return value? Otherwise we leak it don't we?

http://codereview.chromium.org/11088043/diff/14002/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:749: GURL
file_url = GURL("https://www.domain.com/index.html");
Any particular reason this is file_url instead of just url?

http://codereview.chromium.org/11088043/diff/14002/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:751:
drop_data.html_base_url = file_url;
No need to set html_base_url if there's no HTML associated with the drag.

http://codereview.chromium.org/11088043/diff/14002/content/test/data/browser_...
File content/test/data/browser_plugin_dragging.html (right):

http://codereview.chromium.org/11088043/diff/14002/content/test/data/browser_...
content/test/data/browser_plugin_dragging.html:5: tabindex="0"
Nit: Do we need to explicitly set a tabindex?

http://codereview.chromium.org/11088043/diff/14002/content/test/data/browser_...
content/test/data/browser_plugin_dragging.html:10: style='border: solid 1px red'
Nit: It seems like line 10 cancels out line 9?

http://codereview.chromium.org/11088043/diff/14002/content/test/data/browser_...
content/test/data/browser_plugin_dragging.html:18: id_message.offsetTop + 5];
Nit: For uniformity, it'd be nice to use offsetWidth / 2 and offsetHeight / 2
here as well.

http://codereview.chromium.org/11088043/diff/14002/content/test/data/browser_...
content/test/data/browser_plugin_dragging.html:26: function SetSrc(src) {
Nit: This inconsistent naming is wonky. I guess the ship has sailed on that one
though (I see it's used similarly in other browser plugin tests)

[sadrul, 2012-10-11 20:41:21]

Addressed all the comments. Thanks for the quick and thorough review!

http://codereview.chromium.org/11088043/diff/14002/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

http://codereview.chromium.org/11088043/diff/14002/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:727: const
char* kEmbedderURL = "files/browser_plugin_dragging.html";
On 2012/10/11 20:11:51, dcheng wrote:
> Nit: const char[] is preferred to const char* const. It seems like it's
already
> pretty inconsistent in this file though (see lines 39, 46, 60, and 65).

Indeed. I have gone ahead and fixed all to use []

http://codereview.chromium.org/11088043/diff/14002/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:735:
base::Value* v = rvh->ExecuteJavascriptAndGetValue(string16(),
On 2012/10/11 20:11:51, dcheng wrote:
> Don't we need to delete the return value? Otherwise we leak it don't we?

You are correct. This value is indeed leaking. I have fixed this (and in the
other tests in this file as well). Great catch. Thanks!

http://codereview.chromium.org/11088043/diff/14002/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:749: GURL
file_url = GURL("https://www.domain.com/index.html");
On 2012/10/11 20:11:51, dcheng wrote:
> Any particular reason this is file_url instead of just url?

copy pasta :) Fixed

http://codereview.chromium.org/11088043/diff/14002/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:751:
drop_data.html_base_url = file_url;
On 2012/10/11 20:11:51, dcheng wrote:
> No need to set html_base_url if there's no HTML associated with the drag.

Fixed.

http://codereview.chromium.org/11088043/diff/14002/content/test/data/browser_...
File content/test/data/browser_plugin_dragging.html (right):

http://codereview.chromium.org/11088043/diff/14002/content/test/data/browser_...
content/test/data/browser_plugin_dragging.html:5: tabindex="0"
On 2012/10/11 20:11:51, dcheng wrote:
> Nit: Do we need to explicitly set a tabindex?

I believe tabindex is necessary to be able to tab-into the browser plugin, yes.

http://codereview.chromium.org/11088043/diff/14002/content/test/data/browser_...
content/test/data/browser_plugin_dragging.html:10: style='border: solid 1px red'
On 2012/10/11 20:11:51, dcheng wrote:
> Nit: It seems like line 10 cancels out line 9?

Done.

http://codereview.chromium.org/11088043/diff/14002/content/test/data/browser_...
content/test/data/browser_plugin_dragging.html:18: id_message.offsetTop + 5];
On 2012/10/11 20:11:51, dcheng wrote:
> Nit: For uniformity, it'd be nice to use offsetWidth / 2 and offsetHeight / 2
> here as well.

Done.

http://codereview.chromium.org/11088043/diff/14002/content/test/data/browser_...
content/test/data/browser_plugin_dragging.html:26: function SetSrc(src) {
On 2012/10/11 20:11:51, dcheng wrote:
> Nit: This inconsistent naming is wonky. I guess the ship has sailed on that
one
> though (I see it's used similarly in other browser plugin tests)

Indeed. I have kept it as it is for now. But it is a good candidate for cleanup.

[dcheng, 2012-10-11 21:00:38]

Drag and drop LGTM. Thanks for making those fixes.

[Charlie Reis, 2012-10-11 21:28:19]

Thanks, all!  LGTM with nit.

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

http://codereview.chromium.org/11088043/diff/9005/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:248: //
Executes the javascript sunchronously and makes sure the returned value is
nit: synchronously

[sadrul, 2012-10-12 00:18:47]

Thanks for the reviews everyone! :)

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

http://codereview.chromium.org/11088043/diff/9005/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:248: //
Executes the javascript sunchronously and makes sure the returned value is
On 2012/10/11 21:28:20, creis wrote:
> nit: synchronously

Done.