1165438 - Use the displayOrigin option for all ContentPermissionPrompt notifications

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

Comment 1

[:Paolo Amadini]

Comment on attachment 8606440 [details] [diff] [review]
patch

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

::: browser/components/nsBrowserGlue.js
@@ +2402,5 @@
> +    if (!aOptions)
> +      aOptions = {};
> +    aOptions.displayOrigin = requestPrincipal.URI.schemeIs("file") ?
> +                             requestPrincipal.URI.path :
> +                             requestPrincipal.URI.host;

You want something like this to handle spaces in the file name correctly:

  aOptions.displayOrigin = (requestPrincipal.URI instanceof Ci.nsIFileURI) ?
                           requestPrincipal.URI.file.path :
                           requestPrincipal.URI.host;

I'm not sure if this can throw exceptions - I guess it cannot.

@@ +2466,1 @@
>        message = gBrowserBundle.GetStringFromName("geolocation.shareWithSite2");

This is the only instance differentiating the file / site strings. It is also the only case that applies different logic, as far as I can tell, but this may change in the future as we determine which cases allow persistent permissions and which ones don't.

I was thinking you may either leave the strings like this or consider removing the shareWithFile string to align with the other cases. (I don't think it's worth having different strings for the local file case in the other permission types.)

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +396,5 @@
>  pointerLock.alwaysAllow=Always allow hiding
>  pointerLock.alwaysAllow.accesskey=A
>  pointerLock.neverAllow=Never allow hiding
>  pointerLock.neverAllow.accesskey=N
> +pointerLock.title3=Would you like to allow the pointer to be hidden on this site?

I've only tested this case locally, but at least here it seems to me that you can write just "Would you like to allow the pointer to be hidden?" and it will work equally well. Probably geolocation could use this wording as well, and maybe other cases.

I'm not sure whom to ask for a UX review here at this point, but I think you could ask someone on the UX team if you'd like a second opinion.

Comment 2

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/59fa3030fe7a

Comment 3

[Dão Gottwald [:dao]]

I filed bug 1166236 on the strings.

Comment 4

[:Paolo Amadini]

(In reply to Dão Gottwald [:dao] from comment #3)
> I filed bug 1166236 on the strings.

Thanks!

I think there's a typo in the patch that landed (requestPrincipal.URI.path instead of requestPrincipal.URI.file.path) so users would still see things like "Document%20and%20Settings" rather than "Document and Settings".

Comment 5

[Pulsebot]

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

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/59fa3030fe7a
https://hg.mozilla.org/mozilla-central/rev/ea3f44561b36

Comment 7

[Jorge Villalobos [:jorgev] (he/him)]

It looks like this patch broke the Location Guard extension: https://addons.mozilla.org/addon/location-guard/. The extension tries to open a geolocation permission prompt from a resource:// URL, and in 41 it's throwing:

NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIFileURL.file] nsBrowserGlue.js:2287:0

Is this the expected behavior, and is there a workaround for this?

Comment 8

[Dão Gottwald [:dao]]

I don't think it's expected, but I'm also not sure I see how exactly this patch would have caused it. Either way, can you please file a new bug on this?