WebContentsImpl: IPC_BEGIN_MESSAGE_MAP_WITH_PARAM everywhere

Plumb RenderFrameHostImpl* or RenderViewHostImpl* to every message
handler as an argument, rather than passing them as data members.

Eliminate the render_frame_message_source_, render_view_message_source_
data members. Eliminate the HasValidFrameSource() helper method.

Eliminate the combo version of OnMessageReceived that took both an RFH
or an RVH, and instead do the dispatch in the delegate
OnMessageRecieved methods. Use the _WITH_PARAM dispatch macro to
plumb the RenderFrameHostImpl into the dispatch methods.

Fix some bugs that became apparent after this refactoring:
  OnEndColorChooser: insist on rfh/rph match (via a ::Matches helper)
  OnSetSelectedColorInColorChooser: insist on rfh/rph match
  OnOpenDateTimeDialog: use |source| rather than GetRenderViewHost.
  OnEnumerateDirectory: Use the right process for permissions check
  OnRequestPpapiBrokerPermission: rename |routing_id| to reflect that
    it's not a frame or view route ID.
  SendPpapiBrokerPermissionResult: rename this from On... to
    Send..., since it's not a dispatcher. Parameterize by the
    RPH id, so that we send the response to the right process.

BUG=304341
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[ncarter (slow), 2016-12-10 02:34:31]

Description was changed from

==========
WebContentsImpl: always pass the relevant RFH/RVH param to message dispatch

BUG=304341
==========

to

==========
WebContentsImpl: always pass the relevant RFH/RVH param to message dispatch

BUG=304341
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ncarter (slow), 2016-12-10 02:34:49]

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

[commit-bot: I haz the power, 2016-12-10 02:35:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 02:39:15]

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

[commit-bot: I haz the power, 2016-12-10 02:39:15]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ncarter (slow), 2016-12-12 19:43:01]

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

[commit-bot: I haz the power, 2016-12-12 19:44:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 20:00:42]

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

[commit-bot: I haz the power, 2016-12-12 20:00:43]

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

[ncarter (slow), 2016-12-12 20:16:12]

Description was changed from

==========
WebContentsImpl: always pass the relevant RFH/RVH param to message dispatch

BUG=304341
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Plumb RenderFrameHostImpl* or RenderViewHostImpl* to every message
handler as an argument, rather than passing them as data members.

Eliminate the render_frame_message_source_, render_view_message_source_
data members. Eliminate the HasValidFrameSource() helper method.

Eliminate the combo version of OnMessageReceived that took both an RFH
or an RVH, and instead do the dispatch in the delegate
OnMessageRecieved methods. Use the _WITH_PARAM dispatch macro to
plumb the RenderFrameHostImpl into the dispatch methods.

Fix some bugs that became apparent after this refactoring:
  OnEndColorChooser: insist on rfh/rph match (via a ::Matches helper)
  OnSetSelectedColorInColorChooser: insist on rfh/rph match
  OnOpenDateTimeDialog: use |source| rather than GetRenderViewHost.
  OnEnumerateDirectory: Use the right process for permissions check
  OnRequestPpapiBrokerPermission: rename |routing_id| to reflect that
    it's not a frame or view route ID.
  SendPpapiBrokerPermissionResult: Send a response to RPH that initiated
    the request.

BUG=304341
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ncarter (slow), 2016-12-12 20:18:18]

Description was changed from

==========
Plumb RenderFrameHostImpl* or RenderViewHostImpl* to every message
handler as an argument, rather than passing them as data members.

Eliminate the render_frame_message_source_, render_view_message_source_
data members. Eliminate the HasValidFrameSource() helper method.

Eliminate the combo version of OnMessageReceived that took both an RFH
or an RVH, and instead do the dispatch in the delegate
OnMessageRecieved methods. Use the _WITH_PARAM dispatch macro to
plumb the RenderFrameHostImpl into the dispatch methods.

