Issue 465302:System wide share options on Mac

chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm

+#import "chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.h"
+
+#include "base/strings/sys_string_conversions.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/app/chrome_command_ids.h"
+#import  "chrome/browser/app_controller_mac.h"

[Avi (use Gerrit), 2015/05/05 18:47:51]

one space after #import, not 2

+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_commands.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/browser_list.h"
+#include "chrome/browser/ui/browser_list_observer.h"
+#include "chrome/browser/ui/cocoa/last_active_browser_cocoa.h"
+#include "chrome/browser/ui/cocoa/last_active_browser_cocoa.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/grit/generated_resources.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/base/l10n/l10n_util_mac.h"
+#include "net/base/escape.h"
+
+@interface ShareMenuController() <NSSharingServiceDelegate>

[shrike, 2015/05/05 18:20:23]

Should (NSArray*)items; (no extra space)

+- (NSMutableArray*)shareMenuItemsFor:(NSArray*) items;

[shrike, 2015/05/05 18:20:23]

No extra space after (void).

+- (void) initShareMenuItem;

[shrike, 2015/05/05 18:20:23]

No extra space after (void), or in (Browser*)

+- (void) buildShareItemsFor:(Browser* )browser;

[shrike, 2015/05/05 18:20:23]

Similar extra space problems.

+- (NSString *)currentPageUrlFor:(Browser* )browser;
+- (NSMenuItem*)createItemWithTitle:(NSString*)title action:(SEL)sel;
+@end
+
+namespace ShareMenuControllerInternal {
+
+class Observer : public chrome::BrowserListObserver {
+ public:
+  Observer(ShareMenuController* controller) : controller_(controller) {
+    BrowserList::AddObserver(this);
+  }
+
+  ~Observer() override { BrowserList::RemoveObserver(this); }
+
+  // chrome::BrowserListObserver:
+  void OnBrowserAdded(Browser* browser) override {}

[Avi (use Gerrit), 2015/05/05 18:47:51]

Why are you overriding OnBrowserAdded/Removed if you're not going to do anything
with them?

+  void OnBrowserRemoved(Browser* browser) override {}

[shrike, 2015/05/05 18:20:23]

Too many spaces after //

+  //  Allow the browser to add Share Menu once.
+  void OnBrowserSetLastActive(Browser* browser) override {
+    if (browser && ![controller_ menuItemsLoaded]) {

[shrike, 2015/05/05 18:20:23]

Next line indented too far.

+        [controller_ initShareMenuItem];
+    }
+    [controller_ buildShareItemsFor:browser];
+  }
+
+ private:
+  ShareMenuController* controller_;  // Weak; owns this.
+ };
+

[shrike, 2015/05/05 18:20:23]

Two spaces after }, one after //

+} //  namespace ShareMenuControllerInternal
+

[shrike, 2015/05/05 18:20:24]

Is there someplace else in the code base where they add a line of
//////////////////?

[sarka, 2015/05/15 03:35:44]

Yes, I have seen it in profile_menu_controller.

+////////////////////////////////////////////////////////////////////////////////
+
+@implementation ShareMenuController
+
+- (id)initWithMainMenu:(NSMenu*)menu {
+  if (self = [super init]) {
+    mainMenu_ = menu;

[shrike, 2015/05/05 18:20:23]

There needs to be a runtime check somewhere to make sure we're not running on <
10.8 (and in that case not try to instantiate any of the sharing service stuff).

[sarka, 2015/05/15 03:35:43]

Done that in app_controller_mac to make sure it is run only if the version is
10.9 and higher.

+    shareItems_ = [[NSMutableArray alloc] init];

[Avi (use Gerrit), 2015/05/05 18:47:51]

You leak shareItems_. Use a scoped_nsobject.

+    menuItemsLoaded_ = NO;
+    observer_.reset(new ShareMenuControllerInternal::Observer(self));
+  }
+  return self;
+}
+

[shrike, 2015/05/05 18:20:23]

No space after (BOOL)

