Added Background Application entries to Dock menu

Added Background Application entries to Dock menu

BUG=67076
TEST=manual

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

[The wrong rickcam account, 2010-12-16 01:17:09]

Please take a look.

[The wrong rickcam account, 2010-12-16 01:22:26]

Fixed a bit of indentation.  Please take a look.

http://codereview.chromium.org/5908003/diff/3001/chrome/browser/app_controlle...
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/5908003/diff/3001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:1211: for (ExtensionList::const_iterator
cursor = applications.begin();
Need to indent the following two lines a bit more.

[Andrew T Wilson (Slow), 2010-12-16 02:18:24]

http://codereview.chromium.org/5908003/diff/3001/chrome/browser/app_controlle...
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/5908003/diff/3001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:944: [self executeApplication:sender];
Is there any assert we could have here to make sure the value is in the expected
range? What I don't want to have happen is someone adds a menu item/kbd
shortcut, but doesn't update this routine, and then it dies off in
executeApplication() with some confusing error rather than here with a nice
error message.

Also, I think we need a break; statement (superfluous, but I seem to see it used
in most/all places0.

http://codereview.chromium.org/5908003/diff/3001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:1203: NSString* titleStr = NULL;
Should we use nil instead of NULL here? Not sure about the style guide for objc.

http://codereview.chromium.org/5908003/diff/3001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:1205: 
I was confused by us using titleStr/item up here at the top level, but then
setting it in the for loop below since that pattern is usually only used when
values from inside the loop need to be used outside the loop.

It might be easier reverting your change to these lines (just move the
definition of these variables below the for loop) and define your own NSString
and item variables in the for loop itself (in fact, I'm not sure you even need a
titleStr variable if you fold together a couple of lines of code).

http://codereview.chromium.org/5908003/diff/3001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:1210: if (applications.begin() !=
applications.end()) {
I wonder if it's cleaner to move the for loop outside the if statement (that way
you can just have a simple if statement after the for loop):

// Only add separator if there are application items
if (applications.begin() != applications.end()) {
  NSMenuItem* sepItem = [[NSMenuItem separatorItem] init];
  [dockMenu addItem:sepItem];
}

As it stands, the purpose of the if-statement (guarding the addition of the
separator) is separated from the if statement itself.

http://codereview.chromium.org/5908003/diff/3001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:1215: DCHECK(sort_position == position);
I dimly recall talking about this before - why can't this be:

DCHECK(position == applications.GetPosition(*cursor))

That way in non-debug mode we don't call GetPosition()?

Or, just get rid of the position variable and always use GetPosition()?

http://codereview.chromium.org/5908003/diff/3001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:1217: string16 label = ASCIIToUTF16(name);
This seems weird to me. Should this be UTF8ToUTF16(name)?

http://codereview.chromium.org/5908003/diff/3001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:1218: titleStr =
l10n_util::FixUpWindowsStyleLabel(label);
I'm not sure that we should call this - I think this strips & out of the string
or something, which may not be appropriate here for extension names (i.e. if I
have an extension named "Nuts&Bolts" I probably don't want it converted to
"NutsBolts".

http://codereview.chromium.org/5908003/diff/3001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:1220: action:@selector(newWindowFromDock:)
BTW, I'm wondering if newWindowFromDock should be renamed "commandFromDock",
since we do other things other than create new windows now from dock items?

http://codereview.chromium.org/5908003/diff/3001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:1226: titleStr =
l10n_util::GetNSStringWithFixup(IDS_NEW_WINDOW_MAC);
Remove this superfluous set of titleStr.

[The wrong rickcam account, 2010-12-16 18:50:58]

Please take another look.

http://codereview.chromium.org/5908003/diff/3001/chrome/browser/app_controlle...
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/5908003/diff/3001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:944: [self executeApplication:sender];
On 2010/12/16 02:18:25, Andrew T Wilson wrote:
> Is there any assert we could have here to make sure the value is in the
expected
> range? What I don't want to have happen is someone adds a menu item/kbd
> shortcut, but doesn't update this routine, and then it dies off in
> executeApplication() with some confusing error rather than here with a nice
> error message.
> 
> Also, I think we need a break; statement (superfluous, but I seem to see it
used
> in most/all places0.

Done . . . provided that you like the approach that I took.  I introduced
IDC_MinimumLabelValue near the top of chrome_commands_ids.h and I now compare
against that value before sending self the executeApplication message.

I'll note that this also implies that there should be logic to limit the total
number of BackgroundApplications to be less than that value (currently 4000). 
In fact, I'll bet that one could make a reasonable argument that the whole
"command id" thing should be reworked a bit to accomodate this sort of thing. 
However, I do not plan to make that argument at this time :-)

Also, I've added the break, though my unscientific scan of the first dozen or so
hits on "find . -type f -name '*.cc' -print0 | xargs -0 grep -n 'default:'"
makes me unsure whether "break" or "no break" is more common in default:
sections of a switch.

http://codereview.chromium.org/5908003/diff/3001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:1203: NSString* titleStr = NULL;
On 2010/12/16 02:18:25, Andrew T Wilson wrote:
> Should we use nil instead of NULL here? Not sure about the style guide for
objc.

Yes.  Well, as best as I can tell anyway.  I didn't find anything explicit in
the Google style guide, but a broader search of the web strongly suggests that
this should be nil rather than NULL.

Other code rearrangement that you suggested has eliminated the NULL/nil question
-- now the block below the Background Application section initializes these
values with non-null items and strings.

http://codereview.chromium.org/5908003/diff/3001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:1205: 
On 2010/12/16 02:18:25, Andrew T Wilson wrote:
> I was confused by us using titleStr/item up here at the top level, but then
> setting it in the for loop below since that pattern is usually only used when
> values from inside the loop need to be used outside the loop.
> 
> It might be easier reverting your change to these lines (just move the
> definition of these variables below the for loop) and define your own NSString
> and item variables in the for loop itself (in fact, I'm not sure you even need
a
> titleStr variable if you fold together a couple of lines of code).

Done.

http://codereview.chromium.org/5908003/diff/3001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:1210: if (applications.begin() !=
applications.end()) {
On 2010/12/16 02:18:25, Andrew T Wilson wrote:
> I wonder if it's cleaner to move the for loop outside the if statement (that
way
> you can just have a simple if statement after the for loop):
> 
> // Only add separator if there are application items
> if (applications.begin() != applications.end()) {
>   NSMenuItem* sepItem = [[NSMenuItem separatorItem] init];
>   [dockMenu addItem:sepItem];
> }
> 
> As it stands, the purpose of the if-statement (guarding the addition of the
> separator) is separated from the if statement itself.

Done.  I saw the purpose as "avoiding the whole block", but agree that it's more
readable this way.

http://codereview.chromium.org/5908003/diff/3001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:1215: DCHECK(sort_position == position);
On 2010/12/16 02:18:25, Andrew T Wilson wrote:
> I dimly recall talking about this before - why can't this be:
> 
> DCHECK(position == applications.GetPosition(*cursor))
> 
> That way in non-debug mode we don't call GetPosition()?
> 
> Or, just get rid of the position variable and always use GetPosition()?

Done.  In fact, I think I missed that bit of clean-up in BackgroundModeManager. 
Gonna circle back on that one.

http://codereview.chromium.org/5908003/diff/3001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:1217: string16 label = ASCIIToUTF16(name);
On 2010/12/16 02:18:25, Andrew T Wilson wrote:
> This seems weird to me. Should this be UTF8ToUTF16(name)?

Done.

http://codereview.chromium.org/5908003/diff/3001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:1218: titleStr =
l10n_util::FixUpWindowsStyleLabel(label);
On 2010/12/16 02:18:25, Andrew T Wilson wrote:
> I'm not sure that we should call this - I think this strips & out of the
string
> or something, which may not be appropriate here for extension names (i.e. if I
> have an extension named "Nuts&Bolts" I probably don't want it converted to
> "NutsBolts".

Done.  Swtiched to base::SysUTF16ToNSString which is what FixUpWindowsStyleLabel
calls after stripping.

http://codereview.chromium.org/5908003/diff/3001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:1220: action:@selector(newWindowFromDock:)
On 2010/12/16 02:18:25, Andrew T Wilson wrote:
> BTW, I'm wondering if newWindowFromDock should be renamed "commandFromDock",
> since we do other things other than create new windows now from dock items?

Done.

http://codereview.chromium.org/5908003/diff/3001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:1226: titleStr =
l10n_util::GetNSStringWithFixup(IDS_NEW_WINDOW_MAC);
On 2010/12/16 02:18:25, Andrew T Wilson wrote:
> Remove this superfluous set of titleStr.

Done.

[The wrong rickcam account, 2010-12-16 19:21:40]

Code reviewing myself :-\  Please take a look at the combined results of this
and my previous upload.

http://codereview.chromium.org/5908003/diff/9001/chrome/browser/app_controlle...
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/5908003/diff/9001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:1228: NSString* titleStr =
l10n_util::GetNSStringWithFixup(IDS_NEW_WINDOW_MAC);
Fix the indentation of the statement below.

[Andrew T Wilson (Slow), 2010-12-17 02:03:21]

LGTM with one nit.

http://codereview.chromium.org/5908003/diff/16001/chrome/app/chrome_command_i...
File chrome/app/chrome_command_ids.h (right):

http://codereview.chromium.org/5908003/diff/16001/chrome/app/chrome_command_i...
chrome/app/chrome_command_ids.h:8: #define IDC_MinimumLabelValue           4000
Perhaps you could add a comment here explaining what it's used for (basically,
we reserve the first 4000 IDs for dynamic menu items)

[The wrong rickcam account, 2010-12-20 22:17:17]

Thanks.  I added a comment and will dcommit shortly.

http://codereview.chromium.org/5908003/diff/16001/chrome/app/chrome_command_i...
File chrome/app/chrome_command_ids.h (right):

http://codereview.chromium.org/5908003/diff/16001/chrome/app/chrome_command_i...
chrome/app/chrome_command_ids.h:8: #define IDC_MinimumLabelValue           4000
On 2010/12/17 02:03:21, Andrew T Wilson wrote:
> Perhaps you could add a comment here explaining what it's used for (basically,
> we reserve the first 4000 IDs for dynamic menu items)

