Last Comment Bug 771818 - Using WeakMap to store popup notification object
: Using WeakMap to store popup notification object
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-07 11:01 PDT by Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
Modified: 2012-08-10 01:58 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.44 KB, patch)
2012-07-07 11:07 PDT, Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
MattN+bmo: review+
MattN+bmo: checkin+
Details | Diff | Splinter Review
patch rev2 (3.32 KB, patch)
2012-08-09 02:41 PDT, Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
no flags Details | Diff | Splinter Review

Description Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-07-07 11:01:53 PDT
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.
Comment 1 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-07-07 11:07:19 PDT
Created attachment 639974 [details] [diff] [review]
patch
Comment 2 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-07-07 11:25:41 PDT
I"m sorry. I missed Gavin had changed the reviewer.
Comment 3 Matthew N. [:MattN] 2012-08-08 17:42:31 PDT
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, []);
Comment 4 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-08-09 02:41:41 PDT
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.
Comment 5 Matthew N. [:MattN] 2012-08-09 03:30:36 PDT
(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).
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-08-09 11:41:30 PDT
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
Comment 7 Matthew N. [:MattN] 2012-08-09 16:54:42 PDT
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.
Comment 8 Ed Morley [:emorley] 2012-08-10 01:58:11 PDT
https://hg.mozilla.org/mozilla-central/rev/c638bcfd56bc

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