Closed Bug 1309123 Opened 8 years ago Closed 7 years ago

Show persistent-storage permission request notification

Categories

(Firefox :: Site Permissions, defect)

Product:
Firefox
Component:
Site Permissions
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: Fischer, Assigned: Fischer)

References

Details

(Whiteboard: [storage-v1])

Attachments

(7 files, 8 obsolete files)

Bug 1309123 - Show persistent-storage permission request notification,
58 bytes, text/x-review-board-request
florian
: review+
Details
persistent-storage-permission-prompts-v2.png
492.11 KB, image/png
Details
persistent-storage-permission-prompts-v3.png
707.20 KB, image/png
Details
Screenshot of the existing prompt
26.93 KB, image/png
Details
Icon comparison.png
102.54 KB, image/png
Details
notification-icons.svg
12.28 KB, image/svg+xml
Details
bug-1286717-wip-patch.patch
23.80 KB, patch
Details | Diff | Splinter Review 
While website requests persist storage permission, the persist-storage permission request notification should show to ask user for permission.
Assignee: nobody → fliu
Blocks: 1309118
Depends on: 1286717
There are already the front-end test patch and the DOM test patch in the bug 1286717 to enable the permission notification.
Blocks: 1312347
No longer blocks: 1312347
Whiteboard: [storage-v1]
Blocks: 1313313
No longer blocks: 1313313
Depends on: 1313313

    
    

    
  


  

        Attachment #8808929 -
        Attachment is obsolete: true

    
    

    
  


  

    
    

    
  
Comment on attachment 8808928 [details]
Bug 1309123 - Show persistent-storage permission request notification,

@Gijs,

This patch handles the persistent-storage permission request. However the Web API implementation of navigator.storage.persist is not yet landed (bug 1286717). I am not sure if it's good or not to review so request feedback first.

Some items to notice:
- To try the API, after applying the API patch [1], invoking `navigator.storage.persist()` would trigger permission request. Please see the pics [2][3] for my local try.

- The prefs controlling the storage API features are "browser.storageManager.enabled" for the front-end side and "dom.storageManager.enabled" for the DOM side. Please make sure the both are TRUE while trying.

[1] attachment 8808932 [details] [diff] [review]: 0001-WIP-navigator.storage.persist-API.patch
[2] attachment 8808930 [details]: persistent-storage-permission-notification.png
[3] attachment 8808936 [details]: persistent-storage-permission-page-info.png

        Attachment #8808928 -
        Flags: feedback?(gijskruitbosch+bugs)

    
    

    
  
Comment on attachment 8808928 [details]
Bug 1309123 - Show persistent-storage permission request notification,

https://reviewboard.mozilla.org/r/91652/#review91550

I think Florian might be a good final reviewer for this, but this looks mostly OK to me besides the stuff below.

