Using WeakMap to store popup notification object

RESOLVED FIXED in Firefox 17

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: tetsuharu, Assigned: tetsuharu)

Tracking

Trunk
Firefox 17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

At current implementation of PopupNotifications.jsm, internal popup notifications objects are stored to |browser.popupNotifications|.
This is not low coupled design.

So I think it is good that we make this design low-coupled with using WeakMap.
(Assignee)

Updated

5 years ago
Summary: Using WeakMop to store popup notification object → Using WeakMap to store popup notification object
Created attachment 639974 [details] [diff] [review]
patch
Assignee: nobody → saneyuki.s.snyk
Status: NEW → ASSIGNED
Attachment #639974 - Flags: review?(margaret.leibovic)
Attachment #639974 - Flags: review?(margaret.leibovic) → review?(mnoorenberghe+bmo)
(Assignee)

Updated

5 years ago
Attachment #639974 - Flags: review?(mnoorenberghe+bmo) → review?(margaret.leibovic)
(Assignee)

Updated

5 years ago
Attachment #639974 - Flags: review?(margaret.leibovic) → review?(mnoorenberghe+bmo)
I"m sorry. I missed Gavin had changed the reviewer.
Comment on attachment 639974 [details] [diff] [review]
patch

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

Sorry for the delay.  r+ with the change.

::: toolkit/content/PopupNotifications.jsm
@@ +547,5 @@
>    _getNotificationsForBrowser: function PopupNotifications_getNotifications(browser) {
> +    let notifications = popupNotificationsMap.get(browser);
> +    if (!notifications) {
> +      notifications = [];
> +      popupNotificationsMap.set(browser, notifications);

Is setting the value to an empty array a performance optimization?  Otherwise you could just give [] as the default value to get:
return popupNotificationsMap.get(browser, []);
Attachment #639974 - Flags: review?(mnoorenberghe+bmo) → review+
Created attachment 650480 [details] [diff] [review]
patch rev2

(In reply to Matthew N. [:MattN] from comment #3)
> ::: toolkit/content/PopupNotifications.jsm
> @@ +547,5 @@
> >    _getNotificationsForBrowser: function PopupNotifications_getNotifications(browser) {
> > +    let notifications = popupNotificationsMap.get(browser);
> > +    if (!notifications) {
> > +      notifications = [];
> > +      popupNotificationsMap.set(browser, notifications);
> 
> Is setting the value to an empty array a performance optimization? 
> Otherwise you could just give [] as the default value to get:
> return popupNotificationsMap.get(browser, []);

Oops! I forgot the 2nd parameter of WeakMap.get().
This is not for performance optimization.
Attachment #639974 - Attachment is obsolete: true
Attachment #650480 - Flags: review?(mnoorenberghe+bmo)
(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #4)
> Created attachment 650480 [details] [diff] [review]
> 
> > Otherwise you could just give [] as the default value to get:
> > return popupNotificationsMap.get(browser, []);
> 
> Oops! I forgot the 2nd parameter of WeakMap.get().
> This is not for performance optimization.

Ok, just checking.  r+ with a change to return the .get() result directly as I have above (avoiding creating a temporary object).
Attachment #650480 - Flags: review?(mnoorenberghe+bmo) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
This causes mochitest-other failures on all platforms (thankfully caught on Try instead of the entire tree). (Yes, there's other stuff in that push. I confirmed with another push that they're green on their own).
https://tbpl.mozilla.org/?tree=Try&rev=04641e43950a
Keywords: checkin-needed
Comment on attachment 639974 [details] [diff] [review]
patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/c638bcfd56bc

Try results: https://tbpl.mozilla.org/?tree=Try&rev=5e88d343b52c

I now see why you took this initial approach.  This patch was correct and my review comment is what led to the breakage.
Attachment #639974 - Attachment is obsolete: false
Attachment #639974 - Flags: checkin+
Attachment #650480 - Attachment is obsolete: true
Attachment #650480 - Flags: review+

Comment 8

5 years ago
https://hg.mozilla.org/mozilla-central/rev/c638bcfd56bc
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
You need to log in before you can comment on or make changes to this bug.