Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(8)

Issue 2563233002: WebContentsImpl: IPC_BEGIN_MESSAGE_MAP_WITH_PARAM everywhere (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months, 2 weeks ago by ncarter (slow)
Modified:
6 months, 1 week 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.

Description

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}

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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+305 lines, -251 lines) Patch
M content/browser/frame_host/interstitial_page_impl.h View 1 2 3 4 3 chunks +4 lines, -3 lines 0 comments Download
M content/browser/frame_host/interstitial_page_impl.cc View 1 2 3 4 4 chunks +9 lines, -5 lines 0 comments Download
M content/browser/frame_host/navigation_controller_impl_unittest.cc View 1 2 3 4 5 3 chunks +3 lines, -3 lines 0 comments Download
M content/browser/frame_host/render_frame_host_delegate.h View 1 2 chunks +2 lines, -1 line 0 comments Download
M content/browser/frame_host/render_frame_host_delegate.cc View 1 chunk +1 line, -1 line 0 comments Download
M content/browser/frame_host/render_frame_host_manager_unittest.cc View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M content/browser/renderer_host/render_view_host_delegate.h View 1 2 chunks +2 lines, -1 line 0 comments Download
M content/browser/renderer_host/render_view_host_delegate.cc View 1 chunk +3 lines, -2 lines 0 comments Download
M content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc View 1 2 1 chunk +1 line, -2 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.h View 1 2 3 4 5 6 7 8 9 5 chunks +75 lines, -54 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 22 chunks +201 lines, -173 lines 0 comments Download
M content/browser/web_contents/web_contents_impl_unittest.cc View 1 2 3 4 5 6 7 8 2 chunks +3 lines, -5 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 67 (55 generated)
ncarter (slow)
Charlie, please review
6 months, 1 week ago (2016-12-13 17:55:54 UTC) #39
Charlie Reis (slow)
Wow, this is great. Should resolve a lot of bugs, and cleaner code as a ...
6 months, 1 week ago (2016-12-13 19:03:55 UTC) #40
ncarter (slow)
https://codereview.chromium.org/2563233002/diff/180001/content/browser/web_contents/web_contents_impl.cc File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2563233002/diff/180001/content/browser/web_contents/web_contents_impl.cc#newcode684 content/browser/web_contents/web_contents_impl.cc:684: if (observer.OnMessageReceived(message)) On 2016/12/13 19:03:56, Charlie Reis wrote: > ...
6 months, 1 week ago (2016-12-14 18:36:47 UTC) #47
ncarter (slow)
estark: please review the mixed-content related parts of web_contents_impl.cc (OnDidDisplayInsecureContent, OnDidRunInsecureContent, OnDidDisplayContentWithCertificateErrors, OnDidRunContentWithCertificateErrors) kouhei: please ...
6 months, 1 week ago (2016-12-14 18:40:12 UTC) #49
wjmaclean
On 2016/12/14 18:40:12, ncarter wrote: > > wjmaclean: please look at WebContentsImpl::OnPageScaleFactorChanged. Is it > ...
6 months, 1 week ago (2016-12-14 18:50:34 UTC) #50
ncarter (slow)
On 2016/12/14 18:50:34, wjmaclean wrote: > On 2016/12/14 18:40:12, ncarter wrote: > > > > ...
6 months, 1 week ago (2016-12-14 21:10:01 UTC) #53
kouhei (in TOK)
lgtm https://codereview.chromium.org/2563233002/diff/220001/content/browser/web_contents/web_contents_impl.cc File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2563233002/diff/220001/content/browser/web_contents/web_contents_impl.cc#newcode3666 content/browser/web_contents/web_contents_impl.cc:3666: // sent for the main frame? You are ...
6 months, 1 week ago (2016-12-15 00:01:38 UTC) #54
estark
The mixed content methods in web_contents_impl.cc lgtm with the note below. https://codereview.chromium.org/2563233002/diff/220001/content/browser/web_contents/web_contents_impl.cc File content/browser/web_contents/web_contents_impl.cc (right): ...
6 months, 1 week ago (2016-12-15 00:40:31 UTC) #55
Charlie Reis (slow)
Thanks! LGTM. https://codereview.chromium.org/2563233002/diff/220001/content/browser/web_contents/web_contents_impl.cc File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2563233002/diff/220001/content/browser/web_contents/web_contents_impl.cc#newcode3723 content/browser/web_contents/web_contents_impl.cc:3723: // TODO(nick): Should we consider |source| here? ...
6 months, 1 week ago (2016-12-15 00:50:09 UTC) #56
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2563233002/240001
6 months, 1 week ago (2016-12-15 23:30:21 UTC) #62
commit-bot: I haz the power
Committed patchset #12 (id:240001)
6 months, 1 week ago (2016-12-16 00:00:02 UTC) #65
commit-bot: I haz the power
6 months, 1 week ago (2016-12-16 00:02:38 UTC) #67
Message was sent while issue was closed.
Patchset 12 (id:??) landed as
https://crrev.com/a0ac8380d62de7427ec5e0bc3d7e871618497876
Cr-Commit-Position: refs/heads/master@{#438960}
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318