::: browser/components/nsBrowserGlue.js:2498
(Diff revision 1)
> +        let Prefs = Cu.import("resource://gre/modules/Preferences.jsm", {}).Preferences;
> +        if (Prefs.get("browser.storageManager.enabled", false)) {

You don't need to have the backup value because the pref is in all.js, so I think you can just use Services.prefs.getBoolPref("browser.storageManager.enabled") in the if statement, which also means you don't need to separately import Preferences.jsm.

::: browser/locales/en-US/chrome/browser/browser.dtd:217
(Diff revision 1)
>  <!ENTITY urlbar.addonsNotificationAnchor.tooltip          "Open add-on installation message panel">
>  <!ENTITY urlbar.indexedDBNotificationAnchor.tooltip       "Open offline storage message panel">
>  <!ENTITY urlbar.passwordNotificationAnchor.tooltip        "Open save password message panel">
>  <!ENTITY urlbar.pluginsNotificationAnchor.tooltip         "Manage plug-in use">
>  <!ENTITY urlbar.webNotificationAnchor.tooltip             "Change whether you can receive notifications from the site">
> +<!ENTITY urlbar.persistentStorageNotificationAnchor.tooltip     "Save persistent data on system">

Is this the string UX came up with?

::: browser/locales/en-US/chrome/browser/browser.dtd:295
(Diff revision 1)
>  <!ENTITY newNavigatorCmd.accesskey      "N">
>  <!ENTITY newPrivateWindow.label     "New Private Window">
>  <!ENTITY newPrivateWindow.accesskey "W">
>  <!ENTITY newNonRemoteWindow.label   "New Non-e10s Window">
>  
> -<!ENTITY editMenu.label         "Edit"> 
> +<!ENTITY editMenu.label         "Edit">

Your editor made this patch really big, please revert.

::: browser/modules/PermissionUI.jsm:638
(Diff revision 1)
> +    actions.push({
> +      label: gBrowserBundle.GetStringFromName("persistentStorage.alwaysAllowStoring"),
> +      accessKey:
> +        gBrowserBundle.GetStringFromName("persistentStorage.alwaysAllowStoring.accesskey"),
> +      action: Ci.nsIPermissionManager.ALLOW_ACTION,
> +      expireType: null

Why do you need to specify this?

::: browser/modules/SitePermissions.jsm:273
(Diff revision 1)
> +var Prefs = Components.utils.import("resource://gre/modules/Preferences.jsm", {}).Preferences;
> +if (Prefs.get("browser.storageManager.enabled", false) === false) {
> +  delete gPermissionObject["persistent-storage"];
> +}

Again, seems more appropriate to use Services.prefs. Do you really need to delete this entry if the pref is flipped, though?

::: browser/themes/shared/notification-icons.svg:40
(Diff revision 1)
>      }
>    </style>
>  
>    <defs>
> +
> +    <g id="persistent-storage-icon">

Please maintain alphabetical order here.

::: browser/themes/shared/notification-icons.svg:67
(Diff revision 1)
>        <path d="m 0,0 0,31 31,-31 z m 6,32 26,0 0,-26 z"/>
>      </clipPath>
>    </defs>
>  
> +
> +  <use id="persistent-storage" xlink:href="#persistent-storage-icon" />

These are alphabetical right now, please stick to that order.

    
  

        Attachment #8808928 -
        Flags: feedback?(gijskruitbosch+bugs)

    
    

    
  
(In reply to :Gijs Kruitbosch from comment #8)
> Comment on attachment 8808928 [details]
> Bug 1309123 - Show persist-storage permission request notification
> 
> https://reviewboard.mozilla.org/r/91652/#review91550
> 
> I think Florian might be a good final reviewer for this, but this looks
> mostly OK to me besides the stuff below.
Thanks for the feedbacks. I will modify accordingly and send the review request to Florian when ready.

> 
> ::: browser/components/nsBrowserGlue.js:2498
> (Diff revision 1)
> > +        let Prefs = Cu.import("resource://gre/modules/Preferences.jsm", {}).Preferences;
> > +        if (Prefs.get("browser.storageManager.enabled", false)) {
> 
> You don't need to have the backup value because the pref is in all.js, so I
> think you can just use
> Services.prefs.getBoolPref("browser.storageManager.enabled") in the if
> statement, which also means you don't need to separately import
> Preferences.jsm.
> 
> ::: browser/locales/en-US/chrome/browser/browser.dtd:217
> (Diff revision 1)
> >  <!ENTITY urlbar.addonsNotificationAnchor.tooltip          "Open add-on installation message panel">
> >  <!ENTITY urlbar.indexedDBNotificationAnchor.tooltip       "Open offline storage message panel">
> >  <!ENTITY urlbar.passwordNotificationAnchor.tooltip        "Open save password message panel">
> >  <!ENTITY urlbar.pluginsNotificationAnchor.tooltip         "Manage plug-in use">
> >  <!ENTITY urlbar.webNotificationAnchor.tooltip             "Change whether you can receive notifications from the site">
> > +<!ENTITY urlbar.persistentStorageNotificationAnchor.tooltip     "Save persistent data on system">
> 
> Is this the string UX came up with?
> 
> ::: browser/locales/en-US/chrome/browser/browser.dtd:295
> (Diff revision 1)
> >  <!ENTITY newNavigatorCmd.accesskey      "N">
> >  <!ENTITY newPrivateWindow.label     "New Private Window">
> >  <!ENTITY newPrivateWindow.accesskey "W">
> >  <!ENTITY newNonRemoteWindow.label   "New Non-e10s Window">
> >  
> > -<!ENTITY editMenu.label         "Edit"> 
> > +<!ENTITY editMenu.label         "Edit">
> 
> Your editor made this patch really big, please revert.
> 
> ::: browser/modules/PermissionUI.jsm:638
> (Diff revision 1)
> > +    actions.push({
> > +      label: gBrowserBundle.GetStringFromName("persistentStorage.alwaysAllowStoring"),
> > +      accessKey:
> > +        gBrowserBundle.GetStringFromName("persistentStorage.alwaysAllowStoring.accesskey"),
> > +      action: Ci.nsIPermissionManager.ALLOW_ACTION,
> > +      expireType: null
> 
> Why do you need to specify this?
> 
> ::: browser/modules/SitePermissions.jsm:273
> (Diff revision 1)
> > +var Prefs = Components.utils.import("resource://gre/modules/Preferences.jsm", {}).Preferences;
> > +if (Prefs.get("browser.storageManager.enabled", false) === false) {
> > +  delete gPermissionObject["persistent-storage"];
> > +}
> 
> Again, seems more appropriate to use Services.prefs. Do you really need to
> delete this entry if the pref is flipped, though?
> 
> ::: browser/themes/shared/notification-icons.svg:40
> (Diff revision 1)
> >      }
> >    </style>
> >  
> >    <defs>
> > +
> > +    <g id="persistent-storage-icon">
> 
> Please maintain alphabetical order here.
> 
> ::: browser/themes/shared/notification-icons.svg:67
> (Diff revision 1)
> >        <path d="m 0,0 0,31 31,-31 z m 6,32 26,0 0,-26 z"/>
> >      </clipPath>
> >    </defs>
> >  
> > +
> > +  <use id="persistent-storage" xlink:href="#persistent-storage-icon" />
> 
> These are alphabetical right now, please stick to that order.

    
    

    
  
FYI, this Chrome Dev Summit talk mentioned the heuristic they use to avoid prompting for persist() - see https://www.youtube.com/watch?v=gq80Y5-ZJdg&t=17m36s .

    
  
Depends on: 1270038
Summary: Show persist-storage permission request notification → Show persistent-storage permission request notification

    
    

    
  
Florian,

We are implementing the new persistent-storage living standard [1].
Websites can call `navigator.storage.persist()` web API to ask for permission.

This bugs
  - handles the persistent-storage permisson request prompt
  - add the persistent-storage permisson into Page info's Permission section

Although the web API implementation is under review in Bug 1286717, the permission type has been defined and landed as "persistent-storage" in Bug 1270038.
Since the permission type is fixed, are you fine to review this bug in parallel or prefer after Bug 1286717 is fixed?
If yes, I will send you a review request, thanks.

[1] https://storage.spec.whatwg.org/
Flags: needinfo?(florian)

    
    

    
  
(In reply to Fischer [:Fischer] from comment #11)

> Since the permission type is fixed, are you fine to review this bug in
> parallel or prefer after Bug 1286717 is fixed?
> If yes, I will send you a review request, thanks.

Yes, feel free to request review.

It looks like the last activity in bug 1286717 was a month or so ago, do you expect it to start moving again soon?
Flags: needinfo?(florian)

    
    

    
  
(In reply to Florian Quèze [:florian] [:flo] from comment #12)
> Yes, feel free to request review.
> 
> It looks like the last activity in bug 1286717 was a month or so ago, do you
> expect it to start moving again soon?
Thanks, as far as I know the DOM team just had one meeting to discuss about the schedule.
Will send out this review request soon.

    
    

    
      
  

    
    

    
  


  

        Attachment #8808930 -
        Attachment is obsolete: true

        Attachment #8808932 -
        Attachment is obsolete: true

        Attachment #8808936 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8808928 [details]
Bug 1309123 - Show persistent-storage permission request notification,

Florian,

This bug 
 - handles the persistent-storage permission request
 - adds the persistent-storage permission into Page info window.

Please see [1] for the UI in action.

[1] attachment 8825725 [details]: persistent-storage-permission-prompts.png

Thanks.

        Attachment #8808928 -
        Flags: review?(florian)

    
    

    
      
  

    
    

    
  
Comment on attachment 8808928 [details]
Bug 1309123 - Show persistent-storage permission request notification,

Hi Florian,

Just update the patch slightly to add one "learn more" link so no extra review is required for a single link.

Thanks

        Attachment #8808928 -
        Flags: review?(florian)

    
    

    
  
Comment on attachment 8808928 [details]
Bug 1309123 - Show persistent-storage permission request notification,

https://reviewboard.mozilla.org/r/91652/#review107832

Sorry for the long delay here :-(.

Have you already thought about which tests are needed to cover this feature?

::: browser/locales/en-US/chrome/browser/browser.properties:386
(Diff revision 3)
> +# Persistent storage UI
> +persistentStorage.allow=Allow
> +persistentStorage.allow.accesskey=A
> +persistentStorage.dontAllow=Don’t Allow
> +persistentStorage.dontAllow.accesskey=n
> +persistentStorage.allowWithSite=Will you allow %S to add files to priority storage?

what's "priority storage"?

::: browser/modules/PermissionUI.jsm:623
(Diff revision 3)
> +    return "persistent-storage";
> +  },
> +
> +  get popupOptions() {
> +    let checkbox = {
> +      show: true,

Should the 'remember this decision' checkbox be hidden for private browsing? Or is the whole feature disabled in private browsing cases?

::: browser/modules/PermissionUI.jsm:627
(Diff revision 3)
> +    let checkbox = {
> +      show: true,
> +      checked: true,
> +      label: gBrowserBundle.GetStringFromName("persistentStorage.remember")
> +    };
> +    let learnMoreURL = Services.urlFormatter.formatURLPref("app.support.baseURL") + "storage-permissions"

nit: wrap this long line and add the missing ';'.

::: browser/modules/PermissionUI.jsm:627
(Diff revision 3)
> +    let checkbox = {
> +      show: true,
> +      checked: true,
> +      label: gBrowserBundle.GetStringFromName("persistentStorage.remember")
> +    };
> +    let learnMoreURL = Services.urlFormatter.formatURLPref("app.support.baseURL") + "storage-permissions"

I don't see the 'Learn more…' link on attachment 8825725 [details]. Is this something you added after doing the screenshots?

::: browser/modules/PermissionUI.jsm:644
(Diff revision 3)
> +    return "persistent-storage-notification-icon";
> +  },
> +
> +  get message() {
> +    return gBrowserBundle.formatStringFromName(
> +      "persistentStorage.allowWithSite", [this.request.principal.URI.host], 1);

http://searchfox.org/mozilla-central/rev/02a56df6474a97cf84d94bbcfaa126979970905d/browser/modules/PermissionUI.jsm#457 accesses this.principal.URI.hostPort from a try block and falls back to <> if there's an exception.
- Are you sure your code won't throw? Have you tried with about: or data: urls?
- is there a reason for using .host when other similar code in the same file uses .hostPort?
- you can simplify "this.request.principal" to "this.principal" as it's defined in the prototype at http://searchfox.org/mozilla-central/rev/02a56df6474a97cf84d94bbcfaa126979970905d/browser/modules/PermissionUI.jsm#387

::: browser/modules/PermissionUI.jsm:649
(Diff revision 3)
> +      "persistentStorage.allowWithSite", [this.request.principal.URI.host], 1);
> +  },
> +
> +  get promptActions() {
> +
> +    let actions = [];

It seems you don't need this 'actions' variable and can just do:

return [
  {
    ...
  },
  {
    ...
  }
];

::: browser/modules/test/xpcshell/test_SitePermissions.js:12
(Diff revision 3)
>  Components.utils.import("resource://gre/modules/Services.jsm");
>  
>  add_task(function* testPermissionsListing() {
>    Assert.deepEqual(SitePermissions.listPermissions().sort(),
>      ["camera", "cookie", "desktop-notification", "geo", "image",
> -     "indexedDB", "install", "microphone", "popup", "screen"],
> +     "indexedDB", "install", "microphone", "persistent-storage", "popup", "screen"],

nit: wrap after '"popup",' so that the line length stays < 80 characters.

::: browser/themes/shared/notification-icons.svg:63
(Diff revision 3)
>      <path id="indexedDB-icon" d="m 2,24 a 4,4 0 0 0 4,4 l 2,0 0,-4 -2,0 0,-16 20,0 0,16 -2,0 0,4 2,0 a 4,4 0 0 0 4,-4 l 0,-16 a 4,4 0 0 0 -4,-4 l -20,0 a 4,4 0 0 0 -4,4 z m 8,-2 6,7 6,-7 -4,0 0,-8 -4,0 0,8 z" />
>      <path id="login-icon" d="m 2,26 0,4 6,0 0,-2 2,0 0,-2 1,0 0,-1 2,0 0,-3 2,0 2.5,-2.5 1.5,1.5 3,-3 a 8,8 0 1 0 -8,-8 l -3,3 2,2 z m 20,-18.1 a 2,2 0 1 1 0,0.2 z" />
>      <path id="login-detailed-icon" d="m 1,27 0,3.5 a 0.5,0.5 0 0 0 0.5,0.5 l 5,0 a 0.5,0.5 0 0 0 0.5,-0.5 l 0,-1.5 1.5,0 a 0.5,0.5 0 0 0 0.5,-0.5 l 0,-1.5 1,0 a 0.5,0.5 0 0 0 0.5,-0.5 l 0,-1 1,0 a 0.5,0.5 0 0 0 0.5,-0.5 l 0,-2 2,0 2.5,-2.5 q 0.5,-0.5 1,0 l 1,1 c 0.5,0.5 1,0.5 1.5,-0.5 l 1,-2 a 9,9 0 1 0 -8,-8 l -2,1 c -1,0.5 -1,1 -0.5,1.5 l 1.5,1.5 q 0.5,0.5 0,1 z m 21,-19.1 a 2,2 0 1 1 0,0.2 z" />
>      <path id="microphone-icon" d="m 8,14 0,4 a 8,8 0 0 0 6,7.7 l 0,2.3 -2,0 a 2,2 0 0 0 -2,2 l 12,0 a 2,2 0 0 0 -2,-2 l -2,0 0,-2.3 a 8,8 0 0 0 6,-7.7 l 0,-4 -2,0 0,4 a 6,6 0 0 1 -12,0 l 0,-4 z m 4,4 a 4,4 0 0 0 8,0 l 0,-12 a 4,4 0 0 0 -8,0 z" />
>      <path id="microphone-detailed-icon" d="m 8,18 a 8,8 0 0 0 6,7.7 l 0,2.3 -1,0 a 3,2 0 0 0 -3,2 l 12,0 a 3,2 0 0 0 -3,-2 l -1,0 0,-2.3 a 8,8 0 0 0 6,-7.7 l 0,-4 a 1,1 0 0 0 -2,0 l 0,4 a 6,6 0 0 1 -12,0 l 0,-4 a 1,1 0 0 0 -2,0 z m 4,0 a 4,4 0 0 0 8,0 l 0,-12 a 4,4 0 0 0 -8,0 z" />
> +    <g id="persistent-storage-icon">

- Can this all be merged into a single <path> element?
- Is this icon coming from UX? What does it represent?
- The style used inside the 'd=' attributes doesn't seem to match the style used in the rest of the file.

        Attachment #8808928 -
        Flags: review?(florian) → review-

    
  
URL: 1312377

    
  
URL: 1312377

    
    

    
  

      
      
      
      

        Attached image
          
        prompts-with-learnMore.png (obsolete)
        — 
      

    


  

    
    

    
  


  

        Attachment #8825725 -
        Attachment is obsolete: true

        Attachment #8830599 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8808928 [details]
Bug 1309123 - Show persistent-storage permission request notification,

https://reviewboard.mozilla.org/r/91652/#review107832

Thanks for the feedbacks.

The new persistent-storage permission type has been added into the test [1].
And the tests in [2] are testing popup notification functionalities already.
However, please let me know if I missed something.

p.s One try has been run beofre and that xpcshell passed [3]. But I will run test again before commit because of patch updating and rebase.

[1] browser/modules/test/xpcshell/test_SitePermissions.js
[2] browser/base/content/test/popupNotifications
[3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec45f1e45577f8564efa94fa36c97c86ce342ec8

> what's "priority storage"?

This "priority storage" means the type of storage Firefox cannot remove at will even the space is not enough, that is, persistent storage. Only user or website possessing that storage can command to remove.

The strings are currently generated by UX and copy writer. "Priority" is for user to see in case that "persistent" might be a bit too technical for user. Please just let us know if found any potential improvement.

> Should the 'remember this decision' checkbox be hidden for private browsing? Or is the whole feature disabled in private browsing cases?

Thanks for reminding this. Will hide that checkbox in PB mode.

> nit: wrap this long line and add the missing ';'.

OK, will update

> I don't see the 'Learn more…' link on attachment 8825725 [details]. Is this something you added after doing the screenshots?

Yes, now the screenshots got updated.
Please see [1] with the "learn more" link and new compressed svg icon.

[1] attachment 8830626 [details]: persistent-storage-permission-prompts-v2.png

> http://searchfox.org/mozilla-central/rev/02a56df6474a97cf84d94bbcfaa126979970905d/browser/modules/PermissionUI.jsm#457 accesses this.principal.URI.hostPort from a try block and falls back to <> if there's an exception.
> - Are you sure your code won't throw? Have you tried with about: or data: urls?
> - is there a reason for using .host when other similar code in the same file uses .hostPort?
> - you can simplify "this.request.principal" to "this.principal" as it's defined in the prototype at http://searchfox.org/mozilla-central/rev/02a56df6474a97cf84d94bbcfaa126979970905d/browser/modules/PermissionUI.jsm#387

- For non standard URL (not instance of nsIStandardURL), about: or data:, it will check and filter out [1]. So nothing happens with about:prefernces or data:text/html,1234.

- No special reason for .host. Will go for .hostPort like others. Better to show port since different port stands for different site. Thanks for reminding this.

- Thanks for reminding, will go for "this.principal" instead of "this.request.principal".

[1] http://searchfox.org/mozilla-central/rev/02a56df6474a97cf84d94bbcfaa126979970905d/browser/modules/PermissionUI.jsm#261

> It seems you don't need this 'actions' variable and can just do:
> 
> return [
>   {
>     ...
>   },
>   {
>     ...
>   }
> ];

OK, will update

> nit: wrap after '"popup",' so that the line length stays < 80 characters.

OK, will update

> - Can this all be merged into a single <path> element?
> - Is this icon coming from UX? What does it represent?
> - The style used inside the 'd=' attributes doesn't seem to match the style used in the rest of the file.

Yes, the svg icon is from the visual designer in UX team (bug 1313313).
The icon represents disk.
About the svg format, just found that can compress the path into one single <path> element. Will update.
However, about the style inside the "d=", maybe the difference is a result from application outputing SVG or compressing tool. The designer uses Adobe AI to make our SVG icon.

    
    

    
      
  

    
  

        Attachment #8808928 -
        Flags: review?(florian)

    
    

    
  
Thanks for updating the patch. It looks good to me technically. I'm not r+'ing yet as I would like a few of the following points to be addressed or discussed first.


(In reply to Fischer [:Fischer] from comment #24)

> The new persistent-storage permission type has been added into the test [1].
> And the tests in [2] are testing popup notification functionalities already.
> However, please let me know if I missed something.

I would like us to have a test loading a web page using the feature, checking that the prompt appears, and that answering the prompt produces the expected result on the page.


> > what's "priority storage"?
> 
> This "priority storage" means the type of storage Firefox cannot remove at
> will even the space is not enough, that is, persistent storage. Only user or
> website possessing that storage can command to remove.
> 
> The strings are currently generated by UX and copy writer. "Priority" is for
> user to see in case that "persistent" might be a bit too technical for user.
> Please just let us know if found any potential improvement.

For me 'priority' is more an idea of speed than of persistence.

Michelle, do you have suggestions? The string currently in the patch is "Will you allow %S to add files to priority storage?", attachment 8830626 [details] shows the string in the UI.


> > - Is this icon coming from UX? What does it represent?
> 
> Yes, the svg icon is from the visual designer in UX team (bug 1313313).
> The icon represents disk.

Ok, I would not have guessed (I saw a touchpad initially). And it's really difficult to see anything on that icon when it's displayed at the small size. Needinfo on Philipp to have another pair of UX eyes on this.
Flags: needinfo?(philipp)
Flags: needinfo?(mheubusch)

    
    

    
  
The large icon looks good to me, but I agree that the small version (especially on low-dpi screens) could be optimized. Especially when the strikethrough is added, it becomes very hard to decipher.
Helen, do you have an optimized version for the smaller size?

FWIW, I also stumbled over the phrase »priority storage« and it created more questions for me than it answered. But I'm leaving this one up to Michelle :)
Flags: needinfo?(philipp) → needinfo?(hhuang)

    
    

    
  
Drive-by: Please add an entry for this permission prompt to the PermissionPrompts mozscreenshots configuration (http://searchfox.org/mozilla-central/source/browser/tools/mozscreenshots/mozscreenshots/extension/configurations/PermissionPrompts.jsm)

Thank you!

    
    

    
  
For the icon and text, I think it's useful to compare and contrast this permission with IndexedDB, which is the other permission that we already have related to storing offline data.

The current wording for the existing storage permission is "Will you allow %S to store data on your computer?", with the button saying "Allow Storing Data".

The current icon for the storage permission is a small down arrow inside a rounded rectangle.

If you want to try this out, this is the page I'm currently using to test it:

http://clb.demon.fi/dump/html5_popups/?persistent

    
    

    
  
(In reply to Johann Hofmann [:johannh] from comment #28)
> Drive-by: Please add an entry for this permission prompt to the
> PermissionPrompts mozscreenshots configuration
> (http://searchfox.org/mozilla-central/source/browser/tools/mozscreenshots/
> mozscreenshots/extension/configurations/PermissionPrompts.jsm)
> 
> Thank you!
Thanks for reminding this. Will update.

    
    

    
  
(In reply to Florian Quèze [:florian] [:flo] from comment #26)
> > > what's "priority storage"?
> > 
> > This "priority storage" means the type of storage Firefox cannot remove at
> > will even the space is not enough, that is, persistent storage. Only user or
> > website possessing that storage can command to remove.
> > 
> > The strings are currently generated by UX and copy writer. "Priority" is for
> > user to see in case that "persistent" might be a bit too technical for user.
> > Please just let us know if found any potential improvement.
> 
> For me 'priority' is more an idea of speed than of persistence.
> 
> Michelle, do you have suggestions? The string currently in the patch is
> "Will you allow %S to add files to priority storage?", attachment 8830626 [details]
> [details] shows the string in the UI.
> 
Looping UX: Mark to look into this.
-------------------------------------
Mark,
Could you please look at this while reviewing the strings. Thanks
Flags: needinfo?(mliang)

    
    

    
  

      
      
      
      

        Attached image
          
        notification-icons.svg (obsolete)
        — 
      

    


  
Sorry for the late reply. I tried to modify the current icon, but it might still be too complicated to show on low-dpi screens. Here I made another proposal which uses the metaphor of pie chart to persistent storage. The SVG file is updated, please let me know if any questions.
Flags: needinfo?(hhuang)

    
    

    
  
(In reply to Helen Huang [:HHuang] (PTO 11/1-4) from comment #32)
> Created attachment 8833865 [details]
> notification-icons.svg
> 
> Sorry for the late reply. I tried to modify the current icon, but it might
> still be too complicated to show on low-dpi screens. Here I made another
> proposal which uses the metaphor of pie chart to persistent storage. The SVG
> file is updated, please let me know if any questions.
Thanks Helen. Will try it.

    
    

    
  
Florian, Philipp,

Please see the new icon in action [1].
Helen uses a pie chart as a hint to persistent storage usage, Comment 32.
How do you think?

[1] attachment 8833880 [details]: persistent-storage-permission-prompts-v3.png
Flags: needinfo?(philipp)
Flags: needinfo?(florian)

    
    

    
      
  


  

    
    

    
      
  

    
    

    
  
Comment on attachment 8833898 [details]
Bug 1309123 - Add Persistent storage permission to PermissionPrompts mozscreenshots

Johann,

While adding persistent storage permission into PermissionPrompts.jsm, I noticed that
- for `function* closeLastTab(selector)`, the selector arg is not being used.
- for `function* clickOn(selector)`, some places are calling it with extra URL arg.

Saw you were editing this file. Do you know why doing like above or, I guess, we should correct them?

Thanks.

        Attachment #8833898 -
        Flags: feedback?(jhofmann)

    
    

    
  
Comment on attachment 8833898 [details]
Bug 1309123 - Add Persistent storage permission to PermissionPrompts mozscreenshots

This seems correct to me. Please request review once this is actually ready to land (I assume that is once bug 1286717 lands). You can also spin off a new bug with this patch if you want to land the main patch in this bug earlier than bug 1286717.

> Saw you were editing this file. Do you know why doing like above or, I guess, we should correct them?

This looks very much like a copy-paste error or some leftover WIP state. Thanks for correcting!

        Attachment #8833898 -
        Flags: feedback?(jhofmann) → feedback+

    
    

    
  
(In reply to Florian Quèze [:florian] [:flo] from comment #26)
> I would like us to have a test loading a web page using the feature,
> checking that the prompt appears, and that answering the prompt produces the
> expected result on the page.
> 
Hi Florian,

The bug 1286717 of Web API implementation is being requested to test on a real prompt as well[1].
So our bug here and the bug 1286717 depend on each other.

I am thinking that let's add a test page using this new feature.
After your review, I won't check-in here but set our bug here as a duplicate of the bug 1286717.
Then pass our patch to the bug 1286717 to have it checked in with it together.

How do you think?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1286717#c72

Thanks

    
    

    
  
Hello - tl;dr - i used "priority" for reasons, but imagine that there are good reasons for "persistent", too.  the important thing, without conducting research, is that we pick one term that isn't used to mean anything else in the product and use it consistently. i still think priority is a good choice and if we learn from users later, we can change it.

Full Story:

weighing in on "priority" vs "persistent".  I used the word "priority" because it conveys that we handle it differently, that it kind of moves to the front of the line, and I don't think that most users would see the word as connoting speed. That said, I don't know how much our users understand about how storage works and didn't conduct any research prior to choosing priority. I opted for priority over persistent, because I think it signals that it is a way for a user to designate some sites' data as more important that other sites' data, and also that the storage isn't permanent, it only lasts as long as there is room and the user doesn't opt to clear it out.  

In a perfect world, we'd have time and resources to test this, or we'd have more room to explain this concept, or we could trust users would follow the Learn more link, or all of these things would happen.  Or, there would be some other word in English and every other language that was the exact word we needed.
Flags: needinfo?(mheubusch)

    
    

    
  
(In reply to mheubusch from comment #41)

> I opted for priority over persistent, because I think it signals
> that it is a way for a user to designate some sites' data as more important
> that other sites' data, and also that the storage isn't permanent, it only
> lasts as long as there is room

My understanding of the feature is the opposite: the data will never be cleared without the user explicitly requesting it (and this is the reason why we need a permission prompt).

    
    

    
  
(In reply to Fischer [:Fischer] from comment #35)
> Florian, Philipp,
> 
> Please see the new icon in action [1].
> Helen uses a pie chart as a hint to persistent storage usage, Comment 32.
> How do you think?

I think it's really Philipp's input that you need here rather than mine. One thing I mentioned today in a meeting is that when googling 'storage icon', most results are an icon showing three disks, it seems to be a quasi-standard. See http://www.freeiconspng.com/icons/storage-icon for some examples.

But comment 29 is also interesting here; we already have a prompt requesting storage permission, so maybe we should use the same icon (or maybe even the same permission for both?).

(In reply to Fischer [:Fischer] from comment #40)

> After your review, I won't check-in here but set our bug here as a duplicate
> of the bug 1286717.
> Then pass our patch to the bug 1286717 to have it checked in with it
> together.
> 
> How do you think?

Please don't mark the bug as duplicate; just add a comment saying that both bugs need to land at once when you'll add the checkin-needed keyword.
Flags: needinfo?(florian)

    
    

    
  
(In reply to Florian Quèze [:florian] [:flo] from comment #43)
> But comment 29 is also interesting here; we already have a prompt requesting
> storage permission, so maybe we should use the same icon (or maybe even the
> same permission for both?).
> 
For the permission, not sure if it is fine to take the same permission in that the persistent storage and persistent indexedDB is sort of different from each other.
the persistent storage affects not only indexedDB but also localstorage. And there are different works in the DOM-side as I know. In order not to get confusing or other issue so go for another permission type.

> (In reply to Fischer [:Fischer] from comment #40)
> 
> Please don't mark the bug as duplicate; just add a comment saying that both
> bugs need to land at once when you'll add the checkin-needed keyword.
Thanks, OK.
Just one question: If add the checkin-needed keyword, would it be mergered into autoland automatically? And the test would fail because of lack of Web API. Or it would wait for the bug 1286717 with that comment?

    
    

    
  
(In reply to Fischer [:Fischer] from comment #44)
> (In reply to Florian Quèze [:florian] [:flo] from comment #43)
> > But comment 29 is also interesting here; we already have a prompt requesting
> > storage permission, so maybe we should use the same icon (or maybe even the
> > same permission for both?).
> > 
> For the permission, not sure if it is fine to take the same permission in
> that the persistent storage and persistent indexedDB is sort of different
> from each other.
> the persistent storage affects not only indexedDB but also localstorage. And
> there are different works in the DOM-side as I know. In order not to get
> confusing or other issue so go for another permission type.

I think the question is: can we make the two prompts meaningfully different for users?

> > (In reply to Fischer [:Fischer] from comment #40)
> > 
> > Please don't mark the bug as duplicate; just add a comment saying that both
> > bugs need to land at once when you'll add the checkin-needed keyword.
> Thanks, OK.
> Just one question: If add the checkin-needed keyword, would it be mergered
> into autoland automatically? And the test would fail because of lack of Web
> API. Or it would wait for the bug 1286717 with that comment?

Whoever handles checkin-needed should see it and do the right thing. Add something the whiteboard field too if you are afraid the person handling the checkin may not see a comment.

    
    

    
  
(In reply to Florian Quèze [:florian] [:flo] from comment #45)
> (In reply to Fischer [:Fischer] from comment #44)
> > (In reply to Florian Quèze [:florian] [:flo] from comment #43)
> > > But comment 29 is also interesting here; we already have a prompt requesting
> > > storage permission, so maybe we should use the same icon (or maybe even the
> > > same permission for both?).
> > > 
> > For the permission, not sure if it is fine to take the same permission in
> > that the persistent storage and persistent indexedDB is sort of different
> > from each other.
> > the persistent storage affects not only indexedDB but also localstorage. And
> > there are different works in the DOM-side as I know. In order not to get
> > confusing or other issue so go for another permission type.
> 
> I think the question is: can we make the two prompts meaningfully different
> for users?
The persistent storage permission here is a web standard which tries to standardize APIs for persistent data including "persistent indexedDB data", "persistent localstorage data" etc. Firefox persistent indexedDB permission is non-standard. So in the long run, Firefox persistent indexedDB permission will be deprecated.

    
    

    
  
(In reply to Florian Quèze [:florian] [:flo] from comment #42)
> (In reply to mheubusch from comment #41)
> 
> > I opted for priority over persistent, because I think it signals
> > that it is a way for a user to designate some sites' data as more important
> > that other sites' data, and also that the storage isn't permanent, it only
> > lasts as long as there is room
> 
> My understanding of the feature is the opposite: the data will never be
> cleared without the user explicitly requesting it (and this is the reason
> why we need a permission prompt).

To avoid misunderstanding for both developers and users, I think we can start with "persistent" instead of "priority" and learn from users later to see if we need to change that.
Flags: needinfo?(mliang) → needinfo?(florian)

    
    

    
  


  
(In reply to Fischer [:Fischer] from comment #46)
> (In reply to Florian Quèze [:florian] [:flo] from comment #45)
> > (In reply to Fischer [:Fischer] from comment #44)
> > > (In reply to Florian Quèze [:florian] [:flo] from comment #43)
> > > > But comment 29 is also interesting here; we already have a prompt requesting
> > > > storage permission, so maybe we should use the same icon (or maybe even the
> > > > same permission for both?).
> > > > 
> > > For the permission, not sure if it is fine to take the same permission in
> > > that the persistent storage and persistent indexedDB is sort of different
> > > from each other.
> > > the persistent storage affects not only indexedDB but also localstorage. And
> > > there are different works in the DOM-side as I know. In order not to get
> > > confusing or other issue so go for another permission type.
> > 
> > I think the question is: can we make the two prompts meaningfully different
> > for users?
> The persistent storage permission here is a web standard which tries to
> standardize APIs for persistent data including "persistent indexedDB data",
> "persistent localstorage data" etc. Firefox persistent indexedDB permission
> is non-standard. So in the long run, Firefox persistent indexedDB permission
> will be deprecated.

Do you think this is a difference we can meaningfully explain to users?

If not, should we use the same prompt for both cases?

(I'm attaching a screenshot of the existing prompt so that we can be sure everybody has seen the same piece of UI.)

(In reply to Mark Liang(:mark_liang) from comment #47)
> (In reply to Florian Quèze [:florian] [:flo] from comment #42)
> > (In reply to mheubusch from comment #41)
> > 
> > > I opted for priority over persistent, because I think it signals
> > > that it is a way for a user to designate some sites' data as more important
> > > that other sites' data, and also that the storage isn't permanent, it only
> > > lasts as long as there is room
> > 
> > My understanding of the feature is the opposite: the data will never be
> > cleared without the user explicitly requesting it (and this is the reason
> > why we need a permission prompt).
> 
> To avoid misunderstanding for both developers and users, I think we can
> start with "persistent" instead of "priority" and learn from users later to
> see if we need to change that.

I would prefer that, yes.
Flags: needinfo?(florian)

    
    

    
  
(In reply to Florian Quèze [:florian] [:flo] from comment #48)
> Created attachment 8836658 [details]
> Screenshot of the existing prompt
> 
> (In reply to Fischer [:Fischer] from comment #46)
> > The persistent storage permission here is a web standard which tries to
> > standardize APIs for persistent data including "persistent indexedDB data",
> > "persistent localstorage data" etc. Firefox persistent indexedDB permission
> > is non-standard. So in the long run, Firefox persistent indexedDB permission
> > will be deprecated.
> 
> Do you think this is a difference we can meaningfully explain to users?
> 
> If not, should we use the same prompt for both cases?
> 
> (I'm attaching a screenshot of the existing prompt so that we can be sure
> everybody has seen the same piece of UI.)
> 
Yes, there is a difference there. Just that how to explaining the difference to user may need some more discussion and consideration so that it could be present in a clear yet brief fashion inside the limited prompt UI space.

From another view point, one of the main reason for a separate persistent-storage permission type is because of the web standard. Just found that there is a web standard about Permission which defines the persistent-storage permission type[1]. In the persistent storage standard, there is a code example showing how to query permission through "persistent-storage" [2] like below.
  ```
  navigator.permissions.query({name: "persistent-storage"});
  ```
Plus considering, Firefox proprietary persistent indexedDB permission goes through it own way[3] and that permission as well as the underlying DOM-side implementation of it will be retired in the future. Reusing it may bring some issue later so that is why go for a separate persistent-storage permission type.


[1] https://w3c.github.io/permissions/#permission-registry
[2] https://storage.spec.whatwg.org/#example-3a7051a8
[3] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6180

    
    

    
  
(In reply to Fischer [:Fischer] from comment #49)
> (In reply to Florian Quèze [:florian] [:flo] from comment #48)
> > Created attachment 8836658 [details]
> > Screenshot of the existing prompt
> > 
> > (In reply to Fischer [:Fischer] from comment #46)
> > > The persistent storage permission here is a web standard which tries to
> > > standardize APIs for persistent data including "persistent indexedDB data",
> > > "persistent localstorage data" etc. Firefox persistent indexedDB permission
> > > is non-standard. So in the long run, Firefox persistent indexedDB permission
> > > will be deprecated.
> > 
> > Do you think this is a difference we can meaningfully explain to users?
> > 
> > If not, should we use the same prompt for both cases?
> > 
> > (I'm attaching a screenshot of the existing prompt so that we can be sure
> > everybody has seen the same piece of UI.)
> > 
> Yes, there is a difference there. Just that how to explaining the difference
> to user may need some more discussion and consideration so that it could be
> present in a clear yet brief fashion inside the limited prompt UI space.

So I think we are waiting for UX (Philipp) here. I personally still don't understand how the difference is significant to users.

> From another view point, one of the main reason for a separate
> persistent-storage permission type is because of the web standard. Just
> found that there is a web standard about Permission which defines the
> persistent-storage permission type[1]. In the persistent storage standard,
> there is a code example showing how to query permission through
> "persistent-storage" [2] like below.
>   ```
>   navigator.permissions.query({name: "persistent-storage"});
>   ```
> Plus considering, Firefox proprietary persistent indexedDB permission goes
> through it own way[3] and that permission as well as the underlying DOM-side
> implementation of it will be retired in the future. Reusing it may bring
> some issue later so that is why go for a separate persistent-storage
> permission type.

That the back-end implementation uses 2 different permission types internally seems like an implementation detail that we don't have to surface to users.

> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.
> js#6180

This code seems to be dead code leftover after https://hg.mozilla.org/mozilla-central/rev/8892214038df removed the platform code sending the indexedDB-permissions-prompt notification. I filed bug 1339393 to remove it.

I think you wanted to point at http://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/browser/base/content/browser.js#6090

    
    

    
  
(In reply to Fischer [:Fischer] from comment #35)
> Florian, Philipp,
> 
> Please see the new icon in action [1].
> Helen uses a pie chart as a hint to persistent storage usage, Comment 32.
> How do you think?
> 
> [1] attachment 8833880 [details]:
> persistent-storage-permission-prompts-v3.png

I think the disk icon was more useful, simply because it is the metaphor that Windows and macOS use for storage.
So it would be better to fix the issues that has at small scales, rather than switch to an entirely new metaphor.

@Helen, could you maybe take a look at optimizing the small version of the disk icon with and without the strikethrough, so that it works on high and low dpi devices at that size? Thanks!
Flags: needinfo?(philipp) → needinfo?(hhuang)

    
    

    
  

      
      
      
      

        Attached image
          
        Icon comparison.png
        — 
      

    


  
Hi Phillip, thanks for your feedback. 

To optimize the disk icon, I tried to make the corner less rounded and made the circle inside the triangle bigger, so that it should be more recognizable on low-res screens. Attached the comparison for you to take a look. Let me know if any thoughts.
Flags: needinfo?(hhuang)

    
    

    
  

      
      
      
      

        Attached image
          
        notification-icons.svg
        — 
      

    


  
Updated the SVG file.

        Attachment #8833865 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Helen Huang [:HHuang] from comment #52)
> Created attachment 8843847 [details]
> Icon comparison.png
> 
> Hi Phillip, thanks for your feedback. 
> 
> To optimize the disk icon, I tried to make the corner less rounded and made
> the circle inside the triangle bigger, so that it should be more
> recognizable on low-res screens. Attached the comparison for you to take a
> look. Let me know if any thoughts.
Philipp, how's the updated icon from Helen, thanks.
Flags: needinfo?(philipp)

    
    

    
  
Yeah, I think that works better!
Flags: needinfo?(philipp)

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(In reply to Fischer [:Fischer] from comment #56)
> Comment on attachment 8808928 [details]
> Bug 1309123 - Show persistent-storage permission request notification
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/91652/diff/5-6/
Hi Florian,

Please see the updated patch,
  - use the updated icon as Comment 55
  - use “Persistent” string as Comment 47
  - add one test (browser_popupNotification_persistent_storage.js) as Comment 26

Thank you

    
    

    
  
Comment on attachment 8808928 [details]
Bug 1309123 - Show persistent-storage permission request notification,

https://reviewboard.mozilla.org/r/91652/#review123806

::: browser/locales/en-US/chrome/browser/browser.dtd:217
(Diff revisions 5 - 6)
>  <!ENTITY urlbar.addonsNotificationAnchor.tooltip          "Open add-on installation message panel">
>  <!ENTITY urlbar.indexedDBNotificationAnchor.tooltip       "Open offline storage message panel">
>  <!ENTITY urlbar.passwordNotificationAnchor.tooltip        "Open save password message panel">
>  <!ENTITY urlbar.pluginsNotificationAnchor.tooltip         "Manage plug-in use">
>  <!ENTITY urlbar.webNotificationAnchor.tooltip             "Change whether you can receive notifications from the site">
> -<!ENTITY urlbar.persistentStorageNotificationAnchor.tooltip     "Store data on computer">
> +<!ENTITY urlbar.persistentStorageNotificationAnchor.tooltip     "Store data in Persistent Storage">

Is it "persistent storage" or "Persistent Storage"? According to the other tooltip, it should be lowercase.

    
    

    
  
(In reply to Francesco Lodolo [:flod] from comment #59)
> Comment on attachment 8808928 [details]
> Bug 1309123 - Show persistent-storage permission request notification
> 
> https://reviewboard.mozilla.org/r/91652/#review123806
> 
> ::: browser/locales/en-US/chrome/browser/browser.dtd:217
> (Diff revisions 5 - 6)
> > -<!ENTITY urlbar.persistentStorageNotificationAnchor.tooltip     "Store data on computer">
> > +<!ENTITY urlbar.persistentStorageNotificationAnchor.tooltip     "Store data in Persistent Storage">
> 
> Is it "persistent storage" or "Persistent Storage"? According to the other
> tooltip, it should be lowercase.
It's "Persistent Storage". Here "Persistent Storage" is emphasized [1].
[1] https://mozilla.invisionapp.com/share/4Y87EJO39#/screens/179635748

    
    

    
  
Comment on attachment 8808928 [details]
Bug 1309123 - Show persistent-storage permission request notification,

https://reviewboard.mozilla.org/r/91652/#review123928

(not a full review, just sending quickly the comments I had during a first quick look at the new patch)

::: browser/base/content/test/popupNotifications/browser_popupNotification_persistent_storage.js:4
(Diff revision 6)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> + 

trailing whitespace

::: browser/base/content/test/popupNotifications/browser_popupNotification_persistent_storage.js:9
(Diff revision 6)
> + 
> +const TEST_ORIGIN = "http://example.com";
> +const TEST_URL = TEST_ORIGIN +
> +  "/browser/browser/base/content/test/popupNotifications/persistent_storage_permission_test.html";
> +
> +const { NetUtil } = Cu.import("resource://gre/modules/NetUtil.jsm", {});

I don't think we need NetUtil here, Services.io.newURI(origin) should work.

::: browser/base/content/test/popupNotifications/browser_popupNotification_persistent_storage.js:36
(Diff revision 6)
> +
> +var tests = [
> +  { id: "Test#Allow_Persistent_Storage_Permission",
> +    *run() {
> +      yield BrowserTestUtils.loadURI(gBrowser.selectedBrowser, TEST_URL);
> +      yield BrowserTestUtils.waitForLocationChange(gBrowser, TEST_URL);

Why do we need to wait for the location change here? The rest of the test won't run unless the popup notification is shown, so I don't think we need to wait here.

::: browser/base/content/test/popupNotifications/browser_popupNotification_persistent_storage.js:50
(Diff revision 6)
> +    }
> +  },
> +  { id: "Test#Block_Persistent_Storage_Permission",
> +    *run() {
> +      yield BrowserTestUtils.loadURI(gBrowser.selectedBrowser, TEST_URL);
> +      yield BrowserTestUtils.waitForLocationChange(gBrowser, TEST_URL);

same here

::: browser/base/content/test/popupNotifications/browser_popupNotification_persistent_storage.js:53
(Diff revision 6)
> +    *run() {
> +      yield BrowserTestUtils.loadURI(gBrowser.selectedBrowser, TEST_URL);
> +      yield BrowserTestUtils.waitForLocationChange(gBrowser, TEST_URL);
> +    },
> +    onShown(popup) {
> +      triggerSecondaryCommand(popup, 0);

what does 0 mean here? Could you make it a const with an explicit name?

::: browser/locales/en-US/chrome/browser/browser.properties:386
(Diff revision 6)
> +# Persistent storage UI
> +persistentStorage.allow=Allow
> +persistentStorage.allow.accesskey=A
> +persistentStorage.dontAllow=Don’t Allow
> +persistentStorage.dontAllow.accesskey=n
> +persistentStorage.allowWithSite=Will you allow %S to add files to persistent storage?

all the other strings we use in this patch talk about "data" rather than "files", should this say "Will you allow %S to store data persistently?"

::: browser/locales/en-US/chrome/browser/sitePermissions.properties:21
(Diff revision 6)
>  permission.screen.label = Share the Screen
>  permission.install.label = Install Add-ons
>  permission.popup.label = Open Pop-up Windows
>  permission.geo.label = Access Your Location
>  permission.indexedDB.label = Maintain Offline Storage
> +permission.persistent-storage.label = Store Persistent Storage

I don't understand this string. Was it meant to be "Store Persistent Data"?

        Attachment #8808928 -
        Flags: review?(florian) → review-

    
    

    
  
Comment on attachment 8833898 [details]
Bug 1309123 - Add Persistent storage permission to PermissionPrompts mozscreenshots

https://reviewboard.mozilla.org/r/110024/#review123918

I'm unable to test this locally. I strongly suspect that would require me to get the changes from bug 1286717, but since this patch should ideally land very close to bug 1286717 anyway (to not get errors during the screenshot process) I'll clear review for now, since the patch shouldn't be landed right now. FWIW the changes look good to me :)

        Attachment #8833898 -
        Flags: review?(jhofmann)

    
    

    
  
Comment on attachment 8808928 [details]
Bug 1309123 - Show persistent-storage permission request notification,

https://reviewboard.mozilla.org/r/91652/#review123996

::: browser/base/content/test/popupNotifications/browser.ini:24
(Diff revision 6)
>  [browser_popupNotification_keyboard.js]
>  skip-if = (os == "linux" && (debug || asan))
>  [browser_popupNotification_no_anchors.js]
>  skip-if = (os == "linux" && (debug || asan))
> +[browser_popupNotification_persistent_storage.js]
> +support-files=persistent_storage_permission_test.html

Drive-by:

Please don't add this kind of test here. This directory is for testing general popupNotification UI behavior and not if a permission prompt is correctly setting its permissions.

You're looking for http://searchfox.org/mozilla-central/source/browser/modules/test/browser/browser_PermissionUI_prompts.js

You probably just have to add a couple of lines that copy what we do for Geolocation and Notifications already. That test also checks for persistent/temporary permissions.

Thanks!

    
    

    
  
(In reply to Johann Hofmann [:johannh] from comment #62)
> Comment on attachment 8833898 [details]
> Bug 1309123 - Add Persistent storage permission to PermissionPrompts
> mozscreenshots
> 
> https://reviewboard.mozilla.org/r/110024/#review123918
> 
> I'm unable to test this locally. I strongly suspect that would require me to
> get the changes from bug 1286717, but since this patch should ideally land
> very close to bug 1286717 anyway (to not get errors during the screenshot
> process) I'll clear review for now, since the patch shouldn't be landed
> right now. FWIW the changes look good to me :)
Yes, it needs `navigator.storage.persist` web api to run.
Uploaded the bug 1286717 under-reviewing patch[1].
Please apply the patch on the latest m-c codebase if you would like to test it.
[1] attachment 8849469 [details] [diff] [review]: bug-1286717-wip-patch.patch

> Drive-by:
> 
> Please don't add this kind of test here. This directory is for testing general popupNotification UI behavior and not if a permission prompt is correctly setting its permissions.
> 
> You're looking for http://searchfox.org/mozilla-central/source/browser/modules/test/browser/browser_PermissionUI_prompts.js
> 
> You probably just have to add a couple of lines that copy what we do for Geolocation and Notifications already.
> That test also checks for persistent/temporary permissions.
Thanks. Will discuss reusing browser_PermissionUI_prompts.js test with :Florian.

    
    

    
  
(In reply to Fischer [:Fischer] from comment #65)

> > Please don't add this kind of test here. This directory is for testing general popupNotification UI behavior and not if a permission prompt is correctly setting its permissions.
> > 
> > You're looking for http://searchfox.org/mozilla-central/source/browser/modules/test/browser/browser_PermissionUI_prompts.js
> > 
> > You probably just have to add a couple of lines that copy what we do for Geolocation and Notifications already.
> > That test also checks for persistent/temporary permissions.
> Thanks. Will discuss reusing browser_PermissionUI_prompts.js test with
> :Florian.

Go for it :-)

    
    

    
  
(In reply to Fischer [:Fischer] from comment #65)
> (In reply to Johann Hofmann [:johannh] from comment #62)
> > Comment on attachment 8833898 [details]
> > Bug 1309123 - Add Persistent storage permission to PermissionPrompts
> > mozscreenshots
> > 
> > https://reviewboard.mozilla.org/r/110024/#review123918
> > 
> > I'm unable to test this locally. I strongly suspect that would require me to
> > get the changes from bug 1286717, but since this patch should ideally land
> > very close to bug 1286717 anyway (to not get errors during the screenshot
> > process) I'll clear review for now, since the patch shouldn't be landed
> > right now. FWIW the changes look good to me :)
> Yes, it needs `navigator.storage.persist` web api to run.
> Uploaded the bug 1286717 under-reviewing patch[1].
> Please apply the patch on the latest m-c codebase if you would like to test
> it.
> [1] attachment 8849469 [details] [diff] [review]: bug-1286717-wip-patch.patch
> 
Hi Johann, 
if you would like to review it after the patch landed, please let me know. 
I will file another follow-up bug after this bug & the bug 1286717 for PermissionPrompts mozscreenshots, thanks.
Flags: needinfo?(jhofmann)

    
    

    
  
(In reply to Florian Quèze [:florian] [:flo] from comment #66)
> > > You're looking for http://searchfox.org/mozilla-central/source/browser/modules/test/browser/browser_PermissionUI_prompts.js
> > > 
> > > You probably just have to add a couple of lines that copy what we do for Geolocation and Notifications already.
> > > That test also checks for persistent/temporary permissions.
> > Thanks. Will discuss reusing browser_PermissionUI_prompts.js test with
> > :Florian.
> 
> Go for it :-)
Thanks, will reuse it.

    
    

    
  
(In reply to Fischer [:Fischer] from comment #67)
> (In reply to Fischer [:Fischer] from comment #65)
> Hi Johann, 
> if you would like to review it after the patch landed, please let me know. 
> I will file another follow-up bug after this bug & the bug 1286717 for
> PermissionPrompts mozscreenshots, thanks.

Yes, considering the platform work hasn't landed yet that's probably a good idea :)

Thanks!
Flags: needinfo?(jhofmann)

    
  
Blocks: 1349152

    
    

    
  
(In reply to Florian Quèze [:florian] [:flo] from comment #61)
> Comment on attachment 8808928 [details]
> Bug 1309123 - Show persistent-storage permission request notification
> 
> https://reviewboard.mozilla.org/r/91652/#review123928
> 
> ::: browser/locales/en-US/chrome/browser/browser.properties:386
> (Diff revision 6)
> > +persistentStorage.allowWithSite=Will you allow %S to add files to persistent storage?
> 
> all the other strings we use in this patch talk about "data" rather than
> "files", should this say "Will you allow %S to store data persistently?"
> 
Passed the feedback to UX and this has been updated to "Will you allow %S to store data in persistent storage?"

> ::: browser/locales/en-US/chrome/browser/sitePermissions.properties:21
> (Diff revision 6)
> > +permission.persistent-storage.label = Store Persistent Storage
> 
> I don't understand this string. Was it meant to be "Store Persistent Data"?
this has been updated to “Store Data in Persistent Storage”

    
    

    
      
  

    
  

        Attachment #8833898 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8808928 [details]
Bug 1309123 - Show persistent-storage permission request notification,

Hi Florian

The patch has been rebased on the central to get Johann's test for reusing.
So if directly look at the diff between r6 ~ r7, would get a lot of messy and unrelated code changes.
Here list out what changes you might want to know in this update.

- Update strings to using "Persistent"
    - browser/locales/en-US/chrome/browser/browser.dtd,
    - browser/locales/en-US/chrome/browser/browser.properties
    - browser/locales/en-US/chrome/browser/sitePermissions.properties

- browser/modules/test/xpcshell/test_SitePermissions.js has been moved to browser/modules/test/unit/test_SitePermissions.js after rebasing to the central

- Reuse the test of browser/modules/test/browser/browser_PermissionUI_prompts.js

Thank you

        Attachment #8808928 -
        Flags: review?(florian)

    
    

    
  
Comment on attachment 8808928 [details]
Bug 1309123 - Show persistent-storage permission request notification,

https://reviewboard.mozilla.org/r/91652/#review129536

r+ because the code looks reasonable. But I still don't understand how "Store Data in Persistent Storage" is expected to be different from "Maintain Offline Storage" from a user point of view. That's a UX question though (I asked in comment 50 and haven't got any reply about this). See also Paolo's comment 29.

        Attachment #8808928 -
        Flags: review?(florian) → review+

    
    

    
      
  

    
  
Depends on: 1354500

    
    

    
  
(In reply to Florian Quèze [:florian] [:flo] from comment #73)
> Comment on attachment 8808928 [details]
> Bug 1309123 - Show persistent-storage permission request notification,
> 
> https://reviewboard.mozilla.org/r/91652/#review129536
> 
> r+ because the code looks reasonable. But I still don't understand how
> "Store Data in Persistent Storage" is expected to be different from
> "Maintain Offline Storage" from a user point of view. That's a UX question
> though (I asked in comment 50 and haven't got any reply about this). See
> also Paolo's comment 29.
Yes, that would confuse people. A follow-up Bug 1354500 has been filed.

    
    

    
  
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s b6f71c142b78 -d 18a286ccf1be: rebasing 387970:b6f71c142b78 "Bug 1309123 - Show persistent-storage permission request notification, r=florian" (tip)
merging modules/libpref/init/all.js
warning: conflicts while merging modules/libpref/init/all.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

    
    

    
      
  

    
  
Keywords: checkin-needed

    
    

    
  
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 5d3e802a96a0 -d c8193c2ac137: rebasing 388238:5d3e802a96a0 "Bug 1309123 - Show persistent-storage permission request notification, r=florian" (tip)
merging browser/components/nsBrowserGlue.js
merging browser/locales/en-US/chrome/browser/browser.dtd
merging browser/locales/en-US/chrome/browser/browser.properties
merging browser/locales/en-US/chrome/browser/sitePermissions.properties
merging browser/modules/SitePermissions.jsm
merging browser/modules/test/unit/test_SitePermissions.js
merging browser/themes/shared/notification-icons.inc.css
merging browser/themes/shared/notification-icons.svg
merging modules/libpref/init/all.js
warning: conflicts while merging browser/locales/en-US/chrome/browser/sitePermissions.properties! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/modules/SitePermissions.jsm! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/modules/test/unit/test_SitePermissions.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/themes/shared/notification-icons.inc.css! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

    
  
Keywords: checkin-needed

    
    

    
      
  

    
  
Keywords: checkin-needed

    
    

    
  
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4f90a5f5fb16
Show persistent-storage permission request notification, r=florian
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/4f90a5f5fb16
Status: NEW → RESOLVED
Closed: 7 years ago

          status-firefox55:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

    
  
Depends on: 1355795

    
  
No longer depends on: 1355795
Depends on: 1358384

    
    

    
  
I have reproduced this bug with Nightly 52.0a1 (2016-10-10) on Windows 8.1(64 bit).

This bug's fix is verified on Latest Nightly 55.0a1.

Build ID : 20170531030204
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170531]

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: