Update extension events, routing, and delivery to use user gesture.

Update extension events, routing, and delivery to use user gesture.

R=aa@chromium.org
BUG=109425
TEST=*Extension*


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

[Aaron Boodman, 2012-02-03 20:18:47]

LGTM, but Matt should also take a quick look.

https://chromiumcodereview.appspot.com/9316102/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/extension_event_router.h (right):

https://chromiumcodereview.appspot.com/9316102/diff/1/chrome/browser/extensio...
chrome/browser/extensions/extension_event_router.h:38: bool user_caused =
false);
I suggest consistently using the term 'user_gesture' since it has a specific
security meaning in browsers. It's more self-documenting that way.

https://chromiumcodereview.appspot.com/9316102/diff/1/chrome/browser/extensio...
chrome/browser/extensions/extension_event_router.h:38: bool user_caused =
false);
Also, consider using an enum for this flag, like:

enum UserGesture {
  IsUserGesture
  NotUserGesture
}

This might make the code more readable than the boolean flag.

https://chromiumcodereview.appspot.com/9316102/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/extension_menu_manager.cc (right):

https://chromiumcodereview.appspot.com/9316102/diff/1/chrome/browser/extensio...
chrome/browser/extensions/extension_menu_manager.cc:463: item->extension_id(),
event_name, json_args, profile, GURL(), true);
If you don't use an enum, change this to a named constant for readability.

https://chromiumcodereview.appspot.com/9316102/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/extension_message_service.cc (right):

https://chromiumcodereview.appspot.com/9316102/diff/1/chrome/browser/extensio...
chrome/browser/extensions/extension_message_service.cc:80:
ExtensionMessageService::kDispatchOnConnect, args, GURL(), false));
named constant

[Matt Perry, 2012-02-03 20:34:40]

https://chromiumcodereview.appspot.com/9316102/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/extension_event_router.h (right):

https://chromiumcodereview.appspot.com/9316102/diff/1/chrome/browser/extensio...
chrome/browser/extensions/extension_event_router.h:38: bool user_caused =
false);
Default arguments are forbidden by the style guide.

[Greg Billock, 2012-02-03 22:57:56]

https://chromiumcodereview.appspot.com/9316102/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/extension_event_router.h (right):

https://chromiumcodereview.appspot.com/9316102/diff/1/chrome/browser/extensio...
chrome/browser/extensions/extension_event_router.h:38: bool user_caused =
false);
On 2012/02/03 20:18:47, Aaron Boodman wrote:
> I suggest consistently using the term 'user_gesture' since it has a specific
> security meaning in browsers. It's more self-documenting that way.

My plan was to only set the gesture indicator when the flag is on. Shall I
instead make an enum (including a "UnknownUserGesture" value) and make it always
use the passed gesture indicator unless it is unknown? (WebKit has three: Yes,
No, and Possible)

[Aaron Boodman, 2012-02-05 03:00:01]

On 2012/02/03 22:57:56, Greg Billock wrote:
>
https://chromiumcodereview.appspot.com/9316102/diff/1/chrome/browser/extensio...
> File chrome/browser/extensions/extension_event_router.h (right):
> 
>
https://chromiumcodereview.appspot.com/9316102/diff/1/chrome/browser/extensio...
> chrome/browser/extensions/extension_event_router.h:38: bool user_caused =
> false);
> On 2012/02/03 20:18:47, Aaron Boodman wrote:
> > I suggest consistently using the term 'user_gesture' since it has a specific
> > security meaning in browsers. It's more self-documenting that way.
> 
> My plan was to only set the gesture indicator when the flag is on. Shall I
> instead make an enum (including a "UnknownUserGesture" value) and make it
always
> use the passed gesture indicator unless it is unknown? (WebKit has three: Yes,
> No, and Possible)

What is the purpose of the "Possible" value? It seems like you'd only need Yes,
and No, and the value would be No unless you specifically knew that it should be
Yes.

[Greg Billock, 2012-02-06 17:32:24]

