Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10809094:
Context Menus now uses the JSON Schema Compiler. (Closed)
|
Created:
8 years, 5 months ago by chebert Modified:
8 years, 4 months ago Reviewers:
not at google - send to devlin CC:
chromium-reviews, Aaron Boodman, mihaip-chromium-reviews_chromium.org, pam+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@json_functions_as_properties Visibility:
Public. |
DescriptionContext Menus now uses the JSON Schema Compiler.
BUG=121174
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151595
Patch Set 1 #Patch Set 2 : code cleanup #Patch Set 3 : adjust to new json_function_as_parameters mods #
Total comments: 34
Patch Set 4 : switchy switch switch switch switch switch. switchy switch. #
Total comments: 20
Patch Set 5 : templates. #
Total comments: 22
Patch Set 6 : convergence #Patch Set 7 : should pass browser_tests now #Patch Set 8 : * #
Total comments: 23
Patch Set 9 : Parse*() become Get*(), and const s have been added where appropriate #Patch Set 10 : added unittests for Populate() and PopulateURLPatterns() #Patch Set 11 : renamed unittest in url_pattern_set_unittest #
Total comments: 15
Patch Set 12 : made some minor changes #
Total comments: 8
Patch Set 13 : GetParentId returns a scoped_ptr instead of a bool #Messages
Total messages: 21 (0 generated)
This one is still dependent on the https://chromiumcodereview.appspot.com/10824002/ (JSON Schema Compiler supports functions as PropertyTypes.) But I think it's ready for review as soon as that goes in.
Generally if you're doing something which depends on the value of an enum, use switch. Even if there are only a couple of conditions. Also... I think some of the cleanup can be propagated to MenuItem. E.g. that old PopulateURLPatterns method which takes a DictionaryValue can I think be entirely deleted. http://codereview.chromium.org/10809094/diff/5001/chrome/browser/extensions/a... File chrome/browser/extensions/api/context_menu/context_menu_api.cc (right): http://codereview.chromium.org/10809094/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/context_menu/context_menu_api.cc:31: const char kTypeKey[] = "type"; see how many of these constants you can delete. http://codereview.chromium.org/10809094/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/context_menu/context_menu_api.cc:61: namespace Update = extensions::api::context_menus::Update; move these inside the extensions namespace, then you don't need the extensions:: out the front of every one. http://codereview.chromium.org/10809094/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/context_menu/context_menu_api.cc:80: EXTENSION_FUNCTION_VALIDATE(properties != NULL); this will never be NULL because properties wasn't initialized to NULL. Check is unnecessary anyway (and properties should be initialized to NULL). It's a pity to do all the Json schema compiler stuff but still have to validate the dictionary. Ah well. You *could* add an optional generated_id to context_menus.json but put "nodoc" on it. I kinda like that... http://codereview.chromium.org/10809094/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/context_menu/context_menu_api.cc:107: params->create_properties.contexts->at(i); switch plz. http://codereview.chromium.org/10809094/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/context_menu/context_menu_api.cc:155: } switch here too http://codereview.chromium.org/10809094/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/context_menu/context_menu_api.cc:173: scoped_ptr<ListValue> document_url_patterns; This is just converting back to the typeless data for MenuItem to parse it again. Kinda defeats the purpose of JSON Schema Compiler. Changing MenuItem to take the vectors directly rather than ListValues will clean this up a lot. http://codereview.chromium.org/10809094/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/context_menu/context_menu_api.cc:199: if (params->create_properties.parent_id_type != can this be switched over too? This looks very boilerplate-y. http://codereview.chromium.org/10809094/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/context_menu/context_menu_api.cc:241: item_id.uid = *params->id_integer; switch http://codereview.chromium.org/10809094/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/context_menu/context_menu_api.cc:254: Update::Params::UpdateProperties::TYPE_NONE) { switch http://codereview.chromium.org/10809094/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/context_menu/context_menu_api.cc:290: checked = *params->update_properties.checked; combine with line above? http://codereview.chromium.org/10809094/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/context_menu/context_menu_api.cc:313: Update::Params::UpdateProperties::ContextsElement context = switch. Didn't we talk about pulling the enum into a type, then being able to share all the enums? We could pull all this switch stuff into a helper method then. http://codereview.chromium.org/10809094/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/context_menu/context_menu_api.cc:349: if (params->update_properties.parent_id_type != switch or something http://codereview.chromium.org/10809094/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/context_menu/context_menu_api.cc:376: if (params->update_properties.document_url_patterns.get()) { same deal with passing through the vectors http://codereview.chromium.org/10809094/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/context_menu/context_menu_api.cc:416: else if (params->menu_item_id_type == Remove::Params::MENU_ITEM_ID_INTEGER) switch http://codereview.chromium.org/10809094/diff/5001/chrome/browser/extensions/a... File chrome/browser/extensions/api/context_menu/context_menu_api.h (left): http://codereview.chromium.org/10809094/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/context_menu/context_menu_api.h:22: virtual ~ExtensionContextMenuFunction() {} woo! http://codereview.chromium.org/10809094/diff/5001/chrome/browser/extensions/m... File chrome/browser/extensions/menu_manager.cc (right): http://codereview.chromium.org/10809094/diff/5001/chrome/browser/extensions/m... chrome/browser/extensions/menu_manager.cc:273: std::string* error) { When you convert these to taking vectors, you could either call AddPattern directly, or add an alternative Populate method to URLPatternSet. I prefer the latter. http://codereview.chromium.org/10809094/diff/5001/chrome/browser/extensions/m... File chrome/browser/extensions/menu_manager.h (right): http://codereview.chromium.org/10809094/diff/5001/chrome/browser/extensions/m... chrome/browser/extensions/menu_manager.h:189: ListValue* target_url_patterns, Just take const std::vector<std::string>* directly?
switched https://chromiumcodereview.appspot.com/10809094/diff/5001/chrome/browser/exte... File chrome/browser/extensions/api/context_menu/context_menu_api.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/5001/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:31: const char kTypeKey[] = "type"; On 2012/07/26 04:16:20, kalman wrote: > see how many of these constants you can delete. Done. https://chromiumcodereview.appspot.com/10809094/diff/5001/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:61: namespace Update = extensions::api::context_menus::Update; On 2012/07/26 04:16:20, kalman wrote: > move these inside the extensions namespace, then you don't need the extensions:: > out the front of every one. Done. https://chromiumcodereview.appspot.com/10809094/diff/5001/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:80: EXTENSION_FUNCTION_VALIDATE(properties != NULL); This gets generated in the custom_bindings which adds the id to the dictionary. would that work to just put a generated_id into the contexts? On 2012/07/26 04:16:20, kalman wrote: > this will never be NULL because properties wasn't initialized to NULL. Check is > unnecessary anyway (and properties should be initialized to NULL). > > It's a pity to do all the Json schema compiler stuff but still have to validate > the dictionary. Ah well. > > You *could* add an optional generated_id to context_menus.json but put "nodoc" > on it. I kinda like that... https://chromiumcodereview.appspot.com/10809094/diff/5001/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:107: params->create_properties.contexts->at(i); On 2012/07/26 04:16:20, kalman wrote: > switch plz. Done. https://chromiumcodereview.appspot.com/10809094/diff/5001/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:155: } On 2012/07/26 04:16:20, kalman wrote: > switch here too Done. https://chromiumcodereview.appspot.com/10809094/diff/5001/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:173: scoped_ptr<ListValue> document_url_patterns; On 2012/07/26 04:16:20, kalman wrote: > This is just converting back to the typeless data for MenuItem to parse it > again. Kinda defeats the purpose of JSON Schema Compiler. > > Changing MenuItem to take the vectors directly rather than ListValues will clean > this up a lot. Done. https://chromiumcodereview.appspot.com/10809094/diff/5001/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:199: if (params->create_properties.parent_id_type != Switchy... On 2012/07/26 04:16:20, kalman wrote: > can this be switched over too? > > This looks very boilerplate-y. https://chromiumcodereview.appspot.com/10809094/diff/5001/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:241: item_id.uid = *params->id_integer; On 2012/07/26 04:16:20, kalman wrote: > switch Done. https://chromiumcodereview.appspot.com/10809094/diff/5001/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:254: Update::Params::UpdateProperties::TYPE_NONE) { On 2012/07/26 04:16:20, kalman wrote: > switch Done. https://chromiumcodereview.appspot.com/10809094/diff/5001/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:290: checked = *params->update_properties.checked; On 2012/07/26 04:16:20, kalman wrote: > combine with line above? Done. https://chromiumcodereview.appspot.com/10809094/diff/5001/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:313: Update::Params::UpdateProperties::ContextsElement context = I want to do that, but I looked into that, and I think it is more than I have time for right now. In order to do that I would need to change: the json schema compiler all the .json api files all the functions that currently use enums On 2012/07/26 04:16:20, kalman wrote: > switch. > > Didn't we talk about pulling the enum into a type, then being able to share all > the enums? We could pull all this switch stuff into a helper method then. https://chromiumcodereview.appspot.com/10809094/diff/5001/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:349: if (params->update_properties.parent_id_type != On 2012/07/26 04:16:20, kalman wrote: > switch or something Done. https://chromiumcodereview.appspot.com/10809094/diff/5001/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:376: if (params->update_properties.document_url_patterns.get()) { On 2012/07/26 04:16:20, kalman wrote: > same deal with passing through the vectors Done. https://chromiumcodereview.appspot.com/10809094/diff/5001/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:416: else if (params->menu_item_id_type == Remove::Params::MENU_ITEM_ID_INTEGER) On 2012/07/26 04:16:20, kalman wrote: > switch Done. https://chromiumcodereview.appspot.com/10809094/diff/5001/chrome/browser/exte... File chrome/browser/extensions/menu_manager.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/5001/chrome/browser/exte... chrome/browser/extensions/menu_manager.cc:273: std::string* error) { On 2012/07/26 04:16:20, kalman wrote: > When you convert these to taking vectors, you could either call AddPattern > directly, or add an alternative Populate method to URLPatternSet. I prefer the > latter. Done. https://chromiumcodereview.appspot.com/10809094/diff/5001/chrome/browser/exte... File chrome/browser/extensions/menu_manager.h (right): https://chromiumcodereview.appspot.com/10809094/diff/5001/chrome/browser/exte... chrome/browser/extensions/menu_manager.h:189: ListValue* target_url_patterns, On 2012/07/26 04:16:20, kalman wrote: > Just take const std::vector<std::string>* directly? Done.
looking better. still looks kinda boilerplatey; the goal of using JSC is to cut down on boilerplate (as well as type safety of course), but this is an overall increase in line count; we should really try to avoid that. https://chromiumcodereview.appspot.com/10809094/diff/5001/chrome/browser/exte... File chrome/browser/extensions/api/context_menu/context_menu_api.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/5001/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:80: EXTENSION_FUNCTION_VALIDATE(properties != NULL); On 2012/07/27 01:28:24, chebert wrote: > This gets generated in the custom_bindings which adds the id to the dictionary. > would that work to just put a generated_id into the contexts? > > On 2012/07/26 04:16:20, kalman wrote: > > this will never be NULL because properties wasn't initialized to NULL. Check > is > > unnecessary anyway (and properties should be initialized to NULL). > > > > It's a pity to do all the Json schema compiler stuff but still have to > validate > > the dictionary. Ah well. > > > > You *could* add an optional generated_id to context_menus.json but put "nodoc" > > on it. I kinda like that... > Yeah that's what I meant, but I thought of something much better. I'll write it in a comment in the new version of the change. https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/browser/exte... File chrome/browser/extensions/api/context_menu/context_menu_api.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:69: &id.uid)); yep, ignore previous comment. this is fine. Could you add a comment though, like "generatedId is added by context_menus_custom_bindings.js". https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:90: MenuItem::ContextList contexts(MenuItem::PAGE); I had a go at seeing whether JSC can do this, but not without more work than I'm willing to do. However, I believe that C++ can, if you define a templated function for this parsing, something like: template <typename PropertyWithEnum> MenuItem::ContextList GetMenuItemEnum(PropertyWithEnum::ContextEnum value) { switch (value) { case PropertyWithEnum::CONTEXT_ELEMENTS_ALL: return MenuItem::ALL; ... } return MenuItem::PAGE; } (you will probably have to play a bit to make that compile) Then call that from here and below. https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:129: case Create::Params::CreateProperties::TYPE_NONE: same comment as above re enum https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:162: params->create_properties.target_url_patterns.get(), &error_)) { nit: &error_ on next line https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:211: item_id.string_uid = *params->id_string; more switch with template stuff https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:334: break; no point having this default branch https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/browser/exte... File chrome/browser/extensions/menu_manager.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/browser/exte... chrome/browser/extensions/menu_manager.cc:268: return true; ditto https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/browser/exte... File chrome/browser/extensions/menu_manager.h (right): https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/browser/exte... chrome/browser/extensions/menu_manager.h:186: const char* target_url_patterns_key, you should be able to delete this version of the method https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/common/exten... File chrome/common/extensions/url_pattern_set.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/common/exten... chrome/common/extensions/url_pattern_set.cc:227: } ditto. If we do, there is a lot of chance to share code here and above. https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/common/exten... File chrome/common/extensions/url_pattern_set.h (right): https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/common/exten... chrome/common/extensions/url_pattern_set.h:85: std::string* error); check whether we still need this function
total less 8 lines. https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/browser/exte... File chrome/browser/extensions/api/context_menu/context_menu_api.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:69: &id.uid)); On 2012/07/30 15:36:53, kalman wrote: > yep, ignore previous comment. this is fine. > > Could you add a comment though, like "generatedId is added by > context_menus_custom_bindings.js". Done. https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:90: MenuItem::ContextList contexts(MenuItem::PAGE); On 2012/07/30 15:36:53, kalman wrote: > I had a go at seeing whether JSC can do this, but not without more work than I'm > willing to do. However, I believe that C++ can, if you define a templated > function for this parsing, something like: > > template <typename PropertyWithEnum> > MenuItem::ContextList GetMenuItemEnum(PropertyWithEnum::ContextEnum value) { > switch (value) { > case PropertyWithEnum::CONTEXT_ELEMENTS_ALL: > return MenuItem::ALL; > ... > } > return MenuItem::PAGE; > } > > (you will probably have to play a bit to make that compile) > > Then call that from here and below. Done. https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:129: case Create::Params::CreateProperties::TYPE_NONE: On 2012/07/30 15:36:53, kalman wrote: > same comment as above re enum Done. https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:162: params->create_properties.target_url_patterns.get(), &error_)) { On 2012/07/30 15:36:53, kalman wrote: > nit: &error_ on next line Done. https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:211: item_id.string_uid = *params->id_string; The enums are named differently here, so a helper function probably won't help much. On 2012/07/30 15:36:53, kalman wrote: > more switch with template stuff https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:334: break; On 2012/07/30 15:36:53, kalman wrote: > no point having this default branch Done. https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/browser/exte... File chrome/browser/extensions/menu_manager.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/browser/exte... chrome/browser/extensions/menu_manager.cc:268: return true; see menu_manager.h On 2012/07/30 15:36:53, kalman wrote: > ditto https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/browser/exte... File chrome/browser/extensions/menu_manager.h (right): https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/browser/exte... chrome/browser/extensions/menu_manager.h:186: const char* target_url_patterns_key, This method is still being called in the Populate() method here. I traced it back a ways, and the MenuItem reads from storage into a DictionaryValue, so this is still needed, though I suppose I could convert to a vector of strings. On 2012/07/30 15:36:53, kalman wrote: > you should be able to delete this version of the method https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/common/exten... File chrome/common/extensions/url_pattern_set.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/common/exten... chrome/common/extensions/url_pattern_set.cc:227: } On 2012/07/30 15:36:53, kalman wrote: > ditto. > > If we do, there is a lot of chance to share code here and above. Done. https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/common/exten... File chrome/common/extensions/url_pattern_set.h (right): https://chromiumcodereview.appspot.com/10809094/diff/9002/chrome/common/exten... chrome/common/extensions/url_pattern_set.h:85: std::string* error); This is used in the PopulateURLPatterns method. See the comment in menu_manager.h On 2012/07/30 15:36:53, kalman wrote: > check whether we still need this function
almost done. looking good. https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/ext... File chrome/browser/extensions/api/context_menu/context_menu_api.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:47: PropertyWithEnumT& property) { pass const& not &. https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:85: extensions::MenuItem::Type ParseType(PropertyWithEnumT& property) { ditto const& https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:102: PropertyWithEnumT& property) { gitto https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:116: extensions::MenuItem* GetParent(extensions::MenuItem::Id& parent_id, this is just a primitive value, so don't need to pass a reference, just pass by value. https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:181: return false; I think this should be EXTENSION_FUNCTION_VALIDATE. https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:228: bool radioItemUpdated = false; mind changing this to radio_item_updated? https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:273: if (checked && item->type() != MenuItem::CHECKBOX && nit: multiple conditions that don't fit on one line should be like if (checked && item->type() != MenuItem::CHECKBOX && item->type() != MenuItem::RADIO) { } https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:294: ParseContexts(contexts, params->update_properties); ditto, EXTENSION_FUNCTION_VALIDATE. https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/ext... File chrome/browser/extensions/menu_manager.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/ext... chrome/browser/extensions/menu_manager.cc:268: return true; yeah: it'd be slightly less efficient, but easier to maintain: in the function above, convert the DictionaryValue into two vectors then pass into the one below. https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/syn... File chrome/browser/sync/test_profile_sync_service.cc (left): https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/syn... chrome/browser/sync/test_profile_sync_service.cc:108: bool send_passphrase_required = false; what's this change? https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/common/exte... File chrome/common/extensions/url_pattern_set.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/common/exte... chrome/common/extensions/url_pattern_set.cc:181: std::string* error) { ditto last comment. I think there's enough logic to justify making the other implementation of Populate just construct the vector.
https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/ext... File chrome/browser/extensions/api/context_menu/context_menu_api.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:47: PropertyWithEnumT& property) { On 2012/07/31 06:53:09, kalman wrote: > pass const& not &. Done. https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:85: extensions::MenuItem::Type ParseType(PropertyWithEnumT& property) { On 2012/07/31 06:53:09, kalman wrote: > ditto const& Done. https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:102: PropertyWithEnumT& property) { On 2012/07/31 06:53:09, kalman wrote: > gitto Done. https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:116: extensions::MenuItem* GetParent(extensions::MenuItem::Id& parent_id, On 2012/07/31 06:53:09, kalman wrote: > this is just a primitive value, so don't need to pass a reference, just pass by > value. Done. https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:181: return false; On 2012/07/31 06:53:09, kalman wrote: > I think this should be EXTENSION_FUNCTION_VALIDATE. Done. https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:228: bool radioItemUpdated = false; On 2012/07/31 06:53:09, kalman wrote: > mind changing this to radio_item_updated? Done. https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:273: if (checked && item->type() != MenuItem::CHECKBOX && On 2012/07/31 06:53:09, kalman wrote: > nit: multiple conditions that don't fit on one line should be like > > if (checked && > item->type() != MenuItem::CHECKBOX && > item->type() != MenuItem::RADIO) { > } Done. https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:294: ParseContexts(contexts, params->update_properties); On 2012/07/31 06:53:09, kalman wrote: > ditto, EXTENSION_FUNCTION_VALIDATE. Done. https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/ext... File chrome/browser/extensions/menu_manager.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/ext... chrome/browser/extensions/menu_manager.cc:268: return true; On 2012/07/31 06:53:09, kalman wrote: > yeah: it'd be slightly less efficient, but easier to maintain: in the function > above, convert the DictionaryValue into two vectors then pass into the one > below. Done. https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/syn... File chrome/browser/sync/test_profile_sync_service.cc (left): https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/browser/syn... chrome/browser/sync/test_profile_sync_service.cc:108: bool send_passphrase_required = false; Whoops. This was so unittests would compile (this var doesn't do anything). Deleting it. On 2012/07/31 06:53:09, kalman wrote: > what's this change? https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/common/exte... File chrome/common/extensions/url_pattern_set.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/14001/chrome/common/exte... chrome/common/extensions/url_pattern_set.cc:181: std::string* error) { On 2012/07/31 06:53:09, kalman wrote: > ditto last comment. I think there's enough logic to justify making the other > implementation of Populate just construct the vector. Done.
lg but there are those style issues to fix before submitting. http://codereview.chromium.org/10809094/diff/11006/chrome/browser/extensions/... File chrome/browser/extensions/api/context_menu/context_menu_api.cc (right): http://codereview.chromium.org/10809094/diff/11006/chrome/browser/extensions/... chrome/browser/extensions/api/context_menu/context_menu_api.cc:46: bool ParseContexts(extensions::MenuItem::ContextList& contexts, Don't pass non-const refs; this should be const& http://codereview.chromium.org/10809094/diff/11006/chrome/browser/extensions/... chrome/browser/extensions/api/context_menu/context_menu_api.cc:47: PropertyWithEnumT& property) { Ditto non-const refs; if you need to pass an out-param, use a pointer. also nit: one more space indent http://codereview.chromium.org/10809094/diff/11006/chrome/browser/extensions/... chrome/browser/extensions/api/context_menu/context_menu_api.cc:85: extensions::MenuItem::Type ParseType(PropertyWithEnumT& property) { ditto non-const refs (use pointer) http://codereview.chromium.org/10809094/diff/11006/chrome/browser/extensions/... chrome/browser/extensions/api/context_menu/context_menu_api.cc:97: return extensions::MenuItem::NORMAL; All other methods have out-params; this one just returns its result. I generally prefer methods that just return their results if possible... and all of these values come from json-schema-compiler so if they're wrong them something else is wrong. So I'm saying that, if possible, change all the methods to return their results. If not then out-params fine I guess, but we should at least be consistent. http://codereview.chromium.org/10809094/diff/11006/chrome/browser/extensions/... chrome/browser/extensions/api/context_menu/context_menu_api.cc:101: bool ParseParentId(extensions::MenuItem::Id& parent_id, pass pointer, and put argument at end; out-params should go at the end of the parameter list. http://codereview.chromium.org/10809094/diff/11006/chrome/browser/extensions/... chrome/browser/extensions/api/context_menu/context_menu_api.cc:102: PropertyWithEnumT& property) { pass const& http://codereview.chromium.org/10809094/diff/11006/chrome/browser/extensions/... chrome/browser/extensions/api/context_menu/context_menu_api.cc:113: return true; why returning true, I would have expected this to be a failure? I.e. I think the breaks should be "return true" and the fall-through case should be "return false"? http://codereview.chromium.org/10809094/diff/11006/chrome/browser/extensions/... chrome/browser/extensions/api/context_menu/context_menu_api.cc:181: return false; nit: too much indent http://codereview.chromium.org/10809094/diff/11006/chrome/browser/extensions/... chrome/browser/extensions/api/context_menu/context_menu_api.cc:213: if (ParseParentId(parent_id, params->create_properties)) { This looks like "if it succeed parsing", but it will always parse (since json-schema-compiler has already parsed it), so could you rename it to "GetParentId". Ditto all those other Parse functions. http://codereview.chromium.org/10809094/diff/11006/chrome/browser/extensions/... File chrome/browser/extensions/menu_manager.cc (right): http://codereview.chromium.org/10809094/diff/11006/chrome/browser/extensions/... chrome/browser/extensions/menu_manager.cc:266: } plz just convert these to vectors and pass in PopulateURLParameters. Minimise code duplication and you minimise the chances of subtle errors. http://codereview.chromium.org/10809094/diff/11006/chrome/common/extensions/u... File chrome/common/extensions/url_pattern_set.cc (right): http://codereview.chromium.org/10809094/diff/11006/chrome/common/extensions/u... chrome/common/extensions/url_pattern_set.cc:201: URLPattern pattern(valid_schemes); nice, though also just convert this to a vector and call the vector version of Patterns.
This is embarrassing. I definitely made all of the suggested changes in another git branch, but it didn't pass the trybots, so I reverted to an older change to figure out what the problem was. I guess I accidentally uploaded that patch instead of the correct one. Sorry. =[ https://chromiumcodereview.appspot.com/10809094/diff/11006/chrome/browser/ext... File chrome/browser/extensions/menu_manager.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/11006/chrome/browser/ext... chrome/browser/extensions/menu_manager.cc:266: } Umm. Sorry I think I patched from the wrong branch or something, because I spent some quite a bit of time doing this. This is embarrassing. On 2012/08/06 01:55:45, kalman wrote: > plz just convert these to vectors and pass in PopulateURLParameters. Minimise > code duplication and you minimise the chances of subtle errors.
https://chromiumcodereview.appspot.com/10809094/diff/11006/chrome/browser/ext... File chrome/browser/extensions/menu_manager.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/11006/chrome/browser/ext... chrome/browser/extensions/menu_manager.cc:266: } On 2012/08/06 03:38:04, chebert wrote: > Umm. Sorry I think I patched from the wrong branch or something, because I spent > some quite a bit of time doing this. > > This is embarrassing. > > On 2012/08/06 01:55:45, kalman wrote: > > plz just convert these to vectors and pass in PopulateURLParameters. Minimise > > code duplication and you minimise the chances of subtle errors. > No probs :)
Double-checked...I uploaded the right patch this time. https://chromiumcodereview.appspot.com/10809094/diff/11006/chrome/browser/ext... File chrome/browser/extensions/api/context_menu/context_menu_api.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/11006/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:46: bool ParseContexts(extensions::MenuItem::ContextList& contexts, On 2012/08/06 01:55:45, kalman wrote: > Don't pass non-const refs; this should be const& Done. https://chromiumcodereview.appspot.com/10809094/diff/11006/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:47: PropertyWithEnumT& property) { On 2012/08/06 01:55:45, kalman wrote: > Ditto non-const refs; if you need to pass an out-param, use a pointer. > > also nit: one more space indent Done. https://chromiumcodereview.appspot.com/10809094/diff/11006/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:85: extensions::MenuItem::Type ParseType(PropertyWithEnumT& property) { On 2012/08/06 01:55:45, kalman wrote: > ditto non-const refs (use pointer) Done. https://chromiumcodereview.appspot.com/10809094/diff/11006/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:97: return extensions::MenuItem::NORMAL; On 2012/08/06 01:55:45, kalman wrote: > All other methods have out-params; this one just returns its result. I generally > prefer methods that just return their results if possible... and all of these > values come from json-schema-compiler so if they're wrong them something else is > wrong. > > So I'm saying that, if possible, change all the methods to return their results. > If not then out-params fine I guess, but we should at least be consistent. Done. https://chromiumcodereview.appspot.com/10809094/diff/11006/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:101: bool ParseParentId(extensions::MenuItem::Id& parent_id, On 2012/08/06 01:55:45, kalman wrote: > pass pointer, and put argument at end; out-params should go at the end of the > parameter list. Done. https://chromiumcodereview.appspot.com/10809094/diff/11006/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:102: PropertyWithEnumT& property) { On 2012/08/06 01:55:45, kalman wrote: > pass const& Done. https://chromiumcodereview.appspot.com/10809094/diff/11006/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:113: return true; On 2012/08/06 01:55:45, kalman wrote: > why returning true, I would have expected this to be a failure? I.e. I think the > breaks should be "return true" and the fall-through case should be "return > false"? Done. https://chromiumcodereview.appspot.com/10809094/diff/11006/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:181: return false; On 2012/08/06 01:55:45, kalman wrote: > nit: too much indent Done. https://chromiumcodereview.appspot.com/10809094/diff/11006/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:213: if (ParseParentId(parent_id, params->create_properties)) { On 2012/08/06 01:55:45, kalman wrote: > This looks like "if it succeed parsing", but it will always parse (since > json-schema-compiler has already parsed it), so could you rename it to > "GetParentId". > > Ditto all those other Parse functions. Done. https://chromiumcodereview.appspot.com/10809094/diff/11006/chrome/common/exte... File chrome/common/extensions/url_pattern_set.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/11006/chrome/common/exte... chrome/common/extensions/url_pattern_set.cc:201: URLPattern pattern(valid_schemes); On 2012/08/06 01:55:45, kalman wrote: > nice, though also just convert this to a vector and call the vector version of > Patterns. Done.
lgtm. Still some things to fix up but I don't think another review round-trip will be necessary. http://codereview.chromium.org/10809094/diff/7007/chrome/browser/extensions/a... File chrome/browser/extensions/api/context_menu/context_menu_api.cc (right): http://codereview.chromium.org/10809094/diff/7007/chrome/browser/extensions/a... chrome/browser/extensions/api/context_menu/context_menu_api.cc:47: PropertyWithEnumT const& property) { as in const PropertyWithEnumT& property const& is me being lazy and not typing out the type name too. http://codereview.chromium.org/10809094/diff/7007/chrome/browser/extensions/a... chrome/browser/extensions/api/context_menu/context_menu_api.cc:84: extensions::MenuItem::Type GetType(PropertyWithEnumT const& property) { ditto http://codereview.chromium.org/10809094/diff/7007/chrome/browser/extensions/a... chrome/browser/extensions/api/context_menu/context_menu_api.cc:100: bool GetParentId(PropertyWithEnumT const& property, ditto http://codereview.chromium.org/10809094/diff/7007/chrome/browser/extensions/a... chrome/browser/extensions/api/context_menu/context_menu_api.cc:112: return false; Change this method to return an extensions::MenuItem:Id, i.e. to look the same as GetType? The comment I made here was before the one about making things not have out-params :\ sorry, forgot to change it. http://codereview.chromium.org/10809094/diff/7007/chrome/browser/extensions/m... File chrome/browser/extensions/menu_manager.cc (right): http://codereview.chromium.org/10809094/diff/7007/chrome/browser/extensions/m... chrome/browser/extensions/menu_manager.cc:232: if (value.HasKey(kDocumentURLPatternsKey)) { code is basically repeated. Could you pull this out and make it look something like: namespace { bool GetStringList(const DictionaryValue& dict, const std::string& key, std::vector<std::string>* out) { if (!dict.HasKey(key)) return true; const ListValue* list = NULL; if (!dict.GetListWithoutPathExpansion(key, &list) return false; // etc } } // namespace then std::vector<std::string> document_url_patterns; std::vector<std::string> target_url_patterns; if (!GetStringList(value, kDocumentURLPatternsKey, &document_url_patterns) || !GetStringList(value, kTargetURLPatternsKey, &target_url_patterns)) { return NULL; } if (!result->PopulateURLPatterns(...)) { return NULL; } etc. http://codereview.chromium.org/10809094/diff/7007/chrome/browser/extensions/m... File chrome/browser/extensions/menu_manager_unittest.cc (right): http://codereview.chromium.org/10809094/diff/7007/chrome/browser/extensions/m... chrome/browser/extensions/menu_manager_unittest.cc:196: bool incognito = true; have a look at using ValueBuilder (c/c/e/value_builder.h) if you'd like. It would make this a lot more concise and easy to read.
https://chromiumcodereview.appspot.com/10809094/diff/7007/chrome/browser/exte... File chrome/browser/extensions/api/context_menu/context_menu_api.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/7007/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:47: PropertyWithEnumT const& property) { On 2012/08/07 02:00:11, kalman wrote: > as in > > const PropertyWithEnumT& property > > const& is me being lazy and not typing out the type name too. Done. https://chromiumcodereview.appspot.com/10809094/diff/7007/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:84: extensions::MenuItem::Type GetType(PropertyWithEnumT const& property) { On 2012/08/07 02:00:11, kalman wrote: > ditto Done. https://chromiumcodereview.appspot.com/10809094/diff/7007/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:100: bool GetParentId(PropertyWithEnumT const& property, On 2012/08/07 02:00:11, kalman wrote: > ditto Done. https://chromiumcodereview.appspot.com/10809094/diff/7007/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:112: return false; I've left it this way because we check whether the property has a parent_id or not. On 2012/08/07 02:00:11, kalman wrote: > Change this method to return an extensions::MenuItem:Id, i.e. to look the same > as GetType? > > The comment I made here was before the one about making things not have > out-params :\ sorry, forgot to change it. https://chromiumcodereview.appspot.com/10809094/diff/7007/chrome/browser/exte... File chrome/browser/extensions/menu_manager.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/7007/chrome/browser/exte... chrome/browser/extensions/menu_manager.cc:232: if (value.HasKey(kDocumentURLPatternsKey)) { On 2012/08/07 02:00:11, kalman wrote: > code is basically repeated. Could you pull this out and make it look something > like: > > namespace { > bool GetStringList(const DictionaryValue& dict, > const std::string& key, > std::vector<std::string>* out) { > if (!dict.HasKey(key)) > return true; > const ListValue* list = NULL; > if (!dict.GetListWithoutPathExpansion(key, &list) > return false; > // etc > } > } // namespace > > then > > std::vector<std::string> document_url_patterns; > std::vector<std::string> target_url_patterns; > if (!GetStringList(value, kDocumentURLPatternsKey, &document_url_patterns) || > !GetStringList(value, kTargetURLPatternsKey, &target_url_patterns)) { > return NULL; > } > > if (!result->PopulateURLPatterns(...)) { > return NULL; > } > > etc. Done. https://chromiumcodereview.appspot.com/10809094/diff/7007/chrome/browser/exte... File chrome/browser/extensions/menu_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/7007/chrome/browser/exte... chrome/browser/extensions/menu_manager_unittest.cc:196: bool incognito = true; I don't think it would be more concise, since I use the list value in the EXPECT_EQ at the end. I did move the value to be set all in one place to make it more readable though. On 2012/08/07 02:00:11, kalman wrote: > have a look at using ValueBuilder (c/c/e/value_builder.h) if you'd like. It > would make this a lot more concise and easy to read.
still lgtm http://codereview.chromium.org/10809094/diff/7007/chrome/browser/extensions/a... File chrome/browser/extensions/api/context_menu/context_menu_api.cc (right): http://codereview.chromium.org/10809094/diff/7007/chrome/browser/extensions/a... chrome/browser/extensions/api/context_menu/context_menu_api.cc:112: return false; On 2012/08/13 22:41:52, chebert wrote: > I've left it this way because we check whether the property has a parent_id or > not. > > On 2012/08/07 02:00:11, kalman wrote: > > Change this method to return an extensions::MenuItem:Id, i.e. to look the same > > as GetType? > > > > The comment I made here was before the one about making things not have > > out-params :\ sorry, forgot to change it. > Consider returning a scoped_ptr<MenuItem::Id> then? On failure return a NULL scoped_ptr. http://codereview.chromium.org/10809094/diff/13007/chrome/browser/extensions/... File chrome/browser/extensions/api/context_menu/context_menu_api.cc (right): http://codereview.chromium.org/10809094/diff/13007/chrome/browser/extensions/... chrome/browser/extensions/api/context_menu/context_menu_api.cc:217: &parent_id); nit: style should align arguments vertically, so bool got_parent_id = GetParentId(params->create_properties, profile()->IsOffTheRecord(), extension_id(), &parent_id); same below. http://codereview.chromium.org/10809094/diff/13007/chrome/browser/extensions/... File chrome/browser/extensions/menu_manager_unittest.cc (right): http://codereview.chromium.org/10809094/diff/13007/chrome/browser/extensions/... chrome/browser/extensions/menu_manager_unittest.cc:446: base::ListValue* event_args, I don't know how mocks are supposed to work, but I'm surprised to see this as base::ListValue* rather than scoped_ptr<ListValue>. http://codereview.chromium.org/10809094/diff/13007/chrome/browser/extensions/... chrome/browser/extensions/menu_manager_unittest.cc:602: delete list; make list a scoped_ptr so that you don't need to call delete.
https://chromiumcodereview.appspot.com/10809094/diff/7007/chrome/browser/exte... File chrome/browser/extensions/api/context_menu/context_menu_api.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/7007/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:112: return false; On 2012/08/14 05:12:21, kalman wrote: > On 2012/08/13 22:41:52, chebert wrote: > > I've left it this way because we check whether the property has a parent_id or > > not. > > > > On 2012/08/07 02:00:11, kalman wrote: > > > Change this method to return an extensions::MenuItem:Id, i.e. to look the > same > > > as GetType? > > > > > > The comment I made here was before the one about making things not have > > > out-params :\ sorry, forgot to change it. > > > > Consider returning a scoped_ptr<MenuItem::Id> then? On failure return a NULL > scoped_ptr. Done. https://chromiumcodereview.appspot.com/10809094/diff/7007/chrome/browser/exte... chrome/browser/extensions/api/context_menu/context_menu_api.cc:112: return false; On 2012/08/14 05:12:21, kalman wrote: > On 2012/08/13 22:41:52, chebert wrote: > > I've left it this way because we check whether the property has a parent_id or > > not. > > > > On 2012/08/07 02:00:11, kalman wrote: > > > Change this method to return an extensions::MenuItem:Id, i.e. to look the > same > > > as GetType? > > > > > > The comment I made here was before the one about making things not have > > > out-params :\ sorry, forgot to change it. > > > > Consider returning a scoped_ptr<MenuItem::Id> then? On failure return a NULL > scoped_ptr. Done. https://chromiumcodereview.appspot.com/10809094/diff/13007/chrome/browser/ext... File chrome/browser/extensions/api/context_menu/context_menu_api.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/13007/chrome/browser/ext... chrome/browser/extensions/api/context_menu/context_menu_api.cc:217: &parent_id); On 2012/08/14 05:12:21, kalman wrote: > nit: style should align arguments vertically, so > > bool got_parent_id = GetParentId(params->create_properties, > profile()->IsOffTheRecord(), > extension_id(), > &parent_id); > > same below. Done. https://chromiumcodereview.appspot.com/10809094/diff/13007/chrome/browser/ext... File chrome/browser/extensions/menu_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/13007/chrome/browser/ext... chrome/browser/extensions/menu_manager_unittest.cc:446: base::ListValue* event_args, I think that this is a mock specific thing, since the "DispatchEventToExtension" takes a scoped_ptr<ListValue> and releases it to this method. On 2012/08/14 05:12:21, kalman wrote: > I don't know how mocks are supposed to work, but I'm surprised to see this as > base::ListValue* rather than scoped_ptr<ListValue>. https://chromiumcodereview.appspot.com/10809094/diff/13007/chrome/browser/ext... chrome/browser/extensions/menu_manager_unittest.cc:602: delete list; This is another mock specific thing. The address of the list pointer needs to be passed to the Mock for the assertion. Line 563: .WillOnce(SaveArg<2>(&list)); On 2012/08/14 05:12:21, kalman wrote: > make list a scoped_ptr so that you don't need to call delete.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hebert.christopherj@chromium.org/10809...
Try job failure for 10809094-16013 (retry) on win_rel for step "runhooks". It's a second try, previously, step "interactive_ui_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hebert.christopherj@chromium.org/10809...
https://chromiumcodereview.appspot.com/10809094/diff/13007/chrome/browser/ext... File chrome/browser/extensions/menu_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/13007/chrome/browser/ext... chrome/browser/extensions/menu_manager_unittest.cc:602: delete list; On 2012/08/14 18:56:53, chebert wrote: > This is another mock specific thing. > The address of the list pointer needs to be passed to the Mock for the > assertion. > Line 563: > .WillOnce(SaveArg<2>(&list)); > > On 2012/08/14 05:12:21, kalman wrote: > > make list a scoped_ptr so that you don't need to call delete. > You can't do .WillOnce(SaveArg<2>(list.get())) there, if list is a scoped ptr?
https://chromiumcodereview.appspot.com/10809094/diff/13007/chrome/browser/ext... File chrome/browser/extensions/menu_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/10809094/diff/13007/chrome/browser/ext... chrome/browser/extensions/menu_manager_unittest.cc:602: delete list; it would need to be .WillOnce(SaveArg<2>(&(list.get())) and you can't get the address of an RValue. On 2012/08/14 22:04:49, kalman wrote: > On 2012/08/14 18:56:53, chebert wrote: > > This is another mock specific thing. > > The address of the list pointer needs to be passed to the Mock for the > > assertion. > > Line 563: > > .WillOnce(SaveArg<2>(&list)); > > > > On 2012/08/14 05:12:21, kalman wrote: > > > make list a scoped_ptr so that you don't need to call delete. > > > > You can't do > > .WillOnce(SaveArg<2>(list.get())) there, if list is a scoped ptr?
Change committed as 151595 |
