Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(393)

Issue 2222723002: Avoid calling into the ContentBrowserClient interface from ResourceDispatcherHostImpl to determine … (Closed)

Created:
4 years, 4 months ago by ananta
Modified:
4 years, 4 months ago
Reviewers:
Charlie Reis, jam
CC:
chromium-reviews, extensions-reviews_chromium.org, Randy Smith (Not in Mondays), darin-cc_chromium.org, loading-reviews_chromium.org, chromium-apps-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Avoid calling into the ContentBrowserClient interface from ResourceDispatcherHostImpl to determine whether an origin is legal. Continuing changes to remove usage of ContentBrowserClient from ResourceDispatcherHostImpl. This patch adds a facility to the ResourceDispatcherHost class to register HTTP header based interceptors. The caller can specify a prefix for the header value it expects. This is matched in ResourceDispatcherHostImpl before invoking the caller callback. We allow callers to pass in empty strings for the header value, in which case they receive notifications for all occurrences of that HTTP header. I also removed the IsIllegalOrigin method from the ContentBrowserClient interface BUG=598073 Committed: https://crrev.com/a5721fb7ef4c4bd89e143bc5024bbd4b1e844fb6 Cr-Commit-Position: refs/heads/master@{#411114}

Patch Set 1 #

Patch Set 2 : Revert header change and remove blank line #

Patch Set 3 : Rebase to tip and git cl format #

Patch Set 4 : Register the origin interceptor after RDH is created #

Patch Set 5 : Invoke the correct callback on ChromeContentBrowserClientParts. #

Patch Set 6 : Fix compile failures #

Patch Set 7 : Fix compile failures #

Total comments: 14

Patch Set 8 : Address review comments #

Patch Set 9 : Address review comments. Change interceptor to a callback #

Patch Set 10 : Remove the extension origin interceptor class. #

Total comments: 7

Patch Set 11 : Address review comments #

Patch Set 12 : Attempt to fix 64 bit windows redness #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+348 lines, -158 lines) Patch
M chrome/browser/chrome_content_browser_client.h View 8 9 1 chunk +0 lines, -3 lines 0 comments Download
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 8 9 2 chunks +3 lines, -12 lines 0 comments Download
M chrome/browser/chrome_content_browser_client_parts.h View 1 2 3 8 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/extensions/chrome_content_browser_client_extensions_part.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +1 line, -3 lines 0 comments Download
M chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc View 1 2 3 4 5 6 7 8 9 10 4 chunks +79 lines, -46 lines 4 comments Download
M content/browser/loader/resource_dispatcher_host_impl.h View 1 2 3 4 5 6 7 8 9 10 5 chunks +63 lines, -0 lines 0 comments Download
M content/browser/loader/resource_dispatcher_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +151 lines, -66 lines 1 comment Download
M content/browser/security_exploit_browsertest.cc View 1 2 3 4 5 6 7 8 5 chunks +14 lines, -14 lines 0 comments Download
M content/public/browser/content_browser_client.h View 8 9 10 1 chunk +0 lines, -8 lines 0 comments Download
M content/public/browser/content_browser_client.cc View 9 1 chunk +0 lines, -6 lines 0 comments Download
M content/public/browser/resource_dispatcher_host.h View 1 2 3 4 5 6 7 8 9 10 3 chunks +33 lines, -0 lines 0 comments Download
M extensions/browser/bad_message.h View 1 chunk +1 line, -0 lines 1 comment Download

Messages

