Closed Bug 1115421 Opened 5 years ago Closed 5 years ago

Simplify download annotations handling in the Library

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
Points:
5

Tracking

()

RESOLVED FIXED
Firefox 38
Iteration:
38.3 - 23 Feb
Tracking Status
firefox38 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

The Downloads View in the Library does not need to listen to download annotation changes. This consideration greatly simplifies the current code.

Currently, all the annotations are read when the view is created, and they are used when the element shells (even inactive) are created. At that point, any changes to the annotations may only happen because something happened to a Download object, and we already monitor all these changes through the Downloads API.

The fact that everything can work correctly is confirmed by the fact that, despite we apparently listen to nodeAnnotationChanged notifications, they are never fired by Places, even when the call to setPageAnnotation in DownloadsCommon is made. Adding logging to this function shows nothing:

http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/content/allDownloadsViewOverlay.js#1251

I confirmed that nodeInserted notifications worked correctly instead.

Maybe, the reason is that these notifications are only for bookmarks? It seems they're generated here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistoryResult.cpp#3659

Regardless, this isn't an issue for us since we don't need to listen to begin with.

I don't believe any add-on writes download annotations directly, but we shouldn't explicitly handle the case where one does, and does not use a Download object at the same time.
Attached patch The patch (obsolete) — Splinter Review
Not sure who has the least load between Marco and Mano, requesting the review from both, although one review would probably suffice for this change.

In addition to the considerations in comment 0, in general I changed this to a model where metadata from Places is read only by the overall view, and pushed to the DownloadElementShell. This way, the shells don't need to know about how the information is stored, and the overall view (that knows how and when the annotations are read) can handle all the caching.