On 2012/02/05 03:00:01, Aaron Boodman wrote:
> On 2012/02/03 22:57:56, Greg Billock wrote:
> >
>
https://chromiumcodereview.appspot.com/9316102/diff/1/chrome/browser/extensio...
> > File chrome/browser/extensions/extension_event_router.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/9316102/diff/1/chrome/browser/extensio...
> > chrome/browser/extensions/extension_event_router.h:38: bool user_caused =
> > false);
> > On 2012/02/03 20:18:47, Aaron Boodman wrote:
> > > I suggest consistently using the term 'user_gesture' since it has a
specific
> > > security meaning in browsers. It's more self-documenting that way.
> > 
> > My plan was to only set the gesture indicator when the flag is on. Shall I
> > instead make an enum (including a "UnknownUserGesture" value) and make it
> always
> > use the passed gesture indicator unless it is unknown? (WebKit has three:
Yes,
> > No, and Possible)
> 
> What is the purpose of the "Possible" value? It seems like you'd only need
Yes,
> and No, and the value would be No unless you specifically knew that it should
be
> Yes.

Looking at the code, it looks like this value gets triggered when the code
thinks it is in a situation where there's a user gesture, but not an explicit
marker that it is.

I don't see any specific treatment of the "possible" value at present, so I
think the permissioning logic works like you suggest.

[Aaron Boodman, 2012-02-06 18:38:41]

On 2012/02/06 17:32:24, Greg Billock wrote:
> On 2012/02/05 03:00:01, Aaron Boodman wrote:
> > On 2012/02/03 22:57:56, Greg Billock wrote:
> > >
> >
>
https://chromiumcodereview.appspot.com/9316102/diff/1/chrome/browser/extensio...
> > > File chrome/browser/extensions/extension_event_router.h (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/9316102/diff/1/chrome/browser/extensio...
> > > chrome/browser/extensions/extension_event_router.h:38: bool user_caused =
> > > false);
> > > On 2012/02/03 20:18:47, Aaron Boodman wrote:
> > > > I suggest consistently using the term 'user_gesture' since it has a
> specific
> > > > security meaning in browsers. It's more self-documenting that way.
> > > 
> > > My plan was to only set the gesture indicator when the flag is on. Shall I
> > > instead make an enum (including a "UnknownUserGesture" value) and make it
> > always
> > > use the passed gesture indicator unless it is unknown? (WebKit has three:
> Yes,
> > > No, and Possible)
> > 
> > What is the purpose of the "Possible" value? It seems like you'd only need
> Yes,
> > and No, and the value would be No unless you specifically knew that it
should
> be
> > Yes.
> 
> Looking at the code, it looks like this value gets triggered when the code
> thinks it is in a situation where there's a user gesture, but not an explicit
> marker that it is.
> 
> I don't see any specific treatment of the "possible" value at present, so I
> think the permissioning logic works like you suggest.

Ok, so on the chrome side can we just have two values: yes/no?

[Greg Billock, 2012-02-06 20:19:57]

On 2012/02/06 18:38:41, Aaron Boodman wrote:
> On 2012/02/06 17:32:24, Greg Billock wrote:
> > On 2012/02/05 03:00:01, Aaron Boodman wrote:
> > > On 2012/02/03 22:57:56, Greg Billock wrote:
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/9316102/diff/1/chrome/browser/extensio...
> > > > File chrome/browser/extensions/extension_event_router.h (right):
> > > > 
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/9316102/diff/1/chrome/browser/extensio...
> > > > chrome/browser/extensions/extension_event_router.h:38: bool user_caused
=
> > > > false);
> > > > On 2012/02/03 20:18:47, Aaron Boodman wrote:
> > > > > I suggest consistently using the term 'user_gesture' since it has a
> > specific
> > > > > security meaning in browsers. It's more self-documenting that way.
> > > > 
> > > > My plan was to only set the gesture indicator when the flag is on. Shall
I
> > > > instead make an enum (including a "UnknownUserGesture" value) and make
it
> > > always
> > > > use the passed gesture indicator unless it is unknown? (WebKit has
three:
> > Yes,
> > > > No, and Possible)
> > > 
> > > What is the purpose of the "Possible" value? It seems like you'd only need
> > Yes,
> > > and No, and the value would be No unless you specifically knew that it
> should
> > be
> > > Yes.
> > 
> > Looking at the code, it looks like this value gets triggered when the code
> > thinks it is in a situation where there's a user gesture, but not an
explicit
> > marker that it is.
> > 
> > I don't see any specific treatment of the "possible" value at present, so I
> > think the permissioning logic works like you suggest.
> 
> Ok, so on the chrome side can we just have two values: yes/no?

