Initial work to remove the notion of all extension message payloads as strings.

Initial work to remove the notion of all extension message payloads as strings.

The internal IPC events already route ListValues around, however a _lot_ of
places have assumed that the message payload string will be eventually treated
as a JSON string. This change allows arbitrary Value types to be passed in
message payloads while still handling the case of users of the API passing in
hard-coded JSON strings.

BUG=133190
TEST=none


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

[Garret Kelly, 2012-06-17 00:35:57]

Very much depends on 133190.

[bryeung, 2012-06-18 03:12:21]

lgtm

[asargent_no_longer_on_chrome, 2012-06-18 16:25:30]

LGTM

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

https://chromiumcodereview.appspot.com/10566018/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_event_router.cc:126: args.Set(1,
event_args.DeepCopy());
It's a bummer to have to copy the arguments just to hand off to be sent over the
IPC channel. Can you avoid the DeepCopy here by doing:

ListValue args;
args.Set(0, Value::CreateStringValue(event_name));
args.Set(1, static_cast<Value*>(&event_args));

...

// Take back ownership of event_args so it doesn't get deleted.
Value* tmp;
args.Remove(1, &tmp);

?

This can be a follow-up TODO though if you want. (I suppose for pedantry we
might consider changing event_args from const Value& to Value* to avoid needing
to cast away the constness).

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

https://chromiumcodereview.appspot.com/10566018/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_event_router.h:52: static void
DispatchEvent(IPC::Sender* ipc_sender,
Can you add a comment here saying that this is deprecated and to prefer using
the above version that takes a Value instead of a std::string with JSON?

https://chromiumcodereview.appspot.com/10566018/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_event_router.h:108: virtual void
DispatchEventToExtension(
add same comment here

https://chromiumcodereview.appspot.com/10566018/diff/3001/chrome/renderer/res...
File chrome/renderer/resources/extensions/event.js (right):

https://chromiumcodereview.appspot.com/10566018/diff/3001/chrome/renderer/res...
chrome/renderer/resources/extensions/event.js:142: // This is an antiquity.
Until all callers of dispatchJSON use actual
You can add TODO(asargent) to this comment. (There's a similar  TODO for me in
send_request.js).

[miket_OOO, 2012-06-18 16:32:12]

Great! LGTM.

[Garret Kelly, 2012-06-18 18:15:01]

I'll fire this to the CQ if there's no other issues.

http://codereview.chromium.org/10566018/diff/3001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_event_router.cc (right):

http://codereview.chromium.org/10566018/diff/3001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.cc:126: args.Set(1,
event_args.DeepCopy());
I've added a TODO, I'll refactor that as part of the remaining changeover to
Value for event_args.

http://codereview.chromium.org/10566018/diff/3001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_event_router.h (right):

http://codereview.chromium.org/10566018/diff/3001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.h:52: static void
DispatchEvent(IPC::Sender* ipc_sender,
On 2012/06/18 16:25:30, Antony Sargent wrote:
> Can you add a comment here saying that this is deprecated and to prefer using
> the above version that takes a Value instead of a std::string with JSON?

Done.

http://codereview.chromium.org/10566018/diff/3001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.h:108: virtual void
DispatchEventToExtension(
On 2012/06/18 16:25:30, Antony Sargent wrote:
> add same comment here

Done.

http://codereview.chromium.org/10566018/diff/3001/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/event.js (right):

http://codereview.chromium.org/10566018/diff/3001/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/event.js:142: // This is an antiquity.
Until all callers of dispatchJSON use actual
On 2012/06/18 16:25:30, Antony Sargent wrote:
> You can add TODO(asargent) to this comment. (There's a similar  TODO for me in
> send_request.js).
> 

Done.

[commit-bot: I haz the power, 2012-06-18 18:21:10]

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

[commit-bot: I haz the power, 2012-06-18 19:37:33]

Change committed as 142785