Move download messages from Renderer to Frame filter.

Move download messages from Renderer to Frame filter.

Out-of-process iframes have a "swapped out" render view so the ViewHost message doesn't get received. Moving the download messages to the FrameHost allows download links to work for out-of-process iframes also.

The message handling code is just movd unchanged from RenderViewHost to RenderFrameHost. The one change is that the OnSaveImageFromDataURL now checks data_url.is_valid() before interrogating the GURL (which would otherwise assert for invalid URLs).

BUG=596283
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd
Cr-Commit-Position: refs/heads/master@{#400788}

[brettw, 2016-03-28 22:03:59]

Description was changed from

==========
Move download messages from Renderer to Frame filter.

Out-of-process iframes have a "swapped out" render view so the ViewHost message
doesn't get received. Moving the download  messages to the FrameHost allows
download links to work for out-of-process iframes also.

BUG=596283
==========

to

==========
Move download messages from Renderer to Frame filter.

Out-of-process iframes have a "swapped out" render view so the ViewHost message
doesn't get received. Moving the download  messages to the FrameHost allows
download links to work for out-of-process iframes also.

BUG=596283
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[brettw, 2016-03-28 22:04:42]

Description was changed from

==========
Move download messages from Renderer to Frame filter.

Out-of-process iframes have a "swapped out" render view so the ViewHost message
doesn't get received. Moving the download  messages to the FrameHost allows
download links to work for out-of-process iframes also.

BUG=596283
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Move download messages from Renderer to Frame filter.

Out-of-process iframes have a "swapped out" render view so the ViewHost message
doesn't get received. Moving the download messages to the FrameHost allows
download links to work for out-of-process iframes also.

BUG=596283
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[brettw, 2016-03-28 22:18:39]

Description was changed from

==========
Move download messages from Renderer to Frame filter.

Out-of-process iframes have a "swapped out" render view so the ViewHost message
doesn't get received. Moving the download messages to the FrameHost allows
download links to work for out-of-process iframes also.

BUG=596283
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Move download messages from Renderer to Frame filter.

Out-of-process iframes have a "swapped out" render view so the ViewHost message
doesn't get received. Moving the download messages to the FrameHost allows
download links to work for out-of-process iframes also.

The message handling code is just movd unchanged from RenderViewHost to
RenderFrameHost. The one change is that the OnSaveImageFromDataURL now checks
data_url.is_valid() before interrogating the GURL (which would otherwise assert
for invalid URLs).

BUG=596283
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[brettw, 2016-03-28 22:21:13]

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

[brettw, 2016-03-28 22:21:14]

I some work still to do for the image data URL save unit test (there's no
render_frame_host_unittest.cc file so I need to create one). Before I do that, I
wanted to sanity check that I took the correct approach. Is it OK to just move
messages from the view to the frame like I did? Everything seems to work.

[Charlie Reis, 2016-03-29 04:41:48]

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

[Charlie Reis, 2016-03-29 04:41:49]

Interesting-- there's some things I wouldn't expect to work, but that's just
from reading the code without trying it.  Adding asanka@ in case he knows more
about the questions below.

At a high level, though, moving the IPCs to be frame messages makes sense to me,
and should mostly solve the bug.

https://codereview.chromium.org/1836973003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_message_filter.h (right):

https://codereview.chromium.org/1836973003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_message_filter.h:34: struct Referrer;
nit: structs go together after the classes, right?

https://codereview.chromium.org/1836973003/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1836973003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:1421: webview()->saveImageAt(WebPoint(x,
y));
From codesearch, it looks like this is what triggers the saveImageFromDataURL
call.  I'd be surprised if this works for OOPIFs already, unless the browser
process translates the point to the correct frame.  (Maybe this is worth moving
from WebView to WebFrame in a separate CL?)