Yeah, that's the direction I'm going. Still planning on using an enum, since
that reads better.

[Greg Billock, 2012-02-06 22:58:00]

http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_event_router.h (right):

http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_event_router.h:38: bool user_caused =
false);
On 2012/02/03 20:18:47, Aaron Boodman wrote:
> I suggest consistently using the term 'user_gesture' since it has a specific
> security meaning in browsers. It's more self-documenting that way.

Done.

http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_event_router.h:38: bool user_caused =
false);
On 2012/02/03 20:18:47, Aaron Boodman wrote:
> Also, consider using an enum for this flag, like:
> 
> enum UserGesture {
>   IsUserGesture
>   NotUserGesture
> }
> 
> This might make the code more readable than the boolean flag.

Done.

http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_event_router.h:38: bool user_caused =
false);
On 2012/02/03 20:34:41, Matt Perry wrote:
> Default arguments are forbidden by the style guide.

Done.

http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_menu_manager.cc (right):

http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_menu_manager.cc:463: item->extension_id(),
event_name, json_args, profile, GURL(), true);
On 2012/02/03 20:18:47, Aaron Boodman wrote:
> If you don't use an enum, change this to a named constant for readability.

Done.

http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_message_service.cc (right):

http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_message_service.cc:80:
ExtensionMessageService::kDispatchOnConnect, args, GURL(), false));
On 2012/02/03 20:18:47, Aaron Boodman wrote:
> named constant

Done.

[Greg Billock, 2012-02-06 23:00:36]

(Note that this change is dependent on how the WebKit API ends up looking:
https://bugs.webkit.org/show_bug.cgi?id=77690)


On 2012/02/06 22:58:00, Greg Billock wrote:
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
> File chrome/browser/extensions/extension_event_router.h (right):
> 
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
> chrome/browser/extensions/extension_event_router.h:38: bool user_caused =
> false);
> On 2012/02/03 20:18:47, Aaron Boodman wrote:
> > I suggest consistently using the term 'user_gesture' since it has a specific
> > security meaning in browsers. It's more self-documenting that way.
> 
> Done.
> 
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
> chrome/browser/extensions/extension_event_router.h:38: bool user_caused =
> false);
> On 2012/02/03 20:18:47, Aaron Boodman wrote:
> > Also, consider using an enum for this flag, like:
> > 
> > enum UserGesture {
> >   IsUserGesture
> >   NotUserGesture
> > }
> > 
> > This might make the code more readable than the boolean flag.
> 
> Done.
> 
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
> chrome/browser/extensions/extension_event_router.h:38: bool user_caused =
> false);
> On 2012/02/03 20:34:41, Matt Perry wrote:
> > Default arguments are forbidden by the style guide.
> 
> Done.
> 
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
> File chrome/browser/extensions/extension_menu_manager.cc (right):
> 
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
> chrome/browser/extensions/extension_menu_manager.cc:463: item->extension_id(),
> event_name, json_args, profile, GURL(), true);
> On 2012/02/03 20:18:47, Aaron Boodman wrote:
> > If you don't use an enum, change this to a named constant for readability.
> 
> Done.
> 
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
> File chrome/browser/extensions/extension_message_service.cc (right):
> 
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
> chrome/browser/extensions/extension_message_service.cc:80:
> ExtensionMessageService::kDispatchOnConnect, args, GURL(), false));
> On 2012/02/03 20:18:47, Aaron Boodman wrote:
> > named constant
> 
> Done.

