This adds a webui overlay on the extensions page for showing what Extension keybindings are active.…

This adds a webui overlay on the extensions page for showing what Extension keybindings are active. Doesn't allow you to change anything yet (future changelist).

This is the precursor to Extension Commands Conflict Resolution.

BUG=121420
TEST=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=138145

[Finnur, 2012-05-18 15:36:29]

Ben, Aaron, Elliot and Evan, please find your monikers below along with further
instructions:

beng, can you provide owner's LG for browser/ui?


aa, can you validate the high-level approach and maybe these files:

chrome\browser\extensions\api\commands\extension_command_service.h
chrome\browser\extensions\api\commands\extension_command_service.cc
chrome\common\extensions\extension_commands.h
chrome\common\extensions\extension_commands_unittest.cc


erg, can you do a quick sanity check on these (mostly just an OWNERS LG)?

chrome\browser\ui\gtk\browser_actions_toolbar_gtk.cc
chrome\browser\ui\gtk\extensions\extension_keybinding_registry_gtk.cc
chrome\browser\ui\gtk\location_bar_view_gtk.cc
... and maybe these, since it is just more of the same?
chrome\browser\ui\views\extensions\extension_keybinding_registry_views.cc
chrome\browser\ui\views\extensions\extension_installed_bubble.cc
chrome\browser\ui\views\browser_actions_container.cc
chrome\browser\ui\views\location_bar\page_action_image_view.cc


estade, this is modeled after your new extensions page:
chrome\app\generated_resources.grd
chrome\browser\resources\extensions\extension_command_list.js
chrome\browser\resources\extensions\extension_commands_overlay.html
chrome\browser\resources\extensions\extensions.html
chrome\browser\resources\extensions\extension_commands_overlay.css
chrome\browser\resources\extensions\extensions.css
chrome\browser\resources\extensions\extension_commands_overlay.js
chrome\browser\resources\extensions\extensions.js
chrome\browser\browser_resources.grd
chrome\chrome_browser.gypi
chrome\browser\ui\webui\extensions\extension_commands_handler.h
chrome\browser\ui\webui\extensions\extension_commands_handler.cc
chrome\browser\ui\webui\extensions\extension_settings_handler.cc
chrome\browser\ui\webui\extensions\extensions_ui.cc
... and also, this:
chrome\browser\resources\shared\js\load_time_data.js
I can leave out the change in that last file, if you don't care for it but the
comment I'm adding would have saved me quite a bit of head scratching when
debugging why my strings weren't appearing.

[Ben Goodger (Google), 2012-05-18 15:58:55]

random browser ui stuff not covered by other reviewers: LGTM

[Elliot Glaysher, 2012-05-18 16:54:04]

gtk lgtm

[Evan Stade, 2012-05-18 21:08:26]

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_command_list.js (right):

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_command_list.js:33:
createExtensionNode_: function(extension) {
createNodeForExtension is more apt

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_command_list.js:35:
'.extension-command-list-extension-item-wrapper');
too much indent

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_command_list.js:37: node.id =
extension.id;
if this is the same as the extension id, I think it's a bad idea to use that as
the ID without prepending some sort of qualifier, like "extension-command-"

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_command_list.js:57:
'.extension-command-list-command-item-wrapper');
too much indent.

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_commands_overlay.css (right):

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_commands_overlay.css:18: /*
TODO(estade): apply this everywhere. */
you don't need to copy this, it already exists due to pack_extension_overlay.css

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_commands_overlay.html
(right):

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_commands_overlay.html:1: <div
id="extensionCommandsOverlay" class="page">
license

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_commands_overlay.html:3: <div
id="cbdContentArea" class="content-area">
id should be dash-form

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_commands_overlay.html:4: <div
id="extension-command-list" class="empty-extension-commands-list"></div>
wrap

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
File chrome/browser/resources/extensions/extensions.js (right):

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
chrome/browser/resources/extensions/extensions.js:8: <include
src="extension_commands_overlay.js"></include>
alphabet?

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
File chrome/browser/resources/shared/js/load_time_data.js (right):

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
chrome/browser/resources/shared/js/load_time_data.js:47: // GetLocalizedValues()
in your handler class (in C++) is called.
this comment is inaccurate, GetLocalizedValues() does not exist for all
WebUIMessageHandler classes. I don't think it's worth including --- this isn't
where I would expect most people to look for documentation.

