Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(802)

Issue 29436567: Issue 3200 - Element hiding not applied to page preloaded by Safari when typing URL

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 1 week ago by Manish Jethani
Modified:
3 months ago
Reviewers:
Sebastian Noack, kzar
Base URL:
https://hg.adblockplus.org/adblockpluschrome/
Visibility:
Public.

Description

Issue 3200 - Element hiding not applied to page preloaded by Safari when typing URL 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.

Patch Set 1 #

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

Total comments: 4

Patch Set 3 : Address comments to Patch Set 2 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+68 lines, -17 lines) Patch
M composer.postload.js View 1 2 1 chunk +17 lines, -1 line 4 comments Download
M include.preload.js View 1 2 1 chunk +20 lines, -1 line 0 comments Download
M safari/ext/background.js View 1 2 2 chunks +31 lines, -15 lines 0 comments Download

Messages

Total messages: 10
Manish Jethani
3 months, 1 week ago (2017-05-11 19:11:53 UTC) #1
Manish Jethani
Patch Set 1
3 months, 1 week ago (2017-05-11 19:15:33 UTC) #2
Manish Jethani
Patch Set 2: Delay composer.ready until document is visible This fixes the issue of the ...
3 months, 1 week ago (2017-05-12 13:33:52 UTC) #3
kzar
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.js#newcode588 composer.postload.js:588: document.addEventListener("visibilitychange", function onVisibilitychange() Nit: Long line. Also ...
3 months ago (2017-05-19 12:33:05 UTC) #4
Manish Jethani
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.js#newcode588 composer.postload.js:588: ...
3 months ago (2017-05-19 23:58:48 UTC) #5
kzar
LGTM
3 months ago (2017-05-20 10:24:00 UTC) #6
Sebastian Noack
Just to clarify, this patch has been tested against the "safari" bookmark, and is supposed ...
3 months ago (2017-05-20 11:19:06 UTC) #7
Manish Jethani
On 2017/05/20 11:19:06, Sebastian Noack wrote: > Just to clarify, this patch has been tested ...
3 months ago (2017-05-20 19:00:58 UTC) #8
Manish Jethani
https://codereview.adblockplus.org/29436567/diff/29442761/composer.postload.js File composer.postload.js (right): https://codereview.adblockplus.org/29436567/diff/29442761/composer.postload.js#newcode586 composer.postload.js:586: if (document.visibilityState == "prerender") On 2017/05/20 11:19:06, Sebastian Noack ...
3 months ago (2017-05-20 19:01:06 UTC) #9
Sebastian Noack
3 months ago (2017-05-21 20:31:01 UTC) #10
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5