Closed Bug 664379 Opened 13 years ago Closed 13 years ago

make callbacks for head.js functions optional where appropriate

Categories

(Firefox Graveyard :: Panorama, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 8

People

(Reporter: ttaubert, Assigned: ttaubert)

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
* closeGroupItem(groupItem); * showTabView(); * hideTabView(); (etc.)
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)!
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. :)
Attached patch patch v1 (obsolete) — Splinter Review
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?
Attached patch patch v2Splinter Review
(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)
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
Status: ASSIGNED → RESOLVED
Closed: 13 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.

Attachment

General

Creator:
Created:
Updated:
Size: