Closed
Bug 1238312
Opened 8 years ago
Closed 8 years ago
Implement tabs.duplicate API
Categories
(WebExtensions :: Untriaged, defect, P2)
WebExtensions
Untriaged
Tracking
(firefox47 fixed)
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: kmag, Assigned: andy+bugzilla)
References
Details
(Whiteboard: [tabs] triaged)
Attachments
(2 files)
Assignee | ||
Comment 1•8 years ago
|
||
Mind if I take this? Think I've got a working patch for it.
Assignee | ||
Comment 3•8 years ago
|
||
Thanks rpl for showing me the event handling code.
Attachment #8709208 -
Flags: feedback?(lgreco)
Attachment #8709208 -
Flags: feedback?(kmaglione+bmo)
Comment 4•8 years ago
|
||
Comment on attachment 8709208 [details] [diff] [review] chrome.tabs.duplicate.patch Review of attachment 8709208 [details] [diff] [review]: ----------------------------------------------------------------- About the context.callOnClose/forgetOnClose helpers: I put them in my previous suggestion because I'm a bit worried about the following scenario an addon context has a listener already subscribed to the event but the context is closed before the event could be delivered to the that context. Nevertheless, Currently the context.callOnClose/forgetOnClose helpers are not directly used by any of the WebExtension "ext-API.js" files, and in the same file ("ext-tabs.js") tabs.create does something similar and it does not handle the above scenario explicitly: https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-tabs.js#302 On this I would like to hear Kris opinion. Besides the callOnClose/forgetOnClose and the other small comments on the patch, it looks good to me. ::: browser/components/extensions/ext-tabs.js @@ +609,5 @@ > }, > + > + duplicate: function(tabId, callback) { > + let tab = TabManager.getTab(tabId); > + if (!tab) { it could be helpful to log an error if the API method call is invalid (e.g. a non existent tabId), mainly to prevent silent errors (at least until we start to support runtime.lastError) e.g. Cu.reportError(`Unknown tabId ${tabId} on tabs.duplicate`); ::: browser/components/extensions/test/browser/browser_ext_tabs_duplicate.js @@ +1,1 @@ > +"use strict"; Nit, all the other test files has the following two lines of comments to pre-configure the indentation style on some editors: /* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */ /* vim: set sts=2 sw=2 et tw=80: */ @@ +1,5 @@ > +"use strict"; > + > +add_task(function* () { > + let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.net/"); > + gBrowser.selectedTab = tab; not needed, openNewForegroundTab already selects the new tab @@ +16,5 @@ > + }, function(tabs) { > + let source = tabs[1]; > + // By moving it 0, we check that the new tab is created next > + // to the existing one. > + browser.tabs.move(source.id, {index: 0}, function(_) { Nit, our eslint config do not currently checks for unused parameters, but if we do not need the parameter then I would prefer it to be omitted instead
Attachment #8709208 -
Flags: feedback?(lgreco) → feedback+
Reporter | ||
Comment 5•8 years ago
|
||
Comment on attachment 8709208 [details] [diff] [review] chrome.tabs.duplicate.patch Review of attachment 8709208 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Just a few nits. ::: browser/components/extensions/ext-tabs.js @@ +612,5 @@ > + let tab = TabManager.getTab(tabId); > + if (!tab) { > + if (callback) { > + runSafe(context, callback, undefined); > + } I'd rather we just not call the callback, and add a TODO comment, until we have `lastError` support. Either way, please add a comment about lastError so when I implement it I'll know where it needs to be set. @@ +618,5 @@ > + } > + > + let gBrowser = tab.ownerDocument.defaultView.gBrowser; > + let newTab = gBrowser.duplicateTab(tab); > + gBrowser.moveTabTo(newTab, gBrowser.visibleTabs.indexOf(tab) + 1); I wonder if the tab will always be in |visibleTabs|... We should probably check |visibleTabs.includes(tab)| or that the index != -1 first. @@ +619,5 @@ > + > + let gBrowser = tab.ownerDocument.defaultView.gBrowser; > + let newTab = gBrowser.duplicateTab(tab); > + gBrowser.moveTabTo(newTab, gBrowser.visibleTabs.indexOf(tab) + 1); > + gBrowser.selectedTab = newTab; Does Chrome always select the new tab by default? It's not documented :( @@ +629,5 @@ > + if (newTab) { > + newTab.removeEventListener("SSTabRestored", onTabRestored); > + } > + }, > + }; Let's just check |!context.closed| from the callback rather than adding the onClose listener. It doesn't exist yet, but the patch at the top of my queue adds it. @@ +638,5 @@ > + // Remove the one shot listener. > + newTab.removeEventListener("SSTabRestored", onTabRestored); > + > + if (callback) { > + runSafe(context, callback, TabManager.convert(extension, newTab)); Let's check |if (callback)| before we add the listener. I don't think we have any use for it if there's no callback. ::: browser/components/extensions/test/browser/browser_ext_tabs_duplicate.js @@ +16,5 @@ > + }, function(tabs) { > + let source = tabs[1]; > + // By moving it 0, we check that the new tab is created next > + // to the existing one. > + browser.tabs.move(source.id, {index: 0}, function(_) { I generally prefer to include the argument and give it a useful name, in case future readers need to know what they are. Either way, please use arrow functions for both of these callbacks. @@ +20,5 @@ > + browser.tabs.move(source.id, {index: 0}, function(_) { > + browser.tabs.duplicate(source.id, function(tab) { > + browser.test.assertEq("http://example.net/", tab.url); > + browser.test.assertEq(1, tab.index); > + browser.test.assertEq(true, tab.selected); Would be nice to have descriptions for these. It's pretty hard to make sense of test output when you're comparing against values like |1| and |true|. @@ +32,5 @@ > + yield extension.startup(); > + yield extension.awaitFinish("tabs.duplicate.good"); > + yield extension.unload(); > + > + extension = ExtensionTestUtils.loadExtension({ Please add a separate task for this. Also, it's good to use descriptive names for the task functions, since they show up in test output. @@ +50,5 @@ > + yield extension.awaitFinish("tabs.duplicate.invalid"); > + yield extension.unload(); > + > + for (let tab of window.gBrowser.tabs) { > + yield BrowserTestUtils.removeTab(tab); Make sure to only remove the tabs that you created. Trying to remove the first tab can cause problems for other tests.
Attachment #8709208 -
Flags: feedback?(kmaglione+bmo) → feedback+
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Kris Maglione [:kmag] from comment #5) > Comment on attachment 8709208 [details] [diff] [review] > chrome.tabs.duplicate.patch > > Review of attachment 8709208 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good. Just a few nits. > > ::: browser/components/extensions/ext-tabs.js > @@ +612,5 @@ > > + let tab = TabManager.getTab(tabId); > > + if (!tab) { > > + if (callback) { > > + runSafe(context, callback, undefined); > > + } > > I'd rather we just not call the callback, and add a TODO comment, until we > have `lastError` support. Either way, please add a comment about lastError > so when I implement it I'll know where it needs to be set. In my testing Chrome does call the callback with undefined. lastError should be set as well though, you are right. > @@ +618,5 @@ > > + } > > + > > + let gBrowser = tab.ownerDocument.defaultView.gBrowser; > > + let newTab = gBrowser.duplicateTab(tab); > > + gBrowser.moveTabTo(newTab, gBrowser.visibleTabs.indexOf(tab) + 1); > > I wonder if the tab will always be in |visibleTabs|... We should probably > check |visibleTabs.includes(tab)| or that the index != -1 first. Good call. > @@ +619,5 @@ > > + > > + let gBrowser = tab.ownerDocument.defaultView.gBrowser; > > + let newTab = gBrowser.duplicateTab(tab); > > + gBrowser.moveTabTo(newTab, gBrowser.visibleTabs.indexOf(tab) + 1); > > + gBrowser.selectedTab = newTab; > > Does Chrome always select the new tab by default? It's not documented :( It does. > @@ +629,5 @@ > > + if (newTab) { > > + newTab.removeEventListener("SSTabRestored", onTabRestored); > > + } > > + }, > > + }; > > Let's just check |!context.closed| from the callback rather than adding the > onClose listener. It doesn't exist yet, but the patch at the top of my queue > adds it. By the time I get to this closing it will probably be there, so that's good. > @@ +638,5 @@ > > + // Remove the one shot listener. > > + newTab.removeEventListener("SSTabRestored", onTabRestored); > > + > > + if (callback) { > > + runSafe(context, callback, TabManager.convert(extension, newTab)); > > Let's check |if (callback)| before we add the listener. I don't think we > have any use for it if there's no callback. > > ::: browser/components/extensions/test/browser/browser_ext_tabs_duplicate.js > @@ +16,5 @@ > > + }, function(tabs) { > > + let source = tabs[1]; > > + // By moving it 0, we check that the new tab is created next > > + // to the existing one. > > + browser.tabs.move(source.id, {index: 0}, function(_) { > > I generally prefer to include the argument and give it a useful name, in > case future readers need to know what they are. Either way, please use arrow > functions for both of these callbacks. > > @@ +20,5 @@ > > + browser.tabs.move(source.id, {index: 0}, function(_) { > > + browser.tabs.duplicate(source.id, function(tab) { > > + browser.test.assertEq("http://example.net/", tab.url); > > + browser.test.assertEq(1, tab.index); > > + browser.test.assertEq(true, tab.selected); > > Would be nice to have descriptions for these. It's pretty hard to make sense > of test output when you're comparing against values like |1| and |true|. Fair enough. > @@ +32,5 @@ > > + yield extension.startup(); > > + yield extension.awaitFinish("tabs.duplicate.good"); > > + yield extension.unload(); > > + > > + extension = ExtensionTestUtils.loadExtension({ > > Please add a separate task for this. Also, it's good to use descriptive > names for the task functions, since they show up in test output. > > @@ +50,5 @@ > > + yield extension.awaitFinish("tabs.duplicate.invalid"); > > + yield extension.unload(); > > + > > + for (let tab of window.gBrowser.tabs) { > > + yield BrowserTestUtils.removeTab(tab); > > Make sure to only remove the tabs that you created. Trying to remove the > first tab can cause problems for other tests. Sounds good, thanks.
Updated•8 years ago
|
Flags: blocking-webextensions+
Priority: -- → P2
Whiteboard: [tabs] → [tabs] triaged
Reporter | ||
Comment 7•8 years ago
|
||
Comment on attachment 8709208 [details] [diff] [review] chrome.tabs.duplicate.patch Review of attachment 8709208 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/extensions/ext-tabs.js @@ +612,5 @@ > + let tab = TabManager.getTab(tabId); > + if (!tab) { > + if (callback) { > + runSafe(context, callback, undefined); > + } We have support for lastError now, so please do something like |return Promise.reject({message: `Invalid tab ID`: ${tabId}`})| I'm planning to change `getTab` to just throw an error when the tab ID is invalid some time soon, though, so this should only be temporary in any case. @@ +629,5 @@ > + if (newTab) { > + newTab.removeEventListener("SSTabRestored", onTabRestored); > + } > + }, > + }; On second thought, we should really just check this from `wrapPromise`, before we call any callback for the context, so I wouldn't even worry about checking it here. If you could file a follow-up bug, though, that would be helpful. @@ +638,5 @@ > + // Remove the one shot listener. > + newTab.removeEventListener("SSTabRestored", onTabRestored); > + > + if (callback) { > + runSafe(context, callback, TabManager.convert(extension, newTab)); Since bug 1234020 has partially landed, we should be returning a promise rather than taking a callback here, so this comment isn't relevant anymore. Please just add an `"async": "callback"` property to this method's definition in `schemas/tabs.json` and return a promise that resolves when the tab has been restored. You can remove the callback argument too.
Assignee | ||
Comment 8•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/34857/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/34857/
Attachment #8719066 -
Flags: review?(kmaglione+bmo)
Reporter | ||
Comment 9•8 years ago
|
||
Comment on attachment 8719066 [details] MozReview Request: bug 1238312 implement tabs.duplicate r?kmag https://reviewboard.mozilla.org/r/34857/#review31479 Please update the commit summary for the bug number comes at the start. Tree rules require that format. ::: browser/components/extensions/ext-tabs.js:658 (Diff revision 1) > + gBrowser.moveTabTo(newTab, gBrowser.visibleTabs.indexOf(tab) + 1); I don't think we can depend on the tab being visible here. We should only do this if the tab is in `visibleTabs`, or it's going to wind up at position 0 (which, incidentally, might be illegal if the tab isn't pinned and other tabs are.
Attachment #8719066 -
Flags: review?(kmaglione+bmo) → review+
Reporter | ||
Comment 10•8 years ago
|
||
Can you also update browser_ext_tabs_audio.js to use this method rather than its current workaround?
Assignee | ||
Updated•8 years ago
|
Attachment #8719066 -
Attachment description: MozReview Request: implement tabs.duplicate (bug 1238312) r?kmag → MozReview Request: bug 1238312 implement tabs.duplicate r?kmag
Assignee | ||
Comment 11•8 years ago
|
||
Comment on attachment 8719066 [details] MozReview Request: bug 1238312 implement tabs.duplicate r?kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34857/diff/1-2/
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•8 years ago
|
Attachment #8709208 -
Flags: checkin-
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/f9fc67134ca0
Keywords: checkin-needed
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f9fc67134ca0
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•