Save image should download the original image when Data Saver is enabled.

Save image should download the original image when Data
Saver is enabled.

The download manager should not use the cached image and
should bypass Data Saver.

BUG=499941
Committed: https://crrev.com/3d5616a69557d664cc9a3406962df28abe0c1db1
Cr-Commit-Position: refs/heads/master@{#335747}

[megjablon, 2015-06-12 22:53:24]

Patchset #1 (id:1) has been deleted

[megjablon, 2015-06-12 22:56:05]

megjablon@chromium.org changed reviewers:
+ tedchoc@chromium.org

[megjablon, 2015-06-16 00:28:34]

Patchset #1 (id:20001) has been deleted

[megjablon, 2015-06-17 17:54:32]

PTAL

[Ted C, 2015-06-22 20:40:49]

https://codereview.chromium.org/1182293002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java
(right):

https://codereview.chromium.org/1182293002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:82:
if (mNativeContextMenuHelper != 0)
nativeOnStartDownload(mNativeContextMenuHelper, isLink,
braces required since it's more than one line now

https://codereview.chromium.org/1182293002/diff/40001/content/browser/android...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1182293002/diff/40001/content/browser/android...
content/browser/android/download_controller_android_impl.cc:411:
base::SplitString(headers, '\n', &key_value_list);
Instead of this, could it use something like
HttpRequestHeaders::AddHeadersFromString?  I was hoping there would be some sort
of shareable logic for this if possible.  (note the \r\n requirement for that
though)

[bengr, 2015-06-22 22:13:09]

bengr@chromium.org changed reviewers:
+ bengr@chromium.org

[bengr, 2015-06-22 22:13:10]

https://codereview.chromium.org/1182293002/diff/40001/content/browser/android...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1182293002/diff/40001/content/browser/android...
content/browser/android/download_controller_android_impl.cc:411:
base::SplitString(headers, '\n', &key_value_list);
On 2015/06/22 20:40:49, Ted C wrote:
> Instead of this, could it use something like
> HttpRequestHeaders::AddHeadersFromString?  I was hoping there would be some
sort
> of shareable logic for this if possible.  (note the \r\n requirement for that
> though)

Agreed.

[megjablon, 2015-06-22 23:21:02]

https://codereview.chromium.org/1182293002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java
(right):

https://codereview.chromium.org/1182293002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:82:
if (mNativeContextMenuHelper != 0)
nativeOnStartDownload(mNativeContextMenuHelper, isLink,
On 2015/06/22 20:40:49, Ted C wrote:
> braces required since it's more than one line now

Done.

https://codereview.chromium.org/1182293002/diff/40001/content/browser/android...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1182293002/diff/40001/content/browser/android...
content/browser/android/download_controller_android_impl.cc:411:
base::SplitString(headers, '\n', &key_value_list);
On 2015/06/22 20:40:49, Ted C wrote:
> Instead of this, could it use something like
> HttpRequestHeaders::AddHeadersFromString?  I was hoping there would be some
sort
> of shareable logic for this if possible.  (note the \r\n requirement for that
> though)

I don't see another way to add the headers here other than
DownloadURLParameters::add_request_header. We could modify that or add a
add_request_headers_from_string, but that would just move the splitting logic
elsewhere. web_contents_impl.cc also uses this same strategy to add headers to
DowloadURLParameters.

[Ted C, 2015-06-22 23:25:28]

https://codereview.chromium.org/1182293002/diff/40001/content/browser/android...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1182293002/diff/40001/content/browser/android...
content/browser/android/download_controller_android_impl.cc:411:
base::SplitString(headers, '\n', &key_value_list);
On 2015/06/22 23:21:02, megjablon wrote:
> On 2015/06/22 20:40:49, Ted C wrote:
> > Instead of this, could it use something like
> > HttpRequestHeaders::AddHeadersFromString?  I was hoping there would be some
> sort
> > of shareable logic for this if possible.  (note the \r\n requirement for
that
> > though)
> 
> I don't see another way to add the headers here other than
> DownloadURLParameters::add_request_header. We could modify that or add a
> add_request_headers_from_string, but that would just move the splitting logic
> elsewhere. web_contents_impl.cc also uses this same strategy to add headers to
> DowloadURLParameters.

I was thinking that you would still use
DownloadURLParameters::add_request_header, but HttpRequestHeaders would parse
the string and allows you to iterate over the pair.

i.e.:

net::HttpRequestHeaders headers;
headers.AddHeadersFromString(extra_headers);
for (net::HttpRequestHeaders::Iterator it(headers); it.GetNext();) {
  dl_params->add_request_header(it.name(), it.value());
}

[megjablon, 2015-06-23 00:09:25]

Patchset #3 (id:80001) has been deleted

[megjablon, 2015-06-23 00:54:27]

https://codereview.chromium.org/1182293002/diff/40001/content/browser/android...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1182293002/diff/40001/content/browser/android...
content/browser/android/download_controller_android_impl.cc:411:
base::SplitString(headers, '\n', &key_value_list);
On 2015/06/22 23:25:28, Ted C wrote:
> On 2015/06/22 23:21:02, megjablon wrote:
> > On 2015/06/22 20:40:49, Ted C wrote:
> > > Instead of this, could it use something like
> > > HttpRequestHeaders::AddHeadersFromString?  I was hoping there would be
some
> > sort
> > > of shareable logic for this if possible.  (note the \r\n requirement for
> that
> > > though)
> > 
> > I don't see another way to add the headers here other than
> > DownloadURLParameters::add_request_header. We could modify that or add a
> > add_request_headers_from_string, but that would just move the splitting
logic
> > elsewhere. web_contents_impl.cc also uses this same strategy to add headers
to
> > DowloadURLParameters.
> 
> I was thinking that you would still use
> DownloadURLParameters::add_request_header, but HttpRequestHeaders would parse
> the string and allows you to iterate over the pair.
> 
> i.e.:
> 
> net::HttpRequestHeaders headers;
> headers.AddHeadersFromString(extra_headers);
> for (net::HttpRequestHeaders::Iterator it(headers); it.GetNext();) {
>   dl_params->add_request_header(it.name(), it.value());
> }

Ah my bad. I misunderstood. Done.

[bengr, 2015-06-23 04:19:35]

lgtm with nit

https://codereview.chromium.org/1182293002/diff/100001/content/browser/androi...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1182293002/diff/100001/content/browser/androi...
content/browser/android/download_controller_android_impl.cc:413: }
You can remove the curly braces.

[megjablon, 2015-06-23 04:30:23]

https://codereview.chromium.org/1182293002/diff/100001/content/browser/androi...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1182293002/diff/100001/content/browser/androi...
content/browser/android/download_controller_android_impl.cc:413: }
On 2015/06/23 04:19:35, bengr wrote:
> You can remove the curly braces.

Done.

[Ted C, 2015-06-23 16:32:53]

lgtm w/ a couple nits

https://codereview.chromium.org/1182293002/diff/120001/content/browser/androi...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1182293002/diff/120001/content/browser/androi...
content/browser/android/download_controller_android_impl.cc:412:
dl_params->add_request_header(it.name(), it.value());
indented 2 too many

https://codereview.chromium.org/1182293002/diff/120001/content/public/browser...
File content/public/browser/android/download_controller_android.h (right):

https://codereview.chromium.org/1182293002/diff/120001/content/public/browser...
content/public/browser/android/download_controller_android.h:35: bool is_link,
const std::string& headers) = 0;
I would call this extra_headers to be consistent (or rename the others to
headers -- whatever you prefer)