Done.

[The wrong rickcam account, 2010-12-21 02:35:44]

Please take another look.

Previous changes broke a unit test: AppControllerTest.DockMenu

Fix:
 - made the Dock menu code handle a missing profile
 - modified the unit test to look for the commandFromDock selector instead of
the old name: newWindowFromDock.

[Andrew T Wilson (Slow), 2010-12-21 17:49:09]

LGTM, one nit

http://codereview.chromium.org/5908003/diff/25001/chrome/browser/app_controll...
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/5908003/diff/25001/chrome/browser/app_controll...
chrome/browser/app_controller_mac.mm:1198: // Avoid breaking unit tests which
have no profile.
Ultimately we should have a test of some sort once you can mock out
BackgroundApplicationListModel, so add a TODO for this.

[The wrong rickcam account, 2010-12-21 19:20:09]

Added TODO.  Will dcommit shortly.

http://codereview.chromium.org/5908003/diff/25001/chrome/browser/app_controll...
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/5908003/diff/25001/chrome/browser/app_controll...
chrome/browser/app_controller_mac.mm:1198: // Avoid breaking unit tests which
have no profile.
On 2010/12/21 17:49:09, Andrew T Wilson wrote:
> Ultimately we should have a test of some sort once you can mock out
> BackgroundApplicationListModel, so add a TODO for this.

Done.