Fix some bugs that became apparent after this refactoring:
  OnEndColorChooser: insist on rfh/rph match (via a ::Matches helper)
  OnSetSelectedColorInColorChooser: insist on rfh/rph match
  OnOpenDateTimeDialog: use |source| rather than GetRenderViewHost.
  OnEnumerateDirectory: Use the right process for permissions check
  OnRequestPpapiBrokerPermission: rename |routing_id| to reflect that
    it's not a frame or view route ID.
  SendPpapiBrokerPermissionResult: Send a response to RPH that initiated
    the request.

BUG=304341
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Plumb RenderFrameHostImpl* or RenderViewHostImpl* to every message
handler as an argument, rather than passing them as data members.

Eliminate the render_frame_message_source_, render_view_message_source_
data members. Eliminate the HasValidFrameSource() helper method.

Eliminate the combo version of OnMessageReceived that took both an RFH
or an RVH, and instead do the dispatch in the delegate
OnMessageRecieved methods. Use the _WITH_PARAM dispatch macro to
plumb the RenderFrameHostImpl into the dispatch methods.

Fix some bugs that became apparent after this refactoring:
  OnEndColorChooser: insist on rfh/rph match (via a ::Matches helper)
  OnSetSelectedColorInColorChooser: insist on rfh/rph match
  OnOpenDateTimeDialog: use |source| rather than GetRenderViewHost.
  OnEnumerateDirectory: Use the right process for permissions check
  OnRequestPpapiBrokerPermission: rename |routing_id| to reflect that
    it's not a frame or view route ID.
  SendPpapiBrokerPermissionResult: rename this from On... to
    Send..., since it's not a dispatcher. Parameterize by the
    RPH id, so that we send the response to the right one.

BUG=304341
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ncarter (slow), 2016-12-12 20:20:21]

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

[ncarter (slow), 2016-12-12 20:21:00]

Description was changed from

==========
Plumb RenderFrameHostImpl* or RenderViewHostImpl* to every message
handler as an argument, rather than passing them as data members.

Eliminate the render_frame_message_source_, render_view_message_source_
data members. Eliminate the HasValidFrameSource() helper method.

Eliminate the combo version of OnMessageReceived that took both an RFH
or an RVH, and instead do the dispatch in the delegate
OnMessageRecieved methods. Use the _WITH_PARAM dispatch macro to
plumb the RenderFrameHostImpl into the dispatch methods.

Fix some bugs that became apparent after this refactoring:
  OnEndColorChooser: insist on rfh/rph match (via a ::Matches helper)
  OnSetSelectedColorInColorChooser: insist on rfh/rph match
  OnOpenDateTimeDialog: use |source| rather than GetRenderViewHost.
  OnEnumerateDirectory: Use the right process for permissions check
  OnRequestPpapiBrokerPermission: rename |routing_id| to reflect that
    it's not a frame or view route ID.
  SendPpapiBrokerPermissionResult: rename this from On... to
    Send..., since it's not a dispatcher. Parameterize by the
    RPH id, so that we send the response to the right one.

BUG=304341
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Plumb RenderFrameHostImpl* or RenderViewHostImpl* to every message
handler as an argument, rather than passing them as data members.

Eliminate the render_frame_message_source_, render_view_message_source_
data members. Eliminate the HasValidFrameSource() helper method.

Eliminate the combo version of OnMessageReceived that took both an RFH
or an RVH, and instead do the dispatch in the delegate
OnMessageRecieved methods. Use the _WITH_PARAM dispatch macro to
plumb the RenderFrameHostImpl into the dispatch methods.

Fix some bugs that became apparent after this refactoring:
  OnEndColorChooser: insist on rfh/rph match (via a ::Matches helper)
  OnSetSelectedColorInColorChooser: insist on rfh/rph match
  OnOpenDateTimeDialog: use |source| rather than GetRenderViewHost.
  OnEnumerateDirectory: Use the right process for permissions check
  OnRequestPpapiBrokerPermission: rename |routing_id| to reflect that
    it's not a frame or view route ID.
  SendPpapiBrokerPermissionResult: rename this from On... to
    Send..., since it's not a dispatcher. Parameterize by the
    RPH id, so that we send the response to the right process.

