Context menu item to view original image when Data Saver is being used.

Context menu to view original image when Data Saver is being used.

Show menu item to view original image if the request could go over
the data reduction proxy.

BUG=456247
Committed: https://crrev.com/f28021f2f6547ae1aef2bf350caab24edd656732
Cr-Commit-Position: refs/heads/master@{#317720}

[Not at Google. Contact bengr, 2015-02-19 20:06:37]

kundaji@chromium.org changed reviewers:
+ asvitkine@google.com, ben.yan@msn.com, bengr@chromium.org

[Not at Google. Contact bengr, 2015-02-19 20:07:13]

asvitkine:
  tools/metrics/histograms/histograms.xml

ben:
  chrome/app/chrome_command_ids.h
  chrome/browser/renderer_context_menu/render_view_context_menu.cc
  chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
  components/renderer_context_menu/render_view_context_menu_base.cc
  components/renderer_context_menu/render_view_context_menu_base.h

bengr:
  components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
  components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h
 
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc

[Not at Google. Contact bengr, 2015-02-19 20:12:36]

kundaji@chromium.org changed reviewers:
+ asvitkine@chromium.org, cpu@chromium.org, lazyboy@chromium.org
- asvitkine@google.com, ben.yan@msn.com

[Not at Google. Contact bengr, 2015-02-19 20:14:12]

asvitkine:
  tools/metrics/histograms/histograms.xml

cpu:
  chrome/app/chrome_command_ids.h
  chrome/browser/renderer_context_menu/render_view_context_menu.cc
  chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
  components/renderer_context_menu/render_view_context_menu_base.cc
  components/renderer_context_menu/render_view_context_menu_base.h

bengr:
  components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
  components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h
 
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc

lazyboy:
  chrome/browser/renderer_context_menu/render_view_context_menu.cc
  chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
  components/renderer_context_menu/render_view_context_menu_base.cc
  components/renderer_context_menu/render_view_context_menu_base.h

[bengr, 2015-02-20 01:19:25]

lgtm, but fix the nit and add a TODO if the answer to my question is no.

https://codereview.chromium.org/944533002/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/944533002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:195:
return url.is_valid() && (url.scheme() != "https") &&
why not specifically check that the scheme is http? And there's a constant
defined for such schemes somewhere in net/ so use that.

https://codereview.chromium.org/944533002/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h
(right):

https://codereview.chromium.org/944533002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:116:
// return 'true' even when proxy will not be used. Specifically, if
"the proxy"

https://codereview.chromium.org/944533002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:122:
bool CanUseDataReductionProxy(const GURL& url) const;
Is this what we'll want to use going forward? Two other options to consider: (1)
always disable the data reduction proxy when opening an image in a new tab. (2)
making sure that the current image was loaded through the drp and the drp is
still enabled.

[Not at Google. Contact bengr, 2015-02-20 17:08:08]

https://codereview.chromium.org/944533002/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/944533002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:195:
return url.is_valid() && (url.scheme() != "https") &&
On 2015/02/20 01:19:25, bengr wrote:
> why not specifically check that the scheme is http? And there's a constant
> defined for such schemes somewhere in net/ so use that.

Copied from Android implementation:
https://cs.corp.google.com/#clankium/src/clank/java/apps/chrome/src/com/googl...
Not sure if there was a reason for using != "https" rather than "http". Do you
know?

Cannot find any defined constant for "https":
https://cs.corp.google.com/#search/&q=%5C%22https%5C%22%20package:%5Eclankium...

https://codereview.chromium.org/944533002/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h
(right):

https://codereview.chromium.org/944533002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:116:
// return 'true' even when proxy will not be used. Specifically, if
On 2015/02/20 01:19:25, bengr wrote:
> "the proxy"

Done.

https://codereview.chromium.org/944533002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:122:
bool CanUseDataReductionProxy(const GURL& url) const;
On 2015/02/20 01:19:25, bengr wrote:
> Is this what we'll want to use going forward? Two other options to consider:
(1)
> always disable the data reduction proxy when opening an image in a new tab.
(2)
> making sure that the current image was loaded through the drp and the drp is
> still enabled.

I think option 1 is a good one. We could always show "Open image in new tab" but
behavior could be dependent on whether data saver is on. This option has the
advantage that we don't need new string translations as well. Do you have a
preference here?

As for 2, we do check for proxy enabled already. But I think menu options be
independent on whether the currently loaded image was through proxy since this
is transparent to the end user.

[Alexei Svitkine (slow), 2015-02-20 18:37:27]

https://codereview.chromium.org/944533002/diff/20001/chrome/browser/renderer_...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/944533002/diff/20001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1420:
"X-PSA-Client-Options: v=1,m=1\nCache-Control: no-cache");
Hardcoding this string in context menu code is bad - it shouldn't have to know
about this thing.