I've added several comments to clarify the current logic. Probably this change enables further simplification of how shells are added, removed, and changed, but this can be investigated in another bug.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #8541848 - Flags: review?(mano)
Attachment #8541848 - Flags: review?(mak77)
Attached patch Correction (obsolete) — Splinter Review
There was some confusion between nsIURI and URI string parameters.
Attachment #8541848 - Attachment is obsolete: true
Attachment #8541848 - Flags: review?(mano)
Attachment #8541848 - Flags: review?(mak77)
Attachment #8542175 - Flags: review?(mano)
Attachment #8542175 - Flags: review?(mak77)
(In reply to :Paolo Amadini from comment #0)
> Maybe, the reason is that these notifications are only for bookmarks? It
> seems they're generated here:

yes, it's only for bookmarks, we overlooked that originally.

(In reply to :Paolo Amadini from comment #1)
> In addition to the considerations in comment 0, in general I changed this to
> a model where metadata from Places is read only by the overall view, and
> pushed to the DownloadElementShell. This way, the shells don't need to know
> about how the information is stored, and the overall view (that knows how
> and when the annotations are read) can handle all the caching.

I'd really love if we could split out the nodeAnnotationChanged patch to a separate bug, that would be a very trivial r+ and would unblock bug 1115971 and bug 1115972.
I'm not a big fan of removal + refactoring in the same bug.
Flags: qe-verify-
Flags: firefox-backlog+
r+ on the nodeAnnotationChanged changes, you can land that once moved to a separate bug, and land also bug 1115971 and bug 1115972 with it.

I need more time for the other changes, since we lack testing.
Depends on: 1120429
Attached patch Updated patchSplinter Review
Attachment #8542175 - Attachment is obsolete: true
Attachment #8542175 - Flags: review?(mano)
Attachment #8542175 - Flags: review?(mak77)
Attachment #8547588 - Flags: review?(mak77)
Comment on attachment 8547588 [details] [diff] [review]
Updated patch

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

the approach looks ok, but I'd like to look at it again once my comments are answered

::: browser/components/downloads/content/allDownloadsViewOverlay.js
@@ +271,5 @@
>            this._metaData.filePath = this._dataItem.localFile.path;
>          }
>        } else {
>          try {
> +          this._metaData = JSON.parse(this.placesMetaData.jsonDetails);

is there a specific reason we should jeep jsonDetails as a json in first stance?

below, instead of setting .jsonDetails = annotationValue, we could JSON.parse(annotationValue)...
I don't see the benefit of bringing on a json and having to parse it everytime we need its contents.

Clearly we should then move the try/catch to the setter and here check if there's metadata.

@@ +291,5 @@
>          try {
> +          let targetFile = Cc["@mozilla.org/network/protocol;1?name=file"]
> +                             .getService(Ci.nsIFileProtocolHandler)
> +                             .getFileFromURLSpec(this.placesMetaData
> +                                                     .targetFileURISpec);

I think targetFileSpec is enough (I'd avoid URI)

you could just merge it with the other metadata without creating hierarchies
{
  targetFileSpec,
  state,
  fileSize,
  ...
}

@@ +751,5 @@
> +   * matching session download, and at most one such element shell is created
> +   * for each source URI.
> +   *
> +   * @param url
> +   *        URI string of the Places node for which metadata is requested.

see below, we should uniform notation in Places (use either href or spec)

@@ +754,5 @@
> +   * @param url
> +   *        URI string of the Places node for which metadata is requested.
> +   *
> +   * @return Object of the form { targetFileURISpec, jsonDetails }.
> +   *         Any of the properties may be missing from the object.

see above.

@@ +783,5 @@
>        }
>      }
>  
> +    let metadata = this._cachedPlacesMetaData.get(url);
> +    this._cachedPlacesMetaData.delete(url);

This needs a comment explaining we can now remove it from the cache since the shell is created only once and thus the metadata for a url is requested just once

@@ +797,5 @@
> +  /**
> +   * Read the latest version of the Places metadata for the given URI.
> +   *
> +   * @param url
> +   *        URI string of the Places node for which metadata is requested.

based on recent dev in Places, we are using URI for nsIURI, url for URL() and href (or spec) for uri strings...
It would be nice to keep that naming somehow consistent in new code.

@@ +800,5 @@
> +   * @param url
> +   *        URI string of the Places node for which metadata is requested.
> +   *
> +   * @return Object of the form { targetFileURISpec, jsonDetails }.
> +   *         Any of the properties may be missing from the object.

see above, I think we can just simplify the object here.

@@ +897,5 @@
> +      // associated session download, thus we must read the Places metadata,
> +      // because it will not be obscured by the session download.
> +      let metaData = aPlacesNode
> +                     ? this._getCachedPlacesMetaDataFor(aPlacesNode.uri)
> +                     : null;

nit: for readability I prefer the ? to be associated with the condition, and better showing it is continuing

either
  = cond ? expr
         : expr
or
  = cond ?
      expr : expr

@@ +1023,5 @@
> +      // and a history download, and we are now removing the session download.
> +      // Previously, we did not use the Places metadata because it was obscured
> +      // by the session download. Since this is no longer the case, we have to
> +      // read the latest metadata before removing the session download.
> +      shell.placesMetaData = this._getPlacesMetaDataFor(shell.placesNode.uri);

could we somehow just update the required metaData based on the last state of the dataItem and the reason for the removal?
We should already have the cached metadata, we could save on IO if we already know that info.

@@ +1170,5 @@
> +    // When the container is invalidated, it means we are about to re-read all
> +    // the information about history downloads from the database, discarding the
> +    // download element shells, thus we fully rebuild the Places annotation
> +    // cache with the latest values.
> +    this._cachedPlacesMetaData = null;

why do we need to do this, I'm not sure an invalidateContainer call would mean downloads metadata is invalid, it is only related to places nodes.

We were not doing this before, do you think it was a miss?

I see that this would allow to drop a lot of no more valid metadata in case of clear downloads, but before taking it, we should be sure there's no unexpected invalidateContainer traffic... for example when the left pane in the library refreshes, it will invalidate the right pane and we'll lose all of the cached annos even if nothing related changed.
Attachment #8547588 - Flags: review?(mak77) → feedback+
Hi Paolo, can you provide a point value.
Iteration: --- → 38.1 - 26 Jan
Flags: needinfo?(paolo.mozmail)
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #6)
> > +          this._metaData = JSON.parse(this.placesMetaData.jsonDetails);
> 
> is there a specific reason we should jeep jsonDetails as a json in first
> stance?
> you could just merge it with the other metadata without creating hierarchies

Basically it's because the existing code mutates _metaData once it has obtained it, and we'd need to clone the object otherwise, that is more code.

Note that this requirement goes away in one of the next patches in the queue, so I can easily move the point where we do the JSON.parse and create a flat object in a later patch.

> > +    let metadata = this._cachedPlacesMetaData.get(url);
> > +    this._cachedPlacesMetaData.delete(url);
> 
> This needs a comment explaining we can now remove it from the cache since
> the shell is created only once and thus the metadata for a url is requested
> just once

Will do, it's also in the function description if I remember correctly.

> @@ +797,5 @@
> > +   * @param url
> > +   *        URI string of the Places node for which metadata is requested.
> 
> we are using URI for nsIURI, url for URL() and href (or spec) for uri strings...

Ah, "url" for URI strings is consistently used in Downloads.jsm. It seems the invention of URL() has displaced it...

I should probably use "spec" here anyways since this is Places-related code. Not sure about other code where we read "download.source.url" though.

> could we somehow just update the required metaData based on the last state
> of the dataItem and the reason for the removal?

This is a good idea. Actually, this is also much simpler to do on top of the latest patch rather than at this intermediate stage.

> why do we need to do this, I'm not sure an invalidateContainer call would
> mean downloads metadata is invalid, it is only related to places nodes.

By now the cache should be mostly empty, so if we don't repopulate it, we'd access the metadata for the downloads one by one, with a performance impact.

> We were not doing this before, do you think it was a miss?

I don't think we emptied the main cache before (we kept unused entries in memory) so I don't think this was an issue, or if there was a performance issue was caused by other interactions.

> I see that this would allow to drop a lot of no more valid metadata in case
> of clear downloads, but before taking it, we should be sure there's no
> unexpected invalidateContainer traffic... for example when the left pane in
> the library refreshes, it will invalidate the right pane and we'll lose all
> of the cached annos even if nothing related changed.

Exactly, see above. Does it make sense?
Points: --- → 5
Flags: needinfo?(paolo.mozmail)
well, what I'm worried about is that IIRC before we were not emptying the annotations cache on use, nor on invalidation, that means on a refresh we didn't have to reload all of the annotations (could be hundreds or thousands of them).
I'm not sure what's the added IO traffic though, it's something to test out.
Attached patch On latest trunkSplinter Review
Iteration: 38.1 - 26 Jan → 38.2 - 9 Feb
Blocks: 1129896
Iteration: 38.2 - 9 Feb → 38.3 - 23 Feb
Attachment #8547588 - Flags: feedback?(smacleod)
Attachment #8547588 - Flags: feedback?(jaws)
Comment on attachment 8547588 [details] [diff] [review]
Updated patch

Review comments addressed in bug 1129896.
Attachment #8547588 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9a9285ff8387
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Attachment #8547588 - Flags: feedback?(jaws) → feedback+
Attachment #8547588 - Flags: feedback?(smacleod) → feedback+
You need to log in before you can comment on or make changes to this bug.