Offline Previews infobar shows the wrong string |
||||||||
Issue descriptionThe wrong string is being shown for data saver users when they see on offline preview. Basically, the parameters of a method were switched for the offline previews case, so we ended up in a particularly strange situation. is_reload and is_data_saver_user were reversed for offline previews when creating an infobar. Because refresh was reported wrong: Users could see the freshness time stamp (I believe that is Desktop only) for offline previews, but since offline previews is android only, I think there isn't a problem. Because is_data_saver_user was reported wrong: Users would see either "faster page served" or "data saved" strings based on whether it was a reload. Ideally, offline previews is not served on a reload, so they would always see "faster page served", but due to a different bug (explanation below), either string can be shown. Offline previews should never be shown on a reload, however there was a different bug until 64 that made it possible to go from an offline page to an offline preview on a reload. This alone isn't a terrible problem to have, but it went against the decision we made. If a user got an offline preview, they would have to actually opt out to get to different content (this is consistent with out other previews). See crbug.com/735991
,
Dec 13 2017
I'll add a merge request label later today.
,
Dec 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e72881ed26f70c843f017017887cf45ddb06c7ba commit e72881ed26f70c843f017017887cf45ddb06c7ba Author: Ryan Sturm <ryansturm@chromium.org> Date: Wed Dec 13 20:56:42 2017 Fixing offline previews string switch The parameters to Create() were swapped. Bug: 794626 Change-Id: I07efdb86556b36e6e66f2827f6887f184013a6c7 Reviewed-on: https://chromium-review.googlesource.com/825304 Reviewed-by: Tarun Bansal <tbansal@chromium.org> Commit-Queue: Ryan Sturm <ryansturm@chromium.org> Cr-Commit-Position: refs/heads/master@{#523871} [modify] https://crrev.com/e72881ed26f70c843f017017887cf45ddb06c7ba/chrome/browser/previews/previews_infobar_tab_helper.cc
,
Dec 13 2017
Hi, we'd like to merge this small CL back due to a regression.
,
Dec 14 2017
This bug requires manual review: M64 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 14 2017
Please add the affected OSes
,
Dec 14 2017
,
Dec 15 2017
Approving the merge since the change is very minimal. Make sure to test this on branch M64 after merging it.
,
Dec 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9eae7540b2527d4eb1cfaa83d0cdcb9b431573a2 commit 9eae7540b2527d4eb1cfaa83d0cdcb9b431573a2 Author: Ryan Sturm <ryansturm@chromium.org> Date: Mon Dec 18 19:25:44 2017 merging 825304: Offline Previews infobar shows the wrong string Fixing offline previews string switch The parameters to Create() were swapped. Bug: 794626 Change-Id: I07efdb86556b36e6e66f2827f6887f184013a6c7 Reviewed-on: https://chromium-review.googlesource.com/825304 Reviewed-by: Tarun Bansal <tbansal@chromium.org> Commit-Queue: Ryan Sturm <ryansturm@chromium.org> Cr-Commit-Position: refs/heads/master@{#523871} Bug: 794626 Change-Id: I54719957df32b649009c91ee218813185f792521 Reviewed-on: https://chromium-review.googlesource.com/832988 Reviewed-by: Ryan Sturm <ryansturm@chromium.org> Cr-Commit-Position: refs/branch-heads/3282@{#269} Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840} [modify] https://crrev.com/9eae7540b2527d4eb1cfaa83d0cdcb9b431573a2/chrome/browser/previews/previews_infobar_tab_helper.cc
,
Dec 18 2017
Merged to 3282 |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by tbansal@chromium.org
, Dec 13 2017