[Finnur, 2012-05-18 22:02:04]

Thanks for the comments, Evan. I have addressed them with a couple of exceptions
(see questions below). PTAL.

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_command_list.js (right):

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_command_list.js:33:
createExtensionNode_: function(extension) {
On 2012/05/18 21:08:26, Evan Stade wrote:
> createNodeForExtension is more apt

Done.

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_command_list.js:35:
'.extension-command-list-extension-item-wrapper');
Wait... Continuations from previous lines have always used 4 spaces for
indentation, no?

foo +
    bar;

not

foo +
  bar;

right?

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_command_list.js:37: node.id =
extension.id;
This isn't used at the moment so I just removed it.

On 2012/05/18 21:08:26, Evan Stade wrote:
> if this is the same as the extension id, I think it's a bad idea to use that
as
> the ID without prepending some sort of qualifier, like "extension-command-"

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_command_list.js:57:
'.extension-command-list-command-item-wrapper');
Same question here.

On 2012/05/18 21:08:26, Evan Stade wrote:
> too much indent.

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_commands_overlay.css (right):

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_commands_overlay.css:18: /*
TODO(estade): apply this everywhere. */
On 2012/05/18 21:08:26, Evan Stade wrote:
> you don't need to copy this, it already exists due to
pack_extension_overlay.css

Oh, great! :)
Done.

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_commands_overlay.html
(right):

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_commands_overlay.html:1: <div
id="extensionCommandsOverlay" class="page">
On 2012/05/18 21:08:26, Evan Stade wrote:
> license

Done.

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_commands_overlay.html:3: <div
id="cbdContentArea" class="content-area">
Not needed. Removed.

On 2012/05/18 21:08:26, Evan Stade wrote:
> id should be dash-form

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_commands_overlay.html:4: <div
id="extension-command-list" class="empty-extension-commands-list"></div>
On 2012/05/18 21:08:26, Evan Stade wrote:
> wrap

Done.

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
File chrome/browser/resources/extensions/extensions.js (right):

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
chrome/browser/resources/extensions/extensions.js:8: <include
src="extension_commands_overlay.js"></include>
On 2012/05/18 21:08:26, Evan Stade wrote:
> alphabet?

Done.

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
File chrome/browser/resources/shared/js/load_time_data.js (right):

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
chrome/browser/resources/shared/js/load_time_data.js:47: // GetLocalizedValues()
in your handler class (in C++) is called.
Having gone through the exercise of trying to figure out why the messages aren't
showing up properly this is where the dead-end was reached. It wasn't obvious
where to go from here in the C++ code. 

Is there any generic comment we can use here instead (to point people in the
right direction) or should we just remove it?

On 2012/05/18 21:08:26, Evan Stade wrote:
> this comment is inaccurate, GetLocalizedValues() does not exist for all
> WebUIMessageHandler classes. I don't think it's worth including --- this isn't
> where I would expect most people to look for documentation.

[Evan Stade, 2012-05-18 22:14:22]

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
File chrome/browser/resources/extensions/extension_command_list.js (right):

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_command_list.js:35:
'.extension-command-list-extension-item-wrapper');
On 2012/05/18 22:02:04, Finnur wrote:
> Wait... Continuations from previous lines have always used 4 spaces for
> indentation, no?
> 
> foo +
>     bar;
> 
> not
> 
> foo +
>   bar;
> 
> right?

oh, I guess this one is actually 4

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
chrome/browser/resources/extensions/extension_command_list.js:57:
'.extension-command-list-command-item-wrapper');
On 2012/05/18 22:02:04, Finnur wrote:
> Same question here.
> 
> On 2012/05/18 21:08:26, Evan Stade wrote:
> > too much indent.
> 

