Incorrect domain name displayed in Reading List for AMP pages |
||||||||||
Issue descriptionApp Version: 60.0.3073.0 iOS Version: 10.3.2, 10.2 Device: iPhone7 URL: Steps to reproduce: 1. Launch Google Chrome 2. Search for any query that results in some ASP pages listings. (Ex: Hillary Clinton) 3. Tap on any AMP result 4. Once the page is loaded, Tap Menu → Share → Read Later 5. Wait for few seconds, and then Tap Menu → Reading List Observed results: Observe that the domain is displayed as https://www.google.com Note: Opening the same entry in offline mode launches the correct domain url in tab. Expected results: Correct domain name of the article should be displayed. Number of times you were able to reproduce: 5/5 Bug reproducible after clean install: Yes/No Bug reproducible after clearing cache and cookies: Yes Bug reproducible on Chrome Mobile on Android: NA Bug reproducible on Dolphin/Safari/Atomic: Dolphin: NA, Safari: NA Bug reproducible on current stable build (App Version, iOS Version): M57 Yes, But AMP pages are not available for offline as well. Bug reproducible on the current beta channel build (App Version, iOS Version): M58 Yes Link to video/image: Video: https://drive.google.com/file/d/0B-xmXLQhjeKudkp5RWp3TjdHeTA/view Image: https://drive.google.com/file/d/0B-xmXLQhjeKubzNEc3BzajVWczQ/view
,
May 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4b220cac0ff6f2c6af5a2099df2eb332ecf4eba6 commit 4b220cac0ff6f2c6af5a2099df2eb332ecf4eba6 Author: gambard <gambard@chromium.org> Date: Fri May 12 15:35:02 2017 Fix AMP domain displayed in ReadingList If an AMP page is added to the Reading List, the domain displayed should be the one from the page after redirection, not google.com. BUG= 713405 Change-Id: Id50c181b374665924dbdf8ea58b249e4f53a9e75 Reviewed-on: https://chromium-review.googlesource.com/504847 Reviewed-by: Jean-François Geyelin <jif@chromium.org> Commit-Queue: Gauthier Ambard <gambard@chromium.org> Cr-Commit-Position: refs/heads/master@{#471304} [modify] https://crrev.com/4b220cac0ff6f2c6af5a2099df2eb332ecf4eba6/ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm
,
May 12 2017
mardini: Do you think it should be cherry-picked in M59?
,
May 12 2017
If ifix is not too risky, then yes. Thank you.
,
May 15 2017
Cherry-pick request for https://chromium-review.googlesource.com/504847
,
May 15 2017
This bug requires manual review: Less than 18 days to go before AppStore submit on M59 Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 15 2017
This is not critical enough to be merged into M59 at this time.
,
May 16 2017
Verified on chrome canary version 60.0.3100.0 on iPad , iOS 10.3 , iPhone 6 10.3.1, iPhone 7 , iOS 10.2.1 following the steps shown in video & comments. "Incorrect domain name displayed in Reading List for AMP pages" is fixed now
,
May 16 2017
,
May 17 2017
FWIW, the previous behavior was working as intended and I think it should be restored, specially as the fix is not specific to AMP pages. The two main reasons are: - it can lead to fishing: if you add site goo.gl that redirects to site safe.com, then after distillation the URL displayed is safe.com. If you are online, opening the entry will redo the redirection and possibly open unsafe.com - the distilled URL is local and you can now have the same entry with different URLs on different devices. This can be confusing, specially when you delete it. Marking RBS for review as the current behavior may lead to issues.
,
May 18 2017
Thanks, olivierrobin@ for commenting while on leave. I discussed with gambard@ and we will revert the CL.
,
May 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9df188ca91c5c6f35bf5b612ae06a78626bf2544 commit 9df188ca91c5c6f35bf5b612ae06a78626bf2544 Author: Gauthier Ambard <gambard@chromium.org> Date: Fri May 19 09:45:33 2017 Revert "Fix AMP domain displayed in ReadingList" This reverts commit 4b220cac0ff6f2c6af5a2099df2eb332ecf4eba6. Reason for revert: Previous behavior was WAI. See bug. Original change's description: > Fix AMP domain displayed in ReadingList > > If an AMP page is added to the Reading List, the domain displayed should be > the one from the page after redirection, not google.com. > > BUG= 713405 > > Change-Id: Id50c181b374665924dbdf8ea58b249e4f53a9e75 > Reviewed-on: https://chromium-review.googlesource.com/504847 > Reviewed-by: Jean-François Geyelin <jif@chromium.org> > Commit-Queue: Gauthier Ambard <gambard@chromium.org> > Cr-Commit-Position: refs/heads/master@{#471304} TBR=jif@chromium.org,gambard@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. BUG= 713405 Change-Id: If2f1a631e67ad1bf173343efdfca6b10dab29f39 Reviewed-on: https://chromium-review.googlesource.com/509249 Reviewed-by: Gauthier Ambard <gambard@chromium.org> Commit-Queue: Gauthier Ambard <gambard@chromium.org> Cr-Commit-Position: refs/heads/master@{#473140} [modify] https://crrev.com/9df188ca91c5c6f35bf5b612ae06a78626bf2544/ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm
,
May 19 2017
Fixed: the behavior has been reverted to the original state.
,
May 23 2017
Verified in Canary chrome 60.0.3108.0 on iPhone 6plus(10.3.1) and iPAD Air(10.2.1). AS per the comment #10 the previous fix has been reverted back. In iPhone's if the AMP page's are added to reading list, the domain is displayed as https://www.google.com. Note: In iPAD's AMP url's are loaded with the article's domain name. |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by jasonkliu@chromium.org
, Apr 20 2017Status: Assigned (was: Untriaged)