Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(2003)

Issue 2950403002: [Mac] Add system share menu

Created:
3 years, 6 months ago by lgrey
Modified:
3 years, 5 months ago
CC:
chromium-reviews, srahim+watch_chromium.org, mac-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

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
Unified diffs Side-by-side diffs Delta from patch set Stats (+468 lines, -4 lines) Patch
M chrome/app/generated_resources.grd View 1 2 3 4 5 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/app/nibs/MainMenu.xib View 1 2 3 4 5 1 chunk +6 lines, -4 lines 0 comments Download
M chrome/browser/app_controller_mac.h View 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/app_controller_mac.mm View 1 2 3 chunks +13 lines, -0 lines 1 comment Download
M chrome/browser/ui/BUILD.gn View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm View 1 2 2 chunks +9 lines, -0 lines 0 comments Download
A chrome/browser/ui/cocoa/share_menu_controller.h View 1 2 3 4 5 1 chunk +23 lines, -0 lines 0 comments Download
A chrome/browser/ui/cocoa/share_menu_controller.mm View 1 2 3 4 5 1 chunk +237 lines, -0 lines 5 comments Download
A chrome/browser/ui/cocoa/share_menu_controller_browsertest.mm View 1 2 3 4 5 1 chunk +167 lines, -0 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 37 (28 generated)
lgrey
PTAL :)
3 years, 5 months ago (2017-06-26 16:29:15 UTC) #22
lgrey
3 years, 5 months ago (2017-06-28 15:13:37 UTC) #24
Robert Sesek
Cool! Nice job keeping ShareMenuController mostly decoupled. I can see why you were trying to ...
3 years, 5 months ago (2017-06-28 15:37:55 UTC) #25
shrike
A few observations: * When you choose a Sharing service like Twitter or Facebook, the ...
3 years, 5 months ago (2017-06-28 18:49:04 UTC) #26
shrike
Also, I think the cl should be named something like "[Mac] Add System share menu", ...
3 years, 5 months ago (2017-06-28 18:51:24 UTC) #27
lgrey
On 2017/06/28 18:49:04, shrike wrote: > A few observations: > > * When you choose ...
3 years, 5 months ago (2017-06-29 21:11:37 UTC) #33
lgrey
https://codereview.chromium.org/2950403002/diff/80001/chrome/app/nibs/MainMenu.xib File chrome/app/nibs/MainMenu.xib (right): https://codereview.chromium.org/2950403002/diff/80001/chrome/app/nibs/MainMenu.xib#newcode144 chrome/app/nibs/MainMenu.xib:144: <menu key="submenu" title="Share" id="T9x-6Q-SHx"> On 2017/06/28 15:37:54, Robert Sesek ...
3 years, 5 months ago (2017-06-29 21:11:51 UTC) #34
Robert Sesek
https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/app_controller_mac.mm File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/2950403002/diff/80001/chrome/browser/app_controller_mac.mm#newcode1234 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 ...
3 years, 5 months ago (2017-06-30 21:09:34 UTC) #35
Matt Giuca
3 years, 5 months ago (2017-07-05 07:02:55 UTC) #37
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?

Powered by Google App Engine
This is Rietveld 408576698