Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(861)

Issue 2898653003: Prevent previews from breaking offline pages. (Closed)

Created:
3 years, 7 months ago by Pete Williamson
Modified:
3 years, 4 months ago
Reviewers:
CC:
chromium-reviews, romax+watch_chromium.org, cbentzel+watch_chromium.org, sdefresne+watchlist_chromium.org, creis+watch_chromium.org, droger+watchlist_chromium.org, chili+watch_chromium.org, tburkard+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, jam, petewil+watch_chromium.org, gavinp+prer_chromium.org, darin-cc_chromium.org, nasko+codewatch_chromium.org, blundell+watchlist_chromium.org, dimich+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Prevent previews from breaking offline pages. Some of the optimizations enabled by previews work at cross purposes with offline pages. Previews generally limit the downloaded page to save bandwidth and load faster, where as for offline, the user generally wants a fuller page to read when there is no connectivity. The general strategy is to add a new parameter to the LoadURLParams to disable previews, and if that parameter is set to disable the preview functionality. BUG=703875 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation This change was abandoned for a different approach

Patch Set 1 #

Patch Set 2 : Improve Deps #

Patch Set 3 : Second approach, not compiling yet. #

Total comments: 10

Patch Set 4 : New method for overriding previews_state flags #

Patch Set 5 : Remove leftover comment #

Total comments: 4

Patch Set 6 : More CR feedback per RyanSturm #

Total comments: 1

Patch Set 7 : Allow Page Modifications approach (not yet passing presubmit checks) for discussion #

Patch Set 8 : Alternative to not plumb APM (not passing presubmit checks) for discussion #

Unified diffs Side-by-side diffs Delta from patch set Stats (+101 lines, -12 lines) Patch
M components/offline_pages/content/background_loader/background_loader_contents.cc View 1 2 3 4 5 6 7 2 chunks +6 lines, -5 lines 0 comments Download
M content/browser/frame_host/navigation_controller_impl.cc View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/frame_host/navigation_entry_impl.h View 1 2 3 4 5 6 7 3 chunks +15 lines, -0 lines 0 comments Download
M content/browser/frame_host/navigation_entry_impl.cc View 1 2 3 4 5 6 7 1 chunk +3 lines, -1 line 0 comments Download
M content/browser/frame_host/navigator_impl.cc View 1 2 3 4 5 6 7 2 chunks +13 lines, -0 lines 0 comments Download
M content/browser/loader/resource_dispatcher_host_impl.cc View 1 2 3 4 5 6 7 1 chunk +21 lines, -4 lines 0 comments Download
M content/public/browser/navigation_controller.h View 1 2 3 4 5 6 7 2 chunks +4 lines, -0 lines 0 comments Download
M content/public/browser/navigation_controller.cc View 1 2 3 4 5 6 7 3 chunks +6 lines, -2 lines 0 comments Download
A content/public/common/allowed_page_modifications.h View 1 2 3 4 5 6 7 1 chunk +27 lines, -0 lines 0 comments Download
M content/public/common/previews_state.h View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 23 (16 generated)
RyanSturm
https://codereview.chromium.org/2898653003/diff/40001/content/browser/frame_host/navigator_impl.cc File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2898653003/diff/40001/content/browser/frame_host/navigator_impl.cc#newcode378 content/browser/frame_host/navigator_impl.cc:378: const content::PreviewsState ALLOWED_PREVIEWS_FOR_OFFLINE_PAGES = I'd actually recommend only doing ...
3 years, 6 months ago (2017-06-08 21:03:22 UTC) #11
Pete Williamson
CR fixes per RyanSturm Instead of moving the logic of what to mask out into ...
3 years, 6 months ago (2017-06-09 00:15:11 UTC) #12
RyanSturm
I'm fine with the approach of masking it afterward. https://codereview.chromium.org/2898653003/diff/80001/components/previews/content/BUILD.gn File components/previews/content/BUILD.gn (right): https://codereview.chromium.org/2898653003/diff/80001/components/previews/content/BUILD.gn#newcode1 components/previews/content/BUILD.gn:1: ...
3 years, 6 months ago (2017-06-09 17:22:37 UTC) #13
Pete Williamson
https://codereview.chromium.org/2898653003/diff/80001/components/previews/content/BUILD.gn File components/previews/content/BUILD.gn (right): https://codereview.chromium.org/2898653003/diff/80001/components/previews/content/BUILD.gn#newcode1 components/previews/content/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All rights reserved. ...
3 years, 6 months ago (2017-06-09 17:46:57 UTC) #14
RyanSturm
https://codereview.chromium.org/2898653003/diff/100001/components/previews/content/DEPS File components/previews/content/DEPS (right): https://codereview.chromium.org/2898653003/diff/100001/components/previews/content/DEPS#newcode2 components/previews/content/DEPS:2: "+content/public/browser", Probably want to remove all of the previews/content ...
3 years, 6 months ago (2017-06-09 18:00:19 UTC) #17
Pete Williamson
Abandoning this issue in favor of a different approach: https://chromium-review.googlesource.com/c/544586/ I will remove reviewers.
3 years, 6 months ago (2017-06-23 22:08:05 UTC) #20
Pete Williamson
3 years, 6 months ago (2017-06-23 22:08:21 UTC) #22
removed reviewers

Powered by Google App Engine
This is Rietveld 408576698