1471877 - Add link to configure shares on OSX

Status: VERIFIED FIXED

(Firefox :: Menus, enhancement, P1)

Description

[Dale Harvey (:daleharvey)]

In the page actions we have Share URL -> List of Share providers, we want to add a ... More link at the bottom that opens the share preferences so users know how to configure them, similiar to Safari

Comment 1

[Dale Harvey (:daleharvey)]

Hey Aaron, could I get the asset for the share icon you have @ https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/232626421_Explainer_-_Share, Thank you

Comment 2

[Dale Harvey (:daleharvey)]

Hey Marcus, taking a look into this, it seems like it used to be nice and simple

  NSURL *URL = [NSURL URLWithString:@"x-apple.systempreferences:com.apple.preferences.extensions?Share_Menu"];
  [[NSWorkspace sharedWorkspace] openURL:URL];

However this isnt working for me and seems like it was broken at some point, https://stackoverflow.com/questions/6652598/cocoa-button-opens-a-system-preference-page suggests that we need to add ScriptingBridge.framework to our build and use some fairly brittle looking code

Wondering if you know of any other options or if not, is adding the ScriptingBridge framework something that we would want to do?

Cheers

Comment 3

[Dale Harvey (:daleharvey)]

Clearing needinfo, I found some secret api's for this

Comment 4

[Dale Harvey (:daleharvey)]

Attachment: Bug 1471877 - Add link to share menu to configuration.

Review commit: https://reviewboard.mozilla.org/r/254100/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254100/

Comment 5

[Code Review Bot [:reviewbot]]

Comment on attachment 8989010 [details]
Bug 1471877 - Add link to share menu to configuration.

https://reviewboard.mozilla.org/r/254100/#review260830


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/browser-pageActions.js:1243
(Diff revision 1)
> -                                  gBrowser.selectedBrowser.contentTitle);
> +                                gBrowser.selectedBrowser.contentTitle);
> +      } else if (event.target.hasAttribute("share-more")) {
> +        sharingService.openSharingPreferences();
> -        }
> +      }
> -        PanelMultiView.hidePopup(BrowserPageActions.panelNode);
> +      PanelMultiView.hidePopup(BrowserPageActions.panelNode);
> -      });
> +    }

Error: Missing semicolon. [eslint: semi]

Comment 6

[Aaron Benson]

Icons can be found here: https://design.firefox.com/icons/viewer/

Comment 7

[Dale Harvey (:daleharvey)]

Comment on attachment 8989010 [details]
Bug 1471877 - Add link to share menu to configuration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254100/diff/1-2/

Comment 8

[:Gijs (he/him)]

Comment on attachment 8989010 [details]
Bug 1471877 - Add link to share menu to configuration.

https://reviewboard.mozilla.org/r/254100/#review263096

Few nits but the JS generally looks OK. I haven't looked at the ObjC. Note the icon thingy - may want to check with Aaron again.

::: browser/base/content/browser-pageActions.js:1239
(Diff revision 2)
> -        let shareName = event.target.getAttribute("share-name");
> +      let shareName = event.target.getAttribute("share-name");
> -        if (shareName) {
> +      if (shareName) {
> -          sharingService.shareUrl(shareName,
> +        sharingService.shareUrl(shareName,
> -                                  currentURI,
> +                                currentURI,
> -                                  gBrowser.selectedBrowser.contentTitle);
> +                                gBrowser.selectedBrowser.contentTitle);
> +      } else if (event.target.hasAttribute("share-more")) {

Could avoid setting this attribute and use event.target.classList.contains() to check whether it's this item?

::: browser/base/content/test/urlbar/browser_page_action_menu_share_mac.js:150
(Diff revision 2)
> +    // Click on share, panel should hide and sharingService should be
> +    // given the title of service to share with

Nit: please update the comment here.

::: browser/themes/shared/urlbar-searchbar.inc.css:301
(Diff revision 2)
> -#pageActionButton {
> +#pageActionButton, .share-more-button {
>    list-style-image: url("chrome://browser/skin/page-action.svg");

This isn't technically the icon in the design, but I don't see that icon in the icon viewer, either. Maybe check again with Aaron? Anyway, I guess we could land this initially...

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8989010 [details]
Bug 1471877 - Add link to share menu to configuration.

https://reviewboard.mozilla.org/r/254100/#review264500

::: widget/cocoa/nsMacSharingService.mm:159
(Diff revision 2)
> +     dataWithPropertyList:args
> +     format:NSPropertyListXMLFormat_v1_0
> +     options:0
> +     error:nil];
> +  NSAppleEventDescriptor* descriptor =
> +    [[NSAppleEventDescriptor alloc]

This alloc is not paired with a release, so the descriptor will be leaked.

::: widget/cocoa/nsMacSharingService.mm:168
(Diff revision 2)
> +  [[NSWorkspace sharedWorkspace] openURLs:@[ prefPaneURL ]
> +   withAppBundleIdentifier:nil
> +   options:NSWorkspaceLaunchAsync
> +   additionalEventParamDescriptor:descriptor
> +   launchIdentifiers:NULL];
> +

You'll need to add a [descriptor release] down here.

Comment 10

[Dale Harvey (:daleharvey)]

Comment on attachment 8989010 [details]
Bug 1471877 - Add link to share menu to configuration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254100/diff/2-3/

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8989010 [details]
Bug 1471877 - Add link to share menu to configuration.

https://reviewboard.mozilla.org/r/254100/#review264512

Comment 12

[Dale Harvey (:daleharvey)]

Comment on attachment 8989010 [details]
Bug 1471877 - Add link to share menu to configuration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254100/diff/3-4/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/68859b04588d
Add link to share menu to configuration. r=Gijs,mstange

Comment 14

[Noemi Erli[:noemi_erli]]

Backed out changeset 68859b04588d (bug 1471877) for failures in browser/content/browser-pageActions.js on a CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=68859b04588d32f435aba34628ce081d352d5788&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=188714292

Failure log:  https://treeherder.mozilla.org/logviewer.html#?job_id=188714292&repo=autoland&lineNumber=803

Backout: https://hg.mozilla.org/integration/autoland/rev/447b0cc51b8a97febdd2204f1dc0c617fb245e4f

Comment 15

[Dale Harvey (:daleharvey)]

Ugh sorry, I had fixed this but forgot to push the latest to mozreview, will do so now

Comment 16

[Dale Harvey (:daleharvey)]

Comment on attachment 8989010 [details]
Bug 1471877 - Add link to share menu to configuration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254100/diff/4-5/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2df15ca705ec
Add link to share menu to configuration. r=Gijs,mstange

Comment 18

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2df15ca705ec

Comment 19

[Oana Botisan, Desktop QA]

I verified the fix on the latest Nightly 64.0a1 and beta 63.0b11 on macOS 10.13. The "More..." button is displayed in the Page Action Menu and it opens the share preferences.