Added Background Application entries to Dock menu

chrome/browser/app_controller_mac.mm

Index: chrome/browser/app_controller_mac.mm
diff --git a/chrome/browser/app_controller_mac.mm b/chrome/browser/app_controller_mac.mm
index 184b1969ac3a787ca2f66914b814c7218142ca65..81ea20c82121b5da901562daedd05e91a9866f36 100644
--- a/chrome/browser/app_controller_mac.mm
+++ b/chrome/browser/app_controller_mac.mm
@@ -15,6 +15,7 @@
 #include "base/sys_string_conversions.h"
 #import "base/worker_pool_mac.h"
 #include "chrome/app/chrome_command_ids.h"
+#include "chrome/browser/background_application_list_model.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_shutdown.h"
 #include "chrome/browser/browser_thread.h"
@@ -155,6 +156,7 @@ void RecordLastRunAppBundlePath() {
 - (void)showPreferencesWindow:(id)sender
                          page:(OptionsPage)page
                       profile:(Profile*)profile;
+- (void)executeApplication:(id)sender;
 @end
 
 @implementation AppController
@@ -938,9 +940,28 @@ void RecordLastRunAppBundlePath() {
     case IDC_OPTIONS:
       [self showPreferences:sender];
       break;
+    default:
+      [self executeApplication:sender];

[Andrew T Wilson (Slow), 2010/12/16 02:18:25]

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.

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

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.

   }
 }
 
+// Run a (background) application in a new tab.
+- (void)executeApplication:(id)sender {
+  NSInteger tag = [sender tag];
+  Profile* profile = [self defaultProfile];
+  DCHECK(profile);
+  BackgroundApplicationListModel applications(profile);
+  DCHECK(tag >= 0 &&
+         tag < static_cast<int>(applications.size()));
+  Browser* browser = BrowserList::GetLastActive();
+  if (!browser) {
+    Browser::OpenEmptyWindow(profile);
+    browser = BrowserList::GetLastActive();
+  }
+  const Extension* extension = applications.GetExtension(tag);
+  browser->OpenApplicationTab(profile, extension, NULL);
+}
+
 // Same as |-commandDispatch:|, but executes commands using a disposition
 // determined by the key flags. This will get called in the case where the
 // frontmost window is not a browser window, and the user has command-clicked
@@ -1179,11 +1200,37 @@ void RecordLastRunAppBundlePath() {
 
 - (NSMenu*)applicationDockMenu:(NSApplication*)sender {
   NSMenu* dockMenu = [[[NSMenu alloc] initWithTitle: @""] autorelease];
-  NSString* titleStr = l10n_util::GetNSStringWithFixup(IDS_NEW_WINDOW_MAC);
-  scoped_nsobject<NSMenuItem> item([[NSMenuItem alloc]
-                                       initWithTitle:titleStr
-                                       action:@selector(newWindowFromDock:)
-                                       keyEquivalent:@""]);
+  NSString* titleStr = NULL;

[Andrew T Wilson (Slow), 2010/12/16 02:18:25]

Should we use nil instead of NULL here? Not sure about the style guide for objc.

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

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.

+  scoped_nsobject<NSMenuItem> item(NULL);
+

[Andrew T Wilson (Slow), 2010/12/16 02:18:25]

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).

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

Done.

+  Profile* profile = [self defaultProfile];
+  DCHECK(profile);
+  int position = 0;
+  BackgroundApplicationListModel applications(profile);
+  if (applications.begin() != applications.end()) {

[Andrew T Wilson (Slow), 2010/12/16 02:18:25]

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.

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

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

+    for (ExtensionList::const_iterator cursor = applications.begin();

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

Need to indent the following two lines a bit more.

+         cursor != applications.end();
+         ++cursor, ++position) {
+      int sort_position = applications.GetPosition(*cursor);
+      DCHECK(sort_position == position);

[Andrew T Wilson (Slow), 2010/12/16 02:18:25]

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()?

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

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

+      const std::string& name = (*cursor)->name();
+      string16 label = ASCIIToUTF16(name);

[Andrew T Wilson (Slow), 2010/12/16 02:18:25]

This seems weird to me. Should this be UTF8ToUTF16(name)?

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

Done.

+      titleStr = l10n_util::FixUpWindowsStyleLabel(label);

[Andrew T Wilson (Slow), 2010/12/16 02:18:25]

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".

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

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

+      item.reset([[NSMenuItem alloc] initWithTitle:titleStr
+                  action:@selector(newWindowFromDock:)

[Andrew T Wilson (Slow), 2010/12/16 02:18:25]

BTW, I'm wondering if newWindowFromDock should be renamed "commandFromDock",
since we do other things other than create new windows now from dock items?

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

Done.

+                  keyEquivalent:@""]);
+      [item setTarget:self];
+      [item setTag:position];
+      [dockMenu addItem:item];
+    }
+    titleStr = l10n_util::GetNSStringWithFixup(IDS_NEW_WINDOW_MAC);

[Andrew T Wilson (Slow), 2010/12/16 02:18:25]

Remove this superfluous set of titleStr.

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

Done.

+    NSMenuItem* sepItem = [[NSMenuItem separatorItem] init];
+    [dockMenu addItem:sepItem];
+  }
+  titleStr = l10n_util::GetNSStringWithFixup(IDS_NEW_WINDOW_MAC);
+  item.reset([[NSMenuItem alloc] initWithTitle:titleStr
+                                 action:@selector(newWindowFromDock:)
+                                 keyEquivalent:@""]);
   [item setTarget:self];
   [item setTag:IDC_NEW_WINDOW];
   [dockMenu addItem:item];