Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
470403003:
[AiS] Fix parsing for Answers Images. (Closed)
|
Created:
6 years, 4 months ago by groby-ooo-7-16 Modified:
6 years, 3 months ago Reviewers:
Mark P CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[AiS] Fix parsing for Answers Images.
1) Image parsing missed one nesting level of dictionaries, and so
always failed.
2) Images are returned without a scheme prefix - prepend that.
R=mpearson@chromium.org
BUG=none
Committed: https://crrev.com/bf481022576e3584fca6016b43d5909624b4c492
Cr-Commit-Position: refs/heads/master@{#291828}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Address review comments. #
Total comments: 4
Patch Set 3 : Review fixes, round #2 #Patch Set 4 : Rebase to HEAD #
Messages
Total messages: 18 (0 generated)
PTAL
Mark:PTAL (was originally pkasting, but he's OOO)
Bet you never expected quite so many comments on a simple change! :-P :-) --mark https://codereview.chromium.org/470403003/diff/1/components/omnibox/search_su... File components/omnibox/search_suggestion_parser.cc (right): https://codereview.chromium.org/470403003/diff/1/components/omnibox/search_su... components/omnibox/search_suggestion_parser.cc:494: lines->GetDictionary(line, &value); comment: you and put this at the beginning of the if statement below as if (!lines->GetDictionary(line, &value) || !value ... ditto in other if below https://codereview.chromium.org/470403003/diff/1/components/omnibox/search_su... components/omnibox/search_suggestion_parser.cc:495: if (!value || !value->GetDictionary("il", &imageLine)) consider || !imageLine at the end https://codereview.chromium.org/470403003/diff/1/components/omnibox/search_su... components/omnibox/search_suggestion_parser.cc:501: std::string imageUrl; If you're going to prepend a protocol to this, then it isn't a URL. Consider not naming it as such, or at least point this fact out. Perhaps image_host_and_path? (p.s. why is the format odd? why does it pass url-like-things without a protocol?) https://codereview.chromium.org/470403003/diff/1/components/omnibox/search_su... components/omnibox/search_suggestion_parser.cc:502: imageData->GetString("d", &imageUrl); Please don't ignore the return value. https://codereview.chromium.org/470403003/diff/1/components/omnibox/search_su... components/omnibox/search_suggestion_parser.cc:503: urls->push_back(GURL("https:" + imageUrl)); This probably should be one of the protocol value constants. https://codereview.chromium.org/470403003/diff/1/components/omnibox/search_su... File components/omnibox/search_suggestion_parser.h (right): https://codereview.chromium.org/470403003/diff/1/components/omnibox/search_su... components/omnibox/search_suggestion_parser.h:288: GetAnswersImageURLsWithValidImage); nit: can't you do this with only one line GetAnswersImageURLs? (isn't that what _ALL_PREFIXES gets you?) https://codereview.chromium.org/470403003/diff/1/components/omnibox/search_su... File components/omnibox/search_suggestion_parser_unittest.cc (right): https://codereview.chromium.org/470403003/diff/1/components/omnibox/search_su... components/omnibox/search_suggestion_parser_unittest.cc:21: delete value; Am I misunderstanding the flow here? Isn't |value| normally not going to get deleted because you return early?
PTAL https://codereview.chromium.org/470403003/diff/1/components/omnibox/search_su... File components/omnibox/search_suggestion_parser.cc (right): https://codereview.chromium.org/470403003/diff/1/components/omnibox/search_su... components/omnibox/search_suggestion_parser.cc:494: lines->GetDictionary(line, &value); On 2014/08/15 22:49:16, Mark P wrote: > comment: you and put this at the beginning of the if statement below as > if (!lines->GetDictionary(line, &value) || !value ... > > ditto in other if below I'm actually taking all comments here and rewriting this. The whole thing is not as clear as it could be. https://codereview.chromium.org/470403003/diff/1/components/omnibox/search_su... components/omnibox/search_suggestion_parser.cc:495: if (!value || !value->GetDictionary("il", &imageLine)) On 2014/08/15 22:49:16, Mark P wrote: > consider || !imageLine at the end imageLine is guaranteed non-NULL if GetDictionary succeeds. (Technically, guaranteed to be unchanged, but it's initialized to NULL) That's why I'm not checking the return value of GetDictionary above, either. https://codereview.chromium.org/470403003/diff/1/components/omnibox/search_su... components/omnibox/search_suggestion_parser.cc:501: std::string imageUrl; On 2014/08/15 22:49:16, Mark P wrote: > If you're going to prepend a protocol to this, then it isn't a URL. Consider > not naming it as such, or at least point this fact out. Perhaps > image_host_and_path? > > (p.s. why is the format odd? why does it pass url-like-things without a > protocol?) It's odd because GWS returns paths without scheme, so they work in the https and http page version. (HTML lets you spec URLs without scheme, and the browser is then supposed to use the enclosing page's scheme, IIRC) host_and_path will do. (It's technically authority_and_path, but that only makes sense if you read the RFCs on URIs :) https://codereview.chromium.org/470403003/diff/1/components/omnibox/search_su... components/omnibox/search_suggestion_parser.cc:502: imageData->GetString("d", &imageUrl); On 2014/08/15 22:49:16, Mark P wrote: > Please don't ignore the return value. Done. https://codereview.chromium.org/470403003/diff/1/components/omnibox/search_su... components/omnibox/search_suggestion_parser.cc:503: urls->push_back(GURL("https:" + imageUrl)); On 2014/08/15 22:49:16, Mark P wrote: > This probably should be one of the protocol value constants. Done. https://codereview.chromium.org/470403003/diff/1/components/omnibox/search_su... File components/omnibox/search_suggestion_parser.h (right): https://codereview.chromium.org/470403003/diff/1/components/omnibox/search_su... components/omnibox/search_suggestion_parser.h:288: GetAnswersImageURLsWithValidImage); On 2014/08/15 22:49:16, Mark P wrote: > nit: can't you do this with only one line GetAnswersImageURLs? > (isn't that what _ALL_PREFIXES gets you?) Unfortunately not - it just makes sure MAYBE_ and DISABLED_ prefixes are also considered friends. https://codereview.chromium.org/470403003/diff/1/components/omnibox/search_su... File components/omnibox/search_suggestion_parser_unittest.cc (right): https://codereview.chromium.org/470403003/diff/1/components/omnibox/search_su... components/omnibox/search_suggestion_parser_unittest.cc:21: delete value; On 2014/08/15 22:49:16, Mark P wrote: > Am I misunderstanding the flow here? Isn't |value| normally not going to get > deleted because you return early? It's a bit tricky due to base::Value's behavior. What happens is that the lines above are essentially a typesafe cast. So dict equals value, and thus gets returned via the scoped_ptr (and the scoped_ptr assumes ownership) The following would be clearer, but carries duplicate allocation cost, and it relies on manual typecasting. scoped_ptr<base::Value> value(base::JSONReder::Read(json)); if (value->Type() == TYPE_DICTIONARY) return scoped_ptr<base::DictionaryValue>(value->DeepCopy()); return scoped_ptr<base::DictionaryValue>(new base::DictionaryValue) PassAs doesn't work because it's a Downcast I _suppose_ I could construct the dict like this: if (value->Type() == TYPE_DICTIONARY) return scoped_ptr<base::DictionaryValue>(static_cast<DictionaryValue>(value.release())); For my taste, that relies on too many internals, but lmk if you'd prefer this. .
This looks much nicer. https://codereview.chromium.org/470403003/diff/1/components/omnibox/search_su... File components/omnibox/search_suggestion_parser_unittest.cc (right): https://codereview.chromium.org/470403003/diff/1/components/omnibox/search_su... components/omnibox/search_suggestion_parser_unittest.cc:21: delete value; On 2014/08/22 00:43:58, groby wrote: > On 2014/08/15 22:49:16, Mark P wrote: > > Am I misunderstanding the flow here? Isn't |value| normally not going to get > > deleted because you return early? > > It's a bit tricky due to base::Value's behavior. What happens is that the lines > above are essentially a typesafe cast. So dict equals value, and thus gets > returned via the scoped_ptr (and the scoped_ptr assumes ownership) Thanks for the explanation. > The following would be clearer, but carries duplicate allocation cost, and it > relies on manual typecasting. > > scoped_ptr<base::Value> value(base::JSONReder::Read(json)); > if (value->Type() == TYPE_DICTIONARY) > return scoped_ptr<base::DictionaryValue>(value->DeepCopy()); > return scoped_ptr<base::DictionaryValue>(new base::DictionaryValue) > > PassAs doesn't work because it's a Downcast > > I _suppose_ I could construct the dict like this: > if (value->Type() == TYPE_DICTIONARY) > return > scoped_ptr<base::DictionaryValue>(static_cast<DictionaryValue>(value.release())); > > For my taste, that relies on too many internals, but lmk if you'd prefer this. > . The current code looks good as is. https://codereview.chromium.org/470403003/diff/20001/components/omnibox/searc... File components/omnibox/search_suggestion_parser.cc (right): https://codereview.chromium.org/470403003/diff/20001/components/omnibox/searc... components/omnibox/search_suggestion_parser.cc:498: std::string imageUrl; I thought you said you'd use host_and_path? Regardless, camelcase is wrong. https://codereview.chromium.org/470403003/diff/20001/components/omnibox/searc... components/omnibox/search_suggestion_parser.cc:501: urls->push_back(GURL(std::string(url::kHttpsScheme) + ":" + imageUrl)); Only ":", not "://" (the url::kStandardSchemeSeparator)? Either please fix or explain (in comments) why you can't use the standard scheme separator?
https://codereview.chromium.org/470403003/diff/20001/components/omnibox/searc... File components/omnibox/search_suggestion_parser.cc (right): https://codereview.chromium.org/470403003/diff/20001/components/omnibox/searc... components/omnibox/search_suggestion_parser.cc:498: std::string imageUrl; On 2014/08/22 16:21:08, Mark P wrote: > I thought you said you'd use host_and_path? > > Regardless, camelcase is wrong. Done. Now for real. https://codereview.chromium.org/470403003/diff/20001/components/omnibox/searc... components/omnibox/search_suggestion_parser.cc:501: urls->push_back(GURL(std::string(url::kHttpsScheme) + ":" + imageUrl)); On 2014/08/22 16:21:08, Mark P wrote: > Only ":", not "://" (the url::kStandardSchemeSeparator)? > Either please fix or explain (in comments) why you can't use the standard scheme > separator? Done.
lgtm
The CQ bit was checked by groby@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/groby@chromium.org/470403003/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/45347) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/50532) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/45362) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by groby@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/groby@chromium.org/470403003/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Message was sent while issue was closed.
Committed patchset #4 (80001) as 455c26b332c6a66e38d9b19d9fbd136c93f44e81
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/bf481022576e3584fca6016b43d5909624b4c492 Cr-Commit-Position: refs/heads/master@{#291828} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
