Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2792443002:
Adding logic to prevent offline previews fallback to LoFi/Weblite (Closed)
DescriptionAdding logic to prevent offline previews fallback to LoFi/Weblite
This adds ReloadType::DISABLE_LOFI_MODE to opt outs from offline
previews to prevent reloads from offline previews causing a server
preview from being shown.
BUG=707060
Review-Url: https://codereview.chromium.org/2792443002
Cr-Commit-Position: refs/heads/master@{#461818}
Committed: https://chromium.googlesource.com/chromium/src/+/0340eb999c53a854636325ece62d211b661f67d1
Patch Set 1 #
Total comments: 2
Patch Set 2 : added test and comment #Patch Set 3 : fixed test #Patch Set 4 : plznavigate tests #
Messages
Total messages: 32 (24 generated)
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
ryansturm@chromium.org changed reviewers: + bengr@chromium.org
bengr: PTAL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2792443002/diff/1/chrome/browser/previews/pre... File chrome/browser/previews/previews_infobar_delegate.cc (right): https://codereview.chromium.org/2792443002/diff/1/chrome/browser/previews/pre... chrome/browser/previews/previews_infobar_delegate.cc:158: web_contents->GetController().Reload(content::ReloadType::DISABLE_LOFI_MODE, Does this flag also disable lite pages? If so, please file a bug to change the name to DISABLE_SERVER_PREVIEWS, or something like that. If not, please explain how this covers lite pages. Also, please explain how the following is prevented: 1) a lite page is fetched, because there wasn't an offline page 2) an offline page is created from the lite page 3) the user selects show original and gets the offline page. Add a test please.
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bengr: PTAL https://codereview.chromium.org/2792443002/diff/1/chrome/browser/previews/pre... File chrome/browser/previews/previews_infobar_delegate.cc (right): https://codereview.chromium.org/2792443002/diff/1/chrome/browser/previews/pre... chrome/browser/previews/previews_infobar_delegate.cc:158: web_contents->GetController().Reload(content::ReloadType::DISABLE_LOFI_MODE, On 2017/03/31 16:59:56, bengr wrote: > Does this flag also disable lite pages? If so, please file a bug to change the > name to DISABLE_SERVER_PREVIEWS, or something like that. If not, please explain > how this covers lite pages. > > Also, please explain how the following is prevented: > 1) a lite page is fetched, because there wasn't an offline page DISABLE_LOFI_MODE triggers PREVIEWS_NO_TRANSFORM, which prevents any server transform for requests on the page. > 2) an offline page is created from the lite page Is this a problem? If this were a problem, I'd think there would need to be special logic preventing the user from off-lining any weblite preview, which seems like a valid use case. Is there a particular case where this causes a problem? With the regularity of the offline page logic changing perhaps they should be aware that automatically offlining a page is not a good idea when a preview is showing. > 3) the user selects show original and gets the offline page. > Offline previews are not show on *any* reload. > Add a test please. I added a test to verify that DISABLE_LOFI_MODE is added for this case. a browser test checks that this flag does the right thing: https://cs.chromium.org/chromium/src/content/browser/loader/resource_dispatch...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org Link to the patchset: https://codereview.chromium.org/2792443002/#ps60001 (title: "plznavigate tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491330061569800,
"parent_rev": "acf76351e614fa82116875d8a054a0abfb83d39e", "commit_rev":
"c60eaba04ab3eae94cc4d51d2b5fe710dc187e64"}
The CQ bit was unchecked by commit-bot@chromium.org
Prior attempt to commit was detected, but we were not able to check whether the issue was successfully committed. Please check Git history manually and re-check CQ or close this issue as needed.
The CQ bit was checked by ryansturm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491337006473500,
"parent_rev": "7cf40cf83838145d2e6310f4f923f316b859e4f1", "commit_rev":
"0340eb999c53a854636325ece62d211b661f67d1"}
Message was sent while issue was closed.
Description was changed from ========== Adding logic to prevent offline previews fallback to LoFi/Weblite This adds ReloadType::DISABLE_LOFI_MODE to opt outs from offline previews to prevent reloads from offline previews causing a server preview from being shown. BUG=707060 ========== to ========== Adding logic to prevent offline previews fallback to LoFi/Weblite This adds ReloadType::DISABLE_LOFI_MODE to opt outs from offline previews to prevent reloads from offline previews causing a server preview from being shown. BUG=707060 Review-Url: https://codereview.chromium.org/2792443002 Cr-Commit-Position: refs/heads/master@{#461818} Committed: https://chromium.googlesource.com/chromium/src/+/0340eb999c53a854636325ece62d... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/0340eb999c53a854636325ece62d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
