Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
669573005:
Add a class to parse answer json. (Closed)
|
Created:
6 years, 2 months ago by Justin Donnelly Modified:
6 years, 1 month ago CC:
chromium-reviews, James Su Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd a class to parse answer json.
In subsequent changes, this will be used across platforms to replace all answer json parsing.
BUG=427107
Committed: https://crrev.com/7393cee9845330bbe5e4712f5e16751256e6cb7c
Cr-Commit-Position: refs/heads/master@{#302210}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Respond to comments #Patch Set 3 : Tweaked API and finished unit tests #
Total comments: 32
Patch Set 4 : Respond to comments #Patch Set 5 : Respond to comments #
Total comments: 43
Patch Set 6 : Respond to comments #Patch Set 7 : Sync and respond to comments #
Total comments: 3
Patch Set 8 : Fix broken test and switch to new-style for loop #Patch Set 9 : Switch back to passing pointers to answers #
Total comments: 8
Patch Set 10 : Respond to comments #Patch Set 11 : Fixed some unit test issues #
Total comments: 38
Patch Set 12 : Respond to comments, fix a copy bug #
Total comments: 41
Patch Set 13 : Respond to comments #
Total comments: 2
Patch Set 14 : Respond to comment and fix assignment operator issue #
Total comments: 2
Patch Set 15 : Fix case on "copyright" #Messages
Total messages: 54 (8 generated)
jdonnelly@chromium.org changed reviewers: + groby@chromium.org
I'm not done with the unit tests but I was hoping you could provide feedback on the overall shape of this. In particular, note that the amount and complexity of code is increased a bit by our choice to make the answer object in SuggestResult be nullable. After adding the copy constructor and assignment operator and scoped_ptrs in a bunch of places, I'm a little less convinced it's worth the space savings.
You're right - this results in really ugly code. So, given these constraints: * Parsing is expensive and should happen only once * Parsing should happen immediately when the results is received, so we can extract image URLs there are really two options I can see 1) Use a scoped_refptr to share that data 2) Damn the cost, and keep the parsed result copyable. I'm leaning towards 2) unless we have evidence that it's _that_ costly. We already copy a bunch of strings in AutocompleteMatch all the time, so this is just more of the same. Bleh. https://codereview.chromium.org/669573005/diff/1/components/omnibox/autocompl... File components/omnibox/autocomplete_match.cc (right): https://codereview.chromium.org/669573005/diff/1/components/omnibox/autocompl... components/omnibox/autocomplete_match.cc:83: answer(match.answer.get() ? new SuggestionAnswer(*match.answer) : NULL), UGH. I forgot that AutocompleteMatch can be copy-constructed. That probably means we should refcount it, if we really care about savings. (Which opens another can of worms...) https://codereview.chromium.org/669573005/diff/1/components/omnibox/autocompl... File components/omnibox/autocomplete_match.h (right): https://codereview.chromium.org/669573005/diff/1/components/omnibox/autocompl... components/omnibox/autocomplete_match.h:322: base::string16 answer_contents; Since we make answer optional, I think we should put contents/type into SuggestionAnswer as well. (Also, we can probably remove contents as soon as we can move SuggestionAnswer to the Java side) https://codereview.chromium.org/669573005/diff/1/components/omnibox/base_sear... File components/omnibox/base_search_provider.cc (right): https://codereview.chromium.org/669573005/diff/1/components/omnibox/base_sear... components/omnibox/base_search_provider.cc:435: more_relevant_match.answer.reset(less_relevant_match.answer.get() ? Assuming that we don't care about the answer on the lower-ranking item any more (do we?), this could probably be more_relevant_match.answer = less_relevant_match.answer.Pass(); https://codereview.chromium.org/669573005/diff/1/components/omnibox/suggestio... File components/omnibox/suggestion_answer.h (right): https://codereview.chromium.org/669573005/diff/1/components/omnibox/suggestio... components/omnibox/suggestion_answer.h:34: static bool ParseTextField( Why not have a simple struct, plus a factory? Saves you from writing accessors, comparison, etc. https://codereview.chromium.org/669573005/diff/1/components/omnibox/suggestio... components/omnibox/suggestion_answer.h:50: typedef std::vector<TextField> TextFields; I wonder if TextFields should be a single string + a vector of <type, index, len> 3-tuples instead. At least copy cost is somewhat reduced, if we go the "copy it all" route.
https://codereview.chromium.org/669573005/diff/1/components/omnibox/autocompl... File components/omnibox/autocomplete_match.cc (right): https://codereview.chromium.org/669573005/diff/1/components/omnibox/autocompl... components/omnibox/autocomplete_match.cc:83: answer(match.answer.get() ? new SuggestionAnswer(*match.answer) : NULL), On 2014/10/21 00:35:54, groby wrote: > UGH. I forgot that AutocompleteMatch can be copy-constructed. That probably > means we should refcount it, if we really care about savings. (Which opens > another can of worms...) > Yes, I agree that option #2 is probably the right answer (always having a value for the answer object). It seems undesirable to have the overhead but the reality is we're talking about dozens of suggestion and match objects, not thousands. I've made the change to eliminate the scoped_ptrs and the new copy constructor and assignment operator. The code is much nicer now. Let me know what you think. https://codereview.chromium.org/669573005/diff/1/components/omnibox/autocompl... File components/omnibox/autocomplete_match.h (right): https://codereview.chromium.org/669573005/diff/1/components/omnibox/autocompl... components/omnibox/autocomplete_match.h:322: base::string16 answer_contents; On 2014/10/21 00:35:54, groby wrote: > Since we make answer optional, I think we should put contents/type into > SuggestionAnswer as well. (Also, we can probably remove contents as soon as we > can move SuggestionAnswer to the Java side) Agreed. Once I update Clank to use the new type, I'll remove the contents and type fields. (I already had a TODO in SuggestResult and I've added one here in AutocompleteMatch.) I already put type into answer. It's not clear that we'll actually need a string contents field, but I can add it later if necessary. https://codereview.chromium.org/669573005/diff/1/components/omnibox/base_sear... File components/omnibox/base_search_provider.cc (right): https://codereview.chromium.org/669573005/diff/1/components/omnibox/base_sear... components/omnibox/base_search_provider.cc:435: more_relevant_match.answer.reset(less_relevant_match.answer.get() ? On 2014/10/21 00:35:54, groby wrote: > Assuming that we don't care about the answer on the lower-ranking item any more > (do we?), this could probably be more_relevant_match.answer = > less_relevant_match.answer.Pass(); No longer using the scoped_ptr. https://codereview.chromium.org/669573005/diff/1/components/omnibox/suggestio... File components/omnibox/suggestion_answer.h (right): https://codereview.chromium.org/669573005/diff/1/components/omnibox/suggestio... components/omnibox/suggestion_answer.h:34: static bool ParseTextField( On 2014/10/21 00:35:54, groby wrote: > Why not have a simple struct, plus a factory? Saves you from writing accessors, > comparison, etc. I like the consistency of design across these three classes. Plus, while this is currently very simple, I think it might get more complex (see next comment). https://codereview.chromium.org/669573005/diff/1/components/omnibox/suggestio... components/omnibox/suggestion_answer.h:50: typedef std::vector<TextField> TextFields; On 2014/10/21 00:35:54, groby wrote: > I wonder if TextFields should be a single string + a vector of <type, index, > len> 3-tuples instead. At least copy cost is somewhat reduced, if we go the > "copy it all" route. My concern with this is that we discussed a similar structure for each TextField after parsing HTML tags in the strings. If we also do this, we'll end up with a complex structure to represent the indexes of the various segments.
Finished unit tests and tweaked the API. No rush, but please take another look when you get a chance.
https://codereview.chromium.org/669573005/diff/40001/components/omnibox/searc... File components/omnibox/search_suggestion_parser.cc (right): https://codereview.chromium.org/669573005/diff/40001/components/omnibox/searc... components/omnibox/search_suggestion_parser.cc:471: answer.Clear(); Question: What if ParseAnswer() returned void, and error handling was contained in there? (Yielding either a valid answer, or an empty one) https://codereview.chromium.org/669573005/diff/40001/components/omnibox/searc... components/omnibox/search_suggestion_parser.cc:495: const SuggestionAnswer& answer, I think this function should move onto SuggestionAnswer https://codereview.chromium.org/669573005/diff/40001/components/omnibox/searc... File components/omnibox/search_suggestion_parser.h (right): https://codereview.chromium.org/669573005/diff/40001/components/omnibox/searc... components/omnibox/search_suggestion_parser.h:122: const SuggestionAnswer& answer, Can we roll answer_contents/answer_type into SuggestionAnswer already? Downstream code should extract them via functions anyways, so that should transition safely. https://codereview.chromium.org/669573005/diff/40001/components/omnibox/searc... components/omnibox/search_suggestion_parser.h:178: // clients are using the SuggestionAnswer. Ah. See above. Nobody should use the member vars directly already, since they are private. https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... File components/omnibox/suggestion_answer.cc (right): https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:62: const base::DictionaryValue* line_json, ImageLine* image_line) { Why not just return ImageLine by value? (for all ParseXYZ funcs) Less parameters, less pointers. https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:109: if (!image_line->image_url_.is_valid()) I've been thinking of adding UMA metrics for parse failures. I'd rather find out about malformed replies before crbugs are filed :) Thoughts? https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:113: image_line->is_valid_ = true; Probably want to force that to invalid at the top. Or assert it's passed in invalid https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:119: if (text_fields_.size() != line.text_fields_.size()) std::vector has equality comparison, so you should be able to use that. https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:188: void SuggestionAnswer::SetType(const std::string& type) { Do we really need string & string16 for this? https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... File components/omnibox/suggestion_answer.h (right): https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:25: // according to the specification at http://goto.google.com/ais_api. We probably shouldn't have that URL here. https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:120: // google3/logs/gws/config/visual_element_configuration. Probably also not :) https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... File components/omnibox/suggestion_answer_unittest.cc (right): https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... components/omnibox/suggestion_answer_unittest.cc:93: answer.SetType("-238"); Do we need all the tests? Do they actually cover different branches?
https://codereview.chromium.org/669573005/diff/40001/components/omnibox/searc... File components/omnibox/search_suggestion_parser.cc (right): https://codereview.chromium.org/669573005/diff/40001/components/omnibox/searc... components/omnibox/search_suggestion_parser.cc:471: answer.Clear(); On 2014/10/23 00:46:19, groby wrote: > Question: What if ParseAnswer() returned void, and error handling was contained > in there? (Yielding either a valid answer, or an empty one) I considered that, but this is such a standard C++ pattern I decided against it. But what I'm doing is definitely redundant so I'd be willing to drop the return value if you think it's better. https://codereview.chromium.org/669573005/diff/40001/components/omnibox/searc... components/omnibox/search_suggestion_parser.cc:495: const SuggestionAnswer& answer, On 2014/10/23 00:46:19, groby wrote: > I think this function should move onto SuggestionAnswer Yeah, duh, should've thought of that. Done. https://codereview.chromium.org/669573005/diff/40001/components/omnibox/searc... File components/omnibox/search_suggestion_parser.h (right): https://codereview.chromium.org/669573005/diff/40001/components/omnibox/searc... components/omnibox/search_suggestion_parser.h:122: const SuggestionAnswer& answer, On 2014/10/23 00:46:19, groby wrote: > Can we roll answer_contents/answer_type into SuggestionAnswer already? > Downstream code should extract them via functions anyways, so that should > transition safely. Oh, I see what you're saying now. True, SuggestResult only exposes this value by function but its counterpart AutocompleteMatch doesn't. I'd prefer to keep those two in sync and then just remove the two obsolete properties from both at the same time. https://codereview.chromium.org/669573005/diff/40001/components/omnibox/searc... components/omnibox/search_suggestion_parser.h:178: // clients are using the SuggestionAnswer. On 2014/10/23 00:46:19, groby wrote: > Ah. See above. Nobody should use the member vars directly already, since they > are private. See previous response. https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... File components/omnibox/suggestion_answer.cc (right): https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:62: const base::DictionaryValue* line_json, ImageLine* image_line) { On 2014/10/23 00:46:19, groby wrote: > Why not just return ImageLine by value? (for all ParseXYZ funcs) > > Less parameters, less pointers. Fair question. Again, just because of the ubiquity of this pattern. Same question applies to your GetAnswersImageURLs, right? That could (and maybe should) just return a vector of strings by value (we know it's only 0, 1, or 2 URLs). Again, I'm definitely open to what you're suggesting. Maybe we should discuss offline. https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:109: if (!image_line->image_url_.is_valid()) On 2014/10/23 00:46:19, groby wrote: > I've been thinking of adding UMA metrics for parse failures. I'd rather find out > about malformed replies before crbugs are filed :) > > Thoughts? Given how you mentioned that Finch team is about to roll out alerts on UMA metrics, this is a great idea! But you mind if I add it in a subsequent CL? I'd like to keep the size of this one under control. https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:113: image_line->is_valid_ = true; On 2014/10/23 00:46:19, groby wrote: > Probably want to force that to invalid at the top. Or assert it's passed in > invalid Done. https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:119: if (text_fields_.size() != line.text_fields_.size()) On 2014/10/23 00:46:19, groby wrote: > std::vector has equality comparison, so you should be able to use that. Done. https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:188: void SuggestionAnswer::SetType(const std::string& type) { On 2014/10/23 00:46:19, groby wrote: > Do we really need string & string16 for this? Done. https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... File components/omnibox/suggestion_answer.h (right): https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:25: // according to the specification at http://goto.google.com/ais_api. On 2014/10/23 00:46:20, groby wrote: > We probably shouldn't have that URL here. Doesn't seem like this is a thing that's enforced: https://code.google.com/p/chromium/codesearch#search/&q=http://go/&sq=package... Wouldn't it better to make the info easily accessible to Google developers than to leave it out just to avoid a possibly annoying broken link for non-Google developers? I'm genuinely asking and am open to persuasion here. https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:120: // google3/logs/gws/config/visual_element_configuration. On 2014/10/23 00:46:19, groby wrote: > Probably also not :) Ditto https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... File components/omnibox/suggestion_answer_unittest.cc (right): https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... components/omnibox/suggestion_answer_unittest.cc:93: answer.SetType("-238"); On 2014/10/23 00:46:20, groby wrote: > Do we need all the tests? Do they actually cover different branches? You could definitely argue this is overkill but I like to cover a range of input conditions, especially when it's simple and easy like here. If the only negative test was -1, for example, then an implementation of "!= -1" would pass even though the intent of the implementation is to be "< 0".
https://codereview.chromium.org/669573005/diff/40001/components/omnibox/searc... File components/omnibox/search_suggestion_parser.cc (right): https://codereview.chromium.org/669573005/diff/40001/components/omnibox/searc... components/omnibox/search_suggestion_parser.cc:471: answer.Clear(); On 2014/10/23 17:59:37, Justin Donnelly wrote: > On 2014/10/23 00:46:19, groby wrote: > > Question: What if ParseAnswer() returned void, and error handling was > contained > > in there? (Yielding either a valid answer, or an empty one) > > I considered that, but this is such a standard C++ pattern I decided against it. > But what I'm doing is definitely redundant so I'd be willing to drop the return > value if you think it's better. Since the SuggestionAnswer object already needs to carry an is_valid function, I think dropping the return value (and return by value) are a better choice. https://codereview.chromium.org/669573005/diff/40001/components/omnibox/searc... File components/omnibox/search_suggestion_parser.h (right): https://codereview.chromium.org/669573005/diff/40001/components/omnibox/searc... components/omnibox/search_suggestion_parser.h:122: const SuggestionAnswer& answer, On 2014/10/23 17:59:37, Justin Donnelly wrote: > On 2014/10/23 00:46:19, groby wrote: > > Can we roll answer_contents/answer_type into SuggestionAnswer already? > > Downstream code should extract them via functions anyways, so that should > > transition safely. > > Oh, I see what you're saying now. True, SuggestResult only exposes this value > by function but its counterpart AutocompleteMatch doesn't. I'd prefer to keep > those two in sync and then just remove the two obsolete properties from both at > the same time. Works for me https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... File components/omnibox/suggestion_answer.cc (right): https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:62: const base::DictionaryValue* line_json, ImageLine* image_line) { On 2014/10/23 17:59:37, Justin Donnelly wrote: > On 2014/10/23 00:46:19, groby wrote: > > Why not just return ImageLine by value? (for all ParseXYZ funcs) > > > > Less parameters, less pointers. > > Fair question. Again, just because of the ubiquity of this pattern. Same > question applies to your GetAnswersImageURLs, right? That could (and maybe > should) just return a vector of strings by value (we know it's only 0, 1, or 2 > URLs). Again, I'm definitely open to what you're suggesting. Maybe we should > discuss offline. It's ubiquitous and ugly :) You're right, GetAnswersImageURLs could probably do the same thing. The reason I specifically picked on this function is that it returns an object that can be both valid or invalid, _and_ a status that expresses the same thing. https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... File components/omnibox/suggestion_answer.h (right): https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:25: // according to the specification at http://goto.google.com/ais_api. On 2014/10/23 17:59:37, Justin Donnelly wrote: > On 2014/10/23 00:46:20, groby wrote: > > We probably shouldn't have that URL here. > > Doesn't seem like this is a thing that's enforced: > > https://code.google.com/p/chromium/codesearch#search/&q=http://go/&sq=package... > > Wouldn't it better to make the info easily accessible to Google developers than > to leave it out just to avoid a possibly annoying broken link for non-Google > developers? I'm genuinely asking and am open to persuasion here. You're right, it's not enforced. I'm probably overly paranoid. https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:120: // google3/logs/gws/config/visual_element_configuration. On 2014/10/23 17:59:37, Justin Donnelly wrote: > On 2014/10/23 00:46:19, groby wrote: > > Probably also not :) > > Ditto Usually only referring to base or third_party. I'd really rather not... If you want a link, make it a go one?
https://codereview.chromium.org/669573005/diff/40001/components/omnibox/searc... File components/omnibox/search_suggestion_parser.cc (right): https://codereview.chromium.org/669573005/diff/40001/components/omnibox/searc... components/omnibox/search_suggestion_parser.cc:471: answer.Clear(); On 2014/10/23 21:59:54, groby wrote: > On 2014/10/23 17:59:37, Justin Donnelly wrote: > > On 2014/10/23 00:46:19, groby wrote: > > > Question: What if ParseAnswer() returned void, and error handling was > > contained > > > in there? (Yielding either a valid answer, or an empty one) > > > > I considered that, but this is such a standard C++ pattern I decided against > it. > > But what I'm doing is definitely redundant so I'd be willing to drop the > return > > value if you think it's better. > > Since the SuggestionAnswer object already needs to carry an is_valid function, I > think dropping the return value (and return by value) are a better choice. Done. https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... File components/omnibox/suggestion_answer.cc (right): https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:62: const base::DictionaryValue* line_json, ImageLine* image_line) { On 2014/10/23 21:59:54, groby wrote: > On 2014/10/23 17:59:37, Justin Donnelly wrote: > > On 2014/10/23 00:46:19, groby wrote: > > > Why not just return ImageLine by value? (for all ParseXYZ funcs) > > > > > > Less parameters, less pointers. > > > > Fair question. Again, just because of the ubiquity of this pattern. Same > > question applies to your GetAnswersImageURLs, right? That could (and maybe > > should) just return a vector of strings by value (we know it's only 0, 1, or 2 > > URLs). Again, I'm definitely open to what you're suggesting. Maybe we should > > discuss offline. > > It's ubiquitous and ugly :) You're right, GetAnswersImageURLs could probably do > the same thing. > > The reason I specifically picked on this function is that it returns an object > that can be both valid or invalid, _and_ a status that expresses the same thing. > I've left this as-is for now since they "two different things that express the same thing" problem is gone. Let me know if you still want to consider returning by value. https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... File components/omnibox/suggestion_answer.h (right): https://codereview.chromium.org/669573005/diff/40001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:120: // google3/logs/gws/config/visual_element_configuration. On 2014/10/23 21:59:54, groby wrote: > On 2014/10/23 17:59:37, Justin Donnelly wrote: > > On 2014/10/23 00:46:19, groby wrote: > > > Probably also not :) > > > > Ditto > > Usually only referring to base or third_party. I'd really rather not... If you > want a link, make it a go one? Switched to a go link. We should talk more offline if you're still concerned about it. My feeling is that omitting a useful link like this won't make the fact that these codes are publicly undocumented go away, it'll just make it harder for anyone to figure out where they come from.
jdonnelly@chromium.org changed reviewers: + pkasting@chromium.org
jdonnelly@chromium.org changed required reviewers: + groby@chromium.org, pkasting@chromium.org
Hi Peter - can you take a look at this Answers in Suggest JSON parsing code when you get a chance? It's intended to replace the separate JSON parsing currently residing in C++, Obj C (Bling) and Java (Clank).
https://codereview.chromium.org/669573005/diff/80001/components/omnibox/autoc... File components/omnibox/autocomplete_match.h (right): https://codereview.chromium.org/669573005/diff/80001/components/omnibox/autoc... components/omnibox/autocomplete_match.h:322: // clients are using the SuggestionAnswer. Does this just mean chrome/ code? Is it not possible to change this all at once, or were just trying to keep the change smaller and more reviewable? (Which I'm fine with) https://codereview.chromium.org/669573005/diff/80001/components/omnibox/searc... File components/omnibox/search_suggestion_parser.cc (right): https://codereview.chromium.org/669573005/diff/80001/components/omnibox/searc... components/omnibox/search_suggestion_parser.cc:474: answer.Clear(); Nit: Perhaps it would be nicer for ParseAnswer() to return a bool, and when it returns false, it guarantees the provided answer hasn't been touched. That would basically eliminate the need for this else arm. https://codereview.chromium.org/669573005/diff/80001/components/omnibox/searc... components/omnibox/search_suggestion_parser.cc:475: DLOG(ERROR) << "Invalid suggestion answer json: " Nit: I'm never a fan of logging statements unless you have an explicit plan for how to collect the data from them. https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... File components/omnibox/suggestion_answer.cc (right): https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:22: static const char* kAnswerJsonImageData = "i.d"; Nit: I tend to interpret the style guide's "Place a function's variables in the narrowest scope possible" as also applying to constants that are local to one function or block (as all but one of these are). https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:32: void SuggestionAnswer::TextField::ParseTextField( This function returns void and the caller checks validity afterwards. It seems like the API might be cleaner if this function checked validity internally, returned a bool status, and guaranteed not to change its argument's contents in the case of an invalid result. (That last might be optional.) Similar comments apply through the rest of this file. https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:111: image_line->image_url_ = GURL(base::ASCIIToUTF16("https:") + url_string); Nit: Maybe we should use kHttpsScheme from url/url_constants.h. You might also want to add to the comment above noting why "//" in this case is always properly expanded to "https" (instead of having to go fetch some context or something to decide). https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:113: image_line->image_url_ = GURL(url_string); Nit: Shorter: image_line->image_url_ = GURL( StartsWith(url_string, base::ASCIIToUTF16("//"), false) ? (base::ASCIIToUTF16("https:") + url_string) : url_string); https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... File components/omnibox/suggestion_answer.h (right): https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:37: bool operator==(const TextField& field) const; Per the style guide, avoid defining == and != for these classes unless you really need to. Instead use a function like Equals(). (3 places) https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:51: }; I assume you can't use DISALLOW_COPY_AND_ASSIGN here since you want these in a vector. Consider instead having a comment that notes that we allow the compiler to auto-generate copy/assignment operators and why. https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:72: const std::vector<TextField>& text_fields() const { return text_fields_; } Nit: Use TextFields https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:74: bool has_additional_text() const { return additional_text_.is_valid(); } Nit: I'm a bit on the fence as to whether these has_xxx() methods are really useful, given that it's almost as short for callers to just call xxx().is_valid() themselves, and these first two at least are only used in unittests. I tend to like shrinking public APIs as much as possible. That said, I don't feel strongly, so do whatever you think is most maintainable in the end. https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:92: }; Nit: DISALLOW_COPY_AND_ASSIGN (2 places) https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:106: // An answer is valid only if both a type has been set and the JSON Nit: Eliminate "both", add commas (since list seems to really have three items) https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:121: void SetType(const base::string16& type); All but one caller of this has a number to begin with. It would be simpler to push string parsing into the lone client of this that needs to do it and make this take a number, so this API wasn't as easy to misuse (e.g. by code that accidentally passes the wrong string field). https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:128: void GetImageURLs(std::vector<GURL>* urls) const; Nit: I suggest making a typedef for this https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... File components/omnibox/suggestion_answer_unittest.cc (right): https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer_unittest.cc:24: EXPECT_EQ(answer2, answer2copy1); Nit: Instead of answer2 + answer2copy1 and answer2copy2, why not just answer2[copy] and answer3[copy]? https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer_unittest.cc:37: TEST(SuggestionAnswerTest, DifferentValusAreUnequal) { Nit: Values https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer_unittest.cc:82: std::string json = ""; Nit: = "" not necessary
https://codereview.chromium.org/669573005/diff/80001/components/omnibox/autoc... File components/omnibox/autocomplete_match.h (right): https://codereview.chromium.org/669573005/diff/80001/components/omnibox/autoc... components/omnibox/autocomplete_match.h:322: // clients are using the SuggestionAnswer. On 2014/10/24 22:38:09, Peter Kasting wrote: > Does this just mean chrome/ code? Is it not possible to change this all at > once, or were just trying to keep the change smaller and more reviewable? > (Which I'm fine with) Yeah, I'd probably do it just to keep the change size manageable but the driving reason is that Clank (and maybe Bling?) rolls the main repository to their build periodically, so the changes have to be staged. https://codereview.chromium.org/669573005/diff/80001/components/omnibox/searc... File components/omnibox/search_suggestion_parser.cc (right): https://codereview.chromium.org/669573005/diff/80001/components/omnibox/searc... components/omnibox/search_suggestion_parser.cc:474: answer.Clear(); On 2014/10/24 22:38:09, Peter Kasting wrote: > Nit: Perhaps it would be nicer for ParseAnswer() to return a bool, and when it > returns false, it guarantees the provided answer hasn't been touched. That > would basically eliminate the need for this else arm. Done. https://codereview.chromium.org/669573005/diff/80001/components/omnibox/searc... components/omnibox/search_suggestion_parser.cc:475: DLOG(ERROR) << "Invalid suggestion answer json: " On 2014/10/24 22:38:09, Peter Kasting wrote: > Nit: I'm never a fan of logging statements unless you have an explicit plan for > how to collect the data from them. Done. (In another change, I plan to add a UMA stat for answer parsing failures and setup one of the soon-to-be-available Finch alerts.) https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... File components/omnibox/suggestion_answer.cc (right): https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:22: static const char* kAnswerJsonImageData = "i.d"; On 2014/10/24 22:38:10, Peter Kasting wrote: > Nit: I tend to interpret the style guide's "Place a function's variables in the > narrowest scope possible" as also applying to constants that are local to one > function or block (as all but one of these are). Makes sense, but these are a bit of a special case. I wouldn't normally use constants at all if their value was only used in one place (and this is not likely to ever change). The purpose of these is to put the format details together so that if they're updated or added to it's easy to locate and compare to the spec doc. I wouldn't strongly object to just eliminating the constants if you think they don't make sense here. https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:32: void SuggestionAnswer::TextField::ParseTextField( On 2014/10/24 22:38:10, Peter Kasting wrote: > This function returns void and the caller checks validity afterwards. > > It seems like the API might be cleaner if this function checked validity > internally, returned a bool status, and guaranteed not to change its argument's > contents in the case of an invalid result. (That last might be optional.) > > Similar comments apply through the rest of this file. The issue here is that the is_valid() functions are necessary since clients pass the answers by value. I originally had bool return values from the Parse functions but Rachel objected (understandably) to having both a returned bool and is_valid logic that did the same thing. Thoughts? https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:111: image_line->image_url_ = GURL(base::ASCIIToUTF16("https:") + url_string); On 2014/10/24 22:38:10, Peter Kasting wrote: > Nit: Maybe we should use kHttpsScheme from url/url_constants.h. > > You might also want to add to the comment above noting why "//" in this case is > always properly expanded to "https" (instead of having to go fetch some context > or something to decide). Done. https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:113: image_line->image_url_ = GURL(url_string); On 2014/10/24 22:38:10, Peter Kasting wrote: > Nit: Shorter: > > image_line->image_url_ = GURL( > StartsWith(url_string, base::ASCIIToUTF16("//"), false) ? > (base::ASCIIToUTF16("https:") + url_string) : url_string); Done. https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... File components/omnibox/suggestion_answer.h (right): https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:37: bool operator==(const TextField& field) const; On 2014/10/24 22:38:11, Peter Kasting wrote: > Per the style guide, avoid defining == and != for these classes unless you > really need to. Instead use a function like Equals(). (3 places) Done. https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:51: }; On 2014/10/24 22:38:10, Peter Kasting wrote: > I assume you can't use DISALLOW_COPY_AND_ASSIGN here since you want these in a > vector. Consider instead having a comment that notes that we allow the compiler > to auto-generate copy/assignment operators and why. It's because copies of answers are made in AutocompleteMatch's copy and assignment operators and some logic in BaseSearchProvider. Added a comment below in SuggestionAnswer. Let me know if you think it's worth repeating in each of the inner classes. https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:72: const std::vector<TextField>& text_fields() const { return text_fields_; } On 2014/10/24 22:38:10, Peter Kasting wrote: > Nit: Use TextFields Done. https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:74: bool has_additional_text() const { return additional_text_.is_valid(); } On 2014/10/24 22:38:11, Peter Kasting wrote: > Nit: I'm a bit on the fence as to whether these has_xxx() methods are really > useful, given that it's almost as short for callers to just call > xxx().is_valid() themselves, and these first two at least are only used in > unittests. I tend to like shrinking public APIs as much as possible. That > said, I don't feel strongly, so do whatever you think is most maintainable in > the end. Fair question, but there's a Java API with Has methods that will also map onto this so I think it makes sense. https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:92: }; On 2014/10/24 22:38:11, Peter Kasting wrote: > Nit: DISALLOW_COPY_AND_ASSIGN (2 places) See above comment re: answer copying. https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:106: // An answer is valid only if both a type has been set and the JSON On 2014/10/24 22:38:10, Peter Kasting wrote: > Nit: Eliminate "both", add commas (since list seems to really have three items) Done. https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:121: void SetType(const base::string16& type); On 2014/10/24 22:38:10, Peter Kasting wrote: > All but one caller of this has a number to begin with. It would be simpler to > push string parsing into the lone client of this that needs to do it and make > this take a number, so this API wasn't as easy to misuse (e.g. by code that > accidentally passes the wrong string field). Done. https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:128: void GetImageURLs(std::vector<GURL>* urls) const; On 2014/10/24 22:38:11, Peter Kasting wrote: > Nit: I suggest making a typedef for this Seems to me that in the client classes (SearchSuggestionParser and BaseSearchProvider) it would be no terser and slightly less clear to refer to SuggestionAnswer::GURLs rather than vector<GURL>. But if we don't use it outside this class then it's just used in one place here. Let me know if that's what you had in mind or if you had some other motivation here. https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... File components/omnibox/suggestion_answer_unittest.cc (right): https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer_unittest.cc:24: EXPECT_EQ(answer2, answer2copy1); On 2014/10/24 22:38:11, Peter Kasting wrote: > Nit: Instead of answer2 + answer2copy1 and answer2copy2, why not just > answer2[copy] and answer3[copy]? Done. https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer_unittest.cc:37: TEST(SuggestionAnswerTest, DifferentValusAreUnequal) { On 2014/10/24 22:38:11, Peter Kasting wrote: > Nit: Values Done. https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer_unittest.cc:82: std::string json = ""; On 2014/10/24 22:38:11, Peter Kasting wrote: > Nit: = "" not necessary Done.
https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... File components/omnibox/suggestion_answer.cc (right): https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:22: static const char* kAnswerJsonImageData = "i.d"; On 2014/10/27 21:41:15, Justin Donnelly wrote: > On 2014/10/24 22:38:10, Peter Kasting wrote: > > Nit: I tend to interpret the style guide's "Place a function's variables in > the > > narrowest scope possible" as also applying to constants that are local to one > > function or block (as all but one of these are). > > Makes sense, but these are a bit of a special case. I wouldn't normally use > constants at all if their value was only used in one place (and this is not > likely to ever change). The purpose of these is to put the format details > together so that if they're updated or added to it's easy to locate and compare > to the spec doc. > > I wouldn't strongly object to just eliminating the constants if you think they > don't make sense here. I don't care much. If you think there's a win in placing them here like you describe, I would suggest writing a comment above these noting the rationale you detail here. https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:32: void SuggestionAnswer::TextField::ParseTextField( On 2014/10/27 21:41:15, Justin Donnelly wrote: > On 2014/10/24 22:38:10, Peter Kasting wrote: > > This function returns void and the caller checks validity afterwards. > > > > It seems like the API might be cleaner if this function checked validity > > internally, returned a bool status, and guaranteed not to change its > argument's > > contents in the case of an invalid result. (That last might be optional.) > > > > Similar comments apply through the rest of this file. > > The issue here is that the is_valid() functions are necessary since clients pass > the answers by value. Would passing by pointer muck things up too much? > I originally had bool return values from the Parse > functions but Rachel objected (understandably) to having both a returned bool > and is_valid logic that did the same thing. Thoughts? I probably prefer to have both if it actually makes the callers simpler or more readable. If it's not actually any better, then I wouldn't bother. https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... File components/omnibox/suggestion_answer.h (right): https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:128: void GetImageURLs(std::vector<GURL>* urls) const; On 2014/10/27 21:41:16, Justin Donnelly wrote: > On 2014/10/24 22:38:11, Peter Kasting wrote: > > Nit: I suggest making a typedef for this > > Seems to me that in the client classes (SearchSuggestionParser and > BaseSearchProvider) it would be no terser and slightly less clear to refer to > SuggestionAnswer::GURLs rather than vector<GURL>. But if we don't use it > outside this class then it's just used in one place here. Let me know if that's > what you had in mind or if you had some other motivation here. Terseness isn't the goal; the goal is to insulate the caller from the actual type in use. Basically this is a way to purposefully black-box the type so the caller doesn't know or care whether it's a vector, list, or whatever. Also I have an allergy to angle brackets so I'll basically always take any solution other than container<type>. :) It's not really a big deal in the end. Do what you think is best.
https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... File components/omnibox/suggestion_answer.cc (right): https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:22: static const char* kAnswerJsonImageData = "i.d"; On 2014/10/27 21:50:09, Peter Kasting wrote: > On 2014/10/27 21:41:15, Justin Donnelly wrote: > > On 2014/10/24 22:38:10, Peter Kasting wrote: > > > Nit: I tend to interpret the style guide's "Place a function's variables in > > the > > > narrowest scope possible" as also applying to constants that are local to > one > > > function or block (as all but one of these are). > > > > Makes sense, but these are a bit of a special case. I wouldn't normally use > > constants at all if their value was only used in one place (and this is not > > likely to ever change). The purpose of these is to put the format details > > together so that if they're updated or added to it's easy to locate and > compare > > to the spec doc. > > > > I wouldn't strongly object to just eliminating the constants if you think they > > don't make sense here. > > I don't care much. If you think there's a win in placing them here like you > describe, I would suggest writing a comment above these noting the rationale you > detail here. Done. https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:32: void SuggestionAnswer::TextField::ParseTextField( On 2014/10/27 21:50:09, Peter Kasting wrote: > On 2014/10/27 21:41:15, Justin Donnelly wrote: > > On 2014/10/24 22:38:10, Peter Kasting wrote: > > > This function returns void and the caller checks validity afterwards. > > > > > > It seems like the API might be cleaner if this function checked validity > > > internally, returned a bool status, and guaranteed not to change its > > argument's > > > contents in the case of an invalid result. (That last might be optional.) > > > > > > Similar comments apply through the rest of this file. > > > > The issue here is that the is_valid() functions are necessary since clients > pass > > the answers by value. > > Would passing by pointer muck things up too much? Yeah, it creates a lot of slightly more complicated code. You can actually see what it looks like in patchset 1 of this CL. A bunch of new scoped_ptrs are introduced, plus SuggestResult has to get a custom copy constructor and assignment operator. > > > I originally had bool return values from the Parse > > functions but Rachel objected (understandably) to having both a returned bool > > and is_valid logic that did the same thing. Thoughts? > > I probably prefer to have both if it actually makes the callers simpler or more > readable. If it's not actually any better, then I wouldn't bother. It makes the caller code slightly simpler, but it's a bit of a matter of taste. Given Rachel's slightly (I think) stronger preference that yours, I'll leave it as-is for now unless there's anything she wants to add. https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... File components/omnibox/suggestion_answer.h (right): https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.h:128: void GetImageURLs(std::vector<GURL>* urls) const; On 2014/10/27 21:50:09, Peter Kasting wrote: > On 2014/10/27 21:41:16, Justin Donnelly wrote: > > On 2014/10/24 22:38:11, Peter Kasting wrote: > > > Nit: I suggest making a typedef for this > > > > Seems to me that in the client classes (SearchSuggestionParser and > > BaseSearchProvider) it would be no terser and slightly less clear to refer to > > SuggestionAnswer::GURLs rather than vector<GURL>. But if we don't use it > > outside this class then it's just used in one place here. Let me know if > that's > > what you had in mind or if you had some other motivation here. > > Terseness isn't the goal; the goal is to insulate the caller from the actual > type in use. Basically this is a way to purposefully black-box the type so the > caller doesn't know or care whether it's a vector, list, or whatever. > > Also I have an allergy to angle brackets so I'll basically always take any > solution other than container<type>. :) > > It's not really a big deal in the end. Do what you think is best. Interesting. I didn't actually know that that was the rationale. Given how prevalent the pattern of typedef-ing containers is, I guess it'd be better for consistency alone. Done.
Tiny nit, otherwise LGTM (Feel free to do the nit later if pkasting also LGs. No need to waste try runs :) https://codereview.chromium.org/669573005/diff/120001/components/omnibox/base... File components/omnibox/base_search_provider.cc (right): https://codereview.chromium.org/669573005/diff/120001/components/omnibox/base... components/omnibox/base_search_provider.cc:450: for (SuggestionAnswer::URLs::const_iterator it = Since we're touching this anyways, let's go C++11 for(const GURL& url : results->answer_image_urls) client_->PrefetchImage(url); https://codereview.chromium.org/669573005/diff/120001/components/omnibox/sear... File components/omnibox/search_suggestion_parser.h (left): https://codereview.chromium.org/669573005/diff/120001/components/omnibox/sear... components/omnibox/search_suggestion_parser.h:295: FRIEND_TEST_ALL_PREFIXES(SearchSuggestionParser, Yay for less friends!
> Tiny nit, otherwise LGTM (Feel free to do the nit later if > pkasting also LGs. No need to waste try runs :) I had to fix a broken test anyway. https://codereview.chromium.org/669573005/diff/120001/components/omnibox/base... File components/omnibox/base_search_provider.cc (right): https://codereview.chromium.org/669573005/diff/120001/components/omnibox/base... components/omnibox/base_search_provider.cc:450: for (SuggestionAnswer::URLs::const_iterator it = On 2014/10/28 00:50:02, groby wrote: > Since we're touching this anyways, let's go C++11 > > for(const GURL& url : results->answer_image_urls) > client_->PrefetchImage(url); Done. (I can't properly express how much I love range-based for loops.)
Hi Peter - did you want to take another look at this before I submit?
On 2014/10/28 18:48:45, Justin Donnelly wrote: > Hi Peter - did you want to take another look at this before I submit? I do want to look at this. I'll try to get to it tomorrow (my hours have been very sporadic due to a leg fracture).
On 2014/10/29 04:13:32, Peter Kasting wrote: > On 2014/10/28 18:48:45, Justin Donnelly wrote: > > Hi Peter - did you want to take another look at this before I submit? > > I do want to look at this. I'll try to get to it tomorrow (my hours have been > very sporadic due to a leg fracture). Oh, wow, really sorry to hear you're hurt. I'll wait on your next round of comments. One thing to note is that the iOS implementation is blocked on this but otherwise ready to go (https://chromereviews.googleplex.com/100857013/). I don't say this to try to create artificial pressure (ok maybe a little) but just to ask that if you have any big changes you'd like to see, that you consider whether they could be done in a follow-on CL.
https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... File components/omnibox/suggestion_answer.cc (right): https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... components/omnibox/suggestion_answer.cc:32: void SuggestionAnswer::TextField::ParseTextField( On 2014/10/28 00:39:34, Justin Donnelly wrote: > On 2014/10/27 21:50:09, Peter Kasting wrote: > > On 2014/10/27 21:41:15, Justin Donnelly wrote: > > > On 2014/10/24 22:38:10, Peter Kasting wrote: > > > > This function returns void and the caller checks validity afterwards. > > > > > > > > It seems like the API might be cleaner if this function checked validity > > > > internally, returned a bool status, and guaranteed not to change its > > > argument's > > > > contents in the case of an invalid result. (That last might be optional.) > > > > > > > > Similar comments apply through the rest of this file. > > > > > > The issue here is that the is_valid() functions are necessary since clients > > pass > > > the answers by value. > > > > Would passing by pointer muck things up too much? > > Yeah, it creates a lot of slightly more complicated code. You can actually see > what it looks like in patchset 1 of this CL. A bunch of new scoped_ptrs are > introduced, plus SuggestResult has to get a custom copy constructor and > assignment operator. > > > > > I originally had bool return values from the Parse > > > functions but Rachel objected (understandably) to having both a returned > bool > > > and is_valid logic that did the same thing. Thoughts? > > > > I probably prefer to have both if it actually makes the callers simpler or > more > > readable. If it's not actually any better, then I wouldn't bother. > > It makes the caller code slightly simpler, but it's a bit of a matter of taste. > Given Rachel's slightly (I think) stronger preference that yours, I'll leave it > as-is for now unless there's anything she wants to add. OK, I read through patch set 1 and looked at the delta between it and patch set 2. I actually think the original design of having this field be optional is more sensible. The code delta is pretty minimal, mainly just the new copy/assignment operators you defined. There are also multiple places that do something faintly ugly like: answer.reset(other_answer ? new SuggestionAnswer(other_answer) : nullptr); However, these issues can be worked around, and if we leave this field optional, we can get rid of the validity functions entirely, and then we have no excuse not to also clean up the parsing functions here to either return a possibly-null pointer for failure or to return a bool. I think the net gain from that cleanup outweighs the net increased complexity of making the answer optional, especially since in both cases we're making the code read more like how one should actually think of it working (i.e. that answers are optional and parsing can fail, instead of hiding the idea of a non-existent answer in a validity-check function). There are a couple ways to make the copying code a little nicer. One might be to do something like this: class SuggestionAnswer { static scoped_ptr<SuggestionAnswer> Copy(SuggestionAnswer* source) { return make_scoped_ptr(source ? new SuggestionAnswer(source) : nullptr); } }; ... answer = SuggestionAnswer::Copy(other_answer); Another way would be to do something similar, except make Copy() take both answers as args ("SuggestionAnswer::Copy(other_answer, &answer);"). Another way would be to create a wrapper class like OptionalAnswer that's simply a scoped_ptr<SuggestionAnswer> plus copy constructor, assignment operator, and a few other utility functions; this lets you write something like answer = other_answer; ...and avoids the need to define custom copy/assignment code for SuggestResult, but one might argue that at that point we've lost the simplicity and readability that I argued above made this path worth taking to begin with, and that the end code isn't different from just putting the "optional"-ness inside SuggestionAnswer. I realize the idea of "no, go back and change everything back again" is going to be pretty unpalatable. What do you think of my arguments above? Have I missed good reasons to prefer one side over the other?
On 2014/10/29 17:56:17, Peter Kasting wrote: > https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... > File components/omnibox/suggestion_answer.cc (right): > > https://codereview.chromium.org/669573005/diff/80001/components/omnibox/sugge... > components/omnibox/suggestion_answer.cc:32: void > SuggestionAnswer::TextField::ParseTextField( > On 2014/10/28 00:39:34, Justin Donnelly wrote: > > On 2014/10/27 21:50:09, Peter Kasting wrote: > > > On 2014/10/27 21:41:15, Justin Donnelly wrote: > > > > On 2014/10/24 22:38:10, Peter Kasting wrote: > > > > > This function returns void and the caller checks validity afterwards. > > > > > > > > > > It seems like the API might be cleaner if this function checked validity > > > > > internally, returned a bool status, and guaranteed not to change its > > > > argument's > > > > > contents in the case of an invalid result. (That last might be > optional.) > > > > > > > > > > Similar comments apply through the rest of this file. > > > > > > > > The issue here is that the is_valid() functions are necessary since > clients > > > pass > > > > the answers by value. > > > > > > Would passing by pointer muck things up too much? > > > > Yeah, it creates a lot of slightly more complicated code. You can actually > see > > what it looks like in patchset 1 of this CL. A bunch of new scoped_ptrs are > > introduced, plus SuggestResult has to get a custom copy constructor and > > assignment operator. > > > > > > > I originally had bool return values from the Parse > > > > functions but Rachel objected (understandably) to having both a returned > > bool > > > > and is_valid logic that did the same thing. Thoughts? > > > > > > I probably prefer to have both if it actually makes the callers simpler or > > more > > > readable. If it's not actually any better, then I wouldn't bother. > > > > It makes the caller code slightly simpler, but it's a bit of a matter of > taste. > > Given Rachel's slightly (I think) stronger preference that yours, I'll leave > it > > as-is for now unless there's anything she wants to add. > > OK, I read through patch set 1 and looked at the delta between it and patch set > 2. > > I actually think the original design of having this field be optional is more > sensible. The code delta is pretty minimal, mainly just the new copy/assignment > operators you defined. There are also multiple places that do something faintly > ugly like: > > answer.reset(other_answer ? new SuggestionAnswer(other_answer) : nullptr); > > However, these issues can be worked around, and if we leave this field optional, > we can get rid of the validity functions entirely, and then we have no excuse > not to also clean up the parsing functions here to either return a possibly-null > pointer for failure or to return a bool. I think the net gain from that cleanup > outweighs the net increased complexity of making the answer optional, especially > since in both cases we're making the code read more like how one should actually > think of it working (i.e. that answers are optional and parsing can fail, > instead of hiding the idea of a non-existent answer in a validity-check > function). > > There are a couple ways to make the copying code a little nicer. One might be > to do something like this: > > class SuggestionAnswer { > static scoped_ptr<SuggestionAnswer> Copy(SuggestionAnswer* source) { > return make_scoped_ptr(source ? new SuggestionAnswer(source) : nullptr); > } > }; > > ... > answer = SuggestionAnswer::Copy(other_answer); > > Another way would be to do something similar, except make Copy() take both > answers as args ("SuggestionAnswer::Copy(other_answer, &answer);"). Another way > would be to create a wrapper class like OptionalAnswer that's simply a > scoped_ptr<SuggestionAnswer> plus copy constructor, assignment operator, and a > few other utility functions; this lets you write something like > > answer = other_answer; > > ...and avoids the need to define custom copy/assignment code for SuggestResult, > but one might argue that at that point we've lost the simplicity and readability > that I argued above made this path worth taking to begin with, and that the end > code isn't different from just putting the "optional"-ness inside > SuggestionAnswer. > > I realize the idea of "no, go back and change everything back again" is going to > be pretty unpalatable. What do you think of my arguments above? Have I missed > good reasons to prefer one side over the other? It's not that big a change, so I don't especially mind switching it around again if we can all agree on a solution. I prefer the simplicity of passing by value but I take your point about a pointer feeling like the more natural representation of an optional value. In the end, I don't have a strong preference, but I'd like to hear Rachel's thoughts so we can all get on the same page. If we switch back, the Copy function is a nice solution. I'd prefer the first version. (I would indeed argue that OptionalAnswer, while clever, is too complex ).
I do like the idea of an optional answer better than "isValid", because it represents reality slightly better. (Most results simply don't have an answer) Encapsulating it in an OptionalAnswer would be my preferred solution - I'm not sure where that loses simplicity. In fact, I'd argue it increases it, because the consumer of the class shouldn't have to care what the particular copy semantics are. (If you call it an AnswerValuePtr, and it's even obvious at declaration that this still is a pointer) But SuggestionAnswer::Copy is not atrocious enough to me to object, so I'll be fine either way. Sidebar: What we technically want is discussed in N3339[1] as value_ptr. Given the pace of C++ standardization, we probably shouldn't wait :) [1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3339.pdf
On 2014/10/29 18:55:19, groby wrote: > Sidebar: What we technically want is discussed in N3339[1] as value_ptr. Given > the pace of C++ standardization, we probably shouldn't wait :) > > [1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3339.pdf Ah, that's a cool idea. I wonder if we should actually add a value_ptr-type class to base. Do you guys think it's worth proposing such a thing? I feel like other places in Chrome must want to use this concept, but I'm not sure how to find them.
To clarify why I'm changing my mind again - the pointer solution looked _really_ ugly. Peter's suggestion lifts it to the same readability as the pass-by-value approach, and it matches the underlying concepts better.
I think value_ptr could be useful in other places, but we probably should have concrete evidence of it before we propose it :) Also note that value_ptr has open questions, namely array pointers and questions around the deleter. I'd _like_ us to implement it, but I worry how well it'll hold up. (Or how much effort it will take to get right)
On 2014/10/29 19:05:21, groby wrote: > I think value_ptr could be useful in other places, but we probably should have > concrete evidence of it before we propose it :) > > Also note that value_ptr has open questions, namely array pointers and questions > around the deleter. I'd _like_ us to implement it, but I worry how well it'll > hold up. (Or how much effort it will take to get right) I started a thread about this.
Ok, let me know what you guys think of this version.
I like this overall, but it bugs me that we have unnecessary copies when creating a SuggestResult via ctor. Question: What if SuggestResult took a scoped_ptr<> in the ctor? Not a reference, but a actually taking ownership of the object passed in? I don't see an immediate downside to it, except for the unit tests that now need to make copies - but make_scoped_ptr(new SuggestAnswer(*answer)) is not exactly horrible. Thoughts? https://codereview.chromium.org/669573005/diff/160001/components/omnibox/sear... File components/omnibox/search_provider.cc (right): https://codereview.chromium.org/669573005/diff/160001/components/omnibox/sear... components/omnibox/search_provider.cc:848: answer = SuggestionAnswer::Copy(it->answer.get()); I think this would be better off with a naked ptr, since SuggestResult makes a copy anyways. (It happens rarely enough that I don't mind the extra copy) Actually - see overall comment. https://codereview.chromium.org/669573005/diff/160001/components/omnibox/sear... File components/omnibox/search_suggestion_parser.cc (right): https://codereview.chromium.org/669573005/diff/160001/components/omnibox/sear... components/omnibox/search_suggestion_parser.cc:112: const SuggestResult& result) { Idiomatic name for operator='s parameter is rhs. ("Right hand side"). I'd prefer that. https://codereview.chromium.org/669573005/diff/160001/components/omnibox/sear... components/omnibox/search_suggestion_parser.cc:116: Result::operator=(result); Gah. It's valid C++, but horrible to read. static_cast<Result>(*this) = rhs; or at least this->Result::operator=(rhs) https://codereview.chromium.org/669573005/diff/160001/components/omnibox/sear... components/omnibox/search_suggestion_parser.cc:517: answer.get(), suggest_query_params, deletion_url, is_keyword_result, It bugs me that this is yet another unnecessary copy...
https://codereview.chromium.org/669573005/diff/160001/components/omnibox/sear... File components/omnibox/search_provider.cc (right): https://codereview.chromium.org/669573005/diff/160001/components/omnibox/sear... components/omnibox/search_provider.cc:848: answer = SuggestionAnswer::Copy(it->answer.get()); On 2014/10/29 23:51:16, groby wrote: > I think this would be better off with a naked ptr, since SuggestResult makes a > copy anyways. (It happens rarely enough that I don't mind the extra copy) > > Actually - see overall comment. Passing scoped_ptr to the constructor is fine, changed that. So this is no longer a double copy. https://codereview.chromium.org/669573005/diff/160001/components/omnibox/sear... File components/omnibox/search_suggestion_parser.cc (right): https://codereview.chromium.org/669573005/diff/160001/components/omnibox/sear... components/omnibox/search_suggestion_parser.cc:112: const SuggestResult& result) { On 2014/10/29 23:51:16, groby wrote: > Idiomatic name for operator='s parameter is rhs. ("Right hand side"). I'd prefer > that. Done. https://codereview.chromium.org/669573005/diff/160001/components/omnibox/sear... components/omnibox/search_suggestion_parser.cc:116: Result::operator=(result); On 2014/10/29 23:51:16, groby wrote: > Gah. It's valid C++, but horrible to read. > > static_cast<Result>(*this) = rhs; > > or at least > this->Result::operator=(rhs) Done. https://codereview.chromium.org/669573005/diff/160001/components/omnibox/sear... components/omnibox/search_suggestion_parser.cc:517: answer.get(), suggest_query_params, deletion_url, is_keyword_result, On 2014/10/29 23:51:16, groby wrote: > It bugs me that this is yet another unnecessary copy... Done.
Patchset #11 (id:200001) has been deleted
LGTM although there are a few more substantive comments below, because I don't want to have to review again :) https://codereview.chromium.org/669573005/diff/220001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/669573005/diff/220001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:3468: EXPECT_FALSE(matches[1].answer.get()); Nit: No get() on scoped_ptrs used as bools (4 places) https://codereview.chromium.org/669573005/diff/220001/components/omnibox/base... File components/omnibox/base_search_provider.cc (right): https://codereview.chromium.org/669573005/diff/220001/components/omnibox/base... components/omnibox/base_search_provider.cc:432: !more_relevant_match.answer.get()) { Nit: No get() on scoped_ptrs used as bools https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sear... File components/omnibox/search_provider.cc (right): https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sear... components/omnibox/search_provider.cc:845: if (it->answer.get() && it->fill_into_edit == trimmed_verbatim) { Nit: No get() on scoped_ptrs used as bools (2 places) https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sear... File components/omnibox/search_suggestion_parser.cc (right): https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sear... components/omnibox/search_suggestion_parser.cc:115: this->Result::operator=(rhs); I don't understand why Rachel asked for "this->" here. How does that add any clarity? It just seems more verbose to me, which means a slight _reduction_ in clarity. I think the way you had it originally is the clearest way to do this (and seems quite readable to me). Maybe Rachel would be happy with a comment?: // First, use the parent version of operator=() to copy the base portions. Result::operator=(rhs); // Now copy the subclass bits. ... https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... File components/omnibox/suggestion_answer.cc (right): https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer.cc:15: // All of these are defined here (even though they're only used once each) so Nit: Maybe "even if they're used sparingly below" or similar since not all these are only used once (e.g. kAnswerJsonText). https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer.cc:50: additional_text_(line.additional_text_.get() ? Nit: No get() on scoped_ptrs used as bools (6 places) https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer.cc:126: if (additional_text_.get()) { This conditional isn't quite correct, because it will succeed if (!additional_text_ && line.additional_text_) (the outer conditional will fail and we won't check |line|). Same with the next conditional block. (You should probably ensure your tests cover this.) https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer.cc:151: bool SuggestionAnswer::ParseAnswer( Hmm. I think it would be slightly nicer if this function and the similarly-named one below returned a scoped_ptr<SuggestionAnswer> directly instead of having a bool and an outparam. The one non-test caller could accommodate this with only a slight code change (splitting the large conditional that ends by calling ParseAnswer() into two pieces), and it would make some of the test code simpler, e.g.: scoped_ptr<SuggestionAnswer> answer; ASSERT_FALSE(SuggestionAnswer::ParseAnswer(json, &answer)); -> ASSERT_FALSE(SuggestionAnswer::ParseAnswer(json)); Plus this reads a little more clearly to me -- returning a pointer makes it obvious that NULL means failure, whereas with a bool one might wonder if the function could return true/false in different cases than the outparam was set to null/non-null. https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... File components/omnibox/suggestion_answer.h (right): https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:45: }; You still need all of these classes to be copyable, right? Perhaps an addition to the comment above noting that all the classes need to be copyable and why might be good? https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:52: ImageLine(const ImageLine& line); Nit: Explicit https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:79: const std::string& answer_json, scoped_ptr<SuggestionAnswer>* answer); Is this wrapper just a test-only helper? If so, I think it should be defined in the test file instead of here. https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:84: static scoped_ptr<SuggestionAnswer> Copy(const SuggestionAnswer* source) { Is it worth noting a TODO that we could eliminate this sort of thing if callers could use something like std::optional<T>? (It sounds from the thread like that's likely to move forward.) https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:85: return make_scoped_ptr(source ? new SuggestionAnswer(*source) : nullptr); Nit: Either call this copy(), or don't inline it. https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:93: int type() const { return type_; } Should we have an enum that we keep synced to said document? Or is the type basically an opaque value as far as Chrome code is concerned? https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:98: typedef std::vector<GURL> URLs; Nit: typedefs and enums go near the top of the section (in this case, I'd put both this class' typedefs just below the member class declarations). https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... File components/omnibox/suggestion_answer_unittest.cc (right): https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer_unittest.cc:190: EXPECT_EQ("https://example.com/foo.jpg", first_line.image_url().spec()); Nit: Technically you should probably either use possibly_invalid_spec() or ASSERT(url.is_valid()). https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer_unittest.cc:209: std::vector<GURL> urls; Nit: Use typedef
https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sear... File components/omnibox/search_suggestion_parser.cc (right): https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sear... components/omnibox/search_suggestion_parser.cc:115: this->Result::operator=(rhs); On 2014/10/30 03:43:29, Peter Kasting wrote: > I don't understand why Rachel asked for "this->" here. It parses easier, to my brain. Since it doesn't to you, it might not be as helpful as I think :) > Maybe Rachel would be happy with a comment?: I'm fine with a comment. Doesn't even need to be super verbose - just "Assign via base class first", or something. (If we were bikeshedding, I'd argue for the static_cast<Result>(*this) form, but let's not :) https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... File components/omnibox/suggestion_answer.h (right): https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:93: int type() const { return type_; } On 2014/10/30 03:43:29, Peter Kasting wrote: > Should we have an enum that we keep synced to said document? Or is the type > basically an opaque value as far as Chrome code is concerned? It should be opaque to Chrome. We _might_ need to surface some of the types at some point, but maintaining a full list is not necessary. https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... File components/omnibox/suggestion_answer_unittest.cc (right): https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer_unittest.cc:190: EXPECT_EQ("https://example.com/foo.jpg", first_line.image_url().spec()); On 2014/10/30 03:43:30, Peter Kasting wrote: > Nit: Technically you should probably either use possibly_invalid_spec() or > ASSERT(url.is_valid()). Since it's based on json defined in the test, we should do more than just assert validity. (Switching protocols _should_ be surfaced by the test, for example). To sidestep the validity issue, EXPECT_EQ(GURL("https://example.com/foo.jpg"), first_line.image_url())
Patchset #12 (id:240001) has been deleted
Rachel, you mind taking another look at this? I've made a ton of changes and it could probably use a second pair of eyes. https://codereview.chromium.org/669573005/diff/220001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/669573005/diff/220001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:3468: EXPECT_FALSE(matches[1].answer.get()); On 2014/10/30 03:43:29, Peter Kasting wrote: > Nit: No get() on scoped_ptrs used as bools (4 places) Nice. Rachel mentioned this earlier but it slipped my mind. Done. https://codereview.chromium.org/669573005/diff/220001/components/omnibox/base... File components/omnibox/base_search_provider.cc (right): https://codereview.chromium.org/669573005/diff/220001/components/omnibox/base... components/omnibox/base_search_provider.cc:432: !more_relevant_match.answer.get()) { On 2014/10/30 03:43:29, Peter Kasting wrote: > Nit: No get() on scoped_ptrs used as bools Done. https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sear... File components/omnibox/search_provider.cc (right): https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sear... components/omnibox/search_provider.cc:845: if (it->answer.get() && it->fill_into_edit == trimmed_verbatim) { On 2014/10/30 03:43:29, Peter Kasting wrote: > Nit: No get() on scoped_ptrs used as bools (2 places) Done. https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sear... File components/omnibox/search_suggestion_parser.cc (right): https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sear... components/omnibox/search_suggestion_parser.cc:115: this->Result::operator=(rhs); On 2014/10/30 16:21:14, groby wrote: > On 2014/10/30 03:43:29, Peter Kasting wrote: > > I don't understand why Rachel asked for "this->" here. > > It parses easier, to my brain. Since it doesn't to you, it might not be as > helpful as I think :) > > > > Maybe Rachel would be happy with a comment?: > I'm fine with a comment. Doesn't even need to be super verbose - just "Assign > via base class first", or something. > > (If we were bikeshedding, I'd argue for the static_cast<Result>(*this) form, but > let's not :) Done. https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... File components/omnibox/suggestion_answer.cc (right): https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer.cc:15: // All of these are defined here (even though they're only used once each) so On 2014/10/30 03:43:29, Peter Kasting wrote: > Nit: Maybe "even if they're used sparingly below" or similar since not all these > are only used once (e.g. kAnswerJsonText). Done. https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer.cc:50: additional_text_(line.additional_text_.get() ? On 2014/10/30 03:43:29, Peter Kasting wrote: > Nit: No get() on scoped_ptrs used as bools (6 places) Done. https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer.cc:126: if (additional_text_.get()) { On 2014/10/30 03:43:29, Peter Kasting wrote: > This conditional isn't quite correct, because it will succeed if > (!additional_text_ && line.additional_text_) (the outer conditional will fail > and we won't check |line|). Same with the next conditional block. > > (You should probably ensure your tests cover this.) Done and done. https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer.cc:151: bool SuggestionAnswer::ParseAnswer( On 2014/10/30 03:43:29, Peter Kasting wrote: > Hmm. I think it would be slightly nicer if this function and the > similarly-named one below returned a scoped_ptr<SuggestionAnswer> directly > instead of having a bool and an outparam. > > The one non-test caller could accommodate this with only a slight code change > (splitting the large conditional that ends by calling ParseAnswer() into two > pieces), and it would make some of the test code simpler, e.g.: > > scoped_ptr<SuggestionAnswer> answer; > ASSERT_FALSE(SuggestionAnswer::ParseAnswer(json, &answer)); > > -> > > ASSERT_FALSE(SuggestionAnswer::ParseAnswer(json)); > > Plus this reads a little more clearly to me -- returning a pointer makes it > obvious that NULL means failure, whereas with a bool one might wonder if the > function could return true/false in different cases than the outparam was set to > null/non-null. Done. I agree that this is better. To be honest, designing and building this class and figuring out how to properly handle these scoped_ptrs has caused me some pretty serious brain aches. But now that I feel like I'm starting to get a handle on it I'm excited by the idea that one can build C++ that is both reliable and properly OO. And that's the first time I've ever used the words "excited" and "C++" in the same sentence. https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... File components/omnibox/suggestion_answer.h (right): https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:45: }; On 2014/10/30 03:43:29, Peter Kasting wrote: > You still need all of these classes to be copyable, right? Perhaps an addition > to the comment above noting that all the classes need to be copyable and why > might be good? Done. https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:52: ImageLine(const ImageLine& line); On 2014/10/30 03:43:29, Peter Kasting wrote: > Nit: Explicit Done. https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:79: const std::string& answer_json, scoped_ptr<SuggestionAnswer>* answer); On 2014/10/30 03:43:29, Peter Kasting wrote: > Is this wrapper just a test-only helper? If so, I think it should be defined in > the test file instead of here. Done. https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:84: static scoped_ptr<SuggestionAnswer> Copy(const SuggestionAnswer* source) { On 2014/10/30 03:43:29, Peter Kasting wrote: > Is it worth noting a TODO that we could eliminate this sort of thing if callers > could use something like std::optional<T>? (It sounds from the thread like > that's likely to move forward.) Done. https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:85: return make_scoped_ptr(source ? new SuggestionAnswer(*source) : nullptr); On 2014/10/30 03:43:29, Peter Kasting wrote: > Nit: Either call this copy(), or don't inline it. Done. https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:93: int type() const { return type_; } On 2014/10/30 16:21:14, groby wrote: > On 2014/10/30 03:43:29, Peter Kasting wrote: > > Should we have an enum that we keep synced to said document? Or is the type > > basically an opaque value as far as Chrome code is concerned? > > It should be opaque to Chrome. We _might_ need to surface some of the types at > some point, but maintaining a full list is not necessary. Acknowledged. https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:98: typedef std::vector<GURL> URLs; On 2014/10/30 03:43:30, Peter Kasting wrote: > Nit: typedefs and enums go near the top of the section (in this case, I'd put > both this class' typedefs just below the member class declarations). Without forward declaration, TextFields has to go between TextField and ImageLine. I forward declared TextField and put the typedefs at the very top. Other suggestions are welcome. https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... File components/omnibox/suggestion_answer_unittest.cc (right): https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer_unittest.cc:190: EXPECT_EQ("https://example.com/foo.jpg", first_line.image_url().spec()); On 2014/10/30 16:21:14, groby wrote: > On 2014/10/30 03:43:30, Peter Kasting wrote: > > Nit: Technically you should probably either use possibly_invalid_spec() or > > ASSERT(url.is_valid()). > > Since it's based on json defined in the test, we should do more than just assert > validity. (Switching protocols _should_ be surfaced by the test, for example). > > To sidestep the validity issue, EXPECT_EQ(GURL("https://example.com/foo.jpg"), > first_line.image_url()) Done. https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer_unittest.cc:209: std::vector<GURL> urls; On 2014/10/30 03:43:30, Peter Kasting wrote: > Nit: Use typedef Done.
https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... File components/omnibox/suggestion_answer.cc (right): https://codereview.chromium.org/669573005/diff/220001/components/omnibox/sugg... components/omnibox/suggestion_answer.cc:151: bool SuggestionAnswer::ParseAnswer( On 2014/10/30 19:23:13, Justin Donnelly wrote: > On 2014/10/30 03:43:29, Peter Kasting wrote: > > Hmm. I think it would be slightly nicer if this function and the > > similarly-named one below returned a scoped_ptr<SuggestionAnswer> directly > > instead of having a bool and an outparam. > > > > The one non-test caller could accommodate this with only a slight code change > > (splitting the large conditional that ends by calling ParseAnswer() into two > > pieces), and it would make some of the test code simpler, e.g.: > > > > scoped_ptr<SuggestionAnswer> answer; > > ASSERT_FALSE(SuggestionAnswer::ParseAnswer(json, &answer)); > > > > -> > > > > ASSERT_FALSE(SuggestionAnswer::ParseAnswer(json)); > > > > Plus this reads a little more clearly to me -- returning a pointer makes it > > obvious that NULL means failure, whereas with a bool one might wonder if the > > function could return true/false in different cases than the outparam was set > to > > null/non-null. > > Done. > > I agree that this is better. To be honest, designing and building this class > and figuring out how to properly handle these scoped_ptrs has caused me some > pretty serious brain aches. But now that I feel like I'm starting to get a > handle on it I'm excited by the idea that one can build C++ that is both > reliable and properly OO. And that's the first time I've ever used the words > "excited" and "C++" in the same sentence. Uh-oh. Please step back from the computer - excitement and C++ don't work well together ;) https://codereview.chromium.org/669573005/diff/250001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/669573005/diff/250001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:3441: match1.answer = answer1.Pass(); If you call Pass() here, you relinquish ownership (the line is equivalent to match1.answer.reset(answer1.release())) I'm surprised the tests actually pass. You might want to change this to SuggestionAnswer answer1; ... match1.answer = SuggestionAnswer::Copy(&answer1) https://codereview.chromium.org/669573005/diff/250001/components/omnibox/auto... File components/omnibox/autocomplete_match.cc (right): https://codereview.chromium.org/669573005/diff/250001/components/omnibox/auto... components/omnibox/autocomplete_match.cc:83: answer(SuggestionAnswer::copy(match.answer.get())), match.answer should suffice - here and elsewhere. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/base... File components/omnibox/base_search_provider_unittest.cc (right): https://codereview.chromium.org/669573005/diff/250001/components/omnibox/base... components/omnibox/base_search_provider_unittest.cc:146: base::string16(), answer_contents, answer_type, answer.Pass(), If you use SuggestionAnswer::copy here, you save yourself both the scoped_ptr and the re-creation of answer later. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/base... components/omnibox/base_search_provider_unittest.cc:180: base::string16(), answer_contents2, answer_type2, answer2.Pass(), See above. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sear... File components/omnibox/search_suggestion_parser.cc (right): https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sear... components/omnibox/search_suggestion_parser.cc:510: answer_type_str = base::ASCIIToUTF16(""); .. = string16(); https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... File components/omnibox/suggestion_answer.cc (right): https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer.cc:17: static const char* kAnswerJsonImageLine = "il"; text constants should always be const char arrays, e.g. static const char kAnswerImageLine[] = "il"; https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer.cc:119: if (text_fields_.size() != line.text_fields_.size()) I'd argue this is a valid reason why TextField needs operator== - all this just becomes if (text_fields_ != line.text_fields)... IIRC, Peter argued against that, so don't change, but let's have that discussion in a separate CL. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer.cc:156: scoped_ptr<SuggestionAnswer> result(new SuggestionAnswer()); shorter: auto result=make_scoped_ptr(new SuggestionAnswer); https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... File components/omnibox/suggestion_answer.h (right): https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:43: static bool ParseTextField( should probably document that Parse can leave the out-param in an undefined state when false is returned - here and elsewhere. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:44: const base::DictionaryValue* field_json, TextField* text_field); minor style nit: static bool ParseTextField(const base::DictionaryValue* field_json, TextField* text_field); https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:55: FRIEND_TEST_ALL_PREFIXES(SuggestionAnswerTest, DifferentValuesAreUnequal); I commented in reverse - see below. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:65: const base::DictionaryValue* line_json, ImageLine* image_line); See above https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:77: scoped_ptr<TextField> status_text_; Since this has copy ctor/assignment op allowed, you'll need to answer the question of ownership for the assignment operator, or forbid it. I.e. private ImageLine& operator=(const ImageLine&); // to forbid https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:80: FRIEND_TEST_ALL_PREFIXES(SuggestionAnswerTest, DifferentValuesAreUnequal); Do you need this? All fields are accessible via getters. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:117: FRIEND_TEST_ALL_PREFIXES(SuggestionAnswerTest, DifferentValuesAreUnequal); Do you need this? You can access all variables via accessors. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... File components/omnibox/suggestion_answer_unittest.cc (right): https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer_unittest.cc:35: scoped_ptr<SuggestionAnswer> answer2(new SuggestionAnswer()); auto answer2=make_scoped_ptr.. Here and elsewhere. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer_unittest.cc:59: scoped_ptr<SuggestionAnswer> answer1 = ParseAnswer(json); auto, as well.
https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... File components/omnibox/suggestion_answer.cc (right): https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer.cc:119: if (text_fields_.size() != line.text_fields_.size()) On 2014/10/30 20:28:29, groby wrote: > I'd argue this is a valid reason why TextField needs operator== - all this just > becomes > if (text_fields_ != line.text_fields)... I'm very sympathetic to that, and in fact in general I personally believe overloading operators is a sane thing to do that makes code much better. Unfortunately, I think this is pretty clearly banned by the Google style guide, which really only allows overloading operator==() if it's required for use in an STL algorithm (e.g. find_if()). I don't see a way around that. This is an area where I disagree strongly with the style guide but feel bound to enforce it as written.
https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... File components/omnibox/suggestion_answer.cc (right): https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer.cc:119: if (text_fields_.size() != line.text_fields_.size()) On 2014/10/30 20:32:56, Peter Kasting wrote: > On 2014/10/30 20:28:29, groby wrote: > > I'd argue this is a valid reason why TextField needs operator== - all this > just > > becomes > > if (text_fields_ != line.text_fields)... > > I'm very sympathetic to that, and in fact in general I personally believe > overloading operators is a sane thing to do that makes code much better. > > Unfortunately, I think this is pretty clearly banned by the Google style guide, > which really only allows overloading operator==() if it's required for use in an > STL algorithm (e.g. find_if()). I don't see a way around that. > > This is an area where I disagree strongly with the style guide but feel bound to > enforce it as written. Grumble mumble. I'd say that the code base _happily_ ignores this - a quick search in CS reveals 750 declarations of operator== - but yeah, this discussion needs to be had around the style guide, not here.
https://codereview.chromium.org/669573005/diff/250001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/669573005/diff/250001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:3441: match1.answer = answer1.Pass(); On 2014/10/30 20:28:29, groby wrote: > If you call Pass() here, you relinquish ownership (the line is equivalent to > match1.answer.reset(answer1.release())) > > I'm surprised the tests actually pass. > > You might want to change this to > > SuggestionAnswer answer1; > ... > match1.answer = SuggestionAnswer::Copy(&answer1) > Done. This passed because I didn't use to have the Equals test below. I thought I had built and ran these tests before uploading this patch set but apparently not, because the new line below doesn't even compile. Also fixed. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/auto... File components/omnibox/autocomplete_match.cc (right): https://codereview.chromium.org/669573005/diff/250001/components/omnibox/auto... components/omnibox/autocomplete_match.cc:83: answer(SuggestionAnswer::copy(match.answer.get())), On 2014/10/30 20:28:29, groby wrote: > match.answer should suffice - here and elsewhere. It doesn't seem to (I tried and I don't see what in scoped_ptr.h would support that). Let me know if I'm missing something. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/base... File components/omnibox/base_search_provider_unittest.cc (right): https://codereview.chromium.org/669573005/diff/250001/components/omnibox/base... components/omnibox/base_search_provider_unittest.cc:146: base::string16(), answer_contents, answer_type, answer.Pass(), On 2014/10/30 20:28:29, groby wrote: > If you use SuggestionAnswer::copy here, you save yourself both the scoped_ptr > and the re-creation of answer later. Done. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/base... components/omnibox/base_search_provider_unittest.cc:180: base::string16(), answer_contents2, answer_type2, answer2.Pass(), On 2014/10/30 20:28:29, groby wrote: > See above. Done. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sear... File components/omnibox/search_suggestion_parser.cc (right): https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sear... components/omnibox/search_suggestion_parser.cc:510: answer_type_str = base::ASCIIToUTF16(""); On 2014/10/30 20:28:29, groby wrote: > .. = string16(); Done. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... File components/omnibox/suggestion_answer.cc (right): https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer.cc:17: static const char* kAnswerJsonImageLine = "il"; On 2014/10/30 20:28:29, groby wrote: > text constants should always be const char arrays, e.g. > > static const char kAnswerImageLine[] = "il"; Done. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer.cc:119: if (text_fields_.size() != line.text_fields_.size()) On 2014/10/30 20:46:12, groby wrote: > On 2014/10/30 20:32:56, Peter Kasting wrote: > > On 2014/10/30 20:28:29, groby wrote: > > > I'd argue this is a valid reason why TextField needs operator== - all this > > just > > > becomes > > > if (text_fields_ != line.text_fields)... > > > > I'm very sympathetic to that, and in fact in general I personally believe > > overloading operators is a sane thing to do that makes code much better. > > > > Unfortunately, I think this is pretty clearly banned by the Google style > guide, > > which really only allows overloading operator==() if it's required for use in > an > > STL algorithm (e.g. find_if()). I don't see a way around that. > > > > This is an area where I disagree strongly with the style guide but feel bound > to > > enforce it as written. > > Grumble mumble. I'd say that the code base _happily_ ignores this - a quick > search in CS reveals 750 declarations of operator== - but yeah, this discussion > needs to be had around the style guide, not here. Acknowledged. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer.cc:156: scoped_ptr<SuggestionAnswer> result(new SuggestionAnswer()); On 2014/10/30 20:28:29, groby wrote: > shorter: > > auto result=make_scoped_ptr(new SuggestionAnswer); Done. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... File components/omnibox/suggestion_answer.h (right): https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:43: static bool ParseTextField( On 2014/10/30 20:28:29, groby wrote: > should probably document that Parse can leave the out-param in an undefined > state when false is returned - here and elsewhere. Done. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:44: const base::DictionaryValue* field_json, TextField* text_field); On 2014/10/30 20:28:29, groby wrote: > minor style nit: > static bool ParseTextField(const base::DictionaryValue* field_json, > TextField* text_field); Done. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:55: FRIEND_TEST_ALL_PREFIXES(SuggestionAnswerTest, DifferentValuesAreUnequal); On 2014/10/30 20:28:30, groby wrote: > I commented in reverse - see below. Acknowledged. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:65: const base::DictionaryValue* line_json, ImageLine* image_line); On 2014/10/30 20:28:29, groby wrote: > See above Done. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:77: scoped_ptr<TextField> status_text_; On 2014/10/30 20:28:30, groby wrote: > Since this has copy ctor/assignment op allowed, you'll need to answer the > question of ownership for the assignment operator, or forbid it. I.e. > > private ImageLine& operator=(const ImageLine&); // to forbid Done. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:80: FRIEND_TEST_ALL_PREFIXES(SuggestionAnswerTest, DifferentValuesAreUnequal); On 2014/10/30 20:28:30, groby wrote: > Do you need this? All fields are accessible via getters. Yes, the getters return const values. If you read through the relevant test hopefully you'll agree it's worth the friending. The alternative would be to have different fully-compliant json strings for each case which would make it very difficult to see what was different between each one. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:117: FRIEND_TEST_ALL_PREFIXES(SuggestionAnswerTest, DifferentValuesAreUnequal); On 2014/10/30 20:28:30, groby wrote: > Do you need this? You can access all variables via accessors. Acknowledged. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... File components/omnibox/suggestion_answer_unittest.cc (right): https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer_unittest.cc:35: scoped_ptr<SuggestionAnswer> answer2(new SuggestionAnswer()); On 2014/10/30 20:28:30, groby wrote: > auto answer2=make_scoped_ptr.. > > Here and elsewhere. Done, although the use of auto here doesn't seem very compelling to me. Versus, say something like: map<std::string, scoped_ptr<SomeClass>>::Iterator But I do find C++11 features exciting! (I know, I know.) https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer_unittest.cc:59: scoped_ptr<SuggestionAnswer> answer1 = ParseAnswer(json); On 2014/10/30 20:28:30, groby wrote: > auto, as well. auto hurts more than it helps here, it seems to me. With make_scoped_ptr case, the constructor call makes the type clear. But with auto answer = ParseAnswer(json) you have to look up the signature of ParseAnswer to know what the type of the result is. This seems to me to be the kind of case referred to by the style guide as "Continue to use manifest type declarations when it helps readability".
https://codereview.chromium.org/669573005/diff/250001/components/omnibox/auto... File components/omnibox/autocomplete_match.cc (right): https://codereview.chromium.org/669573005/diff/250001/components/omnibox/auto... components/omnibox/autocomplete_match.cc:83: answer(SuggestionAnswer::copy(match.answer.get())), On 2014/10/30 21:38:03, Justin Donnelly wrote: > On 2014/10/30 20:28:29, groby wrote: > > match.answer should suffice - here and elsewhere. > > It doesn't seem to (I tried and I don't see what in scoped_ptr.h would support > that). Let me know if I'm missing something. Yeah, it wouldn't unless you made copy() take a const scoped_ptr& instead of a raw pointer. I thought about suggesting that change, but I wasn't sure whether it would have been a win overall, and it seems a little iffy in principle (the function doesn't care whether the source data is in a scoped_ptr, so why should its API force the issue?). https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... File components/omnibox/suggestion_answer_unittest.cc (right): https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer_unittest.cc:59: scoped_ptr<SuggestionAnswer> answer1 = ParseAnswer(json); On 2014/10/30 21:38:04, Justin Donnelly wrote: > On 2014/10/30 20:28:30, groby wrote: > > auto, as well. > > auto hurts more than it helps here, it seems to me. With make_scoped_ptr case, > the constructor call makes the type clear. But with auto answer = > ParseAnswer(json) you have to look up the signature of ParseAnswer to know what > the type of the result is. > > This seems to me to be the kind of case referred to by the style guide as > "Continue to use manifest type declarations when it helps readability". Shrug. I think auto would be OK here, but I'm not opposed to spelling it out either, because doing so doesn't take any additional lines of code or seem to involve too many ::<>() characters.
push_back requires operator=... Seems you need to implement it after all. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/auto... File components/omnibox/autocomplete_match.cc (right): https://codereview.chromium.org/669573005/diff/250001/components/omnibox/auto... components/omnibox/autocomplete_match.cc:83: answer(SuggestionAnswer::copy(match.answer.get())), On 2014/10/30 21:38:03, Justin Donnelly wrote: > On 2014/10/30 20:28:29, groby wrote: > > match.answer should suffice - here and elsewhere. > > It doesn't seem to (I tried and I don't see what in scoped_ptr.h would support > that). Let me know if I'm missing something. Sorry - was thinking of scoped_nsobject https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... File components/omnibox/suggestion_answer.h (right): https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer.h:80: FRIEND_TEST_ALL_PREFIXES(SuggestionAnswerTest, DifferentValuesAreUnequal); On 2014/10/30 21:38:03, Justin Donnelly wrote: > On 2014/10/30 20:28:30, groby wrote: > > Do you need this? All fields are accessible via getters. > > Yes, the getters return const values. If you read through the relevant test > hopefully you'll agree it's worth the friending. The alternative would be to > have different fully-compliant json strings for each case which would make it > very difficult to see what was different between each one. Acknowledged. https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... File components/omnibox/suggestion_answer_unittest.cc (right): https://codereview.chromium.org/669573005/diff/250001/components/omnibox/sugg... components/omnibox/suggestion_answer_unittest.cc:59: scoped_ptr<SuggestionAnswer> answer1 = ParseAnswer(json); On 2014/10/30 21:38:04, Justin Donnelly wrote: > On 2014/10/30 20:28:30, groby wrote: > > auto, as well. > > auto hurts more than it helps here, it seems to me. With make_scoped_ptr case, > the constructor call makes the type clear. But with auto answer = > ParseAnswer(json) you have to look up the signature of ParseAnswer to know what > the type of the result is. > > This seems to me to be the kind of case referred to by the style guide as > "Continue to use manifest type declarations when it helps readability". SGTM - auto will remain a judgment call, and I'm happy to be conservative there. https://codereview.chromium.org/669573005/diff/270001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/669573005/diff/270001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:3432: scoped_ptr<SuggestionAnswer> answer1(new SuggestionAnswer()); Not sure why pointer, but it does match the normal usage... Is that why you chose to stick with scoped_ptr?
Un-disallowed assignment operator for TextField, with comment explaining, continuing to disallow for the other two classes, which aren't used in vectors. https://codereview.chromium.org/669573005/diff/270001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/669573005/diff/270001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:3432: scoped_ptr<SuggestionAnswer> answer1(new SuggestionAnswer()); On 2014/10/30 22:13:58, groby wrote: > Not sure why pointer, but it does match the normal usage... Is that why you > chose to stick with scoped_ptr? I can't say any particular thought went into it, I think it was more leftover from the various earlier approaches. Switched to values for simplicity.
The CQ bit was checked by groby@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/669573005/290001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/669573005/diff/290001/components/omnibox/sugg... File components/omnibox/suggestion_answer_unittest.cc (right): https://codereview.chromium.org/669573005/diff/290001/components/omnibox/sugg... components/omnibox/suggestion_answer_unittest.cc:1: // copyright 2014 The Chromium Authors. All rights reserved. ARRRRG. Sorry for missing this. Capital C for Copyright, please.
https://codereview.chromium.org/669573005/diff/290001/components/omnibox/sugg... File components/omnibox/suggestion_answer_unittest.cc (right): https://codereview.chromium.org/669573005/diff/290001/components/omnibox/sugg... components/omnibox/suggestion_answer_unittest.cc:1: // copyright 2014 The Chromium Authors. All rights reserved. On 2014/10/31 00:20:14, groby wrote: > ARRRRG. > > Sorry for missing this. Capital C for Copyright, please. Ok, that's kind of embarassing. :) Done.
The CQ bit was checked by jdonnelly@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/669573005/310001
Message was sent while issue was closed.
Committed patchset #15 (id:310001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/7393cee9845330bbe5e4712f5e16751256e6cb7c Cr-Commit-Position: refs/heads/master@{#302210} |