BUG=304341
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

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

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 20:34:57]

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

[commit-bot: I haz the power, 2016-12-12 20:34:57]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ncarter (slow), 2016-12-12 20:56:02]

Patchset #4 (id:60001) has been deleted

[ncarter (slow), 2016-12-12 21:13:39]

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

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

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 21:35:18]

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

[commit-bot: I haz the power, 2016-12-12 21:35:19]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ncarter (slow), 2016-12-12 21:39:35]

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

[commit-bot: I haz the power, 2016-12-12 21:40:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 22:07:42]

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

[commit-bot: I haz the power, 2016-12-12 22:07:43]

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

[ncarter (slow), 2016-12-12 22:44:09]

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

[commit-bot: I haz the power, 2016-12-12 22:46:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 23:45:50]

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

[commit-bot: I haz the power, 2016-12-12 23:45:51]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ncarter (slow), 2016-12-13 00:27:35]

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

[commit-bot: I haz the power, 2016-12-13 00:28:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 00:42:57]

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

[commit-bot: I haz the power, 2016-12-13 00:42:58]

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

[ncarter (slow), 2016-12-13 00:45:40]

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

[commit-bot: I haz the power, 2016-12-13 00:47:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 03:28:23]

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

[commit-bot: I haz the power, 2016-12-13 03:28:24]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2016-12-13 17:55:53]

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

[ncarter (slow), 2016-12-13 17:55:54]

Charlie, please review

[Charlie Reis, 2016-12-13 19:03:55]

Wow, this is great.  Should resolve a lot of bugs, and cleaner code as a result.

Agreed that we should move the remaining ViewMsgs in WebContents to be FrameMsgs
soon, just to eliminate any other chances for bugs.

Some minor thoughts below.

https://codereview.chromium.org/2563233002/diff/180001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2563233002/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:684: if
(observer.OnMessageReceived(message))
Presumably this should be changed or removed someday?  Seems like we might have
more of these types of bugs in here, where people assume a message came from the
current RVH.  Maybe add a TODO?  (Certainly not something to tackle in this CL.)

https://codereview.chromium.org/2563233002/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2439: // TODO(nick): |source|?
I'm with you, but maybe this TODO should be a little clearer.  Something like:
"Should this being using |source|?"

(Or did you specifically mean using |source| instead of |this|?)

https://codereview.chromium.org/2563233002/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:3479: void
WebContentsImpl::OnThemeColorChanged(RenderFrameHostImpl* source,
A bunch of these methods don't check |source| either.  Did you determine that
they're safe (as opposed to the ones above)?

Or should we have a more general bug to audit all the message handlers to make
sure they're checking |source| and not worry about the TODOs?

https://codereview.chromium.org/2563233002/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:3590: // |source|.
This seems like something we should resolve before landing.  Did you get a sense
that this was intentional?  Maybe wjmaclean@ can help if it's unclear.

https://codereview.chromium.org/2563233002/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:3825: new
ViewMsg_PpapiBrokerPermissionResult(ppb_broker_route_id, result));
Nice.  Let's include a TODO for your idea to move these away from being view
messages, given the swapped out filter concern for OOPIFs.

https://codereview.chromium.org/2563233002/diff/180001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/2563233002/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:1368: bool
Matches(RenderFrameHostImpl* render_frame_host, int id);
color_chooser_id

[ncarter (slow), 2016-12-14 00:04:42]

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

[commit-bot: I haz the power, 2016-12-14 00:05:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 03:08:40]

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

[commit-bot: I haz the power, 2016-12-14 03:08:40]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2016-12-14 18:30:59]

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

[commit-bot: I haz the power, 2016-12-14 18:31:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ncarter (slow), 2016-12-14 18:36:47]

https://codereview.chromium.org/2563233002/diff/180001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2563233002/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:684: if
(observer.OnMessageReceived(message))
On 2016/12/13 19:03:56, Charlie Reis wrote:
> Presumably this should be changed or removed someday?  Seems like we might
have
> more of these types of bugs in here, where people assume a message came from
the
> current RVH.  Maybe add a TODO?  (Certainly not something to tackle in this
CL.)

