Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2930323003:
Show client placeholders for Server LoFi images. (Closed)
DescriptionShow client placeholders for Server LoFi images.
This CL causes Chrome to show the same client-generated placeholders for
both Server LoFi and Client LoFi, so that all LoFi images have a
consistent appearance.
BUG=680645
Review-Url: https://codereview.chromium.org/2930323003
Cr-Commit-Position: refs/heads/master@{#485131}
Committed: https://chromium.googlesource.com/chromium/src/+/50e4bb95fb504cde3b317a3d16f601456acc5198
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed megjablon comments. #Patch Set 3 : Added more comments #Patch Set 4 : Added field trial and command line switch controls #
Total comments: 5
Patch Set 5 : Addressed comments #
Total comments: 6
Patch Set 6 : Addressed megjablon's comments #
Total comments: 8
Patch Set 7 : Addressed kouhei comments #Patch Set 8 : Rebased on master #Patch Set 9 : rebase flag_descriptions.cc #Messages
Total messages: 45 (16 generated)
sclittle@chromium.org changed reviewers: + japhet@chromium.org, megjablon@chromium.org
Description was changed from ========== Show client placeholders for Server LoFi images. This CL causes Chrome to show the same client-generated placeholders for both Server LoFi and Client LoFi, so that all LoFi images have a consistent appearance. BUG=680645 ========== to ========== Show client placeholders for Server LoFi images. This CL causes Chrome to show the same client-generated placeholders for both Server LoFi and Client LoFi, so that all LoFi images have a consistent appearance. BUG=680645 ==========
https://codereview.chromium.org/2930323003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right): https://codereview.chromium.org/2930323003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:61: response.HttpHeaderField("chrome-proxy").Contains("q=low"); Can you add a comment that we still check q=low because of the cache? I started commenting asking why we still have this, so it'd be good to comment so that it doesn't get accidentally removed. https://codereview.chromium.org/2930323003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:464: if (HasServerLoFiResponseHeaders(this->GetResponse())) { Shouldn't this already be set? We wouldn't have gotten a lo-fi response if we didn't have the server lo-fi bit set. https://codereview.chromium.org/2930323003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:480: bool ImageResource::ShouldShowPlaceholder() const { Is this method only called if you're in the client lo-fi experiment?
https://codereview.chromium.org/2930323003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right): https://codereview.chromium.org/2930323003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:61: response.HttpHeaderField("chrome-proxy").Contains("q=low"); On 2017/06/13 19:26:14, megjablon wrote: > Can you add a comment that we still check q=low because of the cache? I started > commenting asking why we still have this, so it'd be good to comment so that it > doesn't get accidentally removed. Done. https://codereview.chromium.org/2930323003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:464: if (HasServerLoFiResponseHeaders(this->GetResponse())) { On 2017/06/13 19:26:14, megjablon wrote: > Shouldn't this already be set? We wouldn't have gotten a lo-fi response if we > didn't have the server lo-fi bit set. Normally the bit should be set if we're getting the headers, but the legacy q=low LoFi images didn't use Vary in the way that the newer LoFi images do, so the legacy images could still show up even if Chrome isn't in LoFi mode. Also, if there's ever a server or client bug somewhere or something, it's nice to have it such that the user is still able to reload the original images if Server LoFi images are still served somehow. https://codereview.chromium.org/2930323003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:480: bool ImageResource::ShouldShowPlaceholder() const { On 2017/06/13 19:26:14, megjablon wrote: > Is this method only called if you're in the client lo-fi experiment? It's called by ImageResourceContent::UpdateImage when it decides if it should show a placeholder image or not, for all images regardless of what experiments are active. So this causes client placeholders to be shown for Server LoFi images even if the user isn't in the Client LoFi experiment.
https://codereview.chromium.org/2930323003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right): https://codereview.chromium.org/2930323003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:464: if (HasServerLoFiResponseHeaders(this->GetResponse())) { On 2017/06/13 20:53:23, sclittle wrote: > On 2017/06/13 19:26:14, megjablon wrote: > > Shouldn't this already be set? We wouldn't have gotten a lo-fi response if we > > didn't have the server lo-fi bit set. > > Normally the bit should be set if we're getting the headers, but the legacy > q=low LoFi images didn't use Vary in the way that the newer LoFi images do, so > the legacy images could still show up even if Chrome isn't in LoFi mode. > > Also, if there's ever a server or client bug somewhere or something, it's nice > to have it such that the user is still able to reload the original images if > Server LoFi images are still served somehow. Ah ok. Can you add a comment of this effect? https://codereview.chromium.org/2930323003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:480: bool ImageResource::ShouldShowPlaceholder() const { On 2017/06/13 20:53:23, sclittle wrote: > On 2017/06/13 19:26:14, megjablon wrote: > > Is this method only called if you're in the client lo-fi experiment? > > It's called by ImageResourceContent::UpdateImage when it decides if it should > show a placeholder image or not, for all images regardless of what experiments > are active. > > So this causes client placeholders to be shown for Server LoFi images even if > the user isn't in the Client LoFi experiment. Is this approved to roll out for server lo-fi without any sort of flag? Does the server team know that we essentially won't be using the server placeholders M61+ for all lo-fi users?
https://codereview.chromium.org/2930323003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right): https://codereview.chromium.org/2930323003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:464: if (HasServerLoFiResponseHeaders(this->GetResponse())) { On 2017/06/13 21:27:05, megjablon wrote: > On 2017/06/13 20:53:23, sclittle wrote: > > On 2017/06/13 19:26:14, megjablon wrote: > > > Shouldn't this already be set? We wouldn't have gotten a lo-fi response if > we > > > didn't have the server lo-fi bit set. > > > > Normally the bit should be set if we're getting the headers, but the legacy > > q=low LoFi images didn't use Vary in the way that the newer LoFi images do, so > > the legacy images could still show up even if Chrome isn't in LoFi mode. > > > > Also, if there's ever a server or client bug somewhere or something, it's nice > > to have it such that the user is still able to reload the original images if > > Server LoFi images are still served somehow. > > Ah ok. Can you add a comment of this effect? Done. https://codereview.chromium.org/2930323003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:480: bool ImageResource::ShouldShowPlaceholder() const { On 2017/06/13 21:27:05, megjablon wrote: > On 2017/06/13 20:53:23, sclittle wrote: > > On 2017/06/13 19:26:14, megjablon wrote: > > > Is this method only called if you're in the client lo-fi experiment? > > > > It's called by ImageResourceContent::UpdateImage when it decides if it should > > show a placeholder image or not, for all images regardless of what experiments > > are active. > > > > So this causes client placeholders to be shown for Server LoFi images even if > > the user isn't in the Client LoFi experiment. > > Is this approved to roll out for server lo-fi without any sort of flag? Does the > server team know that we essentially won't be using the server placeholders M61+ > for all lo-fi users? Good point - I'll confirm and clarify with server folks before landing this.
I've added field trial and command line switch controls. PTAL, thanks in advance!
https://codereview.chromium.org/2930323003/diff/60001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2930323003/diff/60001/chrome/browser/about_fl... chrome/browser/about_flags.cc:1902: {"enable-show-client-placeholders-for-server-lo-fi", You could probably shorten this everywhere by removing "show". I think that reads better too "enable-client-placeholders-for-server-lo-fi" https://codereview.chromium.org/2930323003/diff/60001/chrome/browser/about_fl... chrome/browser/about_flags.cc:1908: switches::kDisableShowClientPlaceholdersForServerLoFi)}, We should probably be using a feature instead of a field trial for client lo-fi. Is there a particular reason we aren't? Instead of using command line switches and a separate about flag for the param, I would use FEATURE_WITH_PARAMS_VALUE_TYPE. See kDataReductionMainMenuFeatureVariations.
dglazkov: content/child/runtime_features.cc japhet: third_party/WebKit/* Thanks in advance! https://codereview.chromium.org/2930323003/diff/60001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2930323003/diff/60001/chrome/browser/about_fl... chrome/browser/about_flags.cc:1902: {"enable-show-client-placeholders-for-server-lo-fi", On 2017/06/29 17:35:48, megjablon wrote: > You could probably shorten this everywhere by removing "show". I think that > reads better too "enable-client-placeholders-for-server-lo-fi" Done. https://codereview.chromium.org/2930323003/diff/60001/chrome/browser/about_fl... chrome/browser/about_flags.cc:1908: switches::kDisableShowClientPlaceholdersForServerLoFi)}, On 2017/06/29 17:35:48, megjablon wrote: > We should probably be using a feature instead of a field trial for client lo-fi. > Is there a particular reason we aren't? Instead of using command line switches > and a separate about flag for the param, I would use > FEATURE_WITH_PARAMS_VALUE_TYPE. See kDataReductionMainMenuFeatureVariations. Great idea, thanks! Done.
sclittle@chromium.org changed reviewers: + yhirano@chromium.org
yhirano: content/child/runtime_features.cc Adding yhirano@ instead of dglazkov@ who is OOO.
sclittle@chromium.org changed reviewers: + kinuko@chromium.org
kinuko: third_party/WebKit/Source/platform/*
yhirano@chromium.org changed reviewers: + hiroshige@chromium.org, kouhei@chromium.org
https://codereview.chromium.org/2930323003/diff/60001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2930323003/diff/60001/chrome/browser/about_fl... chrome/browser/about_flags.cc:1908: switches::kDisableShowClientPlaceholdersForServerLoFi)}, On 2017/06/29 21:27:58, sclittle wrote: > On 2017/06/29 17:35:48, megjablon wrote: > > We should probably be using a feature instead of a field trial for client > lo-fi. > > Is there a particular reason we aren't? Instead of using command line switches > > and a separate about flag for the param, I would use > > FEATURE_WITH_PARAMS_VALUE_TYPE. See kDataReductionMainMenuFeatureVariations. > > Great idea, thanks! Done. Woot! This looks a lot cleaner! https://codereview.chromium.org/2930323003/diff/80001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2930323003/diff/80001/chrome/browser/about_fl... chrome/browser/about_flags.cc:1086: {"(replace Server LoFi placeholders)", nit: I'd use Lo-Fi since that's what we use in the other about_flags descriptions. https://codereview.chromium.org/2930323003/diff/80001/chrome/browser/flag_des... File chrome/browser/flag_descriptions.cc (right): https://codereview.chromium.org/2930323003/diff/80001/chrome/browser/flag_des... chrome/browser/flag_descriptions.cc:1208: const char kEnableClientPlaceholdersForServerLoFiName[] = Unused. Remove. https://codereview.chromium.org/2930323003/diff/80001/chrome/browser/flag_des... chrome/browser/flag_descriptions.cc:1211: const char kEnableClientPlaceholdersForServerLoFiDescription[] = Unused. Remove.
https://codereview.chromium.org/2930323003/diff/80001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2930323003/diff/80001/chrome/browser/about_fl... chrome/browser/about_flags.cc:1086: {"(replace Server LoFi placeholders)", On 2017/06/30 18:19:21, megjablon wrote: > nit: I'd use Lo-Fi since that's what we use in the other about_flags > descriptions. Done. https://codereview.chromium.org/2930323003/diff/80001/chrome/browser/flag_des... File chrome/browser/flag_descriptions.cc (right): https://codereview.chromium.org/2930323003/diff/80001/chrome/browser/flag_des... chrome/browser/flag_descriptions.cc:1208: const char kEnableClientPlaceholdersForServerLoFiName[] = On 2017/06/30 18:19:21, megjablon wrote: > Unused. Remove. Done. https://codereview.chromium.org/2930323003/diff/80001/chrome/browser/flag_des... chrome/browser/flag_descriptions.cc:1211: const char kEnableClientPlaceholdersForServerLoFiDescription[] = On 2017/06/30 18:19:21, megjablon wrote: > Unused. Remove. Done.
Sorry for the late response. I'm not familiar with LoFi images, so I would like hiroshige@ or kouhei@ to take a look at core/loader changes. As written in the OWNERS file, I am an owner only for resource loading related files, so I don't have ownership of content/child/runtime_features.cc.
sclittle@chromium.org changed reviewers: + dglazkov@chromium.org - japhet@chromium.org
OK, thanks for the heads up. @dglazkov: PTAL at content/child/runtime_features.cc
https://codereview.chromium.org/2930323003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right): https://codereview.chromium.org/2930323003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:457: IsEntireResource(this->GetResponse())) { Sorry I forgot why we are using this->GetResponse() instead of function param "response". Can we simply use response here? or can we add a comment why? (feel free to split CL for this) My guess is to avoid reacting to revalidation req responses? https://codereview.chromium.org/2930323003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:471: if (HasServerLoFiResponseHeaders(this->GetResponse())) { Can we omit "this->"? https://codereview.chromium.org/2930323003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp (right): https://codereview.chromium.org/2930323003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp:569: const bool kBools[] = {false, true}; Use TEST_P and INSTANTIATE_TEST_CASE_P(..., ::testing::Bool());
Patchset #7 (id:120001) has been deleted
https://codereview.chromium.org/2930323003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right): https://codereview.chromium.org/2930323003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:457: IsEntireResource(this->GetResponse())) { On 2017/07/07 01:38:03, kouhei (in TOK) wrote: > Sorry I forgot why we are using this->GetResponse() instead of function param > "response". > Can we simply use response here? or can we add a comment why? > (feel free to split CL for this) > My guess is to avoid reacting to revalidation req responses? Looking at the code, it looks like you're correct in that it's for handling revalidation responses. I've added a comment. https://codereview.chromium.org/2930323003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:471: if (HasServerLoFiResponseHeaders(this->GetResponse())) { On 2017/07/07 01:38:03, kouhei (in TOK) wrote: > Can we omit "this->"? Done. https://codereview.chromium.org/2930323003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp (right): https://codereview.chromium.org/2930323003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp:569: const bool kBools[] = {false, true}; On 2017/07/07 01:38:03, kouhei (in TOK) wrote: > Use TEST_P and INSTANTIATE_TEST_CASE_P(..., ::testing::Bool()); Done.
Blink-side looks good. I'm a bit worried about PreviewState bitwise operations scattered in the code, but this doesn't block this CL. https://codereview.chromium.org/2930323003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right): https://codereview.chromium.org/2930323003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:481: SetPreviewsState(GetResourceRequest().GetPreviewsState() | Just for clarification: SetPreviewsState() calls here and at Line 494 modifies ResourceRequest, but this looks like a part of response. Do we need to modify ResourceRequest? Does the following alternative work? 1. Store a separate PreviewState as (Image)Resource::response_preview_state_ that is initialized to ResourceRequest's preview state. 2. Update |response_preview_state_| here, leave ResourceRequest's preview state unmodified. 3. Make the lofi-related code in ImageResource.cpp use response_preview_state_ instead of ResourceRequest's PreviewState. This doesn't block this CL, no need to do the changes; I want to track the intention of modifications to ResourceRequest.
lgtm
core/loader non-owner LGTM
lgtm (Interested in if hiroshige's makes sense, and also interested in if we could factor out these preview-state related code into some self-contained code)
On 2017/07/07 07:41:15, kinuko wrote: > lgtm > > (Interested in if hiroshige's makes sense, and also interested in if we could > factor out these preview-state related code into some self-contained code) Yes. I'd love to have (as separate CLs): - separate Lofi class for consolidated Lofi related logic - bitwise ops as functions of Flags class as like in PaintInfo https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai...
https://codereview.chromium.org/2930323003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right): https://codereview.chromium.org/2930323003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:481: SetPreviewsState(GetResourceRequest().GetPreviewsState() | On 2017/07/07 02:52:49, hiroshige wrote: > Just for clarification: > SetPreviewsState() calls here and at Line 494 modifies ResourceRequest, but this > looks like a part of response. > Do we need to modify ResourceRequest? Does the following alternative work? > 1. Store a separate PreviewState as (Image)Resource::response_preview_state_ > that is initialized to ResourceRequest's preview state. > 2. Update |response_preview_state_| here, leave ResourceRequest's preview state > unmodified. > 3. Make the lofi-related code in ImageResource.cpp use response_preview_state_ > instead of ResourceRequest's PreviewState. > > This doesn't block this CL, no need to do the changes; I want to track the > intention of modifications to ResourceRequest. I think that alternative would work, and it might be useful to keep the original PreviewsState from the request intact if we wanted to e.g. record metrics about it in the future. I think it's a bit simpler to update the bits in the ResourceRequest for now (at least for this CL), since e.g. the reloading code already keeps those bits up to date. I also like the idea of splitting the LoFi/Previews logic out into a separate helper or state class like kinuko@ and kouhei@ mentioned, but that seems better suited to a separate CL.
The CQ bit was checked by sclittle@chromium.org
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
Failed to apply patch for chrome/browser/flag_descriptions.cc:
While running git apply --index -3 -p1;
error: patch failed: chrome/browser/flag_descriptions.cc:1213
Falling back to three-way merge...
Applied patch to 'chrome/browser/flag_descriptions.cc' with conflicts.
U chrome/browser/flag_descriptions.cc
Patch: chrome/browser/flag_descriptions.cc
Index: chrome/browser/flag_descriptions.cc
diff --git a/chrome/browser/flag_descriptions.cc
b/chrome/browser/flag_descriptions.cc
index
7d3d8769b826684a4288cacb1b690c2c33141538..68573cd42c90252e91b34623c10d2c09f59ca1bc
100644
--- a/chrome/browser/flag_descriptions.cc
+++ b/chrome/browser/flag_descriptions.cc
@@ -1213,6 +1213,11 @@ const char
kEnableDataReductionProxySavingsPromoDescription[] =
"already saved 1 MB of data, then the promo will not be shown. Data "
"Saver must be enabled for the promo to be shown.";
+const char kEnableClientLoFiName[] = "Client-side Lo-Fi previews";
+
+const char kEnableClientLoFiDescription[] =
+ "Enable showing low fidelity images on some pages on slow networks.";
+
#if defined(OS_ANDROID)
const char kEnableDataReductionProxyMainMenuName[] =
@@ -1232,11 +1237,6 @@ const char kEnableOfflinePreviewsName[] = "Offline Page
Previews";
const char kEnableOfflinePreviewsDescription[] =
"Enable showing offline page previews on slow networks.";
-const char kEnableClientLoFiName[] = "Client-side Lo-Fi previews";
-
-const char kEnableClientLoFiDescription[] =
- "Enable showing low fidelity images on some pages on slow networks.";
-
#endif // defined(OS_ANDROID)
const char kLcdTextName[] = "LCD text antialiasing";
The CQ bit was checked by sclittle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hiroshige@chromium.org, kinuko@chromium.org, kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/2930323003/#ps160001 (title: "Rebased on master")
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
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by sclittle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hiroshige@chromium.org, kinuko@chromium.org, kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/2930323003/#ps180001 (title: "rebase flag_descriptions.cc")
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": 180001, "attempt_start_ts": 1499474038456140,
"parent_rev": "b1b6f101f5078d42160af02e26bd5c3c27a03100", "commit_rev":
"50e4bb95fb504cde3b317a3d16f601456acc5198"}
Message was sent while issue was closed.
Description was changed from ========== Show client placeholders for Server LoFi images. This CL causes Chrome to show the same client-generated placeholders for both Server LoFi and Client LoFi, so that all LoFi images have a consistent appearance. BUG=680645 ========== to ========== Show client placeholders for Server LoFi images. This CL causes Chrome to show the same client-generated placeholders for both Server LoFi and Client LoFi, so that all LoFi images have a consistent appearance. BUG=680645 Review-Url: https://codereview.chromium.org/2930323003 Cr-Commit-Position: refs/heads/master@{#485131} Committed: https://chromium.googlesource.com/chromium/src/+/50e4bb95fb504cde3b317a3d16f6... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/50e4bb95fb504cde3b317a3d16f6... |
