789363 - Work - Live update the history used for Firefox Start and the auto-complete screen

Status: RESOLVED FIXED

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

Description

[Frank Yan (:fryn)]

Currently, we simply generate the history section of the awesome screen once on startup and leave it as a static collection.

The next step is to update the section dynamically during the browsing session.

Comment 1

[Allison Naaktgeboren :ally]

Attachment: wip (missing a functional onVisit)

There are a number of questions about next steps, including for onVisit, onPageExpired, etc, in the comments.

Since I know you(mbrubeck) like to apply & fiddle with patches, I left much of the debug code in the comment for you to use if you like, along with notes & links.

Comment 2

[Frank Yan (:fryn)]

Attachment: WIP w/ suggestions by fryn

Thank you for working on this, Ally! :)

I don't think it's worth modifying the grid binding for a helper function that only has callers inside history.js, due to batching that is specific to our history implementation. I made some modifications to your WIP patch to simplify the loop logic a bit and have attached it here.

For onDeleteURI: If we set the second parameter of removeItemAt (aSkipArrange) to true and then call arrangeItems once at the end of the loop, can the grid handle that?

Otherwise, the code looks good so far.

Let me know what you think.

Comment 3

[Rodrigo Silveira [:rsilveira] [:rodms]]

There is some overlap with what I'm doing on Bug 831916. I'll try to post a WIP patch later.

Comment 4

[Frank Yan (:fryn)]

Comment on attachment 741093 [details] [diff] [review]
wip (missing a functional onVisit)

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

Just some minor feedback if we do it this way, which is okay too.
The thing that matters after all is whether it works.

::: browser/metro/base/content/bindings/grid.xml
@@ +247,5 @@
>        </method>
>  
> +      <!-- returns -1 for invalid argument
> +           returns an array of matches for that url (may have more than one entry on the grid)
> +           returns an empty array if no matches found

I think this should return null for invalid arguments to keep the return type as consistent as possible.

@@ +249,5 @@
> +      <!-- returns -1 for invalid argument
> +           returns an array of matches for that url (may have more than one entry on the grid)
> +           returns an empty array if no matches found
> +       -->
> +      <method name="getIndexOfItemByUrl">

getIndicesOfItemByUrl, perhaps.