+- (BOOL) menuItemsLoaded {
+  return menuItemsLoaded_;
+}
+
+#pragma mark - Private Methods

[Avi (use Gerrit), 2015/05/05 18:47:50]

Don't #pragma mark.

+//  Retrieve the list of possible sharing options and create menu item for each.

[shrike, 2015/05/05 18:20:23]

No space after (NSArray*)

+- (NSMutableArray*)shareMenuItemsFor:(NSArray*) items {
+  NSMutableArray* menuItems = [NSMutableArray array];

[shrike, 2015/05/05 18:20:23]

Should be NSArray* sharingServices

+  NSArray *sharingServices = [NSSharingService sharingServicesForItems:items];
+  for (NSSharingService* currentService in sharingServices) {
+      NSString* titleText = nil;

[shrike, 2015/05/05 18:20:23]

I don't think it's kosher to look for a service with the title "Mail" because
the title could be a localized string. Seems like you instead need to say
something like

        mailSharingService = [NSSharingService
sharingServiceNamed:NSSharingServiceNameComposeEmail]

and then somehow see if currentService == mailSharingService (looks like the
best you might be able to do is [currentService.title
isEqualToString:mailSharingService.title]).

+      if ([currentService.title isEqualToString:@"Mail"]) {

[shrike, 2015/05/05 18:20:23]

Lines should be indented 2 more spaces, not 4.

+          titleText = l10n_util::GetNSString(IDS_EMAIL_PAGE_LOCATION_MAC);
+      } else {

[shrike, 2015/05/05 18:20:24]

I would think you'd want to take currentService.menuItemTitle.

+          titleText = currentService.title;
+      }

[shrike, 2015/05/05 18:20:23]

Colons should be aligned.

+      NSMenuItem* item = [self createItemWithTitle:titleText
+           action:@selector(systemShareService:)];
+      item.image = currentService.image;
+      item.representedObject = currentService;
+      [item setTarget:self];
+      currentService.delegate = self;
+      [menuItems addObject:item];
+  }
+  return menuItems;
+}
+
+- (void) initShareMenuItem {

[shrike, 2015/05/05 18:20:23]

1 space after //

+  //  Create a submenu for displaying various system wide sharing options.
+  shareSubMenu_ = [[NSMenu alloc] initWithTitle:@"ShareSubMenu"];

[Avi (use Gerrit), 2015/05/05 18:47:50]

It's not clear what the ownership story is here, but it seems pretty clear that
you're leaking 
shareSubMenu_. Use a scoped_nsobject.

+  fileMeuItem_ = [mainMenu_ itemWithTag:IDC_FILE_MENU];
+  NSString* shareMenuName =

[shrike, 2015/05/05 18:20:23]

Next line should be indented just 4 spaces.

+        l10n_util::GetNSStringWithFixup(IDS_SHARE_MENU_MAC);
+  shareMenuItem_ = [self createItemWithTitle:shareMenuName action:nil];
+  [shareMenuItem_ setTag:IDC_SHARE_MENU];
+}
+
+- (void) buildShareItemsFor:(Browser *)browser {

[Avi (use Gerrit), 2015/05/05 18:47:50]

No space after )

+  if (!shareSubMenu_)

[shrike, 2015/05/05 18:20:23]

Indented too far.

+      return;
+  NSMutableArray* items = [NSMutableArray array];
+  [items addObject:[self currentPageUrlFor:browser]];
+  [shareItems_ removeAllObjects];
+  [shareItems_ addObject:[self currentPageUrlFor:browser]];
+  [shareSubMenu_ setAutoenablesItems:NO];

[shrike, 2015/05/05 18:20:24]

Why is it called twice?

+  //  Since this method is called twice, we don't want to add duplicate items.
+  if ([shareSubMenu_ numberOfItems] > 0) {
+    [shareSubMenu_ removeAllItems];
+  }

[Avi (use Gerrit), 2015/05/05 18:47:50]

Why are you casting to NSArray below? Don't cast; it's unnecessary and silly.
-shareMenuItemsFor: should return an NSArray*. And read up on the inheritance of
Foundation containers.

