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"

[Avi (use Gerrit), 2015/05/15 16:13:08]

Where is your copyright header?

Also, same question about file location.

+
+#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"
+#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>
+- (void)buildShareItemsFor:(Browser* )browser;
+- (NSMenuItem*)createItemWithTitle:(NSString*)title action:(SEL)sel;
+- (NSString *)currentPageUrlFor:(Browser* )browser;
+- (void)enableShareMenuItemsFor:(Browser *)browser;
+- (NSArray*)shareMenuItemsFor:(NSArray*)items;

[Avi (use Gerrit), 2015/05/15 16:13:09]

Lots of bad formatting.

For types, it's always (NSType*). No spaces.

+@end
+
+namespace ShareMenuControllerInternal {
+
+class Observer : public chrome::BrowserListObserver {
+ public:
+  Observer(ShareMenuController* controller) : controller_(controller) {
+    BrowserList::AddObserver(this);
+  }
+
+  ~Observer() override { BrowserList::RemoveObserver(this); }
+
+  // Wait for a browser tab to become active before building the share menu.
+  void OnBrowserSetLastActive(Browser* browser) override {
+    if (browser && ![controller_ menuItemsLoaded]) {
+      [controller_ initShareMenuItem];
+      [controller_ buildShareItemsFor:browser];
+    }
+    [controller_ enableShareMenuItemsFor:browser];
+  }
+
+ private:
+  ShareMenuController* controller_;  // Weak; owns this.
+ };
+
+} // namespace ShareMenuControllerInternal
+
+////////////////////////////////////////////////////////////////////////////////
+
+@implementation ShareMenuController
+
+- (id)initWithMainMenu:(NSMenu*)menu {
+  if (self = [super init]) {
+    mainMenu_ = menu;
+    shareItems_.reset([[NSMutableArray alloc] init]);
+    menuItemsLoaded_ = NO;
+    observer_.reset(new ShareMenuControllerInternal::Observer(self));
+  }
+  return self;
+}
+
+- (BOOL) menuItemsLoaded {

[Avi (use Gerrit), 2015/05/15 16:13:08]

No space after )

+  return menuItemsLoaded_;
+}
+
+// Create a submenu for displaying various system wide sharing options.

[Avi (use Gerrit), 2015/05/15 16:13:08]

Comments go in the header file, not the .mm file.

+- (void)initShareMenuItem {
+  shareSubMenu_.reset([[NSMenu alloc] initWithTitle:@"ShareSubMenu"]);
+  fileMenuItem_ = [mainMenu_ itemWithTag:IDC_FILE_MENU];
+  NSString* shareMenuName =
+        l10n_util::GetNSStringWithFixup(IDS_SHARE_MENU_MAC);
+  shareMenuItem_ = [self createItemWithTitle:shareMenuName action:nil];
+  [shareMenuItem_ setTag:IDC_SHARE_MENU];
+}
+
+- (NSMenu*)shareSubMenu {
+  return shareSubMenu_;
+}
+
+- (NSMenuItem*)shareSubMenuItem {
+  return shareMenuItem_;
+}
+
+- (void)buildShareItemsFor:(Browser *)browser {

[Avi (use Gerrit), 2015/05/15 16:13:09]

No space before *.

+  if (!shareSubMenu_)
+    return;
+
+  [shareItems_ removeAllObjects];
+  [shareItems_ addObject:[self currentPageUrlFor:browser]];
+
+  // If the sharemenu had already been built, remove all items and rebuild.
+  if ([shareSubMenu_ numberOfItems] > 0) {
+    [shareSubMenu_ removeAllItems];
+  }
+
+  NSArray* menuItems = [self shareMenuItemsFor:shareItems_];
+  for (NSMenuItem* item in menuItems) {
+    [shareSubMenu_ addItem:item];
+  }
+
+  // Make sure the Share menu item is created only once.
+  if (![[fileMenuItem_ submenu] itemWithTag:IDC_SHARE_MENU]) {
+    NSMenuItem* savePageItem = [[fileMenuItem_ submenu]
+        itemWithTag:IDC_SAVE_PAGE];
+    NSInteger idxSavePage = [[fileMenuItem_ submenu] indexOfItem:savePageItem];
+    [[fileMenuItem_ submenu] insertItem:shareMenuItem_ atIndex:idxSavePage + 2];
+  }

[Avi (use Gerrit), 2015/05/15 16:13:09]

?

Why do you have to do this? This code looks like it's addressing a problem you
don't understand. Don't band-aid over issues. Understand them so you can write
clean code.

[sarka, 2015/05/15 16:36:55]

Were you referring to the line containing '[[fileMenuItem_ submenu]
insertItem:shareMenuItem_ atIndex:idxSavePage + 2];' in particular?
Apparently with the new Share menu in place 'Email Page Location' menu item
would be gone.
That leaves me with an option of creating the Share menu dynamically and
inserting between two separator menu items.
To calculate the position where the menuitem needs to be inserted I have coded
what it is right now. I will try to figure out a better way.
Thanks

