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

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Firefox Start, defect)

Description

[Jim Mathies [:jimm]]

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

[Allison Naaktgeboren :ally]

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?

Comment 2

[Matt Brubeck (:mbrubeck)]

The plan to fix this for desktop Firefox is bug 841495.  We should make sure the same code can be used in Metro.

Comment 3

[Dão Gottwald [:dao]]

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

[Allison Naaktgeboren :ally]

(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.

Comment 5

[Matt Brubeck (:mbrubeck)]

(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.

Comment 6

[Matt Brubeck (:mbrubeck)]

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.

Comment 7

[Matt Brubeck (:mbrubeck)]

Attachment: patch

Comment 8

[Allison Naaktgeboren :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;
}

Comment 9

[Matt Brubeck (:mbrubeck)]

(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

Comment 10

[Matt Brubeck (:mbrubeck)]

(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

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Does this handle Cache-control: no-store appropriately? See bug 754608 and bug 755996.

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/adc0079eeb54