Refactor extension event distribution to use Values instead of JSON strings.

Refactor extension event distribution to use Values instead of JSON strings.

Also renames Event.dispatchJSON to Event.dispatch and removes all JSON
serialization surrounding invocations of DispatchEvent variants.

BUG=136045
TEST=none


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

[bryeung, 2012-07-09 15:12:08]

Just nits: this looks like a great change.

https://chromiumcodereview.appspot.com/10694085/diff/8002/chrome/browser/exte...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://chromiumcodereview.appspot.com/10694085/diff/8002/chrome/browser/exte...
chrome/browser/extensions/api/downloads/downloads_api.cc:1085: base::ListValue
*args = new base::ListValue();
nit: s/ \*/\* /

https://chromiumcodereview.appspot.com/10694085/diff/8002/chrome/browser/exte...
File chrome/browser/extensions/extension_event_router.cc (right):

https://chromiumcodereview.appspot.com/10694085/diff/8002/chrome/browser/exte...
chrome/browser/extensions/extension_event_router.cc:97: // TODO(gdk): Reduce
number of DeepCopy() calls throughout the event dispatch
should this get removed now?

https://chromiumcodereview.appspot.com/10694085/diff/8002/chrome/browser/exte...
chrome/browser/extensions/extension_event_router.cc:349: if (event_args) {
nit: no braces

https://chromiumcodereview.appspot.com/10694085/diff/8002/chrome/browser/exte...
File chrome/browser/extensions/extension_event_router.h (right):

https://chromiumcodereview.appspot.com/10694085/diff/8002/chrome/browser/exte...
chrome/browser/extensions/extension_event_router.h:45: // any thread.
It would be nice to update the comments here (and below) to document memory
ownership.

https://chromiumcodereview.appspot.com/10694085/diff/8002/chrome/browser/exte...
File chrome/browser/extensions/settings/settings_frontend.cc (right):

https://chromiumcodereview.appspot.com/10694085/diff/8002/chrome/browser/exte...
chrome/browser/extensions/settings/settings_frontend.cc:44: // string-based
event payloads is removed.
nit: Would be nice to include the bug for the refactoring here.

https://chromiumcodereview.appspot.com/10694085/diff/8002/chrome/browser/spee...
File chrome/browser/speech/speech_input_extension_manager.cc (left):

https://chromiumcodereview.appspot.com/10694085/diff/8002/chrome/browser/spee...
chrome/browser/speech/speech_input_extension_manager.cc:307: VLOG(1) <<
"Results: " << json_args;
Is the speech input team using this VLOG?

[Garret Kelly, 2012-07-09 15:32:28]

@rsleevi, would you mind taking a look at the speech_input_extension_manager.cc
change to see if that VLOG is something you'd be willing to live without?

https://chromiumcodereview.appspot.com/10694085/diff/8002/chrome/browser/exte...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://chromiumcodereview.appspot.com/10694085/diff/8002/chrome/browser/exte...
chrome/browser/extensions/api/downloads/downloads_api.cc:1085: base::ListValue
*args = new base::ListValue();
On 2012/07/09 15:12:08, bryeung wrote:
> nit: s/ \*/\* /

Done.

https://chromiumcodereview.appspot.com/10694085/diff/8002/chrome/browser/exte...
File chrome/browser/extensions/extension_event_router.cc (right):

https://chromiumcodereview.appspot.com/10694085/diff/8002/chrome/browser/exte...
chrome/browser/extensions/extension_event_router.cc:97: // TODO(gdk): Reduce
number of DeepCopy() calls throughout the event dispatch
On 2012/07/09 15:12:08, bryeung wrote:
> should this get removed now?

Done.

https://chromiumcodereview.appspot.com/10694085/diff/8002/chrome/browser/exte...
chrome/browser/extensions/extension_event_router.cc:349: if (event_args) {
On 2012/07/09 15:12:08, bryeung wrote:
> nit: no braces

Done.

https://chromiumcodereview.appspot.com/10694085/diff/8002/chrome/browser/exte...
File chrome/browser/extensions/extension_event_router.h (right):

https://chromiumcodereview.appspot.com/10694085/diff/8002/chrome/browser/exte...
chrome/browser/extensions/extension_event_router.h:45: // any thread.
On 2012/07/09 15:12:08, bryeung wrote:
> It would be nice to update the comments here (and below) to document memory
> ownership.

Done.

https://chromiumcodereview.appspot.com/10694085/diff/8002/chrome/browser/exte...
File chrome/browser/extensions/settings/settings_frontend.cc (right):

https://chromiumcodereview.appspot.com/10694085/diff/8002/chrome/browser/exte...
chrome/browser/extensions/settings/settings_frontend.cc:44: // string-based
event payloads is removed.
On 2012/07/09 15:12:08, bryeung wrote:
> nit: Would be nice to include the bug for the refactoring here.

Done.

https://chromiumcodereview.appspot.com/10694085/diff/8002/chrome/browser/spee...
File chrome/browser/speech/speech_input_extension_manager.cc (left):

https://chromiumcodereview.appspot.com/10694085/diff/8002/chrome/browser/spee...
chrome/browser/speech/speech_input_extension_manager.cc:307: VLOG(1) <<
"Results: " << json_args;
On 2012/07/09 15:12:08, bryeung wrote:
> Is the speech input team using this VLOG?

Unclear. I'll add rsleevi and see if he wants it there still.

[bryeung, 2012-07-09 15:46:38]

lgtm

[Ryan Sleevi, 2012-07-09 17:14:37]

Hi Garret,

I haven't worked on this code, beyond possibly cleaning up RefCounting issues. I
added Primiano as a +cc due to active working on the speech code, but you
probably want to find another OWNERS.

Cheers!

[asargent_no_longer_on_chrome, 2012-07-09 18:51:50]

https://chromiumcodereview.appspot.com/10694085/diff/18002/chrome/browser/ext...
File chrome/browser/extensions/extension_event_router.cc (right):

https://chromiumcodereview.appspot.com/10694085/diff/18002/chrome/browser/ext...
chrome/browser/extensions/extension_event_router.cc:87: // ensure that args'
destruction doesn't result in event_args' deletion.
In the header documentation you specify that this method takes ownership of
event_args, but you don't delete it here. Either the documentation is wrong and
the caller still owns it, or you need to delete it, right?

[Garret Kelly, 2012-07-09 19:18:39]

> In the header documentation you specify that this method takes ownership of
> event_args, but you don't delete it here. Either the documentation is wrong
and
> the caller still owns it, or you need to delete it, right?

The ExtensionEvent structure has scoped_ptrs for both the event_args and
cross_incognito_args. When the ExtensionEvent eventually gets destroyed through
its final linked_ptr ref then the event_args and cross_incognito_args are
deleted too.

[asargent_no_longer_on_chrome, 2012-07-10 22:00:19]

>
>
>> The ExtensionEvent structure has scoped_ptrs for both the event_args and
> cross_incognito_args. When the ExtensionEvent eventually gets destroyed
> through
> its final linked_ptr ref then the event_args and cross_incognito_args are
> deleted too.
>

That is the case for DispatchEventToRenderers and DispatchEventToExtension.
My remark was referring to the  static DispatchEvent method, where you also
modified the documentation in the header to indicate that ownership is
transferred, even though the implementation of DispatchEvent does not take
ownership of the passed in event_args (you explicitly Remove it from the
local ListValue variable you build up for the IPC, to avoid having it get
deleted the local ListValue is destructed).

[miket_OOO, 2012-07-10 22:33:19]

I made it to the end! LGTM as long as ownership is straightened out. From
talking to Antony I think I can defer to his review for future passes.

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/bookmarks/b...
File chrome/browser/bookmarks/bookmark_extension_api.cc (right):

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/bookmarks/b...
chrome/browser/bookmarks/bookmark_extension_api.cc:146: void
BookmarkExtensionEventRouter::DispatchEvent(Profile* profile,
Well done.

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/bookmarks/b...
File chrome/browser/bookmarks/bookmark_manager_extension_api.cc (right):

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/bookmarks/b...
chrome/browser/bookmarks/bookmark_manager_extension_api.cc:182: ListValue* args
= new ListValue();
I talked to Antony and confirmed that this apparent new leak is fixed (though
the way it leaked, and the way it's fixed, it more complex than it appears
here).

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
File chrome/browser/extensions/api/alarms/alarm_manager.cc (right):

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
chrome/browser/extensions/api/alarms/alarm_manager.cc:43:
ExtensionSystem::Get(profile_)->event_router()->DispatchEventToExtension(
I'm guessing I'll see later on how DispatchEventToExtension() takes ownership of
args.

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
File chrome/browser/extensions/api/api_resource_event_notifier.cc (right):

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
chrome/browser/extensions/api/api_resource_event_notifier.cc:137: ListValue*
const arguments = new ListValue();
For future grep/sed/readability sanity, can you change this to args to match
prior usage?

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:1090:
profile_->GetExtensionEventRouter()->DispatchEventToRenderers(
Similar comment about future understanding of ownership model.

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:138:
DispatchEvent(web_contents->GetBrowserContext(), event_name, dict, url);
This CL would be prettier if dict were named args instead.

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_browser_event_router.cc (right):

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
chrome/browser/extensions/extension_browser_event_router.cc:439: if
(!profile_->IsSameProfile(profile) || !profile->GetExtensionEventRouter())
If this guy doesn't take ownership of args, it's confusing because it's
inconsistent. If it does, it leaks on the early return.

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
chrome/browser/extensions/extension_browser_event_router.cc:451: if
(!profile_->IsSameProfile(profile) || !profile->GetExtensionEventRouter())
Same comment about ownership/leak on return.

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_browser_event_router.h (right):

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
chrome/browser/extensions/extension_browser_event_router.h:129: base::ListValue*
arguments);
There should be some documentation here about ownership of these ListValues.

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_devtools_bridge.cc (right):

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
chrome/browser/extensions/extension_devtools_bridge.cc:109: base::ListValue*
arguments = new base::ListValue();
args?

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
chrome/browser/extensions/extension_devtools_bridge.cc:123: base::ListValue*
arguments = new base::ListValue();
same

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_event_router.h (right):

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
chrome/browser/extensions/extension_event_router.h:56: static void
DispatchEvent(IPC::Sender* ipc_sender,
Great, makes more sense now. I'd like to see similar comments in the earlier .h,
even if those are relatively private dispatch functions.

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_processes_api.cc (right):

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
chrome/browser/extensions/extension_processes_api.cc:486: scoped_ptr<ListValue>
delete_event_args(event_args);
That's pretty tricky. I am only about 51% sure it's better than a delete at the
end. I guess it's protection against a hypothetical future early return.

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
File chrome/browser/extensions/menu_manager.cc (right):

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
chrome/browser/extensions/menu_manager.cc:654: args, profile, GURL(),
ExtensionEventRouter::USER_GESTURE_ENABLED);
I think args is dead by this point.

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
File chrome/browser/extensions/system/system_api.cc (right):

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
chrome/browser/extensions/system/system_api.cc:61: base::ListValue* list_args =
new base::ListValue();
Just args! They must all be the same!

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/extensions/...
chrome/browser/extensions/system/system_api.cc:147:
DispatchEvent(kOnVolumeChanged, dict);
aaarrrrgs!

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/speech/spee...
File chrome/browser/speech/speech_input_extension_manager.cc (right):

http://codereview.chromium.org/10694085/diff/18002/chrome/browser/speech/spee...
chrome/browser/speech/speech_input_extension_manager.cc:436: new ListValue()));
Would NULL work here? (I honestly don't know, but it'd save a bit of code/time.)

[sky, 2012-07-16 22:29:01]

https://chromiumcodereview.appspot.com/10694085/diff/18002/chrome/browser/ext...
File chrome/browser/extensions/extension_event_router.h (right):

https://chromiumcodereview.appspot.com/10694085/diff/18002/chrome/browser/ext...
chrome/browser/extensions/extension_event_router.h:59: base::ListValue*
event_args,
Did you consider making this take a const base::ListValue& and doing a deep
copy? While not as efficient it would be less error prone.

[sky, 2012-07-16 22:33:02]

On 2012/07/16 22:29:01, sky wrote:
>
https://chromiumcodereview.appspot.com/10694085/diff/18002/chrome/browser/ext...
> File chrome/browser/extensions/extension_event_router.h (right):
> 
>
https://chromiumcodereview.appspot.com/10694085/diff/18002/chrome/browser/ext...
> chrome/browser/extensions/extension_event_router.h:59: base::ListValue*
> event_args,
> Did you consider making this take a const base::ListValue& and doing a deep
> copy? While not as efficient it would be less error prone.

Another option is to use scoped_ptr as the argument (forgot about that ability).
That provides many of the benefits of using a value but is more effecient than
doing a deep copy.

[Garret Kelly, 2012-07-16 22:35:32]

> Another option is to use scoped_ptr as the argument (forgot about that
ability).
> That provides many of the benefits of using a value but is more effecient than
> doing a deep copy.

I had forgotten that that was an option. I'm going to update the CL with a
changeover to scoped_ptr. Thanks!

[commit-bot: I haz the power, 2012-08-07 22:07:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gdk@chromium.org/10694085/47005

[commit-bot: I haz the power, 2012-08-07 22:07:32]

Failed to apply patch for chrome/browser/bookmarks/bookmark_extension_api.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/bookmarks/bookmark_extension_api.cc
Hunk #1 succeeded at 148 (offset 1 line).
Hunk #2 succeeded at 176 (offset 1 line).
Hunk #3 FAILED at 185.
Hunk #4 succeeded at 213 (offset 1 line).
Hunk #5 succeeded at 238 (offset 1 line).
Hunk #6 succeeded at 250 (offset 1 line).
Hunk #7 succeeded at 261 (offset 1 line).
1 out of 7 hunks FAILED -- saving rejects to file
chrome/browser/bookmarks/bookmark_extension_api.cc.rej

[commit-bot: I haz the power, 2012-08-07 22:14:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gdk@chromium.org/10694085/51007

[commit-bot: I haz the power, 2012-08-07 22:14:52]

Presubmit check for 10694085-51007 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/speech/extension_api
    chrome/browser/speech
    chrome/browser/history
    chrome/browser/bookmarks
    chrome/browser/accessibility

Presubmit checks took 4.3s to calculate.

[sky, 2012-08-07 22:18:11]

LGTM

[commit-bot: I haz the power, 2012-08-07 22:18:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gdk@chromium.org/10694085/51007

[dmazzoni, 2012-08-07 22:26:18]

LGTM for my directories

[commit-bot: I haz the power, 2012-08-07 23:58:18]

Change committed as 150460