[Avi (use Gerrit), 2015/05/15 16:40:33]

I mean this whole "if" block.

You are modifying the xib to add a "Share" item, and building it, but you're
also adding it again here? Am I mis-reading things?

[sarka, 2015/05/15 16:45:47]

You are right. The Share menu and its submenu should be built only once , and as
tab state changes, only state of the sharemenuitems needs to be updated.
I will change that.

+
+  [[fileMenuItem_ submenu] setSubmenu:shareSubMenu_ forItem:shareMenuItem_];
+  menuItemsLoaded_ = YES;
+}
+
+- (NSMenuItem*)createItemWithTitle:(NSString*)title action:(SEL)sel {
+  NSMenuItem* item = [[[NSMenuItem alloc] initWithTitle:title action:sel
+                     keyEquivalent:@""]
+                     autorelease];

[Avi (use Gerrit), 2015/05/15 16:13:09]

Align on :

+  [item setTarget:self];
+  return item;
+}
+
+- (NSString*)currentPageUrlFor:(Browser*)browser {
+  TabStripModel* tabStrip = browser->tab_strip_model();
+  content::WebContents* wc = tabStrip->GetActiveWebContents();
+  std::string pageUrl = net::EscapeQueryParamValue(wc->GetVisibleURL().spec(),
+                                                   false);
+  return base::SysUTF8ToNSString(pageUrl);
+}
+
+- (void)enableShareMenuItemsFor:(Browser *)browser {

[Avi (use Gerrit), 2015/05/15 16:13:09]

No space before *

+  [shareSubMenu_ setAutoenablesItems:NO];
+  for (NSMenuItem* item in [shareSubMenu_ itemArray]) {
+    BOOL enabled = chrome::CanEmailPageLocation(browser) ?
+    [self validateMenuItem:item] : NO;

[Avi (use Gerrit), 2015/05/15 16:13:09]

indent

+    [item setEnabled:enabled];
+  }
+}
+
+// Retrieve the list of possible sharing options and create menu item for each.

[Avi (use Gerrit), 2015/05/15 16:13:09]

comments go in the header file

+- (NSArray*)shareMenuItemsFor:(NSArray*)items {
+  base::scoped_nsobject<NSMutableArray>
+  menuItems([[NSMutableArray alloc] init]);

[Avi (use Gerrit), 2015/05/15 16:13:08]

indent!

+  NSArray *sharingServices = [NSSharingService sharingServicesForItems:items];

[Avi (use Gerrit), 2015/05/15 16:13:08]

space after the *, not before

+  for (NSSharingService* currentService in sharingServices) {
+    NSString* titleText = nil;
+    if ([currentService isEqualTo:[NSSharingService sharingServiceNamed
+                                 :NSSharingServiceNameComposeEmail]]) {

[Avi (use Gerrit), 2015/05/15 16:13:08]

Do not break there! You're breaking a method name onto two lines.

Maybe:

if ([currentService isEqualTo:
      [NSSharingService sharingServiceNamed:NSSharingServiceNameComposeEmail]])
{

+      titleText = l10n_util::GetNSString(IDS_EMAIL_PAGE_LOCATION_MAC);
+    } else {
+      titleText = currentService.title;
+    }
+    NSMenuItem* item = [self createItemWithTitle:titleText
+                                          action:@selector(
+                              systemShareService:)];

[Avi (use Gerrit), 2015/05/15 16:13:08]

One line. Don't align ALL colons, just the ones that belong to the same method
invocation.

+    item.image = currentService.image;
+    item.representedObject = currentService;
+    [item setTarget:self];
+    currentService.delegate = self;
+    [menuItems addObject:item];
+  }
+  return [menuItems copy];

[Avi (use Gerrit), 2015/05/15 16:13:09]

1) You're leaking.
2) Why do you need to copy this?

Try creating an autoreleased NSMutableArray and returning it.

+}
+
+- (BOOL)validateMenuItem:(NSMenuItem *)menuItem {

[Avi (use Gerrit), 2015/05/15 16:13:08]

no space before *

+  return ([menuItem action] == @selector(systemShareService:));
+}
+
+- (void)systemShareService:(id)sender {
+  NSMenuItem* currentMenuItem = (NSMenuItem*)sender;
+  [currentMenuItem.representedObject performWithItems:shareItems_];
+}
+
+@end