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)
Firefox Graveyard
Panorama
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 8
People
(Reporter: ttaubert, Assigned: ttaubert)
Details
Attachments
(1 file, 3 obsolete files)
11.87 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•13 years ago
|
||
* closeGroupItem(groupItem); * showTabView(); * hideTabView(); (etc.)
Comment 2•13 years ago
|
||
Attachment #539644 -
Flags: feedback?(tim.taubert)
Comment 3•13 years ago
|
||
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.
Updated•13 years ago
|
Assignee: nobody → marcos
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•13 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•13 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!
Comment 6•13 years ago
|
||
(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•13 years ago
|
||
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•13 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+
Updated•13 years ago
|
Attachment #540770 -
Flags: review?(dao)
Comment 9•13 years ago
|
||
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•13 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!
Comment 11•13 years ago
|
||
(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•13 years ago
|
||
Assignee: marcos → tim.taubert
Attachment #540770 -
Attachment is obsolete: true
Attachment #548047 -
Flags: review?(dao)
Comment 13•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
Attachment #548048 -
Flags: review?(dao) → review+
Assignee | ||
Comment 15•13 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•13 years ago
|
||
http://hg.mozilla.org/integration/fx-team/rev/7840c41b4943
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Comment 17•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/7840c41b4943
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 8
Updated•8 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•