Issue 3258 - Blockable items: fix Size column

Issue 3258 - Blockable items: fix Size column

[Thomas Greiner, 2015-11-24 14:02:20]

https://codereview.adblockplus.org/29329786/diff/29329797/chrome/content/ui/s...
File chrome/content/ui/sidebar.js (right):

https://codereview.adblockplus.org/29329786/diff/29329797/chrome/content/ui/s...
chrome/content/ui/sidebar.js:316: updateSize();
`getItemSize` no longer returns the result synchronously so why pretend it does?
Instead it should always pass the result to the callback to simplify the logic
by eliminating the conditional recursion.

https://codereview.adblockplus.org/29329786/diff/29329797/lib/requestNotifier.js
File lib/requestNotifier.js (right):

https://codereview.adblockplus.org/29329786/diff/29329797/lib/requestNotifier...
lib/requestNotifier.js:162: * @param {Function} callback  function to be called
with two parameters (x,y)
Suggestion: JSDoc provides a way to document callbacks so we could use that to
avoid putting crucial meta data into the parameter description. However, since I
understand that this may be a bit too much overhead, I'll leave it up to you
whether you want to do that.

See http://usejsdoc.org/tags-param.html#callback-functions

[Wladimir Palant, 2015-11-25 22:22:49]

Some changes due to rebasing here, only sidebar.js really changed.

https://codereview.adblockplus.org/29329786/diff/29329797/chrome/content/ui/s...
File chrome/content/ui/sidebar.js (right):

https://codereview.adblockplus.org/29329786/diff/29329797/chrome/content/ui/s...
chrome/content/ui/sidebar.js:316: updateSize();
On 2015/11/24 14:02:19, Thomas Greiner wrote:
> `getItemSize` no longer returns the result synchronously so why pretend it
does?

Sorting and tree view don't work asynchronously, they need the cached value
synchronously. I made getItemSize() always call the callback rather than return
the result in some cases. Not sure whether this made anything better - now some
callers need to check whether the callback was called synchronously.

https://codereview.adblockplus.org/29329786/diff/29329797/lib/requestNotifier.js
File lib/requestNotifier.js (right):

https://codereview.adblockplus.org/29329786/diff/29329797/lib/requestNotifier...
lib/requestNotifier.js:162: * @param {Function} callback  function to be called
with two parameters (x,y)
On 2015/11/24 14:02:20, Thomas Greiner wrote:
> Suggestion: JSDoc provides a way to document callbacks so we could use that to
> avoid putting crucial meta data into the parameter description. However, since
I
> understand that this may be a bit too much overhead, I'll leave it up to you
> whether you want to do that.

I've seen that but I'd rather not make it too complicated. Might have been
different if this callback were indeed a common type rather than a one-time
thing.

[Wladimir Palant, 2015-11-26 11:09:51]

The new patch cleans up the logic by splitting up getItemSize() into the sync
getCachedItemSize() and async getItemSize() parts. It also fixes two potential
issues: size is only cached if non-null (prevents caching of unknown sizes while
page is loading) and tree row is only invalidated if size is non-null (prevents
infinite recursion if size cannot be retrieved).

[Thomas Greiner, 2015-11-26 13:43:34]

LGTM
Nice approach.