Issue 3472 - Made sure that "Block element" option is shown after page load

Issue 3472 - Made sure that "Block element" option is shown after page load

[Sebastian Noack, 2016-01-09 00:01:50]

https://codereview.adblockplus.org/29333344/diff/29333345/popup.js
File popup.js (right):

https://codereview.adblockplus.org/29333344/diff/29333345/popup.js#newcode88
popup.js:88: && backgroundPage.htmlPages.has(page))
This check is redundant. Or if you want to keep it for consistence, please move
that logic including the DOM manipulation below to a function that is called
here as well when checking initially.

[Thomas Greiner, 2016-01-12 11:35:18]

https://codereview.adblockplus.org/29333344/diff/29333345/popup.js
File popup.js (right):

https://codereview.adblockplus.org/29333344/diff/29333345/popup.js#newcode88
popup.js:88: && backgroundPage.htmlPages.has(page))
On 2016/01/09 00:01:50, Sebastian Noack wrote:
> This check is redundant. Or if you want to keep it for consistence, please
move
> that logic including the DOM manipulation below to a function that is called
> here as well when checking initially.

Unfortunately, it's not redundant. Any page could've sent this message so there
needs to be an extra check to ensure that it concerns the page the popup is
associated with.

Generally, I'd like to avoid creating a function for one line of code,
especially since I can't use `document.body.classList.toggle("nohtml",
!backgroundPage.htmlPages.has(page))` due to Safari 6.1. Therefore I'm checking
the origin of the page because comparing `sender.page == page` doesn't work and
implementing it doesn't appear to be worth it.

[Sebastian Noack, 2016-01-12 16:09:17]

https://codereview.adblockplus.org/29333344/diff/29333345/popup.js
File popup.js (right):

https://codereview.adblockplus.org/29333344/diff/29333345/popup.js#newcode88
popup.js:88: && backgroundPage.htmlPages.has(page))
On 2016/01/12 11:35:17, Thomas Greiner wrote:
> On 2016/01/09 00:01:50, Sebastian Noack wrote:
> > This check is redundant. Or if you want to keep it for consistence, please
> move
> > that logic including the DOM manipulation below to a function that is called
> > here as well when checking initially.
> 
> Unfortunately, it's not redundant. Any page could've sent this message so
there
> needs to be an extra check to ensure that it concerns the page the popup is
> associated with.

Acknowledged.

> Generally, I'd like to avoid creating a function for one line of code,
> especially since I can't use `document.body.classList.toggle("nohtml",
> !backgroundPage.htmlPages.has(page))` due to Safari 6.1. Therefore I'm
checking
> the origin of the page because comparing `sender.page == page` doesn't work
and
> implementing it doesn't appear to be worth it.

I don't think that I agree. But it's probably not too important. LGTM