Last Comment Bug 664379 - make callbacks for head.js functions optional where appropriate
: make callbacks for head.js functions optional where appropriate
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 8
Assigned To: Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-15 01:38 PDT by Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
Modified: 2016-04-12 14:00 PDT (History)
2 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch for bug 664379: Make callbacks optional. (6.78 KB, patch)
2011-06-15 13:42 PDT, Marcos Aruj
ttaubert: feedback-
Details | Diff | Splinter Review
Patch for bug 664379: Make callbacks optional where appropriate. (7.74 KB, patch)
2011-06-21 09:14 PDT, Marcos Aruj
dao+bmo: review-
ttaubert: feedback+
Details | Diff | Splinter Review
patch v1 (11.99 KB, patch)
2011-07-24 14:52 PDT, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
no flags Details | Diff | Splinter Review
patch v2 (11.87 KB, patch)
2011-07-24 15:14 PDT, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
dao+bmo: review+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-06-15 01:38:45 PDT

    
Comment 1 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-06-15 01:39:34 PDT
* closeGroupItem(groupItem);
* showTabView();
* hideTabView();

(etc.)
Comment 2 Marcos Aruj 2011-06-15 13:42:07 PDT
Created attachment 539644 [details] [diff] [review]
Patch for bug 664379: Make callbacks optional.
Comment 3 Dão Gottwald [:dao] 2011-06-15 13:49:43 PDT
Comment on attachment 539644 [details] [diff] [review]
Patch for bug 664379: Make callbacks optional.

>     groupItem.addSubscriber(groupItem, "close", function() {
>       groupItem.removeSubscriber(groupItem, "close");
>+      if (callback)
>-      callback();
>+        callback();
>     });

You don't need to call addSubscriber if there's no callback. The same pattern is repeated a couple of times in this patch.

>   function onLoad() {
>     this.removeEventListener("load", onLoad, true);
>     stillToLoad--;
>-    if (!stillToLoad)
>+    if (!stillToLoad && callback)
>       executeSoon(callback);
>   }
> 
>@@ -123,7 +126,7 @@
>     }
>   }
> 
>-  if (!stillToLoad)
>+  if (!stillToLoad && callback)
>     executeSoon(callback);
> }

Calling this function without a callback probably doesn't make sense.

> function whenWindowLoaded(win, callback) {
>   win.addEventListener("load", function onLoad() {
>     win.removeEventListener("load", onLoad, false);
>+    if (callback)
>-    executeSoon(callback);
>+      executeSoon(callback);
>   }, false);
> }
> 
>@@ -284,7 +304,8 @@
> function whenWindowStateReady(win, callback) {
>   win.addEventListener("SSWindowStateReady", function onReady() {
>     win.removeEventListener("SSWindowStateReady", onReady, false);
>+    if (callback)
>-    executeSoon(callback);
>+      executeSoon(callback);
>   }, false);
> }

These changes definitely don't make sense.
Comment 4 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-06-16 02:01:54 PDT
Comment on attachment 539644 [details] [diff] [review]
Patch for bug 664379: Make callbacks optional.

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

1.) Please use if ("function" == typeof callback) to check if a callback was passed to the function.
2.) Callbacks are optional for the following functions:

* showTabView
* hideTabView
* showSearch
* hideSearch
* closeGroupItem
* hideGroupItem
* unhideGroupItem

3.) Please remove the checks for all other functions because if they're named like "when*" or "after*" it's obvious that we definitely want to wait for an event to occur and we need to pass a callback here. Functions like "newWindowWithState", "restoreTab", "newWindowWithTabView" pass the restored tab or created window to the callback so they should be required here too.

4.) Please use a context of 8 lines when creating a diff - https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_customize_the_format_of_the_patches_Mercurial_creates.3f
5.) Thanks for the patch :)

::: browser/base/content/test/tabview/head.js
@@ +198,2 @@
>  
>    let contentWindow = win.document.getElementById("tab-view").contentWindow;

Please replace this line with "let contentWindow = win.TabView.getContentWindow()".

@@ +213,2 @@
>  
>    let contentWindow = win.document.getElementById("tab-view").contentWindow;