Total messages: 51 (31 generated)
ananta
4 years, 4 months ago (2016-08-08 21:45:42 UTC) #6
mmenke
Do we really need another RDH observer interface for this? We have delegate_->ShouldBeginRequest already. Seems ...
4 years, 4 months ago (2016-08-08 22:06:53 UTC) #9
ananta
On 2016/08/08 22:06:53, mmenke wrote: > Do we really need another RDH observer interface for ...
4 years, 4 months ago (2016-08-08 23:32:42 UTC) #10
jam
great, just some nits mostly around avoiding adding another interface if we can avoid it ...
4 years, 4 months ago (2016-08-09 01:36:24 UTC) #17
ananta
https://codereview.chromium.org/2222723002/diff/120001/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2222723002/diff/120001/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc#newcode320 chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:320: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2016/08/09 01:36:24, jam wrote: > no need ...
4 years, 4 months ago (2016-08-09 05:47:48 UTC) #22
jam
https://codereview.chromium.org/2222723002/diff/120001/content/public/browser/resource_dispatcher_host.h File content/public/browser/resource_dispatcher_host.h (right): https://codereview.chromium.org/2222723002/diff/120001/content/public/browser/resource_dispatcher_host.h#newcode71 content/public/browser/resource_dispatcher_host.h:71: virtual void UnregisterInterceptor( On 2016/08/09 05:47:48, ananta wrote: > ...
4 years, 4 months ago (2016-08-09 19:58:41 UTC) #23
ananta
https://codereview.chromium.org/2222723002/diff/120001/content/public/browser/resource_dispatcher_host.h File content/public/browser/resource_dispatcher_host.h (right): https://codereview.chromium.org/2222723002/diff/120001/content/public/browser/resource_dispatcher_host.h#newcode71 content/public/browser/resource_dispatcher_host.h:71: virtual void UnregisterInterceptor( On 2016/08/09 19:58:41, jam wrote: > ...
4 years, 4 months ago (2016-08-09 23:35:19 UTC) #25
jam
https://codereview.chromium.org/2222723002/diff/120001/extensions/browser/bad_message.h File extensions/browser/bad_message.h (right): https://codereview.chromium.org/2222723002/diff/120001/extensions/browser/bad_message.h#newcode31 extensions/browser/bad_message.h:31: INVALID_ORIGIN = 7, On 2016/08/09 05:47:48, ananta wrote: > ...
4 years, 4 months ago (2016-08-10 00:19:26 UTC) #27
ananta
https://codereview.chromium.org/2222723002/diff/120001/extensions/browser/bad_message.h File extensions/browser/bad_message.h (right): https://codereview.chromium.org/2222723002/diff/120001/extensions/browser/bad_message.h#newcode31 extensions/browser/bad_message.h:31: INVALID_ORIGIN = 7, On 2016/08/10 00:19:26, jam wrote: > ...
4 years, 4 months ago (2016-08-10 00:30:03 UTC) #28
jam
lgtm
4 years, 4 months ago (2016-08-10 01:51:57 UTC) #32
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/2222723002/220001
4 years, 4 months ago (2016-08-10 19:08:19 UTC) #39
commit-bot: I haz the power
Committed patchset #12 (id:220001)
4 years, 4 months ago (2016-08-10 19:13:05 UTC) #41
commit-bot: I haz the power
Patchset 12 (id:??) landed as https://crrev.com/a5721fb7ef4c4bd89e143bc5024bbd4b1e844fb6 Cr-Commit-Position: refs/heads/master@{#411114}
4 years, 4 months ago (2016-08-10 19:15:10 UTC) #43
Charlie Reis
Sorry, please revert ASAP. We'll get corrupted data in the crash reports with this in ...
4 years, 4 months ago (2016-08-10 20:44:02 UTC) #45
jam
Instead of reverting just because of that one line, it's more lightweight to change the ...
4 years, 4 months ago (2016-08-10 20:52:16 UTC) #46
Charlie Reis
On 2016/08/10 20:52:16, jam wrote: > Instead of reverting just because of that one line, ...
4 years, 4 months ago (2016-08-10 20:58:51 UTC) #47
Charlie Reis
On 2016/08/10 20:58:51, Charlie Reis wrote: > On 2016/08/10 20:52:16, jam wrote: > > Instead ...
4 years, 4 months ago (2016-08-10 21:03:26 UTC) #48
Charlie Reis
On 2016/08/10 21:03:26, Charlie Reis wrote: > On 2016/08/10 20:58:51, Charlie Reis wrote: > > ...
4 years, 4 months ago (2016-08-10 21:10:26 UTC) #49
mmenke
On 2016/08/08 23:32:42, ananta wrote: > On 2016/08/08 22:06:53, mmenke wrote: > > Do we ...
4 years, 4 months ago (2016-08-11 20:09:39 UTC) #50
ananta
4 years, 4 months ago (2016-08-11 20:11:32 UTC) #51
Message was sent while issue was closed.
On 2016/08/11 20:09:39, mmenke wrote:
> On 2016/08/08 23:32:42, ananta wrote:
> > On 2016/08/08 22:06:53, mmenke wrote:
> > > Do we really need another RDH observer interface for this?  We have
> > > delegate_->ShouldBeginRequest already.  Seems like we could just piggyback
> > these
> > > call on that.
> > 
> > We wanted to avoid a delegate like pattern given that RDH and most of the
code
> > in content\browser\loader will eventually be
> > moved into a network service.
> 
> Still seems really problematic to me.  Oh well.  Would have followed up on
this,
> but I somehow ended up off of the CC list (Maybe commenting on the issue
removed
> me from it?  Not sure).

Please send in your comments anyways. Will try to address your concerns in a
followup.

Powered by Google App Engine
This is Rietveld 408576698