794028 - Work - Use the Firefox Start Top Sites tile group

Status: RESOLVED FIXED

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

Comment 1

[Allison Naaktgeboren :ally]

yoink! 

mbrubeck, do you have mocks, guidelines, plans, or other intel I should know about before giving this a whirl? Does ux have any strong feelings on this topic?

Comment 2

[Matt Brubeck (:mbrubeck)]

Bug 747789 has some mockups.  Yuan or shorlander may have more detailed specs, or can provide them as needed.

Comment 3

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

[Basic Use Cases] - P1 must have for beta
- View top 8 sites in thumbnail (3(row) by 3(column))
- Rearrange the top site thumbnail
- Tap/Click to launch the site
- Get the most updated top site thumbnails based on users' data.
- If there are fewer than 8 top sites, display blank thumbnails. 


[First run users] - P1 must have
- For first run users, probably will have Mozilla page, SUMO page, Metro public announcement page, etc as the top site thumbnails.

[Layout adjustment] - P2 need to have for beta
- When tab panel is shown, top site thumbnails in 3 by 3 should smoothly move to 4 by 2.
- When users are in portrait orientation, thumbnails should be in 2 by 4. Tab panel doesn't affect the layout of top sites.
- When users are in filled view(2/3 screen), the adjustment should be the same with full screen view. 
(But it's worthy looking into whether it's necessary to slightly reduce the size of thumbnails and tiles.)
- When users are in snapped view(1/3 screen),top site thumbnails should changes to small tiles.
Mockup here: http://cl.ly/image/3l2J2D0g3b1Y/o

[Advanced Use Cases] - P3 nice to have
- Slide to select single thumbnail (typical Windows8 gesture for selection) or right-click on single thumbnail should bring up contextual app bar. Repeat the same gesture or right-click again should exit the app bar.
   App bar for individual top site selection should contain 3 commands: "Open this link","Remove","Pin/Unpin on the current position".
- Slide to select multiple thumbnails, will bring up the app bar as well. Reversible interactions should be applied
   App bar for multiple tops site selection should contain commands: "Open these links", "Remove all", "Clear selection"
- If user removes top sites, the sites ranked behind the removed site should move forward, and load new top sites from the end.(the same interactions with new tab, Firefox desktop).

Hope this is clear. Happy to clarify any question you may have.

Comment 4

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

[Basic Use Cases] - P1 must have for beta
- View top 8 sites in thumbnail (3(row) by 3(column))
- Tap/Click to launch the site
- Get the most updated top site thumbnails based on users' data.
- If there are fewer than 8 top sites, display blank thumbnails. 

I took out "- Rearrange the top site thumbnail". Thumbnails should be displayed based on frequency. Users shouldn't have the ability to rearrange them.

Comment 5

[Allison Naaktgeboren :ally]

So if I understand you correctly, translating the use cases into behavioral requirements yields:

start up:
population of grid
  top sites grid will always appear
  determine top 8 site by frecency
  populate screenshot/thumbnails into topsites grid
  if < 8, fill with blank
  users should not be able to rearrange this grid 
layout events: 
if portrait
  3x3 grid
  if tab panel is triggered
    4x2 grid
if landscape
  3x3 grid
  if tab panel is triggered
    2x4 triggered
if snapped view
  replace thumbnails with small tiles
  layout remains the same

events:
tap
  opens site in new tab
click
  opens site in new tab


Questions:
1) if the grid is 3x3 (a total of 9), why only 8 sites? why not 9?
2) what do you mean by tab panel? What event triggers it? Does a user touch something? You appear to be inconsistent about what yo want when the orientation is portrait (before & after said tab panel is shown)
3) "When users are in filled view(2/3 screen), the adjustment should be the same with full screen view." What exactly do you mean by adjustment here? Adjustment of what?
4) should snapped view change layout/behavior if landscape or portrait?
5) do we populate 'topsites' for people on first run? Your first comment suggests that you want it loaded with mozilla pages. I don't think that is consistent with either desktop or mobile?


