Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(35)

Issue 2484633004: Change Lo-Fi bool to bitmask to support multiple Previews types (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 months, 2 weeks ago by megjablon
Modified:
8 months, 2 weeks ago
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, tyoshino+watch_chromium.org, Yoav Weiss, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, Randy Smith (Not in Mondays), blink-reviews, dglazkov+blink, darin-cc_chromium.org, gavinp+loader_chromium.org, loading-reviews_chromium.org, loading-reviews+fetch_chromium.org, Nate Chapin, tbansal+watch-data-reduction-proxy_chromium.org, blink-reviews-api_chromium.org, mmenke
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Change Lo-Fi bool to bitmask to support multiple Previews types. In order to support Client-side Lo-Fi and other Previews optimizations which can be enabled in parallel, change the Lo-Fi bool to a Previews bitmask. TBR=mek@chromium.org BUG=657997 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2484633004 Cr-Commit-Position: refs/heads/master@{#443303} Committed: https://chromium.googlesource.com/chromium/src/+/caf312f64306f7490999d4587d7b8426b0a6aa67

Patch Set 1 : fix browser side navigation test #

Patch Set 2 : clean up comments #

Total comments: 4

Patch Set 3 : remove previews_unspecified #

Total comments: 4

Patch Set 4 : add back previews_unspecified #

Total comments: 82

Patch Set 5 : rebase #

Total comments: 4

Patch Set 6 : use PreviewsState everywhere #

Total comments: 14

Patch Set 7 : nasko comments #

Total comments: 2

Patch Set 8 : move static_assert #

Patch Set 9 : rebase #

Total comments: 24

Patch Set 10 : rebase #

Patch Set 11 : thakis comments #

Total comments: 2

Patch Set 12 : Add bitmask to comments and small nit #

Patch Set 13 : fix ContentResourceProviderTest #

Unified diffs Side-by-side diffs Delta from patch set Stats (+524 lines, -443 lines) Patch
M chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +8 lines, -11 lines 0 comments Download
M chrome/browser/data_usage/tab_id_annotator_unittest.cc View 1 2 3 4 5 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/data_use_measurement/chrome_data_use_ascriber_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +6 lines, -10 lines 0 comments Download
M chrome/browser/extensions/api/web_request/web_request_permissions_unittest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +10 lines, -13 lines 0 comments Download
M chrome/browser/extensions/extension_protocols_unittest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +9 lines, -11 lines 0 comments Download
M chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc View 1 2 3 4 5 6 7 8 9 1 chunk +7 lines, -4 lines 0 comments Download
M chrome/browser/net/chrome_network_delegate_unittest.cc View 1 2 3 4 5 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/net/spdyproxy/chrome_data_use_group_provider_unittest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +6 lines, -7 lines 0 comments Download
M chrome/browser/net/spdyproxy/chrome_data_use_group_unittest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +8 lines, -8 lines 0 comments Download
M chrome/browser/predictors/resource_prefetch_predictor_unittest.cc View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/prerender/prerender_resource_throttle_unittest.cc View 1 2 3 4 5 6 7 8 9 10 4 chunks +19 lines, -33 lines 0 comments Download
M chrome/browser/safe_browsing/incident_reporting/resource_request_detector_unittest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +9 lines, -9 lines 0 comments Download
M chrome/browser/search/iframe_source_unittest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +7 lines, -11 lines 0 comments Download
M components/data_reduction_proxy/content/browser/content_lofi_decider.cc View 1 2 3 4 5 6 7 8 9 10 4 chunks +9 lines, -4 lines 0 comments Download
M components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -1 line 0 comments Download
M components/data_reduction_proxy/content/browser/content_lofi_ui_service_unittest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +5 lines, -5 lines 0 comments Download
M components/data_reduction_proxy/content/browser/content_resource_type_provider_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +3 lines, -3 lines 0 comments Download
M content/browser/download/download_manager_impl.cc View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -1 line 0 comments Download
M content/browser/download/save_file_manager.cc View 2 chunks +2 lines, -1 line 0 comments Download
M content/browser/frame_host/navigation_entry_impl.h View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -1 line 0 comments Download
M content/browser/frame_host/navigation_entry_impl.cc View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -2 lines 0 comments Download
M content/browser/frame_host/navigation_request.h View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -1 line 0 comments Download
M content/browser/frame_host/navigation_request.cc View 1 2 3 4 5 6 7 8 9 3 chunks +4 lines, -7 lines 0 comments Download
M content/browser/frame_host/navigator_impl.h View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -1 line 0 comments Download
M content/browser/frame_host/navigator_impl.cc View 1 2 3 4 5 6 7 8 5 chunks +9 lines, -9 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.h View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +9 lines, -7 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.cc View 1 2 3 4 5 6 7 8 9 3 chunks +4 lines, -4 lines 0 comments Download
M content/browser/frame_host/render_frame_host_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 5 chunks +5 lines, -4 lines 0 comments Download
M content/browser/loader/DEPS View 1 2 3 4 5 6 7 8 9 4 chunks +4 lines, -0 lines 0 comments Download
M content/browser/loader/async_resource_handler_unittest.cc View 1 2 3 4 5 6 2 chunks +2 lines, -1 line 0 comments Download
M content/browser/loader/intercepting_resource_handler_unittest.cc View 1 2 3 4 5 13 chunks +13 lines, -12 lines 0 comments Download
M content/browser/loader/mime_sniffing_resource_handler_unittest.cc View 1 2 3 4 5 7 chunks +10 lines, -9 lines 0 comments Download
M content/browser/loader/mojo_async_resource_handler_unittest.cc View 1 2 3 4 5 6 7 8 9 3 chunks +5 lines, -4 lines 0 comments Download
M content/browser/loader/resource_dispatcher_host_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 9 chunks +59 lines, -57 lines 0 comments Download
M content/browser/loader/resource_dispatcher_host_impl.h View 1 2 3 4 5 3 chunks +3 lines, -0 lines 0 comments Download
M content/browser/loader/resource_dispatcher_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 6 chunks +27 lines, -22 lines 0 comments Download
M content/browser/loader/resource_loader.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M content/browser/loader/resource_loader_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M content/browser/loader/resource_request_info_impl.h View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +5 lines, -3 lines 0 comments Download
M content/browser/loader/resource_request_info_impl.cc View 1 2 3 4 5 5 chunks +6 lines, -6 lines 0 comments Download
M content/browser/service_worker/link_header_support_unittest.cc View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -1 line 0 comments Download
M content/child/web_url_loader_impl.cc View 1 2 3 4 5 6 7 8 9 2 chunks +3 lines, -2 lines 0 comments Download
M content/child/weburlresponse_extradata_impl.h View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +7 lines, -3 lines 0 comments Download
M content/common/frame_messages.h View 1 2 3 4 5 6 7 8 9 3 chunks +5 lines, -4 lines 0 comments Download
M content/common/navigation_params.h View 1 2 3 4 5 6 7 8 9 4 chunks +5 lines, -16 lines 0 comments Download
M content/common/navigation_params.cc View 1 2 3 4 5 6 7 8 9 3 chunks +3 lines, -3 lines 0 comments Download
M content/common/resource_messages.h View 1 2 3 4 5 6 2 chunks +2 lines, -2 lines 0 comments Download
M content/common/resource_request.h View 1 2 3 4 5 2 chunks +4 lines, -3 lines 0 comments Download
M content/public/browser/resource_dispatcher_host_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +8 lines, -4 lines 0 comments Download
M content/public/browser/resource_dispatcher_host_delegate.cc View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -2 lines 0 comments Download
M content/public/browser/resource_request_info.h View 1 2 3 4 5 3 chunks +4 lines, -3 lines 0 comments Download
M content/public/common/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
A content/public/common/previews_state.h View 1 2 3 4 5 6 7 8 1 chunk +45 lines, -0 lines 0 comments Download
M content/public/common/resource_response.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M content/public/common/resource_response_info.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +4 lines, -2 lines 0 comments Download
M content/public/common/resource_response_info.cc View 2 chunks +2 lines, -1 line 0 comments Download
M content/public/renderer/render_frame.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +4 lines, -2 lines 0 comments Download
M content/public/test/render_view_test.cc View 1 2 3 4 5 6 7 8 9 3 chunks +3 lines, -2 lines 0 comments Download
M content/renderer/render_frame_impl.h View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +6 lines, -3 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 1 2 3 4 5 6 7 8 9 8 chunks +19 lines, -16 lines 0 comments Download
M content/renderer/render_frame_impl_browsertest.cc View 1 2 3 4 5 6 chunks +11 lines, -10 lines 0 comments Download
M extensions/browser/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +21 lines, -20 lines 0 comments Download
M extensions/browser/api/declarative_webrequest/webrequest_condition_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +28 lines, -27 lines 0 comments Download
M third_party/WebKit/Source/core/fetch/Resource.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/loader/ImageLoader.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/loader/resource/ImageResource.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/platform/exported/WebURLRequest.cpp View 1 2 3 4 5 6 1 chunk +5 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/platform/network/ResourceRequest.h View 1 2 3 4 5 6 7 8 3 chunks +5 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/platform/network/ResourceRequest.cpp View 1 2 3 4 5 6 7 8 3 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/public/platform/WebURLRequest.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +17 lines, -11 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 182 (161 generated)
megjablon
PTAL!
9 months, 3 weeks ago (2016-12-01 20:21:52 UTC) #75
RyanSturm
https://codereview.chromium.org/2484633004/diff/320001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2484633004/diff/320001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc#newcode835 chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:835: if (previews_state & content::PREVIEWS_UNSPECIFIED) { Can you add a ...
9 months, 3 weeks ago (2016-12-03 00:28:11 UTC) #78
megjablon
+nasko: content/* https://codereview.chromium.org/2484633004/diff/320001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2484633004/diff/320001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc#newcode835 chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:835: if (previews_state & content::PREVIEWS_UNSPECIFIED) { On 2016/12/03 ...
9 months, 3 weeks ago (2016-12-06 00:59:18 UTC) #88
RyanSturm
lgtm % 1 maybe nit https://codereview.chromium.org/2484633004/diff/360001/content/public/common/previews_state.h File content/public/common/previews_state.h (right): https://codereview.chromium.org/2484633004/diff/360001/content/public/common/previews_state.h#newcode16 content/public/common/previews_state.h:16: SERVER_LOFI_ON = 1 << ...
9 months, 3 weeks ago (2016-12-06 19:39:30 UTC) #91
megjablon
+pfeldman: content/* https://codereview.chromium.org/2484633004/diff/360001/content/public/common/previews_state.h File content/public/common/previews_state.h (right): https://codereview.chromium.org/2484633004/diff/360001/content/public/common/previews_state.h#newcode16 content/public/common/previews_state.h:16: SERVER_LOFI_ON = 1 << 0, // Request ...
9 months, 3 weeks ago (2016-12-06 21:23:47 UTC) #96
nasko
Overall I find the "Previews" naming to be confusing and find that using LoFiState to ...
9 months, 2 weeks ago (2016-12-08 22:19:34 UTC) #100
megjablon
https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_host/navigation_entry_impl.cc File content/browser/frame_host/navigation_entry_impl.cc (right): https://codereview.chromium.org/2484633004/diff/380001/content/browser/frame_host/navigation_entry_impl.cc#newcode674 content/browser/frame_host/navigation_entry_impl.cc:674: int previews_state, On 2016/12/08 22:19:32, nasko (very slow til ...
9 months, 2 weeks ago (2016-12-09 20:35:55 UTC) #108
nasko
Few more nits, but looks mostly there. https://codereview.chromium.org/2484633004/diff/400001/content/common/navigation_params.cc File content/common/navigation_params.cc (right): https://codereview.chromium.org/2484633004/diff/400001/content/common/navigation_params.cc#newcode12 content/common/navigation_params.cc:12: #include "content/public/common/previews_state.h" ...
9 months, 2 weeks ago (2016-12-12 23:05:41 UTC) #111
megjablon
thestig: chrome/browser/* https://codereview.chromium.org/2484633004/diff/440001/content/browser/frame_host/navigator_impl.cc File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2484633004/diff/440001/content/browser/frame_host/navigator_impl.cc#newcode337 content/browser/frame_host/navigator_impl.cc:337: // Determine if previews should be used ...
9 months, 2 weeks ago (2016-12-13 03:03:14 UTC) #115
megjablon
mek: extensions/*
9 months, 2 weeks ago (2016-12-13 03:06:19 UTC) #117
nasko
https://codereview.chromium.org/2484633004/diff/460001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2484633004/diff/460001/content/renderer/render_frame_impl.cc#newcode5518 content/renderer/render_frame_impl.cc:5518: // are kept in sync. This doesn't seem to ...
9 months, 1 week ago (2016-12-14 00:16:42 UTC) #120
megjablon
https://codereview.chromium.org/2484633004/diff/400001/content/common/navigation_params.cc File content/common/navigation_params.cc (right): https://codereview.chromium.org/2484633004/diff/400001/content/common/navigation_params.cc#newcode12 content/common/navigation_params.cc:12: #include "content/public/common/previews_state.h" On 2016/12/12 23:05:41, nasko (very slow til ...
9 months, 1 week ago (2016-12-14 01:10:32 UTC) #123
nasko
I should say it for the record - the name "Previews" is mighty confusing to ...
9 months, 1 week ago (2016-12-15 01:09:11 UTC) #138
megjablon
-thestig +thakis: chrome/browser/*
9 months, 1 week ago (2016-12-15 18:12:19 UTC) #143
megjablon
On 2016/12/15 18:12:19, megjablon wrote: > -thestig > +thakis: chrome/browser/* A friendly ping :)
8 months, 2 weeks ago (2017-01-09 22:27:23 UTC) #144
Nico
one repeated nit, and one real question: https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc File chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc (right): https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc#newcode444 chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:444: request.get(), resource_type, ...
8 months, 2 weeks ago (2017-01-09 22:35:03 UTC) #145
megjablon
https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc File chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc (right): https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc#newcode444 chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:444: request.get(), resource_type, nullptr, 1, /* render_process_id */ On 2017/01/09 ...
8 months, 2 weeks ago (2017-01-10 22:30:01 UTC) #156
Nico
lgtm, thanks. https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h (right): https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h#newcode86 chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h:86: content::PreviewsState GetPreviewsState( On 2017/01/10 22:30:01, megjablon wrote: ...
8 months, 2 weeks ago (2017-01-11 01:14:47 UTC) #157
megjablon
https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h (right): https://codereview.chromium.org/2484633004/diff/540001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h#newcode86 chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h:86: content::PreviewsState GetPreviewsState( On 2017/01/11 01:14:47, Nico wrote: > On ...
8 months, 2 weeks ago (2017-01-12 17:56:29 UTC) #176
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2484633004/680001
8 months, 2 weeks ago (2017-01-12 18:40:19 UTC) #179
commit-bot: I haz the power
8 months, 2 weeks ago (2017-01-12 18:48:41 UTC) #182
Message was sent while issue was closed.
Committed patchset #13 (id:680001) as
https://chromium.googlesource.com/chromium/src/+/caf312f64306f7490999d4587d7b...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b