[iOS Reading List] Add distilling WKWebView to the view hierarchy.

[iOS Reading List] Add distilling WKWebView to the view hierarchy.

Some pages (e.g. uk.businessinsider.com) are not rendered correctly
unless the view is added in the view hierarchy.
Add temporarily the WKWebView to the view hierarchy during distillation.
Move it out ouf the screen to make it invisible to the user.

BUG=685583
Review-Url: https://codereview.chromium.org/2695313004
Cr-Commit-Position: refs/heads/master@{#451643}
Committed: https://chromium.googlesource.com/chromium/src/+/47b54ff088d9c52ece1c28b06e647e64ec8014d7

[Olivier, 2017-02-16 12:12:37]

olivierrobin@chromium.org changed reviewers:
+ gambard@chromium.org

[gambard, 2017-02-16 12:22:16]

The logic lg tm but I don't know if this will affect performance, especially
knowing we will attach/detach a WKWebView every 2s. You should ask someone more
knowledgeable.

[Olivier, 2017-02-16 12:36:36]

olivierrobin@chromium.org changed reviewers:
+ marq@chromium.org, stkhapugin@chromium.org

[Olivier, 2017-02-16 12:36:37]

+marq, stk for UI expertise.

Does adding the view in window have a performance impact?
Is it better to create a new UIWindow?

Thanks

[stkhapugin, 2017-02-16 12:53:59]

lgtm

https://codereview.chromium.org/2695313004/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/reading_list_distiller_page.mm (right):

https://codereview.chromium.org/2695313004/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:80:
frame.origin.x = 2 * std::max(frame.size.width, frame.size.height);
2 can be too small for iPad multitasking: if you start in slideover portrait
(window width ~= 1/3 of screen width) and switch to full screen, it might be
visible? Not 100% sure if autolayout won't fix it for you by pushing it further
offscreen though. 

I don't have a preference of separate window vs offscreen: in a separate window
there might be a bug in the future where WKWebView doesn't render in it, and
same goes for offscreen. Since we've been using this trick for prerendering,
maybe it's logical to just keep using it.

[Olivier, 2017-02-16 13:04:18]

https://codereview.chromium.org/2695313004/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/reading_list_distiller_page.mm (right):

https://codereview.chromium.org/2695313004/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:80:
frame.origin.x = 2 * std::max(frame.size.width, frame.size.height);
On 2017/02/16 12:53:59, stkhapugin wrote:
> 2 can be too small for iPad multitasking: if you start in slideover portrait
> (window width ~= 1/3 of screen width) and switch to full screen, it might be
> visible? Not 100% sure if autolayout won't fix it for you by pushing it
further
> offscreen though. 
> 
> I don't have a preference of separate window vs offscreen: in a separate
window
> there might be a bug in the future where WKWebView doesn't render in it, and
> same goes for offscreen. Since we've been using this trick for prerendering,
> maybe it's logical to just keep using it. 

I don't know if it prerendering is still done that way.
I will check that

[marq (ping after 24h), 2017-02-16 14:28:18]

https://codereview.chromium.org/2695313004/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/reading_list_distiller_page.mm (right):

https://codereview.chromium.org/2695313004/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:76: // Some pages
require that the WKWebView is in the view hierarchy to be
I don't suppose there's any way to determine which pages these are, so we can
avoid this where possible?

https://codereview.chromium.org/2695313004/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:80:
frame.origin.x = 2 * std::max(frame.size.width, frame.size.height);
On 2017/02/16 13:04:18, Olivier Robin wrote:
> On 2017/02/16 12:53:59, stkhapugin wrote:
> > 2 can be too small for iPad multitasking: if you start in slideover portrait
> > (window width ~= 1/3 of screen width) and switch to full screen, it might be
> > visible? Not 100% sure if autolayout won't fix it for you by pushing it
> further
> > offscreen though. 
> > 
> > I don't have a preference of separate window vs offscreen: in a separate
> window
> > there might be a bug in the future where WKWebView doesn't render in it, and
> > same goes for offscreen. Since we've been using this trick for prerendering,
> > maybe it's logical to just keep using it. 
> 
> I don't know if it prerendering is still done that way.
> I will check that

It might be safer to position the offscreen view in the upper left quadrant of
the coordinate space, so even if the window expands, it will do so down and to
the right relative to the webview.

https://codereview.chromium.org/2695313004/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:81:
[CurrentWebState()->GetView() setFrame:frame];
DCHECK that the webstate's view doesn't already have a superview.

https://codereview.chromium.org/2695313004/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:83:
addSubview:CurrentWebState()->GetView()];
Maybe insert at index 0, so it's behind everything else?

