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

Issue 29338764: Issue 3842 - Split up the logic updating the icon and context menu (Closed)

Created:
March 19, 2016, 8:33 p.m. by Sebastian Noack
Modified:
March 23, 2016, 1:33 p.m.
Reviewers:
kzar
Visibility:
Public.

Description

Issue 3842 - Split up the logic updating the icon and context menu

Patch Set 1 #

Total comments: 2

Patch Set 2 : Prevent visible delays #

Patch Set 3 : Don't revalidate when multiple filter changes occur at the same time #

Patch Set 4 : Reorder cases #

Patch Set 5 : Rebased #

Total comments: 2

Patch Set 6 : Rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+106 lines, -62 lines) Patch
M background.js View 1 2 3 4 5 2 chunks +0 lines, -43 lines 0 comments Download
M lib/filterComposer.js View 2 chunks +53 lines, -4 lines 0 comments Download
M lib/icon.js View 3 chunks +12 lines, -14 lines 0 comments Download
M lib/whitelisting.js View 1 2 3 4 5 2 chunks +40 lines, -0 lines 0 comments Download
M popup.js View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 8
Sebastian Noack
March 19, 2016, 8:34 p.m. (2016-03-19 20:34:31 UTC) #1
kzar
LGTM https://codereview.adblockplus.org/29338764/diff/29338765/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29338764/diff/29338765/lib/filterComposer.js#newcode171 lib/filterComposer.js:171: if (!filter && Prefs.shouldShowBlockElementMenu && readyPages.has(page)) (Maybe we ...
March 20, 2016, 12:51 p.m. (2016-03-20 12:51:06 UTC) #2
Sebastian Noack
https://codereview.adblockplus.org/29338764/diff/29338765/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29338764/diff/29338765/lib/filterComposer.js#newcode171 lib/filterComposer.js:171: if (!filter && Prefs.shouldShowBlockElementMenu && readyPages.has(page)) On 2016/03/20 12:51:06, ...
March 20, 2016, 3:12 p.m. (2016-03-20 15:12:01 UTC) #3
Sebastian Noack
I realized that when disabling/enabling Adblock Plus for the current website from the icon popup ...
March 21, 2016, 10:16 a.m. (2016-03-21 10:16:17 UTC) #4
Sebastian Noack
Sorry, I just realized that with patch set #2 we trigger revalidation for each removed ...
March 21, 2016, 10:47 a.m. (2016-03-21 10:47:11 UTC) #5
kzar
LGTM
March 21, 2016, 11:04 a.m. (2016-03-21 11:04:55 UTC) #6
Sebastian Noack
https://codereview.adblockplus.org/29338764/diff/29338882/popup.js File popup.js (right): https://codereview.adblockplus.org/29338764/diff/29338882/popup.js#newcode39 popup.js:39: else if (!require("filterComposer").isPageReady(page)) The changes below are from rebasing. ...
March 21, 2016, 10:10 p.m. (2016-03-21 22:10:04 UTC) #7
kzar
March 22, 2016, 7:06 a.m. (2016-03-22 07:06:04 UTC) #8
LGTM

https://codereview.adblockplus.org/29338764/diff/29338882/popup.js
File popup.js (right):

https://codereview.adblockplus.org/29338764/diff/29338882/popup.js#newcode39
popup.js:39: else if (!require("filterComposer").isPageReady(page))
On 2016/03/21 22:10:03, Sebastian Noack wrote:
> The changes below are from rebasing. However, while palant insisted to make
> require("messaging") inline on the other review, I did the same for
> require("filterComposer") here while rebasing.

Acknowledged.

Powered by Google App Engine
This is Rietveld