Added a TODO

https://codereview.chromium.org/2563233002/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2439: // TODO(nick): |source|?
On 2016/12/13 19:03:56, Charlie Reis wrote:
> I'm with you, but maybe this TODO should be a little clearer.  Something like:
> "Should this being using |source|?"
> 
> (Or did you specifically mean using |source| instead of |this|?)

Done.

https://codereview.chromium.org/2563233002/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:3479: void
WebContentsImpl::OnThemeColorChanged(RenderFrameHostImpl* source,
On 2016/12/13 19:03:56, Charlie Reis wrote:
> A bunch of these methods don't check |source| either.  Did you determine that
> they're safe (as opposed to the ones above)?
> 
> Or should we have a more general bug to audit all the message handlers to make
> sure they're checking |source| and not worry about the TODOs?

I was adding TODOs as I saw obvious problems, but in a lot of cases I just
wasn't deeply thinking through these cases, since the original goal of this CL
was really just a mechanical rearrangement of IPC dispatch :(

Anyhow, I went through, did more thinking, and triaged them all. Now there are
two dozen TODOs. Please let me know if you see anything I missed.

https://codereview.chromium.org/2563233002/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:3590: // |source|.
On 2016/12/13 19:03:56, Charlie Reis wrote:
> This seems like something we should resolve before landing.  Did you get a
sense
> that this was intentional?  Maybe wjmaclean@ can help if it's unclear.

I fixed up this one, but will add wjmaclean to the review.

I was trying to avoid having to think through all the consequences here.

https://codereview.chromium.org/2563233002/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:3825: new
ViewMsg_PpapiBrokerPermissionResult(ppb_broker_route_id, result));
On 2016/12/13 19:03:56, Charlie Reis wrote:
> Nice.  Let's include a TODO for your idea to move these away from being view
> messages, given the swapped out filter concern for OOPIFs.

The CL to fix this is already complete; I'll send it to jam@ as soon as this one
lands.

https://codereview.chromium.org/2563233002/diff/180001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/2563233002/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:1368: bool
Matches(RenderFrameHostImpl* render_frame_host, int id);
On 2016/12/13 19:03:56, Charlie Reis wrote:
> color_chooser_id

Done.

[ncarter (slow), 2016-12-14 18:40:10]

nick@chromium.org changed reviewers:
+ estark@chromium.org, kouhei@chromium.org, wjmaclean@chromium.org

[ncarter (slow), 2016-12-14 18:40:12]

estark: please review the mixed-content related parts of web_contents_impl.cc
(OnDidDisplayInsecureContent, OnDidRunInsecureContent,
OnDidDisplayContentWithCertificateErrors, OnDidRunContentWithCertificateErrors)

kouhei: please look at WebContentsImpl::OnUpdatePageImportanceSignals

wjmaclean: please look at WebContentsImpl::OnPageScaleFactorChanged. Is it
possible this breaks something?

[wjmaclean, 2016-12-14 18:50:34]

On 2016/12/14 18:40:12, ncarter wrote:
> 
> wjmaclean: please look at WebContentsImpl::OnPageScaleFactorChanged. Is it
> possible this breaks something?

Offhand it looks like this should be ok ... existing tests should catch it if it
isn't.

Easy manual test: linux box with two monitors at different DSF, and drag a
window with oopifs between the monitors ... I can try that out tomorrow if
you're concerned.

lgtm for WebContentsImpl::OnPageScaleFactorChanged()

[commit-bot: I haz the power, 2016-12-14 19:46:04]

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

[commit-bot: I haz the power, 2016-12-14 19:46:05]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2016-12-14 21:10:01]

On 2016/12/14 18:50:34, wjmaclean wrote:
> On 2016/12/14 18:40:12, ncarter wrote:
> > 
> > wjmaclean: please look at WebContentsImpl::OnPageScaleFactorChanged. Is it
> > possible this breaks something?
> 
> Offhand it looks like this should be ok ... existing tests should catch it if
it
> isn't.
> 
> Easy manual test: linux box with two monitors at different DSF, and drag a
> window with oopifs between the monitors ... I can try that out tomorrow if
> you're concerned.
> 
> lgtm for WebContentsImpl::OnPageScaleFactorChanged()

Here's my best understanding:
 - GetRenderViewHost() and |source| should be identical unless we're in a corner
case scenario like: the zoom factor changes during an unload handler for a
cross-process navigation. Not sure if that's even possible.
 - However, given the choice, using |source| is almost always the right call
from a security perspective.
 - I don't think we can get here for a swapped-out RVH, because of the filtering
in swapped_out_messages.cc.
 - We should probably port this to RFH and restrict it to the current main
frame?

[kouhei (in TOK), 2016-12-15 00:01:38]

lgtm

https://codereview.chromium.org/2563233002/diff/220001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2563233002/diff/220001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:3666: // sent for the main
frame?
You are right. This won't work for oopifs.

[estark, 2016-12-15 00:40:31]

The mixed content methods in web_contents_impl.cc lgtm with the note below.

https://codereview.chromium.org/2563233002/diff/220001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2563233002/diff/220001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:3534: // replace them with
|source|'s last-committed URL and origin?
It looks like |target_url| isn't used at all except for the log message, so we
can probably just remove that argument entirely.

However, |security_origin| is not necessarily the origin of |source|'s
last-committed URL. If https://a.com loads https://b.com in a frame which loads
http://c.com/script.js, the b.com frame will send the OnDidRunInsecureContent
message, but |security_origin| will be a.com because it's a.com that needs to be
tainted and drawn with a downgraded security indicator in the omnibox. (Really
we should be tainting both a.com and b.com, but that's a separate issue -- the
point is that the inner frame observes that the content is mixed and sends the
IPC, but the topmost frame is the one for which the origin needs to be
downgraded in the omnibox.)

[Charlie Reis, 2016-12-15 00:50:09]

Thanks!  LGTM.

https://codereview.chromium.org/2563233002/diff/220001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2563233002/diff/220001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:3723: // TODO(nick): Should we
consider |source| here?
Also FilterURL for manifest_url.  (I agree with your TODOs for FilterURL in
general-- I'm pretty sure the answer is yes for all of them.)  :)

[ncarter (slow), 2016-12-15 22:12:36]

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

[commit-bot: I haz the power, 2016-12-15 22:13:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ncarter (slow), 2016-12-15 23:29:04]

The CQ bit was unchecked by nick@chromium.org

[ncarter (slow), 2016-12-15 23:29:05]

The CQ bit was checked by nick@chromium.org

[ncarter (slow), 2016-12-15 23:29:05]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
wjmaclean@chromium.org, kouhei@chromium.org, estark@chromium.org
Link to the patchset: https://codereview.chromium.org/2563233002/#ps240001
(title: "Fix comments.")

[commit-bot: I haz the power, 2016-12-15 23:30:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 23:59:28]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1481844545290250,
"parent_rev": "e60ef2686366a41832cb656bde5d5a53437cfaea", "commit_rev":
"181f1f42d27f121c390e7a2b0543c3e095d07bd2"}

[commit-bot: I haz the power, 2016-12-15 23:59:59]

Description was changed from

==========
Plumb RenderFrameHostImpl* or RenderViewHostImpl* to every message
handler as an argument, rather than passing them as data members.

Eliminate the render_frame_message_source_, render_view_message_source_
data members. Eliminate the HasValidFrameSource() helper method.

Eliminate the combo version of OnMessageReceived that took both an RFH
or an RVH, and instead do the dispatch in the delegate
OnMessageRecieved methods. Use the _WITH_PARAM dispatch macro to
plumb the RenderFrameHostImpl into the dispatch methods.

Fix some bugs that became apparent after this refactoring:
  OnEndColorChooser: insist on rfh/rph match (via a ::Matches helper)
  OnSetSelectedColorInColorChooser: insist on rfh/rph match
  OnOpenDateTimeDialog: use |source| rather than GetRenderViewHost.
  OnEnumerateDirectory: Use the right process for permissions check
  OnRequestPpapiBrokerPermission: rename |routing_id| to reflect that
    it's not a frame or view route ID.
  SendPpapiBrokerPermissionResult: rename this from On... to
    Send..., since it's not a dispatcher. Parameterize by the
    RPH id, so that we send the response to the right process.