[Olivier, 2017-02-16 14:43:41]

https://codereview.chromium.org/2695313004/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/reading_list_distiller_page.mm (right):

https://codereview.chromium.org/2695313004/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:76: // Some pages
require that the WKWebView is in the view hierarchy to be
On 2017/02/16 14:28:18, marq wrote:
> I don't suppose there's any way to determine which pages these are, so we can
> avoid this where possible?

I don't think we can.
But we can do it only on second try?

https://codereview.chromium.org/2695313004/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:80:
frame.origin.x = 2 * std::max(frame.size.width, frame.size.height);
On 2017/02/16 14:28:18, marq wrote:
> On 2017/02/16 13:04:18, Olivier Robin wrote:
> > On 2017/02/16 12:53:59, stkhapugin wrote:
> > > 2 can be too small for iPad multitasking: if you start in slideover
portrait
> > > (window width ~= 1/3 of screen width) and switch to full screen, it might
be
> > > visible? Not 100% sure if autolayout won't fix it for you by pushing it
> > further
> > > offscreen though. 
> > > 
> > > I don't have a preference of separate window vs offscreen: in a separate
> > window
> > > there might be a bug in the future where WKWebView doesn't render in it,
and
> > > same goes for offscreen. Since we've been using this trick for
prerendering,
> > > maybe it's logical to just keep using it. 
> > 
> > I don't know if it prerendering is still done that way.
> > I will check that
> 
> It might be safer to position the offscreen view in the upper left quadrant of
> the coordinate space, so even if the window expands, it will do so down and to
> the right relative to the webview.

Done.

https://codereview.chromium.org/2695313004/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:81:
[CurrentWebState()->GetView() setFrame:frame];
On 2017/02/16 14:28:18, marq wrote:
> DCHECK that the webstate's view doesn't already have a superview.

Done.

https://codereview.chromium.org/2695313004/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:83:
addSubview:CurrentWebState()->GetView()];
On 2017/02/16 14:28:18, marq wrote:
> Maybe insert at index 0, so it's behind everything else?

Done.

[wychen, 2017-02-16 16:38:58]

wychen@chromium.org changed reviewers:
+ wychen@chromium.org

[wychen, 2017-02-16 16:38:59]

https://codereview.chromium.org/2695313004/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/reading_list_distiller_page.mm (right):

https://codereview.chromium.org/2695313004/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:76: // Some pages
require that the WKWebView is in the view hierarchy to be
On 2017/02/16 14:43:41, Olivier Robin wrote:
> On 2017/02/16 14:28:18, marq wrote:
> > I don't suppose there's any way to determine which pages these are, so we
can
> > avoid this where possible?
> 
> I don't think we can.
> But we can do it only on second try?

You mean if it distills nothing, then distill it again with this trick again? If
the performance impact of putting WebView in the view hierarchy is not too high,
it might not worth the trouble. Distillation itself alone takes ~0.3 seconds.

[Olivier, 2017-02-16 19:48:30]

olivierrobin@chromium.org changed reviewers:
+ eugenebut@chromium.org

[Olivier, 2017-02-16 19:48:30]

+eugenebut as WKWebView expert.

Hi Eugene,
PTAL.
In particular, do you have any preference between adding the WKWebView to the
application window or create a dedicated window.

