ntp4: add app context menu

ntp4: add app context menu

BUG=none
TEST=manual (see below)
1: all menu items work
2. middle clicking or ctrl-clicking on an app affects the launch type
3: when you change the app launch type, that change is visible both on the current NTP and all other currently open NTP tabs

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

[csilv, 2011-06-22 21:21:29]

http://codereview.chromium.org/7215035/diff/1001/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/7215035/diff/1001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:42: this.launchNewWindow_ =
this.appendMenuItem_('applaunchtypewindow');
launchNewWindow is not supported on Mac.  the apps callback dictionary will
contain a boolean value 'disableAppWindowLaunch' set to true if this is the
case.

http://codereview.chromium.org/7215035/diff/1001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:48: this.createShortcut_ =
this.appendMenuItem_('appcreateshortcut');
createShortcut is not support on Mac and ChromeOS.  the apps callback dictionary
will contain a boolean value 'disableCreateAppShortcut' set to true if this is
the case.

http://codereview.chromium.org/7215035/diff/1001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:73: */
add @private to comment

http://codereview.chromium.org/7215035/diff/1001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:88: */
add @private to doc comment

http://codereview.chromium.org/7215035/diff/1001/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

http://codereview.chromium.org/7215035/diff/1001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:424:
scoped_ptr<AutoReset<bool> > auto_reset;
I assume that space is needed to prevent the the compiler from interpreting that
as an '>>' operator?  If so... ick. (No change required, just commentary).

[Evan Stade, 2011-06-23 01:07:30]

updated

[csilv, 2011-06-23 01:18:12]

LGTM

[Rick Byers, 2011-06-23 17:03:36]

LGTM with a couple minor suggestions

http://codereview.chromium.org/7215035/diff/4002/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/7215035/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:36: this.menu = menu;
Did you intend this to be public?  this.menu_ instead perhaps?

http://codereview.chromium.org/7215035/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:38: this.launch_ =
this.appendMenuItem_();
personally I like seeing the list somewhere of the fields on a class with
documentation for them (ideally including their type) - eg. see all the fields
at the beginning of TouchHandler.prototype (that way I can understand code in
other functions that uses properties by reading the documentation for those
properties - rather than finding where they are written).  

But I think this is more of Google internal style than Chrome style - I'm not
sure how often we follow that in chrome WebUI generally, so I'll leave it up to
you which direction we should go with the NTP4 code to make it consistent.

http://codereview.chromium.org/7215035/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:221: args.push(e.altKey, e.ctrlKey,
e.metaKey, e.shiftKey, e.button);
why initialize the array with some and push the other args?  Seems simpler to
just initialize the array with all the args.

[Evan Stade, 2011-06-23 20:32:08]

since I already committed, I will make the fixes in a follow up

http://codereview.chromium.org/7215035/diff/4002/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/7215035/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:36: this.menu = menu;
On 2011/06/23 17:03:36, rbyers wrote:
> Did you intend this to be public?  this.menu_ instead perhaps?

will do

http://codereview.chromium.org/7215035/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:38: this.launch_ =
this.appendMenuItem_();
On 2011/06/23 17:03:36, rbyers wrote:
> personally I like seeing the list somewhere of the fields on a class with
> documentation for them (ideally including their type) - eg. see all the fields
> at the beginning of TouchHandler.prototype (that way I can understand code in
> other functions that uses properties by reading the documentation for those
> properties - rather than finding where they are written).  
> 
> But I think this is more of Google internal style than Chrome style - I'm not
> sure how often we follow that in chrome WebUI generally, so I'll leave it up
to
> you which direction we should go with the NTP4 code to make it consistent.

We don't really do it in chrome's webui code in general. I saw that you did this
in some (all?) places for the original ntp4 (touch ntp). I agree documentation
is nice. However I also think rigorously applying documentation to all member
vars can be cumbersome and overly verbose in cases where the type/meaning is
obvious.

Way back before I was born, in c, you had to declare all local variables at the
start of a function body. Then in later c compilers this restriction was
relaxed, and today in our c++ style guide we recommend declaring the variable
close to where it is used. I see the declaration of member variables within the
constructor as a further continuation of this idea, which javascript makes
possible even though it's not possible in c++. It could get quite messy if you
declare member variables all over the place, but I think (hope) I've kept it all
within initialize(), which seems like a good compromise.

imho

http://codereview.chromium.org/7215035/diff/4002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:221: args.push(e.altKey, e.ctrlKey,
e.metaKey, e.shiftKey, e.button);
On 2011/06/23 17:03:36, rbyers wrote:
> why initialize the array with some and push the other args?  Seems simpler to
> just initialize the array with all the args.

you are right. I copied this code from ntp3, and simplified it a bit, but
obviously missed this. (Originally the args.push was the extra args were
conditional)

[Rick Byers, 2011-06-23 20:42:43]

On 2011/06/23 20:32:08, Evan Stade wrote:
> since I already committed, I will make the fixes in a follow up

Ah, sorry.  I started reviewing this morning before you'd be in pacific time but
didn't get it done by lunch and missed that it went in in the meantime.

http://codereview.chromium.org/7215035/diff/4002/chrome/browser/resources/ntp...
> chrome/browser/resources/ntp4/apps_page.js:38: this.launch_ =
> this.appendMenuItem_();
> On 2011/06/23 17:03:36, rbyers wrote:
> > personally I like seeing the list somewhere of the fields on a class with
> > documentation for them (ideally including their type) - eg. see all the
fields
> > at the beginning of TouchHandler.prototype (that way I can understand code
in
> > other functions that uses properties by reading the documentation for those
> > properties - rather than finding where they are written).  
> > 
> > But I think this is more of Google internal style than Chrome style - I'm
not
> > sure how often we follow that in chrome WebUI generally, so I'll leave it up
> to
> > you which direction we should go with the NTP4 code to make it consistent.
> 
> We don't really do it in chrome's webui code in general. I saw that you did
this
> in some (all?) places for the original ntp4 (touch ntp). I agree documentation
> is nice. However I also think rigorously applying documentation to all member
> vars can be cumbersome and overly verbose in cases where the type/meaning is
> obvious.
> 
> Way back before I was born, in c, you had to declare all local variables at
the
> start of a function body. Then in later c compilers this restriction was
> relaxed, and today in our c++ style guide we recommend declaring the variable
> close to where it is used. I see the declaration of member variables within
the
> constructor as a further continuation of this idea, which javascript makes
> possible even though it's not possible in c++. It could get quite messy if you
> declare member variables all over the place, but I think (hope) I've kept it
all
> within initialize(), which seems like a good compromise.

Yep, sounds fine.  I definitely agree that it's silly to declare separately when
initializing in the ctor (I think my original code only declares on the
prototype when the ctor doesn't initialize a value).  Still nice to have
somewhere for the documentation though (on prototype or in ctor), but I agree it
can get quite tedious.

>
http://codereview.chromium.org/7215035/diff/4002/chrome/browser/resources/ntp...
> chrome/browser/resources/ntp4/apps_page.js:221: args.push(e.altKey, e.ctrlKey,
> e.metaKey, e.shiftKey, e.button);
> On 2011/06/23 17:03:36, rbyers wrote:
> > why initialize the array with some and push the other args?  Seems simpler
to
> > just initialize the array with all the args.
> 
> you are right. I copied this code from ntp3, and simplified it a bit, but
> obviously missed this. (Originally the args.push was the extra args were
> conditional)

Makes sense, no big deal.