808770 - Work - Ability to pin pages to the "top sites" section of Firefox Start

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Browser, defect)

Description

[Matt Brubeck (:mbrubeck)]

Just like on the new tab page in desktop Firefox, the user should be able to pin a site to the "Top Sites" section of the Metro Firefox start page.

For any page that is currently visible under "Top Sites", the user should be able to select the page from the Top Sites grid and pin/unpin it (possibly using an app bar button, as on the Windows 8 start screen).

For pages that are not currently visible in the Top Sites list, users should be able to pin the site by selecting it from the bookmarks or history list, or maybe while viewing the site directly.  (We can split this part into separate bugs if necessary.)

Comment 1

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

Attachment: [IxD]Pin top site

Comment 2

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

Main use case: 
1. Select a top site
2. Pin a top site
3. Unpin a top site
4. Remove a top site
5. Load a new top site
6. Restore the most recently removed top site if user hasn't exited app bar
7. Deselect a top site

Note:  Users can only select one top site at a single time. Unlike the tiles on Win8 start screen, FX metro top sites have only 8 items. Selecting multiple items for editing and removing are probably not the main scenario. One site at a time simplifies contextual commands in the app bar.

Comment 3

[Matt Brubeck (:mbrubeck)]

Pushing this out to iteration 3 because of the dependency on bug 800996, but we can start it sooner if that work is ready sooner.

Comment 4

[Asa Dotzler [:asa]]

oops, mistook this for another bug. fixing it back up.

Comment 6

[Sam Foster [:sfoster] (he/him)]

Attachment: WIP - hook up pin/unpin topsites

Uses a forked NewTabUtils.jsm to hook up the pin/unpin of top sites. 
For now, the update method clears out the richgrid and re-populates it, rather than using existing tiles. 
Also, more/completion of tests still todo.

Comment 7

[Matt Brubeck (:mbrubeck)]

Comment on attachment 712487 [details] [diff] [review]
WIP - hook up pin/unpin topsites

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

Looks pretty good.

::: browser/metro/base/content/TopSites.js
@@ +10,5 @@
> +const PREF_NEWTAB_ROWS = "browser.newtabpage.rows";
> +const PREF_NEWTAB_COLUMNS = "browser.newtabpage.columns";
> +
> +Cu.import("resource:///modules/NewTabUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Promise", "resource://gre/modules/commonjs/sdk/core/promise.js");

Move both of these into browser-scripts.js, please.