https://codereview.chromium.org/1836973003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:1695: GetRoutingID(),
GetMainRenderFrame()->GetRoutingID(),
This looks problematic-- we probably shouldn't be assuming that
GetMainRenderFrame() exists.  (It won't in OOPIFs.)  I would kind of expect it
to crash in that case, or if not return the wrong routing ID.

Does the CL fix Rob's repro steps as is?  Maybe we can come back to this.

[brettw, 2016-03-29 05:44:39]

https://codereview.chromium.org/1836973003/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1836973003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:1695: GetRoutingID(),
GetMainRenderFrame()->GetRoutingID(),
On 2016/03/29 04:41:49, Charlie Reis wrote:
> This looks problematic-- we probably shouldn't be assuming that
> GetMainRenderFrame() exists.  (It won't in OOPIFs.)  I would kind of expect it
> to crash in that case, or if not return the wrong routing ID.
> 
> Does the CL fix Rob's repro steps as is?  Maybe we can come back to this.

It seems to fix the repro steps. I looked through the code a bit when I started
changing things to make sense of the IDs, and it looks like these are given to
the network system, I assume for prioritization and such. It doesn't look like
we do things like post back to the object (which is what would normally happen)
and the network stack may be tolerant of such errors. I'll check more tomorrow.

[Charlie Reis, 2016-03-29 18:17:13]

[+rdsmith, csharrison]

https://codereview.chromium.org/1836973003/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1836973003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:1695: GetRoutingID(),
GetMainRenderFrame()->GetRoutingID(),
On 2016/03/29 05:44:39, brettw wrote:
> On 2016/03/29 04:41:49, Charlie Reis wrote:
> > This looks problematic-- we probably shouldn't be assuming that
> > GetMainRenderFrame() exists.  (It won't in OOPIFs.)  I would kind of expect
it
> > to crash in that case, or if not return the wrong routing ID.
> > 
> > Does the CL fix Rob's repro steps as is?  Maybe we can come back to this.
> 
> It seems to fix the repro steps. I looked through the code a bit when I
started
> changing things to make sense of the IDs, and it looks like these are given to
> the network system, I assume for prioritization and such. It doesn't look like
> we do things like post back to the object (which is what would normally
happen)
> and the network stack may be tolerant of such errors. I'll check more
tomorrow.

Ok.  CC'ing rdsmith (who added the RFH ID, I think) and csharrison (who has been
refactoring the net stack for RFHs) in case they know more.

I'm ok landing this with a TODO if it's a step in the right direction.  

Still, I'd be curious to know why we aren't crashing here, since
GetMainRenderFrame() returns null in an OOPIF process.  I guess we never get
this call in an OOPIF, due to the bug I mentioned above (line 1421)?

[Randy Smith (Not in Mondays), 2016-03-29 18:30:45]

On 2016/03/29 18:17:13, Charlie Reis wrote:
> [+rdsmith, csharrison]
> 
>
https://codereview.chromium.org/1836973003/diff/1/content/renderer/render_vie...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/1836973003/diff/1/content/renderer/render_vie...
> content/renderer/render_view_impl.cc:1695: GetRoutingID(),
> GetMainRenderFrame()->GetRoutingID(),
> On 2016/03/29 05:44:39, brettw wrote:
> > On 2016/03/29 04:41:49, Charlie Reis wrote:
> > > This looks problematic-- we probably shouldn't be assuming that
> > > GetMainRenderFrame() exists.  (It won't in OOPIFs.)  I would kind of
expect
> it
> > > to crash in that case, or if not return the wrong routing ID.
> > > 
> > > Does the CL fix Rob's repro steps as is?  Maybe we can come back to this.
> > 
> > It seems to fix the repro steps. I looked through the code a bit when I
> started
> > changing things to make sense of the IDs, and it looks like these are given
to
> > the network system, I assume for prioritization and such. It doesn't look
like
> > we do things like post back to the object (which is what would normally
> happen)
> > and the network stack may be tolerant of such errors. I'll check more
> tomorrow.
> 
> Ok.  CC'ing rdsmith (who added the RFH ID, I think) and csharrison (who has
been
> refactoring the net stack for RFHs) in case they know more.
> 
> I'm ok landing this with a TODO if it's a step in the right direction.  
> 
> Still, I'd be curious to know why we aren't crashing here, since
> GetMainRenderFrame() returns null in an OOPIF process.  I guess we never get
> this call in an OOPIF, due to the bug I mentioned above (line 1421)?

I'd like to make sure that Asanka runs his eyes over this before landing; I
think he's the person that's most on top of these issues for downloads
(responsibility's been passed around a bit).  I note he's already on the review
list, so presumably that would happen, but please don't land without his ok.

[asanka, 2016-03-29 20:56:05]

https://codereview.chromium.org/1836973003/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1836973003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:1695: GetRoutingID(),
GetMainRenderFrame()->GetRoutingID(),
On 2016/03/29 at 18:17:13, Charlie Reis wrote:
> On 2016/03/29 05:44:39, brettw wrote:
> > On 2016/03/29 04:41:49, Charlie Reis wrote:
> > > This looks problematic-- we probably shouldn't be assuming that
> > > GetMainRenderFrame() exists.  (It won't in OOPIFs.)  I would kind of
expect it
> > > to crash in that case, or if not return the wrong routing ID.
> > > 
> > > Does the CL fix Rob's repro steps as is?  Maybe we can come back to this.
> > 
> > It seems to fix the repro steps. I looked through the code a bit when I
started
> > changing things to make sense of the IDs, and it looks like these are given
to
> > the network system, I assume for prioritization and such. It doesn't look
like
> > we do things like post back to the object (which is what would normally
happen)
> > and the network stack may be tolerant of such errors. I'll check more
tomorrow.
> 
> Ok.  CC'ing rdsmith (who added the RFH ID, I think) and csharrison (who has
been refactoring the net stack for RFHs) in case they know more.
> 
> I'm ok landing this with a TODO if it's a step in the right direction.  
> 
> Still, I'd be curious to know why we aren't crashing here, since
GetMainRenderFrame() returns null in an OOPIF process.  I guess we never get
this call in an OOPIF, due to the bug I mentioned above (line 1421)?


AFAICT the reason why it's not crashing is:

* None of the test cases in the linked issue trip this code path. In order to
get here, you'll need to create a canvas element and download it via the "Save
image as..." context menu item. For it to be sensitive to OOPIF, the canvas
element needs to be in an OOPIF.

* Invoking "Save image as" on such a canvas element causes the SaveImageAt IPC
to be sent from the main frame's RenderViewHostImpl. The corresponding
RenderView does in fact have a valid main_render_frame_.

* The RenderView that receives the SaveImageAt IPC doesn't have an image at the
location, since there's an iframe there instead. Therefore nothing happens.
Whether or not the main_render_frame_ is valid appears to be moot.

I'm guessing this code path doesn't have test coverage? It's broken ToT.

Also, the code above is trying to dispatch a FrameHostMsg to a RenderViewImpl,
which isn't quite relevant since the code runs on the wrong process.

[brettw, 2016-03-29 22:23:46]

On 2016/03/29 20:56:05, asanka wrote:
>
https://codereview.chromium.org/1836973003/diff/1/content/renderer/render_vie...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/1836973003/diff/1/content/renderer/render_vie...
> content/renderer/render_view_impl.cc:1695: GetRoutingID(),
> GetMainRenderFrame()->GetRoutingID(),
> On 2016/03/29 at 18:17:13, Charlie Reis wrote:
> > On 2016/03/29 05:44:39, brettw wrote:
> > > On 2016/03/29 04:41:49, Charlie Reis wrote:
> > > > This looks problematic-- we probably shouldn't be assuming that
> > > > GetMainRenderFrame() exists.  (It won't in OOPIFs.)  I would kind of
> expect it
> > > > to crash in that case, or if not return the wrong routing ID.
> > > > 
> > > > Does the CL fix Rob's repro steps as is?  Maybe we can come back to
this.
> > > 
> > > It seems to fix the repro steps. I looked through the code a bit when I
> started
> > > changing things to make sense of the IDs, and it looks like these are
given
> to
> > > the network system, I assume for prioritization and such. It doesn't look
> like
> > > we do things like post back to the object (which is what would normally
> happen)
> > > and the network stack may be tolerant of such errors. I'll check more
> tomorrow.
> > 
> > Ok.  CC'ing rdsmith (who added the RFH ID, I think) and csharrison (who has
> been refactoring the net stack for RFHs) in case they know more.
> > 
> > I'm ok landing this with a TODO if it's a step in the right direction.  
> > 
> > Still, I'd be curious to know why we aren't crashing here, since
> GetMainRenderFrame() returns null in an OOPIF process.  I guess we never get
> this call in an OOPIF, due to the bug I mentioned above (line 1421)?
> 
> 
> AFAICT the reason why it's not crashing is:
> 
> * None of the test cases in the linked issue trip this code path. In order to
> get here, you'll need to create a canvas element and download it via the "Save
> image as..." context menu item. For it to be sensitive to OOPIF, the canvas
> element needs to be in an OOPIF.
> 
> * Invoking "Save image as" on such a canvas element causes the SaveImageAt IPC
> to be sent from the main frame's RenderViewHostImpl. The corresponding
> RenderView does in fact have a valid main_render_frame_.
> 
> * The RenderView that receives the SaveImageAt IPC doesn't have an image at
the
> location, since there's an iframe there instead. Therefore nothing happens.
> Whether or not the main_render_frame_ is valid appears to be moot.
> 
> I'm guessing this code path doesn't have test coverage? It's broken ToT.
> 
> Also, the code above is trying to dispatch a FrameHostMsg to a RenderViewImpl,
> which isn't quite relevant since the code runs on the wrong process.

Thanks, I will come back to this (distracted with prepping for talk tomorrow).

[asanka, 2016-03-30 00:59:25]

On 2016/03/29 at 22:23:46, brettw wrote:
> On 2016/03/29 20:56:05, asanka wrote:
> >
https://codereview.chromium.org/1836973003/diff/1/content/renderer/render_vie...
> > File content/renderer/render_view_impl.cc (right):
> > 
> >
https://codereview.chromium.org/1836973003/diff/1/content/renderer/render_vie...
> > content/renderer/render_view_impl.cc:1695: GetRoutingID(),
> > GetMainRenderFrame()->GetRoutingID(),
> > On 2016/03/29 at 18:17:13, Charlie Reis wrote:
> > > On 2016/03/29 05:44:39, brettw wrote:
> > > > On 2016/03/29 04:41:49, Charlie Reis wrote:
> > > > > This looks problematic-- we probably shouldn't be assuming that
> > > > > GetMainRenderFrame() exists.  (It won't in OOPIFs.)  I would kind of
> > expect it
> > > > > to crash in that case, or if not return the wrong routing ID.
> > > > > 
> > > > > Does the CL fix Rob's repro steps as is?  Maybe we can come back to
this.
> > > > 
> > > > It seems to fix the repro steps. I looked through the code a bit when I
> > started
> > > > changing things to make sense of the IDs, and it looks like these are
given
> > to
> > > > the network system, I assume for prioritization and such. It doesn't
look
> > like
> > > > we do things like post back to the object (which is what would normally
> > happen)
> > > > and the network stack may be tolerant of such errors. I'll check more
> > tomorrow.
> > > 
> > > Ok.  CC'ing rdsmith (who added the RFH ID, I think) and csharrison (who
has
> > been refactoring the net stack for RFHs) in case they know more.
> > > 
> > > I'm ok landing this with a TODO if it's a step in the right direction.  
> > > 
> > > Still, I'd be curious to know why we aren't crashing here, since
> > GetMainRenderFrame() returns null in an OOPIF process.  I guess we never get
> > this call in an OOPIF, due to the bug I mentioned above (line 1421)?
> > 
> > 
> > AFAICT the reason why it's not crashing is:
> > 
> > * None of the test cases in the linked issue trip this code path. In order
to
> > get here, you'll need to create a canvas element and download it via the
"Save
> > image as..." context menu item. For it to be sensitive to OOPIF, the canvas
> > element needs to be in an OOPIF.
> > 
> > * Invoking "Save image as" on such a canvas element causes the SaveImageAt
IPC
> > to be sent from the main frame's RenderViewHostImpl. The corresponding
> > RenderView does in fact have a valid main_render_frame_.
> > 
> > * The RenderView that receives the SaveImageAt IPC doesn't have an image at
the
> > location, since there's an iframe there instead. Therefore nothing happens.
> > Whether or not the main_render_frame_ is valid appears to be moot.

Just noting that the hit test done by WebViewImpl::saveImageAt() can traverse a
local frame, but fails with a remote frame. I'll defer to others on what the
correct plumbing is for save-image.

> > 
> > I'm guessing this code path doesn't have test coverage? It's broken ToT.
> > 
> > Also, the code above is trying to dispatch a FrameHostMsg to a
RenderViewImpl,
> > which isn't quite relevant since the code runs on the wrong process.
> 
> Thanks, I will come back to this (distracted with prepping for talk tomorrow).

[brettw, 2016-05-11 19:51:29]

merge

[brettw, 2016-05-13 17:16:51]

brettw@chromium.org changed reviewers:
+ dcheng@chromium.org

[brettw, 2016-05-13 17:16:51]

This should be ready for review, I think it is correct now. This patch is on top
of https://codereview.chromium.org/1977623002/

dcheng: Please look at third_party/WebKit changes.
asanka/creis: Other stuff.

[brettw, 2016-05-13 17:17:13]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-13 17:17:32]

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

[commit-bot: I haz the power, 2016-05-13 17:20:43]

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

[commit-bot: I haz the power, 2016-05-13 17:20:45]

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[brettw, 2016-05-13 21:01:12]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-13 21:01:49]

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

[commit-bot: I haz the power, 2016-05-13 21:05:24]

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

[commit-bot: I haz the power, 2016-05-13 21:05:25]

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Charlie Reis, 2016-05-13 21:50:22]

