Closed Bug 1228980 Opened 4 years ago Closed 4 years ago

Session Restore doesn't display favicons

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: quicksaver, Assigned: quicksaver)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

I noticed any Session Restore-based page doesn't show icons next to the restored tabs, it only shows the generic (globe?) tab icon.

aboutSessionRestore.js gets favicons from .attributes.image [1]. But TabState.jsm sets them directly in the tab's .image [2]. Which one is wrong?

(Changing aboutSessionRestore.js to fetch the icon from .image does make it show the icons.)

[1] http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/content/aboutSessionRestore.js#80
[2] http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/TabState.jsm#187
Gijs, I think it is worth fixing this now, for improved UX when removing Tab Groups, as the migration page in bug 1221050 is affected by this (check your own screenshot of that page in that bug). Do you agree?
Flags: needinfo?(gijskruitbosch+bugs)
Maybe, but I can't answer the question in comment #0, so forwarding to Tim.

I also don't think this blocks landing bug 1221050 on Nightly.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(ttaubert)
(In reply to Luís Miguel [:quicksaver] from comment #0)
> I noticed any Session Restore-based page doesn't show icons next to the
> restored tabs, it only shows the generic (globe?) tab icon.
> 
> aboutSessionRestore.js gets favicons from .attributes.image [1]. But
> TabState.jsm sets them directly in the tab's .image [2]. Which one is wrong?
> 
> (Changing aboutSessionRestore.js to fetch the icon from .image does make it
> show the icons.)

Looks like the second one is right.
Flags: needinfo?(ttaubert)
Bug 1228980 - Display favicons of tab entries in aboutSessionRestore-based pages. r?ttaubert
Attachment #8696663 - Flags: review?(ttaubert)
I was in the neighborhood, so... Tim, I assume you're the man to review this? :)

Bug 866444 was fixed over two and a half years ago, so I thought it was unnecessary to keep compatibility with .attributes.image (I doubt there's outdated session data that old; even if there is, extreme edge cases?)
Assignee: nobody → quicksaver
Status: NEW → ASSIGNED
Comment on attachment 8696663 [details]
MozReview Request: Bug 1228980 - Display favicons of tab entries in aboutSessionRestore-based pages. r?ttaubert

https://reviewboard.mozilla.org/r/27381/#review24751
Attachment #8696663 - Flags: review?(ttaubert) → review+
Any suggestions for a try run? I'm not sure what this could possibly affect. I was thinking of running only mochitests on linux.
Why not run m-bc (+e10s) on all platforms?
No reason really, just still learning where to draw the line between saving resources and err'ing on the safe side.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ec8d7d2f947
(In reply to Luís Miguel [:quicksaver] from comment #9)
> No reason really, just still learning where to draw the line between saving
> resources and err'ing on the safe side.

Good thing to think about, for sure. Running all mochitests seems fine here, figuring out which tests run in which m-bc-e10s suite isn't worth the time spent doing that. I personally would probably rather run a few more tests on try than wasting my development time relanding and fixing a patch that's been backed out.
Try looks good.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5f2d518835e2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
You need to log in before you can comment on or make changes to this bug.