1292639 - [FlyWeb] Ask user permission before allowing navigator.publishServer

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Kannan Vijayan [:djvj]]

When a webpage uses |navigator.publishServer| to publish a FlyWeb server, the call succeeds implicitly.

Publishing a server should cause a user prompt for the page, requesting the user to authorize the action.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: add-publish-server-user-prompt.patch

Draft patch for permission support.  Almost complete.  The icon in the user prompt doesn't show up yet.

Comment 2

[Kannan Vijayan [:djvj]]

Attachment: add-flyweb-user-prompt.patch

This should be the final patch.  Baku is on PTO.. gonna find proper reviewers for this.

Comment 3

[Kannan Vijayan [:djvj]]

Attachment: add-publish-server-user-prompt.patch

Updated patch.  This works on both e10s and non-e10s modes (tested on linux only for now).  Android testing shows that the call to publishServer immediately fails with an error.  Still need to find and fix the issue there.

Comment 4

[Kannan Vijayan [:djvj]]

Attachment: publish-server-user-prompt.patch

Updated patch.  Works on android.  Desktop works in both e10s and non-e10s mode.  Debug logging removed.  Haven't tested on windows yet, but it should work fine.  Will look for reviewers after windows testing.  Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ed0c7a52897

Comment 5

[Kannan Vijayan [:djvj]]

Ok, try run looks good.  The two OSX taskcluster build-reds are due to rust binaries having a stale version on the build machines, not the patch.  Putting up for review.

Comment 6

[Kannan Vijayan [:djvj]]

Comment on attachment 8782550 [details] [diff] [review]
publish-server-user-prompt.patch

Adding mconley for help with finding UI reviewers.

Comment 7

[(no longer active)]

Comment on attachment 8782550 [details] [diff] [review]
publish-server-user-prompt.patch

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

::: browser/app/profile/firefox.js
@@ +758,5 @@
>  // By default, we don't want protocol/content handlers to be registered from a different host, see bug 402287
>  pref("gecko.handlerService.allowRegisterFromDifferentHost", false);
>  
>  pref("browser.geolocation.warning.infoURL", "https://www.mozilla.org/%LOCALE%/firefox/geolocation/");
> +pref("browser.flywebPublishServer.warning.infoURL", "https://flyweb.github.io");

Is this URL temporary?  If not, it needs to change to make the content localizable.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +211,5 @@
>       used to provide accessible labels to users of assistive technology like screenreaders.
>       It is not possible to see them visually in the UI. -->
>  <!ENTITY urlbar.defaultNotificationAnchor.label         "View a notification">
>  <!ENTITY urlbar.geolocationNotificationAnchor.label     "View the location request">
> +<!ENTITY urlbar.flyWebPublishServerNotificationAnchor.label     "View the flyweb pubish request">

Nit: *publish

This doesn't look like a very good string message for something displayed to users.

But more importantly, if you decide to use words such as 'flyweb' which won't mean anything to our users (which is a bad choice) you should remember that it will also mean nothing to our localizers, so they won't have any idea how to translate it.  You would need to provide a localization note helping them translate it.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +377,5 @@
>  geolocation.shareWithFile2=Would you like to share your location with this file?
>  
> +flyWebPublishServer.shareWithSite=Would you like to let this site start a server accessible to nearby devices and people?
> +flyWebPublishServer.allowPublishServer=Allow Server
> +flyWebPublishServer.allowPublishServer.accesskey=a

Nit: A

@@ +378,5 @@
>  
> +flyWebPublishServer.shareWithSite=Would you like to let this site start a server accessible to nearby devices and people?
> +flyWebPublishServer.allowPublishServer=Allow Server
> +flyWebPublishServer.allowPublishServer.accesskey=a
> +flyWebPublishServer.denyPublishServer=Deny Server

We usually use the term "Block" in our UI as an opposite of "Allow".

@@ +379,5 @@
> +flyWebPublishServer.shareWithSite=Would you like to let this site start a server accessible to nearby devices and people?
> +flyWebPublishServer.allowPublishServer=Allow Server
> +flyWebPublishServer.allowPublishServer.accesskey=a
> +flyWebPublishServer.denyPublishServer=Deny Server
> +flyWebPublishServer.denyPublishServer.accesskey=n