@@ +109,5 @@
> +
> +  getSites: function(aOptions) {
> +    let linksDeferred = this._linksDeferred = Promise.defer();
> +    // wait until init's populate cache is finished..
> +    this._promisedInit.then(function(){

If we can replace some of the explicit promises with some Task.spawn / yield code, I would personally find it easier to read.

@@ +149,5 @@
>    }
>  };
> +// configure the Links singleton to use our provider
> +let Links = NewTabUtils.links;
> +Links._provider = TopSites.PlacesProvider;

If we want to use our own provider, we should probably patch NewTabUtils to provide a less-fragile way of overriding it.

Even better, can we just use the default provider (and possibly add options to it if necessary)?

Comment 8

[Sam Foster [:sfoster] (he/him)]

Some background: In NewTabUtils, the Links singleton assigns NewTabUtils' PlacesProvider as the "default provider for links", but no mechanism is provided to configure or set which provider to use. so, tldr; Maybe just adding a setter to Links to assign to this._provider is the fix we need?

The default places provider has a couple of problems we need to work around: 

* Relies on PlacesUtils.jsm - maybe (likely) we'll need this eventually too but at least right now its quite a bit of bloat for not much value

* As the query options are assembled in the getLink method we have no way to configure the query. E.g. `options.maxResults = HISTORY_RESULTS_LIMIT` -  HISTORY_RESULTS_LIMIT is a const defined upstream of us in NewTabUtils.jsm, set to 100 records - which is more than we need, and as its a const we can't configure easily. 

* Only gives us "link" objects with: {url: url, title: title} - we need least .icon as well from the query result node. 

* Calls `db.asyncExecuteLegacyQueries` for the actual query. I don't know the history of this API, but adding legacy queries to a new product seems wrong?

Comment 9

[Matt Brubeck (:mbrubeck)]

(In reply to Sam Foster [:sfoster] from comment #8)
> * Relies on PlacesUtils.jsm - maybe (likely) we'll need this eventually too
> but at least right now its quite a bit of bloat for not much value

We already use PlacesUtils.jsm for our bookmarks grid and a few other places.

> * As the query options are assembled in the getLink method we have no way to
> configure the query. E.g. `options.maxResults = HISTORY_RESULTS_LIMIT` - 
> HISTORY_RESULTS_LIMIT is a const defined upstream of us in NewTabUtils.jsm,
> set to 100 records - which is more than we need, and as its a const we can't
> configure easily.

We should fix this in NewTabUtils if needed -- though remember that the user may have blocked a large number of results, so we may actually want to fetch a lot more than we need if it's not too expensive.

> * Only gives us "link" objects with: {url: url, title: title} - we need
> least .icon as well from the query result node.

Let's fix this in NewTabUtils, or fetch the favicons separately.

> * Calls `db.asyncExecuteLegacyQueries` for the actual query. I don't know
> the history of this API, but adding legacy queries to a new product seems
> wrong?

If this needs to be changed, let's change it in NewTabUtils.

Comment 10

[Sam Foster [:sfoster] (he/him)]

Attachment: WIP - hook up pin/unpin topsites

Update which I think addresses all feedback items. 
Patch applies on top of the patch on #840287 to move NewTabUtils.jsm
As far as what's in this patch, I think its ready for review. But there's a couple of things missing to close out the story: 

* No cross-slide so selection of tabs is still via long-press or right-click

* Placeholder "pinned tile" treatment (dotted border) demonstrates the class is applied but we'll need the real thing (shorlander is aware and working on it)

* Placeholder contextual action icons in the appbar

* I'm usng NewTabUtils as-is, with the default provider on NewTabUtils.links. So it's doing the query asnyc. But topsites is still very slow to come in; I'm not sure what that's about at this point, it may warrant its own bug

* Favicons are temporarily gone. I'd like to treat that and other view/grid issues following on from #835999

Also, although I see the site getting pinnd/unpinned I've not completed the testing to verify that pinning is actually having the expected effect on a site's placement in the grid. It /seems/ to work, but I should really do that :)

Comment 11

[Matt Brubeck (:mbrubeck)]

Comment on attachment 713665 [details] [diff] [review]
WIP - hook up pin/unpin topsites

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

Looks good.  I think we can check the code in once some of the debugging code and FIXMEs are cleaned up (and bug 840287 is finished), and we can un-hide the contextual actions tray once we get the icons and strings.

Comment 12

[Sam Foster [:sfoster] (he/him)]

Attachment: WIP, TopSites pin testing w. sites cache oddness

I've been trying to track down an issue I introduced where the sites collection is being reset/not reset as the tests run. The issue first appears in the "load and display top sites" test. I've got a load of logging in here which appears to show my TopSites._sites property correctly as null (and the _sitesDirty correctly as true) all the way up to the call to the TopSitesStartView.update (from the NewTabUtils allPages.update) when it has flipped somehow: sitesDirty is false and _sites is an empty array.

Comment 13

[Matt Brubeck (:mbrubeck)]

(In reply to Sam Foster [:sfoster] from comment #12)
> I've been trying to track down an issue I introduced where the sites
> collection is being reset/not reset as the tests run. The issue first
> appears in the "load and display top sites" test. I've got a load of logging
> in here which appears to show my TopSites._sites property correctly as null
> (and the _sitesDirty correctly as true) all the way up to the call to the
> TopSitesStartView.update (from the NewTabUtils allPages.update) when it has
> flipped somehow: sitesDirty is false and _sites is an empty array.

It looks like we are loading this code multiple times into separate global scopes.  We need to fix this code to *not* call loadSubScript multiple times for the same file, for files that "export" multiple symbols:
http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser-scripts.js#134

...either by making each of those files into a JSM, or by including it directly in browser.xul, or by modifying browser-scripts.js to keep track of which globals share the same script.  For all the files that are used on the start page at least, let's just include them directly in browser.xul via <script> tags (since they won't benefit much from delayed loading anyway).

Comment 14

[Sam Foster [:sfoster] (he/him)]

Attachment: Pin/unpin of topsites functionality

This is the rewrite of the top sites to use NewTabUtils, and the wiring up of pin/unpin. I broke out css etc into a seperate patch, likewise tests

Known issues: somewhere in there Top Sites now take up to 3-4 seconds to show up, vs. the sync gHistSvc.executeQuery approach used before in populateGrid. I've not dug deep into whats going on - but its obviously bad and will likely need fixing before this can land (and certainly before the story can be closed)

I also sometimes see null values coming back out of NewTabUtils.links.getLinks() - I've not been able to track down where they are introduced yet. I'm working around the issue for now, but it may be cumulative and start to affect the number of valid top sites you get back eventually?

Comment 15

[Sam Foster [:sfoster] (he/him)]

Attachment: Chrome tests for pin/unpin of Top Sites

Comment 16

[Sam Foster [:sfoster] (he/him)]

Attachment: Chrome tests for pin/unpin of Top Sites

Known issues: I do some comparisons with the grid children expecting exactly 9 tiles. That should at least come from a const somewhere and maybe is not a constant anyhow (tho' tests are always run in landscape mode so that reduces the variables)
Also, if/when we start reusing the grid tiles and just updating their contents, these tests will likewise need to be updated as we currently assume all .children are top sites tiles.

Comment 17

[Sam Foster [:sfoster] (he/him)]

Attachment: Placeholder css for the pinned state of topsites tiles

Do we have design specs for this pinned tile state somewhere to hand?

Comment 18

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 721527 [details] [diff] [review]
Pin/unpin of topsites functionality

Removing ttaubert as he's out this week.

Comment 19

[Matt Brubeck (:mbrubeck)]

Comment on attachment 721532 [details] [diff] [review]
Placeholder css for the pinned state of topsites tiles

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

::: browser/metro/theme/platform.css
@@ +359,5 @@
>    opacity: 1.0;
>  }
> +richgriditem.pinned .richgrid-icon-box {
> +  border: 2px dotted #f00!important;
> +}

I know this is just a placeholder, so the only thing I want to make sure of is that the tile's size doesn't change when it gets pinned/unpinned.  (It's annoying if changing the tile state causes other tiles to move around -- I think we already have this problem for selected tiles.)

Comment 20

[Sam Foster [:sfoster] (he/him)]

Agreed. The mocks I've seen show a translucent an icon in a top corner. But selection should use outline (or an overlayed element) if possible? I'll change this placeholder meantime though

Comment 21

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 721527 [details] [diff] [review]
Pin/unpin of topsites functionality

This is pin/unpin. The known issues are: 
* Performance - as discussed. This may improve with non-debug build (the code /should/ be the same as that used by desktop)
* I noticed the ui spec calls for a grid of 3x3 with 8 tiles (and the last one empty). Also, depending on the state (landscape vs. portrait, appbar visible or not) sometimes we use 1x9, 4x2, 3x3. I believe Ive preserved the behavior as it was, but I'm not sure its all correct. I think that's more in scope for bug #794028 though? 
* The Site model may eventually want to become its own module if it turns out we want to use it and NewTabUtils for other tiles/groups on our start page

Comment 22

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 721530 [details] [diff] [review]
Chrome tests for pin/unpin of Top Sites

Tests all pass for me. See comments on the functionality patch re: 9 vs. 8 tiles. We don't yet have the empty-tile binding I was wanting to use to support that. These tests will need revision at that time. I'm open to blocking this patch landing on that if we feel it necessary.

Comment 23

[Sam Foster [:sfoster] (he/him)]

Attachment: Adds pin/unpin context buttons and placeholder pinned tile state

The icons for pin/unpin contextual appbar buttons seen in https://bug836387.bugzilla.mozilla.org/attachment.cgi?id=719300 already exist, so I've hooked those up. The pinned tile state visual treatment is still being finalized so that is placeholder CSS for now.

Comment 24

[Asa Dotzler [:asa]]

I think the empty tile is no longer a requirement.  Yuan, we're not going to have a mock tile for visiting the full page view of that tile group, right? I think the new plan is (for v2) to have the headers for those tile groups be clickable to take you to the full view and not to have a "more ..." tile at the end of those groups.

Comment 25

[Matt Brubeck (:mbrubeck)]

Comment on attachment 722229 [details] [diff] [review]
Adds pin/unpin context buttons and placeholder pinned tile state

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

This part looks fine, but looks like it might depend on a missing patch?  Or maybe *I'm* missing something. ;)

::: browser/metro/theme/browser.css
@@ +622,5 @@
>  }
>  
> +/* Tile-selection-Specific */
> +#pin-selected-button {
> +  -moz-image-region: rect(0px, 240px, 40px, 200px) !important;

Should there by a list-style-image for this, somewhere, or is that waiting on assets?

Comment 26

[Matt Brubeck (:mbrubeck)]

Comment on attachment 721527 [details] [diff] [review]
Pin/unpin of topsites functionality

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

Looks good but please silence the console output before landing, and see below for some optional suggestions for cleanup/organization.

I'd also like to understand the FIXME before we land it.

::: browser/metro/base/content/TopSites.js
@@ +164,5 @@
>  
> +  _updating: false,
> +  get isUpdating() {
> +    return this._updating;
> +  },

Please add a comment that this is used for testing.  You can also remove the getter and have tests access the property directly, if you want.

@@ +242,5 @@
> +    Util.dumpLn("populateGrid, TopSites._sites: " + JSON.stringify(TopSites._sites));
> +    let createItemNode = function(aSite) {
> +      // Site instances should always return something for the title
> +      return this.appendItem(aSite.title, aSite.url);
> +    }.bind(this._set);

Why is this wrapped in a closure instead of simply inline below?

@@ +262,5 @@
>  
> +    for (let idx=0; idx < length; idx++) {
> +      let item = tileset.children[idx],
> +          site = sites[idx];
> +      Util.dumpLn("populateGrid with site: " + JSON.stringify(site));

Could you remove or disable most of the debugging output before landing, please?  (It's okay for error handlers or for code that runs rarely, but I prefer to avoid having it on by default for code that runs frequently in regular use.)

@@ +274,5 @@
> +          // FIXME: grid/tile update implementation needed, e.g.
> +          // item.label = site.title
> +          // item.href = site.url
> +          item.setAttribute("label", site.title);
> +          item.setAttribute("value", site.url);

Do we need to address this FIXME before landing?

@@ +276,5 @@
> +          // item.href = site.url
> +          item.setAttribute("label", site.title);
> +          item.setAttribute("value", site.url);
> +        } catch (e) {
> +          Util.dumpLn("Error reusing existing tile: " + e.message);

Does this ever throw?  If so, what does it throw?  Should we do something to recover (e.g. delete the node and insert a new one)?

@@ +394,5 @@
>      }
> +    NewTabUtils.allPages.register(this._view);
> +    TopSites.prepareCache().then(function(){
> +      this._view.populateGrid();
> +    }.bind(this));

Could we move this code into the TopSitesView constructor?

Comment 27

[Matt Brubeck (:mbrubeck)]

Comment on attachment 721530 [details] [diff] [review]
Chrome tests for pin/unpin of Top Sites

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

I confess my eyes start to glaze over when reviewing big test patches, but I believe I looked over everything here and did not see any problems.

Comment 28

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

(In reply to Asa Dotzler [:asa] from comment #24)
> I think the empty tile is no longer a requirement.  Yuan, we're not going to
> have a mock tile for visiting the full page view of that tile group, right?
> I think the new plan is (for v2) to have the headers for those tile groups
> be clickable to take you to the full view and not to have a "more ..." tile
> at the end of those groups.

Yes you are correct, we are going to use "Bookmarks >", "History >", instead of using "More...".

For Top Sites, tile group should still remain 8 sites due to a different reason. When the tab strip is visible or in portrait mode, the grid needs to be 4*2/2*4. If there are 9 tiles, one must be left out at the end, which breaks the pattern. Having 8 tiles instead gives the layout more flexibility.

Comment 29

[Sam Foster [:sfoster] (he/him)]

Re the /* Tile-selection-Specific */ rules in browser.css, these extend a general #appbar toolbarbutton/:hover/:active rule, see http://mxr.mozilla.org/mozilla-central/source/browser/metro/theme/browser.css#547

Re: empty tile and the FIXME on update-tile implementation, this is a planned/proposed optimization and as such can be shelved for if/when we observe a problem that needs fixing. The idea was to keep a fixed number of DOM elements around as slots in the grid, and to update these in place rather than destroying and recreating them with each change. When we have less sites to show than the number of grid slots, we would just apply an empty-tile binding. This can be visible or not. If we destroy and recreate the grid items on each update, the empty tile is not needed as we simply only create as many as necessary.

The richgriditem binding doesn't support runtime property updates today. So, as ally found we leak grid implementation details into the view (or blow away the element and recreate it)

Re: other comments, yes, and will do.

Comment 30

[Allison Naaktgeboren :ally]

I did add a filter with the thumbnails. However, mbrubeck points out that this patch may need to remove them  https://bugzilla.mozilla.org/show_bug.cgi?id=794028#c32

Comment 31

[Allison Naaktgeboren :ally]

(In reply to Yuan Wang(:Yuan) – Firefox UX Team from comment #28)

> For Top Sites, tile group should still remain 8 sites due to a different
> reason. When the tab strip is visible or in portrait mode, the grid needs to
> be 4*2/2*4. If there are 9 tiles, one must be left out at the end, which
> breaks the pattern. Having 8 tiles instead gives the layout more flexibility.

There are still 9 tiles, as when part 1 of 794028 went in, the mock (& the offline discussions) had it at 9 tiles. I did file a followup bug for you, bug 848999. Should be a pretty easy tweak. 

Yuan, we don't need empty tiles to get the behavior you desire with layout.

Comment 32

[Allison Naaktgeboren :ally]

Comment on attachment 721530 [details] [diff] [review]
Chrome tests for pin/unpin of Top Sites

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

"what he said"

Serious kudos for going through the pain of getting tests to run for this, I know its been rough.

Comment 33

[Allison Naaktgeboren :ally]

Comment on attachment 721527 [details] [diff] [review]
Pin/unpin of topsites functionality

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

Sam & I had a long productive conversation offline about this series of patches and my concerns/feedback.

The tl;dr is that I would not be comfortable shipping this code as-is in the final release. That said, I understand that is a stepping stone, and there are plans to iterate on it, so I would not want to block this landing.

Comment 34

[Sam Foster [:sfoster] (he/him)]

Talked with ally about these TopSites changes: 
* Current implementation doesn't represent the final state - we expect similar work on bookmarks and other tile groups to yield some refactoring, code-reuse and more consistency
* populateGrid method of the TopSitesView is lengthy and could use some organization/refactoring. Some of this functionality might be able to move to the grid item binding
* Its likely the Site model or something like it may end up used by History and perhaps Bookmarks, it is here to limit the scope of the changes

I've got these patches based on top of 794028 locally, will attach for review after taking a look at what can sensibly be done to address these issues and put it back up for review.

Comment 35

[Sam Foster [:sfoster] (he/him)]

Attachment: Refactored pin/unpin topsites, Site, tests and richgriditem refresh

I've addressed the feedback by moving much of the guts of populateGrid out to a beefed up the Site model. Along the way I was able to get us a refresh method on the richgriditem which means for simple property/state changes we don't need to re-populate the entire grid. 

Note that as of https://hg.mozilla.org/mozilla-central/rev/196693568165 seltype on richgrid is actually meaningful, so we need to set seltype="multiple" wherever we want multiple-selection behavior.

Comment 36

[Matt Brubeck (:mbrubeck)]

Comment on attachment 727622 [details] [diff] [review]
Refactored pin/unpin topsites, Site, tests and richgriditem refresh

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

::: browser/metro/base/content/bindings/grid.xml
@@ +573,5 @@
> +                  this.iconSrc = this.getAttribute('iconURI');
> +                }
> +                break;
> +              case "backgroundimage":
> +                if (needsUpdate('backgroundimage'), this._backgroundimage) {

Drive-by comment: I think the first ")" is misplaced here, and in similar expressions on the following lines.

Comment 37

[Sam Foster [:sfoster] (he/him)]

Attachment: Refactored pin/unpin topsites, Site, tests and richgriditem refresh

Fixed the calls to needsUpdate in richgriditem binding, thanks mbrubeck

Comment 38

[Allison Naaktgeboren :ally]

Comment on attachment 728613 [details] [diff] [review]
Refactored pin/unpin topsites, Site, tests and richgriditem refresh

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

Thanks for working so hard on this.
Thanks for writing comments.
Thanks for writing tests, extra kudos because metro tests are not easy to write right now.

I'm starting to feel like much of the model of sites.js could be merged with richgriditem, and that really any tile on the home screen should be able to honor context actions like pin/unpin/hide/etc. That said, we've spent a lot of time on this, and may need to call this good enough, file some followup bugs, and refactor later.

::: browser/metro/base/content/Site.js
@@ +15,5 @@
> +  this._link = aLink;
> +}
> +
> +Site.prototype = {
> +  icon: '', // is some empty/default icon data-uri we can use here?

we should use the default icon, the grey globe, browser\metro\theme\images\identity-icons-generic.png

@@ +44,5 @@
> +      label: this.title,
> +      pinned: this.pinned ? true : undefined,
> +      selected: this.selected,
> +      color: this.color,
> +      backgroundimage: this.backgroundimage,

Can we change this to backgroundImage? After friday's xbl issues(854011), we'd like to name this value consistently to prevent future bugs.

@@ +65,5 @@
> +    if (aNode.refresh) {
> +      // just update it
> +      aNode.refresh();
> +    } else {
> +      // these attribute values will get picked up later when the binding is applied

I'm not sure we can deterministically say when binding happens. Mbrubeck spent some time studying this problem, so I defer to him on this.

::: browser/metro/base/content/TopSites.js
@@ +24,5 @@
> +    if (this._promisedCache && !aForce) {
> +      return this._promisedCache;
> +    }
> +    let deferred = Promise.defer();
> +    this._promisedCache = deferred.promise;

Limitation of reviewer's brain: I am not conversant in promises, so I can not be certain that promise based code is correct.

@@ +45,5 @@
> +
> +    let links = NewTabUtils.links.getLinks();
> +    links = Array.slice(links).filter(function(aLink, aIndex) {
> +      // Sometimes seeing null in getLinks' response,
> +      // <sfoster> Haven't yet tracked down the source, so just treat the symptom

Could we have a followup bug, and include the bug number in the comment?

@@ +49,5 @@
> +      // <sfoster> Haven't yet tracked down the source, so just treat the symptom
> +      if (aLink && aLink.url) {
> +        return true;
> +      } else {
> +        Util.dumpLn("Caught dud links in getLinks response: " + JSON.stringify(aLink));

Please make sure we don't dump in release builds. (I do not know if dumpln converts to a no op in release)

@@ +120,1 @@
>      // FIXME: implementation needed

For followup/missing work, please include the bug number of the followup in the comment

@@ +185,4 @@
>              aNode.contextActions.delete('pin');
>              aNode.contextActions.add('unpin');
> +          } else {
> +            // site not pinned?

log an error here perhaps?

@@ +196,4 @@
>              aNode.contextActions.delete('unpin');
>              aNode.contextActions.add('pin');
> +          } else {
> +            // site not unpinned?

when does this happen? unpinning failing is probably worse for a user, in the help-i-cant-make-an-embarrassing-screenshot-go-away way.

@@ +258,5 @@
> +    let tileset = this._set;
> +
> +    // remove any extra tiles
> +    while (tileset.children.length > length) {
> +      tileset.removeChild(tileset.children[tileset.children.length -1]);

why do we need this?

@@ +355,5 @@
>      }
> +    NewTabUtils.allPages.register(this._view);
> +    TopSites.prepareCache().then(function(){
> +      this._view.populateGrid();
> +    }.bind(this));

this code repeats in both startviews. perhaps we could refactor it into the shared prototype?

::: browser/metro/base/content/bindings/grid.xml
@@ +539,5 @@
> +        <parameter name="aProps"/>
> +        <body><![CDATA[
> +          // Seed the binding properties from bound-node attribute values
> +          let props;
> +          let allAttributes = ['iconURI', 'backgroundimage', 'color', 'label', 'value', 'pinned', 'selected'];

see previous comment about remaning backgroundimage to backgroundImage (matt's code fixing it landed friday, but I could see how hg would not notice the conflict in this block)

@@ +543,5 @@
> +          let allAttributes = ['iconURI', 'backgroundimage', 'color', 'label', 'value', 'pinned', 'selected'];
> +          let dirtyAttributes = allAttributes;
> +
> +          // convenience: spread an optional property-bag into real attributes
> +          // so we can seed properties uniformly in either case

limitation of reviewer's brain. I don't understand what you mean here?

::: browser/metro/base/tests/browser_topsites.js
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

limitation of reviewer's brain: I do not know enough about the promise/yield/task to be able to say whether or not your tests are good. I'm sorry.

Comment 39

[Sam Foster [:sfoster] (he/him)]

Thanks for the comments Ally. As I don't know how to remark on your remarks I'll address them in order: 

Re: Site model / richgriditem: Some of Site may be Top Site-specific. In which case its more like TopSiteLink extends Site. I'm not confident I can nail this up front, before attempting to reuse this code to implement similar functionality for the other grid views. I do like having the getters in front of the calls back to NewTabUtils functionality - we certainly don't want the richlistitem binding knowing about that. So I'd like to land this part in something like its curent shape with an understanding that it might evolve.

Re: default icon - thanks, will hook that up. 

Re: backgroundImage / backgroundimage: yes, this patch predates that fix, I'll update it. 

Re: item.refresh - as the binding will invoke the constructor, and the constructor simply calls refresh(), I think this method lets us setup a node for later binding, or update an already-bound node with new values. And, we can neatly check typeof node.refresh before calling it and the right thing should happen. 

Re: promisedCache - the tests seem to indicate this is working. I dislike having caches of caches, but we have enough mismatch with desktop that I think NewTabUtils needs wrapping and this extra cache is a side-effect of that. The idea is that promisedCache is a Promise that gets resolved to the actual cache values when that all gets completed. Promises spark some controversy but IMO this kind of thing is where they shine. 

Re: null links. I keep hoping its an artifact of testing bad/incomplete code ad ending up with bad prefs values. But it keeps coming up. I'll try and reproduce again before filing followup bug. 

Re: implementation FIXME: didn't know that was the convention, makes sense, will do 

Re: Failure of pin/unpin block - NewTabUtils uses prefs for storage, which are sync to save and dont supply any OOTB way to verify success or catch failures. I.e. This is hypothetical only; it will be clearer if I just remove it

Re: Removing of tileset.children - If # of Top Sites dips below 8 when we are redrawing, we'll could end up with extra tiles. Currently this isn't possible as we do a full recreation of the grid whenever we remove an item (via populateGrid) but its a useful safety net should we later optimize that. And a noop meanteam. 

Re: grid view setup boilerplate - yes, I'll see if this can move to TopSiteView

Re: speading of property-bags - sorry, jargon overload. A property-bag is just an object/dictionary. So we take { foo: "Foo", bar: "Bar" } and end up with node.setAttribute("foo", "Foo"); node.setAttribute("bar", "Bar); So, to avoid refreshing all an items properties, can can call it like item.refresh({ "backgroundImage", "url('blah')" }) and it will update (only) the attribute and invoke the backgroundImage setter. I'll add a comment to this effect. 

Re: tests - these a) Pass, and b) have previously had an r+ from mbrubeck so I think that's ok.

Comment 40

[Matt Brubeck (:mbrubeck)]

Comment on attachment 728613 [details] [diff] [review]
Refactored pin/unpin topsites, Site, tests and richgriditem refresh

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

Some minor nits below, and a question about the complexity of refresh().

Overall I want to get this checked in soon, and am happy to see it land with just the minor nits addressed (plus your responses to ally's review, from comment 39).  If you have questions/comments about the refresh() stuff we can talk about it tomorrow before you land this, or you can land it as-is and we can hash out any changes in separate bugs.

(In reply to Sam Foster [:sfoster] from comment #39)
> Re: Failure of pin/unpin block - NewTabUtils uses prefs for storage, which
> are sync to save and dont supply any OOTB way to verify success or catch
> failures. I.e. This is hypothetical only; it will be clearer if I just
> remove it

I agree -- since these functions currently throw on error and never return false, let's just remove the "return true", stop checking the return value, and rely the exceptions instead.

::: browser/metro/base/content/Site.js
@@ +15,5 @@
> +  this._link = aLink;
> +}
> +
> +Site.prototype = {
> +  icon: '', // is some empty/default icon data-uri we can use here?

Our CSS should automatically display the generic favicon for tiles/tabs/etc. without an explicit icon.  In code we should probably just use '' or null by default.

::: browser/metro/base/content/bindings/grid.xml
@@ +516,5 @@
>        <property name="backgroundimage"
>                  onset="this._backgroundimage = val; this.setBackgroundImage();"
>                  onget="return this._backgroundimage;" />
>        <property name="selected"
> +                onget="return this.getAttribute('selected') == 'true';"

Please use hasAttribute (just for conciseness).

@@ +520,5 @@
> +                onget="return this.getAttribute('selected') == 'true';"
> +                onset="if (val) { this.setAttribute('selected', val) } else this.removeAttribute('selected');"/>
> +      <property name="url"
> +                onget="return this._url"
> +                onset="this._url = val; this.setAttribute('value', val);"/>

Instead of storing the same value in two places, can we just store it in the attribute alone?  I.e.,

  onget="return this.getAttribute('value')"
  onset="this.setAttribute('value', val)"

@@ +537,5 @@
>  
> +      <method name="refresh">
> +        <parameter name="aProps"/>
> +        <body><![CDATA[
> +          // Seed the binding properties from bound-node attribute values

I like the idea of this function, but all the dirtiness-testing seems like premature optimization.  Is it possible to shorten this to just a bunch of lines that run unconditionally, like:

<body><![CDATA[
  this.iconSrc = this.getAttribute('iconURI');
  this.color = this.getAttribute('customColor');
  this.backgroundImage = this.getAttribute('customImage');
  this.url = this.getAttribute('value');
  ...
]]></body>

If there are specific properties that are expensive/dangerous to refresh unnecessarily then maybe we could wrap just those ones in simple "if" blocks, like:

  let color = this.getAttribute('customColor');
  if (this.color != color)
    this.color = color;

@@ +575,5 @@
> +                break;
> +              case "backgroundimage":
> +                if (needsUpdate('backgroundimage', this._backgroundimage)) {
> +                  this._backgroundimage = this.getAttribute('backgroundimage');
> +                  this.setBackgroundImage();

After bug 854011 this should change to "this.backgroundImage = ..."

@@ +581,5 @@
> +                break;
> +              case "color":
> +                if (needsUpdate('color', this._color)) {
> +                  this._color = this.getAttribute('color');
> +                  this.setColor();

...and "this.color = ..."

::: browser/metro/theme/platform.css
@@ +485,4 @@
>    opacity: 1.0;
>  }
>  
> +/* <sfoster> placeholder pinned-state indication */

Do we have a bug for final assets/styles?

Comment 41

[Sam Foster [:sfoster] (he/him)]

On inbound:  https://hg.mozilla.org/integration/mozilla-inbound/rev/44d4d2234a6e

I addressed all the comments, including chopping down richgriditem.refresh right down to simply setting from the attributes. I added a call to refreshBackgroundImage and reset _contextActions in refresh. 

I was able to track down the sporadic issue with null links noted above. This is patched and landed in #855270. That gets rid of the filter block in TopSites.getSites and the tests pass again/still. 

I've left the contextualactions-tray in browser.xul hidden as we don't have the expected contextual actions for bookmarks etc. implemented yet. So, to try all this out, remove the hidden attribute in there.

Comment 42

[Matt Brubeck (:mbrubeck)]

Pushed a trivial followup to revert some lines that were accidentally added twice (by this bug and by bug 846636):
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5584aa212c9

Comment 43

[Matt Brubeck (:mbrubeck)]

Comment on attachment 728613 [details] [diff] [review]
Refactored pin/unpin topsites, Site, tests and richgriditem refresh

(Tim, your feedback on our Top Sites code is still welcome, but you can look at the code in the tree rather than at this patch, now that it's landed.)

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/44d4d2234a6e

Comment 45

[Ryan VanderMeulen [:RyanVM]]

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