::: browser/metro/base/content/history.js
@@ +104,5 @@
> +    let changedItems = this._set.getIndexOfItemByUrl(aURI.spec);
> +    //Cu.reportError("changed item list: "+ changedItems.toString());
> +    let that = this;
> +    changedItems.forEach(function(element, index, array){
> +      let item = that._set.getItemAtIndex(element);

Typo: s/element/index/

@@ +117,5 @@
> +    //Cu.reportError("anaaktge- history.onDeleteURI: "+aURI.spec+" matching indices: "+removedItems.toString());
> +    let that = this;
> +    removedItems.forEach(function(element, index, array){
> +      //second param tells it to relayout grid
> +      that._set.removeItemAt(element, false);

Typo: s/element/index/

@@ +148,5 @@
> +    */
> +    if(aWhat == 3) {
> +      //Cu.reportError("favicon change");
> +      let changedItems = this._set.getIndexOfItemByUrl(aURI.spec);
> +      let that = this;

Tip: You can get the right `this` by providing `this` as the second param to forEach, e.g.:
changedItems.forEach(function(...) { ... }, this);

Comment 5

[Matt Brubeck (:mbrubeck)]

Comment on attachment 741093 [details] [diff] [review]
wip (missing a functional onVisit)

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

I think the conclusion of the IRC discussion today sounded good.  I'll try to summarize below along with any other feedback.

It looks like we can make very minimal changes to grid.xml to support this patch.  It's also great if people also want to convert other richgrid methods and callers to use items instead of indices, but I don't think that needs to block this work.

::: browser/metro/base/content/bindings/grid.xml
@@ +249,5 @@
> +      <!-- returns -1 for invalid argument
> +           returns an array of matches for that url (may have more than one entry on the grid)
> +           returns an empty array if no matches found
> +       -->
> +      <method name="getIndexOfItemByUrl">

This will become "getItemsByUrl" and will return elements instead of indices.  I think this is a reasonable method to add to richgrid.

@@ +255,5 @@
> +        <body>
> +          <![CDATA[
> +            if (!urlStr) {
> +              return -1;
> +            }

I don't think we need this check here; without it we will just return an empty array, assuming nothing matches the passed value.

Nit: parameter should be called "aUrl".

::: browser/metro/base/content/history.js
@@ +93,5 @@
> +    // might make sense just to repopulate grid so that the ordering happens
> +    //  right now that is part of the db query, so figuring out where to stick
> +    //  it when the grid has no visit or awareness of the time values might be error prone & time consuming
> +    //  nb this doesnt seem to cause the grid to visually update & reorder. refresh possibly required?
> +    this.populateGrid();

I like the idea of just calling populateGrid here, as long as it doesn't get too expensive.  It looks like you might need to clear the grid first (populateGrid seems to just add items, not remove any existing ones).

I tried to find the equivalent code in desktop Firefox that handles updates in the history sidebar or history menu, but I haven't managed to dig far enough yet...

@@ +104,5 @@
> +    let changedItems = this._set.getIndexOfItemByUrl(aURI.spec);
> +    //Cu.reportError("changed item list: "+ changedItems.toString());
> +    let that = this;
> +    changedItems.forEach(function(element, index, array){
> +      let item = that._set.getItemAtIndex(element);

This will be "for (let item of this._set.getItemsByUrl(aURI.spec)) {"

@@ +117,5 @@
> +    //Cu.reportError("anaaktge- history.onDeleteURI: "+aURI.spec+" matching indices: "+removedItems.toString());
> +    let that = this;
> +    removedItems.forEach(function(element, index, array){
> +      //second param tells it to relayout grid
> +      that._set.removeItemAt(element, false);

...and we'll want to add a "removeItem" method to richgrid, and use it here.

@@ +132,5 @@
>      this._set.clearAll();
>    },
>  
> +  // Question: Where do I find a list of the aWhats?
> +  // Question: I think we only care about icon?

Yeah, it looks like ATTRIBUTE_FAVICON is the only possible value for aWhat:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsINavHistoryService.idl#700

@@ +133,5 @@
>    },
>  
> +  // Question: Where do I find a list of the aWhats?
> +  // Question: I think we only care about icon?
> +  //    ATTRIBUTE_FAVICON 3  Where can I get at this programmatically?

Ci.nsINavHistoryService.ATTRIBUTE_FAVICON

@@ +163,5 @@
>    },
>  
> +  //Question: If it is just a visit that expires & not hte whole history entry, do we need to do anything?
> +  // i see this on the doc page. i am not sure sure what to do
> +  // "Note: This method was removed in Gecko 2.0. Use {{ manch("onDeleteVisits") }} instead."

Here's the new method that replaced onPageExpired:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsINavHistoryService.idl#723

If the reason is REASON_EXPIRED then I don't think we need to do anything.  If it's REASON_DELETED, maybe we should just clear the grid and re-populate, like when a visit is added.

Comment 6

[Allison Naaktgeboren :ally]

Thanks for the feedback, and the tip. 

(In reply to Frank Yan (:fryn) from comment #4)
> Comment on attachment 741093 [details] [diff] [review]
> @@ +117,5 @@
> > +    //Cu.reportError("anaaktge- history.onDeleteURI: "+aURI.spec+" matching indices: "+removedItems.toString());
> > +    let that = this;
> > +    removedItems.forEach(function(element, index, array){
> > +      //second param tells it to relayout grid
> > +      that._set.removeItemAt(element, false);
> 
> Typo: s/element/index/

Not a typo, but it is a gotcha. removedItems is a list whose elements are numbers-that-happen-to-be-indexes, ie [2, 16]. The index parameter in the forEach refers to the index of that elements in removedItems, not index of the elements on the grid. (I didn't love the name for this case, but that's the format of the forEach args)

So if I passed index to removeItemAt(), I'd remove the items at 0, 1, and not say items at 2, 16. Does that make sense?

Comment 7

[Frank Yan (:fryn)]

(In reply to :Ally Naaktgeboren from comment #6)
> Not a typo, but it is a gotcha.

You are absolutely correct! I had confused myself when I wrote the alternate patch. Thank you for the patient, fantastic explanation.

Comment 8

[Rodrigo Silveira [:rsilveira] [:rodms]]

Comment on attachment 741093 [details] [diff] [review]
wip (missing a functional onVisit)

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

::: browser/metro/base/content/history.js
@@ +168,1 @@
>    onPageExpired: function(aURI, aVisitTime, aWholeEntry) {

onPageExpired was replaced by onDeleteVisits see: https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsINavHistoryService.idl#744

I made this change at the patch for Bug 831916 if it helps: https://bugzilla.mozilla.org/attachment.cgi?id=741609&action=diff

Comment 9

[Allison Naaktgeboren :ally]

Attachment: nsI history observers

This is based on sfoster's richgrid changes. 

Note that the NodeList objects arent allowed to do some things one might expect, like forEach(), for...in, for each..in, and extending the dom to add foreach would get me lynched :) . https://developer.mozilla.org/en-US/docs/DOM/NodeList

refreshAndRepopulate: rsilveira's work on 831916 includes a much nicer, more heavy weight refresh behavior. When that lands this method & its call sites can be replaced with calls to his updated populateGrid().

Comment 10

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

Comment on attachment 742100 [details] [diff] [review]
nsI history observers

remember that richgriditems have their own refresh method which should normally be called after you update attribute values on the item node (see TopSites updateTile for an example).

Comment 11

[Matt Brubeck (:mbrubeck)]

Comment on attachment 742100 [details] [diff] [review]
nsI history observers

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

Looks good; just one substantive change (the first comment below, about aSkipArrange) and some minor mechanical changes/nits.

::: browser/metro/base/content/history.js
@@ +66,4 @@
>  
>    onBeginUpdateBatch: function() {
> +    // Message warns to avoid heavy processing before the end message arrives.
> +    // We don't have any.

We should probably pass |aSkipArrange = true| to appendItem and removeItem while we are in a batch.

We could set a flag like |this._inBatch = true;| here and reset it in onOndUpdateBatch.  Then change the grid calls to look like |appendItem(title, uri, this._inBatch)| or |removeItem(item, this._inBatch)|.

@@ +80,5 @@
>  
>    onTitleChanged: function(aURI, aPageTitle) {
> +    let changedItems = this._set.getItemsByUrl(aURI.spec);
> +    // NodeLists cannot foreach, for in, etc according to mdn. classic for loop is ok
> +    for(let i=0; i < changedItems.length; ++i){

You can use "for (let item of changedItems)" here.

Don't forget the space after "for". :)

Details:  "for...of" is the new, ES6 standards-track replacement for Mozilla's non-standard "for each"" and it works with any iterable or Array-like object, including NodeList.  You could also use the new JS1.6 Array.forEach(changedItems, ...) but I find "for...of" a bit more readable in this particular case.

https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Statements/for...of

https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Array#Array_generic_methods

@@ +87,4 @@
>    },
>  
>    onDeleteURI: function(aURI) {
> +    this._set.removeItemByUrl(aURI.spec);

Sorry for the churn, but I made sfoster change this to "removeItem", so this should now be:

  for (let item of this._set.getItemsByUrl(aURI.spec)) {
    this._set.removeItem(item);
  }

@@ +95,4 @@
>    },
>  
>    onPageChanged: function(aURI, aWhat, aValue) {
> +    if(aWhat ==  Ci.nsINavHistoryObserver.ATTRIBUTE_FAVICON) {

space after "if"

@@ +97,5 @@
>    onPageChanged: function(aURI, aWhat, aValue) {
> +    if(aWhat ==  Ci.nsINavHistoryObserver.ATTRIBUTE_FAVICON) {
> +      let changedItems = this._set.getItemsByUrl(aURI.spec);
> +      // NodeLists cannot foreach, for in, etc according to mdn. classic for loop is ok
> +      for(let i=0; i < changedItems.length; ++i){

Nits: "for...of", space

@@ +99,5 @@
> +      let changedItems = this._set.getItemsByUrl(aURI.spec);
> +      // NodeLists cannot foreach, for in, etc according to mdn. classic for loop is ok
> +      for(let i=0; i < changedItems.length; ++i){
> +        let currIcon = changedItems[i].getAttribute("iconURI");
> +        if( currIcon != aValue) {

misplaced space

Comment 12

[Allison Naaktgeboren :ally]

Attachment: nsiObservers, inbatch flag

Comment 13

[Matt Brubeck (:mbrubeck)]

Comment on attachment 742488 [details] [diff] [review]
nsiObservers, inbatch flag

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

Great!  Just one minor thing that I didn't think of in my last review, sorry:

::: browser/metro/base/content/history.js
@@ +76,5 @@
>    },
>  
>    onVisit: function(aURI, aVisitID, aTime, aSessionID,
>                      aReferringID, aTransitionType) {
> +    this.refreshAndRepopulate();

I think we should skip this call if this._inBatch is true.

@@ +111,5 @@
>  
> +  onDeleteVisits: function (aURI, aVisitTime, aGUID, aReason, aTransitionType) {
> +    if (aReason ==  Ci.nsINavHistoryObserver.REASON_DELETED) {
> +      Cu.reportError("got REASON_DELETED");
> +      this.refreshAndRepopulate();

Here too, we should bail out if this._inBatch is true.

Comment 14

[Allison Naaktgeboren :ally]

https://hg.mozilla.org/integration/mozilla-inbound/rev/384746174d6d

Comment 15

[Allison Naaktgeboren :ally]

Attachment: followup: missed nits

Didn't see the nit comments until I added the hg link to the bug. >.<

Comment 16

[Allison Naaktgeboren :ally]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54106990fcd8

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/384746174d6d
https://hg.mozilla.org/mozilla-central/rev/54106990fcd8