Nit: please use the first letter of the string unless it clashes with an existing shortcut.

::: browser/themes/shared/notification-icons.inc.css
@@ +102,5 @@
>  %endif
>  }
>  
> +.flyweb-publish-server-icon {
> +  list-style-image: url("chrome://flyweb/skin/icon-64.png");

What's chrome://flyweb?  This makes it look like the content is being loaded from an extension.

Please put resources in browser/ and mobile/ under chrome://browser/ and those in toolkit/ under chrome://global/.

@@ +117,5 @@
>  %endif
>  }
>  
> +.popup-notification-icon[popupid="flyweb-publish-server"] {
> +  list-style-image: url("chrome://flyweb/skin/icon-64.png");

Ditto.

::: dom/flyweb/FlyWebPublishedServer.cpp
@@ +261,5 @@
>  FlyWebPublishedServerChild::FlyWebPublishedServerChild(nsPIDOMWindowInner* aOwner,
>                                                         const nsAString& aName,
>                                                         const FlyWebPublishOptions& aOptions)
>    : FlyWebPublishedServer(aOwner, aName, aOptions)
> +  , mActorDestroyed(true)

With this change you're changing the meaning of the assertions in this code.  After your patch, MOZ_ASSERT(!mActorDestroyed) now also checks whether PermissionGranted() has been called too.

I think it's better to add a mActorCreated variable separately and adjust the assertions to express the exact condition being asserted.

::: dom/flyweb/FlyWebService.cpp
@@ +47,5 @@
> +  , public nsIRunnable
> +{
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(FlyWebPublishServerPermissionCheck, nsIContentPermissionRequest)

Why does this need to participate in cycle collection?  It doesn't seem that this class can participate in a cycle.  If my understanding here isn't correct, please describe how this can happen!

@@ +59,5 @@
> +
> +  const nsCString& ServiceName() const
> +  {
> +    return mServiceName;
> +  }

Nit: this seems to be dead code.

@@ +70,5 @@
> +  NS_IMETHOD Run() override
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    mWindow = nsGlobalWindow::GetInnerWindowWithId(mWindowID)->AsInner();

This will crash when GetInnerWindowWithId() returns null.

@@ +132,5 @@
> +    return NS_OK;
> +  }
> +
> +private:
> +  nsresult Resolve(bool aResolve)

Make this return void please.

@@ +1001,5 @@
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      LOG_E("PublishServer: Failed to get main thread for %s",
> +            NS_ConvertUTF16toUTF8(aName).get());
> +      return MakeRejectionPromise(__func__);
> +    }

You _are_ on the main thread here, so this all looks pointless.

Instead, please us NS_DispatchToCurrentThread().

