Do not display the 2nd storage pressure notification when there is one being displayed

VERIFIED FIXED in Firefox 57

Status

()

Core
DOM: Push Notifications
P2
normal
VERIFIED FIXED
6 months ago
4 months ago

People

(Reporter: Ioana Crisan, Assigned: Fischer)

Tracking

(Blocks: 1 bug)

56 Branch
mozilla57
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [storage-v1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

6 months ago
Created attachment 8882995 [details]
NotificationStack.mp4

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170701181359

Steps to reproduce:
1. Launch Nightly 56.0a1
2. Set the "browser.storageManager.pressureNotification.minIntervalMS" pref to "1000"(1s)
3. Open an url(e.g: https://bug1377104.bmoattachments.org/attachment.cgi?id=8882153) 
4. Persist the site from step 3
5. Store data so the global limit is reached
6. Store more data 


Expected results:
Full disk warnings are not overlapped. Only one warning is displayed.

Actual results:
Full disk warnings are overlapped. Multiple warnings are displayed, one warning for each time the user tries to store more data from the global limit breach. Please see the attached video.
(Assignee)

Comment 1

6 months ago
(In reply to Ioana Crisan from comment #0)
> Created attachment 8882995 [details]
> NotificationStack.mp4
> 
> User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101
> Firefox/56.0
> Build ID: 20170701181359
> 
> Steps to reproduce:
> 1. Launch Nightly 56.0a1
> 2. Set the "browser.storageManager.pressureNotification.minIntervalMS" pref
> to "1000"(1s)
> 3. Open an url(e.g:
> https://bug1377104.bmoattachments.org/attachment.cgi?id=8882153) 
> 4. Persist the site from step 3
> 5. Store data so the global limit is reached
> 6. Store more data 
> 
> 
> Expected results:
> Full disk warnings are not overlapped. Only one warning is displayed.
> 
> Actual results:
> Full disk warnings are overlapped. Multiple warnings are displayed, one
> warning for each time the user tries to store more data from the global
> limit breach. Please see the attached video.
Would you please upload the video again? Got the error message of "Couldn't play because the video is corrupt", thanks.
Flags: needinfo?(icrisan)
(Reporter)

Comment 2

6 months ago
(In reply to Fischer [:Fischer] from comment #1)
> Would you please upload the video again? Got the error message of "Couldn't
> play because the video is corrupt", thanks.

Please try this link: https://drive.google.com/open?id=0B_L6mNdlzO5TOGV4ZU80SkItRFE
Flags: needinfo?(icrisan)

Updated

5 months ago
Assignee: nobody → fliu
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [storage-v1][triage] → [storage-v1]
Comment hidden (mozreview-request)

Comment 4

4 months ago
hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: abort: abandoned transaction found!
remote: (run 'hg recover' to clean up transaction)
abort: stream ended unexpectedly (got 0 bytes, expected 4)
Comment hidden (mozreview-request)

Comment 6

4 months ago
hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: abort: abandoned transaction found!
remote: (run 'hg recover' to clean up transaction)
abort: stream ended unexpectedly (got 0 bytes, expected 4)

Comment 7

4 months ago
hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: abort: abandoned transaction found!
remote: (run 'hg recover' to clean up transaction)
abort: stream ended unexpectedly (got 0 bytes, expected 4)
(Assignee)

Comment 8

4 months ago
Update the bug title to reflect what this bug is addressing.
Summary: Full disk warnings are overlapped → Do not display the 2nd storage pressure notification when there is one being displayed
Comment hidden (mozreview-request)
(Assignee)

Comment 10

4 months ago
(In reply to Fischer [:Fischer] from comment #9)
> Comment on attachment 8898162 [details]
> Bug 1377852 - Do not display the 2nd storage pressure notification when
> there is one being displayed,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/169528/diff/2-3/
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9d611604f047da43286ab8429a578929e60ffa7

Comment 11

4 months ago
mozreview-review
Comment on attachment 8898162 [details]
Bug 1377852 - Do not display the 2nd storage pressure notification when there is one being displayed,

https://reviewboard.mozilla.org/r/169528/#review175472

r=me, but if these should be one per host, then please change the notification value to include the host in it like the example I linked to below.

::: browser/base/content/browser.js:487
(Diff revision 3)
> +    if (notificationBox.getNotificationWithValue(NOTIFICATION_VALUE)) {
> +      // Do not display the 2nd notification when there is already one

Are these per domain or URI? Should we do like for feeds and still show multiple if they are for different domains or URIs?

http://searchfox.org/mozilla-central/source/browser/components/feeds/WebContentConverter.js#564,569
Attachment #8898162 - Flags: review?(jaws) → review+
(Assignee)

Comment 12

4 months ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> Comment on attachment 8898162 [details]
> Bug 1377852 - Do not display the 2nd storage pressure notification when
> there is one being displayed,
> 
> https://reviewboard.mozilla.org/r/169528/#review175472
> 
> r=me, but if these should be one per host, then please change the
> notification value to include the host in it like the example I linked to
> below.
> 
> ::: browser/base/content/browser.js:487
> (Diff revision 3)
> > +    if (notificationBox.getNotificationWithValue(NOTIFICATION_VALUE)) {
> > +      // Do not display the 2nd notification when there is already one
> 
> Are these per domain or URI? Should we do like for feeds and still show
> multiple if they are for different domains or URIs?
> 
> http://searchfox.org/mozilla-central/source/browser/components/feeds/WebContentConverter.js#564,569
Thank you for reminding this. No these are not per domain/URI. It warns when the total storage usage of all websites hits the limit.
(Assignee)

Updated

4 months ago
Keywords: checkin-needed

Comment 13

4 months ago
hg error in cmd: hg update --clean -r 4a092af42641: abort: unknown revision '4a092af42641'!
Keywords: checkin-needed
Comment hidden (mozreview-request)
(Assignee)

Comment 15

4 months ago
Rebased on the latest Central.
Keywords: checkin-needed

Comment 16

4 months ago
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8e19920c0fd8
Do not display the 2nd storage pressure notification when there is one being displayed, r=jaws
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8e19920c0fd8
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Reproduced the issue on Nightly 2017-06-12 using the testpage: https://bug1376444.bmoattachments.org/attachment.cgi?id=8881384
Verified fixed on 57.0a1 (2017-08-25) Win 7, Win 10, Ubuntu 14.04, OS X 10.12.6.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
You need to log in before you can comment on or make changes to this bug.