|
|
Created:
4 years ago by ncarter (slow) Modified:
4 years ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, site-isolation-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlumb 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}
Patch Set 1 #Patch Set 2 : WebContentsImpl::OnMessageReceived always dispatch with RFH/RVH param. #Patch Set 3 : Fix things. #Patch Set 4 : OnGoToEntryAtOffset #Patch Set 5 : Fix mac compile. #Patch Set 6 : Unittest compile fix. #Patch Set 7 : Fix unittest. #Patch Set 8 : Fix unittest. #Patch Set 9 : Fix typo #
Total comments: 12
Patch Set 10 : Charlie's fixes. #Patch Set 11 : More TODOs #
Total comments: 3
Patch Set 12 : Fix comments. #Messages
Total messages: 67 (55 generated)
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 ==========
The CQ bit was checked by nick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by nick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
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 ==========
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 ==========
The CQ bit was checked by nick@chromium.org to run a CQ dry run
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 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by nick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by nick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by nick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by nick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by nick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nick@chromium.org changed reviewers: + creis@chromium.org
Charlie, please review
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
The CQ bit was checked by nick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
nick@chromium.org changed reviewers: + estark@chromium.org, kouhei@chromium.org, wjmaclean@chromium.org
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?
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()
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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?
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.
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.)
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.) :)
The CQ bit was checked by nick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by nick@chromium.org
The CQ bit was checked by nick@chromium.org
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.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 240001, "attempt_start_ts": 1481844545290250, "parent_rev": "e60ef2686366a41832cb656bde5d5a53437cfaea", "commit_rev": "181f1f42d27f121c390e7a2b0543c3e095d07bd2"}
Message was sent while issue was closed.
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 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:240001)
Message was sent while issue was closed.
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} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/a0ac8380d62de7427ec5e0bc3d7e871618497876 Cr-Commit-Position: refs/heads/master@{#438960} |