[megjablon, 2015-06-23 20:29:48]

https://codereview.chromium.org/1182293002/diff/120001/content/browser/androi...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1182293002/diff/120001/content/browser/androi...
content/browser/android/download_controller_android_impl.cc:412:
dl_params->add_request_header(it.name(), it.value());
On 2015/06/23 16:32:53, Ted C wrote:
> indented 2 too many

Done.

https://codereview.chromium.org/1182293002/diff/120001/content/public/browser...
File content/public/browser/android/download_controller_android.h (right):

https://codereview.chromium.org/1182293002/diff/120001/content/public/browser...
content/public/browser/android/download_controller_android.h:35: bool is_link,
const std::string& headers) = 0;
On 2015/06/23 16:32:53, Ted C wrote:
> I would call this extra_headers to be consistent (or rename the others to
> headers -- whatever you prefer)

Done.

[megjablon, 2015-06-23 20:29:54]

The CQ bit was checked by megjablon@chromium.org

[megjablon, 2015-06-23 20:29:55]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org,
tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/1182293002/#ps140001
(title: "nits")

[commit-bot: I haz the power, 2015-06-23 20:30:08]

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

[commit-bot: I haz the power, 2015-06-23 21:19:52]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2015-06-23 21:22:00]

Patchset 5 (id:??) landed as
https://crrev.com/3d5616a69557d664cc9a3406962df28abe0c1db1
Cr-Commit-Position: refs/heads/master@{#335747}