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

RESOLVED FIXED in Firefox 32

Status

()

Firefox
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: madhava, Assigned: jaws)

Tracking

unspecified
Firefox 34
Points:
3
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(firefox32+ fixed, firefox33+ fixed, firefox34 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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?

Updated

4 years ago
QA Whiteboard: [qa-]
Flags: firefox-backlog+
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.
OS: Mac OS X → All
Hardware: x86 → All

Updated

4 years ago
Points: --- → 3
Depends on: 1043505
The PDF that was linked to in comment #0 is not available anymore. Can you upload that PDF to this bug?
Flags: needinfo?(madhava)
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Summary: Breakdown: allow snippets in about:home to highlight sync in the firefox menu → allow snippets in about:home to highlight sync in the firefox menu

Updated

4 years ago
Iteration: --- → 34.1
Blocks: 1043505
No longer depends on: 1043505
Created attachment 8463590 [details] [diff] [review]
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.
Attachment #8463590 - Flags: review?(bmcbride)
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?
Attachment #8463590 - Flags: review?(bmcbride) → review-
(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?
Created attachment 8463773 [details] [diff] [review]
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.
Attachment #8463590 - Attachment is obsolete: true
Attachment #8463773 - Flags: review?(bmcbride)
(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 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 :)
Attachment #8463773 - Flags: review?(bmcbride) → review+
[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
tracking-firefox32: --- → ?
tracking-firefox33: --- → ?
Flags: needinfo?(madhava) → in-testsuite+
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/46566f1ad643
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
tracking-firefox32: ? → +
tracking-firefox33: ? → +
jaws - If this looks good on m-c tomorrow, can you please request uplift so that we can get this into beta3?
Flags: needinfo?(jaws)
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
Attachment #8463773 - Flags: approval-mozilla-aurora?
Flags: needinfo?(jaws)
Created attachment 8464477 [details] [diff] [review]
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
Attachment #8464477 - Flags: approval-mozilla-beta?
status-firefox32: --- → affected
status-firefox33: --- → affected
status-firefox34: --- → fixed
Attachment #8463773 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8464477 [details] [diff] [review]
Patch for beta32

beta+ Please land today so that we can pickup this change for beta3.
Attachment #8464477 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(jaws)
https://hg.mozilla.org/releases/mozilla-beta/rev/4bb3a5226b75
status-firefox32: affected → fixed
Flags: needinfo?(jaws)
You need to log in before you can comment on or make changes to this bug.