@@ +1016,5 @@
> +  } else {
> +    // If aWindow is null, we're definitely in the e10s parent process.
> +    // In this case, we know that permission has already been granted
> +    // by the user because of content-process prompt.
> +    MOZ_ASSERT(XRE_GetProcessType() != GeckoProcessType_Content);

This assertion is checking something different than the content, since we have many process types.  Please check that you're in the main process directly.

::: mobile/android/locales/en-US/chrome/browser.properties
@@ +135,5 @@
> +flyWebPublishServer.allow=Allow
> +flyWebPublishServer.dontAllow=Deny
> +flyWebPublishServer.ask=Would you like to let this site start a server accessible to nearby devices and people?
> +flyWebPublishServer.dontAskAgain=Don't ask again for this site
> +flyWebPublishServer.publishServer=Publish Server

Repeating strings like this is considered bad form, because it causes unnecessary work for our localizers.  Please move the shared strings to somewhere under toolkit/ so that you can share them between browser/ and mobile/.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Hm - I think a lot of what Ehsan is noticing is the tension between the FlyWeb UI currently being a System Add-on, vs the built-in support we have for content permissions.

Let me see if there are extension hook points you can use instead, which will make this more self contained.

Comment 9

[(no longer active)]

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #8)
> Hm - I think a lot of what Ehsan is noticing is the tension between the
> FlyWeb UI currently being a System Add-on, vs the built-in support we have
> for content permissions.
> 
> Let me see if there are extension hook points you can use instead, which
> will make this more self contained.

Did you mean the comments about the strings or something else?

Comment 10

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Ehsan Akhgari from comment #9)
> (In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #8)
> > Hm - I think a lot of what Ehsan is noticing is the tension between the
> > FlyWeb UI currently being a System Add-on, vs the built-in support we have
> > for content permissions.
> > 
> > Let me see if there are extension hook points you can use instead, which
> > will make this more self contained.
> 
> Did you mean the comments about the strings or something else?

I meant regarding the strings, but also about the chrome://flyweb URI. We're attempting to isolate as much of the FlyWeb UI into a system add-on as possible, but I think we've found an API limitation here wrt content permissions. I've noticed a similar thing crop up in bug 1289974.

What I'm suggesting is that we design an API for system add-ons to register handlers for content permission prompts. I looked for a pre-existing hook, but had no luck - I think we're going to have to add one. I've filed bug 1297475 about this, and will kick off a firefox-dev discussion about it.

Comment 11

[Kannan Vijayan [:djvj]]

Thanks mike.  In the meantime, what's the path forward on these?  Can we assume that because the URLs refer to a system addon it's ok since we know for a fact that they will be present?

Comment 12

[Mike Conley (:mconley) (:⚙️)]

(In reply to Kannan Vijayan [:djvj] from comment #11)
> Thanks mike.  In the meantime, what's the path forward on these?  Can we
> assume that because the URLs refer to a system addon it's ok since we know
> for a fact that they will be present?

Hello seems to have set the precedent (at least until it was removed):

https://dxr.mozilla.org/mozilla-release/rev/440936afbdbcc443de50100956a446f7702f118d/browser/components/about/AboutRedirector.cpp#103

so it's probably still okay, yes.

What's your timeline on this? I'm hoping to have consensus with the Firefox org and a patch up for bug 1297475 by the end of the week. Would you be willing to wait until then, and then modify the patch to use the mechanism that gets added instead?

Comment 13

[Kannan Vijayan [:djvj]]

(In reply to :Ehsan Akhgari from comment #7)
> Comment on attachment 8782550 [details] [diff] [review]
> publish-server-user-prompt.patch
> 
> Is this URL temporary?  If not, it needs to change to make the content
> localizable.

Yes this is temporary.  This will need to be fixed before we actually go live in beta or release.  No scope to fix it now.

> ::: browser/locales/en-US/chrome/browser/browser.dtd
> @@ +211,5 @@
> >       used to provide accessible labels to users of assistive technology like screenreaders.
> >       It is not possible to see them visually in the UI. -->
> >  <!ENTITY urlbar.defaultNotificationAnchor.label         "View a notification">
> >  <!ENTITY urlbar.geolocationNotificationAnchor.label     "View the location request">
> > +<!ENTITY urlbar.flyWebPublishServerNotificationAnchor.label     "View the flyweb pubish request">
> 
> Nit: *publish
> 
> This doesn't look like a very good string message for something displayed to
> users.
> 
> But more importantly, if you decide to use words such as 'flyweb' which
> won't mean anything to our users (which is a bad choice) you should remember
> that it will also mean nothing to our localizers, so they won't have any
> idea how to translate it.  You would need to provide a localization note
> helping them translate it.

Same as above.  We are not shipping the feature in anything but nightly (and preffed off on nightly too).  Localization will need to be fixed before we move this up the release tier, but we're not in that phase right now.


> ::: browser/themes/shared/notification-icons.inc.css
> @@ +102,5 @@
> >  %endif
> >  }
> >  
> > +.flyweb-publish-server-icon {
> > +  list-style-image: url("chrome://flyweb/skin/icon-64.png");
> 
> What's chrome://flyweb?  This makes it look like the content is being loaded
> from an extension.
> 
> Please put resources in browser/ and mobile/ under chrome://browser/ and
> those in toolkit/ under chrome://global/.

The resources are being referred to from a system extension.  They should always be there.  As mconley noted, this is friction between packaging the FlyWeb UI as a system-addon, and the fact that the permission request infrastructure doesn't play well with it.  I don't know if there's a good way to fix this without just duplicating the resources in the extension onto the main tree, which feels worse to me.

Thoughts?


> ::: dom/flyweb/FlyWebService.cpp
> @@ +47,5 @@
> > +  , public nsIRunnable
> > +{
> > +public:
> > +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> > +  NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(FlyWebPublishServerPermissionCheck, nsIContentPermissionRequest)
> 
> Why does this need to participate in cycle collection?  It doesn't seem that
> this class can participate in a cycle.  If my understanding here isn't
> correct, please describe how this can happen!

Will investigate this more closely.  We do hold a handle to window and a FlyWebPublishedServer, but I don't know for sure that this leads to a cycle.  The corresponding permission checker for geolocation code DOES implement cycle-collectable, so I figured there's some way that the runnable could be reachable from the window.  I may be wrong though.

> ::: mobile/android/locales/en-US/chrome/browser.properties
> @@ +135,5 @@
> > +flyWebPublishServer.allow=Allow
> > +flyWebPublishServer.dontAllow=Deny
> > +flyWebPublishServer.ask=Would you like to let this site start a server accessible to nearby devices and people?
> > +flyWebPublishServer.dontAskAgain=Don't ask again for this site
> > +flyWebPublishServer.publishServer=Publish Server
> 
> Repeating strings like this is considered bad form, because it causes
> unnecessary work for our localizers.  Please move the shared strings to
> somewhere under toolkit/ so that you can share them between browser/ and
> mobile/.

This doesn't seem to be the approach adopted by the other content-permission-request users (e.g. geolocation, which I used as a reference when implementing this).  Do we really want to unify these under toolkit or keep it consistent with existing practice?

Comment 14

[Kannan Vijayan [:djvj]]

Attachment: add-publish-server-user-prompt.patch

Updated patch.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8785411 [details] [diff] [review]
add-publish-server-user-prompt.patch

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

::: browser/app/profile/firefox.js
@@ +762,5 @@
>  // By default, we don't want protocol/content handlers to be registered from a different host, see bug 402287
>  pref("gecko.handlerService.allowRegisterFromDifferentHost", false);
>  
>  pref("browser.geolocation.warning.infoURL", "https://www.mozilla.org/%LOCALE%/firefox/geolocation/");
> +pref("browser.flywebPublishServer.warning.infoURL", "https://flyweb.github.io");

Since this URL is being hard-coded, and this is restricted to Nightly-only (and, as we discussed, once bug 1297475 is fixed, be isolated to the System Add-on), for now, let's hard-code this into the code in nsBrowserGlue.js.

There's precedent for this - see the patch that was added in bug 1064885 (and subsequently torn out in bug 1227230).

::: browser/base/content/browser.xul
@@ +716,5 @@
>                    <image id="default-notification-icon" class="notification-anchor-icon" role="button"
>                           aria-label="&urlbar.defaultNotificationAnchor.label;"/>
>                    <image id="geo-notification-icon" class="notification-anchor-icon geo-icon" role="button"
>                           aria-label="&urlbar.geolocationNotificationAnchor.label;"/>
> +                  <image id="flyweb-publish-server-notification-icon" class="notification-anchor-icon flyweb-publish-server-icon" role="button"

Instead of adding this to browser.xul, I suggest we modify the nsBrowserGlue.js code that calls PopupNotification to add this node at run-time. You should be able to set the src on the created element as the URI for the FlyWeb icon.

::: browser/components/nsBrowserGlue.js
@@ +2614,5 @@
>                       "geo-notification-icon", options);
>    },
>  
> +  _promptFlyWebPublishServer : function(aRequest) {
> +    var message = gBrowserBundle.GetStringFromName("flyWebPublishServer.shareWithSite");

Let's hardcode the message for now.

let, not var

@@ +2615,5 @@
>    },
>  
> +  _promptFlyWebPublishServer : function(aRequest) {
> +    var message = gBrowserBundle.GetStringFromName("flyWebPublishServer.shareWithSite");
> +    var actions = [

let, not var

@@ +2617,5 @@
> +  _promptFlyWebPublishServer : function(aRequest) {
> +    var message = gBrowserBundle.GetStringFromName("flyWebPublishServer.shareWithSite");
> +    var actions = [
> +      {
> +        stringId: "flyWebPublishServer.allowPublishServer",

This is unfortunate that we have to use stringIds here. :(

@@ +2620,5 @@
> +      {
> +        stringId: "flyWebPublishServer.allowPublishServer",
> +        action: Ci.nsIPermissionManager.ALLOW_ACTION,
> +        expireType: Ci.nsIPermissionManager.EXPIRE_SESSION,
> +        callback: function() {}

Looking at the code, it looks like callback is optional. You can remove these empty functions.

@@ +2626,5 @@
> +      {
> +        stringId: "flyWebPublishServer.denyPublishServer",
> +        action: Ci.nsIPermissionManager.DENY_ACTION,
> +        expireType: Ci.nsIPermissionManager.EXPIRE_SESSION,
> +        callback: function() {}

Same here for removal.

@@ +2631,5 @@
> +      }
> +    ];
> +
> +    let options = {
> +      learnMoreURL: Services.urlFormatter.formatURLPref("browser.flywebPublishServer.warning.infoURL"),

So this is where we can hardcode the URL (precedent: bug 1227230)

@@ +2632,5 @@
> +    ];
> +
> +    let options = {
> +      learnMoreURL: Services.urlFormatter.formatURLPref("browser.flywebPublishServer.warning.infoURL"),
> +    };

We can also use popupIconURL: <chrome URL to the icon> to set the icon that will show up in the popup itself. We should do that instead of adding it to the stylesheet.

@@ +2633,5 @@
> +
> +    let options = {
> +      learnMoreURL: Services.urlFormatter.formatURLPref("browser.flywebPublishServer.warning.infoURL"),
> +    };
> +

This is where we should dynamically create the anchor node if it doesn't already exist.

This is how you can get at the window object:

let browser = this._getBrowserForRequest(aRequest);
let chromeDoc = browser.ownerDocument;

and then you can use chromeDoc.getElementById to see if the node exists already. If not, create it (and set the src on it to point to your icon URI directly instead of adding it to the stylesheet)

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +210,5 @@
>       used to provide accessible labels to users of assistive technology like screenreaders.
>       It is not possible to see them visually in the UI. -->
>  <!ENTITY urlbar.defaultNotificationAnchor.label         "View a notification">
>  <!ENTITY urlbar.geolocationNotificationAnchor.label     "View the location request">
> +<!ENTITY urlbar.flyWebPublishServerNotificationAnchor.label     "View the flyweb pubish request">

Let's hardcode this where we create the node (this should probably not mention "flyweb", and there's a typo: "pubish" to "publish").

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +377,5 @@
>  geolocation.neverShareLocation.accesskey=N
>  geolocation.shareWithSite2=Would you like to share your location with this site?
>  geolocation.shareWithFile2=Would you like to share your location with this file?
>  
> +flyWebPublishServer.shareWithSite=Would you like to let this site start a server accessible to nearby devices and people?

These need LOCALIZATION NOTE's describing what these are for and that they don't need to be translated, since the feature is Nightly only. They're only here in the properties list because we're using an API that expects string keys.

::: browser/themes/shared/notification-icons.inc.css
@@ +55,5 @@
>  .pointerLock-icon,
>  .popup-icon,
>  .screen-icon,
>  .desktop-notification-icon,
> +.flyweb-publish-server-icon,

Let's remove these changes and use the hardcoded mechanisms I mentioned above.

::: mobile/android/chrome/content/PermissionsHelper.js
@@ +17,5 @@
>        label: "geolocation.location",
>        allowed: "geolocation.allow",
>        denied: "geolocation.dontAllow"
>      },
> +    "flyweb-publish-server": {

You're going to need someone from Fennec to review this and the other changes under mobile. Maybe snorp?

Comment 16

[(no longer active)]

Comment on attachment 8785411 [details] [diff] [review]
add-publish-server-user-prompt.patch

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

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +210,5 @@
>       used to provide accessible labels to users of assistive technology like screenreaders.
>       It is not possible to see them visually in the UI. -->
>  <!ENTITY urlbar.defaultNotificationAnchor.label         "View a notification">
>  <!ENTITY urlbar.geolocationNotificationAnchor.label     "View the location request">
> +<!ENTITY urlbar.flyWebPublishServerNotificationAnchor.label     "View the flyweb pubish request">

Since this string is intended to be a temporary string that is meant to be replaced with something better, please add a LOCALIZATION NOTE telling the localizers that they should ignore it for now.

Also, please fix the typo!

::: dom/flyweb/FlyWebService.cpp
@@ +46,5 @@
> +  : public nsIContentPermissionRequest
> +  , public nsIRunnable
> +{
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS

I double checked and there's absolutely no way this can end up in a cycle.  Please remove the CC garbage from this class.

@@ +998,5 @@
> +  if (aWindow) {
> +    MOZ_ASSERT(NS_IsMainThread());
> +    nsresult rv = NS_DispatchToCurrentThread(
> +      MakeAndAddRef<FlyWebPublishServerPermissionCheck>(
> +        NS_ConvertUTF16toUTF8(aName), aWindow->WindowID(), server));

As discussed offline, since the permission prompt that will be triggered from here relies on resources (strings and images) from the system add-on, you'd need to check here that the add-on is present and enabled before invoking the UI.

Looking at the add-on manager API, this seems to be very easy to achieve by calling amIAddonPathService::MapURIToAddonId() to check that a URL such as "chrome://flyweb/skin/icon-64.png" is mapped to the flyweb's add-on ID.  This is a synchronous API, so you can quickly reject the promise here if the check fails.

::: mobile/android/locales/en-US/chrome/browser.properties
@@ +135,5 @@
> +flyWebPublishServer.allow=Allow
> +flyWebPublishServer.dontAllow=Deny
> +flyWebPublishServer.ask=Would you like to let this site start a server accessible to nearby devices and people?
> +flyWebPublishServer.dontAskAgain=Don't ask again for this site
> +flyWebPublishServer.publishServer=Publish Server

To me, the argument for consistency with geolocation code doesn't trump consistency with everything else in the code base.  But more importantly, from your other comment about the strings it seems that we may not want our localizers to waste their time localizing this stuff at all anyways?  If that's the case, please add LOCALIZATION NOTEs to that effect here.

Comment 17

[Kannan Vijayan [:djvj]]

Attachment: publish-server-user-prompt.patch

Updated patch with review comments addressed.

Comment 18

[Kannan Vijayan [:djvj]]

Sebastian, could you review the android-chrome relevant changes in this patch?  It's a content permission prompt change to ask user consent for web-pages using the |navigator.publishServer| API.

Comment 19

[(no longer active)]

Comment on attachment 8786108 [details] [diff] [review]
publish-server-user-prompt.patch

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

Sorry for another round, but minusing because of the missing check for the flyweb system add-on.

::: dom/flyweb/FlyWebService.cpp
@@ +65,5 @@
> +  NS_IMETHOD Run() override
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    {

Why the nested scope?  There's nothing with a ctor in the nested scope, so please just remove it.

@@ +992,5 @@
> +    rv = NS_NewURI(getter_AddRefs(uri), NS_LITERAL_CSTRING("chrome://flyweb/skin/icon-64.png"));
> +    if (NS_FAILED(rv)) {
> +      return MakeRejectionPromise(__func__);
> +    }
> +    

Nit: trailing space.

@@ +994,5 @@
> +      return MakeRejectionPromise(__func__);
> +    }
> +    
> +    JSAddonId *addonId = MapURIToAddonID(uri);
> +    if (!addonId) {

You're just checking that _an_ add-on is providing this URL.  You need to check to make sure it's actually the flyweb add-on.  (Note that JSAddonId is just a JS atom.)

::: mobile/android/locales/en-US/chrome/browser.properties
@@ +132,5 @@
>  desktopNotification.notifications=Notifications
>  
> +# FlyWeb UI
> +# LOCALIZATION NOTE
> +# This is an experimental feature only shipping in Nightly, and doesn't need translation.

Please adhere to the format of the rest of the l10n notes.  That is,

LOCALIZATION NOTE (name.of.your.string): Note goes here.

You'd need to repeat it per each string you're adding the note to.

(Note that automated tools may try to parse out the notes and display them in the translation tools.)

Comment 20

[Kannan Vijayan [:djvj]]

Attachment: publish-server-user-prompt.patch

Updated patch with ehsan's comments addressed.

Comment 21

[(no longer active)]

Comment on attachment 8786433 [details] [diff] [review]
publish-server-user-prompt.patch

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

Looks great, thanks!  r=me with the nits below fixed.

::: dom/flyweb/FlyWebService.cpp
@@ +999,5 @@
> +      return MakeRejectionPromise(__func__);
> +    }
> +
> +    JSFlatString* flat = JS_ASSERT_STRING_IS_FLAT(JS::StringOfAddonId(addonId));
> +    nsString addonIdString;

Nit: nsAutoString would be a tiny bit more efficient for small strings.

@@ +1002,5 @@
> +    JSFlatString* flat = JS_ASSERT_STRING_IS_FLAT(JS::StringOfAddonId(addonId));
> +    nsString addonIdString;
> +    AssignJSFlatString(addonIdString, flat);
> +    nsCString addonIdCString = NS_ConvertUTF16toUTF8(addonIdString);
> +    if (!addonIdCString.Equals("[e-mail]")) {

Instead of converting strings manually like this, you could simply do:

  if (!addonIdString.EqualsLiteral("[e-mail]"))

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8786433 [details] [diff] [review]
publish-server-user-prompt.patch

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

Just a few things to fix / answer. Otherwise this looks good.

::: browser/components/nsBrowserGlue.js
@@ +2620,5 @@
> +      {
> +        stringId: "flyWebPublishServer.allowPublishServer",
> +        action: Ci.nsIPermissionManager.ALLOW_ACTION,
> +        expireType: Ci.nsIPermissionManager.EXPIRE_SESSION,
> +        callback: function() {}

It turns out that you don't need to supply these if they don't do anything.

@@ +2631,5 @@
> +      }
> +    ];
> +
> +    let options = {
> +      learnMoreURL: "https://flyweb.github.io",

You should be able to set popupIconURL here and set it to chrome://flyweb/skin/icon-64.png, and then avoid the notification-icons.inc.css change.

@@ +2642,5 @@
> +      let notificationPopupBox = chromeDoc.getElementById("notification-popup-box");
> +      let notificationIcon = chromeDoc.createElement("image");
> +      notificationIcon.setAttribute("id", "flyweb-publish-server-notification-icon");
> +      notificationIcon.setAttribute("class", "notification-anchor-icon flyweb-publish-server-icon");
> +      notificationIcon.setAttribute("style", "filter: url(chrome://browser/skin/filters.svg#fill); fill: currentColor; opacity: .4; list-style-image: url(chrome://flyweb/skin/icon-64.png);");

Out of curiosity, was setting the src on the notificationIcon insufficient? This kind of scripted styling is usually avoided if possible.

@@ +2761,5 @@
>      case "desktop-notification":
>        this._promptWebNotifications(request);
>        break;
> +    case "flyweb-publish-server":
> +      this._promptFlyWebPublishServer(request);

If we want to restrict this to Nightly, let's wrap this call to promptFlyWebPublishServer with:

if (AppConstants.NIGHTLY_BUILD) {

}

Comment 23

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8786433 [details] [diff] [review]
publish-server-user-prompt.patch

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

The Android bits look good to me.

Comment 24

[Kannan Vijayan [:djvj]]

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #22)
> Comment on attachment 8786433 [details] [diff] [review]
> 
> Out of curiosity, was setting the src on the notificationIcon insufficient?
> This kind of scripted styling is usually avoided if possible.

It does seem to work to just use an |src| attribute.  Doing that.

Other comments addressed.  Putting up final patch for r? soon.

Comment 25

[Kannan Vijayan [:djvj]]

Attachment: publish-server-user-prompt.patch

Updated patch.  Ehsan's final review comments addressed.  All review comments from conley also addressed.

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8786885 [details] [diff] [review]
publish-server-user-prompt.patch

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

This looks good to me. Great work, djvj!

Comment 27

[Kannan Vijayan [:djvj]]

Try run looks ok: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e34d05109d4

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbe18f181112
Ask user permission before allowing navigator.publishServer. r=mconley r=ehsan r=sebastian

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cbe18f181112

Comment 30

[Francesco Lodolo [:flod]]

Mostly an FYI for the future: localizers are going to see these strings and they are going to translate them to make dashboards and tools happy (no missing strings), adding a note "don't localize them" might be more confusing than helpful. On the other hand, explaining that the feature is experimental and available only on Nightly is a good idea.

Not sure if this is the right place to ask, but the strings look quite strange:
* /browser only has Allow/Deny buttons, no associated messages or explanations.
* "Would you like to let this site start a server accessible to nearby devices and people?". A server is not really accessible to people.
* "Publish Server". Didn't you just ask to "start" a server?

Is there a screenshot somewhere of the mobile UI? Because I can hardly put together "Publish Server", "Allow", "Deny".

Comment 31

[Kannan Vijayan [:djvj]]

(In reply to Francesco Lodolo [:flod] from comment #30)
> Mostly an FYI for the future: localizers are going to see these strings and
> they are going to translate them to make dashboards and tools happy (no
> missing strings), adding a note "don't localize them" might be more
> confusing than helpful. On the other hand, explaining that the feature is
> experimental and available only on Nightly is a good idea.
> 
> Not sure if this is the right place to ask, but the strings look quite
> strange:
> * /browser only has Allow/Deny buttons, no associated messages or
> explanations.
> * "Would you like to let this site start a server accessible to nearby
> devices and people?". A server is not really accessible to people.
> * "Publish Server". Didn't you just ask to "start" a server?
> 
> Is there a screenshot somewhere of the mobile UI? Because I can hardly put
> together "Publish Server", "Allow", "Deny".

Thanks for bringing this to my attention.  I'm actually working with Ryan Feeley to do user testing on the right phrasing for this permission prompt.  When we get results back from that I was planning on changing these strings.

CC-ing ryan on this bug so he's aware of the discussion.

Currently we're testing phrasing along the lines of "Would you like to let <website.com> serve a web-app to devices and people on your network?".  There are a few different variants of that sort of phrasing we're testing. Hoping that comes back with useful survey feedback.

What sort of a timeline do you feel is adequate to get the phrasing fixed in nightly?  I can push a change immediately to change the comments before the localization strings to say "This is an experimental feature", if that's OK for the short term..

Comment 32

[Francesco Lodolo [:flod]]

If possible I would fix the comments, removing the part about "you're not supposed to translate this". 

As for changing the strings, it depends on which version of Firefox you're targeting: strings should land only in mozilla-central and ride the trains, not jump them. As for changing the strings, at this point you'll also need to follow this guideline
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

Comment 33

[Mike Conley (:mconley) (:⚙️)]

(In reply to Francesco Lodolo [:flod] from comment #32)
> If possible I would fix the comments, removing the part about "you're not
> supposed to translate this". 

I'll take responsibility for that - I'm pretty sure I suggested that to djvj as a way of avoiding extra work on the part of our localizers.

Comment 34

[(no longer active)]

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #33)
> (In reply to Francesco Lodolo [:flod] from comment #32)
> > If possible I would fix the comments, removing the part about "you're not
> > supposed to translate this". 
> 
> I'll take responsibility for that - I'm pretty sure I suggested that to djvj
> as a way of avoiding extra work on the part of our localizers.

So did I.

It's sad that we have no way to land strings for experimental features without overloading our localizers.  :(

Comment 35

[Francesco Lodolo [:flod]]

(In reply to :Ehsan Akhgari from comment #34)
> It's sad that we have no way to land strings for experimental features
> without overloading our localizers.  :(

Technically we have: land the strings in properties files outside of standard paths used for localization, they'll be invisible to tools.

It's not pretty, and I don't think we ever used it for mobile, but it won't force you to hardcode strings and later figure out where the strings were, you just need to move them into files exposed for localization.

Comment 36

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e68e09a4334c
Backout cbe18f181112 for crashes - bug 1292639

Comment 37

[Kannan Vijayan [:djvj]]

Re-opening this so we can land again with fewer crashes.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/89a168219747
Ask user permission before allowing navigator.publishServer. r=mconley r=ehsan r=sebastian

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/89a168219747