Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(495)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 7 months ago by Wladimir Palant
Modified:
4 years, 6 months ago
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
4 years, 7 months ago (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 ...
4 years, 6 months ago (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 ...
4 years, 6 months ago (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() ...
4 years, 6 months ago (2015-11-26 11:09:51 UTC) #4
Thomas Greiner
4 years, 6 months ago (2015-11-26 13:43:34 UTC) #5
LGTM
Nice approach.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5