Add user customizable launch type for apps.

o Add user customizable launch type for apps by adding options in each apps context menu.
o Updated some comments that were using the outdated NOTIFY_PREF_CHANGED notification.
o Make LAUNCH_PINNED the default type returned by ExtensionPrefs if it does not already exist.
o Some minor refactoring within the code to reduce duplication.

BUG=54731
TEST=NONE

patch from issue 3419010

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

Reverted: http://crrev.com/61000

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

[arv (Not doing code reviews), 2010-09-28 01:12:35]

Andy, thanks for taking over.

I can't see the diffs in the last three files in this patch

http://codereview.chromium.org/3453029/diff/9001/10001
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/3453029/diff/9001/10001#newcode589
chrome/browser/browser.cc:589: TabContents* tab_contents =
local_browser->AddTabWithURL(
Is this really the desired behavior? Don't we want a new window for this?

http://codereview.chromium.org/3453029/diff/9001/10003
File chrome/browser/dom_ui/app_launcher_handler.cc (right):

http://codereview.chromium.org/3453029/diff/9001/10003#newcode62
chrome/browser/dom_ui/app_launcher_handler.cc:62: kExtensionsPref, this);
indentation

http://codereview.chromium.org/3453029/diff/9001/10011
File chrome/browser/resources/ntp/apps.js (right):