BUG=304341
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Plumb RenderFrameHostImpl* or RenderViewHostImpl* to every message
handler as an argument, rather than passing them as data members.

Eliminate the render_frame_message_source_, render_view_message_source_
data members. Eliminate the HasValidFrameSource() helper method.

Eliminate the combo version of OnMessageReceived that took both an RFH
or an RVH, and instead do the dispatch in the delegate
OnMessageRecieved methods. Use the _WITH_PARAM dispatch macro to
plumb the RenderFrameHostImpl into the dispatch methods.

Fix some bugs that became apparent after this refactoring:
  OnEndColorChooser: insist on rfh/rph match (via a ::Matches helper)
  OnSetSelectedColorInColorChooser: insist on rfh/rph match
  OnOpenDateTimeDialog: use |source| rather than GetRenderViewHost.
  OnEnumerateDirectory: Use the right process for permissions check
  OnRequestPpapiBrokerPermission: rename |routing_id| to reflect that
    it's not a frame or view route ID.
  SendPpapiBrokerPermissionResult: rename this from On... to
    Send..., since it's not a dispatcher. Parameterize by the
    RPH id, so that we send the response to the right process.

BUG=304341
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2563233002
==========

[commit-bot: I haz the power, 2016-12-16 00:00:02]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2016-12-16 00:02:37]

Description was changed from

==========
Plumb RenderFrameHostImpl* or RenderViewHostImpl* to every message
handler as an argument, rather than passing them as data members.

Eliminate the render_frame_message_source_, render_view_message_source_
data members. Eliminate the HasValidFrameSource() helper method.

Eliminate the combo version of OnMessageReceived that took both an RFH
or an RVH, and instead do the dispatch in the delegate
OnMessageRecieved methods. Use the _WITH_PARAM dispatch macro to
plumb the RenderFrameHostImpl into the dispatch methods.

Fix some bugs that became apparent after this refactoring:
  OnEndColorChooser: insist on rfh/rph match (via a ::Matches helper)
  OnSetSelectedColorInColorChooser: insist on rfh/rph match
  OnOpenDateTimeDialog: use |source| rather than GetRenderViewHost.
  OnEnumerateDirectory: Use the right process for permissions check
  OnRequestPpapiBrokerPermission: rename |routing_id| to reflect that
    it's not a frame or view route ID.
  SendPpapiBrokerPermissionResult: rename this from On... to
    Send..., since it's not a dispatcher. Parameterize by the
    RPH id, so that we send the response to the right process.

BUG=304341
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2563233002
==========

to

==========
Plumb RenderFrameHostImpl* or RenderViewHostImpl* to every message
handler as an argument, rather than passing them as data members.

Eliminate the render_frame_message_source_, render_view_message_source_
data members. Eliminate the HasValidFrameSource() helper method.

Eliminate the combo version of OnMessageReceived that took both an RFH
or an RVH, and instead do the dispatch in the delegate
OnMessageRecieved methods. Use the _WITH_PARAM dispatch macro to
plumb the RenderFrameHostImpl into the dispatch methods.

Fix some bugs that became apparent after this refactoring:
  OnEndColorChooser: insist on rfh/rph match (via a ::Matches helper)
  OnSetSelectedColorInColorChooser: insist on rfh/rph match
  OnOpenDateTimeDialog: use |source| rather than GetRenderViewHost.
  OnEnumerateDirectory: Use the right process for permissions check
  OnRequestPpapiBrokerPermission: rename |routing_id| to reflect that
    it's not a frame or view route ID.
  SendPpapiBrokerPermissionResult: rename this from On... to
    Send..., since it's not a dispatcher. Parameterize by the
    RPH id, so that we send the response to the right process.

BUG=304341
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2016-12-16 00:02:38]

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