Awesome!  Content/ looks good now, but you mentioned you were going to add a
unit test, right?

On 2016/03/28 22:21:14, brettw wrote:
> I some work still to do for the image data URL save unit test (there's no
> render_frame_host_unittest.cc file so I need to create one). Before I do that,
I
> wanted to sanity check that I took the correct approach. Is it OK to just move
> messages from the view to the frame like I did? Everything seems to work.

https://codereview.chromium.org/1836973003/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_message_filter.cc (right):

https://codereview.chromium.org/1836973003/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_message_filter.cc:268: new
DownloadUrlParameters(url, render_process_id_, render_view_id,
Someday we should remove the use of RenderView ID and RenderProcess ID as the
way to identify a WebContents in DownloadUrlParameters, but it's not for this
CL.

https://codereview.chromium.org/1836973003/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_message_filter.h (right):

https://codereview.chromium.org/1836973003/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_message_filter.h:34: struct Referrer;
nit: structs after classes, right?

https://codereview.chromium.org/1836973003/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrame.h (right):

https://codereview.chromium.org/1836973003/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrame.h:543: virtual void copyImageAt(const
WebPoint&) = 0;
Do these need to live on WebFrame, or can they just be on WebLocalFrame?  Looks
like they're just NOTREACHED in WebRemoteFrame, and RenderFrameImpl knows it has
a WebLocalFrame.

