Issue 2217 - Adapt for content scripts running in about:blank frames

Issue 2217 - Adapt for content scripts running in about:blank frames

[kzar, 2015-03-26 12:07:57]

http://codereview.adblockplus.org/4883433876619264/diff/5629499534213120/incl...
File include.preload.js (right):

http://codereview.adblockplus.org/4883433876619264/diff/5629499534213120/incl...
include.preload.js:269: if (contentDocument instanceof
contentWindow.HTMLDocument)
Perhaps we should check that contentWindow isn't null first? According to the
MDN it's possible
https://developer.mozilla.org/en-US/docs/Web/API/Document/defaultView

http://codereview.adblockplus.org/4883433876619264/diff/5629499534213120/incl...
include.preload.js:283:
[].forEach.call(contentDocument.querySelectorAll(Object.keys(typeMap).join(",")),
checkCollapse);
Couldn't we put this forEach loop under the above `if (!("init" in
contentWindow))` clause? I don't understand what assigning and checking the
collapsing variable adds over just checking that the init function exists. I'm
probably missing something.

[Sebastian Noack, 2015-03-26 13:41:19]

http://codereview.adblockplus.org/4883433876619264/diff/5629499534213120/incl...
File include.preload.js (right):

http://codereview.adblockplus.org/4883433876619264/diff/5629499534213120/incl...
include.preload.js:269: if (contentDocument instanceof
contentWindow.HTMLDocument)
On 2015/03/26 12:07:57, kzar wrote:
> Perhaps we should check that contentWindow isn't null first? According to the
> MDN it's possible
> https://developer.mozilla.org/en-US/docs/Web/API/Document/defaultView

I think we can assume frames having a document, also having window (we also did
before). AFAIK, the only case where a document won't have a window is when you
manually create it, e.g. with document.implementation.createDocument().

http://codereview.adblockplus.org/4883433876619264/diff/5629499534213120/incl...
include.preload.js:283:
[].forEach.call(contentDocument.querySelectorAll(Object.keys(typeMap).join(",")),
checkCollapse);
On 2015/03/26 12:07:57, kzar wrote:
> Couldn't we put this forEach loop under the above `if (!("init" in
> contentWindow))` clause? I don't understand what assigning and checking the
> collapsing variable adds over just checking that the init function exists. I'm
> probably missing something.

We are dealing two different issues here:

1. The content script isn't running in the frame. Hence we have to manually
initialize element hiding there.

2. The frame doesn't dispatch load/error events for its elements. So we have to
check for all elements in the loaded frame to potentially collapse them. This
Chrome bug isn't fixed yet, and occurs also for frames running our content
scripts. Therfore this workaround must be performed regardless of the one above.

[kzar, 2015-03-26 14:14:58]

LGTM

http://codereview.adblockplus.org/4883433876619264/diff/5629499534213120/incl...
File include.preload.js (right):

http://codereview.adblockplus.org/4883433876619264/diff/5629499534213120/incl...
include.preload.js:283:
[].forEach.call(contentDocument.querySelectorAll(Object.keys(typeMap).join(",")),
checkCollapse);
On 2015/03/26 13:41:19, Sebastian Noack wrote:
> On 2015/03/26 12:07:57, kzar wrote:
> > Couldn't we put this forEach loop under the above `if (!("init" in
> > contentWindow))` clause? I don't understand what assigning and checking the
> > collapsing variable adds over just checking that the init function exists.
I'm
> > probably missing something.
> 
> We are dealing two different issues here:
> 
> 1. The content script isn't running in the frame. Hence we have to manually
> initialize element hiding there.
> 
> 2. The frame doesn't dispatch load/error events for its elements. So we have
to
> check for all elements in the loaded frame to potentially collapse them. This
> Chrome bug isn't fixed yet, and occurs also for frames running our content
> scripts. Therfore this workaround must be performed regardless of the one
above.

OK, thanks for explaining.