827405 - Lazily "activate" download items when they become visible in the richlistbox area

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect, P1)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Discussed briefly with Mano and we came to the conclusion this is a good first step in the lazy direction before taking decision on bug 825846.
Regardless the fact we fix the widget performances or fake it by loading stuff in chunks, in both cases we need to avoid stat()ing thousands of files for nothing.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

This seems to do the job quite fine, the only hack I don't like much is the mainthread enqueueing, but in the end it sounds safer to let layout change before recalculating everything.
Let me know what you think.

Comment 2

[Asaf Romano (gone)]

Comment on attachment 698955 [details] [diff] [review]
patch v1.0

>- if (aAnnotations)
>+  if (aAnnotations)
>     this._annotations = aAnnotations;
>-  if (aDataItem)
>+  if (aDataItem) {
>+    this._active = true;
>     this.dataItem = aDataItem;
>+  }
>   if (aPlacesNode)
>     this.placesNode = aPlacesNode;
> }
> 
> DownloadElementShell.prototype = {
>   // The richlistitem for the download
>   get element() this._element,
> 
>+  /**
>+   * Manages the "active" state of the shell.  By default all the shells
>+   * without a dataitem are inactive, thus their UI is not updated.

State explicitly that session-downloads are automatically activated.

>+   * be activated when entering the visible area.
>+   */
>+  set active(val) {

I'd rather go with a simple "activate" method, because we're not got to deactivate shells. It's not worth the trouble.

>+      this._updateStatusUI();
>+      this._fetchTargetFileInfo();
>+    }
>+    return this._active;
>+  },
>+  get active() !!this._active,
>+
>   // The data item for the download
>   _dataItem: null,
>   get dataItem() this._dataItem,
> 
>   set dataItem(aValue) {

Setting the data item at any point should activate the shell, not just in the constructor.

>   _fetchTargetFileInfo: function DES__fetchTargetFileInfo() {
>     if (this._targetFileInfoFetched)
>       throw new Error("_fetchTargetFileInfo should not be called if the information was already fetched");
>+    if (!this.active)
>+      return;

Hrm, it seems more like the caller's job.

>   _updateStatusUI: function DES__updateStatusUI() {
>+    if (!this.active)
>+      return;

Ditto.

>   // Returns whether or not the download handled by this shell should
>   // show up in the search results for the given term.  Both the display
>   // name for the download and the url are searched.
>   matchesSearchTerm: function DES_matchesSearchTerm(aTerm) {
>     // Stub implemention until we figure out something better
>+    if (!aTerm)
>+      return true;

Please remove my comment while you're there. Both because it has a typo and because we're not going to figure out something better.

>+  // Resizing the window may change items visibility.
>+  window.addEventListener("resize", function() {
>+    this._ensureVisibleElementsAreActive();
>+  }.bind(this), true);
> }
...
>+  _ensureVisibleElementsAreActive:
>+  function DPV__ensureVisibleElementsAreActive() {
>+    if (!this._richlistbox.firstChild)
>+      return;
>+
>+    // When the view location is set, the details pane gets hidden and the
>+    // richlistbox size changes.  So ensure to run after any initialization.
>+    Services.tm.mainThread.dispatch(function() {

I deliberately made it so this view initializes once for the window, but it turns out we do need some kind of "active" property on the view as well, to be altered by the places organizer shows the view.

That'd avoid the need for both the need for this hack and the unnecessary work we'd do when the places organizer is resized as it shows some bookmarks.

The "active" property should default to true so views that do not need it may just return true. However, because we need this view to be inactive when it's initialized, please add it as a constructor argument (Note that the in-content view should set it true).

>+      let rbo = this._richlistbox.boxObject;
>+      let fbo = this._richlistbox.firstChild.boxObject;

As you're already using the standard elementFromPoint method, let's go with getBoundingClientRect.

>   nodeInserted: function DPV_nodeInserted(aParent, aPlacesNode) {
>     this._addDownloadData(null, aPlacesNode);
>+    this._ensureVisibleElementsAreActive();
>   },
> 
>   nodeRemoved: function DPV_nodeRemoved(aParent, aPlacesNode, aOldIndex) {
>     this._removeHistoryDownloadFromView(aPlacesNode);
>+    this._ensureVisibleElementsAreActive();
>   },
> 

_addDownloadData and _removeHistoryDownloadFromView seem like better entry points.

In _addDownloadData you can check if the dom fragment is set in order to leave the "activate"
work to invalidateContainer.

>   },
> 
>   onDataItemRemoved: function DPV_onDataItemRemoved(aDataItem) {
>     this._removeSessionDownloadFromView(aDataItem);
>+    this._ensureVisibleElementsAreActive();
>   },
>

This belongs to _removeSessionDownloadFromView.

>   onDataItemAdded: function DPV_onDataItemAdded(aDataItem, aNewest) {
>     this._addDownloadData(aDataItem, null, aNewest);
>+    this._ensureVisibleElementsAreActive();

This shouldn't be necessary. Session downloads are pushing away history downloads.

>+
>+  onScroll: function DPV_onScroll()
>+  {

Nit: move the brace to its place

Comment 3

[Asaf Romano (gone)]

Attachment: the "active" property i was thinking of

As discussed over IRC.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: wip

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.1

I think I covered everything we discussed over IRC

Comment 6

[Asaf Romano (gone)]

Comment on attachment 699194 [details] [diff] [review]
patch v1.1

>+      // If the found last visible child is not a richlistitem, then there are
"If the last visible child found"

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.2

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/303b3bb5f27f

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/303b3bb5f27f

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 699264 [details] [diff] [review]
patch v1.2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Downloads panel feature
User impact if declined: Performance issues
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): limited to the feature
String or UUID changes made by this patch: none

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6ec8ebb4f02e