6) I think a number of the advanced use cases/ nice to haves should be filed as separate enhancement bugs. This one should represent mvp only.

Comment 6

[yuan]

The requirements above look right to me. 

To answer your questions:
1) if the grid is 3x3 (a total of 9), why only 8 sites? why not 9?
I think that is a decision made by Shorlander for aesthetic reason. Since the layout needs to be adaptive, 8 thumbnails are easier to break down into 4x2 or 2x4 grid. If 9 sites, there will be one tile being left out at 4x2 or 2x4 grid. 
 
2) what do you mean by tab panel? What event triggers it? Does a user touch something? You appear to be inconsistent about what yo want when the orientation is portrait (before & after said tab panel is shown)
   Tab panel is area that contains tabs. 
   Single swipe from top or bottom edge or right-click by mouse will trigger URL bar and app bar. If you swipe one more time or right-click one more time, tab panel will open. The same time, app bar slides back. (Edge swipe and right-click are the standard Win8 interactions to bring up controls)

3) "When users are in filled view(2/3 screen), the adjustment should be the same with full screen view." What exactly do you mean by adjustment here? Adjustment of what?
That means in filled view,
 - When tab panel is shown, top site thumbnails in 3 by 3 should smoothly move to 4 by 2.
- When users are in portrait orientation, thumbnails should be in 2 by 4. Tab panel doesn't affect the layout of top sites.
These adjustments on layout is the same with adjustments on full screen view. 
 

4) should snapped view change layout/behavior if landscape or portrait?
Thanks for asking this. Snap view or filled view doesn't apply to portrait orientation. 
- If users rotate snap&filled view screen from landscape to portrait, the app that was in filled view(2/3 screen) will take the whole screen and display itself in full-screen portrait. Rotating the device back to landscape doesn't bring back the app that was in snap view.

5) do we populate 'topsites' for people on first run? Your first comment suggests that you want it loaded with mozilla pages. I don't think that is consistent with either desktop or mobile?
- I will check in with designers from other platforms to keep consistent. Will keep u posted. 

6) I think a number of the advanced use cases/ nice to haves should be filed as separate enhancement bugs. This one should represent mvp only. 
That sounds good to me.

Comment 7

[Allison Naaktgeboren :ally]

as from today's ux-metro meeting, examine pinning for topsites. May require a separate bug.

Comment 8

[Allison Naaktgeboren :ally]

hey yuan, did you ever have a chance to check in with the other designers about populating topsites on first run?

Comment 9

[Allison Naaktgeboren :ally]

After discussing/demoing the current wip with yuan, I am re-organizing this a bit.

1) This bug is going to include: topsites not appearing on startup (there is no history and none of the other fx prepopulate.) Once the user has a populated profile, Topsites as thumbnails will appear in an 3x3 grid. 

2) Pinning will be a followup feature (bug 808770)

3) The filled view is not going to change from normal view

4) The snapped view will be a totally different awesomescreen(bug 801000) , and it may use favicons for topsites instead of thumnails to conserve space. Handling of topsites in the snapped view will be part of that bug

Comment 10

[Allison Naaktgeboren :ally]

Attachment: wip topsits patch, part 1/2

