Issue 3200 - Delay initialization for prerendered documents

Issue 3200 - Delay initialization for prerendered documents

Lazily instantiate the message proxy for a Page object in the background page; wait until a prerendered document is made visible in the content script.

[Manish Jethani, 2017-05-11 19:15:33]

Patch Set 1

[Manish Jethani, 2017-05-12 13:33:52]

Patch Set 2: Delay composer.ready until document is visible

This fixes the issue of the "Block element" option not appearing in the popup
for prerendered pages.

The general problem here is that we're trying to send messages to the
background/popup page from the content script, and it's failing because the page
property of the tab is undefined. I feel like there will have to be a more
general fix for this that works for all messages, e.g. queue up messages
automatically until the document becomes visible.

[kzar, 2017-05-19 12:33:05]

Otherwise LGTM

https://codereview.adblockplus.org/29436567/diff/29437573/composer.postload.js
File composer.postload.js (right):

https://codereview.adblockplus.org/29436567/diff/29437573/composer.postload.j...
composer.postload.js:588: document.addEventListener("visibilitychange", function
onVisibilitychange()
Nit: Long line.

Also it's kind of unusual how you've put the named function inside the call to
addEventListener and then referenced that name from inside the callback. Mind
putting it above instead? (Same in include.preload.js.)

https://codereview.adblockplus.org/29436567/diff/29437573/safari/ext/backgrou...
File safari/ext/background.js (right):

https://codereview.adblockplus.org/29436567/diff/29437573/safari/ext/backgrou...
safari/ext/background.js:191: this.contextMenus = new ContextMenus(this);
I guess we should assign this._messageProxy to undefined (or null?) here
explicitly, since the modified logic in _getMessageProxy will sometimes return
that?

[Manish Jethani, 2017-05-19 23:58:48]

Patch Set 3: Address comments to Patch Set 2

https://codereview.adblockplus.org/29436567/diff/29437573/composer.postload.js
File composer.postload.js (right):

https://codereview.adblockplus.org/29436567/diff/29437573/composer.postload.j...
composer.postload.js:588: document.addEventListener("visibilitychange", function
onVisibilitychange()
On 2017/05/19 12:33:05, kzar wrote:
> Nit: Long line.
> 
> Also it's kind of unusual how you've put the named function inside the call to
> addEventListener and then referenced that name from inside the callback. Mind
> putting it above instead? (Same in include.preload.js.)

Sure, I wasn't sure what the preferred style was.

https://codereview.adblockplus.org/29436567/diff/29437573/safari/ext/backgrou...
File safari/ext/background.js (right):

https://codereview.adblockplus.org/29436567/diff/29437573/safari/ext/backgrou...
safari/ext/background.js:191: this.contextMenus = new ContextMenus(this);
On 2017/05/19 12:33:05, kzar wrote:
> I guess we should assign this._messageProxy to undefined (or null?) here
> explicitly, since the modified logic in _getMessageProxy will sometimes return
> that?

Yeah, it'll always return undefined or a non-null object. I think I'll just
assign it to null just to make it clear.

[kzar, 2017-05-20 10:24:00]

LGTM

[Sebastian Noack, 2017-05-20 11:19:06]

Just to clarify, this patch has been tested against the "safari" bookmark, and
is supposed to land there, right?

https://codereview.adblockplus.org/29436567/diff/29442761/composer.postload.js
File composer.postload.js (right):

https://codereview.adblockplus.org/29436567/diff/29442761/composer.postload.j...
composer.postload.js:586: if (document.visibilityState == "prerender")
Note that the visibility API doesn't exists before Safari 7. But that shouldn't
be a problem because this check (and the one in include.preload.js) will just
evaluate to false and the behavior will remain the same as before on Safari 6,
where there is no document.visibilityState, right? And since Safari 6 doesn't do
prerendering it shouldn't be effected by the bug we address here, anyway.

[Manish Jethani, 2017-05-20 19:00:58]

On 2017/05/20 11:19:06, Sebastian Noack wrote:
> Just to clarify, this patch has been tested against the "safari" bookmark, and
> is supposed to land there, right?

Yes, this is for the safari bookmark.

[Manish Jethani, 2017-05-20 19:01:06]

https://codereview.adblockplus.org/29436567/diff/29442761/composer.postload.js
File composer.postload.js (right):

https://codereview.adblockplus.org/29436567/diff/29442761/composer.postload.j...
composer.postload.js:586: if (document.visibilityState == "prerender")
On 2017/05/20 11:19:06, Sebastian Noack wrote:
> Note that the visibility API doesn't exists before Safari 7. But that
shouldn't
> be a problem because this check (and the one in include.preload.js) will just
> evaluate to false and the behavior will remain the same as before on Safari 6,
> where there is no document.visibilityState, right? And since Safari 6 doesn't
do
> prerendering it shouldn't be effected by the bug we address here, anyway.

Yes, then visibilityState should just be undefined. If it's anything other than
"prerender" it'll just do what it's been doing so far.

The "prerender" value was added back in Safari 7 so I think this'll fix the
issue for all versions that have this issue.

https://developer.apple.com/library/content/releasenotes/General/WhatsNewInSa...

[Sebastian Noack, 2017-05-21 20:31:01]

https://codereview.adblockplus.org/29436567/diff/29442761/composer.postload.js
File composer.postload.js (right):

https://codereview.adblockplus.org/29436567/diff/29442761/composer.postload.j...
composer.postload.js:586: if (document.visibilityState == "prerender")
On 2017/05/20 19:01:06, Manish Jethani wrote:
> On 2017/05/20 11:19:06, Sebastian Noack wrote:
> > Note that the visibility API doesn't exists before Safari 7. But that
> shouldn't
> > be a problem because this check (and the one in include.preload.js) will
just
> > evaluate to false and the behavior will remain the same as before on Safari
6,
> > where there is no document.visibilityState, right? And since Safari 6
doesn't
> do
> > prerendering it shouldn't be effected by the bug we address here, anyway.
> 
> Yes, then visibilityState should just be undefined. If it's anything other
than
> "prerender" it'll just do what it's been doing so far.
> 
> The "prerender" value was added back in Safari 7 so I think this'll fix the
> issue for all versions that have this issue.
> 
>
https://developer.apple.com/library/content/releasenotes/General/WhatsNewInSa...

Yeah, I supposed so. Thanks for confirming my assumption. However, it is not
great to duplicate this logic. Note that composer.postload.js runs after
include.preload.js in the same context. So how about moving the logic tat runs
code as soon as the document visible into a function?

https://codereview.adblockplus.org/29436567/diff/29442761/composer.postload.j...
composer.postload.js:591: 
Nit: The blank line here looks weird.

[Manish Jethani, 2017-08-24 14:07:58]

Patch Set 4: Move common logic into separate function

Also rebased.

https://codereview.adblockplus.org/29436567/diff/29442761/composer.postload.js
File composer.postload.js (right):

https://codereview.adblockplus.org/29436567/diff/29442761/composer.postload.j...
composer.postload.js:586: if (document.visibilityState == "prerender")
On 2017/05/21 20:31:01, Sebastian Noack wrote:
> On 2017/05/20 19:01:06, Manish Jethani wrote:
> > On 2017/05/20 11:19:06, Sebastian Noack wrote:
> > > Note that the visibility API doesn't exists before Safari 7. But that
> > shouldn't
> > > be a problem because this check (and the one in include.preload.js) will
> just
> > > evaluate to false and the behavior will remain the same as before on
Safari
> 6,
> > > where there is no document.visibilityState, right? And since Safari 6
> doesn't
> > do
> > > prerendering it shouldn't be effected by the bug we address here, anyway.
> > 
> > Yes, then visibilityState should just be undefined. If it's anything other
> than
> > "prerender" it'll just do what it's been doing so far.
> > 
> > The "prerender" value was added back in Safari 7 so I think this'll fix the
> > issue for all versions that have this issue.
> > 
> >
>
https://developer.apple.com/library/content/releasenotes/General/WhatsNewInSa...
> 
> Yeah, I supposed so. Thanks for confirming my assumption. However, it is not
> great to duplicate this logic. Note that composer.postload.js runs after
> include.preload.js in the same context. So how about moving the logic tat runs
> code as soon as the document visible into a function?

Done.

https://codereview.adblockplus.org/29436567/diff/29442761/composer.postload.j...
composer.postload.js:591: 
On 2017/05/21 20:31:01, Sebastian Noack wrote:
> Nit: The blank line here looks weird.

Acknowledged.

[Sebastian Noack, 2017-08-24 14:09:29]

LGTM. kzar, can you have a look as well?

[kzar, 2017-08-29 13:07:00]

LGTM