Request uncompressed images from data reduction proxy when saving.

Request uncompressed images from data reduction proxy when saving.

When user saves an image, add headers requesting a pass through
from the data reduction proxy. Without this, images will be
saved in webp format which is not supported by most image viewers.
BUG=464436
Committed: https://crrev.com/6c7f969a830fe55df8b3be5a55305168b1c26106
Cr-Commit-Position: refs/heads/master@{#319681}

[Not at Google. Contact bengr, 2015-03-05 18:56:52]

kundaji@chromium.org changed reviewers:
+ avi@chromium.org

[Not at Google. Contact bengr, 2015-03-05 18:57:09]

avi:
  chrome/browser/renderer_context_menu/render_view_context_menu.cc
  content/browser/web_contents/web_contents_impl.cc
  content/browser/web_contents/web_contents_impl.h
  content/public/browser/web_contents.h

[Avi (use Gerrit), 2015-03-05 20:35:40]

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

https://codereview.chromium.org/988453002/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1394:
std::string header = "";
Drop the assignment; strings are constructed empty.

https://codereview.chromium.org/988453002/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1395: if
(!browser_context_->IsOffTheRecord() &&
I'm not thrilled with the idea of using the knowledge that there is no proxy for
incognito sessions here.

What does DataReductionProxyChromeSettingsFactory::GetForBrowserContext return
for an incognito session? If nullptr, then I'd much rather you do:

std::string header;
DataReductionProxyChromeSettings* settings =
DataReductionProxyChromeSettingsFactory::GetForBrowserContext(browser_context);
if (settings && settings->CanUseDataReductionProxy(params_.src_url))
  header = data_reduction_proxy::kDataReductionPassThroughHeader;

https://codereview.chromium.org/988453002/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/988453002/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2327: std::vector<std::string>
keyValueList;
style, please! key_value_list

https://codereview.chromium.org/988453002/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2329: for
(std::vector<std::string>::iterator it = keyValueList.begin();
for (auto& ...)

https://codereview.chromium.org/988453002/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2333: DCHECK(pair.size() ==
2);
DCHECK_EQ

https://codereview.chromium.org/988453002/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2342: SaveFrameWithHeader(url,
referrer, "");
"" -> std::string()

https://codereview.chromium.org/988453002/diff/40001/content/public/browser/w...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/988453002/diff/40001/content/public/browser/w...
content/public/browser/web_contents.h:478: // provided to make the request.
Document the format of the "headers" string.

https://codereview.chromium.org/988453002/diff/40001/content/public/browser/w...
content/public/browser/web_contents.h:479: virtual void
SaveFrameWithHeader(const GURL& url,
SaveFrameWithHeaders

https://codereview.chromium.org/988453002/diff/40001/content/public/browser/w...
content/public/browser/web_contents.h:481: const std::string& header) = 0;
There may be more than one header in this parameter; call it "headers".

[Not at Google. Contact bengr, 2015-03-06 22:23:48]

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

https://codereview.chromium.org/988453002/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1394:
std::string header = "";
On 2015/03/05 20:35:39, Avi wrote:
> Drop the assignment; strings are constructed empty.

Done.

https://codereview.chromium.org/988453002/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1395: if
(!browser_context_->IsOffTheRecord() &&
On 2015/03/05 20:35:39, Avi wrote:
> I'm not thrilled with the idea of using the knowledge that there is no proxy
for
> incognito sessions here.
> 
> What does DataReductionProxyChromeSettingsFactory::GetForBrowserContext return
> for an incognito session? If nullptr, then I'd much rather you do:
> 
> std::string header;
> DataReductionProxyChromeSettings* settings =
>
DataReductionProxyChromeSettingsFactory::GetForBrowserContext(browser_context);
> if (settings && settings->CanUseDataReductionProxy(params_.src_url))
>   header = data_reduction_proxy::kDataReductionPassThroughHeader;

Unfortunately, DataReductionProxyChromeSettingsFactory::GetForBrowserContext
does not return a null for incognito. It returns a valid object.

https://codereview.chromium.org/988453002/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/988453002/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2327: std::vector<std::string>
keyValueList;
On 2015/03/05 20:35:39, Avi wrote:
> style, please! key_value_list

Done.

https://codereview.chromium.org/988453002/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2329: for
(std::vector<std::string>::iterator it = keyValueList.begin();
On 2015/03/05 20:35:39, Avi wrote:
> for (auto& ...)

Done.

https://codereview.chromium.org/988453002/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2333: DCHECK(pair.size() ==
2);
On 2015/03/05 20:35:39, Avi wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/988453002/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2342: SaveFrameWithHeader(url,
referrer, "");
On 2015/03/05 20:35:39, Avi wrote:
> "" -> std::string()

Done.

https://codereview.chromium.org/988453002/diff/40001/content/public/browser/w...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/988453002/diff/40001/content/public/browser/w...
content/public/browser/web_contents.h:478: // provided to make the request.
On 2015/03/05 20:35:39, Avi wrote:
> Document the format of the "headers" string.

Good call. Done.

https://codereview.chromium.org/988453002/diff/40001/content/public/browser/w...
content/public/browser/web_contents.h:479: virtual void
SaveFrameWithHeader(const GURL& url,
On 2015/03/05 20:35:39, Avi wrote:
> SaveFrameWithHeaders

Done.

https://codereview.chromium.org/988453002/diff/40001/content/public/browser/w...
content/public/browser/web_contents.h:481: const std::string& header) = 0;
On 2015/03/05 20:35:39, Avi wrote:
> There may be more than one header in this parameter; call it "headers".

Done.

[Avi (use Gerrit), 2015-03-06 23:55:20]

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

https://codereview.chromium.org/988453002/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1395: if
(!browser_context_->IsOffTheRecord() &&
This is even weirder then. I'm assuming that there is no proxying done with
incognito. So why where would there be a valid factory?

It's rather unfortunate that everyone who calls
DataReductionProxyChromeSettingsFactory has to know not to call it for incognito
contexts. It would be nice if that knowledge were in the
DataReductionProxyChromeSettingsFactory.

https://codereview.chromium.org/988453002/diff/60001/chrome/browser/renderer_...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (left):

https://codereview.chromium.org/988453002/diff/60001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1313: 
Why drop the space here?

https://codereview.chromium.org/988453002/diff/60001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/988453002/diff/60001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2346: DCHECK_EQ(pair.size(),
2ul);
Switch parameters; when the DCHECK fails, the error message assumes the expected
value is the first param, not the second.

https://codereview.chromium.org/988453002/diff/60001/content/public/browser/w...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/988453002/diff/60001/content/public/browser/w...
content/public/browser/web_contents.h:484: // "<key1>: <value1>\n<key2>:
<value2>".
Are these spaces right? You split on ":", not on ": ".

[Not at Google. Contact bengr, 2015-03-07 00:45:03]

Thanks for the quick review.

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

https://codereview.chromium.org/988453002/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1395: if
(!browser_context_->IsOffTheRecord() &&
On 2015/03/06 23:55:20, Avi wrote:
> This is even weirder then. I'm assuming that there is no proxying done with
> incognito. So why where would there be a valid factory?
> 
> It's rather unfortunate that everyone who calls
> DataReductionProxyChromeSettingsFactory has to know not to call it for
incognito
> contexts. It would be nice if that knowledge were in the
> DataReductionProxyChromeSettingsFactory.

Done.

Even though DataReductionProxyChromeSettingsFactory doesn't differentiate
incognito, I realized that the default implementation in
BrowserContextKeyedBaseFactory is to return a null for incognito. So
DataReductionProxyChromeSettingsFactory::GetForBrowserContext(browser_context)
does indeed return null for incognito. Fixed another occurance of this check as
well.

https://codereview.chromium.org/988453002/diff/60001/chrome/browser/renderer_...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (left):

https://codereview.chromium.org/988453002/diff/60001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1313: 
On 2015/03/06 23:55:20, Avi wrote:
> Why drop the space here?

Done.

https://codereview.chromium.org/988453002/diff/60001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/988453002/diff/60001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2346: DCHECK_EQ(pair.size(),
2ul);
On 2015/03/06 23:55:20, Avi wrote:
> Switch parameters; when the DCHECK fails, the error message assumes the
expected
> value is the first param, not the second.

Done.

https://codereview.chromium.org/988453002/diff/60001/content/public/browser/w...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/988453002/diff/60001/content/public/browser/w...
content/public/browser/web_contents.h:484: // "<key1>: <value1>\n<key2>:
<value2>".
On 2015/03/06 23:55:20, Avi wrote:
> Are these spaces right? You split on ":", not on ": ".

Space doesn't matter since each they are trimmed by the SplitString method. Even
if the trimming didn't happen, this would still work since space is allowed in
headers.

[Avi (use Gerrit), 2015-03-07 00:50:02]

LGTM!

https://codereview.chromium.org/988453002/diff/60001/content/public/browser/w...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/988453002/diff/60001/content/public/browser/w...
content/public/browser/web_contents.h:484: // "<key1>: <value1>\n<key2>:
<value2>".
On 2015/03/07 00:45:02, kundaji wrote:
> Space doesn't matter since each they are trimmed by the SplitString method.

Oh. I didn't know that about SplitString. Thank you for pointing that out.

[Not at Google. Contact bengr, 2015-03-09 17:53:33]

The CQ bit was checked by kundaji@chromium.org

[Not at Google. Contact bengr, 2015-03-09 17:53:34]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org
Link to the patchset: https://codereview.chromium.org/988453002/#ps100001
(title: "Sync to head")

[commit-bot: I haz the power, 2015-03-09 17:54:39]

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

[commit-bot: I haz the power, 2015-03-09 18:00:50]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-09 18:01:26]

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