[Greg Billock, 2012-02-13 21:36:10]

On the webkit API side, I've taken out the user gesture constructor for
non-user-caused gestures after talking with Darin. I don't think we need them at
present. Does that mean I should switch the messaging field back to a bool from
the enum?

On 2012/02/06 23:00:36, Greg Billock wrote:
> (Note that this change is dependent on how the WebKit API ends up looking:
> https://bugs.webkit.org/show_bug.cgi?id=77690)
> 
> 
> On 2012/02/06 22:58:00, Greg Billock wrote:
> >
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
> > File chrome/browser/extensions/extension_event_router.h (right):
> > 
> >
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
> > chrome/browser/extensions/extension_event_router.h:38: bool user_caused =
> > false);
> > On 2012/02/03 20:18:47, Aaron Boodman wrote:
> > > I suggest consistently using the term 'user_gesture' since it has a
specific
> > > security meaning in browsers. It's more self-documenting that way.
> > 
> > Done.
> > 
> >
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
> > chrome/browser/extensions/extension_event_router.h:38: bool user_caused =
> > false);
> > On 2012/02/03 20:18:47, Aaron Boodman wrote:
> > > Also, consider using an enum for this flag, like:
> > > 
> > > enum UserGesture {
> > >   IsUserGesture
> > >   NotUserGesture
> > > }
> > > 
> > > This might make the code more readable than the boolean flag.
> > 
> > Done.
> > 
> >
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
> > chrome/browser/extensions/extension_event_router.h:38: bool user_caused =
> > false);
> > On 2012/02/03 20:34:41, Matt Perry wrote:
> > > Default arguments are forbidden by the style guide.
> > 
> > Done.
> > 
> >
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
> > File chrome/browser/extensions/extension_menu_manager.cc (right):
> > 
> >
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
> > chrome/browser/extensions/extension_menu_manager.cc:463:
item->extension_id(),
> > event_name, json_args, profile, GURL(), true);
> > On 2012/02/03 20:18:47, Aaron Boodman wrote:
> > > If you don't use an enum, change this to a named constant for readability.
> > 
> > Done.
> > 
> >
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
> > File chrome/browser/extensions/extension_message_service.cc (right):
> > 
> >
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
> > chrome/browser/extensions/extension_message_service.cc:80:
> > ExtensionMessageService::kDispatchOnConnect, args, GURL(), false));
> > On 2012/02/03 20:18:47, Aaron Boodman wrote:
> > > named constant
> > 
> > Done.

[Aaron Boodman, 2012-02-13 21:38:46]

I'm sorry, I don't follow. Are there still two cases? If so, I still
prefer the enum rather than a boolean flag. If there is only one case,
then of course no flag is necessary at all.

- a

