Closed Bug 559641 Opened 15 years ago Closed 15 years ago

integrate tab-browser module

Categories

(Add-on SDK Graveyard :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: myk, Assigned: myk)

References

()

Details

Attachments

(2 files, 1 obsolete file)

Some modules (like the selection module in bug 547092) use the tab-browser module from Atul's personal module repository, so we should integrate that into jetpack-core. Note that patch in bug 547092 includes a change to the tab-browser module that we should include when integrating it. Also, tab-browser depends on the unload-2 module, whose functionality has been merged into the unload module per bug 556940, so tab-browser needs to be updated to take that into account.
Comment on attachment 439557 [details] [diff] [review] patch v1: straight copy from Atul's packages repository >+ * Portions created by the Initial Developer are Copyright (C) 2007 Nit: 2009 or 2010? >+ var browser = win.document.querySelector("tabbrowser"); >+ browser.selectedTab = browser.addTab(url); It seems like this should allow callers to distinguish between opening a tab in the foreground vs. the background, f.e. with a "background" boolean option whose default value is false. >+ require("unload-2").ensure(this); This should use the "unload" module, which has acquired unload-2's functionality. >+ onUntrack: function onUntrack(window) { >+ for (browser in tabBrowserIterator(window)) >+ this._browsers.splice(this._browsers.indexOf(browser), 1); Array.splice counts a negative index from the end of the array, so if browser isn't in this._browsers for some reason, making this._browsers.indexOf(browser) be -1, then this statement is the equivalent of this._browsers.splice(-1, 1), which has the effect of removing the last browser from the array. It should make sure browser is in the array before attempting to splice it out. Finally, the test file should also have a license header. Otherwise looks great.
Attachment #439557 - Flags: review?(myk) → review-
(In reply to comment #2) > (From update of attachment 439557 [details] [diff] [review]) > >+ * Portions created by the Initial Developer are Copyright (C) 2007 > > Nit: 2009 or 2010? Atul looked up the creation date in his personal packages repository and told me this module was created in 2010, so I've corrected this. > >+ var browser = win.document.querySelector("tabbrowser"); > >+ browser.selectedTab = browser.addTab(url); > > It seems like this should allow callers to distinguish between opening a tab in > the foreground vs. the background, f.e. with a "background" boolean option > whose default value is false. This is an enhancement rather than a bug, so I'm going to punt on this for now (really, it matters more for the high-level API than the low-level one, which can acquire it as needed). > >+ require("unload-2").ensure(this); > > This should use the "unload" module, which has acquired unload-2's > functionality. Fixed. > >+ onUntrack: function onUntrack(window) { > >+ for (browser in tabBrowserIterator(window)) > >+ this._browsers.splice(this._browsers.indexOf(browser), 1); > > Array.splice counts a negative index from the end of the array, so if browser > isn't in this._browsers for some reason, making this._browsers.indexOf(browser) > be -1, then this statement is the equivalent of this._browsers.splice(-1, 1), > which has the effect of removing the last browser from the array. It should > make sure browser is in the array before attempting to splice it out. Fixed. > Finally, the test file should also have a license header. Fixed. Atul: I reviewed your code earlier; can you review these changes to it to ensure they are correct?
Attachment #439557 - Attachment is obsolete: true
Attachment #439622 - Flags: review?(avarma)
Thanks Myk! Your fixes are good, one minor nit though: regarding the splice() issue, it'd be great if you could raise an error with the text "internal error: browser tab not found" or somesuch if the index did turn out to be -1, since this is something that, as far as I know, should never happen, and if it does happen, it'd be helpful to know that our assumptions aren't correct. Again though, it's a nit, so no worries if you don't agree or have the time.
Attachment #439622 - Flags: review?(avarma) → review+
Added whenTabClosed() function and test. The test doesn't work, and I don't know really how to get it to work. My knowledge of opening/closing tabs, browers, and windows within the jetpack cfx environment is very limited. I have little confidence that whenTabClosed() itself is properly implemented, but I need something like it for bug 547092. Please help if possible
Attachment #440121 - Attachment is patch: true
Attachment #440121 - Attachment mime type: application/x-gzip → text/plain
Attachment #440121 - Attachment is patch: false
Attachment #440121 - Attachment mime type: text/plain → application/x-gzip
Blocks: 560467
(In reply to comment #4) > Thanks Myk! Your fixes are good, one minor nit though: regarding the splice() > issue, it'd be great if you could raise an error with the text "internal error: > browser tab not found" or somesuch if the index did turn out to be -1, since > this is something that, as far as I know, should never happen, and if it does > happen, it'd be helpful to know that our assumptions aren't correct. Fixed by changeset http://hg.mozilla.org/labs/jetpack-sdk/rev/52d2fc691038 with the only change being the addition of |console.error("internal error: browser tab not found")| in onUntrack when a browser being iterated isn't found in this._browsers. (In reply to comment #5) > Added whenTabClosed() function and test. I filed bug 560467 on adding this function, attached an updated version of your patch that applies to the trunk, and requested review from Atul.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product. To filter bugmail related to this change, filter on the word "looptid".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: