make callbacks for head.js functions optional where appropriate

RESOLVED FIXED in Firefox 8

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Firefox 8

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
* closeGroupItem(groupItem);
* showTabView();
* hideTabView();

(etc.)

Comment 2

6 years ago
Created attachment 539644 [details] [diff] [review]
Patch for bug 664379: Make callbacks optional.
Attachment #539644 - Flags: feedback?(tim.taubert)
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.
Assignee: nobody → marcos
Status: NEW → ASSIGNED
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);
Attachment #539644 - Flags: feedback?(tim.taubert) → feedback-
(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!
(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

6 years ago
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.
Attachment #539644 - Attachment is obsolete: true
Attachment #540770 - Flags: feedback?(tim.taubert)
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.
Attachment #540770 - Flags: feedback?(tim.taubert) → feedback+
Attachment #540770 - Flags: review?(dao)
Comment on attachment 540770 [details] [diff] [review]
Patch for bug 664379: Make callbacks optional where appropriate.

need to address comment 8
Attachment #540770 - Flags: review?(dao) → review-
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!
(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. :)
Created attachment 548047 [details] [diff] [review]
patch v1
Assignee: marcos → tim.taubert
Attachment #540770 - Attachment is obsolete: true
Attachment #548047 - Flags: review?(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?
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.
Attachment #548047 - Attachment is obsolete: true
Attachment #548047 - Flags: review?(dao)
Attachment #548048 - Flags: review?(dao)

Updated

6 years ago
Attachment #548048 - Flags: review?(dao) → review+
bugspam

(Fx7 was branched, removing open bugs from Fx7 meta bug, we won't create new meta bugs for upcoming Fx versions)
No longer blocks: 660175
http://hg.mozilla.org/integration/fx-team/rev/7840c41b4943
Whiteboard: [fixed-in-fx-team]
http://hg.mozilla.org/mozilla-central/rev/7840c41b4943
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 8
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.