On Mon, Feb 13, 2012 at 1:36 PM,  <gbillock@chromium.org> wrote:
> On the webkit API side, I've taken out the user gesture constructor for
> non-user-caused gestures after talking with Darin. I don't think we need
> them at
> present. Does that mean I should switch the messaging field back to a bool
> from
> the enum?
>
>
> On 2012/02/06 23:00:36, Greg Billock wrote:
>>
>> (Note that this change is dependent on how the WebKit API ends up looking:
>> https://bugs.webkit.org/show_bug.cgi?id=77690)
>
>
>
>> On 2012/02/06 22:58:00, Greg Billock wrote:
>> >
>
>
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
>>
>> > File chrome/browser/extensions/extension_event_router.h (right):
>> >
>> >
>
>
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
>>
>> > chrome/browser/extensions/extension_event_router.h:38: bool user_caused
>> > =
>> > false);
>> > On 2012/02/03 20:18:47, Aaron Boodman wrote:
>> > > I suggest consistently using the term 'user_gesture' since it has a
>
> specific
>>
>> > > security meaning in browsers. It's more self-documenting that way.
>> >
>> > Done.
>> >
>> >
>
>
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
>>
>> > chrome/browser/extensions/extension_event_router.h:38: bool user_caused
>> > =
>> > false);
>> > On 2012/02/03 20:18:47, Aaron Boodman wrote:
>> > > Also, consider using an enum for this flag, like:
>> > >
>> > > enum UserGesture {
>> > >   IsUserGesture
>> > >   NotUserGesture
>> > > }
>> > >
>> > > This might make the code more readable than the boolean flag.
>> >
>> > Done.
>> >
>> >
>
>
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
>>
>> > chrome/browser/extensions/extension_event_router.h:38: bool user_caused
>> > =
>> > false);
>> > On 2012/02/03 20:34:41, Matt Perry wrote:
>> > > Default arguments are forbidden by the style guide.
>> >
>> > Done.
>> >
>> >
>
>
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
>>
>> > File chrome/browser/extensions/extension_menu_manager.cc (right):
>> >
>> >
>
>
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
>>
>> > chrome/browser/extensions/extension_menu_manager.cc:463:
>
> item->extension_id(),
>>
>> > event_name, json_args, profile, GURL(), true);
>> > On 2012/02/03 20:18:47, Aaron Boodman wrote:
>> > > If you don't use an enum, change this to a named constant for
>> > > readability.
>> >
>> > Done.
>> >
>> >
>
>
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
>>
>> > File chrome/browser/extensions/extension_message_service.cc (right):
>> >
>> >
>
>
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
>>
>> > chrome/browser/extensions/extension_message_service.cc:80:
>> > ExtensionMessageService::kDispatchOnConnect, args, GURL(), false));
>> > On 2012/02/03 20:18:47, Aaron Boodman wrote:
>> > > named constant
>> >
>> > Done.
>
>
>
>
> http://codereview.chromium.org/9316102/

[Greg Billock, 2012-02-13 22:36:47]

