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]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-15 01:38 PDT by Tim Taubert [:ttaubert]
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]
no flags Details | Diff | Splinter Review
patch v2 (11.87 KB, patch)
2011-07-24 15:14 PDT, Tim Taubert [:ttaubert]
dao+bmo: review+
Details | Diff | Splinter Review

Description User image Tim Taubert [:ttaubert] 2011-06-15 01:38:45 PDT

    
Comment 1 User image Tim Taubert [:ttaubert] 2011-06-15 01:39:34 PDT
* closeGroupItem(groupItem);
* showTabView();
* hideTabView();

(etc.)
Comment 2 User image Marcos Aruj 2011-06-15 13:42:07 PDT
Created attachment 539644 [details] [diff] [review]
Patch for bug 664379: Make callbacks optional.
Comment 3 User image 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 User image Tim Taubert [:ttaubert] 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 User image Tim Taubert [:ttaubert] 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 User image 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 User image 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 User image Tim Taubert [:ttaubert] 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 User image 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 User image Tim Taubert [:ttaubert] 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 User image 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 User image Tim Taubert [:ttaubert] 2011-07-24 14:52:21 PDT
Created attachment 548047 [details] [diff] [review]
patch v1
Comment 13 User image 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 User image Tim Taubert [:ttaubert] 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 User image Tim Taubert [:ttaubert] 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 User image Tim Taubert [:ttaubert] 2011-08-09 10:17:40 PDT
http://hg.mozilla.org/integration/fx-team/rev/7840c41b4943
Comment 17 User image Tim Taubert [:ttaubert] 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.