Handle extension webrequest API on the IO thread. This speeds up blocking event

Handle extension webrequest API on the IO thread. This speeds up blocking event
dispatch and handling.

BUG=no
TEST=should not change functionality

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

[Matt Perry, 2011-06-07 00:13:12]

There's a few main changes here:
- IOThreadExtensionFunction, used by the webrequest functions to handle the
extension request without going to the UI thread.
- a new IPC message so we know when to make an IOThreadExtensionFunction.
- Changes to the ExtensionWebRequestEventRouter to dispatch the events without
going to the UI thread. This required some extra bookkeeping, like knowing the
profile/OTRprofile relationships.
- ExtensionInfoMap now caches some extension prefs on the IO thread.

[Matt Perry, 2011-06-07 21:51:58]

Dominic: FYI

[Mihai Parparita -not on Chrome, 2011-06-08 01:11:58]

Someone from chrome/browser/net/OWNERS needs to look at this too.

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_function.h (right):

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_function.h:328: ~AsyncExtensionFunction();
Why is this no longer virtual?

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_function_dispatcher.h (right):

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_function_dispatcher.h:117: // Does not set
all properties - particularly, RenderViewHost, Profile, and
RenderViewHost, Profile and dispatcher are all
UIThreadExtensionFunction-specific. Maybe the comment would be clearer if it
said that it doesn't set any ExtensionFunction subclass-specific properties?

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_webrequest_api.cc (right):

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webrequest_api.cc:349:
GetMatchingListeners(extension_info_map, profile_id,
All of the functions in ExtensionWebRequestEventRouter take a profile_id and
then the extension_info_map, but GetMatchingListeners has them reversed. Mind
making it consistent?

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile.cc (right):

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.cc:84: static void
NotifyOTRProfileCreatedOnIOThread(ProfileId original_profile_id,
I thought that using an anonymous namespace was equivalent to the static
keyword, so you wouldn't need it here.

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.cc:243: profile_->GetRuntimeId(),
reinterpret_cast<ProfileId>(this)));
Use GetRuntimeId instead of the cast? I think that should work even though
GetRuntimeId is virtual since it's defined in this class.

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.cc:257: profile_->GetRuntimeId(),
reinterpret_cast<ProfileId>(this)));
Ditto.

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:913: if (extension->location() !=
Extension::COMPONENT) {
Is this check really necessary? ExtensionPrefs::GetInstallTime makes it sound
like it'll return the default time if the actual install time can't be
determined.

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right):

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:221: // Makes sure
that async task ExtensionPrefs::OnExtensionInstalled gets a
Isn't OnExtensionInstalled invoked directly/synchronously by
OnLoadSingleExtension now, so this this is no longer necessary? (I'm also
unclear why AddExtension was changed to OnLoadSingleExtension).

http://codereview.chromium.org/7024056/diff/5001/chrome/renderer/extensions/e...
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/7024056/diff/5001/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_process_bindings.cc:468: static
v8::Handle<v8::Value> StartRequestForIOThread(
This seems very similar to StartRequest. Can the for_io_thread bool just be a JS
parameter to StartRequest?

http://codereview.chromium.org/7024056/diff/5001/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_process_bindings.cc:478: NOTREACHED() <<
"Invalid JSON passed to StartRequest.";
If you do keep this function, the message here should be updated to say
StartRequestForIOThread.

http://codereview.chromium.org/7024056/diff/5001/chrome/renderer/resources/ex...
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/7024056/diff/5001/chrome/renderer/resources/ex...
chrome/renderer/resources/extension_process_bindings.js:189: function
sendRequest(functionName, args, argSchemas, opt_customCallback,
The callsites of this are confusing to follow because of all the optional
arguments. Perhaps have a sendRequestOnIOThread wrapper (and sendIconRequest for
the other usecase)?

[Matt Perry, 2011-06-08 20:46:59]

Dominic, adding you to reviewers list for chrome/browsers/net approval.

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_function.h (right):

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_function.h:328: ~AsyncExtensionFunction();
On 2011/06/08 01:11:59, Mihai Parparita wrote:
> Why is this no longer virtual?

oops

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_function_dispatcher.h (right):

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_function_dispatcher.h:117: // Does not set
all properties - particularly, RenderViewHost, Profile, and
On 2011/06/08 01:11:59, Mihai Parparita wrote:
> RenderViewHost, Profile and dispatcher are all
> UIThreadExtensionFunction-specific. Maybe the comment would be clearer if it
> said that it doesn't set any ExtensionFunction subclass-specific properties?

Done.

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile.cc (right):

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.cc:84: static void
NotifyOTRProfileCreatedOnIOThread(ProfileId original_profile_id,
On 2011/06/08 01:11:59, Mihai Parparita wrote:
> I thought that using an anonymous namespace was equivalent to the static
> keyword, so you wouldn't need it here.

Done.

(There's a slight difference, but it doesn't matter here. See
http://groups.google.com/group/comp.lang.c++.moderated/browse_thread/thread/6...
for an explanation.)

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.cc:243: profile_->GetRuntimeId(),
reinterpret_cast<ProfileId>(this)));
On 2011/06/08 01:11:59, Mihai Parparita wrote:
> Use GetRuntimeId instead of the cast? I think that should work even though
> GetRuntimeId is virtual since it's defined in this class.

Done.

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.cc:257: profile_->GetRuntimeId(),
reinterpret_cast<ProfileId>(this)));
On 2011/06/08 01:11:59, Mihai Parparita wrote:
> Ditto.