Can a utility function be added somewhere more appropriate (in data reduction
proxy code) that this calls?

https://codereview.chromium.org/944533002/diff/20001/chrome/browser/renderer_...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/944533002/diff/20001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:49:
blink::WebContextMenuData::MediaType media_type) {
This doesn't follow style. Run "git cl format"?

https://codereview.chromium.org/944533002/diff/20001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:70:
TestRenderViewContextMenu* CreateContextMenu(GURL unfiltered_url, GURL url) {
Pass params by const refs.

Same as above.

Also, give a different name as above, as overloads are discouraged.

https://codereview.chromium.org/944533002/diff/20001/components/renderer_cont...
File components/renderer_context_menu/render_view_context_menu_base.cc (right):

https://codereview.chromium.org/944533002/diff/20001/components/renderer_cont...
components/renderer_context_menu/render_view_context_menu_base.cc:398: if
(!extra_headers.empty()) {
Nit: No {}'s

https://codereview.chromium.org/944533002/diff/20001/components/renderer_cont...
File components/renderer_context_menu/render_view_context_menu_base.h (right):

https://codereview.chromium.org/944533002/diff/20001/components/renderer_cont...
components/renderer_context_menu/render_view_context_menu_base.h:164: void
OpenURL(const GURL& url, const GURL& referrer,
Overloads are discouraged. OpenURLWithExtraHeaders().

Also, one param per line if params wrap - or let "git cl format" do this for
you.

Add a line comment.

[Not at Google. Contact bengr, 2015-02-20 18:43:24]

New patchsets have been uploaded after l-g-t-m from bengr@chromium.org

[Not at Google. Contact bengr, 2015-02-20 18:43:38]

https://codereview.chromium.org/944533002/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/944533002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:195:
return url.is_valid() && (url.scheme() != "https") &&
On 2015/02/20 01:19:25, bengr wrote:
> why not specifically check that the scheme is http? And there's a constant
> defined for such schemes somewhere in net/ so use that.

Used url::kHttpScheme and changed to '=='.

https://codereview.chromium.org/944533002/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h
(right):

https://codereview.chromium.org/944533002/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:122:
bool CanUseDataReductionProxy(const GURL& url) const;
On 2015/02/20 17:08:08, kundaji wrote:
> On 2015/02/20 01:19:25, bengr wrote:
> > Is this what we'll want to use going forward? Two other options to consider:
> (1)
> > always disable the data reduction proxy when opening an image in a new tab.
> (2)
> > making sure that the current image was loaded through the drp and the drp is
> > still enabled.
> 
> I think option 1 is a good one. We could always show "Open image in new tab"
but
> behavior could be dependent on whether data saver is on. This option has the
> advantage that we don't need new string translations as well. Do you have a
> preference here?
> 
> As for 2, we do check for proxy enabled already. But I think menu options be
> independent on whether the currently loaded image was through proxy since this
> is transparent to the end user. 

Discussed in person. We need the "Open original image.." string because
otherwise users will have not indication that they have the option of
downloading the original image as opposed to using "save image" and saving the
webp. So keeping this as is.

Will file a bug to reconsider the UX here.

[Not at Google. Contact bengr, 2015-02-21 00:11:17]

https://codereview.chromium.org/944533002/diff/20001/chrome/browser/renderer_...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/944533002/diff/20001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1420:
"X-PSA-Client-Options: v=1,m=1\nCache-Control: no-cache");
On 2015/02/20 18:37:27, Alexei Svitkine wrote:
> Hardcoding this string in context menu code is bad - it shouldn't have to know
> about this thing.
> 
> Can a utility function be added somewhere more appropriate (in data reduction
> proxy code) that this calls?

Moved the string to drp_settings. The OpenUrl(..) method is specific to context
menu so cannot move that call.

https://codereview.chromium.org/944533002/diff/20001/chrome/browser/renderer_...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/944533002/diff/20001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:49:
blink::WebContextMenuData::MediaType media_type) {
On 2015/02/20 18:37:27, Alexei Svitkine wrote:
> This doesn't follow style. Run "git cl format"?

Done.

https://codereview.chromium.org/944533002/diff/20001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:70:
TestRenderViewContextMenu* CreateContextMenu(GURL unfiltered_url, GURL url) {
On 2015/02/20 18:37:27, Alexei Svitkine wrote:
> Pass params by const refs.
> 
> Same as above.
> 
> Also, give a different name as above, as overloads are discouraged.

Done.

https://codereview.chromium.org/944533002/diff/20001/components/renderer_cont...
File components/renderer_context_menu/render_view_context_menu_base.cc (right):

https://codereview.chromium.org/944533002/diff/20001/components/renderer_cont...
components/renderer_context_menu/render_view_context_menu_base.cc:398: if
(!extra_headers.empty()) {
On 2015/02/20 18:37:27, Alexei Svitkine wrote:
> Nit: No {}'s

Done.

https://codereview.chromium.org/944533002/diff/20001/components/renderer_cont...
File components/renderer_context_menu/render_view_context_menu_base.h (right):

https://codereview.chromium.org/944533002/diff/20001/components/renderer_cont...
components/renderer_context_menu/render_view_context_menu_base.h:164: void
OpenURL(const GURL& url, const GURL& referrer,
On 2015/02/20 18:37:27, Alexei Svitkine wrote:
> Overloads are discouraged. OpenURLWithExtraHeaders().
> 
> Also, one param per line if params wrap - or let "git cl format" do this for
> you.
> 
> Add a line comment.

Done.

[Alexei Svitkine (slow), 2015-02-23 19:13:16]

lgtm % comments

https://codereview.chromium.org/944533002/diff/100001/chrome/browser/renderer...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/944533002/diff/100001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:318:
CreateContextMenu(GURL(""), GURL("http://url.com/image.png"),
Nit: GURL("") -> GURL()

https://codereview.chromium.org/944533002/diff/100001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:333:
CreateContextMenu(GURL(""), GURL("https://url.com/image.png"),
Ditto.

https://codereview.chromium.org/944533002/diff/100001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:343:
CreateContextMenu(GURL(""), GURL("http://url.com/image.png"),
Ditto.

https://codereview.chromium.org/944533002/diff/100001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/944533002/diff/100001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:122:
return url.is_valid() && (url.scheme() == url::kHttpScheme) &&
Nit: No need for parens around the == cond.

https://codereview.chromium.org/944533002/diff/100001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h
(right):

https://codereview.chromium.org/944533002/diff/100001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:32:
extern const char kDataReductionPassThroughHeader[];
Add a comment.

https://codereview.chromium.org/944533002/diff/100001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:83:
// return 'true' even when the proxy will not be used. Specifically, if
Nit: Don't quote true. Either nothing around it, or ||'s. Make it consistent,
since the rest of the comment doesn't quote true or false.

[Not at Google. Contact bengr, 2015-02-23 20:39:00]

https://codereview.chromium.org/944533002/diff/100001/chrome/browser/renderer...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/944533002/diff/100001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:318:
CreateContextMenu(GURL(""), GURL("http://url.com/image.png"),
On 2015/02/23 19:13:15, Alexei Svitkine wrote:
> Nit: GURL("") -> GURL()

Done.

https://codereview.chromium.org/944533002/diff/100001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:333:
CreateContextMenu(GURL(""), GURL("https://url.com/image.png"),
On 2015/02/23 19:13:15, Alexei Svitkine wrote:
> Ditto.

Done.

https://codereview.chromium.org/944533002/diff/100001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:343:
CreateContextMenu(GURL(""), GURL("http://url.com/image.png"),
On 2015/02/23 19:13:16, Alexei Svitkine wrote:
> Ditto.

Done.

https://codereview.chromium.org/944533002/diff/100001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/944533002/diff/100001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:122:
return url.is_valid() && (url.scheme() == url::kHttpScheme) &&
On 2015/02/23 19:13:16, Alexei Svitkine wrote:
> Nit: No need for parens around the == cond.

Done.

https://codereview.chromium.org/944533002/diff/100001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h
(right):

https://codereview.chromium.org/944533002/diff/100001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:32:
extern const char kDataReductionPassThroughHeader[];
On 2015/02/23 19:13:16, Alexei Svitkine wrote:
> Add a comment.

Done.

https://codereview.chromium.org/944533002/diff/100001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:83:
// return 'true' even when the proxy will not be used. Specifically, if
On 2015/02/23 19:13:16, Alexei Svitkine wrote:
> Nit: Don't quote true. Either nothing around it, or ||'s. Make it consistent,
> since the rest of the comment doesn't quote true or false.

Done.

[lazyboy, 2015-02-23 20:56:46]

lgtm

https://codereview.chromium.org/944533002/diff/140001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/944533002/diff/140001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:123:
IsDataReductionProxyEnabled();
nit: indent 4 spaces from previous line.

[cpu_(ooo_6.6-7.5), 2015-02-23 21:11:16]

chrome_command_ids.h lgtm

I did not look at the rest of the code.

[Not at Google. Contact bengr, 2015-02-23 21:25:11]

https://codereview.chromium.org/944533002/diff/140001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/944533002/diff/140001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:123:
IsDataReductionProxyEnabled();
On 2015/02/23 20:56:46, lazyboy wrote:
> nit: indent 4 spaces from previous line.

Done.

[Not at Google. Contact bengr, 2015-02-23 22:18:07]

The CQ bit was checked by kundaji@chromium.org

[Not at Google. Contact bengr, 2015-02-23 22:18:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, bengr@chromium.org, lazyboy@chromium.org,
cpu@chromium.org
Link to the patchset: https://codereview.chromium.org/944533002/#ps160001
(title: "indent 4 space.")

[commit-bot: I haz the power, 2015-02-23 22:19:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/944533002/160001

[commit-bot: I haz the power, 2015-02-23 23:19:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-23 23:19:17]

Failed to apply the patch.

[commit-bot: I haz the power, 2015-02-23 23:20:10]

The CQ bit was unchecked by commit-bot@chromium.org

[Not at Google. Contact bengr, 2015-02-24 00:09:33]

The CQ bit was checked by kundaji@chromium.org

[Not at Google. Contact bengr, 2015-02-24 00:09:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, bengr@chromium.org, lazyboy@chromium.org,
cpu@chromium.org
Link to the patchset: https://codereview.chromium.org/944533002/#ps180001
(title: "Sync to head")

[commit-bot: I haz the power, 2015-02-24 00:10:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/944533002/180001

[commit-bot: I haz the power, 2015-02-24 01:27:17]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-02-24 01:28:18]

Patchset 10 (id:??) landed as
https://crrev.com/f28021f2f6547ae1aef2bf350caab24edd656732
Cr-Commit-Position: refs/heads/master@{#317720}