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

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 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);

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

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 on attachment 540770 [details] [diff] [review] Patch for bug 664379: Make callbacks optional where appropriate. need to address comment 8

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. :)

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?

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

bugspam (Fx7 was branched, removing open bugs from Fx7 meta bug, we won't create new meta bugs for upcoming Fx versions)