Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2848293002:
Adding the Previews infobar to pages that show a client LoFi image (Closed)
|
Created:
3 years, 7 months ago by RyanSturm Modified:
3 years, 7 months ago Reviewers:
sclittle CC:
chromium-reviews, cbentzel+watch_chromium.org, jam, net-reviews_chromium.org, darin-cc_chromium.org, asvitkine+watch_chromium.org, tbansal+watch-data-reduction-proxy_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding the Previews infobar to pages that show a client LoFi image
This adds the Previews infobar to any page loads that use client LoFi
for images. This follows the existing LoFi path for the infobar.
Detection of client LoFi uses the PreviewsState and resource type at
request completion time. Even if an image is full downloaded (due to
small size) or we don't display the image, the infobar is shown because
the range request was issued to the server and could have impacted the
resulting image in some way.
BUG=704322
Review-Url: https://codereview.chromium.org/2848293002
Cr-Commit-Position: refs/heads/master@{#470148}
Committed: https://chromium.googlesource.com/chromium/src/+/2278661de721b27db538323ec60bfdff0aad064a
Patch Set 1 #Patch Set 2 : removed range check #
Total comments: 6
Patch Set 3 : sclittle comments #
Total comments: 14
Patch Set 4 : sclittle comments #Patch Set 5 : merged lofis #
Total comments: 2
Patch Set 6 : sclittle nit #Messages
Total messages: 53 (40 generated)
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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...
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
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 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.
ryansturm@chromium.org changed reviewers: + sclittle@chromium.org
sclittle: PTAL
https://codereview.chromium.org/2848293002/diff/60001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_delegate.cc (right): https://codereview.chromium.org/2848293002/diff/60001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.cc:181: web_contents->ReloadLoFiImages(); If "Show Original" is clicked on a page with both Server and Client LoFi, and Client LoFi is seen first and used for the infobar, then does this mean that none of the above DRP opt-out stuff happens? https://codereview.chromium.org/2848293002/diff/60001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_delegate.h (right): https://codereview.chromium.org/2848293002/diff/60001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.h:30: CLIENT_LOFI, // Client-side image replacement. Since both Server LoFi and Client LoFi will often be used at the same time on the same page, what do we gain from having a separate CLIENT_LOFI value here? Could we just simplify this and make both kinds of LoFi use the same infobar type?
https://codereview.chromium.org/2848293002/diff/60001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_delegate.cc (right): https://codereview.chromium.org/2848293002/diff/60001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.cc:181: web_contents->ReloadLoFiImages(); On 2017/05/03 18:40:21, sclittle wrote: > If "Show Original" is clicked on a page with both Server and Client LoFi, and > Client LoFi is seen first and used for the infobar, then does this mean that > none of the above DRP opt-out stuff happens? Yes. It's definitely a race between the two. What percent of server LoFi page loads are expected to use both types of LoFi. I expect an HTTP page with ads would be a clear example. https://codereview.chromium.org/2848293002/diff/60001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_delegate.h (right): https://codereview.chromium.org/2848293002/diff/60001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.h:30: CLIENT_LOFI, // Client-side image replacement. On 2017/05/03 18:40:21, sclittle wrote: > Since both Server LoFi and Client LoFi will often be used at the same time on > the same page, what do we gain from having a separate CLIENT_LOFI value here? > > Could we just simplify this and make both kinds of LoFi use the same infobar > type? I'd prefer to know the difference between the opt out rates of server LoFi vs Client LoFi since they do actually have differences when it comes to sprite detection, etc. Does Client LoFi only apply to HTTPS within a page load or can it also be used on HTTP resources?
https://codereview.chromium.org/2848293002/diff/60001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_delegate.cc (right): https://codereview.chromium.org/2848293002/diff/60001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.cc:181: web_contents->ReloadLoFiImages(); On 2017/05/03 18:48:52, Ryan Sturm wrote: > On 2017/05/03 18:40:21, sclittle wrote: > > If "Show Original" is clicked on a page with both Server and Client LoFi, and > > Client LoFi is seen first and used for the infobar, then does this mean that > > none of the above DRP opt-out stuff happens? > > Yes. It's definitely a race between the two. What percent of server LoFi page > loads are expected to use both types of LoFi. I expect an HTTP page with ads > would be a clear example. http:// pages with ads/analytics/social widgets are the main example. I don't know the actual percentage, but I'd guess that most http:// pages have at least one of those kinds of https:// resources on them, which implies that almost all pages that use Server LoFi will also use Client LoFi. https://codereview.chromium.org/2848293002/diff/60001/chrome/browser/previews... File chrome/browser/previews/previews_infobar_delegate.h (right): https://codereview.chromium.org/2848293002/diff/60001/chrome/browser/previews... chrome/browser/previews/previews_infobar_delegate.h:30: CLIENT_LOFI, // Client-side image replacement. On 2017/05/03 18:48:52, Ryan Sturm wrote: > On 2017/05/03 18:40:21, sclittle wrote: > > Since both Server LoFi and Client LoFi will often be used at the same time on > > the same page, what do we gain from having a separate CLIENT_LOFI value here? > > > > Could we just simplify this and make both kinds of LoFi use the same infobar > > type? > > I'd prefer to know the difference between the opt out rates of server LoFi vs > Client LoFi since they do actually have differences when it comes to sprite > detection, etc. Does Client LoFi only apply to HTTPS within a page load or can > it also be used on HTTP resources? Client LoFi tries to target all http:// and https:// images that don't look like they're already handled by another kind of preview. In practice, this means that it targets no images when Server Lite Pages is used, it targets only https:// images when Server LoFi is enabled for the page, and it targets both http:// and https:// images otherwise. See https://docs.google.com/document/d/1rWCON90u3j8JtgRk_SB6JTOIeKqp4rQjlRfj_W1zc... for more info.
On 2017/05/03 19:02:47, sclittle wrote: > https://codereview.chromium.org/2848293002/diff/60001/chrome/browser/previews... > File chrome/browser/previews/previews_infobar_delegate.cc (right): > > https://codereview.chromium.org/2848293002/diff/60001/chrome/browser/previews... > chrome/browser/previews/previews_infobar_delegate.cc:181: > web_contents->ReloadLoFiImages(); > On 2017/05/03 18:48:52, Ryan Sturm wrote: > > On 2017/05/03 18:40:21, sclittle wrote: > > > If "Show Original" is clicked on a page with both Server and Client LoFi, > and > > > Client LoFi is seen first and used for the infobar, then does this mean that > > > none of the above DRP opt-out stuff happens? > > > > Yes. It's definitely a race between the two. What percent of server LoFi page > > loads are expected to use both types of LoFi. I expect an HTTP page with ads > > would be a clear example. > > http:// pages with ads/analytics/social widgets are the main example. I don't > know the actual percentage, but I'd guess that most http:// pages have at least > one of those kinds of https:// resources on them, which implies that almost all > pages that use Server LoFi will also use Client LoFi. > > https://codereview.chromium.org/2848293002/diff/60001/chrome/browser/previews... > File chrome/browser/previews/previews_infobar_delegate.h (right): > > https://codereview.chromium.org/2848293002/diff/60001/chrome/browser/previews... > chrome/browser/previews/previews_infobar_delegate.h:30: CLIENT_LOFI, // > Client-side image replacement. > On 2017/05/03 18:48:52, Ryan Sturm wrote: > > On 2017/05/03 18:40:21, sclittle wrote: > > > Since both Server LoFi and Client LoFi will often be used at the same time > on > > > the same page, what do we gain from having a separate CLIENT_LOFI value > here? > > > > > > Could we just simplify this and make both kinds of LoFi use the same infobar > > > type? > > > > I'd prefer to know the difference between the opt out rates of server LoFi vs > > Client LoFi since they do actually have differences when it comes to sprite > > detection, etc. Does Client LoFi only apply to HTTPS within a page load or can > > it also be used on HTTP resources? > > Client LoFi tries to target all http:// and https:// images that don't look like > they're already handled by another kind of preview. > > In practice, this means that it targets no images when Server Lite Pages is > used, it targets only https:// images when Server LoFi is enabled for the page, > and it targets both http:// and https:// images otherwise. > > See > https://docs.google.com/document/d/1rWCON90u3j8JtgRk_SB6JTOIeKqp4rQjlRfj_W1zc... > for more info. I updated crbug.com/704335 to track removing the CLIENT_LOFI in both places. If you think that it should land in the other order, that is fine, but I'd rather land this first.
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...
sclittle: PTAL
https://codereview.chromium.org/2848293002/diff/80001/chrome/browser/net/spdy... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc (right): https://codereview.chromium.org/2848293002/diff/80001/chrome/browser/net/spdy... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:68: if (is_server_lofi) { If both Server and Client LoFi are used on a page, and Server LoFi wins the race, it looks like we're avoiding updating the blacklist here. What if we just add it to the blacklist in either case? That way, Client LoFi can use the blacklist, Server LoFi can continue to ignore the blacklist for now, and in the future when Server LoFi is moved onto the blacklist, it'll already have a bunch of history. Also, if Client LoFi was used, and we don't add it to the blacklist here, does that mean that the blacklist think that Client LoFi worked fine on the page, e.g. regarding the 2 out of the last 4 pages rule? https://codereview.chromium.org/2848293002/diff/80001/components/data_reducti... File components/data_reduction_proxy/content/browser/content_lofi_decider.cc (right): https://codereview.chromium.org/2848293002/diff/80001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:221: request_info->GetPreviewsState() & content::CLIENT_LOFI_ON; style nit: could you put parentheses around the bitwise check to make it a little cleaner? https://codereview.chromium.org/2848293002/diff/80001/components/data_reducti... File components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc (right): https://codereview.chromium.org/2848293002/diff/80001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc:799: content::RESOURCE_TYPE_MAIN_FRAME, true, content::CLIENT_LOFI_ON); nit: Could we also have a test for when it's an image without CLIENT_LOFI_ON set? https://codereview.chromium.org/2848293002/diff/80001/components/data_reducti... File components/data_reduction_proxy/content/browser/content_lofi_ui_service_unittest.cc (right): https://codereview.chromium.org/2848293002/diff/80001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_ui_service_unittest.cc:94: void VerifyOnLoFiResponseReceivedCallback(bool is_server_lofi) { nit: could this |is_server_lofi| argument here be renamed to |expected_is_server_lofi| here to distinguish it from the class member variable? https://codereview.chromium.org/2848293002/diff/80001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2848293002/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:393: *request, server_lofi /* is_server_lofi */); nit: I don't think you need the /* is_server_lofi */ comment here, since |server_lofi| should be pretty self-evident. https://codereview.chromium.org/2848293002/diff/80001/components/data_reducti... File components/data_reduction_proxy/core/common/lofi_ui_service.h (right): https://codereview.chromium.org/2848293002/diff/80001/components/data_reducti... components/data_reduction_proxy/core/common/lofi_ui_service.h:23: // |is_server_lofi| is whether this was a client LoFi optimization or a server nit: s/client LoFi/Client Lo-Fi/ https://codereview.chromium.org/2848293002/diff/80001/components/data_reducti... components/data_reduction_proxy/core/common/lofi_ui_service.h:24: // LoFi optimization. nit: for consistency, could you hyphenate LoFi as "Lo-Fi" in comments? Here and elsewhere in the CL
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 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...
https://codereview.chromium.org/2848293002/diff/80001/chrome/browser/net/spdy... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc (right): https://codereview.chromium.org/2848293002/diff/80001/chrome/browser/net/spdy... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:68: if (is_server_lofi) { On 2017/05/03 22:51:29, sclittle wrote: > If both Server and Client LoFi are used on a page, and Server LoFi wins the > race, it looks like we're avoiding updating the blacklist here. > > What if we just add it to the blacklist in either case? That way, Client LoFi > can use the blacklist, Server LoFi can continue to ignore the blacklist for now, > and in the future when Server LoFi is moved onto the blacklist, it'll already > have a bunch of history. > > Also, if Client LoFi was used, and we don't add it to the blacklist here, does > that mean that the blacklist think that Client LoFi worked fine on the page, > e.g. regarding the 2 out of the last 4 pages rule? Good Point. I was trying to land these CLs side-by-side since there isn't support for LitePage/ServerLofi in the blacklsit just yet. https://codereview.chromium.org/2848293002/diff/80001/components/data_reducti... File components/data_reduction_proxy/content/browser/content_lofi_decider.cc (right): https://codereview.chromium.org/2848293002/diff/80001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_decider.cc:221: request_info->GetPreviewsState() & content::CLIENT_LOFI_ON; On 2017/05/03 22:51:29, sclittle wrote: > style nit: could you put parentheses around the bitwise check to make it a > little cleaner? Done. https://codereview.chromium.org/2848293002/diff/80001/components/data_reducti... File components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc (right): https://codereview.chromium.org/2848293002/diff/80001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc:799: content::RESOURCE_TYPE_MAIN_FRAME, true, content::CLIENT_LOFI_ON); On 2017/05/03 22:51:29, sclittle wrote: > nit: Could we also have a test for when it's an image without CLIENT_LOFI_ON > set? Done. https://codereview.chromium.org/2848293002/diff/80001/components/data_reducti... File components/data_reduction_proxy/content/browser/content_lofi_ui_service_unittest.cc (right): https://codereview.chromium.org/2848293002/diff/80001/components/data_reducti... components/data_reduction_proxy/content/browser/content_lofi_ui_service_unittest.cc:94: void VerifyOnLoFiResponseReceivedCallback(bool is_server_lofi) { On 2017/05/03 22:51:29, sclittle wrote: > nit: could this |is_server_lofi| argument here be renamed to > |expected_is_server_lofi| here to distinguish it from the class member variable? Done. https://codereview.chromium.org/2848293002/diff/80001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2848293002/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:393: *request, server_lofi /* is_server_lofi */); On 2017/05/03 22:51:30, sclittle wrote: > nit: I don't think you need the /* is_server_lofi */ comment here, since > |server_lofi| should be pretty self-evident. Oops. Definitely agree. Done. https://codereview.chromium.org/2848293002/diff/80001/components/data_reducti... File components/data_reduction_proxy/core/common/lofi_ui_service.h (right): https://codereview.chromium.org/2848293002/diff/80001/components/data_reducti... components/data_reduction_proxy/core/common/lofi_ui_service.h:23: // |is_server_lofi| is whether this was a client LoFi optimization or a server On 2017/05/03 22:51:30, sclittle wrote: > nit: s/client LoFi/Client Lo-Fi/ Done. https://codereview.chromium.org/2848293002/diff/80001/components/data_reducti... components/data_reduction_proxy/core/common/lofi_ui_service.h:24: // LoFi optimization. On 2017/05/03 22:51:30, sclittle wrote: > nit: for consistency, could you hyphenate LoFi as "Lo-Fi" in comments? Here and > elsewhere in the CL Done.
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 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: 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 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.
sclittle: PTAL @ most recent patch
LGTM % nit, assuming you've tested it thoroughly. https://codereview.chromium.org/2848293002/diff/120001/chrome/browser/preview... File chrome/browser/previews/previews_infobar_delegate.h (right): https://codereview.chromium.org/2848293002/diff/120001/chrome/browser/preview... chrome/browser/previews/previews_infobar_delegate.h:27: LOFI, // Server-side image replacement. nit: could you update the comment here to indicate that it's both kinds of LoFi?
https://codereview.chromium.org/2848293002/diff/120001/chrome/browser/preview... File chrome/browser/previews/previews_infobar_delegate.h (right): https://codereview.chromium.org/2848293002/diff/120001/chrome/browser/preview... chrome/browser/previews/previews_infobar_delegate.h:27: LOFI, // Server-side image replacement. On 2017/05/08 20:28:01, sclittle wrote: > nit: could you update the comment here to indicate that it's both kinds of LoFi? Done.
The CQ bit was checked by ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sclittle@chromium.org Link to the patchset: https://codereview.chromium.org/2848293002/#ps140001 (title: "sclittle nit")
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": 140001, "attempt_start_ts": 1494275784619120,
"parent_rev": "d5843c47cf797621cfc476c9914f8cb5ce258e88", "commit_rev":
"2278661de721b27db538323ec60bfdff0aad064a"}
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494275784619120,
"parent_rev": "d5843c47cf797621cfc476c9914f8cb5ce258e88", "commit_rev":
"2278661de721b27db538323ec60bfdff0aad064a"}
Message was sent while issue was closed.
Description was changed from ========== Adding the Previews infobar to pages that show a client LoFi image This adds the Previews infobar to any page loads that use client LoFi for images. This follows the existing LoFi path for the infobar. Detection of client LoFi uses the PreviewsState and resource type at request completion time. Even if an image is full downloaded (due to small size) or we don't display the image, the infobar is shown because the range request was issued to the server and could have impacted the resulting image in some way. BUG=704322 ========== to ========== Adding the Previews infobar to pages that show a client LoFi image This adds the Previews infobar to any page loads that use client LoFi for images. This follows the existing LoFi path for the infobar. Detection of client LoFi uses the PreviewsState and resource type at request completion time. Even if an image is full downloaded (due to small size) or we don't display the image, the infobar is shown because the range request was issued to the server and could have impacted the resulting image in some way. BUG=704322 Review-Url: https://codereview.chromium.org/2848293002 Cr-Commit-Position: refs/heads/master@{#470148} Committed: https://chromium.googlesource.com/chromium/src/+/2278661de721b27db538323ec60b... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/chromium/src/+/2278661de721b27db538323ec60b... |
