Top Sites thumbnails don't generate for any HTTPS pages in Metro

RESOLVED FIXED

Status

RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: jimm, Assigned: mbrubeck)

Tracking

Trunk
All
Windows 8.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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
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?
Assignee: nobody → ally
(Assignee)

Comment 2

6 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
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?
(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

6 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

6 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

6 years ago
Created attachment 724663 [details] [diff] [review]
patch
Attachment #724663 - Flags: review?(ally)
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

6 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

6 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.
Does this handle Cache-control: no-store appropriately? See bug 754608 and bug 755996.
https://hg.mozilla.org/mozilla-central/rev/adc0079eeb54
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.