exposes topsites (up to 9) on startui after the first run (hides based on "browser.firstrun.show.localepicker" pref, based on what I saw in b2g & android code. If I've got the wrong pref, let me know).

- Since the thumbnail code is dependent on a port into toolkit(which may be time consuming), this uses favicons. (So we can get a functioning, if not super polished, form of the feature out)

- Tim, is this worth writing a test for? Do we have a functioning framework to test that? I am not sure

Comment 11

[Matt Brubeck (:mbrubeck)]

Comment on attachment 681685 [details] [diff] [review]
wip topsits patch, part 1/2

Review of attachment 681685 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/content/bindings/grid.xml
@@ +216,5 @@
>        <property name="columnCount" readonly="true" onget="return this._columnCount;"/>
>  
>        <method name="arrangeItems">
> +        <parameter name="numRows"/>
> +        <parameter name="numColumns"/>

Nit: Please use "aNumRows" and "aNumColumns".

@@ +245,5 @@
>   
> +	    if (numRows) {
> +	      this._rowCount = numRows;
> +            }
> +            else {

Our totally undocumented Fennec style guide says to put "} else {" on one line.  (We can change this if there is popular demand, but we will have to tie up mfinkle in the basement first.)

::: browser/metro/base/content/history.js
@@ +3,4 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +'use strict';

\o/

Comment 12

[Allison Naaktgeboren :ally]

Part 1:
https://hg.mozilla.org/projects/elm/rev/4dbdec9d6424

Comment 13

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 681685 [details] [diff] [review]
wip topsits patch, part 1/2

(In reply to :Ally Naaktgeboren from comment #10)
> [...]
>
> - Tim, is this worth writing a test for? Do we have a functioning framework
> to test that? I am not sure

This functionality seems like it will be manually tested every time someone opens the modern browser, so it's probably not a big deal to add a test for it.

On the other hand, it's always nice to have at least one test already written so that down the road when bugs crop up we can just edit a js file to add more tests.  If you're interested in adding a test, check out the files in browser/metro/base/tests - those are all the browser chrome tests we have for metro.  To run the tests, "pymake -C objdir mochitest-metro-chrome" then look in objdir for a log file with the results.  I've got a patch on the back burner (on hold while I work on some widget events) that might add some helper functions for use by our mochitests, but you probably don't need them.

Comment 14

[Asa Dotzler [:asa]]

Attachment: UC-50 Use the Awesome screen Top Sites tile group

Comment 15

[Allison Naaktgeboren :ally]

Attachment: wip - images as background of the tiles

-icon disappears, name appears in light txt, and the watermark appears as the background image (its faint, but there).

Comment 16

[Allison Naaktgeboren :ally]

Attachment: wip - sometimes, thumbnails on pages

So, this *sort* of works, but in a buggy way that suggests that I may be going about this the wrong way or missing a case. I'm hoping for some feedback from the other front-end folks.

functionally:
- Sometimes loaduri() does not fire when I load a page in the browser. Is this expected? (seems to cause some pages not to get a screenshot atm)
- For others, there is a screenshot in residence, but the link returned from PageThumbs is incorrect/DNE (this suggests that I may be consuming it incorrectly)

eyesore:
- captured pagethumbs are probably too large. need to be shrunk
- tiles are tiny now that the icon box is collapased

flatout not covered in user story, but should before we ship:
- when to skip taking screenshots (see shouldcapture() in browser-thumbnails)

Comment 17

[Matt Brubeck (:mbrubeck)]

(In reply to :Ally Naaktgeboren from comment #16)
> - Sometimes loaduri() does not fire when I load a page in the browser. Is
> this expected? (seems to cause some pages not to get a screenshot atm)

There are a variety of ways to load a page in the browser, only some of which call Browser.loadURI.  For example, clicking a link or using back/foward navigation does not go through our frontend code.

To catch all of these, you can use messageManager.addMessageListener to listen for a "Content:LocationChange" message from the browser (which fires when a new URL is about to load) or a "pageshow" message (which fires when the page has actually loaded -- this is probably what you want).  

I haven't looked into the other issues from this comment yet.

Comment 18

[Allison Naaktgeboren :ally]

Attachment: wip - thumbnails show up when expected, but poorly sized

- looking functionally correct now. (thanks mbrubeck)

- wrong size (tiles are too small & images are too large)
- missing shouldcapture logic (sometimes, we dont actually want to take a screenshot)
- does not update dynamically (more interesting given that we need a shut down to get images to show up. it seems. This is ok on desktop, this might not be ok in the metro world)

Comment 19

[Allison Naaktgeboren :ally]

Attachment: wip core -functional

- has hardcoded tile dimensions (don't like that)
- has hardcoded color for tile (dont like that in all cases either)
-- not covered in user story, but with skipped capture this can be ugly
- does not pick up new thumbs until restart (really dont like that)

Comment 20

[Allison Naaktgeboren :ally]

Attachment: fully functional, with updates, but horrible, horrible hack (and probably a perf hit)

ok, so it all works, but has a horrible hack that seems really bad for our performance. I feel I need less fugly css if possible, and updateTileHack() should only update the changed tile

I think we should also get this reviewed by someone who knows the desktop thumbnail. I cant say whether or not we need the expiration filter logic with certainty or whether that could wait for v2

Comment 21

[Matt Brubeck (:mbrubeck)]

Comment on attachment 719232 [details] [diff] [review]
fully functional, with updates, but horrible, horrible hack (and probably a perf hit)

Review of attachment 719232 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!  Various suggestions/requests below.

::: browser/metro/base/content/TopSites.js
@@ +170,5 @@
> +    for(let i=0; i<9;++i) {
> +      let item = this._set.getItemAtIndex(i);
> +      let bkgrd = item.backgroundImage;
> +      Cu.reportError("hackloop:"+i+" bkgrd = "+ bkgrd + " getcomputedsyle = " +window.getComputedStyle(item._box, "").backgroundImage);
> +      item._box.style.removeProperty("background-image");

Rather than do this for all 9 tiles every time, let's pass in the URI of the page we just captured, and update only when we find a tile for that specific page.

To avoid the implementation leakage mentioned in your comment, we could move the code from this loop into a "refreshBackground" method in the richgriditem binding.

::: browser/metro/base/content/bindings/grid.xml
@@ +536,5 @@
> +            if (this.backgroundImage != undefined) {
> +              this._box.parentNode.setAttribute("customImagePresent", "true");
> +              this._box.style.backgroundImage = this.backgroundImage;
> +            } else {
> +              this._box.parentNode.removeAttribute("customImagePresent");

Perhaps we should also unset this._box.style.backgroundImage.

::: browser/metro/base/content/browser-ui.js
@@ +787,5 @@
> +      case "pageshow":
> +        // anaaktge
> +        let currBrowser = Browser.selectedBrowser;
> +        if( this._shouldCapture(currBrowser)) {
> +          PageThumbs.captureAndStore(currBrowser);

I know I'm the one who suggested "pageshow", but now that I look at the desktop implementation, we might want to duplicate more of their logic.  Desktop's browser-thumbnails.js listens for a "network stop" notification and then captures the screenshot after a 1-second delay.

In the Metro world, you can do this by listening for the "Content:StateChange" message; see /browser/metro/base/content/WebProgress.js for sample code.

@@ +792,5 @@
> +          Cu.reportError("captured. should be image in directory");
> +        }
> +        // for the love of all that is good, don't let this ship as-is
> +        let topsitesobj = window["TopSitesStartView"];
> +        topsitesobj._view.updateTilesHack();

We can replace the direct _view call with a Server.obs notification to help make this less hacky.

@@ +801,5 @@
>    },
>  
> +  // Private Browsing is not supported on metro at this time, when it is added
> +  //  this function must be updated to skip capturing those pages
> +  _shouldCapture: function _shouldCapture(aBrowser) {

Naming nit: Can we call this _shouldCaptureThumbnail?  Alternately we could move both it and the listener out of BrowserUI and into a new object ("BrowserThumbnails"?).

@@ +807,5 @@
> +    if (aBrowser != Browser.selectedBrowser) {
> +
> +      Cu.reportError("not capturing - not selected tab");
> +      return false;
> +    }

I see this logic came from desktop Firefox but I'm curious *why* they don't capture background tabs...

@@ +870,5 @@
> +        return false;
> +      }
> +
> +      // Don't capture HTTPS pages unless the user explicitly enabled it.
> +      if (uri.schemeIs("https") && !this._sslDiskCacheEnabled) {

Looks like we'll need to define _sslDiskCacheEnabled somewhere.

::: browser/metro/theme/platform.css
@@ +344,2 @@
>    background: #fff;
>  }

I think just ".richgrid-item-content { background: #fff; }" should be enough here, since we override the background style when we set any custom image or color.

@@ +364,1 @@
>    color: #f1f1f1;

While we're modifying this, could you change it to avoid the descendant selector (for performance reasons)?  I think you can just remove " description" from the selectors, and set "color" directly on the richgriditem instead; the description will inherit it.

@@ +370,5 @@
>  }
>  
> +richgriditem[customImagePresent="true"] .richgrid-item-content {
> +  height: 120px; /*anaaktge - there should be something better than hardcode*/
> +  width: 200px;

Here we can change the descendant selector to a child selector:

  richgriditem[customImagePresent] > .richgrid-item-content

Having a default height/width hardcoded here is fine with me; it won't prevent us from changing these styles elsewhere if we need to.

@@ +380,5 @@
> +
> +/* hide icon if there is an image background */
> +richgriditem[customImagePresent="true"] .richgrid-icon-container {
> +  visibility: collapse;
> +}

We could also avoid this descendant selector by using inherits="customImagePresent" on the .richgrid-icon-container element in the binding.  However, there are much worse selector problems elsewhere in these richgrid styles; we could file a separate bug to deal with them all in one swoop if you prefer.

Comment 22

[Allison Naaktgeboren :ally]

Attachment: draft 0 part 2/2

I tried to leave comments in the code in places where the behavior is odd, ie we are missing a pref for the user override for ssl (and I'm going to say that until Asa has a pressing user case for it, I'm not too worried about it), and whether the layout engine does not respond as expected to refreshing the image in xbl.

Afaik, the reason desktop only does the current tab is they no longer load the background tab the user foregrounds it (probably because of users like me, who have >100 tabs between their devices :) ). I'm inclined to follow their lead on that for now unless there is a compelling reason not to?

Comment 23

[Matt Brubeck (:mbrubeck)]

Comment on attachment 720915 [details] [diff] [review]
draft 0 part 2/2

Review of attachment 720915 [details] [diff] [review]:
-----------------------------------------------------------------

Great!  Aside from a few style nits, I'd like to take one more crack at forceReloadOfThumbnail before landing, and also get some UX feedback on a text color problem.  Details below.

::: browser/metro/base/content/TopSites.js
@@ +31,5 @@
>      return true; // operation was successful
>    }
>  };
>  
> +function TopSitesView(aGrid, maxSites, useThumbnails) {

Style nit: aMaxSites, aUseThumbnails

(I know the former is already there -- I must have missed it before, sorry.)

@@ +43,5 @@
>  
>    let history = Cc["@mozilla.org/browser/nav-history-service;1"].
>                  getService(Ci.nsINavHistoryService);
>    history.addObserver(this, false);
> +  Services.obs.addObserver(this, "Metro:RefreshTopsiteThumbnail", false);

Please put "if (this._useThumbs)" around this (and the matching removeObserver).

@@ +161,5 @@
>    },
>  
> +  forceReloadOfThumbnail: function forceReloadOfThumbnail(url) {
> +      let nodes = this._set.querySelectorAll('richgriditem[value="'+url+'"]');
> +      let item = nodes[0];

Rather than take the first match, let's do "for (let item of nodes)", in case there are zero matches, or multiple matches.

(Or if we do know there will be exactly one match, we can use querySelector() here.)

@@ +167,5 @@
> +      // _box is leaking richgrid implmentation, which is bad. However,
> +      //  for unknown reason(s), this code in xbl does not induce the
> +      //  layout engine to reload the page with the new and/or now valid image
> +      item._box.style.removeProperty("background-image");
> +      item._box.style.setProperty("background-image", bkgrd);

Moving this to a richgrid-item method like this seemed to work for me, though I haven't tested it extensively:

      <method name="refreshBackgroundImage">
        <body><![CDATA[
          if (this.backgroundImage) {
            this._box.style.removeProperty("background-image");
            this._box.style.setProperty("background-image", this.backgroundImage);
          }
        ]]></body>
      </method>

If there's some reason we can't get this to work, putting the code here is okay -- but let's see if we can work through the issues first.

::: browser/metro/base/content/browser-ui.js
@@ +786,5 @@
>          this.goToURI(json.uri);
>          break;
> +      case "Content:StateChange":
> +        let currBrowser = Browser.selectedBrowser;
> +        if( this.shouldCaptureThumbnails(currBrowser)) {

whitespace nit: misplaced space after "if"

@@ +789,5 @@
> +        let currBrowser = Browser.selectedBrowser;
> +        if( this.shouldCaptureThumbnails(currBrowser)) {
> +          PageThumbs.captureAndStore(currBrowser);
> +          let currPage = currBrowser.currentURI.spec;
> +          Services.obs.notifyObservers(null, "Metro:RefreshTopsiteThumbnail", currPage);

After the StateChange notification, desktop Firefox waits for 1 second before capturing the thumbnail.  I haven't looked into exactly why they do this, but I'm guessing it's to avoid / spread out any jank when the page is just loading.  Can we do the same here?

::: browser/metro/theme/platform.css
@@ +365,2 @@
>    color: #f1f1f1;
>  }

This color is unreadable on tiles where we show the default background because we can't actually load a thumbnail (because it's not captured yet, or because SSL or cache-control prevents it), and also on thumbnails of sites with light backgrounds.  This is bad enough that I think it will interfere with dogfooding and testing.

Stephen, do we have a design spec that addresses this already?  If not, can we give this text a background or a text-shadow or something, at least until until bug 826556 is fixed?

Ally, if we don't have a final design before this lands, let's deal with it in a follow-up bug and just land with a temporary solution, like:

.richgrid-item-content[customImagePresent] > .richgrid-item-desc {
  background: rgba(0, 0, 0, 0.4);
}

@@ +369,5 @@
>    opacity: 0.8;
>    background-color: #fff;
>  }
>  
> +.richgrid-item-content[customImagePresent="true"] {

Minor style suggestion:  I prefer [attribute] instead of [attribute="true"] when the attribute has only one possible value -- mostly just to make the selectors more concise.

Since the existing code here uses [attribute="true"], you can either stick with that style, or update both the old and new code to use [attribute] if you feel like it.

Comment 24

[Matt Brubeck (:mbrubeck)]

Stephen, can we get a design decision for how to display text so it will be readable on thumbnails with both light and dark backgrounds?  (See comment 23 for more details.)

Comment 25

[Allison Naaktgeboren :ally]

Attachment: screenshot

the white text currently used for bookmarks (custom color tiles) and topsites. sometimes this is ok (see pandora tile) other times not (4 of the tiles are https/loading/ etc and are not captured, so remain white squares) (others are predominantly white pages, see corporette)

Comment 26

[Stephen Horlander [:shorlander]]

(In reply to Matt Brubeck (:mbrubeck) from comment #23)
> Stephen, do we have a design spec that addresses this already?  If not, can
> we give this text a background or a text-shadow or something, at least until
> until bug 826556 is fixed?

Sorry, I guess the original mockup never got linked here: https://bug747789.bugzilla.mozilla.org/attachment.cgi?id=617342

The title should be in a bar at the bottom. Originally the spec called for pulling the thumbnail dominant color, not sure if we can do that yet? Also it will probably have to wait on bug 826556.

In the meantime we can just do something like:

.tileTitleOrSomeClass {
  background-color: hsl(0,2%,98%);
  color: #1a1a1a
}

Comment 27

[Stephen Horlander [:shorlander]]

Oops meant:

.tileTitleOrSomeClass {
  background-color: hsla(0,2%,98%,.95);
  color: #1a1a1a
}

Comment 28

[Allison Naaktgeboren :ally]

Attachment: closer to success (missing expiration filters, ux-quick fix)

1)Desktop waiting one second to snapshot - according to the owner, this is just a not-very-smart heuristic to try and not distrub the page while loading. He isn't thrilled with it, so I'm inclined to not copy this code until there is a compelling reason (like a perf hit).

2)Expiration Filters - I had noticed that, but wasn't sure if they were needed (its not covered by my user story). After talking with Tim it sounds like we do.

3)Text coloring - Yea, this wasn't really a case covered in the original spec I had (which seems to assume that your topsites are safe to snapshot all the time and did not have the bars at bottom). I'll do the easy change for now, and it looks like followup is going to be needed in general.

4)Css style nit - I am conflicted because I agree with you, but it has been beaten into my head that consistency of the file trumps all and I am concerned about scope bloat. So, I'll compromise: we now have bug 848134 and it already has a big patch on it. Seems we had this issue in a bunch of places. Everyone wins?

Comment 29

[Allison Naaktgeboren :ally]

Attachment: draft 2, part 2/2 post spec change

I know a prior version has an r+, but the desired behavior has been modified and the functionality is sufficiently different that I'd like a quick once-over on the new version.

Comment 30

[Matt Brubeck (:mbrubeck)]

Comment on attachment 721898 [details] [diff] [review]
draft 2, part 2/2 post spec change

Review of attachment 721898 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good except for the missing expiration filter code (see below).

::: browser/metro/base/content/TopSites.js
@@ +46,5 @@
>    let history = Cc["@mozilla.org/browser/nav-history-service;1"].
>                  getService(Ci.nsINavHistoryService);
>    history.addObserver(this, false);
> +  if (this._useThumbs) {
> +    PageThumbs.addExpirationFilter(this);

I don't see a filterForThumbnailExpiration method in TopSitesView.prototype.  Is part of the patch missing?

::: browser/metro/theme/platform.css
@@ +391,1 @@
>  richgriditem description {

This can change to .richgrid-item-desc.

@@ +397,5 @@
>  
> +.richgrid-item-content[customImagePresent] > .richgrid-item-desc {
> +  background: hsla(0,2%,98%,.95);
> +  margin-bottom: 0px;
> +  margin-right: 0px;

Minor style nit: Is it possible to just use "margin: 0;" here?

Comment 31

[Allison Naaktgeboren :ally]

Attachment: draft 3, part 2/2

Sorry about the missing bit. Must train myself to qref more aggressively :)

I'm not sure solid on the ExpirationFilter function. I tried to test it as best I could.

Comment 32

[Matt Brubeck (:mbrubeck)]

Comment on attachment 722495 [details] [diff] [review]
draft 3, part 2/2

>+  filterForThumbnailExpiration: function filterForThumbnailExpiration(aCallback) {
>+    aCallback([browser.currentURI.spec for (browser of Browser.browsers)]);
>+  },

I think we want to pass back a list of the top sites, rather than a list of current tabs.  That way, items currently in the grid will not expire, even if they are not currently loaded in a tab.  For now we can loop over the current tiles:

  [item.getAttribute("value") for (item of this._set.children)]

And after bug 808770 lands, it looks like NewTabUtils will add its own expiration filter that keeps "top sites" from expiring:
http://hg.mozilla.org/mozilla-central/file/71395a927025/toolkit/modules/NewTabUtils.jsm#l770

...so maybe we don't need to add one here after all (or we can remove it when bug 808770 lands -- but it looks like both of these will land very soon).

So... r=mbrubeck either with the alternate filter above or with no addExpirationFilter at all.

Comment 33

[Allison Naaktgeboren :ally]

Attachment: tweaked reviewed draft3 part 2/2: while I wait for commit access to m-i back!

even well intentioned scripts have bugs

Comment 34

[Richard Newman [:rnewman]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/a1586c3b0da8

Comment 35

[Ryan VanderMeulen [:RyanVM]]

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