http://codereview.chromium.org/3453029/diff/9001/10011#newcode42
chrome/browser/resources/ntp/apps.js:42: document.querySelector('.app a[app-id='
+ app['id'] + ']').setAttribute(
Maybe you should ensure that querySelector does not return null. I'm not sure we
always create the DOM elements for these.

[Bons, 2010-09-28 16:08:19]

Thanks for the quick pass, arv. Will address code comments when I get into the
office (WFH/Email this morning).

The last three files can be viewed by viewing the unified diff. The side-by-side
issue is a reitveld thing (there is a bug still open for it).

http://codereview.chromium.org/3453029/diff/9001/10001
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/3453029/diff/9001/10001#newcode589
chrome/browser/browser.cc:589: TabContents* tab_contents =
local_browser->AddTabWithURL(
On 2010/09/28 01:12:36, arv wrote:
> Is this really the desired behavior? Don't we want a new window for this?

<shrug> After speaking with Aaron, this is what we decided for now.

[arv (Not doing code reviews), 2010-09-28 16:16:23]

The last 3 files look good to me :-)

[Erik does not do reviews, 2010-09-28 16:54:37]

http://codereview.chromium.org/3453029/diff/9001/10001
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/3453029/diff/9001/10001#newcode579
chrome/browser/browser.cc:579: case ExtensionPrefs::LAUNCH_PINNED:
add default: and just put the logic right here rather than falling through

http://codereview.chromium.org/3453029/diff/9001/10001#newcode596
chrome/browser/browser.cc:596: return local_browser->AddTabWithURL(
make this the default: case for clarity

http://codereview.chromium.org/3453029/diff/9001/10003
File chrome/browser/dom_ui/app_launcher_handler.cc (right):

http://codereview.chromium.org/3453029/diff/9001/10003#newcode61
chrome/browser/dom_ui/app_launcher_handler.cc:61:
extensions_service_->extension_prefs()->pref_service()->RemovePrefObserver(
I'm a bit nervous about this.  What's the lifetime of this object?  Could it be
deleted during browser shutdown?  If so, is it guaranteed that all of these
objects will still be around and valid?

http://codereview.chromium.org/3453029/diff/9001/10006
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/3453029/diff/9001/10006#newcode15
chrome/browser/extensions/extension_prefs.cc:15: // A preference that keeps
track of per-extension settings. This is a dictionary
remove duplicated comment

http://codereview.chromium.org/3453029/diff/9001/10006#newcode453
chrome/browser/extensions/extension_prefs.cc:453: *out_value =
static_cast<LaunchType>(value);
static_cast won't verify that value is in range, so you'll need to do it
manually

http://codereview.chromium.org/3453029/diff/9001/10007
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/3453029/diff/9001/10007#newcode19
chrome/browser/extensions/extension_prefs.h:19: // A preference that keeps track
of per-extension settings. This is a dictionary
"key name for a preference that..."

http://codereview.chromium.org/3453029/diff/9001/10007#newcode20
chrome/browser/extensions/extension_prefs.h:20: // object read from the
Preferences file, keyed off of extension id's.
nit: id's -> ids

http://codereview.chromium.org/3453029/diff/9001/10007#newcode21
chrome/browser/extensions/extension_prefs.h:21: extern const char
kExtensionsPref[];
why not make this a static member of ExtensionPrefs?

http://codereview.chromium.org/3453029/diff/9001/10007#newcode133
chrome/browser/extensions/extension_prefs.h:133: void GetLaunchType(const
std::string& extension_id, LaunchType* out_value);
why does this need to be an out param?

http://codereview.chromium.org/3453029/diff/9001/10007#newcode222
chrome/browser/extensions/extension_prefs.h:222: // Additionally fires a
PREF_CHANGED notification with the top-level
why is this manual notification necessary?  doesn't PrefService do this
automatically?

[Aaron Boodman, 2010-09-28 19:09:49]

http://codereview.chromium.org/3453029/diff/9001/10003
File chrome/browser/dom_ui/app_launcher_handler.cc (right):

http://codereview.chromium.org/3453029/diff/9001/10003#newcode61
chrome/browser/dom_ui/app_launcher_handler.cc:61:
extensions_service_->extension_prefs()->pref_service()->RemovePrefObserver(
On 2010/09/28 16:54:37, Erik Kay wrote:
> I'm a bit nervous about this.  What's the lifetime of this object?  Could it
be
> deleted during browser shutdown?  If so, is it guaranteed that all of these
> objects will still be around and valid?

Seems legit to me. ExtensionsService and PrefService are owned by Profile, which
is supposed to be deleted after all the UI objects. So this should never run
with invalid ExtensionsService or PrefService IMO.

[Bons, 2010-09-28 19:54:27]

http://codereview.chromium.org/3453029/diff/9001/10001
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/3453029/diff/9001/10001#newcode579
chrome/browser/browser.cc:579: case ExtensionPrefs::LAUNCH_PINNED:
On 2010/09/28 16:54:37, Erik Kay wrote:
> add default: and just put the logic right here rather than falling through

Done.

http://codereview.chromium.org/3453029/diff/9001/10001#newcode596
chrome/browser/browser.cc:596: return local_browser->AddTabWithURL(
On 2010/09/28 16:54:37, Erik Kay wrote:
> make this the default: case for clarity

Done.

http://codereview.chromium.org/3453029/diff/9001/10003
File chrome/browser/dom_ui/app_launcher_handler.cc (right):

http://codereview.chromium.org/3453029/diff/9001/10003#newcode62
chrome/browser/dom_ui/app_launcher_handler.cc:62: kExtensionsPref, this);
On 2010/09/28 01:12:36, arv wrote:
> indentation

Done.

http://codereview.chromium.org/3453029/diff/9001/10006
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/3453029/diff/9001/10006#newcode15
chrome/browser/extensions/extension_prefs.cc:15: // A preference that keeps
track of per-extension settings. This is a dictionary
On 2010/09/28 16:54:37, Erik Kay wrote:
> remove duplicated comment

Done.

http://codereview.chromium.org/3453029/diff/9001/10006#newcode453
chrome/browser/extensions/extension_prefs.cc:453: *out_value =
static_cast<LaunchType>(value);
On 2010/09/28 16:54:37, Erik Kay wrote:
> static_cast won't verify that value is in range, so you'll need to do it
> manually

Done.

http://codereview.chromium.org/3453029/diff/9001/10007
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/3453029/diff/9001/10007#newcode19
chrome/browser/extensions/extension_prefs.h:19: // A preference that keeps track
of per-extension settings. This is a dictionary
On 2010/09/28 16:54:37, Erik Kay wrote:
> "key name for a preference that..."

Done.

http://codereview.chromium.org/3453029/diff/9001/10007#newcode20
chrome/browser/extensions/extension_prefs.h:20: // object read from the
Preferences file, keyed off of extension id's.
On 2010/09/28 16:54:37, Erik Kay wrote:
> nit: id's -> ids

Done.

http://codereview.chromium.org/3453029/diff/9001/10007#newcode21
chrome/browser/extensions/extension_prefs.h:21: extern const char
kExtensionsPref[];
On 2010/09/28 16:54:37, Erik Kay wrote:
> why not make this a static member of ExtensionPrefs?

Done.

http://codereview.chromium.org/3453029/diff/9001/10007#newcode133
chrome/browser/extensions/extension_prefs.h:133: void GetLaunchType(const
std::string& extension_id, LaunchType* out_value);
On 2010/09/28 16:54:37, Erik Kay wrote:
> why does this need to be an out param?

Done.

http://codereview.chromium.org/3453029/diff/9001/10007#newcode222
chrome/browser/extensions/extension_prefs.h:222: // Additionally fires a
PREF_CHANGED notification with the top-level
On 2010/09/28 16:54:37, Erik Kay wrote:
> why is this manual notification necessary?  doesn't PrefService do this
> automatically?
Actually, no. ExtensionPrefs is subverting a lot of the code used by PrefService
such as SetUserPref which in turn triggers the notification. We grab the mutable
dictionary and save the prefs ourselves within this class.

FWIW this should change.

http://codereview.chromium.org/3453029/diff/9001/10011
File chrome/browser/resources/ntp/apps.js (right):

http://codereview.chromium.org/3453029/diff/9001/10011#newcode42
chrome/browser/resources/ntp/apps.js:42: document.querySelector('.app a[app-id='
+ app['id'] + ']').setAttribute(
On 2010/09/28 01:12:36, arv wrote:
> Maybe you should ensure that querySelector does not return null. I'm not sure
we
> always create the DOM elements for these.

Done.

[Erik does not do reviews, 2010-09-28 22:34:06]

LGTM

http://codereview.chromium.org/3453029/diff/9001/10007
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/3453029/diff/9001/10007#newcode222
chrome/browser/extensions/extension_prefs.h:222: // Additionally fires a
PREF_CHANGED notification with the top-level
On 2010/09/28 19:54:27, Andrew Bonventre (Bons) wrote:
> On 2010/09/28 16:54:37, Erik Kay wrote:
> > why is this manual notification necessary?  doesn't PrefService do this
> > automatically?
> Actually, no. ExtensionPrefs is subverting a lot of the code used by
PrefService
> such as SetUserPref which in turn triggers the notification. We grab the
mutable
> dictionary and save the prefs ourselves within this class.
> 
> FWIW this should change.
> 

OK.  Could you explain this in the comment a bit more clearly.  Is it a second
TODO item?

[Aaron Boodman, 2010-09-28 22:36:37]

http://codereview.chromium.org/3453029/diff/45001/46003
File chrome/browser/dom_ui/app_launcher_handler.cc (right):

http://codereview.chromium.org/3453029/diff/45001/46003#newcode121
chrome/browser/dom_ui/app_launcher_handler.cc:121: 
These blank lines aren't necessary. You could remove them all, or put
launch_container and launch_type up with the rest of the values and leave the
blank before icon_*, since those are huger.

http://codereview.chromium.org/3453029/diff/45001/46006
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/3453029/diff/45001/46006#newcode171
chrome/browser/extensions/extension_prefs.cc:171: SavePrefsAndNotify();
Nice cleanup.

http://codereview.chromium.org/3453029/diff/45001/46006#newcode453
chrome/browser/extensions/extension_prefs.cc:453: value == LAUNCH_PINNED ||
This formatting looks weird. Not sure what it should look like, but I don't
think this is it.

http://codereview.chromium.org/3453029/diff/45001/46007
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/3453029/diff/45001/46007#newcode132
chrome/browser/extensions/extension_prefs.h:132: // Returns LAUNCH_PINNED if the
pref doesn't exist or could not be read.
Nit: I don't think it is necessary to comment on what the current default is. We
might change it.

http://codereview.chromium.org/3453029/diff/45001/46011
File chrome/browser/resources/ntp/apps.js (right):

http://codereview.chromium.org/3453029/diff/45001/46011#newcode113
chrome/browser/resources/ntp/apps.js:113: var LaunchType = {
What about an attribute on the command like launch_type="1". That way the
commandToType map is not necessary, I don't think.

http://codereview.chromium.org/3453029/diff/45001/46011#newcode152
chrome/browser/resources/ntp/apps.js:152: app['launch_container'] ||
LaunchContainer.LAUNCH_TAB;
Unfortunate to duplicate the default launch container here. It seems like
app['launch_container'] will never be empty. In that case, there is no need to
default it.

[Bons, 2010-09-29 17:55:21]

Changes made.

http://codereview.chromium.org/3453029/diff/9001/10007
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/3453029/diff/9001/10007#newcode222
chrome/browser/extensions/extension_prefs.h:222: // Additionally fires a
PREF_CHANGED notification with the top-level
On 2010/09/28 22:34:07, Erik Kay wrote:
> On 2010/09/28 19:54:27, Andrew Bonventre (Bons) wrote:
> > On 2010/09/28 16:54:37, Erik Kay wrote:
> > > why is this manual notification necessary?  doesn't PrefService do this
> > > automatically?
> > Actually, no. ExtensionPrefs is subverting a lot of the code used by
> PrefService
> > such as SetUserPref which in turn triggers the notification. We grab the
> mutable
> > dictionary and save the prefs ourselves within this class.
> > 
> > FWIW this should change.
> > 
> 
> OK.  Could you explain this in the comment a bit more clearly.  Is it a second
> TODO item?
> 

Done.

http://codereview.chromium.org/3453029/diff/45001/46003
File chrome/browser/dom_ui/app_launcher_handler.cc (right):

http://codereview.chromium.org/3453029/diff/45001/46003#newcode121
chrome/browser/dom_ui/app_launcher_handler.cc:121: 
On 2010/09/28 22:36:37, Aaron Boodman wrote:
> These blank lines aren't necessary. You could remove them all, or put
> launch_container and launch_type up with the rest of the values and leave the
> blank before icon_*, since those are huger.

Done.

http://codereview.chromium.org/3453029/diff/45001/46006
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/3453029/diff/45001/46006#newcode171
chrome/browser/extensions/extension_prefs.cc:171: SavePrefsAndNotify();
On 2010/09/28 22:36:37, Aaron Boodman wrote:
> Nice cleanup.
Done.

:)

http://codereview.chromium.org/3453029/diff/45001/46007
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/3453029/diff/45001/46007#newcode132
chrome/browser/extensions/extension_prefs.h:132: // Returns LAUNCH_PINNED if the
pref doesn't exist or could not be read.
On 2010/09/28 22:36:37, Aaron Boodman wrote:
> Nit: I don't think it is necessary to comment on what the current default is.
We
> might change it.

Done.

http://codereview.chromium.org/3453029/diff/45001/46011
File chrome/browser/resources/ntp/apps.js (right):

http://codereview.chromium.org/3453029/diff/45001/46011#newcode152
chrome/browser/resources/ntp/apps.js:152: app['launch_container'] ||
LaunchContainer.LAUNCH_TAB;
On 2010/09/28 22:36:37, Aaron Boodman wrote:
> Unfortunate to duplicate the default launch container here. It seems like
> app['launch_container'] will never be empty. In that case, there is no need to
> default it.

Done.

[Aaron Boodman, 2010-09-29 21:14:10]

lgtm