[Eugene But (OOO till 7-30), 2017-02-16 19:56:14]

Is this reproducible with stock WKWebView? If so, this is lgtm but please file a
radar and add a TODO to cleanup the workaround. If this is Chrome-only problem,
then we should try to understand why this happening.

[Olivier, 2017-02-16 20:02:27]

https://codereview.chromium.org/2695313004/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/reading_list_distiller_page.mm (right):

https://codereview.chromium.org/2695313004/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:76: // Some pages
require that the WKWebView is in the view hierarchy to be
On 2017/02/16 16:38:59, wychen wrote:
> On 2017/02/16 14:43:41, Olivier Robin wrote:
> > On 2017/02/16 14:28:18, marq wrote:
> > > I don't suppose there's any way to determine which pages these are, so we
> can
> > > avoid this where possible?
> > 
> > I don't think we can.
> > But we can do it only on second try?
> 
> You mean if it distills nothing, then distill it again with this trick again?
If
> the performance impact of putting WebView in the view hierarchy is not too
high,
> it might not worth the trouble. Distillation itself alone takes ~0.3 seconds.

We always try multiple time to distill as some network issues can prevent
distillation on first try.
We could do this work around only starting on second try.

[wychen, 2017-02-17 00:59:55]

https://codereview.chromium.org/2695313004/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/reading_list_distiller_page.mm (right):

https://codereview.chromium.org/2695313004/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:76: // Some pages
require that the WKWebView is in the view hierarchy to be
On 2017/02/16 20:02:27, Olivier Robin wrote:
> On 2017/02/16 16:38:59, wychen wrote:
> > On 2017/02/16 14:43:41, Olivier Robin wrote:
> > > On 2017/02/16 14:28:18, marq wrote:
> > > > I don't suppose there's any way to determine which pages these are, so
we
> > can
> > > > avoid this where possible?
> > > 
> > > I don't think we can.
> > > But we can do it only on second try?
> > 
> > You mean if it distills nothing, then distill it again with this trick
again?
> If
> > the performance impact of putting WebView in the view hierarchy is not too
> high,
> > it might not worth the trouble. Distillation itself alone takes ~0.3
seconds.
> 
> We always try multiple time to distill as some network issues can prevent
> distillation on first try.
> We could do this work around only starting on second try.

Makes sense. Thanks!

[Olivier, 2017-02-17 09:20:55]

On 2017/02/16 19:56:14, Eugene But wrote:
> Is this reproducible with stock WKWebView? If so, this is lgtm but please file
a
> radar and add a TODO to cleanup the workaround. If this is Chrome-only
problem,
> then we should try to understand why this happening.

Yes, it is reproducible.

I created an app with vanilla WKWebView loading the page and executing 
document.getElementsByClassName('article-wrap')[0].innerText
after 10 seconds and the result is empty is the page is not in the view
hierarchy.
I will add a todo to investigate more.

But I am not sure what to write in a radar.
Is it not possible for a page to have an event on the page display?
In that case, is it not possible that this particular website only show content
when it is on screen?

[Olivier, 2017-02-17 09:46:17]

On 2017/02/17 09:20:55, Olivier Robin wrote:
> On 2017/02/16 19:56:14, Eugene But wrote:
> > Is this reproducible with stock WKWebView? If so, this is lgtm but please
file
> a
> > radar and add a TODO to cleanup the workaround. If this is Chrome-only
> problem,
> > then we should try to understand why this happening.
> 
> Yes, it is reproducible.
> 
> I created an app with vanilla WKWebView loading the page and executing 
> document.getElementsByClassName('article-wrap')[0].innerText
> after 10 seconds and the result is empty is the page is not in the view
> hierarchy.
> I will add a todo to investigate more.
> 
> But I am not sure what to write in a radar.
> Is it not possible for a page to have an event on the page display?
> In that case, is it not possible that this particular website only show
content
> when it is on screen?

I investigated more.
The page uses document.visibilityState API.
The result is

