Issue 3890 - Fix "Downloading..." indication for subscriptions on the options page

Issue 3890 - Fix "Downloading..." indication for subscriptions on the options page

[Sebastian Noack, 2016-04-05 18:38:49]

https://codereview.adblockplus.org/29339387/diff/29339388/background.js
File background.js (left):

https://codereview.adblockplus.org/29339387/diff/29339388/background.js#oldco...
background.js:290: subscription.lastDownload = 0;
This was wrong. The lastDownload property isn't reset by the core code when the
download starts. However, "subscription.downloading" is dispatched.

https://codereview.adblockplus.org/29339387/diff/29339388/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29339387/diff/29339388/messageResponder.js...
messageResponder.js:71: obj.isDownloading =
Synchronizer.isExecuting(subscription);
We require a property that indicates whether the subscription is currently
downloading, as the UI only updates the subscription if any property changed.

https://codereview.adblockplus.org/29339387/diff/29339388/options.js
File options.js (right):

https://codereview.adblockplus.org/29339387/diff/29339388/options.js#newcode414
options.js:414: Object.keys(subscription).forEach(function(property)
We would have to add "isDownloading" and "downloadStatus" to the list. However,
if we simply observe all properties, we don't need any hard-coded list, and the
behavior is also more consistent with Object.observe().

https://codereview.adblockplus.org/29339387/diff/29339388/options.js#newcode416
options.js:416: var value = subscription[property];
This is unrelated, but why using a prefixed key, if we can just have closure
variable. That simplifies the logic below quite a bit, and minimizes potential
side effects.

[kzar, 2016-04-06 08:53:38]

IMHO we should get rid of the Object.observe stuff completely, that way we could
skip adding the isDownloading property and just listen for the "downloading"
event instead. Perhaps it's too much to change with this issue though?

https://codereview.adblockplus.org/29339387/diff/29339388/background.js
File background.js (right):

https://codereview.adblockplus.org/29339387/diff/29339388/background.js#newco...
background.js:291:
modules.filterNotifier.FilterNotifier.emit("subscription.downloading",
subscription);
Nit: Mind breaking this long line at the comma?

[Sebastian Noack, 2016-04-06 09:13:01]

On 2016/04/06 08:53:38, kzar wrote:
> IMHO we should get rid of the Object.observe stuff completely

I agree, that the Object.observe() hack seems to make things unnecessary
complicated. However, changing it at the current point is far less from trivial
(I already gave it a try). IMO, we should still do it at some point, but it will
be a major refactoring, and is arguably unrelated to this fix.

> and just listen for the "downloading" event instead.

Either way, just listening for the "downloading" event isn't sufficient. The
subscription might already/still be loading when the options page is initially
loaded. That case isn't too uncommon on slow internet connections. Also it's
theoretically possible that other subscription changes occur while the
subscription is loading. So we have to check whether it's downloading everytime
we update the subscription on the page.

https://codereview.adblockplus.org/29339387/diff/29339388/background.js
File background.js (right):

https://codereview.adblockplus.org/29339387/diff/29339388/background.js#newco...
background.js:291:
modules.filterNotifier.FilterNotifier.emit("subscription.downloading",
subscription);
On 2016/04/06 08:53:37, kzar wrote:
> Nit: Mind breaking this long line at the comma?

Done.

[kzar, 2016-04-06 09:25:12]

> Either way, just listening for the "downloading" event isn't sufficient. The
subscription might already/still be loading when the options page is initially
loaded. That case isn't too uncommon on slow internet connections. Also it's
theoretically possible that other subscription changes occur while the
subscription is loading. So we have to check whether it's downloading everytime
we update the subscription on the page.

Ah right, I didn't think of that!

LGTM

[Thomas Greiner, 2016-04-06 14:19:43]

>> IMHO we should get rid of the Object.observe stuff completely
> I agree, that the Object.observe() hack seems to make things unnecessary
complicated. However, changing it at the current point is far less from trivial
(I already gave it a try). IMO, we should still do it at some point, but it will
be a major refactoring, and is arguably unrelated to this fix.

Yes, we definitely need to get rid of `Object.observe()` at some point since it
has been deprecated in the meantime in favor of `Proxy`. The fallback solution
was merely for browsers that didn't support `Object.observe()` yet and should
therefore also be removed at that point.

No need to do that in this review though, I agree.

https://codereview.adblockplus.org/29339387/diff/29339407/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29339387/diff/29339407/messageResponder.js...
messageResponder.js:71: obj.isDownloading =
Synchronizer.isExecuting(subscription);
`Synchronizer.isExecuting()` is expecting a URL but you're passing in an object
so that shouldn't work from what I see.

https://codereview.adblockplus.org/29339387/diff/29339407/options.js
File options.js (right):

https://codereview.adblockplus.org/29339387/diff/29339407/options.js#newcode416
options.js:416: var value = subscription[property];
Well spotted.

[Sebastian Noack, 2016-04-06 17:22:23]

https://codereview.adblockplus.org/29339387/diff/29339407/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29339387/diff/29339407/messageResponder.js...
messageResponder.js:71: obj.isDownloading =
Synchronizer.isExecuting(subscription);
On 2016/04/06 14:19:43, Thomas Greiner wrote:
> `Synchronizer.isExecuting()` is expecting a URL but you're passing in an
object
> so that shouldn't work from what I see.

Well spotted. Done.

https://codereview.adblockplus.org/29339387/diff/29339515/options.js
File options.js (right):

https://codereview.adblockplus.org/29339387/diff/29339515/options.js#newcode914
options.js:914: getPref("subscriptions_exceptionsurl", function(value)
Please ignore this change. It's not part of this patch, but due to rebasing.

[Thomas Greiner, 2016-04-06 18:08:17]

LGTM