Closed
Bug 849819
Opened 11 years ago
Closed 11 years ago
Top Sites thumbnails don't generate for any HTTPS pages in Metro
Categories
(Firefox for Metro Graveyard :: Firefox Start, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jimm, Assigned: mbrubeck)
References
Details
Attachments
(1 file)
2.74 KB,
patch
|
ally
:
review+
|
Details | Diff | Splinter Review |
I'm currently not getting thumbnails for some sites. Wondering how I can debug this. It looks like bugzilla bugs are the most common. str: 1) open start 2) find a site in Top Sites that doesn't have a thumbnail 3) open the site, let it fully render 4) close the tab 5) open start result: thumbnail still not rendered
Comment 1•11 years ago
|
||
A lot of pages are not captured for various reasons http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser-ui.js#828 bugzilla uses the store control: no cache setting, v.mozilla uses https, both of which are 'do not capture' reasons. What other sites have you experienced this with?
Updated•11 years ago
|
Assignee: nobody → ally
Assignee | ||
Comment 2•11 years ago
|
||
The plan to fix this for desktop Firefox is bug 841495. We should make sure the same code can be used in Metro.
Depends on: 841495
Comment 3•11 years ago
|
||
Desktop Firefox excludes https only if browser.cache.disk_cache_ssl is set to false. It's true by default. Any reason why you dropped that condition?
Comment 4•11 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #3) > Desktop Firefox excludes https only if browser.cache.disk_cache_ssl is set > to false. It's true by default. Any reason why you dropped that condition? browser.cache.disk_cache_ssl appears not to exist on metro. Afaik it did not exist on old fennec and it has not been added since we forked.
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to :Ally Naaktgeboren from comment #4) > browser.cache.disk_cache_ssl appears not to exist on metro. Afaik it did not > exist on old fennec and it has not been added since we forked. browser.cache.disk_cache_ssl defaults to true for all Gecko applications, including Metro Firefox.
Assignee | ||
Comment 6•11 years ago
|
||
Stealing this bug and morphing it just to handle the HTTPS case. I'll file a separate bug to use background thumbnailing (bug 841495) once it is available.
Assignee: ally → mbrubeck
Status: NEW → ASSIGNED
No longer depends on: 841495
Hardware: x86_64 → All
Summary: thumbnails don't generate for some sites → Top Sites thumbnails don't generate for any HTTPS pages in Metro
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #724663 -
Flags: review?(ally)
Comment 8•11 years ago
|
||
Comment on attachment 724663 [details] [diff] [review] patch Review of attachment 724663 [details] [diff] [review]: ----------------------------------------------------------------- thank you. r+ with a couple nits ::: browser/metro/base/content/browser-ui.js @@ +93,5 @@ > window.addEventListener("MozPrecisePointer", this, true); > window.addEventListener("MozImprecisePointer", this, true); > > Services.prefs.addObserver("browser.tabs.tabsOnly", this, false); > + Services.prefs.addObserver("browser.cache.disk_cache_ssl", this, false); no removeObserver call? @@ +533,4 @@ > this._updateTabsOnly(); > + } else if (aData == "browser.cache.disk_cache_ssl") { > + this._sslDiskCacheEnabled = Services.prefs.getBoolPref(this.PREF_DISK_CACHE_SSL); > + } This is legal in terms of style, but I find it hard to read. Might I suggest replacing the aData comparisons with a switch instead? case "nsPref:changed": switch(aData) { case: "browser.tabs.tabsonly": .... break; case: "browser.cache.disk_cache_ssl": .... break; }
Attachment #724663 -
Flags: review?(ally) → review+
Assignee | ||
Comment 9•11 years ago
|
||
(In reply to :Ally Naaktgeboren from comment #8) > no removeObserver call? Since BrowserUI lives until shutdown, I prefer to let the observer clean up the observers automatically rather than adding code to do it manually. > This is legal in terms of style, but I find it hard to read. Might I suggest > replacing the aData comparisons with a switch instead? Fixed. https://hg.mozilla.org/integration/mozilla-inbound/rev/adc0079eeb54
Assignee | ||
Comment 10•11 years ago
|
||
(In reply to Matt Brubeck (:mbrubeck) from comment #9) > Since BrowserUI lives until shutdown, I prefer to let the observer clean up > the observers... I meant, I prefer to let the observer *service* clean up the observers.
Comment 11•11 years ago
|
||
Does this handle Cache-control: no-store appropriately? See bug 754608 and bug 755996.
Comment 12•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/adc0079eeb54
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
OS: Windows 8 Metro → Windows 8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•