yea but this is 6 not 4

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
File chrome/browser/resources/shared/js/load_time_data.js (right):

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
chrome/browser/resources/shared/js/load_time_data.js:47: // GetLocalizedValues()
in your handler class (in C++) is called.
not obvious where to go in the C++ code? You added the C++ code which attempted
but failed to set new strings, so I would have thought it pretty clear you have
to go there to see why it wasn't working.

On 2012/05/18 22:02:04, Finnur wrote:
> Having gone through the exercise of trying to figure out why the messages
aren't
> showing up properly this is where the dead-end was reached. It wasn't obvious
> where to go from here in the C++ code. 
> 
> Is there any generic comment we can use here instead (to point people in the
> right direction) or should we just remove it?
> 
> On 2012/05/18 21:08:26, Evan Stade wrote:
> > this comment is inaccurate, GetLocalizedValues() does not exist for all
> > WebUIMessageHandler classes. I don't think it's worth including --- this
isn't
> > where I would expect most people to look for documentation.
>

https://chromiumcodereview.appspot.com/10383240/diff/8004/chrome/browser/reso...
File chrome/browser/resources/extensions/extension_commands_overlay.html
(right):

https://chromiumcodereview.appspot.com/10383240/diff/8004/chrome/browser/reso...
chrome/browser/resources/extensions/extension_commands_overlay.html:5: -->
hmm, it seems I was wrong and we don't use license headers in other html files.
You can just remove it.

[Finnur, 2012-05-18 22:27:01]

Updated.

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
File chrome/browser/resources/shared/js/load_time_data.js (right):

https://chromiumcodereview.appspot.com/10383240/diff/1/chrome/browser/resourc...
chrome/browser/resources/shared/js/load_time_data.js:47: // GetLocalizedValues()
in your handler class (in C++) is called.
I added GetLocalizedValues() but overlooked the fact that you have to explicitly
call it. Then it looks like the C++ code is correct, but it isn't actually doing
anything so you blame the .js side of things. Anyway, I was just trying to help
the next guy. I'll just remove the comment. 

On 2012/05/18 22:14:23, Evan Stade wrote:
> not obvious where to go in the C++ code? You added the C++ code which
attempted
> but failed to set new strings, so I would have thought it pretty clear you
have
> to go there to see why it wasn't working.
> 
> On 2012/05/18 22:02:04, Finnur wrote:
> > Having gone through the exercise of trying to figure out why the messages
> aren't
> > showing up properly this is where the dead-end was reached. It wasn't
obvious
> > where to go from here in the C++ code. 
> > 
> > Is there any generic comment we can use here instead (to point people in the
> > right direction) or should we just remove it?
> > 
> > On 2012/05/18 21:08:26, Evan Stade wrote:
> > > this comment is inaccurate, GetLocalizedValues() does not exist for all
> > > WebUIMessageHandler classes. I don't think it's worth including --- this
> isn't
> > > where I would expect most people to look for documentation.
> >

[Evan Stade, 2012-05-18 22:31:34]

lgtm

[Aaron Boodman, 2012-05-19 05:33:25]

Low priority nit:

In general, a file in (browser|common|renderer)/extensions directory should be
named bar.h and be defined like:

namespace extensions {

class Bar {
};

}  // namespace

https://chromiumcodereview.appspot.com/10383240/diff/1028/chrome/browser/exte...
File chrome/browser/extensions/api/commands/extension_command_service.h (right):

https://chromiumcodereview.appspot.com/10383240/diff/1028/chrome/browser/exte...
chrome/browser/extensions/api/commands/extension_command_service.h:34: class
ExtensionCommandService : public ProfileKeyedService,
I may have already mentioned this, and it's a low priority, but:

extensions::CommandService - then you rename the file to be shorter, and whoever
owns this can call it just command_service.

