Closed Bug 980231 Opened 6 years ago Closed 5 years ago

MRU tab order is broken in some cases

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 36

People

(Reporter: tabutils+bugzilla, Assigned: tabutils+bugzilla)

References

Details

Attachments

(1 file, 2 obsolete files)

Blocks: 445461, 747338
Attached patch bug980231.patch (obsolete) — Splinter Review
Attachment #8386766 - Flags: review?(dao)
Attached patch bug980231.patch2 (obsolete) — Splinter Review
Attachment #8386767 - Flags: review?(dao)
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.
Attachment #8386766 - Flags: review?(dao) → review-
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.
Attachment #8386767 - Flags: review?(dao) → review-
(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?
Flags: needinfo?(dao)
(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.
Attached patch Patch v2Splinter Review
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)
Flags: needinfo?(dao)
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
Attachment #8388572 - Flags: review+
Attachment #8386766 - Attachment is obsolete: true
Attachment #8386767 - Attachment is obsolete: true
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
Assignee: nobody → tabutils+bugzilla
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/e3230aab66da
https://hg.mozilla.org/mozilla-central/rev/fd58f9be4a4d
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Depends on: 1105579
Depends on: 1292049
You need to log in before you can comment on or make changes to this bug.