Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29329786: Issue 3258 - Blockable items: fix Size column (Closed)

Created:
Nov. 5, 2015, 9:28 p.m. by Wladimir Palant
Modified:
Nov. 26, 2015, 1:45 p.m.
Visibility:
Public.

Description

Issue 3258 - Blockable items: fix Size column

Patch Set 1 #

Patch Set 2 : Better handling of dead objects #

Total comments: 4

Patch Set 3 : Made getItemSize always call the callback #

Patch Set 4 : Cleaner approach: split up getItemSize into sync and async parts #

Unified diffs Side-by-side diffs Delta from patch set Stats (+143 lines, -18 lines) Patch
M chrome/content/ui/sidebar.js View 1 2 3 6 chunks +49 lines, -18 lines 0 comments Download
M lib/child/requestNotifier.js View 1 2 3 3 chunks +52 lines, -0 lines 0 comments Download
M lib/requestNotifier.js View 2 chunks +42 lines, -0 lines 0 comments Download

Messages

Total messages: 5
Wladimir Palant
Nov. 5, 2015, 9:28 p.m. (2015-11-05 21:28:38 UTC) #1
Thomas Greiner
https://codereview.adblockplus.org/29329786/diff/29329797/chrome/content/ui/sidebar.js File chrome/content/ui/sidebar.js (right): https://codereview.adblockplus.org/29329786/diff/29329797/chrome/content/ui/sidebar.js#newcode316 chrome/content/ui/sidebar.js:316: updateSize(); `getItemSize` no longer returns the result synchronously so ...
Nov. 24, 2015, 2:02 p.m. (2015-11-24 14:02:20 UTC) #2
Wladimir Palant
Some changes due to rebasing here, only sidebar.js really changed. https://codereview.adblockplus.org/29329786/diff/29329797/chrome/content/ui/sidebar.js File chrome/content/ui/sidebar.js (right): https://codereview.adblockplus.org/29329786/diff/29329797/chrome/content/ui/sidebar.js#newcode316 ...
Nov. 25, 2015, 10:22 p.m. (2015-11-25 22:22:49 UTC) #3
Wladimir Palant
The new patch cleans up the logic by splitting up getItemSize() into the sync getCachedItemSize() ...
Nov. 26, 2015, 11:09 a.m. (2015-11-26 11:09:51 UTC) #4
Thomas Greiner
Nov. 26, 2015, 1:43 p.m. (2015-11-26 13:43:34 UTC) #5
LGTM
Nice approach.

Powered by Google App Engine
This is Rietveld