|
|
Description[Mac] Add system share menu
Replaces the "Email Link to Page" item in the file menu with a submenu
that allows sharing via macOS Sharing Extensions. The previous e-mail
command is superseded by the Mail sharing service, which inherits the
key command.
Reminders sharing service still needs the browser's NSUserActivity to
have a title in order to prefill the subject. This is forthcoming in
a separate CL.
BUG=465302
Patch Set 1 #Patch Set 2 : Remove More menu in 10.9 and cleanup #Patch Set 3 : Fix tests #Patch Set 4 : Whoops #Patch Set 5 : Guard against empty web contents #
Total comments: 35
Patch Set 6 : CL comments #
Total comments: 6
Messages
Total messages: 37 (28 generated)
The CQ bit was checked by lgrey@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lgrey@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lgrey@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lgrey@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lgrey@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgrey@chromium.org changed reviewers: + shrike@chromium.org
PTAL :)
lgrey@chromium.org changed reviewers: + rsesek@chromium.org
Cool! Nice job keeping ShareMenuController mostly decoupled. I can see why you were trying to avoid chrome::GetLastActiveBrowser. I don't have a better option, unfortunately. https://codereview.chromium.org/2950403002/diff/80001/chrome/app/nibs/MainMen... File chrome/app/nibs/MainMenu.xib (right): https://codereview.chromium.org/2950403002/diff/80001/chrome/app/nibs/MainMen... chrome/app/nibs/MainMenu.xib:144: <menu key="submenu" title="Share" id="T9x-6Q-SHx"> Should the title be ^IDS_SHARE_MAC ? https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/app_cont... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/app_cont... chrome/browser/app_controller_mac.mm:1234: [fileMenu itemWithTitle:l10n_util::GetNSString(IDS_SHARE_MAC)]; optional: This could also be setup as an IBOutlet. https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/share_menu_controller.h (right): https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.h:10: @interface ShareMenuController Class needs a comment. How should a client use this (e.g., "Set this as the delegate of a menu to populate with potential sharing service items."). https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/share_menu_controller.mm (right): https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:26: const UInt32 kOpenSharingSubpaneDescriptorType = 'ptru'; Is this documented, or did you find it? Maybe leave a comment. https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:31: } " // namespace" https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:35: NSRect rectForShare_; Document what these are used for. https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:60: if ([service isEqual:[NSSharingService Hoist the argument to isEqual: to a local outside the loop. It'll make this easier to read, and it won't be recomputed each iteration. https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:71: NSMenuItem* moreItem = [[NSMenuItem alloc] This is currently leaked. Use scoped_nsobject<>. https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:78: } nit: blank line after https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:116: windowForShare_ = browser->window()->GetNativeWindow(); Is it possible for the window to be closed between this method and -sharingService:sourceWindowForSharingItems:sharingContentScope: ? https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:126: snapshotForShare_.reset([image.ToNSImage() retain]); .CopyNSImage() will return a strong reference. You can also store the gfx::Image as a member (it's cheaply copied) and defer the NSImage conversion to -sharingService:transitionImageForShareItem:contentRect:. https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:179: data:data] autorelease]; Prefer scoped_nsobject to autorelease for locally alloc'd objects. https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:203: NSString* keyEquivalent = isMail ? @"I" : @""; Look this up using AcceleratorsCocoa::GetAcceleratorForCommand rather than hard-coding it here. https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:206: NSMenuItem* item = [[NSMenuItem alloc] initWithTitle:title This could also be put in a scoped_nsobject<> and return .autorelease() or the scoped_nsobject<> itself. https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/share_menu_controller_browsertest.mm (right): https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller_browsertest.mm:71: [[NSMenuItem alloc] initWithTitle:@"test" action:nil keyEquivalent:@""]; This is leaked. (Use scoped_nsobject). https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller_browsertest.mm:85: NSArray* sharingServicesForURL = [NSSharingService Nit (and throughout this file): Since this is a C++ function, use under_score naming.
A few observations: * When you choose a Sharing service like Twitter or Facebook, the AppKit overlays a small window and grays out the browser window. I noticed that the browser window grows a small amount when that overlay gets added, and when it gets removed. * Less problematic, I noticed that repeated clicking of the grayed-out window contents reenables and then disables the browser window's spotlight buttons. It must be something about the the browser and overlay windows trading off isMainWindow status. * I ran a Google search and then shared that page as a Note and as a Reminder. In both cases the item in each app shows a URL of "google.com", but only Notes goes back to my Google search page when I click the link. Reminders always just takes me to google.com. * I see there's a Flickr sharing extension but I couldn't get it to show up in the Share menu. I'm guessing the items in these menus are keyed off of a pasteboard type? An ideas on how to get this to work?
Also, I think the cl should be named something like "[Mac] Add System share menu", the idea being it describes the intent of the cl.
The CQ bit was checked by lgrey@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [Mac] System share menu Replaces the "Email Link to Page" item in the file menu with a submenu that allows sharing via macOS Sharing Extensions. The previous e-mail command is superseded by the Mail sharing service, which inherits the key command. Reminders sharing service still needs the browser's NSUserActivity to have a title in order to prefill the subject. This is forthcoming in a separate CL. BUG=465302 ========== to ========== [Mac] Add system share menu Replaces the "Email Link to Page" item in the file menu with a submenu that allows sharing via macOS Sharing Extensions. The previous e-mail command is superseded by the Mail sharing service, which inherits the key command. Reminders sharing service still needs the browser's NSUserActivity to have a title in order to prefill the subject. This is forthcoming in a separate CL. BUG=465302 ==========
On 2017/06/28 18:49:04, shrike wrote: > A few observations: > > * When you choose a Sharing service like Twitter or Facebook, the AppKit > overlays a small window and grays out the browser window. I noticed that the > browser window grows a small amount when that overlay gets added, and when it > gets removed. This is due to the full size content window (same root cause as crbug.com/734713, not sure if we have a separate bug for it.) > > * Less problematic, I noticed that repeated clicking of the grayed-out window > contents reenables and then disables the browser window's spotlight buttons. It > must be something about the the browser and overlay windows trading off > isMainWindow status. Not sure we can do anything about this, since it's a remote view and we don't own it. Safari behaves the same FWIW. > * I ran a Google search and then shared that page as a Note and as a Reminder. > In both cases the item in each app shows a URL of "google.com", but only Notes > goes back to my Google search page when I click the link. Reminders always just > takes me to http://google.com. This is an...interesting UX choice on Reminders' part. There's a little Chrome (or whatever your default browser is) icon on the right side of the reminder. Click that and it goes to the URL. The link in the body of the reminder just goes to the origin for (seemingly) all URLs. Needless to say, Safari behaves the same. > > * I see there's a Flickr sharing extension but I couldn't get it to show up in > the Share menu. I'm guessing the items in these menus are keyed off of a > pasteboard type? An ideas on how to get this to work? Correct. To get the list of extensions, we provide an instantiated object (a dummy NSURL in practice). My guess would be that we'd get Flickr if we requested extensions for an NSImage. For a regular page, it would be hard to pick one out without heuristics of some kind, or site provided metadata. I think we could conceivably do an NSImage if the URL is a direct image link, but that seems problematic re: implementation. Safari doesn't do this.
https://codereview.chromium.org/2950403002/diff/80001/chrome/app/nibs/MainMen... File chrome/app/nibs/MainMenu.xib (right): https://codereview.chromium.org/2950403002/diff/80001/chrome/app/nibs/MainMen... chrome/app/nibs/MainMenu.xib:144: <menu key="submenu" title="Share" id="T9x-6Q-SHx"> On 2017/06/28 15:37:54, Robert Sesek wrote: > Should the title be ^IDS_SHARE_MAC ? Done. https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/app_cont... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/app_cont... chrome/browser/app_controller_mac.mm:1234: [fileMenu itemWithTitle:l10n_util::GetNSString(IDS_SHARE_MAC)]; On 2017/06/28 15:37:54, Robert Sesek wrote: > optional: This could also be setup as an IBOutlet. I'm trying to be minimally invasive with the NIBs due to how much of a pain it is to work with. Are there strong advantages? https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/share_menu_controller.h (right): https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.h:10: @interface ShareMenuController On 2017/06/28 15:37:54, Robert Sesek wrote: > Class needs a comment. How should a client use this (e.g., "Set this as the > delegate of a menu to populate with potential sharing service items."). Done. https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/share_menu_controller.mm (right): https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:26: const UInt32 kOpenSharingSubpaneDescriptorType = 'ptru'; On 2017/06/28 15:37:54, Robert Sesek wrote: > Is this documented, or did you find it? Maybe leave a comment. Done. https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:31: } On 2017/06/28 15:37:54, Robert Sesek wrote: > " // namespace" Done. https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:35: NSRect rectForShare_; On 2017/06/28 15:37:54, Robert Sesek wrote: > Document what these are used for. Done. https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:60: if ([service isEqual:[NSSharingService On 2017/06/28 15:37:54, Robert Sesek wrote: > Hoist the argument to isEqual: to a local outside the loop. It'll make this > easier to read, and it won't be recomputed each iteration. Done. https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:71: NSMenuItem* moreItem = [[NSMenuItem alloc] On 2017/06/28 15:37:54, Robert Sesek wrote: > This is currently leaked. Use scoped_nsobject<>. :shamecube: done https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:78: } On 2017/06/28 15:37:54, Robert Sesek wrote: > nit: blank line after Done. https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:116: windowForShare_ = browser->window()->GetNativeWindow(); On 2017/06/28 15:37:54, Robert Sesek wrote: > Is it possible for the window to be closed between this method and > -sharingService:sourceWindowForSharingItems:sharingContentScope: ? In practice, no (the delegate methods are called synchronously during [NSSharingService performWithItems:]). Would it be better to make it a strong ref just in case? https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:126: snapshotForShare_.reset([image.ToNSImage() retain]); On 2017/06/28 15:37:54, Robert Sesek wrote: > .CopyNSImage() will return a strong reference. > > You can also store the gfx::Image as a member (it's cheaply copied) and defer > the NSImage conversion to > -sharingService:transitionImageForShareItem:contentRect:. Done. Decided against storing the gfx::Image since it's a little awkward to reset (unless there's a better way than image_.SwapRepresentations(gfx::Image()))? AFAICT we will never *not* need the image if we create it, so no savings in deferring. https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:179: data:data] autorelease]; On 2017/06/28 15:37:54, Robert Sesek wrote: > Prefer scoped_nsobject to autorelease for locally alloc'd objects. Done. I kind of did autorelease for aesthetic reasons to avoid the get() in the NSWorkspace method call, but I guess not using the pool outweighs that? https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:203: NSString* keyEquivalent = isMail ? @"I" : @""; On 2017/06/28 15:37:55, Robert Sesek wrote: > Look this up using AcceleratorsCocoa::GetAcceleratorForCommand rather than > hard-coding it here. Done. https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:206: NSMenuItem* item = [[NSMenuItem alloc] initWithTitle:title On 2017/06/28 15:37:54, Robert Sesek wrote: > This could also be put in a scoped_nsobject<> and return .autorelease() or the > scoped_nsobject<> itself. Done. https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/share_menu_controller_browsertest.mm (right): https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller_browsertest.mm:71: [[NSMenuItem alloc] initWithTitle:@"test" action:nil keyEquivalent:@""]; On 2017/06/28 15:37:55, Robert Sesek wrote: > This is leaked. (Use scoped_nsobject). Done.
https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/app_cont... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/app_cont... chrome/browser/app_controller_mac.mm:1234: [fileMenu itemWithTitle:l10n_util::GetNSString(IDS_SHARE_MAC)]; On 2017/06/29 21:11:50, lgrey wrote: > On 2017/06/28 15:37:54, Robert Sesek wrote: > > optional: This could also be setup as an IBOutlet. > > I'm trying to be minimally invasive with the NIBs due to how much of a pain it > is to work with. Are there strong advantages? That's fine. It would avoid scanning the menu by name, but since that happens exactly once, it's not a major point. https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/share_menu_controller.mm (right): https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:116: windowForShare_ = browser->window()->GetNativeWindow(); On 2017/06/29 21:11:50, lgrey wrote: > On 2017/06/28 15:37:54, Robert Sesek wrote: > > Is it possible for the window to be closed between this method and > > -sharingService:sourceWindowForSharingItems:sharingContentScope: ? > > In practice, no (the delegate methods are called synchronously during > [NSSharingService performWithItems:]). > > Would it be better to make it a strong ref just in case? OK, if it's synchronous then it should be fine. https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:126: snapshotForShare_.reset([image.ToNSImage() retain]); On 2017/06/29 21:11:50, lgrey wrote: > On 2017/06/28 15:37:54, Robert Sesek wrote: > > .CopyNSImage() will return a strong reference. > > > > You can also store the gfx::Image as a member (it's cheaply copied) and defer > > the NSImage conversion to > > -sharingService:transitionImageForShareItem:contentRect:. > > Done. > > Decided against storing the gfx::Image since it's a little awkward to reset > (unless there's a better way than image_.SwapRepresentations(gfx::Image()))? > AFAICT we will never *not* need the image if we create it, so no savings in > deferring. image_ = gfx::Image() would work, but what you're doing here is also fine. https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/share_menu_controller.mm:179: data:data] autorelease]; On 2017/06/29 21:11:50, lgrey wrote: > On 2017/06/28 15:37:54, Robert Sesek wrote: > > Prefer scoped_nsobject to autorelease for locally alloc'd objects. > > Done. I kind of did autorelease for aesthetic reasons to avoid the get() in the > NSWorkspace method call, but I guess not using the pool outweighs that? Per other comment, the .get() shouldn't be necessary (except for things going in container literals). https://codereview.chromium.org/2950403002/diff/100001/chrome/browser/app_con... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2950403002/diff/100001/chrome/browser/app_con... chrome/browser/app_controller_mac.mm:1234: [fileMenu itemWithTitle:l10n_util::GetNSString(IDS_SHARE_MAC)]; Maybe DCHECK(shareMenuItem) ? https://codereview.chromium.org/2950403002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/share_menu_controller.mm (right): https://codereview.chromium.org/2950403002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/share_menu_controller.mm:70: // Don't include "Add to Reading List" nit: period https://codereview.chromium.org/2950403002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/share_menu_controller.mm:86: [menu addItem:moreItem.get()]; You shouldn't need .get() in most cases. scoped_nsobject<T> is implicitly convertible to T*. https://codereview.chromium.org/2950403002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/share_menu_controller.mm:166: itemsToShare = @[ url, title ]; Should we always put the title in the item?
mgiuca@chromium.org changed reviewers: + mgiuca@chromium.org
Drive-by review from a Web Share perspective. Consider these comments non-required since we can obviously refactor it later. I'm just trying to consider what would be necessary to reuse this code from Web Share and I don't think it's in the right shape to do so at the moment. Note that we'd eventually need to call this code from ShareServiceImpl::Share: https://cs.chromium.org/chromium/src/chrome/browser/webshare/share_service_im... https://codereview.chromium.org/2950403002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/share_menu_controller.mm (right): https://codereview.chromium.org/2950403002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/share_menu_controller.mm:66: sharingServicesForItems:@[ [NSURL URLWithString:@"https://google.com"] ]]; I'm confused what this URL is used for (why there is a hard-coded "https://google.com" in the share service). https://codereview.chromium.org/2950403002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/share_menu_controller.mm:147: - (void)performShare:(NSMenuItem*)sender { I think this method is too tightly coupling the browser (tab strip model) and the Mac share interface (NSSharingService). This is okay for now, but from a Web Share perspective, we have no method to call into with a custom title and URL (since this reads directly from the tab strip model). As a first step, it would be good to split this up into two methods: performShareOfCurrentTab (name can be changed), which gets the title and URL from the tab strip model, and performShare, which takes title and URL as input and calls the NSSharingService. As a second step, I'd like to expose performShare externally so that I can call it from chrome/browser/webshare/share_service_impl.cc. It probably belongs in its own class, decoupled from the ShareMenuController. I'm not sure what we'd use as a "sender" when called from share_service_impl. Does that have to be some Cocoa control? |