Enabled the "search web for image" context menu item when "Yandex"

Enabled the "search web for image" context menu item when "Yandex"
is selected by user as the default search provider.

Filled the "image_url" and "image_url_post_params" fields
for the Yandex' search engine. Inserted a "Content-Type"
header to the post parameter containing binary image data.

R=pkasting@chromium.org

Committed: https://crrev.com/deb285397112a5ea50b1f7df1e9d2f5f797f4a3c
Cr-Commit-Position: refs/heads/master@{#317310}

[jam, 2015-02-18 21:19:08]

please specify which files you want each reviewer to look at

[Peter Kasting, 2015-02-18 21:55:52]

Why go to such lengths to plumb a thumbnail format argument from the renderer
when we're always going to set it to JPEG?  It seems like all that should be
removed and we should just set "image/jpeg" as the content type directly when
constructing the actual POST data.

https://codereview.chromium.org/939713003/diff/1/components/search_engines/te...
File components/search_engines/template_url.h (right):

https://codereview.chromium.org/939713003/diff/1/components/search_engines/te...
components/search_engines/template_url.h:184: // The format of the raw content
of an image thumbnail, e.g. "jpeg".
Nit: May want to say something about this needing to be a valid MIME type once
we prepend "image/"?

Optionally, just call this |image_thumbnail_mime_type| and make callers give the
whole type instead of prepending "image/" in template_url.cc.  This is probably
less error-prone.

https://codereview.chromium.org/939713003/diff/1/components/search_engines/te...
components/search_engines/template_url.h:352: // Type to store parameter for
POST URLs.
Nit: How about: "Stores a single parameter for a POST."

https://codereview.chromium.org/939713003/diff/1/components/search_engines/te...
components/search_engines/template_url.h:353: struct PostParam {
Nit: Move this struct above the two typedefs and put a blank line between it and
them.

https://codereview.chromium.org/939713003/diff/1/components/search_engines/te...
components/search_engines/template_url.h:354: std::string value_name;
Nit: Why |value_name| instead of just |name|?

https://codereview.chromium.org/939713003/diff/1/components/search_engines/te...
File components/search_engines/template_url_unittest.cc (right):

https://codereview.chromium.org/939713003/diff/1/components/search_engines/te...
components/search_engines/template_url_unittest.cc:161:
EXPECT_EQ("unknown_template", bad_post_params[0].value_name);
Nit: Many of these place you're touching might get shorter (and perhaps more
readable) if you added some helper function like

  ExpectPostParamIs(const PostParam& param,
                    const std::string& name,
                    const std::string& value,
                    const std::string& content_type);

...and did the EXPECT_EQs in there.

[Vitaly Baranov, 2015-02-19 16:02:41]

vitbar@yandex-team.ru changed reviewers:
- jam@chromium.org, jochen@chromium.org

[Vitaly Baranov, 2015-02-19 16:30:09]

On 2015/02/18 21:55:52, Peter Kasting wrote:
> It seems like all that should be
> removed and we should just set "image/jpeg" as the content type directly when
> constructing the actual POST data.
Done.

[Vitaly Baranov, 2015-02-19 17:31:20]

https://codereview.chromium.org/939713003/diff/1/components/search_engines/te...
File components/search_engines/template_url.h (right):

https://codereview.chromium.org/939713003/diff/1/components/search_engines/te...
components/search_engines/template_url.h:184: // The format of the raw content
of an image thumbnail, e.g. "jpeg".
On 2015/02/18 21:55:51, Peter Kasting wrote:
> Nit: May want to say something about this needing to be a valid MIME type once
> we prepend "image/"?

The Yandex.Images service expects a valid image Content-Type at this post
parameter and it seems to be correct to set this type because the format of
binary image data depends on the Chromium internals.

https://codereview.chromium.org/939713003/diff/1/components/search_engines/te...
components/search_engines/template_url.h:352: // Type to store parameter for
POST URLs.
On 2015/02/18 21:55:51, Peter Kasting wrote:
> Nit: How about: "Stores a single parameter for a POST."

Done.

https://codereview.chromium.org/939713003/diff/1/components/search_engines/te...
components/search_engines/template_url.h:353: struct PostParam {
On 2015/02/18 21:55:52, Peter Kasting wrote:
> Nit: Move this struct above the two typedefs and put a blank line between it
and
> them.

Done.

https://codereview.chromium.org/939713003/diff/1/components/search_engines/te...
components/search_engines/template_url.h:354: std::string value_name;
On 2015/02/18 21:55:51, Peter Kasting wrote:
> Nit: Why |value_name| instead of just |name|?

Renamed. And that was because that name is used in the declaration of function
net::AddMultipartValueForUpload:

NET_EXPORT void AddMultipartValueForUpload(const std::string& value_name,
                                           const std::string& value,
                                           const std::string& mime_boundary,
                                           const std::string& content_type,
                                           std::string* post_data);

https://codereview.chromium.org/939713003/diff/1/components/search_engines/te...
File components/search_engines/template_url_unittest.cc (right):

https://codereview.chromium.org/939713003/diff/1/components/search_engines/te...
components/search_engines/template_url_unittest.cc:161:
EXPECT_EQ("unknown_template", bad_post_params[0].value_name);
On 2015/02/18 21:55:52, Peter Kasting wrote:
> Nit: Many of these place you're touching might get shorter (and perhaps more
> readable) if you added some helper function like
> 
>   ExpectPostParamIs(const PostParam& param,
>                     const std::string& name,
>                     const std::string& value,
>                     const std::string& content_type);
> 
> ...and did the EXPECT_EQs in there.

Done.

[Peter Kasting, 2015-02-19 22:06:24]

LGTM

https://codereview.chromium.org/939713003/diff/20001/components/search_engine...
File components/search_engines/template_url_unittest.cc (right):

https://codereview.chromium.org/939713003/diff/20001/components/search_engine...
components/search_engines/template_url_unittest.cc:177: "{UnknownTemplate}",
"");
Since almost all callsites want to pass the empty string as the content type, I
suggest using a default arg for ExpectPostParamIs():

  void ExpectPostParamIs(const TemplateURLRef::PostParam& param,
                         const std::string& name,
                         const std::string& value,
                         const std::string& content_type = std::string());

