Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1105143005:
Issue 465302:System wide share options on Mac
DescriptionThis is a patch for allowing users on mac to share a page via Email, Messages, FaceBook, Twitter etc.
With this change 'Email page location' would no longer be needed.
The implementation uses NSSharingServices framework available on OSX 10.9 SDK and higher.
TEST:
Navigate to File->Share-><Pick one from the options (Email, Twitter, Messages etc).
Appropriate handler will take care of the actions from there.
BUG=465302
Patch Set 1 #Patch Set 2 : System wide share menu for Mac #
Total comments: 53
Patch Set 3 : Added unit tests, honored Cocoa memory management issues #
Total comments: 33
Patch Set 4 : Adds an attachment to Mail #Messages
Total messages: 18 (4 generated)
shrike@chromium.org changed reviewers: + shrike@chromium.org
Hello sarka, Most of my feedback is formatting nits (Chromium project likes code formatted in a particular way). I do have a couple questions for you in the code. Also, if I launch the browser and navigate to a page, the items in the File -> Share menu are disabled (in contrast to Safari where if I follow the same steps the items are all enabled). https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/app_cont... File chrome/browser/app_controller_mac.mm (left): https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/app_cont... chrome/browser/app_controller_mac.mm:605: Browser* browser = chrome::FindBrowserWithWindow([notify object]); Seems like this change is two lines being indented more (when they should not be)? https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.h (right): https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. Should by copyright 2015. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.h:4: What's the reason for putting this new class in chrome/browser/ui/cocoa/renderer_context_menu? Seems like it should live in chrome/browser/ui/cocoa/. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.h:11: Should be one space after namespace. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.h:18: __weak NSMenu* mainMenu_; All of these lines should be indented 2 spaces? https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.h:26: No space between (id) and initWithMainMenu (same issue with menuItemsLoaded). https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.h:31: Two spaces between #endif and //, followed by 1 space. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm (right): https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:20: @interface ShareMenuController() <NSSharingServiceDelegate> Should (NSArray*)items; (no extra space) https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:21: - (NSMutableArray*)shareMenuItemsFor:(NSArray*) items; No extra space after (void). https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:22: - (void) initShareMenuItem; No extra space after (void), or in (Browser*) https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:23: - (void) buildShareItemsFor:(Browser* )browser; Similar extra space problems. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:40: void OnBrowserRemoved(Browser* browser) override {} Too many spaces after // https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:43: if (browser && ![controller_ menuItemsLoaded]) { Next line indented too far. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:52: Two spaces after }, one after // https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:54: Is there someplace else in the code base where they add a line of //////////////////? https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:61: mainMenu_ = menu; 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). https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:68: No space after (BOOL) https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:74: // Retrieve the list of possible sharing options and create menu item for each. No space after (NSArray*) https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:76: NSMutableArray* menuItems = [NSMutableArray array]; Should be NSArray* sharingServices https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:79: NSString* titleText = nil; 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]). https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:80: if ([currentService.title isEqualToString:@"Mail"]) { Lines should be indented 2 more spaces, not 4. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:82: } else { I would think you'd want to take currentService.menuItemTitle. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:84: } Colons should be aligned. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:96: - (void) initShareMenuItem { 1 space after // https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:100: NSString* shareMenuName = Next line should be indented just 4 spaces. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:107: if (!shareSubMenu_) Indented too far. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:113: [shareSubMenu_ setAutoenablesItems:NO]; Why is it called twice? https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:118: NSArray* menuItems = (NSArray*)[self shareMenuItemsFor:items]; Should be NSMenuItem* item https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:121: BOOL enabled = chrome::CanEmailPageLocation(browser) ? Space before and after colon. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:126: if (![[fileMeuItem_ submenu] itemWithTag:IDC_SHARE_MENU]) { 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. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:143: [item setTarget:self]; 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. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:148: - (BOOL)validateMenuItem:(NSMenuItem *)menuItem { Why not just return [menuItem action] == @selector(systemShareService:); https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:158: Why is this defined as an IBAction (it's not hooked up through the nib)?
Hi @shrike I didn't realize that I had forgotten to uncheck 'send email' to developers email chain when the CL was uploaded. I was not entirely done with the CL yet. Have some more work including a unit test. Appreciate your comments. Thanks
On 2015/05/05 18:28:31, sarka wrote: > Hi @shrike > > I didn't realize that I had forgotten to uncheck 'send email' to developers > email chain when the CL was uploaded. > > I was not entirely done with the CL yet. > > Have some more work including a unit test. > > Appreciate your comments. > > Thanks You updated the Share menu bug report to say that you had uploaded an initial cl.
avi@chromium.org changed reviewers: + avi@chromium.org
Adopting. https://codereview.chromium.org/1105143005/diff/20001/.gitignore File .gitignore (right): https://codereview.chromium.org/1105143005/diff/20001/.gitignore#newcode1 .gitignore:1: *.mk This file is inappropriate to have in your patch. Please remove it. https://codereview.chromium.org/1105143005/diff/20001/chrome/app/chrome_comma... File chrome/app/chrome_command_ids.h (right): https://codereview.chromium.org/1105143005/diff/20001/chrome/app/chrome_comma... chrome/app/chrome_command_ids.h:87: #define IDC_SHARE_MENU 65000 As stated on line 26 (the TODO), these are in numeric order, not visual order. This belongs below "manage passwords for page" and should get an id of 35011. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/app_cont... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/app_cont... chrome/browser/app_controller_mac.mm:229: remove this extra line. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/app_cont... chrome/browser/app_controller_mac.mm:396: // Initialize the Share Menu period at the end; complete sentences, please. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/app_cont... chrome/browser/app_controller_mac.mm:1321: - (void) initShareMenu { no space after ) https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/app_cont... chrome/browser/app_controller_mac.mm:1325: [[ShareMenuController alloc] initWithMainMenu:mainMenu]); Why are you passing in the main menu? Why wouldn't the ShareMenuController be able to get it itself? https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.h (right): https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.h:21: NSMenuItem* fileMeuItem_; typo: Menu, not Meu https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm (right): https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:6: #import "chrome/browser/app_controller_mac.h" one space after #import, not 2 https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:39: void OnBrowserAdded(Browser* browser) override {} Why are you overriding OnBrowserAdded/Removed if you're not going to do anything with them? https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:62: shareItems_ = [[NSMutableArray alloc] init]; You leak shareItems_. Use a scoped_nsobject. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:73: #pragma mark - Private Methods Don't #pragma mark. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:98: shareSubMenu_ = [[NSMenu alloc] initWithTitle:@"ShareSubMenu"]; It's not clear what the ownership story is here, but it seems pretty clear that you're leaking shareSubMenu_. Use a scoped_nsobject. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:106: - (void) buildShareItemsFor:(Browser *)browser { No space after ) https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:117: } 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. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:133: - (NSString *)currentPageUrlFor:(Browser* )browser { No spaces before the *, no space after the *. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:136: std::string pageUrl = net::EscapeQueryParamValue(wc->GetURL().spec(), false); DO NOT CALL GetURL on WebContents. In web_contents.h it CLEARLY says that GetURL is deprecated. Use GetVisibleURL() instead. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:140: - (NSMenuItem*)createItemWithTitle:(NSString*)title action:(SEL)sel { 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. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:143: [item setTarget:self]; On 2015/05/05 18:20:23, shrike wrote: > 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. Yeah. Don't use scoped_nsobject here. Above, do [[[alloc] init] autorelease] and just return here.
Oh, didn't realize that you accidentally sent this out. My comments still apply, but in general watch for formatting and memory correctness.
On 2015/05/05 19:01:33, Avi wrote: > Oh, didn't realize that you accidentally sent this out. > > My comments still apply, but in general watch for formatting and memory > correctness. No worries :) Thanks for all the comments though.
Patchset #3 (id:40001) has been deleted
On 2015/05/05 19:04:32, sarka wrote: > On 2015/05/05 19:01:33, Avi wrote: > > Oh, didn't realize that you accidentally sent this out. > > > > My comments still apply, but in general watch for formatting and memory > > correctness. > > No worries :) Thanks for all the comments though. I pretty much addressed most of the issues. I am having some trouble with my .gitignore file, trying to figure out whats causing it. PTAL. Thanks
a.sarkar.arun@gmail.com changed reviewers: + sky@chromium.org
https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm (right): https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:54: On 2015/05/05 18:20:24, shrike wrote: > Is there someplace else in the code base where they add a line of > //////////////////? Yes, I have seen it in profile_menu_controller. https://codereview.chromium.org/1105143005/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:61: mainMenu_ = menu; On 2015/05/05 18:20:23, shrike wrote: > 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). Done that in app_controller_mac to make sure it is run only if the version is 10.9 and higher.
Again, I request that you read the style guide, https://google-styleguide.googlecode.com/svn/trunk/objcguide.xml. https://codereview.chromium.org/1105143005/diff/60001/.gitignore File .gitignore (right): https://codereview.chromium.org/1105143005/diff/60001/.gitignore#newcode440 .gitignore:440: .gitignore The upload isn't going to pay attention to this line. Do a git --amend to remove .gitignore from the commit, and you can stash it away if you want. https://codereview.chromium.org/1105143005/diff/60001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1105143005/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:13110: + Remove these extra lines. https://codereview.chromium.org/1105143005/diff/60001/chrome/app/nibs/MainMen... File chrome/app/nibs/MainMenu.xib (right): https://codereview.chromium.org/1105143005/diff/60001/chrome/app/nibs/MainMen... chrome/app/nibs/MainMenu.xib:8: <string key="IBDocument.HIToolboxVersion">755.00</string> Please read https://www.chromium.org/developers/design-documents/mac-xib-files . In particular: - You must use Mac OS X 10.8.x (you are using 10.10) - You must use Xcode 4.4-4.6 (you are using 6.3.1) Also, you need to describe the changes you are making so that others can follow along. https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/app_cont... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/app_cont... chrome/browser/app_controller_mac.mm:399: #endif No. You're doing a compile-time check, and you need a run-time check. Also, what happens before 10.9? We just leave a dead menu item? https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/app_cont... chrome/browser/app_controller_mac.mm:617: [self windowChangedToProfile:browser->profile()->GetOriginalProfile()]; Please don't make random formatting changes. https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:1139: BrowserList::SetLastActive(browser_); Are the existing calls to SetLastActive not working correctly? https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.h (right): https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. Why are you putting this into renderer_context_menu? https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.h:13: namespace ShareMenuControllerInternal { Blank line before this one. https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.h:14: class Observer; Bad spacing; no indenting inside namespaces. https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.h:25: BOOL menuItemsLoaded_; Indent 2. https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.h:28: - (id)initWithMainMenu:(NSMenu* )menu; No space after * https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.h:29: - (BOOL)menuItemsLoaded; comment functions https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm (right): https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:1: #import "chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.h" Where is your copyright header? Also, same question about file location. https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:24: - (NSArray*)shareMenuItemsFor:(NSArray*)items; Lots of bad formatting. For types, it's always (NSType*). No spaces. https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:66: - (BOOL) menuItemsLoaded { No space after ) https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:70: // Create a submenu for displaying various system wide sharing options. Comments go in the header file, not the .mm file. https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:88: - (void)buildShareItemsFor:(Browser *)browser { No space before *. https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:111: } ? 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. https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:120: autorelease]; Align on : https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:133: - (void)enableShareMenuItemsFor:(Browser *)browser { No space before * https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:137: [self validateMenuItem:item] : NO; indent https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:142: // Retrieve the list of possible sharing options and create menu item for each. comments go in the header file https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:145: menuItems([[NSMutableArray alloc] init]); indent! https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:146: NSArray *sharingServices = [NSSharingService sharingServicesForItems:items]; space after the *, not before https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:150: :NSSharingServiceNameComposeEmail]]) { Do not break there! You're breaking a method name onto two lines. Maybe: if ([currentService isEqualTo: [NSSharingService sharingServiceNamed:NSSharingServiceNameComposeEmail]]) { https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:157: systemShareService:)]; One line. Don't align ALL colons, just the ones that belong to the same method invocation. https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:164: return [menuItems copy]; 1) You're leaking. 2) Why do you need to copy this? Try creating an autoreleased NSMutableArray and returning it. https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:167: - (BOOL)validateMenuItem:(NSMenuItem *)menuItem { no space before * https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/share_menu_controller_unittest.mm (right): https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller_unittest.mm:74: // close the tab Complete sentences here and below. Capital letter, period, the works.
https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:1139: BrowserList::SetLastActive(browser_); On 2015/05/15 16:13:08, Avi wrote: > Are the existing calls to SetLastActive not working correctly? From what I understand not when a new tab is added, or switched to a different tab. https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm (right): https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:111: } On 2015/05/15 16:13:09, Avi wrote: > ? > > 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. 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
https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm (right): https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:111: } 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?
https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm (right): https://codereview.chromium.org/1105143005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/renderer_context_menu/share_menu_controller.mm:111: } On 2015/05/15 16:40:33, Avi wrote: > 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? 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.
Aside from the style issues, this code does not work correctly: 1. File -> Share -> Email Page Location sends a pretty ugly looking URL, where the forward slashes, for example, are %2Fs 2. Safari's comparable command File -> Share -> Email This Page sets the Mail message subject line to the title of the browser tab, and fills the mail message body with HTML. I assume it should be possible for us to do the same, and I would aim for that high bar 3. I created a browser window with Tab A and Tab B. With Tab A active I get Tab A's URL when I choose File -> Share -> Email Page Location (right). With Tab B active, I still get Tab A's URL. If I drag Tab B so that it's the first tab in the window, and make Tab B the active tab in the window, I still get Tab A's URL. 4. If I create another window and choose File -> Share -> Email Page Location, I am still getting the URL for Tab A from the other window.
On 2015/05/15 17:10:50, shrike wrote: > Aside from the style issues, this code does not work correctly: > > 1. File -> Share -> Email Page Location sends a pretty ugly looking URL, where > the forward slashes, for example, are %2Fs > > 2. Safari's comparable command File -> Share -> Email This Page sets the Mail > message subject line to the title of the browser tab, and fills the mail message > body with HTML. I assume it should be possible for us to do the same, and I > would aim for that high bar > > 3. I created a browser window with Tab A and Tab B. With Tab A active I get Tab > A's URL when I choose File -> Share -> Email Page Location (right). With Tab B > active, I still get Tab A's URL. If I drag Tab B so that it's the first tab in > the window, and make Tab B the active tab in the window, I still get Tab A's > URL. > > 4. If I create another window and choose File -> Share -> Email Page Location, I > am still getting the URL for Tab A from the other window. Thanks for your comments. I will revisit all of these issues and address them. |