Right now the WebKit API only supports creating a user gesture object which
indicates user gesture. (That's currently all anyone wants to do.)

There's still the question of whether or not to make the indicator at all, but
that's now a binary distinction. If the need to mark explicitly that we're not
executing as a result of a user gesture arises, we can retrofit the enum into
the code fairly easily, so I'm inclined to change back to a boolean. Agreed?


On 2012/02/13 21:38:46, Aaron Boodman wrote:
> I'm sorry, I don't follow. Are there still two cases? If so, I still
> prefer the enum rather than a boolean flag. If there is only one case,
> then of course no flag is necessary at all.
> 
> - a
> 
> On Mon, Feb 13, 2012 at 1:36 PM,  <mailto:gbillock@chromium.org> wrote:
> > On the webkit API side, I've taken out the user gesture constructor for
> > non-user-caused gestures after talking with Darin. I don't think we need
> > them at
> > present. Does that mean I should switch the messaging field back to a bool
> > from
> > the enum?
> >
> >
> > On 2012/02/06 23:00:36, Greg Billock wrote:
> >>
> >> (Note that this change is dependent on how the WebKit API ends up looking:
> >> https://bugs.webkit.org/show_bug.cgi?id=77690)
> >
> >
> >
> >> On 2012/02/06 22:58:00, Greg Billock wrote:
> >> >
> >
> >
> >
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
> >>
> >> > File chrome/browser/extensions/extension_event_router.h (right):
> >> >
> >> >
> >
> >
> >
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
> >>
> >> > chrome/browser/extensions/extension_event_router.h:38: bool user_caused
> >> > =
> >> > false);
> >> > On 2012/02/03 20:18:47, Aaron Boodman wrote:
> >> > > I suggest consistently using the term 'user_gesture' since it has a
> >
> > specific
> >>
> >> > > security meaning in browsers. It's more self-documenting that way.
> >> >
> >> > Done.
> >> >
> >> >
> >
> >
> >
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
> >>
> >> > chrome/browser/extensions/extension_event_router.h:38: bool user_caused
> >> > =
> >> > false);
> >> > On 2012/02/03 20:18:47, Aaron Boodman wrote:
> >> > > Also, consider using an enum for this flag, like:
> >> > >
> >> > > enum UserGesture {
> >> > > &nbsp; IsUserGesture
> >> > > &nbsp; NotUserGesture
> >> > > }
> >> > >
> >> > > This might make the code more readable than the boolean flag.
> >> >
> >> > Done.
> >> >
> >> >
> >
> >
> >
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
> >>
> >> > chrome/browser/extensions/extension_event_router.h:38: bool user_caused
> >> > =
> >> > false);
> >> > On 2012/02/03 20:34:41, Matt Perry wrote:
> >> > > Default arguments are forbidden by the style guide.
> >> >
> >> > Done.
> >> >
> >> >
> >
> >
> >
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
> >>
> >> > File chrome/browser/extensions/extension_menu_manager.cc (right):
> >> >
> >> >
> >
> >
> >
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
> >>
> >> > chrome/browser/extensions/extension_menu_manager.cc:463:
> >
> > item->extension_id(),
> >>
> >> > event_name, json_args, profile, GURL(), true);
> >> > On 2012/02/03 20:18:47, Aaron Boodman wrote:
> >> > > If you don't use an enum, change this to a named constant for
> >> > > readability.
> >> >
> >> > Done.
> >> >
> >> >
> >
> >
> >
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
> >>
> >> > File chrome/browser/extensions/extension_message_service.cc (right):
> >> >
> >> >
> >
> >
> >
>
http://codereview.chromium.org/9316102/diff/1/chrome/browser/extensions/exten...
> >>
> >> > chrome/browser/extensions/extension_message_service.cc:80:
> >> > ExtensionMessageService::kDispatchOnConnect, args, GURL(), false));
> >> > On 2012/02/03 20:18:47, Aaron Boodman wrote:
> >> > > named constant
> >> >
> >> > Done.
> >
> >
> >
> >
> > http://codereview.chromium.org/9316102/

[Greg Billock, 2012-02-24 18:15:01]

OK, this is in on the webkit side now. Take another look?

[Greg Billock, 2012-02-29 00:59:06]

On 2012/02/24 18:15:01, Greg Billock wrote:
> OK, this is in on the webkit side now. Take another look?

ping. Still LGTM?

[Aaron Boodman, 2012-02-29 01:20:37]

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

http://codereview.chromium.org/9316102/diff/24001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.h:33: enum UserGesture {
Naming suggestion: UserGestureState?

http://codereview.chromium.org/9316102/diff/24001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.h:34: kUnknownUserGesture = 0,
according to the style guide (http://www.chromium.org/developers/coding-style),
this should be:

USER_GESTURE_UNKNOWN,
USER_GESTURE_ENABLED,
USER_GESTURE_NOT_ENABLED

or something...

http://codereview.chromium.org/9316102/diff/24001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_message_service.cc (right):

http://codereview.chromium.org/9316102/diff/24001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_message_service.cc:80:
ExtensionMessageService::kDispatchOnConnect, args, GURL(), false));
In these cases where you have to use a boolean constant, it's still nice to add
an inline comment describing the meaning:

GURL(),
false);  // not in a user gesture

[Greg Billock, 2012-02-29 17:58:08]

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

http://codereview.chromium.org/9316102/diff/24001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.h:33: enum UserGesture {
On 2012/02/29 01:20:37, Aaron Boodman wrote:
> Naming suggestion: UserGestureState?

Done.

http://codereview.chromium.org/9316102/diff/24001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_router.h:34: kUnknownUserGesture = 0,
I'd read this: "Though the style guide says to use kConstantNaming for enums
now, we still use MACRO_STYLE naming for enums for consistency with existing
code."

And that made me think that the style guide wanted me to use kEnumValue style,
except where existing code differed (i.e. adding a value to an existing list).

Reading it again, I am not sure what it means!