[asanka, 2016-05-18 16:26:54]

https://codereview.chromium.org/1836973003/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_message_filter.h (right):

https://codereview.chromium.org/1836973003/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_message_filter.h:34: struct Referrer;
On 2016/05/13 at 21:50:22, Charlie Reis (slow to reply) wrote:
> nit: structs after classes, right?

Hm. I've also been sorting by identifier ignoring the class/struct, though I
can't find anything in the style guide that supports this.

https://codereview.chromium.org/1836973003/diff/100001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1836973003/diff/100001/content/common/frame_m...
content/common/frame_messages.h:1007:
IPC_MESSAGE_CONTROL5(FrameHostMsg_DownloadUrl,
Shall we switch these two messages to ROUTED and skip the IDs?

[dcheng, 2016-05-24 07:18:09]

https://codereview.chromium.org/1836973003/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_message_filter.h (right):

https://codereview.chromium.org/1836973003/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_message_filter.h:34: struct Referrer;
On 2016/05/18 at 16:26:53, asanka wrote:
> On 2016/05/13 at 21:50:22, Charlie Reis (slow to reply) wrote:
> > nit: structs after classes, right?
> 
> Hm. I've also been sorting by identifier ignoring the class/struct, though I
can't find anything in the style guide that supports this.

I also sort it by identifier, ignoring class name. IMO, this is consistent with
how #include / #imports are sorted by the path.

https://codereview.chromium.org/1836973003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1836973003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1355: if (!frame())
Can this actually happen in practice?

https://codereview.chromium.org/1836973003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1375: if (!frame() ||
!m_client)
Ditto: can this actually happen? //content is calling into Blink here. We know
we must have a non-null m_client at this point, and I think frame() should
probably be non-null here too.

[brettw, 2016-05-25 19:10:20]

