"Load image" context menu item to reload a LoFi image.

"Load image" context menu item to reload a LoFi image.

LoFi uses solid color image placeholders. Provide the user a context
menu option to load the origial image in place. Also, provide a way
for the context menu to know that an image was LoFi.

BUG=485295
Committed: https://crrev.com/3eec0d31013c3596f418deaec96e119d1125b607
Cr-Commit-Position: refs/heads/master@{#330267}

[megjablon, 2015-04-28 23:48:34]

Patchset #1 (id:1) has been deleted

[megjablon, 2015-05-04 20:58:20]

Patchset #2 (id:40001) has been deleted

[megjablon, 2015-05-07 00:24:42]

Patchset #1 (id:20001) has been deleted

[megjablon, 2015-05-07 00:25:30]

Patchset #2 (id:80001) has been deleted

[megjablon, 2015-05-07 00:32:52]

Patchset #2 (id:100001) has been deleted

[megjablon, 2015-05-07 00:34:20]

Patchset #1 (id:60001) has been deleted

[megjablon, 2015-05-07 23:56:26]

Patchset #2 (id:140001) has been deleted

[megjablon, 2015-05-07 23:56:30]

Patchset #1 (id:120001) has been deleted

[megjablon, 2015-05-07 23:59:16]

Patchset #1 (id:160001) has been deleted

[megjablon, 2015-05-08 00:46:39]

Patchset #1 (id:180001) has been deleted

[megjablon, 2015-05-08 19:15:01]

Patchset #1 (id:200001) has been deleted

[megjablon, 2015-05-11 19:15:18]

Patchset #1 (id:220001) has been deleted

[megjablon, 2015-05-11 19:48:04]

Patchset #1 (id:240001) has been deleted

[megjablon, 2015-05-11 19:57:40]

megjablon@chromium.org changed reviewers:
+ bengr@chromium.org, creis@chromium.org, thestig@chromium.org

[megjablon, 2015-05-11 19:57:41]

bengr: components/*
creis: content/*
thestig: chrome/*

[Lei Zhang, 2015-05-11 20:16:54]

You should find someone else to review chrome/android.

https://codereview.chromium.org/1105263004/diff/260001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1105263004/diff/260001/chrome/app/generated_r...
chrome/app/generated_resources.grd:1016: +            Load image
Is this going to be Android-only? If so, why not just provide the version that
Android uses and omit the other one? Also do the GRD equivalent of #if
defined(OS_ANDROID).

Also, shouldn't this be "Load Image" ?

https://codereview.chromium.org/1105263004/diff/260001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1105263004/diff/260001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:219: {66,
IDC_CONTENT_CONTEXT_RELOAD_ORIGINAL_IMAGE},
You need to update tools/metrics/histograms/histograms.xml. Look for
"IDC_WRITING_DIRECTION_RTL" in histograms.xml. Can you add a comment here since
it's not obvious that one should do that?

https://codereview.chromium.org/1105263004/diff/260001/chrome/browser/rendere...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/1105263004/diff/260001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:321:
IN_PROC_BROWSER_TEST_F(ContextMenuBrowserTest, DataSaverLoadImage) {
Can this be a unit test instead?

https://codereview.chromium.org/1105263004/diff/260001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:569:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
Use DCHECK_CURRENTLY_ON()

https://codereview.chromium.org/1105263004/diff/260001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:638:
// URLRequestFilter::ClearHandlers(), |interceptor_| can become invalid.
I realized this is copied over from another test, but how does
LoadImageBrowserTest know when |interceptor_| is invalid, and when to stop using
it?

https://codereview.chromium.org/1105263004/diff/260001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc
(right):

https://codereview.chromium.org/1105263004/diff/260001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc:14:
#include
"components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h"
Is this needed?

[bengr, 2015-05-11 20:35:40]

https://codereview.chromium.org/1105263004/diff/260001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
(right):

https://codereview.chromium.org/1105263004/diff/260001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:226:
request ? request->load_flags() : net::LOAD_NORMAL);
Since looking at the load flags is probably not the way we'll do this in the
long run, I'd instead make the decision here, and pass the result in. E.g.:

MaybeAdd...(..., ShouldForceDisableLoFi(request));


namespace { 
bool ShouldForceDisableLoFi(const URLRequest* request) {
 if (!request)
    return false;
 return request->load_flags() & LOAD_BYPASS_CACHE;
}
}

https://codereview.chromium.org/1105263004/diff/260001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h
(right):

https://codereview.chromium.org/1105263004/diff/260001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h:173:
void UpdateLoFi(int request_load_flags);
Add a comment that the LoFi header is not added if the request bypasses cache.

[Charlie Reis, 2015-05-11 22:32:57]

creis@chromium.org changed reviewers:
+ jam@chromium.org

[Charlie Reis, 2015-05-11 22:32:58]

[+jam for content API question]

https://codereview.chromium.org/1105263004/diff/260001/content/child/web_url_...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/1105263004/diff/260001/content/child/web_url_...
content/child/web_url_loader_impl.cc:987:
info.headers->HasHeaderValue("Chrome-Proxy", "q=low"));
It's not clear to me that this belongs in content/.  Looks like all the places
that set and check it are outside content/.  @jam, is there a way for embedders
to add things to a response's extra data?

https://codereview.chromium.org/1105263004/diff/260001/content/public/common/...
File content/public/common/context_menu_params.h (right):

https://codereview.chromium.org/1105263004/diff/260001/content/public/common/...
content/public/common/context_menu_params.h:87: // this contains the if the
image was a Lo-Fi image.
nit: this contains whether the image

https://codereview.chromium.org/1105263004/diff/260001/content/public/common/...
content/public/common/context_menu_params.h:88: bool image_was_fetched_lo_fi;
Same question: all the uses seem to be outside content, which suggests that this
should not be in the content public interface.

https://www.chromium.org/developers/content-module/content-api
"all classes/structs/enums in the public API must be used by embedders and
content. i.e. if the chrome layer uses a struct but content doesn't know about
it, it doesn't belong in content/public but instead some module that's higher
level."

[megjablon, 2015-05-11 22:58:41]

https://codereview.chromium.org/1105263004/diff/260001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1105263004/diff/260001/chrome/app/generated_r...
chrome/app/generated_resources.grd:1016: +            Load image
On 2015/05/11 20:16:53, Lei Zhang wrote:
> Is this going to be Android-only? If so, why not just provide the version that
> Android uses and omit the other one? Also do the GRD equivalent of #if
> defined(OS_ANDROID).
> 
> Also, shouldn't this be "Load Image" ?

This CL creates the context menu option "Load image" for both desktop and
Android. Currently, this feature can be turned on for both desktop and Android
using a flag, and while we will only be experimenting this on mobile, it is
helpful to have desktop implementations for manual testing.

I'm using the same format as other context menu options, such as "Copy image"
and "Save image," which only have the first word capitalized.

https://codereview.chromium.org/1105263004/diff/260001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1105263004/diff/260001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:219: {66,
IDC_CONTENT_CONTEXT_RELOAD_ORIGINAL_IMAGE},
On 2015/05/11 20:16:53, Lei Zhang wrote:
> You need to update tools/metrics/histograms/histograms.xml. Look for
> "IDC_WRITING_DIRECTION_RTL" in histograms.xml. Can you add a comment here
since
> it's not obvious that one should do that?

Done.

https://codereview.chromium.org/1105263004/diff/260001/chrome/browser/rendere...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/1105263004/diff/260001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:321:
IN_PROC_BROWSER_TEST_F(ContextMenuBrowserTest, DataSaverLoadImage) {
On 2015/05/11 20:16:53, Lei Zhang wrote:
> Can this be a unit test instead?

I looked into writing it as a unit test, but the
IDC_CONTENT_CONTEXT_OPENIMAGENEWTAB option in AppendImageItems makes this
complicated because the params in DataReductionProxyChromeSettings do not get
initialized and settings->CanUseDataReductionProxy(params_.src_url) crashes.

https://codereview.chromium.org/1105263004/diff/260001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:569:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
On 2015/05/11 20:16:53, Lei Zhang wrote:
> Use DCHECK_CURRENTLY_ON()

Done.

https://codereview.chromium.org/1105263004/diff/260001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:638:
// URLRequestFilter::ClearHandlers(), |interceptor_| can become invalid.
On 2015/05/11 20:16:53, Lei Zhang wrote:
> I realized this is copied over from another test, but how does
> LoadImageBrowserTest know when |interceptor_| is invalid, and when to stop
using
> it?

Looks like ClearHandlers is only called by tests and was on tear down from where
I followed this implementation, so I don't think this comment is necessary.

https://codereview.chromium.org/1105263004/diff/260001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc
(right):

https://codereview.chromium.org/1105263004/diff/260001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc:14:
#include
"components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h"
On 2015/05/11 20:16:53, Lei Zhang wrote:
> Is this needed?

Nope, sorry I missed this. This was left in here when I was trying to write the
unit test that ended up being a browser test.

https://codereview.chromium.org/1105263004/diff/260001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
(right):

https://codereview.chromium.org/1105263004/diff/260001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:226:
request ? request->load_flags() : net::LOAD_NORMAL);
On 2015/05/11 20:35:40, bengr wrote:
> Since looking at the load flags is probably not the way we'll do this in the
> long run, I'd instead make the decision here, and pass the result in. E.g.:
> 
> MaybeAdd...(..., ShouldForceDisableLoFi(request));
> 
> 
> namespace { 
> bool ShouldForceDisableLoFi(const URLRequest* request) {
>  if (!request)
>     return false;
>  return request->load_flags() & LOAD_BYPASS_CACHE;
> }
> }

Done.

https://codereview.chromium.org/1105263004/diff/260001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h
(right):

https://codereview.chromium.org/1105263004/diff/260001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h:173:
void UpdateLoFi(int request_load_flags);
On 2015/05/11 20:35:40, bengr wrote:
> Add a comment that the LoFi header is not added if the request bypasses cache.

Done.

https://codereview.chromium.org/1105263004/diff/260001/content/child/web_url_...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/1105263004/diff/260001/content/child/web_url_...
content/child/web_url_loader_impl.cc:987:
info.headers->HasHeaderValue("Chrome-Proxy", "q=low"));
On 2015/05/11 22:32:58, Charlie Reis wrote:
> It's not clear to me that this belongs in content/.  Looks like all the places
> that set and check it are outside content/.  @jam, is there a way for
embedders
> to add things to a response's extra data?

Acknowledged.

https://codereview.chromium.org/1105263004/diff/260001/content/public/common/...
File content/public/common/context_menu_params.h (right):

https://codereview.chromium.org/1105263004/diff/260001/content/public/common/...
content/public/common/context_menu_params.h:87: // this contains the if the
image was a Lo-Fi image.
On 2015/05/11 22:32:58, Charlie Reis wrote:
> nit: this contains whether the image

Done.

https://codereview.chromium.org/1105263004/diff/260001/content/public/common/...
content/public/common/context_menu_params.h:88: bool image_was_fetched_lo_fi;
On 2015/05/11 22:32:58, Charlie Reis wrote:
> Same question: all the uses seem to be outside content, which suggests that
this
> should not be in the content public interface.
> 
> https://www.chromium.org/developers/content-module/content-api
> "all classes/structs/enums in the public API must be used by embedders and
> content. i.e. if the chrome layer uses a struct but content doesn't know about
> it, it doesn't belong in content/public but instead some module that's higher
> level."

Acknowledged.

[jam, 2015-05-11 22:59:47]

https://codereview.chromium.org/1105263004/diff/260001/content/child/web_url_...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/1105263004/diff/260001/content/child/web_url_...
content/child/web_url_loader_impl.cc:987:
info.headers->HasHeaderValue("Chrome-Proxy", "q=low"));
On 2015/05/11 22:32:58, Charlie Reis wrote:
> It's not clear to me that this belongs in content/.  Looks like all the places
> that set and check it are outside content/.  @jam, is there a way for
embedders
> to add things to a response's extra data?

i agree that this shouldn't go in content; seeing "Chrome-Proxy" is a big
warning sign since it's something in components and this string is duplicated
here because content can't depend on that.

can we call the embedder to put a key-value property bag on the extra data? and
then the context menu builder code can send along this property bag to chrome
over ipc.

[bengr, 2015-05-12 17:21:47]

https://codereview.chromium.org/1105263004/diff/280001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
(right):

https://codereview.chromium.org/1105263004/diff/280001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:179:
bool DataReductionProxyRequestOptions::ShouldForceDisableLoFi(
Move this to an anonymous namespace.

https://codereview.chromium.org/1105263004/diff/280001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h
(right):

https://codereview.chromium.org/1105263004/diff/280001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h:175:
bool ShouldForceDisableLoFi(const net::URLRequest* request);
Separate the comment for this method.

https://codereview.chromium.org/1105263004/diff/280001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc
(right):

https://codereview.chromium.org/1105263004/diff/280001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc:328:
std::vector<std::string>(), &expected_header);
#include <string>
#include <vector>

[megjablon, 2015-05-12 20:15:11]

On 2015/05/11 22:59:47, jam wrote:
>
https://codereview.chromium.org/1105263004/diff/260001/content/child/web_url_...
> File content/child/web_url_loader_impl.cc (right):
> 
>
https://codereview.chromium.org/1105263004/diff/260001/content/child/web_url_...
> content/child/web_url_loader_impl.cc:987:
> info.headers->HasHeaderValue("Chrome-Proxy", "q=low"));
> On 2015/05/11 22:32:58, Charlie Reis wrote:
> > It's not clear to me that this belongs in content/.  Looks like all the
places
> > that set and check it are outside content/.  @jam, is there a way for
> embedders
> > to add things to a response's extra data?
> 
> i agree that this shouldn't go in content; seeing "Chrome-Proxy" is a big
> warning sign since it's something in components and this string is duplicated
> here because content can't depend on that.
> 
> can we call the embedder to put a key-value property bag on the extra data?
and
> then the context menu builder code can send along this property bag to chrome
> over ipc.

this sounds good. can you clarify what you mean by a property bag? would this be
a key-value dictionary that extra data gets?

[Lei Zhang, 2015-05-12 21:16:39]

https://codereview.chromium.org/1105263004/diff/260001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1105263004/diff/260001/chrome/app/generated_r...
chrome/app/generated_resources.grd:1016: +            Load image
On 2015/05/11 22:58:40, megjablon wrote:
> On 2015/05/11 20:16:53, Lei Zhang wrote:
> > Is this going to be Android-only? If so, why not just provide the version
that
> > Android uses and omit the other one? Also do the GRD equivalent of #if
> > defined(OS_ANDROID).
> > 
> > Also, shouldn't this be "Load Image" ?
> 
> This CL creates the context menu option "Load image" for both desktop and
> Android. Currently, this feature can be turned on for both desktop and Android
> using a flag, and while we will only be experimenting this on mobile, it is
> helpful to have desktop implementations for manual testing.

Is the long term goal to have this on desktop platforms as well, or is it just
there to make testing easier?

> I'm using the same format as other context menu options, such as "Copy image"
> and "Save image," which only have the first word capitalized.

You already have the non-title case version above. This is the Title Case
version, so it should be Load Image. Otherwise, why bother defining the same
resource twice in this file?

https://codereview.chromium.org/1105263004/diff/260001/chrome/browser/rendere...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/1105263004/diff/260001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:321:
IN_PROC_BROWSER_TEST_F(ContextMenuBrowserTest, DataSaverLoadImage) {
On 2015/05/11 22:58:40, megjablon wrote:
> On 2015/05/11 20:16:53, Lei Zhang wrote:
> > Can this be a unit test instead?
> 
> I looked into writing it as a unit test, but the
> IDC_CONTENT_CONTEXT_OPENIMAGENEWTAB option in AppendImageItems makes this
> complicated because the params in DataReductionProxyChromeSettings do not get
> initialized and settings->CanUseDataReductionProxy(params_.src_url) crashes.

It's probably possible, just a pain to set up all the services that are needed
to exercise the code.

[jam, 2015-05-13 17:44:31]

On 2015/05/12 20:15:11, megjablon wrote:
> On 2015/05/11 22:59:47, jam wrote:
> >
>
https://codereview.chromium.org/1105263004/diff/260001/content/child/web_url_...
> > File content/child/web_url_loader_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1105263004/diff/260001/content/child/web_url_...
> > content/child/web_url_loader_impl.cc:987:
> > info.headers->HasHeaderValue("Chrome-Proxy", "q=low"));
> > On 2015/05/11 22:32:58, Charlie Reis wrote:
> > > It's not clear to me that this belongs in content/.  Looks like all the
> places
> > > that set and check it are outside content/.  @jam, is there a way for
> > embedders
> > > to add things to a response's extra data?
> > 
> > i agree that this shouldn't go in content; seeing "Chrome-Proxy" is a big
> > warning sign since it's something in components and this string is
duplicated
> > here because content can't depend on that.
> > 
> > can we call the embedder to put a key-value property bag on the extra data?
> and
> > then the context menu builder code can send along this property bag to
chrome
> > over ipc.
> 
> this sounds good. can you clarify what you mean by a property bag? would this
be
> a key-value dictionary that extra data gets?

exactly.

looking at this more closely, it might be hard to call out to the embedder here
since we're in content/child and we don't have a ContentChildClient interface
yet. so the easiest thing might be to change the newly added
WebContextMenuData::imageResponseExtraData to instead be
WebContextMenuData::imageResponse. that way, we can call out to
ContentRendererClient in ContextMenuParamsBuilder::Build with the
WebURLResponse* and a key-value dictionary. chrome can then inspect the headers
and look for Chrome-Proxy and based on that add keys that the chrome browser
side has access to when displaying the context menu UI.

[megjablon, 2015-05-13 23:34:04]

Patchset #3 (id:290001) has been deleted

[megjablon, 2015-05-13 23:36:38]

https://codereview.chromium.org/1105263004/diff/260001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1105263004/diff/260001/chrome/app/generated_r...
chrome/app/generated_resources.grd:1016: +            Load image
On 2015/05/12 21:16:39, Lei Zhang wrote:
> On 2015/05/11 22:58:40, megjablon wrote:
> > On 2015/05/11 20:16:53, Lei Zhang wrote:
> > > Is this going to be Android-only? If so, why not just provide the version
> that
> > > Android uses and omit the other one? Also do the GRD equivalent of #if
> > > defined(OS_ANDROID).
> > > 
> > > Also, shouldn't this be "Load Image" ?
> > 
> > This CL creates the context menu option "Load image" for both desktop and
> > Android. Currently, this feature can be turned on for both desktop and
Android
> > using a flag, and while we will only be experimenting this on mobile, it is
> > helpful to have desktop implementations for manual testing.
> 
> Is the long term goal to have this on desktop platforms as well, or is it just
> there to make testing easier?
> 
> > I'm using the same format as other context menu options, such as "Copy
image"
> > and "Save image," which only have the first word capitalized.
> 
> You already have the non-title case version above. This is the Title Case
> version, so it should be Load Image. Otherwise, why bother defining the same
> resource twice in this file?

There aren't any plans for it now, but we allow LoFi mode to be enabled on
desktop by the command line flag. We have been using desktop to reverse tether
to local versions of the proxy. Also, there currently isn't a way to have a
browser test such as this for android.

Oh sorry, I missed that. My bad. Fixed.

https://codereview.chromium.org/1105263004/diff/260001/chrome/browser/rendere...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/1105263004/diff/260001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:321:
IN_PROC_BROWSER_TEST_F(ContextMenuBrowserTest, DataSaverLoadImage) {
On 2015/05/12 21:16:39, Lei Zhang wrote:
> On 2015/05/11 22:58:40, megjablon wrote:
> > On 2015/05/11 20:16:53, Lei Zhang wrote:
> > > Can this be a unit test instead?
> > 
> > I looked into writing it as a unit test, but the
> > IDC_CONTENT_CONTEXT_OPENIMAGENEWTAB option in AppendImageItems makes this
> > complicated because the params in DataReductionProxyChromeSettings do not
get
> > initialized and settings->CanUseDataReductionProxy(params_.src_url) crashes.
> 
> It's probably possible, just a pain to set up all the services that are needed
> to exercise the code.

Just found out someone is already doing this work in the context menu unit tests
:) once that is landed i'll sync and change this to a unit test

https://codereview.chromium.org/1105263004/diff/280001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
(right):

https://codereview.chromium.org/1105263004/diff/280001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:179:
bool DataReductionProxyRequestOptions::ShouldForceDisableLoFi(
On 2015/05/12 17:21:46, bengr wrote:
> Move this to an anonymous namespace.

Done.

https://codereview.chromium.org/1105263004/diff/280001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h
(right):

https://codereview.chromium.org/1105263004/diff/280001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h:175:
bool ShouldForceDisableLoFi(const net::URLRequest* request);
On 2015/05/12 17:21:46, bengr wrote:
> Separate the comment for this method.

Done.

https://codereview.chromium.org/1105263004/diff/280001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc
(right):

https://codereview.chromium.org/1105263004/diff/280001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc:328:
std::vector<std::string>(), &expected_header);
On 2015/05/12 17:21:46, bengr wrote:
> #include <string>
> #include <vector>

Done.

[Lei Zhang, 2015-05-13 23:50:20]

As I mentioned before, you should find someone to review the Android bits.
You'll also need a security reviewer for the new IPC message, but I expect that
to not be contested, so it can probably wait until the end.

https://codereview.chromium.org/1105263004/diff/260001/chrome/browser/rendere...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/1105263004/diff/260001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:321:
IN_PROC_BROWSER_TEST_F(ContextMenuBrowserTest, DataSaverLoadImage) {
On 2015/05/13 23:36:38, megjablon wrote:
> Just found out someone is already doing this work in the context menu unit
tests
> :) once that is landed i'll sync and change this to a unit test

Yay! Can you add a TODO to convert ContextMenuBrowserTest.DataSaverLoadImage to
a unit test?

https://codereview.chromium.org/1105263004/diff/310001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1105263004/diff/310001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:703: const char
lo_fi_header_key[] = "Chrome-Proxy";
nit: static const...

https://codereview.chromium.org/1105263004/diff/310001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:705:
std::map<std::string, std::string>::iterator it =
const_iterator

https://codereview.chromium.org/1105263004/diff/310001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1105263004/diff/310001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1653: const char
lo_fi_header_key[] = "Chrome-Proxy";
Aren't the same values also seen in RenderViewContextMenu::AppendImageItems()
and DataReductionProxyRequestOptions::SetHeader() ?

[megjablon, 2015-05-14 00:22:49]

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

[megjablon, 2015-05-14 00:22:50]

Adding tedchoc for chrome/android and chrome/browser/android

[Ted C, 2015-05-14 00:38:00]

the android side of this seems ok, but the !android bits look like the more
interesting part of the change.

https://codereview.chromium.org/1105263004/diff/310001/chrome/android/java/re...
File chrome/android/java/res/menu/chrome_context_menu.xml (right):

https://codereview.chromium.org/1105263004/diff/310001/chrome/android/java/re...
chrome/android/java/res/menu/chrome_context_menu.xml:31: <item
android:id="@+id/contextmenu_reload_original_image"
shouldn't this just be "load original image"?  It was never loaded in the first
place, so the re- prefix is a bit odd to me.

This has ripple effects through out the entire change.

https://codereview.chromium.org/1105263004/diff/310001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/1105263004/diff/310001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:2553: if
(mNativeTabAndroid != 0) {
Is this method called anywhere else?

Why not just inline the native call in the method above?

Public methods should have javadoc, and this doesn't seem necessary anywhere.

https://codereview.chromium.org/1105263004/diff/310001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1105263004/diff/310001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:1177: <message
name="IDS_CONTEXTMENU_RELOAD_ORIGINAL_IMAGE" desc="Context sensitive menu item
for reloading the original version of the selected image. [CHAR-LIMIT=30]">
Should we expand on this to further describe that the previous thing was a
placeholder image and this will load the original content?

Just trying to help translators get more context than we usually do.

[bengr, 2015-05-14 18:30:51]

https://codereview.chromium.org/1105263004/diff/310001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
(right):

https://codereview.chromium.org/1105263004/diff/310001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:37:

Remove blank line.

https://codereview.chromium.org/1105263004/diff/310001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:38:
namespace {
Add a blank line after.

https://codereview.chromium.org/1105263004/diff/310001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:48:
}  //namespace
Add a blank line before, and a space after //.

[megjablon, 2015-05-14 19:26:26]

https://codereview.chromium.org/1105263004/diff/260001/chrome/browser/rendere...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/1105263004/diff/260001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:321:
IN_PROC_BROWSER_TEST_F(ContextMenuBrowserTest, DataSaverLoadImage) {
On 2015/05/13 23:50:19, Lei Zhang wrote:
> On 2015/05/13 23:36:38, megjablon wrote:
> > Just found out someone is already doing this work in the context menu unit
> tests
> > :) once that is landed i'll sync and change this to a unit test
> 
> Yay! Can you add a TODO to convert ContextMenuBrowserTest.DataSaverLoadImage
to
> a unit test?

Done.

https://codereview.chromium.org/1105263004/diff/310001/chrome/android/java/re...
File chrome/android/java/res/menu/chrome_context_menu.xml (right):

https://codereview.chromium.org/1105263004/diff/310001/chrome/android/java/re...
chrome/android/java/res/menu/chrome_context_menu.xml:31: <item
android:id="@+id/contextmenu_reload_original_image"
On 2015/05/14 00:37:59, Ted C wrote:
> shouldn't this just be "load original image"?  It was never loaded in the
first
> place, so the re- prefix is a bit odd to me.
> 
> This has ripple effects through out the entire change.

There was an original request for the image which got the placeholder, and then
we're resending the request for the image, just making sure to not have the
placeholder header, then reloading the image into the page. Just noticed the
latest UI mocks actually have "Show image". Switching that.

https://codereview.chromium.org/1105263004/diff/310001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/1105263004/diff/310001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:2553: if
(mNativeTabAndroid != 0) {
On 2015/05/14 00:37:59, Ted C wrote:
> Is this method called anywhere else?
> 
> Why not just inline the native call in the method above?
> 
> Public methods should have javadoc, and this doesn't seem necessary anywhere.

Done.

https://codereview.chromium.org/1105263004/diff/310001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1105263004/diff/310001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:1177: <message
name="IDS_CONTEXTMENU_RELOAD_ORIGINAL_IMAGE" desc="Context sensitive menu item
for reloading the original version of the selected image. [CHAR-LIMIT=30]">
On 2015/05/14 00:38:00, Ted C wrote:
> Should we expand on this to further describe that the previous thing was a
> placeholder image and this will load the original content?
> 
> Just trying to help translators get more context than we usually do.

Done.

https://codereview.chromium.org/1105263004/diff/310001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1105263004/diff/310001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:703: const char
lo_fi_header_key[] = "Chrome-Proxy";
On 2015/05/13 23:50:19, Lei Zhang wrote:
> nit: static const...

Done.

https://codereview.chromium.org/1105263004/diff/310001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:705:
std::map<std::string, std::string>::iterator it =
On 2015/05/13 23:50:19, Lei Zhang wrote:
> const_iterator

Done.

https://codereview.chromium.org/1105263004/diff/310001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1105263004/diff/310001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1653: const char
lo_fi_header_key[] = "Chrome-Proxy";
On 2015/05/13 23:50:19, Lei Zhang wrote:
> Aren't the same values also seen in RenderViewContextMenu::AppendImageItems()
> and DataReductionProxyRequestOptions::SetHeader() ?

They're used little different in DRPRequestOptions.
@bengr can you suggest a good place to put these?

https://codereview.chromium.org/1105263004/diff/310001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
(right):

https://codereview.chromium.org/1105263004/diff/310001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:37:

On 2015/05/14 18:30:51, bengr wrote:
> Remove blank line.

Done.

https://codereview.chromium.org/1105263004/diff/310001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:38:
namespace {
On 2015/05/14 18:30:51, bengr wrote:
> Add a blank line after.

Done.

https://codereview.chromium.org/1105263004/diff/310001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:48:
}  //namespace
On 2015/05/14 18:30:51, bengr wrote:
> Add a blank line before, and a space after //.

Done.

[megjablon, 2015-05-14 19:29:27]

megjablon@chromium.org changed reviewers:
+ asvitkine@chromium.org, wfh@chromium.org

[megjablon, 2015-05-14 19:29:29]

asvitkine: histograms.xml
wfh: render_messages.h and frame_messages.h

[Alexei Svitkine (slow), 2015-05-14 20:42:41]

histograms.xml lgtm, did not look at the rest

[Will Harris, 2015-05-14 20:57:01]

IPC message changes look fine from a security perspective.

lgtm on content/common/frame_messages.h and chrome/common/render_messages.h

[jam, 2015-05-15 06:52:19]

lgtm with comments

https://codereview.chromium.org/1105263004/diff/330001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1105263004/diff/330001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:703: static
const char lo_fi_header_key[] = "Chrome-Proxy";
ditto

https://codereview.chromium.org/1105263004/diff/330001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:704: static
const char lo_fi_header_value[] = "q=low";
this should also be a constant shared with the renderer

https://codereview.chromium.org/1105263004/diff/330001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1105263004/diff/330001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1653: const char
lo_fi_header_key[] = "Chrome-Proxy";
this should really be a constant in components/data_reduction_proxy that is used
here

https://codereview.chromium.org/1105263004/diff/330001/content/renderer/conte...
File content/renderer/context_menu_params_builder.cc (right):

https://codereview.chromium.org/1105263004/diff/330001/content/renderer/conte...
content/renderer/context_menu_params_builder.cc:10: #include
"content/public/common/content_client.h"
nit: by convention we don't include content_client.h since
content_renderer_client.h always includes it (to avoid every place that uses it
to have to include two headers)

[megjablon, 2015-05-15 20:27:34]

Patchset #6 (id:370001) has been deleted

[megjablon, 2015-05-15 20:27:39]

Patchset #6 (id:370001) has been deleted

[megjablon, 2015-05-15 20:31:08]

https://codereview.chromium.org/1105263004/diff/330001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1105263004/diff/330001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:703: static
const char lo_fi_header_key[] = "Chrome-Proxy";
On 2015/05/15 06:52:19, jam wrote:
> ditto

Done.

https://codereview.chromium.org/1105263004/diff/330001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:704: static
const char lo_fi_header_value[] = "q=low";
On 2015/05/15 06:52:19, jam wrote:
> this should also be a constant shared with the renderer

Done.

https://codereview.chromium.org/1105263004/diff/330001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1105263004/diff/330001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1653: const char
lo_fi_header_key[] = "Chrome-Proxy";
On 2015/05/15 06:52:19, jam wrote:
> this should really be a constant in components/data_reduction_proxy that is
used
> here

Done.

https://codereview.chromium.org/1105263004/diff/330001/content/renderer/conte...
File content/renderer/context_menu_params_builder.cc (right):

https://codereview.chromium.org/1105263004/diff/330001/content/renderer/conte...
content/renderer/context_menu_params_builder.cc:10: #include
"content/public/common/content_client.h"
On 2015/05/15 06:52:19, jam wrote:
> nit: by convention we don't include content_client.h since
> content_renderer_client.h always includes it (to avoid every place that uses
it
> to have to include two headers)

Done.

[megjablon, 2015-05-15 20:31:16]

Patchset #6 (id:390001) has been deleted

[Lei Zhang, 2015-05-15 20:36:15]

non-android chrome/ bits lgtm

[Ted C, 2015-05-15 21:01:07]

lgtm -- android

still don't like the naming, but won't keep pushing it if you feel this is
correct

https://codereview.chromium.org/1105263004/diff/410001/chrome/android/java/re...
File chrome/android/java/res/menu/chrome_context_menu.xml (right):

https://codereview.chromium.org/1105263004/diff/410001/chrome/android/java/re...
chrome/android/java/res/menu/chrome_context_menu.xml:23: <item
android:id="@+id/contextmenu_reload_original_image"
Same comment as last time.  Why is it still reload instead of
show_original_image?

This again applies throughout the entire change.  It seems odd to have the
string be one thing and they method names all be different.

If this is just the way it needs to be, then so be it, but it seems odd to me.

https://codereview.chromium.org/1105263004/diff/410001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/1105263004/diff/410001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:345: public void
onReloadOriginalImage() {
same here based on previous comment

https://codereview.chromium.org/1105263004/diff/410001/chrome/browser/ui/andr...
File chrome/browser/ui/android/context_menu_helper.cc (right):

https://codereview.chromium.org/1105263004/diff/410001/chrome/browser/ui/andr...
chrome/browser/ui/android/context_menu_helper.cc:78: static const char
lo_fi_header_key[] = "Chrome-Proxy";
should this not be using the constant values from the component?  (it uses
lowercase chrome-proxy)

[bengr, 2015-05-15 21:23:22]

lgtm, with a couple of minor comments.

https://codereview.chromium.org/1105263004/diff/410001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc
(right):

https://codereview.chromium.org/1105263004/diff/410001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc:150:
request_ = context->CreateRequest(GURL(), net::DEFAULT_PRIORITY, nullptr);
#include gurl.h

https://codereview.chromium.org/1105263004/diff/410001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc:167:
request_ ? request_.get() : nullptr,
Why can't this just be request_?

https://codereview.chromium.org/1105263004/diff/410001/components/data_reduct...
File components/data_reduction_proxy/core/common/data_reduction_proxy_headers.cc
(right):

https://codereview.chromium.org/1105263004/diff/410001/components/data_reduct...
components/data_reduction_proxy/core/common/data_reduction_proxy_headers.cc:51:
const char kChromeProxyHeader[] = "chrome-proxy";
I would expose methods to get these string constants, but not the constants
themselves, and would put these in the anonymous namespace above.

[megjablon, 2015-05-15 22:25:28]

Changed any methods or variables named ReloadOriginalImage to ShowOriginalImage.

https://codereview.chromium.org/1105263004/diff/410001/chrome/android/java/re...
File chrome/android/java/res/menu/chrome_context_menu.xml (right):

https://codereview.chromium.org/1105263004/diff/410001/chrome/android/java/re...
chrome/android/java/res/menu/chrome_context_menu.xml:23: <item
android:id="@+id/contextmenu_reload_original_image"
On 2015/05/15 21:01:07, Ted C wrote:
> Same comment as last time.  Why is it still reload instead of
> show_original_image?
> 
> This again applies throughout the entire change.  It seems odd to have the
> string be one thing and they method names all be different.
> 
> If this is just the way it needs to be, then so be it, but it seems odd to me.

Agreed. Just didn't want to cause confusing since this calls force reload on an
image in blink. Switching naming up to the IPC call so that the code closer to
the UI is the same.

https://codereview.chromium.org/1105263004/diff/410001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/1105263004/diff/410001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:345: public void
onReloadOriginalImage() {
On 2015/05/15 21:01:07, Ted C wrote:
> same here based on previous comment

Done.

https://codereview.chromium.org/1105263004/diff/410001/chrome/browser/ui/andr...
File chrome/browser/ui/android/context_menu_helper.cc (right):

https://codereview.chromium.org/1105263004/diff/410001/chrome/browser/ui/andr...
chrome/browser/ui/android/context_menu_helper.cc:78: static const char
lo_fi_header_key[] = "Chrome-Proxy";
On 2015/05/15 21:01:07, Ted C wrote:
> should this not be using the constant values from the component?  (it uses
> lowercase chrome-proxy)

Whoops sorry missed moving over this location too. Done.

https://codereview.chromium.org/1105263004/diff/410001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc
(right):

https://codereview.chromium.org/1105263004/diff/410001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc:150:
request_ = context->CreateRequest(GURL(), net::DEFAULT_PRIORITY, nullptr);
On 2015/05/15 21:23:21, bengr wrote:
> #include gurl.h

Done.

https://codereview.chromium.org/1105263004/diff/410001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc:167:
request_ ? request_.get() : nullptr,
On 2015/05/15 21:23:21, bengr wrote:
> Why can't this just be request_?

needs a pointer go request_.get() should be fine

https://codereview.chromium.org/1105263004/diff/410001/components/data_reduct...
File components/data_reduction_proxy/core/common/data_reduction_proxy_headers.cc
(right):

https://codereview.chromium.org/1105263004/diff/410001/components/data_reduct...
components/data_reduction_proxy/core/common/data_reduction_proxy_headers.cc:51:
const char kChromeProxyHeader[] = "chrome-proxy";
On 2015/05/15 21:23:21, bengr wrote:
> I would expose methods to get these string constants, but not the constants
> themselves, and would put these in the anonymous namespace above.

Done.

[megjablon, 2015-05-15 22:34:48]

Patchset #7 (id:430001) has been deleted

[Charlie Reis, 2015-05-15 22:37:46]

Thanks for the updates!  Just one question below.

https://codereview.chromium.org/1105263004/diff/450001/content/renderer/conte...
File content/renderer/context_menu_params_builder.cc (right):

https://codereview.chromium.org/1105263004/diff/450001/content/renderer/conte...
content/renderer/context_menu_params_builder.cc:49:
GetContentClient()->renderer()->AddContextMenuProperties(
Seems strange that this is only called in the imageResponse case, since the
AddContextMenuProperties name is more general.  Should we change the name, or
should it be called in other cases as well?

[megjablon, 2015-05-15 22:52:22]

https://codereview.chromium.org/1105263004/diff/450001/content/renderer/conte...
File content/renderer/context_menu_params_builder.cc (right):

https://codereview.chromium.org/1105263004/diff/450001/content/renderer/conte...
content/renderer/context_menu_params_builder.cc:49:
GetContentClient()->renderer()->AddContextMenuProperties(
On 2015/05/15 22:37:46, Charlie Reis wrote:
> Seems strange that this is only called in the imageResponse case, since the
> AddContextMenuProperties name is more general.  Should we change the name, or
> should it be called in other cases as well?

Getting a ResourceResponse for more or all cases from blink would be require a
lot of changes. We are able to grab it for our specific image case through the
HTMLImageElement's ImageResource. Until there is a need to have the
ResourceResponse for other cases, I don't think we should add extra
implementation for it. I named AddContextMenuProperties as generally as possible
since in the future it could be used in other locations. Maybe changing it to
AddContextMenuResourceResponseProperties makes better sense?

[Charlie Reis, 2015-05-15 22:59:10]

https://codereview.chromium.org/1105263004/diff/450001/content/renderer/conte...
File content/renderer/context_menu_params_builder.cc (right):

https://codereview.chromium.org/1105263004/diff/450001/content/renderer/conte...
content/renderer/context_menu_params_builder.cc:49:
GetContentClient()->renderer()->AddContextMenuProperties(
On 2015/05/15 22:52:22, megjablon wrote:
> On 2015/05/15 22:37:46, Charlie Reis wrote:
> > Seems strange that this is only called in the imageResponse case, since the
> > AddContextMenuProperties name is more general.  Should we change the name,
or
> > should it be called in other cases as well?
> 
> Getting a ResourceResponse for more or all cases from blink would be require a
> lot of changes. We are able to grab it for our specific image case through the
> HTMLImageElement's ImageResource. Until there is a need to have the
> ResourceResponse for other cases, I don't think we should add extra
> implementation for it.

Yes, that's certainly fine.

> I named AddContextMenuProperties as generally as possible
> since in the future it could be used in other locations.

The name can be changed if it needs to be more general in the future, but we
shouldn't imply that it does more currently.

> Maybe changing it to
> AddContextMenuResourceResponseProperties makes better sense?

That implies that it's for all resource responses, but it's only called for
images.  My concern is that other people will see the name of the method and
think it's more general than it is.

AddImageContextMenuProperties seems the most accurate name to me, unless I'm
missing something.

[megjablon, 2015-05-15 23:14:03]

Patchset #8 (id:470001) has been deleted

[megjablon, 2015-05-15 23:16:05]

https://codereview.chromium.org/1105263004/diff/450001/content/renderer/conte...
File content/renderer/context_menu_params_builder.cc (right):

https://codereview.chromium.org/1105263004/diff/450001/content/renderer/conte...
content/renderer/context_menu_params_builder.cc:49:
GetContentClient()->renderer()->AddContextMenuProperties(
On 2015/05/15 22:59:09, Charlie Reis wrote:
> On 2015/05/15 22:52:22, megjablon wrote:
> > On 2015/05/15 22:37:46, Charlie Reis wrote:
> > > Seems strange that this is only called in the imageResponse case, since
the
> > > AddContextMenuProperties name is more general.  Should we change the name,
> or
> > > should it be called in other cases as well?
> > 
> > Getting a ResourceResponse for more or all cases from blink would be require
a
> > lot of changes. We are able to grab it for our specific image case through
the
> > HTMLImageElement's ImageResource. Until there is a need to have the
> > ResourceResponse for other cases, I don't think we should add extra
> > implementation for it.
> 
> Yes, that's certainly fine.
> 
> > I named AddContextMenuProperties as generally as possible
> > since in the future it could be used in other locations.
> 
> The name can be changed if it needs to be more general in the future, but we
> shouldn't imply that it does more currently.
> 
> > Maybe changing it to
> > AddContextMenuResourceResponseProperties makes better sense?
> 
> That implies that it's for all resource responses, but it's only called for
> images.  My concern is that other people will see the name of the method and
> think it's more general than it is.
> 
> AddImageContextMenuProperties seems the most accurate name to me, unless I'm
> missing something.

SGTM

[Charlie Reis, 2015-05-15 23:21:18]

Thanks!  LGTM with nit.

https://codereview.chromium.org/1105263004/diff/490001/content/public/rendere...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/1105263004/diff/490001/content/public/rendere...
content/public/renderer/content_renderer_client.h:311: // Gives the embedder a
chance to add properties to the context menu.
nit: Mention that this is currently only called when the context menu is for an
image.

[megjablon, 2015-05-15 23:36:54]

https://codereview.chromium.org/1105263004/diff/490001/content/public/rendere...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/1105263004/diff/490001/content/public/rendere...
content/public/renderer/content_renderer_client.h:311: // Gives the embedder a
chance to add properties to the context menu.
On 2015/05/15 23:21:17, Charlie Reis wrote:
> nit: Mention that this is currently only called when the context menu is for
an
> image.

Done.

[megjablon, 2015-05-15 23:50:19]

Patchset #9 (id:240002) has been deleted

[megjablon, 2015-05-15 23:50:41]

Patchset #9 (id:520001) has been deleted

[megjablon, 2015-05-16 00:51:45]

Patchset #10 (id:550001) has been deleted

[megjablon, 2015-05-16 02:00:05]

The CQ bit was checked by megjablon@chromium.org

[megjablon, 2015-05-16 02:00:05]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
asvitkine@chromium.org, wfh@chromium.org, thestig@chromium.org,
tedchoc@chromium.org, bengr@chromium.org, creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1105263004/#ps570001
(title: "try again")

[commit-bot: I haz the power, 2015-05-16 02:01:16]

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

[commit-bot: I haz the power, 2015-05-16 02:07:40]

Committed patchset #10 (id:570001)

[commit-bot: I haz the power, 2015-05-18 11:30:57]

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