+  NSArray* menuItems = (NSArray*)[self shareMenuItemsFor:items];

[shrike, 2015/05/05 18:20:24]

Should be NSMenuItem* item

+  for (NSMenuItem *item in menuItems) {
+    [shareSubMenu_ addItem:item];
+    BOOL enabled = chrome::CanEmailPageLocation(browser) ?

[shrike, 2015/05/05 18:20:23]

Space before and after colon.

+        [self validateMenuItem:item]:NO;
+    [item setEnabled:enabled];
+  }
+  //  Make sure we don't insert ShareMenuItem twice.
+  if (![[fileMeuItem_ submenu] itemWithTag:IDC_SHARE_MENU]) {

[shrike, 2015/05/05 18:20:24]

Where does the # 12 come from? In general I think you should look for a known
menu item, such as File -> Save Page As..., determine its index, and then insert
the share menu a that index + the appropriate offset. Actually, probably as you
suggest replace the Email Page Location... item with this Share submenu.

+    [[fileMeuItem_ submenu] insertItem:shareMenuItem_ atIndex:12];
+  }
+  [[fileMeuItem_ submenu] setSubmenu:shareSubMenu_ forItem:shareMenuItem_];
+  menuItemsLoaded_ = YES;
+}
+
+- (NSString *)currentPageUrlFor:(Browser* )browser {

[Avi (use Gerrit), 2015/05/05 18:47:50]

No spaces before the *, no space after the *.

+  TabStripModel* tabStrip = browser->tab_strip_model();
+  content::WebContents* wc = tabStrip->GetActiveWebContents();
+  std::string pageUrl = net::EscapeQueryParamValue(wc->GetURL().spec(), false);

[Avi (use Gerrit), 2015/05/05 18:47:50]

DO NOT CALL GetURL on WebContents.

In web_contents.h it CLEARLY says that GetURL is deprecated.

Use GetVisibleURL() instead.

+  return base::SysUTF8ToNSString(pageUrl);
+}
+
+- (NSMenuItem*)createItemWithTitle:(NSString*)title action:(SEL)sel {

[Avi (use Gerrit), 2015/05/05 18:47:51]

Do not call this "createItemWithTitle". That makes it unclear as to object
ownership.

Call this itemWithTitle:action:. It will be clear that the return value is
autoreleased.

+  base::scoped_nsobject<NSMenuItem> item(
+      [[NSMenuItem alloc] initWithTitle:title action:sel keyEquivalent:@""]);
+  [item setTarget:self];

[shrike, 2015/05/05 18:20:23]

I don't think saying item.release() is quite correct. In general you don't have
to worry about releasing the Objc object because the scoped_nsobject will
automatically call release on its object once it goes out of scope.

Is there much value to putting these two lines of code into its own method, and
then managing with a scoped_nsobject? Seems like it would be easier to just
create an autoreleased menu item in the calling code above.

[Avi (use Gerrit), 2015/05/05 18:47:51]

Yeah. Don't use scoped_nsobject here. Above, do [[[alloc] init] autorelease] and
just return here.

+  return [item.release() autorelease];
+}
+
+#pragma mark - validate NSMenuItems
+- (BOOL)validateMenuItem:(NSMenuItem *)menuItem {

[shrike, 2015/05/05 18:20:23]

Why not just

  return  [menuItem action] == @selector(systemShareService:);

+  BOOL ret = NO;
+  SEL action = [menuItem action];
+  if (action == @selector(systemShareService:)) {
+    ret = YES;
+  }
+  return ret;
+}
+
+#pragma mark - Handle Actions for individual MenuItems
+

[shrike, 2015/05/05 18:20:23]

Why is this defined as an IBAction (it's not hooked up through the nib)?

+- (IBAction)systemShareService:(id)sender {
+  NSMenuItem* currentMenuItem = (NSMenuItem*)sender;
+  [currentMenuItem.representedObject performWithItems:shareItems_];
+}
+
+
+@end