Make the ChromeNetworkDelegate use the ExtensionEventRouterForwarder

Make the ChromeNetworkDelegate use the ExtensionEventRouterForwarder

BUG=73903
TEST=tests for proxy and webrequest API should still work

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=76372

[jochen (gone - plz use gerrit), 2011-02-25 11:47:23]

please review

[Matt Perry, 2011-02-25 23:28:40]

LGTM, but please wait for any comments from Will.

http://codereview.chromium.org/6598002/diff/1/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (left):

http://codereview.chromium.org/6598002/diff/1/chrome/browser/io_thread.cc#old...
chrome/browser/io_thread.cc:182: class SystemNetworkDelegate : public
net::NetworkDelegate {
I think Will had other plans for this beyond making it a ChromeNetworkDelegate
without a profile. For example, with your change, ChromeNetworkDelegate can no
longer assume that any URLRequests it sees have a ChromeURLRequestContext.

But I'll let Will comment further.

[willchan no longer on Chromium, 2011-02-26 02:11:35]

http://codereview.chromium.org/6598002/diff/1/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (left):

http://codereview.chromium.org/6598002/diff/1/chrome/browser/io_thread.cc#old...
chrome/browser/io_thread.cc:182: class SystemNetworkDelegate : public
net::NetworkDelegate {
On 2011/02/25 23:28:40, Matt Perry wrote:
> I think Will had other plans for this beyond making it a ChromeNetworkDelegate
> without a profile. For example, with your change, ChromeNetworkDelegate can no
> longer assume that any URLRequests it sees have a ChromeURLRequestContext.
> 
> But I'll let Will comment further.

I actually forgot to mention this to jochen when he spoke to me about it
earlier, but yes, mpcomplete is right. But it's fine to delete this. I'll
resurrect it eventually. When that happens, I may end up creating some sort of
ExtensionNetworkDelegate object or something that will be shared by both
ChromeNetworkDelegate and SystemNetworkDelegate, where all the existing logic
(since all of it is strictly extension stuff right now) will move.

Note that eventually we will shim in other things, like NetLog, and DevTools, a
replacement for URLRequest::Interceptor/URLRequest::ProtocolFactory (both are
globals) / etc. Interceptors/ProtocolFactories often need to know that they're
using a ChromeURLRequestContext, and some won't need to be included in the
SystemNetworkDelegate.

http://codereview.chromium.org/6598002/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/6598002/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_io_data.h:114: Profile* profile;
jochen, I think I told you this was ok before, but I am concerned about this
Profile*, especially since it's stored in ChromeNetworkDelegate where you don't
have a comment like this. Also, people never read comments. I think there's
probably something we could do where the global event forwarder could maintain a
mapping of Profile*<=>Foo, where Foo can be passed in ProfileParams and used by
the ChromeNetworkDelegate when calling back into the event forwarder.

I'm curious what ericroman thinks here. I suspect he shares my concern with
exposing the Profile* on the IO thread.

[jochen (gone - plz use gerrit), 2011-02-28 10:18:40]

updated the CL to pass around a ProfileId instead of a Profile*

[willchan no longer on Chromium, 2011-02-28 21:48:40]

LGTM, thanks!

http://codereview.chromium.org/6598002/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_event_router_forwarder.cc (right):

http://codereview.chromium.org/6598002/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_event_router_forwarder.cc:73: if (profile_id
!= Profile::InvalidProfileId && !profile)
Shouldn't we only call GetProfileWithId() if profile_id !=
Profile::kInvalidProfileId (s/InvalidProfileId/kInvalidProfileId/)?

Perhaps you should initialize |profile| to NULL, and then call
GetProfileWithId() if |profile_id| != Profile::kInvalidProfileId.

PS: OIC, InvalidProfileId already existed. Then it's ok for you to use that, but
if you feel so inclined, you could fix the naming (it doesn't look too hard).
I'd encourage (but not require) doing so, since it's not widely used yet so it
should be easy to fix.

http://codereview.chromium.org/6598002/diff/4001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/6598002/diff/4001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.cc:142: Profile*
ProfileManager::GetProfileWithId(ProfileId profile_id) {
I think it should be a bug for profile_id == kInvalidProfileId. I'd add a
DCHECK_NE.