Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2465253005:
Fix 'Load image' context menu item (Closed)
DescriptionFix 'Load image' context menu item
The 'Load image context menu item isn't showing because the new
Content-Transform header was not being checked. Check for the
"chrome-proxy-content-transform" instead of the "chrome-proxy" header
when adding the image context menu params.
BUG=620306
Committed: https://crrev.com/050660fcb39c7f161a687c33d4b19571b874b18e
Cr-Commit-Position: refs/heads/master@{#429383}
Patch Set 1 #
Total comments: 4
Patch Set 2 : thestig comments #
Total comments: 6
Patch Set 3 : ryansturm comments #Patch Set 4 : fix dependent patchset #
Messages
Total messages: 17 (7 generated)
Description was changed from ========== Fix 'Load image' context menu item The 'Load image context menu item isn't showing because the new Content-Transform header was not being checked. Check for the "chrome-proxy-content-transform" instead of the "Chrome-Proxy" header when adding the image context menu params. BUG=620306 ========== to ========== Fix 'Load image' context menu item The 'Load image context menu item isn't showing because the new Content-Transform header was not being checked. Check for the "chrome-proxy-content-transform" instead of the "chrome-proxy" header when adding the image context menu params. BUG=620306 ==========
megjablon@chromium.org changed reviewers: + thestig@chromium.org
PTAL, thanks!
Can you ask someone on your team to review as well? https://codereview.chromium.org/2465253005/diff/1/chrome/renderer/chrome_cont... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2465253005/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client_unittest.cc:419: blink::WebURLResponse webUrlResponse; nit: web_url_response https://codereview.chromium.org/2465253005/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client_unittest.cc:429: data_reduction_proxy::empty_image_directive()); Is this the expected value? If so, flip the arguments because EXPECT_EQ() takes EXPECT_EQ(expected, actual).
megjablon@chromium.org changed reviewers: + ryansturm@chromium.org
Ryan, PTAL https://codereview.chromium.org/2465253005/diff/1/chrome/renderer/chrome_cont... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2465253005/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client_unittest.cc:419: blink::WebURLResponse webUrlResponse; On 2016/11/01 22:59:42, Lei Zhang wrote: > nit: web_url_response Whoops, got my head in android world. https://codereview.chromium.org/2465253005/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client_unittest.cc:429: data_reduction_proxy::empty_image_directive()); On 2016/11/01 22:59:42, Lei Zhang wrote: > Is this the expected value? If so, flip the arguments because EXPECT_EQ() takes > EXPECT_EQ(expected, actual). Done.
https://codereview.chromium.org/2465253005/diff/20001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2465253005/diff/20001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_unittest.cc:425: std::map<std::string, std::string> properties; #include <map> https://codereview.chromium.org/2465253005/diff/20001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_unittest.cc:425: std::map<std::string, std::string> properties; #include <string> https://codereview.chromium.org/2465253005/diff/20001/content/public/test/ren... File content/public/test/render_view_test.cc (right): https://codereview.chromium.org/2465253005/diff/20001/content/public/test/ren... content/public/test/render_view_test.cc:514: PREVIEW_UNSPECIFIED, base::TimeTicks::Now(), "GET", nullptr); PREVIEW_UNSPECIFIED here seems like it is from a different CL maybe. If not, I don't understand this part of the change.
https://codereview.chromium.org/2465253005/diff/20001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2465253005/diff/20001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_unittest.cc:425: std::map<std::string, std::string> properties; On 2016/11/01 23:49:14, Ryan Sturm wrote: > #include <string> Done. https://codereview.chromium.org/2465253005/diff/20001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_unittest.cc:425: std::map<std::string, std::string> properties; On 2016/11/01 23:49:14, Ryan Sturm wrote: > #include <map> Done. https://codereview.chromium.org/2465253005/diff/20001/content/public/test/ren... File content/public/test/render_view_test.cc (right): https://codereview.chromium.org/2465253005/diff/20001/content/public/test/ren... content/public/test/render_view_test.cc:514: PREVIEW_UNSPECIFIED, base::TimeTicks::Now(), "GET", nullptr); On 2016/11/01 23:49:14, Ryan Sturm wrote: > PREVIEW_UNSPECIFIED here seems like it is from a different CL maybe. If not, I > don't understand this part of the change. Ya this was from rebasing and re-parenting branches so I could try this out in the cl I caught this issue in. Whoops.
lgtm
lgtm
The CQ bit was checked by megjablon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, ryansturm@chromium.org Link to the patchset: https://codereview.chromium.org/2465253005/#ps60001 (title: "fix dependent patchset")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix 'Load image' context menu item The 'Load image context menu item isn't showing because the new Content-Transform header was not being checked. Check for the "chrome-proxy-content-transform" instead of the "chrome-proxy" header when adding the image context menu params. BUG=620306 ========== to ========== Fix 'Load image' context menu item The 'Load image context menu item isn't showing because the new Content-Transform header was not being checked. Check for the "chrome-proxy-content-transform" instead of the "chrome-proxy" header when adding the image context menu params. BUG=620306 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix 'Load image' context menu item The 'Load image context menu item isn't showing because the new Content-Transform header was not being checked. Check for the "chrome-proxy-content-transform" instead of the "chrome-proxy" header when adding the image context menu params. BUG=620306 ========== to ========== Fix 'Load image' context menu item The 'Load image context menu item isn't showing because the new Content-Transform header was not being checked. Check for the "chrome-proxy-content-transform" instead of the "chrome-proxy" header when adding the image context menu params. BUG=620306 Committed: https://crrev.com/050660fcb39c7f161a687c33d4b19571b874b18e Cr-Commit-Position: refs/heads/master@{#429383} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/050660fcb39c7f161a687c33d4b19571b874b18e Cr-Commit-Position: refs/heads/master@{#429383} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
