664379 - make callbacks for head.js functions optional where appropriate

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

* closeGroupItem(groupItem);
* showTabView();
* hideTabView();

(etc.)

Comment 2

[Marcos Aruj]

Attachment: Patch for bug 664379: Make callbacks optional.

Comment 3

[Dão Gottwald [:dao]]

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] (inactive)]

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] (inactive)]

(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]]

(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]

Attachment: 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] (inactive)]

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]]

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] (inactive)]

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]]

(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] (inactive)]

Attachment: patch v1

Comment 13

[Dão Gottwald [:dao]]

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] (inactive)]

Attachment: 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] (inactive)]

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] (inactive)]

http://hg.mozilla.org/integration/fx-team/rev/7840c41b4943

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/7840c41b4943