New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 677848 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Jan 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Bug



Sign in to add a comment

Reading List: handle AMP pages.

Project Member Reported by olivierrobin@chromium.org, Jan 2 2017

Issue description

The AMP pages on the google search result page cannot be distilled by the Reading List.

This is because the actual page content is in an iframe.
Example page
https://www.google.fr/amp/s/ampbyexample.com/?client=safari

has an iframe to
<iframe class="u2quVpIWVr0__amp-doc" allowfullscreen="true" scrolling="no" src="https://cdn.ampproject.org/v/s/ampbyexample.com/?amp_js_v=6#origin=https%3A%2F%2Fwww.google.fr&amp;exp=a4a%3A0&amp;channelid=0&amp;cid=1&amp;dialog=0&amp;prerenderSize=1&amp;visibilityState=prerender&amp;paddingTop=54&amp;history=1&amp;p2r=0&amp;horizontalScrolling=0&amp;csi=0&amp;storage=1&amp;viewerUrl=https%3A%2F%2Fwww.google.fr%2Famp%2Fs%2Fampbyexample.com%2F" style="height: 116px;"></iframe>

The iFrame prevent to inject the distillation javascript on the page.

A possibility to work around this problem is to open the page directly without the google search container.

The page can be opened directly in https://ampbyexample.com or via the google cache
https://cdn.ampproject.org/c/s/ampbyexample.com/

This cause two issues:
1) detect that we are displaying a google AMP page.
We can check that the host is a google domain and the path starts with /amp/
We may need a stronger test to avoid false positives, particularly as there are a lot of google domains.

2) get the amp address.
A simple way to get the amp cache url is to replace www.google.com/amp by cdn.ampproject.org/c in the URL.
Another possibility is to check the noscript tag in the google page
<noscript> &lt;meta content="0;url=https://cdn.ampproject.org/c/s/ampbyexample.com/" http-equiv="refresh"&gt; </noscript>
 
Cc: pinkerton@chromium.org
Status: Started (was: Assigned)
Labels: ReleaseBlock-Stable
Project Member

Comment 4 by bugdroid1@chromium.org, Jan 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a4a3f4f5e4d89e1afb83192f2f556e25cd9190c6

commit a4a3f4f5e4d89e1afb83192f2f556e25cd9190c6
Author: olivierrobin <olivierrobin@chromium.org>
Date: Wed Jan 18 18:27:22 2017

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}

[modify] https://crrev.com/a4a3f4f5e4d89e1afb83192f2f556e25cd9190c6/ios/chrome/browser/reading_list/BUILD.gn
[modify] https://crrev.com/a4a3f4f5e4d89e1afb83192f2f556e25cd9190c6/ios/chrome/browser/reading_list/reading_list_distiller_page.h
[modify] https://crrev.com/a4a3f4f5e4d89e1afb83192f2f556e25cd9190c6/ios/chrome/browser/reading_list/reading_list_distiller_page.mm

Status: Fixed (was: Started)
Labels: Merge-TBD
[Auto-generated comment by a script] We noticed that this issue is targeted for M-57; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-57 label, otherwise remove Merge-TBD label. Thanks.
Status: Verified (was: Fixed)
Verified in 58.0.2991.0 dev, iPhone 6S iOS 10.1, iPhone 6 iOS 9.3.5 iPad mini 4 10.1

Project Member

Comment 8 by sheriffbot@chromium.org, Mar 8 2017

Labels: -Merge-TBD

Sign in to add a comment