IOS Reading distillation: Handle Google AMP iFrame.

IOS Reading distillation: Handle Google AMP iFrame.

Google search page presents AMP pages in iFrames. This prevents normal
distillation from happening as the WKWebView does not allow injection in iFrames.
This CL works around this issue by navigating to the iframe address when
a Google AMP page is detected.

BUG=677848
Review-Url: https://codereview.chromium.org/2635193003
Cr-Commit-Position: refs/heads/master@{#444422}
Committed: https://chromium.googlesource.com/chromium/src/+/a4a3f4f5e4d89e1afb83192f2f556e25cd9190c6

[Olivier, 2017-01-17 15:40:25]

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

[Olivier, 2017-01-17 15:40:26]

Congrats, you win!

[jif-google, 2017-01-17 16:15:28]

Description was changed from

==========
IOS Reading distillation: Handle Google AMP iFrame.

Google search page presents AMP pages in iFrames. This prevent normal
distillation to happen as WKWebView does not allow injection in iFrames.
This CL works around this issue by navigating to the iframe address when
a Google AMP page is detected.

BUG=677848
==========

to

==========
IOS Reading distillation: Handle Google AMP iFrame.

Google search page presents AMP pages in iFrames. This prevents normal
distillation from happening as the WKWebView does not allow injection in
iFrames.
This CL works around this issue by navigating to the iframe address when
a Google AMP page is detected.

BUG=677848
==========

[jif, 2017-01-17 17:43:10]

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

https://codereview.chromium.org/2635193003/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:25: const char*
kJavaScriptNavigateToIframe =
s/kJavaScriptNavigateToIframe/kGetIframeURLJavaScript/

https://codereview.chromium.org/2635193003/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:69: if
(load_completion_status == web::PageLoadCompletionStatus::FAILURE) {
If somebody adds a 3rd status to PageLoadCompletionStatus, this code will break
silently.

Either compare load_completion_status to web::PageLoadCompletionStatus::SUCCESS
(good), or do a switch...case on |load_completion_status| (better).

https://codereview.chromium.org/2635193003/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:73: if
(HandleGoogleCachedAMP()) {
How about this:

if (IsGoogleCachedAMP()) {
  // Workaround for Google AMP pages.
  HandleGoogleCachedAMP();
} else {
  WaitForPageLoadCompletion();
}

Also, consider renaming |IsGoogleCachedAMP| to |IsGoogleCachedAMPPage|.

https://codereview.chromium.org/2635193003/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:91:
DistillerPageIOS::OnLoadURLDone(web::PageLoadCompletionStatus::SUCCESS);
There's some funny business going on here.

https://codereview.chromium.org/2635193003/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:98: GURL url =
CurrentWebState()->GetLastCommittedURL();
const GURL &

https://codereview.chromium.org/2635193003/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:130: if
(!weak_this->HandleGoogleCachedAMPJavaScriptResult(result, error)) {
weak_this can be null.

https://codereview.chromium.org/2635193003/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:146: NSURL*
new_url = [NSURL URLWithString:result];
can you add an DCHECK that verifies that |result| is an NSString?
Also, can you avoid using a NSURL, and only use a GURL?

[jif, 2017-01-17 17:43:45]

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

https://codereview.chromium.org/2635193003/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:91:
DistillerPageIOS::OnLoadURLDone(web::PageLoadCompletionStatus::SUCCESS);
On 2017/01/17 17:43:10, jif wrote:
> There's some funny business going on here.

Ignore this comment, I forgot to remove it.

[Olivier, 2017-01-18 09:30:13]

Thanks, PTAL

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

https://codereview.chromium.org/2635193003/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:25: const char*
kJavaScriptNavigateToIframe =
On 2017/01/17 17:43:10, jif wrote:
> s/kJavaScriptNavigateToIframe/kGetIframeURLJavaScript/

mmm, I obviously did not upload my cleaning PS...
Done

https://codereview.chromium.org/2635193003/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:69: if
(load_completion_status == web::PageLoadCompletionStatus::FAILURE) {
On 2017/01/17 17:43:10, jif wrote:
> If somebody adds a 3rd status to PageLoadCompletionStatus, this code will
break
> silently.
> 
> Either compare load_completion_status to
web::PageLoadCompletionStatus::SUCCESS
> (good), or do a switch...case on |load_completion_status| (better).

Any not success is a failure for this feature.
So compare to success.
Done.

https://codereview.chromium.org/2635193003/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:73: if
(HandleGoogleCachedAMP()) {
On 2017/01/17 17:43:10, jif wrote:
> How about this:
> 
> if (IsGoogleCachedAMP()) {
>   // Workaround for Google AMP pages.
>   HandleGoogleCachedAMP();
> } else {
>   WaitForPageLoadCompletion();
> }
> 
> Also, consider renaming |IsGoogleCachedAMP| to |IsGoogleCachedAMPPage|.

Done.

https://codereview.chromium.org/2635193003/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:91:
DistillerPageIOS::OnLoadURLDone(web::PageLoadCompletionStatus::SUCCESS);
On 2017/01/17 17:43:45, jif wrote:
> On 2017/01/17 17:43:10, jif wrote:
> > There's some funny business going on here.
> 
> Ignore this comment, I forgot to remove it.

Acknowledged.

https://codereview.chromium.org/2635193003/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:98: GURL url =
CurrentWebState()->GetLastCommittedURL();
On 2017/01/17 17:43:10, jif wrote:
> const GURL &

Done.

https://codereview.chromium.org/2635193003/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:130: if
(!weak_this->HandleGoogleCachedAMPJavaScriptResult(result, error)) {
On 2017/01/17 17:43:10, jif wrote:
> weak_this can be null.
Interesting question on what to do if weak_this is null.
This is an error, but we cannot call the handler with failure.
So the only think we can possibly do is... nothing. 
Added a test.

https://codereview.chromium.org/2635193003/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:146: NSURL*
new_url = [NSURL URLWithString:result];
On 2017/01/17 17:43:10, jif wrote:
> can you add an DCHECK that verifies that |result| is an NSString?
> Also, can you avoid using a NSURL, and only use a GURL?

Added a cast.
If result is not a string (which is possible), the new_url will be nil.

Using NSURL provides hasSuffix, which is really convenient here :)

[jif, 2017-01-18 09:42:12]

lgtm

[stkhapugin, 2017-01-18 12:38:23]

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

[stkhapugin, 2017-01-18 12:38:24]

drive-by.

Please add information about this workaround to reading list design doc.

https://codereview.chromium.org/2635193003/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_distiller_page.h (right):

https://codereview.chromium.org/2635193003/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_distiller_page.h:50: bool
HandleGoogleCachedAMPPageJavaScriptResult(id result, NSError* error);
Please add comments to these methods.

[Olivier, 2017-01-18 14:31:40]

Done, thanks.

[Olivier, 2017-01-18 14:31:57]

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

[Olivier, 2017-01-18 14:31:58]

+noyau as owner.

[noyau (Ping after 24h), 2017-01-18 16:19:37]

https://codereview.chromium.org/2635193003/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_distiller_page.mm (right):

https://codereview.chromium.org/2635193003/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:70: if
(load_completion_status != web::PageLoadCompletionStatus::SUCCESS) {
Can you explain this change a bit?

https://codereview.chromium.org/2635193003/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:132:
weak_this->WaitForPageLoadCompletion();
When I see a weak_this I always wonder if it could be deallocated between the
test if(weak_this) and here...

[Olivier, 2017-01-18 17:04:25]

Thanks

https://codereview.chromium.org/2635193003/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_distiller_page.mm (right):

https://codereview.chromium.org/2635193003/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:70: if
(load_completion_status != web::PageLoadCompletionStatus::SUCCESS) {
On 2017/01/18 16:19:37, noyau wrote:
> Can you explain this change a bit?

At the moment, there is only two values, SUCCESS and FAILURE.
But jif pointed out this is more conservative if a third choice is added.
We only want to continue in case of a success anyway.

https://codereview.chromium.org/2635193003/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:132:
weak_this->WaitForPageLoadCompletion();
On 2017/01/18 16:19:37, noyau wrote:
> When I see a weak_this I always wonder if it could be deallocated between the
> test if(weak_this) and here...

Both this block and the code destroying this object are on main thread, so it
should not happen...

[noyau (Ping after 24h), 2017-01-18 17:28:06]

lgtm

[Olivier, 2017-01-18 17:30:14]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2017-01-18 17:30:14]

The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org,
noyau@chromium.org
Link to the patchset: https://codereview.chromium.org/2635193003/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-18 17:30:29]

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-01-18 18:27:25]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484760614083540,
"parent_rev": "452fb457125d6af6dfe33d7be7892eb98ed0d33c", "commit_rev":
"a4a3f4f5e4d89e1afb83192f2f556e25cd9190c6"}

[commit-bot: I haz the power, 2017-01-18 18:27:56]

Description was changed from

==========
IOS Reading distillation: Handle Google AMP iFrame.

Google search page presents AMP pages in iFrames. This prevents normal
distillation from happening as the WKWebView does not allow injection in
iFrames.
This CL works around this issue by navigating to the iframe address when
a Google AMP page is detected.

BUG=677848
==========

to

==========
IOS Reading distillation: Handle Google AMP iFrame.

Google search page presents AMP pages in iFrames. This prevents normal
distillation from happening as the WKWebView does not allow injection in
iFrames.
This CL works around this issue by navigating to the iframe address when
a Google AMP page is detected.

BUG=677848

Review-Url: https://codereview.chromium.org/2635193003
Cr-Commit-Position: refs/heads/master@{#444422}
Committed:
https://chromium.googlesource.com/chromium/src/+/a4a3f4f5e4d89e1afb83192f2f55...
==========

[commit-bot: I haz the power, 2017-01-18 18:27:57]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/a4a3f4f5e4d89e1afb83192f2f55...