Done.

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:913: if (extension->location() !=
Extension::COMPONENT) {
On 2011/06/08 01:11:59, Mihai Parparita wrote:
> Is this check really necessary? ExtensionPrefs::GetInstallTime makes it sound
> like it'll return the default time if the actual install time can't be
> determined.

GetInstallTime asserts that the ExtensionPref exists, which is not the case for
component extensions. I was wary of removing that assert since the function was
probably not meant to be used with component extensions.

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right):

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:221: // Makes sure
that async task ExtensionPrefs::OnExtensionInstalled gets a
On 2011/06/08 01:11:59, Mihai Parparita wrote:
> Isn't OnExtensionInstalled invoked directly/synchronously by
> OnLoadSingleExtension now, so this this is no longer necessary? (I'm also
> unclear why AddExtension was changed to OnLoadSingleExtension).

I changed it to OnLoadSingleExtension so that it would get registered with
ExtensionPrefs (and have an InstallTime).

http://codereview.chromium.org/7024056/diff/5001/chrome/renderer/extensions/e...
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/7024056/diff/5001/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_process_bindings.cc:468: static
v8::Handle<v8::Value> StartRequestForIOThread(
On 2011/06/08 01:11:59, Mihai Parparita wrote:
> This seems very similar to StartRequest. Can the for_io_thread bool just be a
JS
> parameter to StartRequest?

Done.

http://codereview.chromium.org/7024056/diff/5001/chrome/renderer/resources/ex...
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/7024056/diff/5001/chrome/renderer/resources/ex...
chrome/renderer/resources/extension_process_bindings.js:189: function
sendRequest(functionName, args, argSchemas, opt_customCallback,
On 2011/06/08 01:11:59, Mihai Parparita wrote:
> The callsites of this are confusing to follow because of all the optional
> arguments. Perhaps have a sendRequestOnIOThread wrapper (and sendIconRequest
for
> the other usecase)?

I've changed the optional arguments to a single optional object with named args.
Does that clarify it enough?

[battre, 2011-06-08 22:26:04]

LGTM for chrome/browsers/net

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/7024056/diff/5001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:913: if (extension->location() !=
Extension::COMPONENT) {
On 2011/06/08 20:47:00, Matt Perry wrote:
> On 2011/06/08 01:11:59, Mihai Parparita wrote:
> > Is this check really necessary? ExtensionPrefs::GetInstallTime makes it
sound
> > like it'll return the default time if the actual install time can't be
> > determined.
> 
> GetInstallTime asserts that the ExtensionPref exists, which is not the case
for
> component extensions. I was wary of removing that assert since the function
was
> probably not meant to be used with component extensions.

I have introduced GetInstallTime to get a deterministic order for extensions.

The "if (!extension) { NOTREACHED(); ... }" in GetInstallTime was introduced to
be sure that the installation order is actually deterministic and that we do not
accidentally call the function without noticing that the return values are
non-deterministic.

http://codereview.chromium.org/7024056/diff/6025/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_function_dispatcher.h (right):

http://codereview.chromium.org/7024056/diff/6025/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_function_dispatcher.h:115: // Helper to
creates an ExtensionFunction to handle the function given by
nit: typo "Helper to creates"

http://codereview.chromium.org/7024056/diff/6025/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_webrequest_api.h (right):

http://codereview.chromium.org/7024056/diff/6025/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webrequest_api.h:239: // of the event, the
next for the cross profile (incognito if event is for
nit: "the next *time*"

http://codereview.chromium.org/7024056/diff/6025/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webrequest_api.h:240: // normal profile, or
vice versa).
I don't understand the sentence in the parenthesis.

[Mihai Parparita -not on Chrome, 2011-06-09 18:37:24]

LGTM

http://codereview.chromium.org/7024056/diff/6025/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_webrequest_api.cc (right):

http://codereview.chromium.org/7024056/diff/6025/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webrequest_api.cc:349:
GetMatchingListeners(extension_info_map, profile_id,
Ping on this comment: "All of the functions in ExtensionWebRequestEventRouter
take a profile_id and then the extension_info_map, but GetMatchingListeners has
them reversed. Mind making it consistent?"

http://codereview.chromium.org/7024056/diff/6025/chrome/renderer/resources/ex...
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/7024056/diff/6025/chrome/renderer/resources/ex...
chrome/renderer/resources/extension_process_bindings.js:187: // - rawArgs: true
if we should not stringify the request arguments.
noStringify or another boolean-like name seems better here.

[Matt Perry, 2011-06-09 18:50:29]

http://codereview.chromium.org/7024056/diff/6025/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_webrequest_api.cc (right):

http://codereview.chromium.org/7024056/diff/6025/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webrequest_api.cc:349:
GetMatchingListeners(extension_info_map, profile_id,
On 2011/06/09 18:37:24, Mihai Parparita wrote:
> Ping on this comment: "All of the functions in ExtensionWebRequestEventRouter
> take a profile_id and then the extension_info_map, but GetMatchingListeners
has
> them reversed. Mind making it consistent?"

Sorry, I missed that comment. Done.

http://codereview.chromium.org/7024056/diff/6025/chrome/renderer/resources/ex...
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/7024056/diff/6025/chrome/renderer/resources/ex...
chrome/renderer/resources/extension_process_bindings.js:187: // - rawArgs: true
if we should not stringify the request arguments.
On 2011/06/09 18:37:24, Mihai Parparita wrote:
> noStringify or another boolean-like name seems better here.

Done.