Make HelpApp component extension.

Make HelpApp component extension.

BUG=http://code.google.com/p/chromium-os/issues/detail?id=6923
TEST=manual

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

[Aaron Boodman, 2010-12-10 18:38:22]

You can use any string for the key. We tend to use public keys just for
consistency. The easiest way to get a string in the same format is to use
chrome://extensions/ -> developer mode to pack the folder containing the
extension into a crx. Then install that CRX and find the key in your Preferences
file.

http://codereview.chromium.org/5758002/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/5758002/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_impl.cc:416: FilePath
path("/usr/share/chromeos-assets/helpapp");
Can you generalize the std::pair<> above into a struct and give it a constructor
that accepts a path argument? That way all the registration stays in one place.

[Dmitry Polukhin, 2010-12-13 16:17:33]

PTAL

http://codereview.chromium.org/5758002/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/5758002/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_impl.cc:416: FilePath
path("/usr/share/chromeos-assets/helpapp");
On 2010/12/10 18:38:22, Aaron Boodman wrote:
> Can you generalize the std::pair<> above into a struct and give it a
constructor
> that accepts a path argument? That way all the registration stays in one
place.

I did it a bit different but kept registration only in one place. We already
have structs for similar propose (ComponentExtensionInfo) so there is no sense
to create one more that will be used only in one function.

[Aaron Boodman, 2010-12-13 16:29:43]

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

http://codereview.chromium.org/5758002/diff/3001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:404: if (!path.IsAbsolute()) {
Why this change? It seems if anything, we should CHECK that we never get a
relative path, not silently ignore them.

http://codereview.chromium.org/5758002/diff/3001/chrome/browser/resources/hel...
File chrome/browser/resources/help_app/manifest.json (right):

http://codereview.chromium.org/5758002/diff/3001/chrome/browser/resources/hel...
chrome/browser/resources/help_app/manifest.json:3: "name": "__MSG_extName__",
Is the help app localized? (does it has a _locales folder?). Otherwise this
string will show up literally in the UI.

[Dmitry Polukhin, 2010-12-13 19:52:38]

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

http://codereview.chromium.org/5758002/diff/3001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:404: if (!path.IsAbsolute()) {
On 2010/12/13 16:29:43, Aaron Boodman wrote:
> Why this change? It seems if anything, we should CHECK that we never get a
> relative path, not silently ignore them.

As you can see from the code all other component extensions uses relative paths
to resource dir that is added as prefix in this loop. If the path is already
full, don't add resource dir as prefix.

http://codereview.chromium.org/5758002/diff/3001/chrome/browser/resources/hel...
File chrome/browser/resources/help_app/manifest.json (right):

http://codereview.chromium.org/5758002/diff/3001/chrome/browser/resources/hel...
chrome/browser/resources/help_app/manifest.json:3: "name": "__MSG_extName__",
On 2010/12/13 16:29:43, Aaron Boodman wrote:
> Is the help app localized? (does it has a _locales folder?). Otherwise this
> string will show up literally in the UI.

Yes, you can see sources here https://chromereview.prom.corp.google.com/2134023

[Aaron Boodman, 2010-12-13 20:06:43]

lgtm

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

http://codereview.chromium.org/5758002/diff/3001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:404: if (!path.IsAbsolute()) {
On 2010/12/13 19:52:38, Dmitry Polukhin wrote:
> On 2010/12/13 16:29:43, Aaron Boodman wrote:
> > Why this change? It seems if anything, we should CHECK that we never get a
> > relative path, not silently ignore them.
> 
> As you can see from the code all other component extensions uses relative
paths
> to resource dir that is added as prefix in this loop. If the path is already
> full, don't add resource dir as prefix.

Ah.

[Dmitry Polukhin, 2010-12-14 13:25:11]

