Bug 445461 comment 11 Bug 747338 comment 11

Thanks for filing this bug and writing patches! The bug itself and the patches could really use some descriptions of failure cases and how you are fixing those. That makes reviewing changes much easier.

Comment on attachment 8386766 [details] [diff] [review] bug980231.patch Review of attachment 8386766 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/tabbrowser.xml @@ +1552,5 @@ > notificationbox.id = uniqueId; > t.linkedPanel = uniqueId; > t.linkedBrowser = b; > t._tPos = position; > + t.lastAccessed = Date.now(); tab.lastAccessed is already initialized with zero. Just adding a tab doesn't mean it was accessed.

Comment on attachment 8386767 [details] [diff] [review] bug980231.patch2 Review of attachment 8386767 [details] [diff] [review]: ----------------------------------------------------------------- It would be great if you could elaborate *what* you are fixing here and *why* you are fixing it this way. ::: browser/base/content/tabbrowser.xml @@ +1048,5 @@ > } > > if (!this._previewMode) { > + oldTab.lastAccessed = Date.now(); > + this.mCurrentTab.lastAccessed = Infinity; Why would we want to return Infinity for the selected tab? @@ -4716,5 @@ > > - <property name="lastAccessed"> > - <getter> > - return this.selected ? Date.now() : this._lastAccessed; > - </getter> Removing this means that for selected tabs we wouldn't return the current time.

(In reply to Tim Taubert [:ttaubert] from comment #4) > tab.lastAccessed is already initialized with zero. Just adding a tab doesn't > mean it was accessed. Bug 445461 comment 11: Currently, the lastAccessed stamp is not set on a never accessed new background tab, which will have lastAccessed = 0 stored. This does not match how ctrlTab handles a new tab. Ctrl-Tab should be able to switch to the newly added tab immediately. See: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-tabPreviews.js#482

I don't know much about Ctrl-Tab and its desired behavior. Dão, can you please take a look at the issue raised in comment #6?

(In reply to Tim Taubert [:ttaubert] from comment #5) > > - <property name="lastAccessed"> > > - <getter> > > - return this.selected ? Date.now() : this._lastAccessed; > > - </getter> > > Removing this means that for selected tabs we wouldn't return the current > time. Bug 747338 comment 11: Seems problematic in preview mode. We may set the time stamp on selected tab to Infinity. Return Date.now() for "selected" tab is wrong in preview mode. The "selected" state could be temporarily set by ctrlTab.

(In reply to ithinc from comment #8) > Return Date.now() for "selected" tab is wrong in preview mode. The > "selected" state could be temporarily set by ctrlTab. That could be fixed quite easily by doing something like: <getter> return this.selected && !this.parentNode.tabContainer.tabbrowser._previewMode ? Date.now() : this._lastAccessed; </getter>

By this way, you'll get wrong value for the truly selected tab. You even don't know which one is the truly selected tab in preview mode.

Setting lastAccessed to Infinity is in fact identifying the tab as the truly selected one. If you really desire a Date.now(), Infinity could be converted to Date.now() in the getter. In my opinion, Infinity could serve the same purpose as Date.now() here.

I find a 3rd issue. The tab.lastAccessed field will be re-initialized to 0 after a tab is moved.

The patch is to address 3 issues. 1/ tab.lastAccessed is re-initialized to 0 after one tab is moved 2/ tab.lastAccessed is not set for a new tab 3/ tab.lastAccessed may return wrong value in preview mode

(dashboard cleanup)

Comment on attachment 8388572 [details] [diff] [review] Patch v2 > if (listener && listener.mStateFlags) { > this._callProgressListeners(null, "onUpdateCurrentBrowser", > [listener.mStateFlags, listener.mStatus, > listener.mMessage, listener.mTotalProgress], > true, false); > } > > if (!this._previewMode) { >+ oldTab._lastAccessed = Date.now(); >+ this.mCurrentTab._lastAccessed = Infinity; > this.mCurrentTab.removeAttribute("unread"); >- oldTab.lastAccessed = Date.now(); Use lastAccessed rather than _lastAccessed, and don't unnecessarily move the oldTab line. > notificationbox.appendChild(browserSidebarContainer); > > var position = this.tabs.length - 1; > var uniqueId = this._generateUniquePanelID(); > notificationbox.id = uniqueId; > t.linkedPanel = uniqueId; > t.linkedBrowser = b; > t._tPos = position; >+ t._lastAccessed = Date.now(); s/_lastAccessed/lastAccessed/ > document.addEventListener("keypress", this, false); > window.addEventListener("sizemodechange", this, false); > > var uniqueId = this._generateUniquePanelID(); > this.mPanelContainer.childNodes[0].id = uniqueId; > this.mCurrentTab.linkedPanel = uniqueId; > this.mCurrentTab._tPos = 0; > this.mCurrentTab._fullyOpen = true; >+ this.mCurrentTab._lastAccessed = Infinity; s/_lastAccessed/lastAccessed/ > <setter> >- this._lastAccessed = val; >+ if (this._lastAccessed != Infinity) >+ this._lastAccessed = val; >+ return val; > </setter> This restriction doesn't seem useful, so please drop this change. r=me with these things addressed

Oh, and browser/base/content/test/general/browser_lastAccessedTab.js will need to be updated.

Addressed my points from comment 15: https://hg.mozilla.org/integration/fx-team/rev/e3230aab66da Updated browser_lastAccessedTab.js: https://hg.mozilla.org/integration/fx-team/rev/fd58f9be4a4d

https://hg.mozilla.org/mozilla-central/rev/e3230aab66da https://hg.mozilla.org/mozilla-central/rev/fd58f9be4a4d

(In reply to Dão Gottwald [:dao] from comment #17) > Updated browser_lastAccessedTab.js: > https://hg.mozilla.org/integration/fx-team/rev/fd58f9be4a4d and: https://hg.mozilla.org/integration/fx-team/rev/31b92e349f6a