On 2012/02/29 01:20:37, Aaron Boodman wrote:
> according to the style guide
(http://www.chromium.org/developers/coding-style),
> this should be:
> 
> USER_GESTURE_UNKNOWN,
> USER_GESTURE_ENABLED,
> USER_GESTURE_NOT_ENABLED
> 
> or something...

http://codereview.chromium.org/9316102/diff/24001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_message_service.cc (right):

http://codereview.chromium.org/9316102/diff/24001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_message_service.cc:80:
ExtensionMessageService::kDispatchOnConnect, args, GURL(), false));
Good call. Done.

On 2012/02/29 01:20:37, Aaron Boodman wrote:
> In these cases where you have to use a boolean constant, it's still nice to
add
> an inline comment describing the meaning:
> 
> GURL(),
> false);  // not in a user gesture

[Aaron Boodman, 2012-02-29 20:32:36]

On 2012/02/29 17:58:08, Greg Billock wrote:
>
http://codereview.chromium.org/9316102/diff/24001/chrome/browser/extensions/e...
> File chrome/browser/extensions/extension_event_router.h (right):
> 
>
http://codereview.chromium.org/9316102/diff/24001/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_event_router.h:33: enum UserGesture {
> On 2012/02/29 01:20:37, Aaron Boodman wrote:
> > Naming suggestion: UserGestureState?
> 
> Done.
> 
>
http://codereview.chromium.org/9316102/diff/24001/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_event_router.h:34: kUnknownUserGesture =
0,
> I'd read this: "Though the style guide says to use kConstantNaming for enums
> now, we still use MACRO_STYLE naming for enums for consistency with existing
> code."
> 
> And that made me think that the style guide wanted me to use kEnumValue style,
> except where existing code differed (i.e. adding a value to an existing list).
> 
> Reading it again, I am not sure what it means!

The "style guide" is referencing the Google C++ Style Guide, which Chrome's
style guide is based on, but not identical to.

I've updated the text to make it more clear.

[Greg Billock, 2012-03-01 00:50:07]

On 2012/02/29 20:32:36, Aaron Boodman wrote:
> On 2012/02/29 17:58:08, Greg Billock wrote:
> >
>
http://codereview.chromium.org/9316102/diff/24001/chrome/browser/extensions/e...
> > File chrome/browser/extensions/extension_event_router.h (right):
> > 
> >
>
http://codereview.chromium.org/9316102/diff/24001/chrome/browser/extensions/e...
> > chrome/browser/extensions/extension_event_router.h:33: enum UserGesture {
> > On 2012/02/29 01:20:37, Aaron Boodman wrote:
> > > Naming suggestion: UserGestureState?
> > 
> > Done.
> > 
> >
>
http://codereview.chromium.org/9316102/diff/24001/chrome/browser/extensions/e...
> > chrome/browser/extensions/extension_event_router.h:34: kUnknownUserGesture =
> 0,
> > I'd read this: "Though the style guide says to use kConstantNaming for enums
> > now, we still use MACRO_STYLE naming for enums for consistency with existing
> > code."
> > 
> > And that made me think that the style guide wanted me to use kEnumValue
style,
> > except where existing code differed (i.e. adding a value to an existing
list).
> > 
> > Reading it again, I am not sure what it means!
> 
> The "style guide" is referencing the Google C++ Style Guide, which Chrome's
> style guide is based on, but not identical to.
> 
> I've updated the text to make it more clear.

Ah, that makes sense. Changed.

[Aaron Boodman, 2012-03-01 07:55:29]

lgtm

[commit-bot: I haz the power, 2012-03-01 21:31:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/9316102/39001

[commit-bot: I haz the power, 2012-03-01 21:31:48]

Presubmit check for 9316102-39001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  chrome/browser/extensions/extension_menu_manager_unittest.cc, line 460, 83
chars

Presubmit checks took 1.7s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2012-03-01 23:05:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/9316102/41003

[commit-bot: I haz the power, 2012-03-02 04:37:28]

Change committed as 124596