1035586 - allow snippets in about:home to highlight sync in the firefox menu

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Madhava Enros [:madhava]]

The leading short-term possibility for promoting sync, in product, is the one on page 8 of this UX concepting deck: http://people.mozilla.org/~zfang/Others/FeaturePromotion_FFAccount_0701.pdf

It's the one about having a snippet that advertises sync and, when the user engages with the snippet, highlights the sync signup item in the menu using the update tour highlighting API.

A quick chat with MattN and Unfocused in IRC suggests that this will require some permissions changes, either to about:home or to wherever the snippets are served from. How could we make this work?

Comment 1

[Matthew N. [:MattN]]

I doubt we need a breakdown for this as it'll likely be one patch.

Note that we also added the option to open about:accounts?action=signup (bypassing the menu) in bug 998934 which may also be useful.

It seems like we could add about:home to the whitelist which also means dealing with the "about" scheme (currently only https is allowed by default). I think adding "about" to the scheme whitelist in general would be a security problem because I believe one can document.write to about:blank and there is also about:srcdoc which could potentially cause a security problem down the road (e.g. if we remove the window.top == window check). We could have about:home skip the scheme check though assuming there is no way to spoof about URIs.

Another option is to re-implement UITour behaviour specifically for about:home with its own listeners but that seems less maintainable.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

The PDF that was linked to in comment #0 is not available anymore. Can you upload that PDF to this bug?

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

The PermissionsUtils' importPrefBranch hardcoded http:// on the hostname. I discussed this with MattN and we agreed that it would be better to fix up the importPrefBranch code as compared to introducing some other way to manage this permission.

Note that about: permissions cannot be modified or viewed from the Page Info dialog or about:permissions. Using the Permission Manager will still allow us to have the same code paths as well as ability to disable the permission through a hotfix or future release.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8463590 [details] [diff] [review]
Patch

Review of attachment 8463590 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/PermissionsUtils.jsm
@@ +30,5 @@
>        try {
> +        uri = Services.io.newURI("http://" + host, null, null);
> +      } catch (e) {
> +        try {
> +          uri = Services.io.newURI(host, null, null);

Can you update the test (test_PermissionsUtils.js) to cover this?

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> Note that about: permissions cannot be modified or viewed from the Page Info
> dialog or about:permissions.

That a limitation in the UI code? Do we have a bug for that?

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch (with test updated)

I renamed the permission from text-permission to test-permission as it seems like it should have always been test-permission and was likely a typo.

Also added some checks at the beginning of the test to make sure that the permission action changes from 'unknown' to its expected action.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Blair McBride [:Unfocused] from comment #5)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> > Note that about: permissions cannot be modified or viewed from the Page Info
> > dialog or about:permissions.
> 
> That a limitation in the UI code? Do we have a bug for that?

SitePermissions.jsm is what blocks showing it:

  /* Checks whether a UI for managing permissions should be exposed for a given
   * URI. This excludes file URIs, for instance, as they don't have a host,
   * even though nsIPermissionManager can still handle them.
   */
  isSupportedURI: function (aURI) {
    return aURI.schemeIs("http") || aURI.schemeIs("https");
  },

Note that chrome-level pages that have the Firefox identity block don't have a button to view the Page Info dialog. People can right-click on use the content context menu to get to the Page Info dialog though.

I don't think it is worth it to support the Permissions pane for about: pages since they are effectively part of the application and disabling permissions for them may later confuse people down the road or potentially cause some feature to discontinue working without a clear explanation (for example: disabling popup windows for about:preferences).

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8463773 [details] [diff] [review]
Patch (with test updated)

Review of attachment 8463773 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/tests/xpcshell/test_PermissionsUtils.js
@@ +8,5 @@
>  //       converted into permissions on startup.
>  
>  
>  const PREF_ROOT = "testpermissions.";
> +const TEST_PERM = "test-permission";

So, it turns out I can't type :)

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

[Tracking Requested - why for this release]:
The Firefox desktop team + UX would like this ability for 32 to help promote Sync.

https://hg.mozilla.org/integration/fx-team/rev/46566f1ad643

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/46566f1ad643

Comment 11

[Lawrence Mandel [:lmandel] (use needinfo)]

jaws - If this looks good on m-c tomorrow, can you please request uplift so that we can get this into beta3?

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8463773 [details] [diff] [review]
Patch (with test updated)

Approval Request Comment
[Feature/regressing bug #]: request from marketing, push related to our "in product promotion" effort
[User impact if declined]: less ability for marketing to promote sync
[Describe test coverage new/current, TBPL]: api changes to PermissionsUtils are covered by xpcshell tests
[Risks and why]: about:home will now have access to run the uitour. add-ons can override/inject content in to about:home, but add-ons that can do this can already do anything else that they want
[String/UUID change made/needed]: none

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for beta32

Approval Request Comment
[Feature/regressing bug #]: request from marketing, push related to our "in product promotion" effort
[User impact if declined]: less ability for marketing to promote sync
[Describe test coverage new/current, TBPL]: api changes to PermissionsUtils are covered by xpcshell tests
[Risks and why]: about:home will now have access to run the uitour. add-ons can override/inject content in to about:home, but add-ons that can do this can already do anything else that they want
[String/UUID change made/needed]: none

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/39a43bd1f6cd

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8464477 [details] [diff] [review]
Patch for beta32

beta+ Please land today so that we can pickup this change for beta3.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-beta/rev/4bb3a5226b75