https://chromiumcodereview.appspot.com/10383240/diff/1028/chrome/browser/exte...
chrome/browser/extensions/api/commands/extension_command_service.h:39: enum
ExtensionCommandQueryType {
Seems like QueryType is sufficient.

https://chromiumcodereview.appspot.com/10383240/diff/1028/chrome/browser/exte...
chrome/browser/extensions/api/commands/extension_command_service.h:59: // Gets
the keybinding (if any) for the browser action of an extension given
Did you mean to change this? Also, we're killing page actions. It probably makes
sense to skip supporting them with keybindings. Sorry I didn't realize this
earlier.

https://chromiumcodereview.appspot.com/10383240/diff/1028/chrome/browser/exte...
chrome/browser/extensions/api/commands/extension_command_service.h:77: void
GetAllCommands(base::DictionaryValue* commands);
Weird to have an interface that deals in Values here. Seems like the UI layer
should just call GetNamedCommands() and GetBrowserActionCommand() directly, and
handle the construction of the DictionaryValue itself.

https://chromiumcodereview.appspot.com/10383240/diff/1028/chrome/browser/exte...
chrome/browser/extensions/api/commands/extension_command_service.h:117:
base::DictionaryValue* CreateExtensionDetailValue(
Seems like this should be Command::ToValue().

https://chromiumcodereview.appspot.com/10383240/diff/1028/chrome/common/exten...
File chrome/common/extensions/extension_commands.h (left):

https://chromiumcodereview.appspot.com/10383240/diff/1028/chrome/common/exten...
chrome/common/extensions/extension_commands.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
Nit: This file could be called 'commands.h' for more shortness.

[Finnur, 2012-05-21 21:28:43]

https://chromiumcodereview.appspot.com/10383240/diff/1028/chrome/browser/exte...
File chrome/browser/extensions/api/commands/extension_command_service.h (right):

https://chromiumcodereview.appspot.com/10383240/diff/1028/chrome/browser/exte...
chrome/browser/extensions/api/commands/extension_command_service.h:34: class
ExtensionCommandService : public ProfileKeyedService,
How about a separate changelist, so as to not mess up the review diffs?

On 2012/05/19 05:33:26, Aaron Boodman wrote:
> I may have already mentioned this, and it's a low priority, but:
> 
> extensions::CommandService - then you rename the file to be shorter, and
whoever
> owns this can call it just command_service.

https://chromiumcodereview.appspot.com/10383240/diff/1028/chrome/browser/exte...
chrome/browser/extensions/api/commands/extension_command_service.h:39: enum
ExtensionCommandQueryType {
On 2012/05/19 05:33:26, Aaron Boodman wrote:
> Seems like QueryType is sufficient.

Done.

https://chromiumcodereview.appspot.com/10383240/diff/1028/chrome/browser/exte...
chrome/browser/extensions/api/commands/extension_command_service.h:59: // Gets
the keybinding (if any) for the browser action of an extension given
Did not mean to change it. I'm hesitant to remove the code on the basis that we
might remove support for page actions at some point in the future. It would be
kind of weird to only support browser actions in the interim.

On 2012/05/19 05:33:26, Aaron Boodman wrote:
> Did you mean to change this? Also, we're killing page actions. It probably
makes
> sense to skip supporting them with keybindings. Sorry I didn't realize this
> earlier.

https://chromiumcodereview.appspot.com/10383240/diff/1028/chrome/browser/exte...
chrome/browser/extensions/api/commands/extension_command_service.h:77: void
GetAllCommands(base::DictionaryValue* commands);
On 2012/05/19 05:33:26, Aaron Boodman wrote:
> Weird to have an interface that deals in Values here. Seems like the UI layer
> should just call GetNamedCommands() and GetBrowserActionCommand() directly,
and
> handle the construction of the DictionaryValue itself.

Done.

https://chromiumcodereview.appspot.com/10383240/diff/1028/chrome/browser/exte...
chrome/browser/extensions/api/commands/extension_command_service.h:117:
base::DictionaryValue* CreateExtensionDetailValue(
On 2012/05/19 05:33:26, Aaron Boodman wrote:
> Seems like this should be Command::ToValue().

Done.

https://chromiumcodereview.appspot.com/10383240/diff/1028/chrome/common/exten...
File chrome/common/extensions/extension_commands.h (left):

https://chromiumcodereview.appspot.com/10383240/diff/1028/chrome/common/exten...
chrome/common/extensions/extension_commands.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
Sure. OK for followup changelist?

On 2012/05/19 05:33:26, Aaron Boodman wrote:
> Nit: This file could be called 'commands.h' for more shortness.