https://codereview.chromium.org/1836973003/diff/100001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1836973003/diff/100001/content/common/frame_m...
content/common/frame_messages.h:1007:
IPC_MESSAGE_CONTROL5(FrameHostMsg_DownloadUrl,
On 2016/05/18 16:26:54, asanka wrote:
> Shall we switch these two messages to ROUTED and skip the IDs?

This message isn't actually routed to the view. It's grabbed by the filter on
the I/O thread that then needs the parameters. If we changed it to be routed we
would need to use a lower-level message dispatch so the message handlers can get
the view IDs out. This would be more complicated.

https://codereview.chromium.org/1836973003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1836973003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1355: if (!frame())
On 2016/05/24 07:18:08, dcheng wrote:
> Can this actually happen in practice?

No idea, I just copied some other code in this file. I can remove it if you
think it's OK. Same below.

[brettw, 2016-05-25 19:13:27]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[brettw, 2016-05-25 19:13:37]

merge

[commit-bot: I haz the power, 2016-05-25 19:14:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1836973003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1836973003/120001

[brettw, 2016-05-25 19:26:31]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[brettw, 2016-05-25 19:26:33]

fixmerge

[commit-bot: I haz the power, 2016-05-25 19:26:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1836973003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1836973003/140001

[commit-bot: I haz the power, 2016-05-25 19:42:40]

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

[commit-bot: I haz the power, 2016-05-25 19:42:41]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[brettw, 2016-06-02 23:50:15]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-02 23:51:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1836973003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1836973003/160001

[commit-bot: I haz the power, 2016-06-03 00:07:38]

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

[commit-bot: I haz the power, 2016-06-03 00:07:39]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[brettw, 2016-06-06 21:39:25]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[brettw, 2016-06-06 21:39:32]

merge, visualviewport

[commit-bot: I haz the power, 2016-06-06 21:39:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1836973003/180001

[commit-bot: I haz the power, 2016-06-06 21:53:26]

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

[commit-bot: I haz the power, 2016-06-06 21:53:29]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[brettw, 2016-06-06 22:32:16]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[brettw, 2016-06-06 22:32:18]

coordinates

[commit-bot: I haz the power, 2016-06-06 22:33:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1836973003/200001

[commit-bot: I haz the power, 2016-06-06 22:50:00]

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

[commit-bot: I haz the power, 2016-06-06 22:50:01]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[brettw, 2016-06-06 23:20:27]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[brettw, 2016-06-06 23:20:30]

.

[commit-bot: I haz the power, 2016-06-06 23:20:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1836973003/220001

[brettw, 2016-06-06 23:21:24]

Patchset #12 (id:220001) has been deleted

[brettw, 2016-06-06 23:22:35]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[brettw, 2016-06-06 23:22:39]

.

[commit-bot: I haz the power, 2016-06-06 23:22:55]

Dry run: Try jobs failed on following builders:
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[commit-bot: I haz the power, 2016-06-06 23:23:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1836973003/240001

[brettw, 2016-06-06 23:24:26]

This is ready for review again. PTAL!

https://codereview.chromium.org/1836973003/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrame.h (right):

https://codereview.chromium.org/1836973003/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrame.h:543: virtual void copyImageAt(const
WebPoint&) = 0;
On 2016/05/13 21:50:22, Charlie Reis wrote:
> Do these need to live on WebFrame, or can they just be on WebLocalFrame? 
Looks
> like they're just NOTREACHED in WebRemoteFrame, and RenderFrameImpl knows it
has
> a WebLocalFrame.

Nothing else follows this pattern (WebLocal frame has non non-override members
it adds) so I'm hesitant to make any changes unless an expert in this API layer
asks me to change it.

[commit-bot: I haz the power, 2016-06-06 23:35:41]

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

[commit-bot: I haz the power, 2016-06-06 23:35:42]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[brettw, 2016-06-07 22:05:47]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[brettw, 2016-06-07 22:05:56]

RenderViewHostTest

[commit-bot: I haz the power, 2016-06-07 22:06:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1836973003/260001

[commit-bot: I haz the power, 2016-06-07 22:23:18]

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

[commit-bot: I haz the power, 2016-06-07 22:23:20]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Charlie Reis, 2016-06-07 22:51:25]

Thanks!  content/ LGTM, though I think we'll still want to move things from
WebFrame to WebLocalFrame.

https://codereview.chromium.org/1836973003/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrame.h (right):

https://codereview.chromium.org/1836973003/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrame.h:543: virtual void copyImageAt(const
WebPoint&) = 0;
On 2016/06/06 23:24:25, brettw wrote:
> On 2016/05/13 21:50:22, Charlie Reis wrote:
> > Do these need to live on WebFrame, or can they just be on WebLocalFrame? 
> Looks
> > like they're just NOTREACHED in WebRemoteFrame, and RenderFrameImpl knows it
> has
> > a WebLocalFrame.
> 
> Nothing else follows this pattern (WebLocal frame has non non-override members
> it adds) so I'm hesitant to make any changes unless an expert in this API
layer
> asks me to change it.

I don't follow.  WebLocalFrame has lots of things that WebFrame doesn't, like
localRoot(), dispatchBeforeUnloadEvent(), loadData(), etc.  This seems to fall
in that category (i.e., things you can't do on a WebRemoteFrame). 
WebLocalFrameImpl then overrides and implements these, from one interface or the
other.

That said, I'm happy to defer to Daniel here.

https://codereview.chromium.org/1836973003/diff/260001/components/test_runner...
File components/test_runner/pixel_dump.cc (right):

https://codereview.chromium.org/1836973003/diff/260001/components/test_runner...
components/test_runner/pixel_dump.cc:188:
web_view->mainFrame()->copyImageAt(blink::WebPoint(x, y));
Let's add:
// TODO(lukasza): Support image capture in OOPIFs for https://crbug.com/477150.

https://codereview.chromium.org/1836973003/diff/260001/content/browser/frame_...
File content/browser/frame_host/render_frame_message_filter.cc (right):

https://codereview.chromium.org/1836973003/diff/260001/content/browser/frame_...
content/browser/frame_host/render_frame_message_filter.cc:428: // Please refer
to RenderViewImpl::saveImageFromDataURL().
nit: RenderFrameImpl

[dcheng, 2016-06-08 16:00:33]

dcheng@chromium.org changed reviewers:
+ bokan@chromium.org

[dcheng, 2016-06-08 16:00:35]

https://codereview.chromium.org/1836973003/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrame.h (right):

https://codereview.chromium.org/1836973003/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrame.h:543: virtual void copyImageAt(const
WebPoint&) = 0;
On 2016/06/07 at 22:51:25, Charlie Reis wrote:
> On 2016/06/06 23:24:25, brettw wrote:
> > On 2016/05/13 21:50:22, Charlie Reis wrote:
> > > Do these need to live on WebFrame, or can they just be on WebLocalFrame? 
> > Looks
> > > like they're just NOTREACHED in WebRemoteFrame, and RenderFrameImpl knows
it
> > has
> > > a WebLocalFrame.
> > 
> > Nothing else follows this pattern (WebLocal frame has non non-override
members
> > it adds) so I'm hesitant to make any changes unless an expert in this API
layer
> > asks me to change it.
> 
> I don't follow.  WebLocalFrame has lots of things that WebFrame doesn't, like
localRoot(), dispatchBeforeUnloadEvent(), loadData(), etc.  This seems to fall
in that category (i.e., things you can't do on a WebRemoteFrame). 
WebLocalFrameImpl then overrides and implements these, from one interface or the
other.
> 
> That said, I'm happy to defer to Daniel here.

These should be on WebLocalFrame: this is consistent with the other changes
we've been making for OOPI.

https://codereview.chromium.org/1836973003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1836973003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1794: IntPoint
rootFramePoint(frame()->host()->visualViewport().viewportToRootFrame(posInViewport));
+bokan, is this the right thing to do for a local frame with a remote parent?

[bokan, 2016-06-08 16:13:27]

https://codereview.chromium.org/1836973003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1836973003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1794: IntPoint
rootFramePoint(frame()->host()->visualViewport().viewportToRootFrame(posInViewport));
On 2016/06/08 16:00:35, dcheng wrote:
> +bokan, is this the right thing to do for a local frame with a remote parent?

Hmm...I'm not sure how VisualViewport behaves in OOPIF scenarios. Suppose we
pinch-zoom in and move around such that the non-OOPIF VisualViewport would have
a scaleFactor and scrollOffset. In OOPIF, does the VisualViewport in the child
process reflect the scaleFactor and scrollOffset seen by the user? Or does each
process have its own independent VisualViewport?

If the former, and we've done the work to sync the VisualViewport values between
processes, than this would be ok w.r.t. passed in points that are actually in
"viewport coordinates". That is, relative to the window origin on desktop.

VisualViewport aside, the local frame doesn't have access to the entire Widget
hierarchy does it? I think rootFrameToContents below will be problematic since
it needs to walk through all the parent widgets and adjust by their locations
and scroll offsets. At a quick glance, it doesn't look like that code has been
OOPIF'd so remote frames will be problematic there.

[alexmos, 2016-06-08 16:40:31]

alexmos@chromium.org changed reviewers:
+ alexmos@chromium.org

[alexmos, 2016-06-08 16:40:33]

https://codereview.chromium.org/1836973003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1836973003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1794: IntPoint
rootFramePoint(frame()->host()->visualViewport().viewportToRootFrame(posInViewport));
On 2016/06/08 16:13:27, bokan wrote:
> On 2016/06/08 16:00:35, dcheng wrote:
> > +bokan, is this the right thing to do for a local frame with a remote
parent?
> 
> Hmm...I'm not sure how VisualViewport behaves in OOPIF scenarios. Suppose we
> pinch-zoom in and move around such that the non-OOPIF VisualViewport would
have
> a scaleFactor and scrollOffset. In OOPIF, does the VisualViewport in the child
> process reflect the scaleFactor and scrollOffset seen by the user? Or does
each
> process have its own independent VisualViewport?

Note that I recently did some work to sync the VisualViewport size in
https://codereview.chromium.org/1834913002/.  I'm not sure about scaleFactor and
scrollOffset though.

[brettw, 2016-06-09 23:41:01]

New snap up.

I changed a lot of tests. The code moved from WebViewTest to WebFrameTest and I
updated the names of the helpers and the way they were instantiated, but nothing
else changed.

https://codereview.chromium.org/1836973003/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrame.h (right):

https://codereview.chromium.org/1836973003/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrame.h:543: virtual void copyImageAt(const
WebPoint&) = 0;
Ah, when I read it I confused "= 0" with "override" and thought everything as
implemented from WebFrame! Changed.

https://codereview.chromium.org/1836973003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1836973003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1794: IntPoint
rootFramePoint(frame()->host()->visualViewport().viewportToRootFrame(posInViewport));
I didn't make any changes here. If I should change something please just let me
know. I wrote this code based on what was happening elsewhere and what I could
figure out that worked.

[brettw, 2016-06-09 23:41:11]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-09 23:41:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1836973003/280001

[commit-bot: I haz the power, 2016-06-10 00:01:40]

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

[commit-bot: I haz the power, 2016-06-10 00:01:41]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Charlie Reis, 2016-06-10 18:56:48]

On 2016/06/09 23:41:01, brettw wrote:
> New snap up.
> 
> I changed a lot of tests. The code moved from WebViewTest to WebFrameTest and
I
> updated the names of the helpers and the way they were instantiated, but
nothing
> else changed.

There's a rebase in the same patch set, so it's hard for me to tell what
changed.  It looks like the main changes are in Blink (i.e., mostly WebFrameTest
and WebViewTest)?  I'll let dcheng@ and bokan@ take a look, since content/ still
seems fine.

[brettw, 2016-06-10 21:09:28]

On 2016/06/10 18:56:48, Charlie Reis wrote:
> On 2016/06/09 23:41:01, brettw wrote:
> > New snap up.
> > 
> > I changed a lot of tests. The code moved from WebViewTest to WebFrameTest
and
> I
> > updated the names of the helpers and the way they were instantiated, but
> nothing
> > else changed.
> 
> There's a rebase in the same patch set, so it's hard for me to tell what
> changed.  It looks like the main changes are in Blink (i.e., mostly
WebFrameTest
> and WebViewTest)?  I'll let dcheng@ and bokan@ take a look, since content/
still
> seems fine.

Sorry, yes. The changes are just moving the stuff to WebLocalFrame and moving
the tests.

[dcheng, 2016-06-12 05:58:35]

https://codereview.chromium.org/1836973003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1836973003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1764: IntPoint
rootFramePoint(frame()->host()->visualViewport().viewportToRootFrame(posInViewport));
Do you mind doing a quick test whether this does the right thing for an OOPI
that you'd have to scroll into the viewport? It doesn't seem like it'll work
properly in that case, and if that's true, we should stick a todo on it.

https://codereview.chromium.org/1836973003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1836973003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:8674: static void
setSecurityOrigin(WebFrame* frame, PassRefPtr<SecurityOrigin> securityOrigin)
Lines 8674 through 8731 look like they're from code that no longer exists (the
CL was reverted): let's remove it.

[brettw, 2016-06-14 21:48:28]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[brettw, 2016-06-14 21:48:38]

pixel_dump

[commit-bot: I haz the power, 2016-06-14 21:49:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1836973003/300001

[commit-bot: I haz the power, 2016-06-14 22:10:58]

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

[commit-bot: I haz the power, 2016-06-14 22:11:00]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[brettw, 2016-06-15 17:22:12]

https://codereview.chromium.org/1836973003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1836973003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1764: IntPoint
rootFramePoint(frame()->host()->visualViewport().viewportToRootFrame(posInViewport));
On 2016/06/12 05:58:34, dcheng wrote:
> Do you mind doing a quick test whether this does the right thing for an OOPI
> that you'd have to scroll into the viewport? It doesn't seem like it'll work
> properly in that case, and if that's true, we should stick a todo on it.

It seems to work.

[brettw, 2016-06-15 17:49:11]

move test, no code changes

[brettw, 2016-06-15 17:59:20]

Latest patch fixes content_browertests. PS 16 just moves the test with no code
change, PS 17 fixes the test (so just diff 16 and 17 to see what actually
changed).

[brettw, 2016-06-15 17:59:30]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-15 18:00:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1836973003/340001

[commit-bot: I haz the power, 2016-06-15 18:15:52]

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

[commit-bot: I haz the power, 2016-06-15 18:15:54]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[brettw, 2016-06-15 18:27:25]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[brettw, 2016-06-15 18:27:28]

merge boo boo

[commit-bot: I haz the power, 2016-06-15 18:27:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1836973003/360001

[commit-bot: I haz the power, 2016-06-15 20:13:09]

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

[commit-bot: I haz the power, 2016-06-15 20:13:12]

Dry run: This issue passed the CQ dry run.

[brettw, 2016-06-16 21:14:37]

ping

[dcheng, 2016-06-17 20:17:57]

Blink + IPC LGTM

https://codereview.chromium.org/1836973003/diff/360001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1836973003/diff/360001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1902: frame_->copyImageAt(WebPoint(x, y));
Unrelated, but it feels like (x, y) should just be a gfx::Point. I wonder why
ContextMenuParams doesn't just use gfx::Point...

[asanka, 2016-06-20 17:13:18]

LGTM.

Would you be willing to add a render_view_context_menu_browsertest test that
uses iframes and either save image or copy image?

https://codereview.chromium.org/1836973003/diff/360001/content/renderer/rende...
File content/renderer/render_frame_impl_browsertest.cc (right):

https://codereview.chromium.org/1836973003/diff/360001/content/renderer/rende...
content/renderer/render_frame_impl_browsertest.cc:317:
EXPECT_EQ(std::get<2>(param1).length(), image_data_url.length());
Redundant? Here and below.

[brettw, 2016-06-20 19:34:45]

On 2016/06/20 17:13:18, asanka wrote:
> LGTM.
> 
> Would you be willing to add a render_view_context_menu_browsertest test that
> uses iframes and either save image or copy image?

I can do that if you think it's important. Realistically it's going to take me
many weeks at my current pace (this patch is almost 3 months old, I do a bit
every week). If you think it's worth the time and effort, is there an example of
testing OOPIF frames in this way?

[Charlie Reis, 2016-06-20 19:38:00]

On 2016/06/20 19:34:45, brettw wrote:
> On 2016/06/20 17:13:18, asanka wrote:
> > LGTM.
> > 
> > Would you be willing to add a render_view_context_menu_browsertest test that
> > uses iframes and either save image or copy image?
> 
> I can do that if you think it's important. Realistically it's going to take me
> many weeks at my current pace (this patch is almost 3 months old, I do a bit
> every week). If you think it's worth the time and effort, is there an example
of
> testing OOPIF frames in this way?

I'm always in favor of tests, but we're hoping to get this fix landed for M53 if
possible.  Maybe that test can be a followup?

[brettw, 2016-06-20 19:39:40]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[brettw, 2016-06-20 19:39:49]

review comments, merge

[commit-bot: I haz the power, 2016-06-20 19:40:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1836973003/380001

[asanka, 2016-06-20 19:50:12]

On 2016/06/20 at 19:34:45, brettw wrote:
> On 2016/06/20 17:13:18, asanka wrote:
> > LGTM.
> > 
> > Would you be willing to add a render_view_context_menu_browsertest test that
> > uses iframes and either save image or copy image?
> 
> I can do that if you think it's important. Realistically it's going to take me
many weeks at my current pace (this patch is almost 3 months old, I do a bit
every week). If you think it's worth the time and effort, is there an example of
testing OOPIF frames in this way?

Yeah, I was worried this CL was heading towards contributor-fatigue territory.
:-) So I'll leave it up to you. There aren't any tests for saving/copying images
from OOPIFs, which is why I suggested adding some.

download_browsertests.cc has a test for 'Save link as...', which could be copied
and modified to invoke IDC_CONTENT_CONTEXT_SAVEIMAGEAS instead. You'd also need
to construct an appropriate test document containing an image within an iframe
at a deterministic location. download_browsertest.cc is probably a better place
to keep this test since it has the infrastructure necessary for dealing with the
resulting file chooser.

[asanka, 2016-06-20 19:52:44]

On 2016/06/20 at 19:50:12, asanka wrote:
> On 2016/06/20 at 19:34:45, brettw wrote:
> > On 2016/06/20 17:13:18, asanka wrote:
> > > LGTM.
> > > 
> > > Would you be willing to add a render_view_context_menu_browsertest test
that
> > > uses iframes and either save image or copy image?
> > 
> > I can do that if you think it's important. Realistically it's going to take
me many weeks at my current pace (this patch is almost 3 months old, I do a bit
every week). If you think it's worth the time and effort, is there an example of
testing OOPIF frames in this way?
> 
> Yeah, I was worried this CL was heading towards contributor-fatigue territory.
:-) So I'll leave it up to you. There aren't any tests for saving/copying images
from OOPIFs, which is why I suggested adding some.
> 
> download_browsertests.cc has a test for 'Save link as...', which could be
copied and modified to invoke IDC_CONTENT_CONTEXT_SAVEIMAGEAS instead. You'd
also need to construct an appropriate test document containing an image within
an iframe at a deterministic location. download_browsertest.cc is probably a
better place to keep this test since it has the infrastructure necessary for
dealing with the resulting file chooser.

To clarify, you or I can toss in a test for this later. So feel free to consider
it to be non-blocking for this CL.

[commit-bot: I haz the power, 2016-06-20 21:17:38]

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

[commit-bot: I haz the power, 2016-06-20 21:17:40]

Dry run: This issue passed the CQ dry run.

[brettw, 2016-06-20 21:24:53]

Thanks, will press CQ on this.

[brettw, 2016-06-20 21:25:05]

The CQ bit was checked by brettw@chromium.org

[brettw, 2016-06-20 21:25:10]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
asanka@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1836973003/#ps380001
(title: "review comments, merge")

[commit-bot: I haz the power, 2016-06-20 21:25:33]

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

[commit-bot: I haz the power, 2016-06-20 21:34:39]

Description was changed from

==========
Move download messages from Renderer to Frame filter.

Out-of-process iframes have a "swapped out" render view so the ViewHost message
doesn't get received. Moving the download messages to the FrameHost allows
download links to work for out-of-process iframes also.

The message handling code is just movd unchanged from RenderViewHost to
RenderFrameHost. The one change is that the OnSaveImageFromDataURL now checks
data_url.is_valid() before interrogating the GURL (which would otherwise assert
for invalid URLs).

BUG=596283
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Move download messages from Renderer to Frame filter.

Out-of-process iframes have a "swapped out" render view so the ViewHost message
doesn't get received. Moving the download messages to the FrameHost allows
download links to work for out-of-process iframes also.

The message handling code is just movd unchanged from RenderViewHost to
RenderFrameHost. The one change is that the OnSaveImageFromDataURL now checks
data_url.is_valid() before interrogating the GURL (which would otherwise assert
for invalid URLs).

BUG=596283
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-06-20 21:34:41]

Committed patchset #19 (id:380001)

[commit-bot: I haz the power, 2016-06-20 21:39:52]

Description was changed from

==========
Move download messages from Renderer to Frame filter.

Out-of-process iframes have a "swapped out" render view so the ViewHost message
doesn't get received. Moving the download messages to the FrameHost allows
download links to work for out-of-process iframes also.

The message handling code is just movd unchanged from RenderViewHost to
RenderFrameHost. The one change is that the OnSaveImageFromDataURL now checks
data_url.is_valid() before interrogating the GURL (which would otherwise assert
for invalid URLs).

BUG=596283
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Move download messages from Renderer to Frame filter.

Out-of-process iframes have a "swapped out" render view so the ViewHost message
doesn't get received. Moving the download messages to the FrameHost allows
download links to work for out-of-process iframes also.

The message handling code is just movd unchanged from RenderViewHost to
RenderFrameHost. The one change is that the OnSaveImageFromDataURL now checks
data_url.is_valid() before interrogating the GURL (which would otherwise assert
for invalid URLs).

BUG=596283
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd
Cr-Commit-Position: refs/heads/master@{#400788}
==========

[commit-bot: I haz the power, 2016-06-20 21:39:55]

Patchset 19 (id:??) landed as
https://crrev.com/8f508f442da78a72c61cfec1dc4c77df57406cbd
Cr-Commit-Position: refs/heads/master@{#400788}