FYI, had to change char type for paths to make it work for all platforms
(previous variant didn't worked on Window where char type is wchar_t).

[Dmitry Polukhin, 2010-12-14 15:14:12]

Chrome tests don't like new component extensions that are running in background.
15 tests count number of running things - it makes them unstable if you add or
remove new component extensions. It looks like HelpApp shouldn't have background
components so I blocked on
http://code.google.com/p/chromium-os/issues/detail?id=10105

[Dmitry Polukhin, 2011-01-24 12:42:46]

Aaron, please take another look to this CL. I have to change more files because
new version of HelpApp has browser_action so tests starts fail because they
don't expect any browser actions by default. Moreover context menu shouldn't be
shown for component extensions because no one action in context menu is
applicable to component extension.

[Aaron Boodman, 2011-01-24 19:45:58]

If you decide to implement Extension::ShowConfigureContextMenus() in this
change, please send back to me for another review.

If you don't, this LGTM, but please do ShowConfigureContextMenus() in a
different change.

Thanks!

http://codereview.chromium.org/5758002/diff/26001/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/5758002/diff/26001/chrome/browser/browser_main...
chrome/browser/browser_main.cc:1066: if (parameters.ui_task)
You already know how I feel about doing things differently in tests :|.

http://codereview.chromium.org/5758002/diff/26001/chrome/browser/browser_reso...
File chrome/browser/browser_resources.grd (right):

http://codereview.chromium.org/5758002/diff/26001/chrome/browser/browser_reso...
chrome/browser/browser_resources.grd:104: <include name="IDR_HELP_MANIFEST"
file="resources\help_app\manifest.json" flattenhtml="true" type="BINDATA" />
I don't think you want flattenhtml="true" here.

http://codereview.chromium.org/5758002/diff/26001/chrome/browser/defaults.h
File chrome/browser/defaults.h (right):

http://codereview.chromium.org/5758002/diff/26001/chrome/browser/defaults.h#n...
chrome/browser/defaults.h:87: // Are HelpApp enabled? True by default. Has sense
only for Chrome OS now.
Whether HelpApp is enabled. True by default. This is only used by Chrome OS
today.

http://codereview.chromium.org/5758002/diff/26001/chrome/browser/ui/views/bro...
File chrome/browser/ui/views/browser_actions_container.cc (right):

http://codereview.chromium.org/5758002/diff/26001/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_actions_container.cc:256: if
(extension()->location() == Extension::COMPONENT)
I could see 'options' making sense, but I think this is a good change for now.

What about page actions? They will have the same problem.

How about adding a method to Extension like ::ShowConfigureContextMenus() and
checking that for both page and browser actions.

You can do this in a separate change if you are worried about not being able to
get it done in time for your branch point.

[Dmitry Polukhin, 2011-01-25 07:23:20]

Thank you for review!

http://codereview.chromium.org/5758002/diff/26001/chrome/browser/browser_reso...
File chrome/browser/browser_resources.grd (right):

http://codereview.chromium.org/5758002/diff/26001/chrome/browser/browser_reso...
chrome/browser/browser_resources.grd:104: <include name="IDR_HELP_MANIFEST"
file="resources\help_app\manifest.json" flattenhtml="true" type="BINDATA" />
On 2011/01/24 19:45:58, Aaron Boodman wrote:
> I don't think you want flattenhtml="true" here.

Done.

http://codereview.chromium.org/5758002/diff/26001/chrome/browser/defaults.h
File chrome/browser/defaults.h (right):

http://codereview.chromium.org/5758002/diff/26001/chrome/browser/defaults.h#n...
chrome/browser/defaults.h:87: // Are HelpApp enabled? True by default. Has sense
only for Chrome OS now.
On 2011/01/24 19:45:58, Aaron Boodman wrote:
> Whether HelpApp is enabled. True by default. This is only used by Chrome OS
> today.

Done.

http://codereview.chromium.org/5758002/diff/26001/chrome/browser/ui/views/bro...
File chrome/browser/ui/views/browser_actions_container.cc (right):

http://codereview.chromium.org/5758002/diff/26001/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_actions_container.cc:256: if
(extension()->location() == Extension::COMPONENT)
On 2011/01/24 19:45:58, Aaron Boodman wrote:
> I could see 'options' making sense, but I think this is a good change for now.

Yes, options would make sense for some component extensions. But HelpApp has no
options at the moment. Showing whole menu with only 1 active item looks odd. It
needs to be revised somehow for component extensions.

> What about page actions? They will have the same problem.

I didn't make it. Also the same changes need to be done for Win,Linux,Mac - now
I did it only for Chrome OS.

> How about adding a method to Extension like ::ShowConfigureContextMenus() and
> checking that for both page and browser actions.
> 
> You can do this in a separate change if you are worried about not being able
to
> get it done in time for your branch point.

I like this idea and will send separate CL with such changes.