Please replace this line with "let contentWindow = win.TabView.getContentWindow()".

@@ +228,2 @@
>  
>    let contentWindow = win.document.getElementById("tab-view").contentWindow;

Please replace this line with "let contentWindow = win.TabView.getContentWindow()".

::: browser/base/content/test/tabview/browser_tabview_bug649006.js
@@ +12,1 @@
>    });

Even shorter: registerCleanupFunction(hideTabView);
Comment 5 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-06-16 02:03:56 PDT
(In reply to comment #3)

Oops, didn't see your feedback when posting mine.

> >     groupItem.addSubscriber(groupItem, "close", function() {
> >       groupItem.removeSubscriber(groupItem, "close");
> >+      if (callback)
> >-      callback();
> >+        callback();
> >     });
> 
> You don't need to call addSubscriber if there's no callback. The same
> pattern is repeated a couple of times in this patch.

Yeah, good catch, thanks!
Comment 6 Dão Gottwald [:dao] 2011-06-16 03:18:41 PDT
(In reply to comment #4)
> 1.) Please use if ("function" == typeof callback) to check if a callback was
> passed to the function.

No, this is unnecessary, please use if (callback)!
Comment 7 Marcos Aruj 2011-06-21 09:14:09 PDT
Created attachment 540770 [details] [diff] [review]
Patch for bug 664379: Make callbacks optional where appropriate.

Hi, 

I've applied your suggestions and created this new patch. What do you think?

Thanks,

Marcos.
Comment 8 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-06-22 01:28:13 PDT
Comment on attachment 540770 [details] [diff] [review]
Patch for bug 664379: Make callbacks optional where appropriate.

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

Looks good!

F+ with the fix for closeGroupItem().

::: browser/base/content/test/tabview/head.js
@@ +51,5 @@
> +      groupItem.addSubscriber(groupItem, "close", function() {
> +        groupItem.removeSubscriber(groupItem, "close");
> +        callback();
> +      });
> +      groupItem.closeHidden();

We must ensure that .closeHidden() is called. So we want the "groupHidden" subscriber but need no "close" subscriber if no callback is given.
Comment 9 Dão Gottwald [:dao] 2011-06-28 01:08:10 PDT
Comment on attachment 540770 [details] [diff] [review]
Patch for bug 664379: Make callbacks optional where appropriate.

need to address comment 8
Comment 10 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-07-21 19:56:03 PDT
Hey Marcos, do you still want to be assigned to this bug or do you want me to finish the patch? Anyway, thanks for preparing!
Comment 11 Raymond Lee [:raymondlee] 2011-07-24 10:16:50 PDT
(In reply to comment #10)
> Hey Marcos, do you still want to be assigned to this bug or do you want me
> to finish the patch? Anyway, thanks for preparing!

Tim: if you have time, feel free to finish the patch. :)
Comment 12 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-07-24 14:52:21 PDT
Created attachment 548047 [details] [diff] [review]
patch v1
Comment 13 Dão Gottwald [:dao] 2011-07-24 14:59:37 PDT
Comment on attachment 548047 [details] [diff] [review]
patch v1

> function showTabView(callback, win) {

You seem to never call this without a callback and there's probably no point in doing so.

> function showSearch(callback, win) {

This appears to be unused?
Comment 14 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-07-24 15:14:59 PDT
Created attachment 548048 [details] [diff] [review]
patch v2

(In reply to comment #13)
> > function showTabView(callback, win) {
> 
> You seem to never call this without a callback and there's probably no point
> in doing so.

Yes, you're right. This doesn't seem sensible - changes reverted.

> > function showSearch(callback, win) {
> 
> This appears to be unused?

Interesting, didn't know that. Removed.
Comment 15 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-07-24 18:38:16 PDT
bugspam

(Fx7 was branched, removing open bugs from Fx7 meta bug, we won't create new meta bugs for upcoming Fx versions)
Comment 16 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-08-09 10:17:40 PDT
http://hg.mozilla.org/integration/fx-team/rev/7840c41b4943
Comment 17 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-08-09 16:37:50 PDT
http://hg.mozilla.org/mozilla-central/rev/7840c41b4943

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