"visible" if the page is in the view hierarchy
"prerender" if the page is not.

businessinsider uses this value to not render the whole page in prerender mode
(it sets body = hidden on prerender).

[marq (ping after 24h), 2017-02-17 10:33:01]

lgtm

[stkhapugin, 2017-02-17 13:14:43]

lgtm

[Eugene But (OOO till 7-30), 2017-02-17 18:27:41]

Thank you for digging into it. Looks like a feature, not a bug :). Should we set
document.visibilityState to True for DOM distillation? Or just write a comment,
explaining that this code is a workaround to make document.visibilityState
return True?

[Olivier, 2017-02-17 19:39:41]

On 2017/02/17 18:27:41, Eugene But wrote:
> Thank you for digging into it. Looks like a feature, not a bug :). Should we
set
> document.visibilityState to True for DOM distillation? Or just write a
comment,
> explaining that this code is a workaround to make document.visibilityState
> return True?

You cannot directly set visibilityState as it is a readonly webkit property.
However this javascript code works in this parrticular case.

Object.defineProperty(document, 'hidden', {value: false});
Object.defineProperty(document, 'visibilityState', {value: 'visible'});
var event = new Event('visibilitychange');
document.dispatchEvent(event);


I don't know which solution is the best and which is the most hacky one...

[Eugene But (OOO till 7-30), 2017-02-17 19:41:35]

On 2017/02/17 19:39:41, Olivier Robin wrote:
> On 2017/02/17 18:27:41, Eugene But wrote:
> > Thank you for digging into it. Looks like a feature, not a bug :). Should we
> set
> > document.visibilityState to True for DOM distillation? Or just write a
> comment,
> > explaining that this code is a workaround to make document.visibilityState
> > return True?
> 
> You cannot directly set visibilityState as it is a readonly webkit property.
> However this javascript code works in this parrticular case.
> 
> Object.defineProperty(document, 'hidden', {value: false});
> Object.defineProperty(document, 'visibilityState', {value: 'visible'});
> var event = new Event('visibilitychange');
> document.dispatchEvent(event);
> 
> 
> I don't know which solution is the best and which is the most hacky one...
Yeah, I don't know either. Keeping your current code and adding comments is less
work though :)

[Olivier, 2017-02-20 17:31:37]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2017-02-20 17:31:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
stkhapugin@chromium.org, eugenebut@chromium.org, marq@chromium.org
Link to the patchset: https://codereview.chromium.org/2695313004/#ps40001
(title: "feedback")

[commit-bot: I haz the power, 2017-02-20 17:32:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-20 17:46:15]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487611897829850,
"parent_rev": "ec7f2777385a8a84d4cb03ebf077240b7f70e41b", "commit_rev":
"47b54ff088d9c52ece1c28b06e647e64ec8014d7"}

[commit-bot: I haz the power, 2017-02-20 17:46:58]

Description was changed from

==========
[iOS Reading List] Add distilling WKWebView to the view hierarchy.

Some pages (e.g. uk.businessinsider.com) are not rendered correctly
unless the view is added in the view hierarchy.
Add temporarily the WKWebView to the view hierarchy during distillation.
Move it out ouf the screen to make it invisible to the user.

BUG=685583
==========

to

==========
[iOS Reading List] Add distilling WKWebView to the view hierarchy.

Some pages (e.g. uk.businessinsider.com) are not rendered correctly
unless the view is added in the view hierarchy.
Add temporarily the WKWebView to the view hierarchy during distillation.
Move it out ouf the screen to make it invisible to the user.

BUG=685583

Review-Url: https://codereview.chromium.org/2695313004
Cr-Commit-Position: refs/heads/master@{#451643}
Committed:
https://chromium.googlesource.com/chromium/src/+/47b54ff088d9c52ece1c28b06e64...
==========

[commit-bot: I haz the power, 2017-02-20 17:47:00]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/47b54ff088d9c52ece1c28b06e64...