New issue
Advanced search Search tips

Issue 684554 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

AMP pages are still not distilled

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

Issue description

When adding AMP pages to Bling Reading List, they are still not distilled
 
Cc: mard...@chromium.org
It seems that we need a delay for loading the Google SRP too.
I assumed that the simple page could be rendered immediately, but it is done by JavaScript too and need to fetch the AMP scripts.
Project Member

Comment 3 by bugdroid1@chromium.org, Jan 24 2017

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

commit 7cf0a2ae60d21e220291cab0cf3f6579bf933f25
Author: olivierrobin <olivierrobin@chromium.org>
Date: Tue Jan 24 17:50:56 2017

[ReadingList Offline] Handle AMP pages after the rendering delay.

Google SRP pages are built by javascript and they need time to create
the iframe.
Navigate to the iframe after the 2 seconds delay.

BUG= 684554 

Review-Url: https://codereview.chromium.org/2649263007
Cr-Commit-Position: refs/heads/master@{#445756}

[modify] https://crrev.com/7cf0a2ae60d21e220291cab0cf3f6579bf933f25/ios/chrome/browser/reading_list/reading_list_distiller_page.h
[modify] https://crrev.com/7cf0a2ae60d21e220291cab0cf3f6579bf933f25/ios/chrome/browser/reading_list/reading_list_distiller_page.mm

Cc: shbarezer@chromium.org
+ Sharon
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.
Labels: -Merge-TBD Merge-Request-57
Project Member

Comment 8 by sheriffbot@chromium.org, Jan 25 2017

Labels: -Merge-Request-57 Hotlist-Merge-Approved Merge-Approved-57
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 9 by bugdroid1@chromium.org, Jan 26 2017

Labels: -merge-approved-57 merge-merged-2987
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b8ca9fcaff83dfb1bfab45006c2bdc2884352c79

commit b8ca9fcaff83dfb1bfab45006c2bdc2884352c79
Author: Olivier Robin <olivierrobin@chromium.org>
Date: Thu Jan 26 08:21:16 2017

[ReadingList Offline] Handle AMP pages after the rendering delay.

Google SRP pages are built by javascript and they need time to create
the iframe.
Navigate to the iframe after the 2 seconds delay.

BUG= 684554 

Review-Url: https://codereview.chromium.org/2649263007
Cr-Commit-Position: refs/heads/master@{#445756}
(cherry picked from commit 7cf0a2ae60d21e220291cab0cf3f6579bf933f25)

Review-Url: https://codereview.chromium.org/2647323012 .
Cr-Commit-Position: refs/branch-heads/2987@{#100}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

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

Status: Verified (was: Fixed)
Verified in 58.0.2998.0 canary, iPhone 6+ iOS 10.2, iPad mini 10.1

Google search pages are saved in Reading List and distilled.
Steps:
1. Go to Google.com
2. enter any search e.g. summer houses
3. Tap on Share -> Read Later

Looks good.
Verified in 57.0.2987.20 dev, iPad air 10.2.1

Followed steps on comment #10.
Looks good.

Sign in to add a comment