This will make things a little more readable most places.

https://codereview.chromium.org/939713003/diff/20001/components/search_engine...
components/search_engines/template_url_unittest.cc:250: ExpectPostParamIs(*i,
"empty_param", "", "");
Nit: "" -> std::string() (after you add a default arg this line will be the only
remaining line that has one of these)

[Peter Kasting, 2015-02-19 22:06:49]

Oops, forgot one thing.  You need to bump the version number atop
prepopulated_engines.json as well.

[Vitaly Baranov, 2015-02-20 13:05:43]

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

[Vitaly Baranov, 2015-02-20 13:08:27]

On 2015/02/19 22:06:49, Peter Kasting wrote:
> Oops, forgot one thing.  You need to bump the version number atop
> prepopulated_engines.json as well.

Done.

[Vitaly Baranov, 2015-02-20 13:12:09]

https://codereview.chromium.org/939713003/diff/20001/components/search_engine...
File components/search_engines/template_url_unittest.cc (right):

https://codereview.chromium.org/939713003/diff/20001/components/search_engine...
components/search_engines/template_url_unittest.cc:177: "{UnknownTemplate}",
"");
On 2015/02/19 22:06:24, Peter Kasting wrote:
> Since almost all callsites want to pass the empty string as the content type,
I
> suggest using a default arg for ExpectPostParamIs():
> 
>   void ExpectPostParamIs(const TemplateURLRef::PostParam& param,
>                          const std::string& name,
>                          const std::string& value,
>                          const std::string& content_type = std::string());
> 
> This will make things a little more readable most places.

Done.

https://codereview.chromium.org/939713003/diff/20001/components/search_engine...
components/search_engines/template_url_unittest.cc:250: ExpectPostParamIs(*i,
"empty_param", "", "");
On 2015/02/19 22:06:24, Peter Kasting wrote:
> Nit: "" -> std::string() (after you add a default arg this line will be the
only
> remaining line that has one of these)

Done.

[Vitaly Baranov, 2015-02-20 13:18:45]

The CQ bit was checked by vitbar@yandex-team.ru

[commit-bot: I haz the power, 2015-02-20 13:20:14]

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

[commit-bot: I haz the power, 2015-02-20 14:03:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-20 14:03:36]

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