Closed Bug 588305 Opened 9 years ago Closed 7 years ago

Convert offline storage notifications to a doorhanger panel

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 23

People

(Reporter: faaborg, Assigned: markh)

References

Details

(Whiteboard: [doorhanger][good first bug][mentor=margaret])

Attachments

(1 file, 9 obsolete files)

We should convert offline Storage notifications (permission, quota) from using a notification bar to using a doorhanger panel anchored to the site identity block.

More details about the general design direction for doorhanger panels can be found in the parent bug 588240, please use that bug for any discussion or concerns.
Depends on: 588613
Whiteboard: [doorhanger]
Whiteboard: [doorhanger] → [doorhanger][good first bug][mentor=margaret]
Comment on attachment 578600 [details] [diff] [review]
Changed Notification bar to Door hanging panel.

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

This is a great start! Once you address these comments, you can upload a new patch and flag me for review.

As a general comment, you should use let instead of var for your variable declarations (the vars in there right now were likely written before we had let).

Also, to get a large icon to appear in the notification, you need to add our new popup to the CSS selectors here: http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser.css#2297, http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/browser.css#2423, and http://mxr.mozilla.org/mozilla-central/source/browser/themes/gnomestripe/browser.css#1230. Since we're including the host in each notificationID, you'll need to use a slightly different attribute selector: |.popup-notification-icon[popupid*="offline-app-requested"]|

::: browser/base/content/browser.js
@@ +6382,2 @@
>      var notificationID = "offline-app-requested-" + host;
> +    const anchorID = "indexedDB-notification-icon";

I don't think this needs to use const - it could just use let. Also, since you don't use this until you call PopupNotifications.show, you can declare it right above there.

@@ +6382,3 @@
>      var notificationID = "offline-app-requested-" + host;
> +    const anchorID = "indexedDB-notification-icon";
> +    var notification = PopupNotifications.getNotification(anchorID,browser);

To get the notification, you're going to want to pass in notificationID here, not anchorID.

@@ +6390,5 @@
> +	    return;
> +	options.contentWindow = null;
> +	options.sourceURI = null;
> +    };
> +    

You're not using any of these three objects, so I'm not sure why you're setting them. Was this just a copy/paste error?

@@ +6419,5 @@
>        var message = gNavigatorBundle.getFormattedString("offlineApps.available",
>                                                          [ host ]);
> +      notification = PopupNotifications.show(browser, notificationID,message,
> +					     anchorID,mainAction,
> +					     secondaryActions,options);

To follow the style of the rest of the file, you should put spaces after the commas in your argument list.
Attachment #578600 - Flags: feedback+
Assignee: nobody → jigneshhk1992
Status: NEW → ASSIGNED
(In reply to Margaret Leibovic [:margaret] from comment #2)

> @@ +6390,5 @@
> > +	    return;
> > +	options.contentWindow = null;
> > +	options.sourceURI = null;
> > +    };
> > +    
> 
> You're not using any of these three objects, so I'm not sure why you're
> setting them. Was this just a copy/paste error?

Grr, splinter review cut off what I was trying to comment on. I'm talking about these three things:

+    options.contentWindow = browser.contentWindow;
+    options.sourceURI = browser.currentURI;
+    options.eventCallback = function(aEvent) {
+    	if (aEvent != "removed")
+	    return;
+	options.contentWindow = null;
+	options.sourceURI = null;
+    };
Attached patch Modified one. (obsolete) — Splinter Review
Attachment #578628 - Flags: review?
Comment on attachment 578600 [details] [diff] [review]
Changed Notification bar to Door hanging panel.

Marking this patch as obsolete, since you uploaded a new version.
Attachment #578600 - Attachment is obsolete: true
Comment on attachment 578628 [details] [diff] [review]
Modified one.

Almost there! The CSS you added looks good, but there are a few more things you should fix in your JS changes.

>diff -r c101c5f8c928 browser/base/content/browser.js

>+    let notification = PopupNotifications.getNotification(notificationID,browser);

Nit: space after comma.

>+    let options= {};
>+    options.contentWindow = browser.contentWindow;
>+    options.sourceURI = browser.currentURI;
>+    options.eventCallback = function(aEvent) {
>+    	if (aEvent != "removed")
>+	    return;
>+    };

You can get rid of this whole chunk of code, since it's not used, and just create the options object down below with the documents property to pass to PopupNotifications.show.
                                                
>+      notification = PopupNotifications.show(browser, notificationID, message,
>+					     anchorID, mainAction,
>+					     secondaryActions, options);
>+      notification.options.documents = [ aContentWindow.document ];

Instead of setting this, I'm thinking you can do something like:

let options = {
  documents: [ aContentWindow.document ]
};
PopupNotifications.show(..., options);

