Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12314164:
Modified Omnibox extension api to use JSON Schema Compiler (Closed)
|
Created:
7 years, 9 months ago by Aaron Jacobs Modified:
7 years, 8 months ago CC:
chromium-reviews, Aaron Boodman, chromium-apps-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionModified Omnibox extension api to use JSON Schema Compiler
BUG=121174
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196628
Patch Set 1 #
Total comments: 12
Patch Set 2 : Devlin's requests #
Total comments: 4
Patch Set 3 : Devlin's requests #
Total comments: 10
Patch Set 4 : kalman's requests #
Total comments: 32
Patch Set 5 : kalman's requests #
Total comments: 31
Patch Set 6 : kalman's requests #
Total comments: 9
Patch Set 7 : kalman's requests #Patch Set 8 : Merged with master #Patch Set 9 : Merge with master #Patch Set 10 : Replaced include with forward declaration #Messages
Total messages: 26 (0 generated)
https://codereview.chromium.org/12314164/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right): https://codereview.chromium.org/12314164/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/omnibox/omnibox_api.cc:230: SendSuggestions::Params::Create(*args_)); Line continuation has 4-space indentation. https://codereview.chromium.org/12314164/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/omnibox/omnibox_api.cc:234: std::vector<linked_ptr< yeah, this is another ugly line. But I think it might look at least a little better as std::vector<linked_ptr<SendSuggestions::Params::SuggestResultsType> > suggest_results = params->suggest_results; https://codereview.chromium.org/12314164/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/omnibox/omnibox_api.cc:322: bool ExtensionOmniboxSuggestion::PopulateFromSuggestion( Perhaps better as PopulateFromDefaultSuggestion, since the DefaultSuggestion is the only one that contains the description and nothing else? https://codereview.chromium.org/12314164/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/omnibox/omnibox_api.cc:328: EXTENSION_FUNCTION_VALIDATE(UTF8ToUTF16(suggestion.description.c_str(), Change to description = UTF8ToUTF16(suggestion.description); https://codereview.chromium.org/12314164/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/omnibox/omnibox_api.h (right): https://codereview.chromium.org/12314164/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/omnibox/omnibox_api.h:40: namespace SetDefaultSuggestion = api::omnibox::SetDefaultSuggestion; Don't set a namespace like this in the .h file (use the full name on line 160). It's ugly and long, but it doesn't contaminate any files which include this one. https://codereview.chromium.org/12314164/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/omnibox/omnibox_api.h:158: // compiler (only sets the value of the description) Comments should always be grammatically correct (including articles, punctuation, etc).
https://codereview.chromium.org/12314164/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right): https://codereview.chromium.org/12314164/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/omnibox/omnibox_api.cc:230: SendSuggestions::Params::Create(*args_)); On 2013/02/27 22:14:17, D Cronin wrote: > Line continuation has 4-space indentation. Done. https://codereview.chromium.org/12314164/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/omnibox/omnibox_api.cc:234: std::vector<linked_ptr< On 2013/02/27 22:14:17, D Cronin wrote: > yeah, this is another ugly line. But I think it might look at least a little > better as > > std::vector<linked_ptr<SendSuggestions::Params::SuggestResultsType> > > suggest_results = params->suggest_results; Done. https://codereview.chromium.org/12314164/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/omnibox/omnibox_api.cc:322: bool ExtensionOmniboxSuggestion::PopulateFromSuggestion( On 2013/02/27 22:14:17, D Cronin wrote: > Perhaps better as PopulateFromDefaultSuggestion, since the DefaultSuggestion is > the only one that contains the description and nothing else? Done. https://codereview.chromium.org/12314164/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/omnibox/omnibox_api.cc:328: EXTENSION_FUNCTION_VALIDATE(UTF8ToUTF16(suggestion.description.c_str(), On 2013/02/27 22:14:17, D Cronin wrote: > Change to > description = UTF8ToUTF16(suggestion.description); Done. https://codereview.chromium.org/12314164/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/omnibox/omnibox_api.h (right): https://codereview.chromium.org/12314164/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/omnibox/omnibox_api.h:40: namespace SetDefaultSuggestion = api::omnibox::SetDefaultSuggestion; On 2013/02/27 22:14:17, D Cronin wrote: > Don't set a namespace like this in the .h file (use the full name on line 160). > It's ugly and long, but it doesn't contaminate any files which include this one. Done. https://codereview.chromium.org/12314164/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/omnibox/omnibox_api.h:158: // compiler (only sets the value of the description) On 2013/02/27 22:14:17, D Cronin wrote: > Comments should always be grammatically correct (including articles, > punctuation, etc). Done.
https://codereview.chromium.org/12314164/diff/2002/chrome/browser/extensions/... File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right): https://codereview.chromium.org/12314164/diff/2002/chrome/browser/extensions/... chrome/browser/extensions/api/omnibox/omnibox_api.cc:243: suggest_results.at(i)->additional_properties, true)); nit: why use at(), instead of just suggest_results[i]? https://codereview.chromium.org/12314164/diff/2002/chrome/browser/extensions/... chrome/browser/extensions/api/omnibox/omnibox_api.cc:257: SetDefaultSuggestion::Params::Create(*args_)); nit: still 4-space line continuation :)
https://codereview.chromium.org/12314164/diff/2002/chrome/browser/extensions/... File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right): https://codereview.chromium.org/12314164/diff/2002/chrome/browser/extensions/... chrome/browser/extensions/api/omnibox/omnibox_api.cc:243: suggest_results.at(i)->additional_properties, true)); On 2013/02/27 23:18:43, D Cronin wrote: > nit: why use at(), instead of just suggest_results[i]? Done. https://codereview.chromium.org/12314164/diff/2002/chrome/browser/extensions/... chrome/browser/extensions/api/omnibox/omnibox_api.cc:257: SetDefaultSuggestion::Params::Create(*args_)); On 2013/02/27 23:18:43, D Cronin wrote: > nit: still 4-space line continuation :) Done.
lgtm
+kalman Kalman, this is a quick CL from one of our new hires, Aaron. Mind taking a look?
So - I think that a cleanup change that results in an overall increase in line count is an indication that we haven't gone far enough. The thing with this JSON schema compilation is that it fundamentally changes the structure of API implementations - if we just try to pick parts of it to use, it'll just end up being awkward. If we're going to do this - and you've already done part of it, and it's a nice thing to do - we should go all the way. I'm not very familiar with the omnibox code, but here goes - The omnibox API definition just has that "description" field, as you've seen, plus the request ID parameter. It gets the request ID populated from... somewhere. Things that use ExtensionOmniboxSuggestion populate it from that sendSuggestions function and half-populate it from the setDefaultSuggestions function. Maybe other places. To cut a long and vague story short, here's what we should try to do: add all the structured content to omnibox.json behind "nodoc" (e.g. descriptionStyles/Raw, the type/offset/length things of it, etc). omnibox.js already populates something like it but we can do better, not use "any", get proper JSON validation there, etc. Then delete ExtensionOmniboxSuggestion(s) and just use the structs that the JSON schema compiler generates everywhere. The only thing not convertible is that (a) it'll use string16 not std::string (possibly that's an improvement we can make to the compiler, but for now conversion isn't so bad), and description_styles - but that's only used in 1 place and we can just write our own method which does the conversion as necessary. This all is probably a slight understatement of the work that needs to be done - but if we're going to use the JSON schema compiler, we should use it properly. I'd make our high-level goal to be to remove all reference to base::Values, those kSuggestionDescriptionStyles etc values, and the structs from omnibox_api.h. The first two of those might not be entirely possible, but it's something to keep in mind. https://codereview.chromium.org/12314164/diff/7001/chrome/browser/extensions/... File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right): https://codereview.chromium.org/12314164/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/api/omnibox/omnibox_api.cc:232: EXTENSION_FUNCTION_VALIDATE(params.get()); just params not params.get(). https://codereview.chromium.org/12314164/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/api/omnibox/omnibox_api.cc:240: ExtensionOmniboxSuggestion& suggestion = suggestions.suggestions[i]; inline this https://codereview.chromium.org/12314164/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/api/omnibox/omnibox_api.cc:259: blank line unnecessary https://codereview.chromium.org/12314164/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/api/omnibox/omnibox_api.cc:329: blank lines unecessary https://codereview.chromium.org/12314164/diff/7001/chrome/browser/extensions/... File chrome/browser/extensions/api/omnibox/omnibox_api.h (right): https://codereview.chromium.org/12314164/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/api/omnibox/omnibox_api.h:18: #include "chrome/common/extensions/api/omnibox.h" Should be able to forward-declare api::omnibox::SetDefaultSuggestion::Params::Suggestion rather than needing to include the header here.
https://codereview.chromium.org/12314164/diff/7001/chrome/browser/extensions/... File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right): https://codereview.chromium.org/12314164/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/api/omnibox/omnibox_api.cc:232: EXTENSION_FUNCTION_VALIDATE(params.get()); On 2013/03/11 16:32:40, kalman wrote: > just params not params.get(). Done. https://codereview.chromium.org/12314164/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/api/omnibox/omnibox_api.cc:240: ExtensionOmniboxSuggestion& suggestion = suggestions.suggestions[i]; On 2013/03/11 16:32:40, kalman wrote: > inline this Done. https://codereview.chromium.org/12314164/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/api/omnibox/omnibox_api.cc:259: On 2013/03/11 16:32:40, kalman wrote: > blank line unnecessary Done. https://codereview.chromium.org/12314164/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/api/omnibox/omnibox_api.cc:329: On 2013/03/11 16:32:40, kalman wrote: > blank lines unecessary Done. https://codereview.chromium.org/12314164/diff/7001/chrome/browser/extensions/... File chrome/browser/extensions/api/omnibox/omnibox_api.h (right): https://codereview.chromium.org/12314164/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/api/omnibox/omnibox_api.h:18: #include "chrome/common/extensions/api/omnibox.h" On 2013/03/11 16:32:40, kalman wrote: > Should be able to forward-declare > api::omnibox::SetDefaultSuggestion::Params::Suggestion rather than needing to > include the header here. Unless I'm mistaken, I don't think a forward declaration is possible, as the |Suggestion| struct is nested within the |Params| struct. See: http://stackoverflow.com/questions/951234/forward-declaration-of-nested-types...
nice! https://codereview.chromium.org/12314164/diff/14001/chrome/browser/autocomple... File chrome/browser/autocomplete/keyword_provider.cc (right): https://codereview.chromium.org/12314164/diff/14001/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider.cc:563: const omnibox::SendSuggestions::Params& suggestions = I think this should be omnibox_api:: rather than omnibox::, it reads like it's part of Chrome's omnibox code when it's not really. https://codereview.chromium.org/12314164/diff/14001/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider.cc:594: suggestion); prefer line break after the = https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right): https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:39: namespace omnibox = extensions::api::omnibox; fwiw extensions:: is unnecessary because we're already in the extensions namespace here. https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:251: if (!params->suggestion.description_styles) { I think it would be better to just do params->suggestion.description_styles.reset(). And add a comment why you're doing this. https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:254: omnibox::SuggestDefaultResult::DescriptionStylesType> >); I can't remember what SuggestDefaultResult is, but again I'd rather not have an alias for omnibox and rather do 'using api::omnibox::SuggestDefaultResult' or whatever. https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:279: *suggestion.description_styles; This is pretty unwieldy. Just use an iterator below and access suggestion.description_styles both times. https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:287: if (style_types[i].get() && style_types[i]->length) don't need .get(), use !empty() https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:296: if (style_types[i]->type == use switch https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:330: ACMatchClassifications DefaultStyleTypesToACMatchClassifications( what is the difference between this function and the one above? https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... File chrome/browser/extensions/api/omnibox/omnibox_unittest.cc (right): https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_unittest.cc:56: dict->SetWithoutPathExpansion("descriptionStyles", styles_value.release()); you'll probably find chrome/common/extensions/value_builder.h useful. It will clean up all of these tests a bit. https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_unittest.cc:74: api::omnibox::SendSuggestions::Params::Create(list)); for consistency, use same typedefs in the api file? https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_unittest.cc:76: EXPECT_TRUE(params->suggest_results[0].get()); .get() unnecessary https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... File chrome/browser/extensions/extension_prefs.cc (right): https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/extension_prefs.cc:1162: scoped_ptr<api::omnibox::SuggestDefaultResult> suggestion(0); use NULL not 0, but besides scoped_ptrs are already initialized to null, so argument not needed. https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/extension_prefs.cc:1177: scoped_ptr<base::DictionaryValue> dict = suggestion.ToValue().Pass(); not sure if Pass is necessary here, rvalues on scoped_ptrs don't need it. https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... File chrome/browser/extensions/extension_prefs.h (right): https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/extension_prefs.h:334: const extensions::api::omnibox::SuggestDefaultResult& suggestion); extensions:: looks unnecssary https://codereview.chromium.org/12314164/diff/14001/chrome/common/extensions/... File chrome/common/extensions/api/omnibox.json (right): https://codereview.chromium.org/12314164/diff/14001/chrome/common/extensions/... chrome/common/extensions/api/omnibox.json:58: "id": "SuggestDefaultResult", Ah, I see. Let's just make content on SuggestResult optional and document it ("this field is required for $ref:sendSuggestions, and must be ommitted for $ref:setDefaultSuggestion").
https://codereview.chromium.org/12314164/diff/14001/chrome/browser/autocomple... File chrome/browser/autocomplete/keyword_provider.cc (right): https://codereview.chromium.org/12314164/diff/14001/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider.cc:563: const omnibox::SendSuggestions::Params& suggestions = On 2013/03/16 00:04:25, kalman wrote: > I think this should be omnibox_api:: rather than omnibox::, it reads like it's > part of Chrome's omnibox code when it's not really. Done. https://codereview.chromium.org/12314164/diff/14001/chrome/browser/autocomple... chrome/browser/autocomplete/keyword_provider.cc:594: suggestion); On 2013/03/16 00:04:25, kalman wrote: > prefer line break after the = Done. https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right): https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:39: namespace omnibox = extensions::api::omnibox; On 2013/03/16 00:04:25, kalman wrote: > fwiw extensions:: is unnecessary because we're already in the extensions > namespace here. Done. https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:251: if (!params->suggestion.description_styles) { On 2013/03/16 00:04:25, kalman wrote: > I think it would be better to just do > params->suggestion.description_styles.reset(). And add a comment why you're > doing this. This section of code was not actually needed, as it was mimicking part of the old ExtensionOmniboxSuggestion::Populate function (which is all now handled by the schema-compiled code), so I've removed it. https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:254: omnibox::SuggestDefaultResult::DescriptionStylesType> >); On 2013/03/16 00:04:25, kalman wrote: > I can't remember what SuggestDefaultResult is, but again I'd rather not have an > alias for omnibox and rather do 'using api::omnibox::SuggestDefaultResult' or > whatever. Done. https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:279: *suggestion.description_styles; On 2013/03/16 00:04:25, kalman wrote: > This is pretty unwieldy. Just use an iterator below and access > suggestion.description_styles both times. Done. https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:287: if (style_types[i].get() && style_types[i]->length) On 2013/03/16 00:04:25, kalman wrote: > don't need .get(), use !empty() I can't remove .get(), as it is operating on a linked_ptr, not a scoped_ptr; also, length is a scoped_ptr to an int, not a vector, so it is appropriate. https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:296: if (style_types[i]->type == On 2013/03/16 00:04:25, kalman wrote: > use switch Done. https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:330: ACMatchClassifications DefaultStyleTypesToACMatchClassifications( On 2013/03/16 00:04:25, kalman wrote: > what is the difference between this function and the one above? They served pretty much the same purpose, though with the different types. It has now been removed (see https://codereview.chromium.org/12314164/diff/14001/chrome/common/extensions/...). https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... File chrome/browser/extensions/api/omnibox/omnibox_unittest.cc (right): https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_unittest.cc:56: dict->SetWithoutPathExpansion("descriptionStyles", styles_value.release()); On 2013/03/16 00:04:25, kalman wrote: > you'll probably find chrome/common/extensions/value_builder.h useful. It will > clean up all of these tests a bit. Done. https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_unittest.cc:74: api::omnibox::SendSuggestions::Params::Create(list)); On 2013/03/16 00:04:25, kalman wrote: > for consistency, use same typedefs in the api file? Done. https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_unittest.cc:76: EXPECT_TRUE(params->suggest_results[0].get()); On 2013/03/16 00:04:25, kalman wrote: > .get() unnecessary I can't remove it (it's a linked_ptr, not a scoped_ptr). https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... File chrome/browser/extensions/extension_prefs.cc (right): https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/extension_prefs.cc:1162: scoped_ptr<api::omnibox::SuggestDefaultResult> suggestion(0); On 2013/03/16 00:04:25, kalman wrote: > use NULL not 0, but besides scoped_ptrs are already initialized to null, so > argument not needed. Done. https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/extension_prefs.cc:1177: scoped_ptr<base::DictionaryValue> dict = suggestion.ToValue().Pass(); On 2013/03/16 00:04:25, kalman wrote: > not sure if Pass is necessary here, rvalues on scoped_ptrs don't need it. Done. https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... File chrome/browser/extensions/extension_prefs.h (right): https://codereview.chromium.org/12314164/diff/14001/chrome/browser/extensions... chrome/browser/extensions/extension_prefs.h:334: const extensions::api::omnibox::SuggestDefaultResult& suggestion); On 2013/03/16 00:04:25, kalman wrote: > extensions:: looks unnecssary Done. https://codereview.chromium.org/12314164/diff/14001/chrome/common/extensions/... File chrome/common/extensions/api/omnibox.json (right): https://codereview.chromium.org/12314164/diff/14001/chrome/common/extensions/... chrome/common/extensions/api/omnibox.json:58: "id": "SuggestDefaultResult", On 2013/03/16 00:04:25, kalman wrote: > Ah, I see. Let's just make content on SuggestResult optional and document it > ("this field is required for $ref:sendSuggestions, and must be ommitted for > $ref:setDefaultSuggestion"). Done, except I had to remove the reference to sendSuggestions, as it has been nodoc'd.
sorry about the mind-changing. https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right): https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:236: EXTENSION_FUNCTION_VALIDATE(params); if we don't validate that the content field was set then this will crash that that CHECK in the omnibox code. besides which, we need to check for it and set error_ if it's not there. see comment in omnibox.json though. https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:250: likewise need to check somewhere that it isn't set (but see comment). https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:272: styles.resize(description.length()); // sets all styles to 0 you should be able to give this as an argument to the constructor. std::vector<int> styles(description.length(), 0); make sure the order is right.. https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:274: for (std::vector<linked_ptr<omnibox::SuggestResult::DescriptionStylesType> > wow. maybe we should generate typedefs in the schema compiler. https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:276: i != suggestion.description_styles->end(); ++i) { super nit: for loop style is to align vertically, not 4 spaces, i.e. for (std::vector... ::iterator...) not for (std::vector, ::iterator) https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:277: int type_class, offset, length; just declare them when they're initialized. https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:279: if ((*i).get() && (*i)->length) please save a reference to *i (or better: the raw pointer underlying it), it's pretty hard to read. https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:286: offset = std::max(0, static_cast<int>(description.length()) + offset); if you make this a size_t and ternary-initialize it then you wouldn't need the static_cast below. https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/api/omnibox/omnibox_unittest.cc (right): https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_unittest.cc:43: ListValue* list = ListBuilder().Append(42).Append(ListBuilder().Append( looks like a memory leak? you're releasing this (at the end) but nothing seems to take ownership of it. leave it as a scoped_ptr here. https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_unittest.cc:54: .Set("length", 3))))).Build().release(); using some indentation here would be nice to make it a bit easier to follow what data you're creating. e.g. scoped_ptr<ListValue> list = ListBuilder() .Append(42) .Append(ListBuilder() .Append(DictionaryBuilder() .Set(content) .Set(description) .Set(descriptionStyles, ListBuilder() .Append(DictionaryBuilder()) .Set(..) .Set(..) etc. https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_unittest.cc:71: ListValue* swap_list = ListBuilder().Append(42).Append(ListBuilder().Append( same comments https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_unittest.cc:100: ListValue* list = ListBuilder().Append(42).Append(ListBuilder().Append( same comments https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_unittest.cc:142: ListValue* moved_list = ListBuilder().Append(42).Append(ListBuilder().Append( same comments https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_unittest.cc:183: ListValue* list = ListBuilder().Append(42).Append(ListBuilder().Append( same comments https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/extension_prefs.cc (right): https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/extension_prefs.cc:1163: scoped_ptr<base::DictionaryValue> dict = suggestion.ToValue(); I guess if you change omnibox.json you'd need to delete content from the dict here. https://codereview.chromium.org/12314164/diff/24001/chrome/common/extensions/... File chrome/common/extensions/api/omnibox.json (right): https://codereview.chromium.org/12314164/diff/24001/chrome/common/extensions/... chrome/common/extensions/api/omnibox.json:16: "optional": true, ugh, ok, I change my mind here sorry. this being optional is introducing some complexity in the code that is a bit worrying (needing to check for null etc). let's make this required again, but modify omnibox_custom_bindings.js to put it in for setDefaultSuggestions: something like https://codereview.chromium.org/12663022 should work (haven't tested it).
https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right): https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:236: EXTENSION_FUNCTION_VALIDATE(params); On 2013/03/22 17:33:30, kalman wrote: > if we don't validate that the content field was set then this will crash that > that CHECK in the omnibox code. besides which, we need to check for it and set > error_ if it's not there. > > see comment in omnibox.json though. I believe the check is no longer needed now that the content field is not optional. https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:250: On 2013/03/22 17:33:30, kalman wrote: > likewise need to check somewhere that it isn't set (but see comment). This should no longer be needed with the modifications made to chrome/renderer/resources/extensions/omnibox_custom_bindings.js https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:272: styles.resize(description.length()); // sets all styles to 0 On 2013/03/22 17:33:30, kalman wrote: > you should be able to give this as an argument to the constructor. > std::vector<int> styles(description.length(), 0); > > make sure the order is right.. Done. https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:276: i != suggestion.description_styles->end(); ++i) { On 2013/03/22 17:33:30, kalman wrote: > super nit: for loop style is to align vertically, not 4 spaces, i.e. > > for (std::vector... > ::iterator...) > > not > > for (std::vector, > ::iterator) Done. https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:277: int type_class, offset, length; On 2013/03/22 17:33:30, kalman wrote: > just declare them when they're initialized. Done. https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:279: if ((*i).get() && (*i)->length) On 2013/03/22 17:33:30, kalman wrote: > please save a reference to *i (or better: the raw pointer underlying it), it's > pretty hard to read. Done. https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:286: offset = std::max(0, static_cast<int>(description.length()) + offset); On 2013/03/22 17:33:30, kalman wrote: > if you make this a size_t and ternary-initialize it then you wouldn't need the > static_cast below. Done. https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/api/omnibox/omnibox_unittest.cc (right): https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_unittest.cc:43: ListValue* list = ListBuilder().Append(42).Append(ListBuilder().Append( On 2013/03/22 17:33:30, kalman wrote: > looks like a memory leak? you're releasing this (at the end) but nothing seems > to take ownership of it. leave it as a scoped_ptr here. Done. https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_unittest.cc:54: .Set("length", 3))))).Build().release(); On 2013/03/22 17:33:30, kalman wrote: > using some indentation here would be nice to make it a bit easier to follow what > data you're creating. > > e.g. > > scoped_ptr<ListValue> list = ListBuilder() > .Append(42) > .Append(ListBuilder() > .Append(DictionaryBuilder() > .Set(content) > .Set(description) > .Set(descriptionStyles, ListBuilder() > .Append(DictionaryBuilder()) > .Set(..) > .Set(..) > > > etc. Done. https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_unittest.cc:71: ListValue* swap_list = ListBuilder().Append(42).Append(ListBuilder().Append( On 2013/03/22 17:33:30, kalman wrote: > same comments Done. https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_unittest.cc:100: ListValue* list = ListBuilder().Append(42).Append(ListBuilder().Append( On 2013/03/22 17:33:30, kalman wrote: > same comments Done. https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_unittest.cc:142: ListValue* moved_list = ListBuilder().Append(42).Append(ListBuilder().Append( On 2013/03/22 17:33:30, kalman wrote: > same comments Done. https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_unittest.cc:183: ListValue* list = ListBuilder().Append(42).Append(ListBuilder().Append( On 2013/03/22 17:33:30, kalman wrote: > same comments Done. https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/extension_prefs.cc (right): https://codereview.chromium.org/12314164/diff/24001/chrome/browser/extensions... chrome/browser/extensions/extension_prefs.cc:1163: scoped_ptr<base::DictionaryValue> dict = suggestion.ToValue(); On 2013/03/22 17:33:30, kalman wrote: > I guess if you change omnibox.json you'd need to delete content from the dict > here. Done. https://codereview.chromium.org/12314164/diff/24001/chrome/common/extensions/... File chrome/common/extensions/api/omnibox.json (right): https://codereview.chromium.org/12314164/diff/24001/chrome/common/extensions/... chrome/common/extensions/api/omnibox.json:16: "optional": true, On 2013/03/22 17:33:30, kalman wrote: > ugh, ok, I change my mind here sorry. this being optional is introducing some > complexity in the code that is a bit worrying (needing to check for null etc). > > let's make this required again, but modify omnibox_custom_bindings.js to put it > in for setDefaultSuggestions: something like > https://codereview.chromium.org/12663022 should work (haven't tested it). Done.
lgtm https://codereview.chromium.org/12314164/diff/37001/chrome/browser/extensions... File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right): https://codereview.chromium.org/12314164/diff/37001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:276: omnibox::SuggestResult::DescriptionStylesType* itr = i->get(); "itr" could have a nicer name. How about just "style" or "description_style". https://codereview.chromium.org/12314164/diff/37001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:279: if (itr && itr->length) doesn't seem possible for itr to be null here. https://codereview.chromium.org/12314164/diff/37001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:281: if (length < 0) this seems buggy, since length may not have been initialized. should this all be int length = description.length(); if (itr->length) length = *itr->length; https://codereview.chromium.org/12314164/diff/37001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:285: std::max(0, static_cast<int>(description.length()) + itr->offset); i'm surprised that you need the static_cast<int> here but... these things are typically a mystery to me. also some compilers might want you do static_cast<size_t> around the itr->offset, let's see. https://codereview.chromium.org/12314164/diff/37001/chrome/browser/extensions... File chrome/browser/extensions/extension_prefs.cc (right): https://codereview.chromium.org/12314164/diff/37001/chrome/browser/extensions... chrome/browser/extensions/extension_prefs.cc:1165: dict->SetString("content", ""); dict->Remove("content")?
https://codereview.chromium.org/12314164/diff/37001/chrome/browser/extensions... File chrome/browser/extensions/api/omnibox/omnibox_api.cc (right): https://codereview.chromium.org/12314164/diff/37001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:276: omnibox::SuggestResult::DescriptionStylesType* itr = i->get(); On 2013/03/22 23:30:32, kalman wrote: > "itr" could have a nicer name. How about just "style" or "description_style". Done. https://codereview.chromium.org/12314164/diff/37001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:279: if (itr && itr->length) On 2013/03/22 23:30:32, kalman wrote: > doesn't seem possible for itr to be null here. Done. https://codereview.chromium.org/12314164/diff/37001/chrome/browser/extensions... chrome/browser/extensions/api/omnibox/omnibox_api.cc:281: if (length < 0) On 2013/03/22 23:30:32, kalman wrote: > this seems buggy, since length may not have been initialized. should this all be > > int length = description.length(); > if (itr->length) > length = *itr->length; Done. https://codereview.chromium.org/12314164/diff/37001/chrome/browser/extensions... File chrome/browser/extensions/extension_prefs.cc (right): https://codereview.chromium.org/12314164/diff/37001/chrome/browser/extensions... chrome/browser/extensions/extension_prefs.cc:1165: dict->SetString("content", ""); On 2013/03/22 23:30:32, kalman wrote: > dict->Remove("content")? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/Samusaaron3@gmail.com/12314164/45001
Presubmit check for 12314164-45001 failed and returned exit status 1.
INFO:root:Found 9 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/common/extensions/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
chrome/browser/autocomplete/keyword_provider.cc
chrome/common/chrome_notification_types.h
Presubmit checks took 5.6s to calculate.
When you specify multiple reviewers, you need to say what each one is reviewing. Also, when you send out a ping email after review has already been ongoing, it needs to say who and why you're pinging, not be blank.
Thanks for the info, Peter. Peter: owners for chrome/browser/autocomplete/keyword_provider.cc Nico: owners for chrome/common/chrome_notification_types.h
chrome/common/chrome_notification_types.h lgtm
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/Samusaaron3@gmail.com/12314164/70001
Sorry for I got bad news for ya. Compile failed with a clobber build on ios_rel_device. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/Samusaaron3@gmail.com/12314164/72004
Message was sent while issue was closed.
Change committed as 196628 |
