The default bug view has changed. See this FAQ.

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)
(Assignee)

Comment 1

6 years ago
* 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
(Assignee)

Comment 4

6 years ago
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-
(Assignee)

Comment 5

6 years ago
(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)
(Assignee)

Comment 8

6 years ago
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-
(Assignee)

Comment 10

6 years ago
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. :)
(Assignee)

Comment 12

6 years ago
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?
(Assignee)

Comment 14

6 years ago
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+
(Assignee)

Comment 15

6 years ago
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
(Assignee)

Comment 16

6 years ago
http://hg.mozilla.org/integration/fx-team/rev/7840c41b4943
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 17

6 years ago
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.