Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10828401:
Add a getter for search provider logos. (Closed)
|
Created:
8 years, 4 months ago by samarth Modified:
8 years, 3 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd a getter for search provider logos.
Add locations of "Google" logos to the prepopulated data.
BUG=140955
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155130
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed msw's comments. #Patch Set 3 : Rebase and switch to a simple getter. #
Total comments: 26
Patch Set 4 : Fixed nits and added size enum. #
Total comments: 8
Patch Set 5 : Renamed enum and other nits. #
Total comments: 9
Patch Set 6 : Fixed nits. #Patch Set 7 : Rebase. #Patch Set 8 : Rebase. #Patch Set 9 : Rebase and fixed nit. #Patch Set 10 : Fix bad merge. #
Messages
Total messages: 19 (0 generated)
Hey Mike, Dead simple first change to add new fields for search provider logos. I'm not sure if I'm a huge fan of the 100_percent and 200_percent names though. They're nice and explicit but a little too verbose IMO. What do you think of logo_url and logo_2x_url instead? I probably should also add some more tests and make sure that this works well with search engines added via OSDD, but I wanted to get the review started. Thanks, Samarth
> What do you think of logo_url and logo_2x_url instead? "[100|200]_percent" matches src/chrome/app/theme/[default|touch]_[100|200]_percent/, and those names came about from a lot of discussion. That said, we won't necessarily enforce that the images be scaled by a factor of 2, there are plans to add assets for 140/180/etc percent display modes, and we plan to support a range of logo sizes for each dpi mode... so we could reconsider the names, but 100_percent and 200_percent match the status quo and leave the potential for 140_percent, and 400_percent way down the line, etc. (*shrug*). As a general nit for this CL, I would have ordered the logo urls after the favicon url everywhere. http://codereview.chromium.org/10828401/diff/1/chrome/browser/search_engines/... File chrome/browser/search_engines/template_url.h (right): http://codereview.chromium.org/10828401/diff/1/chrome/browser/search_engines/... chrome/browser/search_engines/template_url.h:272: // URLs pointing to regular and high-DPI versions of logos for this search nit: "Optional logo image URLs at 100% and 200% scale. Both may be empty." http://codereview.chromium.org/10828401/diff/1/chrome/browser/search_engines/... File chrome/browser/search_engines/template_url_prepopulate_data.cc (right): http://codereview.chromium.org/10828401/diff/1/chrome/browser/search_engines/... chrome/browser/search_engines/template_url_prepopulate_data.cc:3730: engine->Get("name", &val) && val->GetAsString(&name) && Wow, do this function a favor, nix |val| and just use DictionaryValue::GetString|GetInteger instead of Get+GetAs*.
OK, let's keep the same naming scheme -- it's worth being consistent. Moved around the logo URL fields as you suggested. I double checked the OSDD download flow and it doesn't look like we need to do anything there. If this looks good to you, any suggestions for whom to send this to? OWNERS here are sky, pkasting and stevet. Thanks, Samarth http://codereview.chromium.org/10828401/diff/1/chrome/browser/search_engines/... File chrome/browser/search_engines/template_url.h (right): http://codereview.chromium.org/10828401/diff/1/chrome/browser/search_engines/... chrome/browser/search_engines/template_url.h:272: // URLs pointing to regular and high-DPI versions of logos for this search On 2012/08/21 00:37:17, msw wrote: > nit: "Optional logo image URLs at 100% and 200% scale. Both may be empty." Done. http://codereview.chromium.org/10828401/diff/1/chrome/browser/search_engines/... File chrome/browser/search_engines/template_url_prepopulate_data.cc (right): http://codereview.chromium.org/10828401/diff/1/chrome/browser/search_engines/... chrome/browser/search_engines/template_url_prepopulate_data.cc:3730: engine->Get("name", &val) && val->GetAsString(&name) && On 2012/08/21 00:37:17, msw wrote: > Wow, do this function a favor, nix |val| and just use > DictionaryValue::GetString|GetInteger instead of Get+GetAs*. Done.
LGTM. +Peter for review and OWNERS; please take a look, thanks! Can/should we change Google URLs to https in the same version bump?
You can't simply add fields to TemplateURLData, they'd need to be persisted in the keyword table or these fields will be empty for all providers as soon as they're roundtripped through there. If you expect the logo usage to be restricted to a very small number of providers, we may not want to either modify TemplateURLData or add fields to the proepopulate structs, but rather just define a getter function (like we have to look up the search engine type) that can return the relevant URL(s).
On 2012/08/25 04:33:29, Peter Kasting wrote: > You can't simply add fields to TemplateURLData, they'd need to be persisted in > the keyword table or these fields will be empty for all providers as soon as > they're roundtripped through there. > > If you expect the logo usage to be restricted to a very small number of > providers, we may not want to either modify TemplateURLData or add fields to the > proepopulate structs, but rather just define a getter function (like we have to > look up the search engine type) that can return the relevant URL(s). Ok, switched to a getter; that's definitely simpler than making sure the new fields persist everywhere. I debated between accepting an engine type versus a URL -- went with a URL since that gives the most flexibility in case we decide to do per-locale logos. I'm ambivalent between having multiple functions for each logo size versus returning a single struct or something. This seemed cleanest for now, but perhaps a single function might make more sense if there many more resolutions to deal with. Thanks, Samarth
http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_prepopulate_data.cc (right): http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3486: // Restricted to UI thread because GetEngineType() is so restricted. nit: There doesn't seem to be much value in DCHECKing this at every level of use... I suggest removing these and letting GetEngineType DCHECK as appropriate. http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3495: return GURL(""); nit: remove empty string literal. http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3497: } nit: some compilers may complain about not all paths returning a value, you may as well return GURL() here too. http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3500: // Restricted to UI thread because GetEngineType() is so restricted. ditto: (nit: remove DCHECKs) http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3509: return GURL(""); ditto (nit: remove empty string literal) http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3511: } ditto (nit: return GURL() here) http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_prepopulate_data.h (right): http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.h:64: GURL GetLogoURL(const std::string& url); Make these args const TemplateURL& (or pointers) instead of a url string, then determination via url/id/etc. is encapsulated. http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc (right): http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc:181: EXPECT_EQ("www.google.com", nit: get each logo URL once and test the host/scheme on locals. http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc:195: "http://www.yahoo.com/").is_empty()); nit: this will fail if/when yahoo is added, use a fake/obscure domain.
http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_prepopulate_data.cc (right): http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3287: Nit: Two blank lines above these section dividers http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3301: Nit: Can you add a ////////// nameless divider here? http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3491: switch (GetEngineType(url)) { Nit: For now, use "if (GetEngineType(url) == SEARCH_ENGINE_GOOGLE)" or even "return <conditional> ? GURL(...) : GURL();. We can expand to a switch, or even better an array of logos, if/when we add more providers. http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_prepopulate_data.h (right): http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.h:64: GURL GetLogoURL(const std::string& url); Nit: I suggest one function that takes a second arg about which size to return. I suspect this will map better to how callers use this, and we might change to return a gfx::Image or something someday which can package all the logo sizes.
I looked around for a standard way of specifying the 1x/2x sizes but didn't find anything so I added my own enum. Let me know if there's a more canonical way of doing this. Thanks, Samarth http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_prepopulate_data.cc (right): http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3287: On 2012/08/28 00:04:07, Peter Kasting wrote: > Nit: Two blank lines above these section dividers Done. http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3301: On 2012/08/28 00:04:07, Peter Kasting wrote: > Nit: Can you add a ////////// nameless divider here? Done. http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3486: // Restricted to UI thread because GetEngineType() is so restricted. On 2012/08/27 21:57:26, msw wrote: > nit: There doesn't seem to be much value in DCHECKing this at every level of > use... I suggest removing these and letting GetEngineType DCHECK as appropriate. Done. http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3491: switch (GetEngineType(url)) { On 2012/08/28 00:04:07, Peter Kasting wrote: > Nit: For now, use "if (GetEngineType(url) == SEARCH_ENGINE_GOOGLE)" or even > "return <conditional> ? GURL(...) : GURL();. We can expand to a switch, or even > better an array of logos, if/when we add more providers. Done. http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3495: return GURL(""); On 2012/08/27 21:57:26, msw wrote: > nit: remove empty string literal. Done. http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3497: } On 2012/08/27 21:57:26, msw wrote: > nit: some compilers may complain about not all paths returning a value, you may > as well return GURL() here too. Done. http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3500: // Restricted to UI thread because GetEngineType() is so restricted. On 2012/08/27 21:57:26, msw wrote: > ditto: (nit: remove DCHECKs) Done. http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3509: return GURL(""); On 2012/08/27 21:57:26, msw wrote: > ditto (nit: remove empty string literal) Done. http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3511: } On 2012/08/27 21:57:26, msw wrote: > ditto (nit: return GURL() here) Done. http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_prepopulate_data.h (right): http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.h:64: GURL GetLogoURL(const std::string& url); On 2012/08/28 00:04:07, Peter Kasting wrote: > Nit: I suggest one function that takes a second arg about which size to return. > I suspect this will map better to how callers use this, and we might change to > return a gfx::Image or something someday which can package all the logo sizes. Done. http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.h:64: GURL GetLogoURL(const std::string& url); On 2012/08/27 21:57:26, msw wrote: > Make these args const TemplateURL& (or pointers) instead of a url string, then > determination via url/id/etc. is encapsulated. Done. http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc (right): http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc:181: EXPECT_EQ("www.google.com", On 2012/08/27 21:57:26, msw wrote: > nit: get each logo URL once and test the host/scheme on locals. Done. http://codereview.chromium.org/10828401/diff/10001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc:195: "http://www.yahoo.com/").is_empty()); On 2012/08/27 21:57:26, msw wrote: > nit: this will fail if/when yahoo is added, use a fake/obscure domain. Done.
Some nits, please also update the CL title and description. http://codereview.chromium.org/10828401/diff/15001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_prepopulate_data.cc (right): http://codereview.chromium.org/10828401/diff/15001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3304: //////////////////////////////////////////////////////////////////////////////// nit: slashes shouldn't straddle the anonymous namespace closing brace. http://codereview.chromium.org/10828401/diff/15001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_prepopulate_data.h (right): http://codereview.chromium.org/10828401/diff/15001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.h:64: enum LogoSize { nit: define the enum at line 24, keep the comment with the function. http://codereview.chromium.org/10828401/diff/15001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.h:65: SIZE_100_PERCENT, nit: TemplateURLPrepopulateData::SIZE_100_PERCENT doesn't convey context for the size, consider renaming the enum values LOGO_[1|2]00_PERCENT. http://codereview.chromium.org/10828401/diff/15001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.h:68: GURL GetLogoURL(const TemplateURL& url, LogoSize size); nit: s/url/template_url/ or change the comment's use of "|template_url|".
Address msw's comment and updated patch description. Thanks, Samarth http://codereview.chromium.org/10828401/diff/15001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_prepopulate_data.cc (right): http://codereview.chromium.org/10828401/diff/15001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3304: //////////////////////////////////////////////////////////////////////////////// On 2012/08/28 18:22:00, msw wrote: > nit: slashes shouldn't straddle the anonymous namespace closing brace. Done. http://codereview.chromium.org/10828401/diff/15001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_prepopulate_data.h (right): http://codereview.chromium.org/10828401/diff/15001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.h:64: enum LogoSize { On 2012/08/28 18:22:00, msw wrote: > nit: define the enum at line 24, keep the comment with the function. Done. http://codereview.chromium.org/10828401/diff/15001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.h:65: SIZE_100_PERCENT, On 2012/08/28 18:22:00, msw wrote: > nit: TemplateURLPrepopulateData::SIZE_100_PERCENT doesn't convey context for the > size, consider renaming the enum values LOGO_[1|2]00_PERCENT. Done. http://codereview.chromium.org/10828401/diff/15001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.h:68: GURL GetLogoURL(const TemplateURL& url, LogoSize size); On 2012/08/28 18:22:00, msw wrote: > nit: s/url/template_url/ or change the comment's use of "|template_url|". Done.
LGTM with a hilarious nit :) http://codereview.chromium.org/10828401/diff/19001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_prepopulate_data.cc (right): http://codereview.chromium.org/10828401/diff/19001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3289: // Logo URLs /////////////////////////////////////////////////////////////////// nit: this needs to be about 15 lines of ascii art :)
LGTM http://codereview.chromium.org/10828401/diff/19001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_prepopulate_data.cc (right): http://codereview.chromium.org/10828401/diff/19001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3493: } else if (size == LOGO_200_PERCENT) { Nit: No else after return. Just do this: return GURL((size == LOGO_200_PERCENT) ? google_logos.logo_200_percent_url : google_logos.logo_100_percent_url); http://codereview.chromium.org/10828401/diff/19001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc (right): http://codereview.chromium.org/10828401/diff/19001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc:188: EXPECT_TRUE(logo_url.SchemeIsSecure()); Why is testing for a secure scheme something the unittest should ensure? Seems like this should be omitted. http://codereview.chromium.org/10828401/diff/19001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc:191: TEST(TemplateURLPrepopulateDataTest, GetLogoURLGoogle200Percent) { Nit: Combine these first two tests, a la: ... GURL logo_100 = ...; EXPECT_EQ("www.google.com", logo_100.host()); GURL logo_200 = ...; EXPECT_EQ("www.google.com", logo_200.host()); EXPECT_NE(logo_100, logo_200);
Waiting for trybot results and then I'll submit. Thanks for the reviews! Samarth http://codereview.chromium.org/10828401/diff/19001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_prepopulate_data.cc (right): http://codereview.chromium.org/10828401/diff/19001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3289: // Logo URLs /////////////////////////////////////////////////////////////////// On 2012/08/28 22:26:22, msw wrote: > nit: this needs to be about 15 lines of ascii art :) You mean this? :) # # #### #### #### # # # # # # # # # # # # # # # # # ### # # # # # # # # # ####### #### #### #### # # ###### # # # # # # #### # # # # # # # # ###### # #### # # # # # # # # # # # # # ##### # # ####### #### http://codereview.chromium.org/10828401/diff/19001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3493: } else if (size == LOGO_200_PERCENT) { On 2012/08/28 23:20:20, Peter Kasting wrote: > Nit: No else after return. Just do this: > > return GURL((size == LOGO_200_PERCENT) ? > google_logos.logo_200_percent_url : google_logos.logo_100_percent_url); This style could easily break if we added more sizes and not all logos were available in each size. Right now, every case not covered falls through to the empty GURL below. But I don't feel too strongly if you'd like me to use the ternary. http://codereview.chromium.org/10828401/diff/19001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc (right): http://codereview.chromium.org/10828401/diff/19001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc:188: EXPECT_TRUE(logo_url.SchemeIsSecure()); On 2012/08/28 23:20:20, Peter Kasting wrote: > Why is testing for a secure scheme something the unittest should ensure? Seems > like this should be omitted. Done. http://codereview.chromium.org/10828401/diff/19001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc:191: TEST(TemplateURLPrepopulateDataTest, GetLogoURLGoogle200Percent) { On 2012/08/28 23:20:20, Peter Kasting wrote: > Nit: Combine these first two tests, a la: > > ... > GURL logo_100 = ...; > EXPECT_EQ("www.google.com", logo_100.host()); > > GURL logo_200 = ...; > EXPECT_EQ("www.google.com", logo_200.host()); > > EXPECT_NE(logo_100, logo_200); Done.
http://codereview.chromium.org/10828401/diff/19001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_prepopulate_data.cc (right): http://codereview.chromium.org/10828401/diff/19001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_prepopulate_data.cc:3493: } else if (size == LOGO_200_PERCENT) { On 2012/08/28 23:29:20, samarth wrote: > On 2012/08/28 23:20:20, Peter Kasting wrote: > > Nit: No else after return. Just do this: > > > > return GURL((size == LOGO_200_PERCENT) ? > > google_logos.logo_200_percent_url : > google_logos.logo_100_percent_url); > > This style could easily break if we added more sizes and not all logos were > available in each size. Right now, every case not covered falls through to the > empty GURL below. But I don't feel too strongly if you'd like me to use the > ternary. I think it's nice to be brief since we don't have any immediate plans to add Yet More Sizes. I would also be OK with replacing the "else if" with a DCHECK_EQ if you really want to verify this, but the pattern we normally use in e.g. infobar button handling (where we have a similar two-value enum) is to just use ?: and not worry much. Up to you as long as there's no return after else.
Warning: do not trust the try job results on patchset 6: many are green but haven't been run. Sorry it's an error on my part.
On 2012/08/28 23:32:54, Peter Kasting wrote: > I think it's nice to be brief since we don't have any immediate plans to add Yet > More Sizes. Ok, makes sense, done. Thanks for the review! Samarth
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/samarth@chromium.org/10828401/33003
Change committed as 155130 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
