1248460 - Calling tabs.duplicated on a pinned tab

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P4)

Description

[Andy McKay]

Should create a pinned tab next to the existing one, which is what it does on chrome. Currently it creates it next to all the pinned tabs and the new tab is not pinned.

Comment 1

[Andy McKay]

Attachment: MozReview Request: bug 1248460 pin during SSTabRestoring r=kmag

Review commit: https://reviewboard.mozilla.org/r/37023/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37023/

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 8724419 [details]
MozReview Request: bug 1248460 pin during SSTabRestoring r=kmag

https://reviewboard.mozilla.org/r/37023/#review33593

::: browser/components/extensions/ext-tabs.js:789
(Diff revision 1)
> +          newTab.addEventListener("SSTabRestoring", function listener() {

Is there a reason we have to wait for this event before making these changes?

::: browser/components/extensions/ext-tabs.js:803
(Diff revision 1)
> +            gBrowser.selectTabAtIndex(newTab._tPos);

This should really just be `gBrowser.selectedTab = newTab`

::: browser/components/extensions/test/browser/browser_ext_tabs_duplicate.js:78
(Diff revision 1)
> +    if (tab.linkedBrowser.currentURI.spec === "http://example.net/") {

This is going to get into an infinite loop if this test fails.

Maybe `while (gBrowser.tabs[0].currentURI.spec == "http://example.net/")`?

Comment 3

[Andy McKay]

https://reviewboard.mozilla.org/r/37023/#review33593

> Is there a reason we have to wait for this event before making these changes?

According to the docs: https://developer.mozilla.org/en/docs/Session_store_API

"If you want to set permissions or otherwise manipulate a restored tab before the page is loaded into it, you should watch SSTabRestoring. If you want to do something after the page is loaded, you should watch SSTabRestored."

If you set tab.pinned before the tab is restored, then you lose the pinned data. If you set in the SSTabRestored event, there is a really awkward jump as the tab appears and then moves over to be pinned. We still need SSTabRestored so we can send the callback when its completed.

> This should really just be `gBrowser.selectedTab = newTab`

Sure.

> This is going to get into an infinite loop if this test fails.
> 
> Maybe `while (gBrowser.tabs[0].currentURI.spec == "http://example.net/")`?

And its shorter too, good call.

Comment 4

[Andy McKay]

Comment on attachment 8724419 [details]
MozReview Request: bug 1248460 pin during SSTabRestoring r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37023/diff/1-2/

Comment 5

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/37023/#review33593

> According to the docs: https://developer.mozilla.org/en/docs/Session_store_API
> 
> "If you want to set permissions or otherwise manipulate a restored tab before the page is loaded into it, you should watch SSTabRestoring. If you want to do something after the page is loaded, you should watch SSTabRestored."
> 
> If you set tab.pinned before the tab is restored, then you lose the pinned data. If you set in the SSTabRestored event, there is a really awkward jump as the tab appears and then moves over to be pinned. We still need SSTabRestored so we can send the callback when its completed.

OK. Please add a comment.

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8724419 [details]
MozReview Request: bug 1248460 pin during SSTabRestoring r=kmag

https://reviewboard.mozilla.org/r/37023/#review33747

Looks good. Thanks

Comment 7

[Andy McKay]

Comment on attachment 8724419 [details]
MozReview Request: bug 1248460 pin during SSTabRestoring r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37023/diff/2-3/

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b525f31f69ff

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b525f31f69ff