625329 - Pinned tabs are used when setting homepage from current tabs

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Tolga Hosgor]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8) Gecko/20100101 Firefox/4.0b8
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8) Gecko/20100101 Firefox/4.0b8

When you click "use opened tabs" for settings homepage in preferences menu, it counts application tab as normal tab and adds it too but when you restart the browser it opens 3 tabs, one app tab, one tab with same url as app tab, one tab with actual homepage.

Reproducible: Always

Steps to Reproduce:
1.make an app tab and normal tab
2.go pref. menu and click use current tabs to set homepage
3.save and restart
Actual Results:  
Application tab left acting the same way it does, I got two new tabs on startup.

Expected Results:  
Application tab should be a constant homepage-like URL on startup instead of the page I was at before closing Firefox.

Comment 1

[Luke Iliffe (Harlequin99)]

Confirmed on trunk.

Comment 2

[Marco Bonardo [:mak]]

setHomePageToCurrent does not seem to be aware of tabs and visibleTabs and so on, so it's also probably misbehaving with Panorama.
Probably it should use visibleTabs and ignore apptabs, adding uiwanted to define this.

Comment 3

[Maniac Vlad Florin (:vladmaniac)]

Confirm this on 

Mozilla/5.0 (Windows NT 6.1; rv:2.0b10pre) Gecko/20110118 Firefox/4.0b10pre

Comment 4

[Maniac Vlad Florin (:vladmaniac)]

This is happening so far as Fx4b6 candidate builds | all platforms

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Bug 636149 made it so only visible tabs are used, but pinned tabs are still included.

Comment 6

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 552529 [details] [diff] [review]
Patch v0.1

>diff --git a/browser/components/preferences/main.js b/browser/components/preferences/main.js

>+    let _this = this;
>+    window.addEventListener("focus", function() _this._updateUseCurrentButton(), false);

Use .bind()?

>+  _getTabsForHomePage: function ()

>-    if (win && win.document.documentElement
>-                  .getAttribute("windowtype") == "navigator:browser") {

Isn't this check still needed to catch the cases where .opener is not a browser window? The previous logic didn't needed it for the command case since the button was disabled then.

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.2

Addressed comments.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 575227 [details] [diff] [review]
Patch v0.2

Can't lost the window.opener null check in _getTabsForHomePage, since that isn't guaranteed to be non-null.

Comment 10

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.3

Ah, I was being a bit naive there and assumed the only situation left was where the pref window is opened from another window.

Comment 11

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

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

Comment 12

[Rob Campbell [:rc] (:robcee)]

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