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

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

[kzar, 2016-03-20 12:51:06]

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....
lib/filterComposer.js:171: if (!filter && Prefs.shouldShowBlockElementMenu &&
readyPages.has(page))
(Maybe we should first check the preference before calling checkWhitelisted when
the filter parameter wasn't passed. (It's wasteful to check if the page is
whitelisted when the feature is disabled anyway.) On the other hand it would
make the logic here slightly messier and I'm not sure how much it would improve
the performance. I leave it up to you.)

[Sebastian Noack, 2016-03-20 15:12:01]

https://codereview.adblockplus.org/29338764/diff/29338765/lib/filterComposer.js
File lib/filterComposer.js (right):

https://codereview.adblockplus.org/29338764/diff/29338765/lib/filterComposer....
lib/filterComposer.js:171: if (!filter && Prefs.shouldShowBlockElementMenu &&
readyPages.has(page))
On 2016/03/20 12:51:06, kzar wrote:
> (Maybe we should first check the preference before calling checkWhitelisted
when
> the filter parameter wasn't passed. (It's wasteful to check if the page is
> whitelisted when the feature is disabled anyway.) On the other hand it would
> make the logic here slightly messier and I'm not sure how much it would
improve
> the performance. I leave it up to you.)

As you said, that micro optimization would make the logic more complicated here.
And that preference set to false isn't a particular common scenario.

[Sebastian Noack, 2016-03-21 10:16:17]

I realized that when disabling/enabling Adblock Plus for the current website
from the icon popup there was a well visible delay until the icon got updated.
This issue wasn't new, though it subjectively got a little worse with the
changes here. Anyway, I figured the reason was that we wait until the filter
changes got saved to disk before revalidating the whitelisting state. However,
revalidating the whitelisting state as soon as any filter is
added/removed/disabled eliminates that delay. So here is a new patch set.

[Sebastian Noack, 2016-03-21 10:47:11]

Sorry, I just realized that with patch set #2 we trigger revalidation for each
removed and added filter when saving filters in raw mode. So here is another
patch set. Also slightly simpler than my previous attempt.

[kzar, 2016-03-21 11:04:55]

LGTM

[Sebastian Noack, 2016-03-21 22:10:04]

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. However, while palant insisted to make
require("messaging") inline on the other review, I did the same for
require("filterComposer") here while rebasing.

[kzar, 2016-03-22 07:06:04]

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.