(You don't need to set notification like you're doing now, since the function just returns after this call and doesn't use notification anywhere.)
Attachment #578628 - Flags: review? → review-
Attached patch removed options (obsolete) — Splinter Review
Attachment #578642 - Flags: review?(margaret.leibovic)
Attached patch door hanger panel (obsolete) — Splinter Review
Attachment #578648 - Flags: review?(margaret.leibovic)
Comment on attachment 578642 [details] [diff] [review]
removed options

Looks good! I just have two nits. Since I'm not technically a reviewer, I'm flagging Gavin to review this as well, but I think this is pretty solid. If you interested in more work, you could also file (and fix) a follow-up bug to change the other quota usage warning notification as well: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6308. 

>diff -r c101c5f8c928 browser/base/content/browser.js
                                                
>+      let options= {
>+	documents:[ aContentWindow.document ]

Nit: add a space after the colon.

>+      };
>+      notification = PopupNotifications.show(browser, notificationID, message,
>+					     anchorID, mainAction,
>+					     secondaryActions, options);

Nit: You don't need to set notification.
Attachment #578642 - Flags: review+
Attachment #578642 - Flags: review?(margaret.leibovic) → review?(gavin.sharp)
Attachment #578628 - Attachment is obsolete: true
Attached patch door hanger panel (obsolete) — Splinter Review
Attachment #578657 - Flags: review?(margaret.leibovic)
Comment on attachment 578657 [details] [diff] [review]
door hanger panel

For future reference, when you upload new versions of a patch, it's helpful to mark old versions as obsolete, and to include what you've changed in a comment with the patch.
Attachment #578657 - Flags: review?(margaret.leibovic) → review?(gavin.sharp)
Attachment #578648 - Attachment is obsolete: true
Attachment #578648 - Flags: review?(margaret.leibovic)
Attachment #578642 - Attachment is obsolete: true
Attachment #578642 - Flags: review?(gavin.sharp)
ps:I double checked Previous patch(#578657).It doesn't response to mouse clicks on allow buttons. Here is the final version which works well and stores offline cache.
Attachment #578657 - Attachment is obsolete: true
Attachment #578908 - Flags: review?(margaret.leibovic)
Attachment #578657 - Flags: review?(gavin.sharp)
Comment on attachment 578908 [details] [diff] [review]
Changed options for popupnotification.

Looking at the inter-diff between this patch and the last one, I'm confused about what you changed to fix a bug. What exactly was wrong with the last patch that you changed to make it work in this patch?

>diff -r c101c5f8c928 browser/base/content/browser.js
>+    let notification = PopupNotifications.getNotification(notificationID,browser);

Somehow, the space you added here got left out in this version.

>+      let options= { };
>+      notification = PopupNotifications.show(browser, notificationID, message, 
>+					     anchorID, mainAction, 
>+					     secondaryActions, options);
>+      notification.options.documents = [ aContentWindow.document ];

Why did you change the way the options object was declared here?
Attached patch patch_12 (obsolete) — Splinter Review
Changes in previous patch : 
 notification = Popupnotification.show();
 - Because notification.options.documents is used in callback afterwords. 

Earlier it was :
 Popupnotification.show();

This patch :
 - corrected options object.
Attachment #578908 - Attachment is obsolete: true
Attachment #579003 - Flags: review?(margaret.leibovic)
Attachment #578908 - Flags: review?(margaret.leibovic)
Comment on attachment 579003 [details] [diff] [review]
patch_12

(In reply to Jignesh kakadiya from comment #14)
> Changes in previous patch : 
>  notification = Popupnotification.show();
>  - Because notification.options.documents is used in callback afterwords. 

Ah, okay, that's pretty tricky. Good catch :)

We still need Gavin to r+ this before it lands, so I'm flagging him for review again.
Attachment #579003 - Flags: review?(margaret.leibovic)
Attachment #579003 - Flags: review?(gavin.sharp)
Attachment #579003 - Flags: review+
Gavin, I'm not sure this needs to wait for bug 588613 before landing. Any ETA on a review?
Comment on attachment 579003 [details] [diff] [review]
patch_12

Sorry that I never got to this :(

The patch no longer applies cleanly now, but I can find someone to review this if someone is interested in updating it.
Attachment #579003 - Flags: review?(gavin.sharp)
This should make the social permissions work (bug 809085) a little less painful, so here you go.

This is the original patch by :jhk with a couple of changes.  The code no longer looks for an existing notification with the same ID as this functionality is provided directly by PopupNotifications.jsm.  There are also 2 tests impacted by this and fixes for these are also included.  Single-platform try run at https://tbpl.mozilla.org/?tree=Try&rev=6d468fd4ad4c

Gavin, it would be great if you could suggest a reviewer...
Flags: needinfo?(gavin.sharp)
Comment on attachment 732788 [details] [diff] [review]
Original patch with tweaks, plus test changes

Maybe Matt can help out?
Attachment #732788 - Flags: review?(mnoorenberghe+bmo)
Flags: needinfo?(gavin.sharp)
Attachment #579003 - Attachment is obsolete: true
Comment on attachment 732788 [details] [diff] [review]
Original patch with tweaks, plus test changes

This patch is wrong - we do indeed need to find an existing notification as OffineApps.[dis]allowSite() must be called explicitly for each document, not just once for the domain.  I'll upload a new patch as soon as I get a green try.
Attachment #732788 - Attachment is obsolete: true
Attachment #732788 - Flags: review?(mnoorenberghe+bmo)
Closer to the original patch.  Green try (for a single platform) at https://tbpl.mozilla.org/?tree=Try&rev=11a625c17331
Attachment #733169 - Flags: review?(mnoorenberghe+bmo)
FWIW, maybe:
  let anchorID = "indexedDB-notification-icon";
should be replaced with:
  let anchorID = "webapps-notification-icon";

This impacts which icon is shown in the URL bar when the notification is "current".  OTOH though, while this feature is used regularly by "apps", it can also be used by regular web pages - so maybe the generic storage icon is indeed better?  What is an "app" again? :)
Blocks: 809085
taking this bug, and now sounds like a good opportunity for a review-ping for MattN.
Assignee: jigneshhk1992 → mhammond
Comment on attachment 733169 [details] [diff] [review]
updated so all documents get the correct permissions

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

Sorry for the delay. r=me

::: browser/base/content/test/test_offlineNotification.html
@@ +104,5 @@
>  
>  function loaded() {
>    testEventHandling();
>  
> +  // Click the notification box's "Allow" button.  This should kick

Nit: s/box/panel/

@@ +118,5 @@
>  
> +  // should have dismissed one of the notifications.
> +  is(panel.childElementCount, 1, "1 notification now being displayed");
> +  button = win.document.getAnonymousElementByAttribute(panel.firstElementChild, "anonid", "button");
> +  button.click();

panel.firstElementChild.button.click();

::: browser/base/content/test/test_offline_gzip.html
@@ +95,5 @@
>    }
>  }
>  
>  function loaded() {
> +  // Click the notification box's "Allow" button.  This should kick

Nit: s/box/panel/

@@ +102,5 @@
>    var wm = SpecialPowers.Cc["@mozilla.org/appshell/window-mediator;1"].
>             getService(SpecialPowers.Ci.nsIWindowMediator);
>    var win = wm.getMostRecentWindow("navigator:browser");
> +  var panel = win.PopupNotifications.panel;
> +  var button = win.document.getAnonymousElementByAttribute(panel.firstElementChild, "anonid", "button");

Reaching into the panel like that seems dirtier than the following which is similar to triggerMainCommand in browser_popupNotification.js:
panel.firstElementChild.button.click();

::: browser/themes/windows/browser.css
@@ +2220,1 @@
>    list-style-image: url(chrome://mozapps/skin/extensions/extensionGeneric.png);

This image doesn't feel right to me. Could you make sure there's a bug on file to get a storage image for this and indexedDB or handle it in this one (preferred).  In the meantime you could use the same one as indexedDB-permissions-prompt: https://mxr.mozilla.org/mozilla-central/search?string=popupid=%22indexedDB-permissions-prompt%22
Attachment #733169 - Flags: review?(mnoorenberghe+bmo) → review+
All comments addressed - the changes requested are simple but a quick look can't hurt.  However, I'm not completely clear on:

(In reply to Matthew N. [:MattN] from comment #24)
> Comment on attachment 733169 [details] [diff] [review]
> This image doesn't feel right to me. Could you make sure there's a bug on
> file to get a storage image for this and indexedDB or handle it in this one
> (preferred).  In the meantime you could use the same one as
> indexedDB-permissions-prompt:
> https://mxr.mozilla.org/mozilla-central/search?string=popupid=%22indexedDB-
> permissions-prompt%22

I updated the patch to use the existing indexeddb icon and anchor, but I'm unclear if you are also suggesting we need a bug on file to use a different image here, or that the indexeddb image should be different than the generic "question" icon, or something else?

Try for this and the patch in bug 857868 at https://tbpl.mozilla.org/?tree=Try&rev=e4efb20b98ac (hopefully I'll wake up to it being all green ;)
Attachment #733169 - Attachment is obsolete: true
Attachment #739447 - Flags: review?(mnoorenberghe+bmo)
(In reply to Mark Hammond (:markh) from comment #25)
> I updated the patch to use the existing indexeddb icon and anchor, but I'm
> unclear if you are also suggesting we need a bug on file to use a different
> image here, or that the indexeddb image should be different than the generic
> "question" icon, or something else?

I think both indexedDB and offline storage notifications should use a better anchor and notification icon (something which makes them more identifiable as the question mark is too generic.).  I'll leave the decision of whether they should use the same image or not up to the UI/UX folks.
Comment on attachment 739447 [details] [diff] [review]
Updated per feedback

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

Could you reference the bug number for icons in this bug so people can follow along? Thanks!
Attachment #739447 - Flags: review?(mnoorenberghe+bmo) → review+
Bug 593901 is for an indexeddb specific icon we can use.
https://hg.mozilla.org/mozilla-central/rev/ea5490a3bca7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
You need to log in before you can comment on or make changes to this bug.