Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9424009:
Cleaning up Extension::InitFromValue() (Closed)
|
Created:
8 years, 10 months ago by Devlin Modified:
8 years, 9 months ago Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionCleaning up Extension::InitFromValue()
This reduces the size of the Extension::InitFromValue() function from 1009 lines
of code to around 80 lines of code through the creation of helper functions.
There are four main helper functions - LoadRequiredFeatures, LoadSharedFeatures,LoadAppFeatures, and LoadExtensionFeatures.
BUG=104096
TEST=Run existing extension tests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=125823
Patch Set 1 #Patch Set 2 : Updated for the most recent versions of chrome/common/extensions/extension.* #
Total comments: 26
Patch Set 3 : Requested changed made #
Total comments: 14
Patch Set 4 : Update for most recent tree; requested changes made #
Total comments: 3
Patch Set 5 : Final update with requested changes, latest versions of extension.* #
Messages
Total messages: 14 (0 generated)
Refactoring of Extension::InitFromValue(). As of 2/16, this passes trybots mac_rel, win_rel, and linux_rel. As always, any feedback is much appreciated.
Latest version of the refactor of Extension::InitFromValue(). Any feedback is appreciated.
Yoyo, can you take this review? http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... File chrome/common/extensions/extension.cc (right): http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:899: if (manifest_->HasKey(keys::kDescription) && !LoadDescription(error)) If you put the check for the presence of the key in LoadDescription, you centralize knowledge of the key name in that function. Also, it allows you do things like: if (!LoadName(error) || !LoadDescription(error) || !LoadVersion(error) ...) { return false; } http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:960: &minimum_version_string)) { +1 indent
http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... File chrome/common/extensions/extension.cc (right): http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:908: !LoadMinimumChromeVersion(error)) Maybe call this CheckMinimumChromeVersion instead, since it's not stored (per the comment). http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:1100: if (manifest_->HasKey(keys::kOptionsPage) && As Aaron wrote, you can check for the presence of the keys in the helper functions. LoadAppIsolation shows an example of this. http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:1101: !LoadOptionsPage(error)) unfortunately these aren't all exclusively extension features (e.g. options page and icons). Maybe a better way to break this down would be to move the call to LoadSharedFeatures after the permissions parsing (which looks okay, but please check the dependencies) and put these into LoadSharedFeatures. If you do this, at least the minimum Chrome version check should come earlier (maybe it can be top-level in InitFromValue). chrome/common/extensions/api/_manifest_features.json shows which features are extension-only; anything that's "all" or includes "hosted_app" should be considered 'shared'. (However, you still can't skip this function if is_app() because packaged apps are mostly just like extensions.) http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:1114: &converted_from_user_script_); indent +1 http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:1171: // Defaults to false. Comments like this seem like they should be in the helper function. http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:1177: incognito_split_mode_ = is_app(); Should be inside LoadIncognito, which I would rename LoadIncognitoMode. http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:2051: manifest_->Get(keys::kBackgroundPageLegacy, &background_page_value); leave the original indent http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:2148: // package this is. This comment belongs in InitValue (at this function's call site). http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:2765: GURL Extension::GetBaseURLFromExtensionId(const std::string& extension_id) { Put a // static before this. http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:2793: if (!LoadSharedFeatures(flags, error)) In this and in LoadExtensionFeatures and whatever they call, you can use creation_flags_ (which is set earlier) so you don't need to pass in flags. http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:2821: !LoadAppIsolation(error)) This used to have an is_app check. Ideally this would be in LoadAppFeatures, but I guess it requires the permissions to have been parsed. http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... File chrome/common/extensions/extension.h (right): http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.h:683: // extension. ...from the manifest
Next version is up, with all requested changes made. Thanks in advance. http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... File chrome/common/extensions/extension.cc (right): http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:899: if (manifest_->HasKey(keys::kDescription) && !LoadDescription(error)) On 2012/02/29 20:38:25, Aaron Boodman wrote: > If you put the check for the presence of the key in LoadDescription, you > centralize knowledge of the key name in that function. Also, it allows you do > things like: > > if (!LoadName(error) || > !LoadDescription(error) || > !LoadVersion(error) > ...) { > return false; > } Done. http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:908: !LoadMinimumChromeVersion(error)) On 2012/03/01 01:17:25, Yoyo Zhou wrote: > Maybe call this CheckMinimumChromeVersion instead, since it's not stored (per > the comment). Done. http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:960: &minimum_version_string)) { On 2012/02/29 20:38:25, Aaron Boodman wrote: > +1 indent Done. http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:1100: if (manifest_->HasKey(keys::kOptionsPage) && On 2012/03/01 01:17:25, Yoyo Zhou wrote: > As Aaron wrote, you can check for the presence of the keys in the helper > functions. LoadAppIsolation shows an example of this. Done. http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:1101: !LoadOptionsPage(error)) On 2012/03/01 01:17:25, Yoyo Zhou wrote: > unfortunately these aren't all exclusively extension features (e.g. options page > and icons). Maybe a better way to break this down would be to move the call to > LoadSharedFeatures after the permissions parsing (which looks okay, but please > check the dependencies) and put these into LoadSharedFeatures. If you do this, > at least the minimum Chrome version check should come earlier (maybe it can be > top-level in InitFromValue). > > chrome/common/extensions/api/_manifest_features.json shows which features are > extension-only; anything that's "all" or includes "hosted_app" should be > considered 'shared'. (However, you still can't skip this function if is_app() > because packaged apps are mostly just like extensions.) Thanks! A while back, Aaron listed the ones he thought were shared, but it looks like a few may have changed since then (the hazards of modifying a highly dynamic file). I appreciate the pointer to the .json; it made it all much easier. :) http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:1114: &converted_from_user_script_); On 2012/03/01 01:17:25, Yoyo Zhou wrote: > indent +1 Done. http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:1171: // Defaults to false. On 2012/03/01 01:17:25, Yoyo Zhou wrote: > Comments like this seem like they should be in the helper function. Done. http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:1177: incognito_split_mode_ = is_app(); On 2012/03/01 01:17:25, Yoyo Zhou wrote: > Should be inside LoadIncognito, which I would rename LoadIncognitoMode. Done. I was debating making it LoadIncognitoMode before, but I wanted it to match the name of the key; since the key knowledge is now centralized within the functions (per your and Aaron's suggestion), this is no longer a concern. http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:2051: manifest_->Get(keys::kBackgroundPageLegacy, &background_page_value); On 2012/03/01 01:17:25, Yoyo Zhou wrote: > leave the original indent Whoops, didn't mean to do that. Done. http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:2148: // package this is. On 2012/03/01 01:17:25, Yoyo Zhou wrote: > This comment belongs in InitValue (at this function's call site). Done. http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... File chrome/common/extensions/extension.h (right): http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.h:683: // extension. On 2012/03/01 01:17:25, Yoyo Zhou wrote: > ...from the manifest Done.
LGTM with a few more comments, some of which are cleaning up the existing code. (You also missed 1 comment.) The CL description might also be stale. http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... File chrome/common/extensions/extension.cc (right): http://codereview.chromium.org/9424009/diff/5004/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:2821: !LoadAppIsolation(error)) On 2012/03/01 01:17:25, Yoyo Zhou wrote: > This used to have an is_app check. > > Ideally this would be in LoadAppFeatures, but I guess it requires the > permissions to have been parsed. Can you put the is_app check back here? http://codereview.chromium.org/9424009/diff/17001/chrome/common/extensions/ex... File chrome/common/extensions/extension.cc (right): http://codereview.chromium.org/9424009/diff/17001/chrome/common/extensions/ex... chrome/common/extensions/extension.cc:975: bool Extension::LoadRequiredFeatures(string16* error) { So, the required features are actually name and version (description is optional). http://codereview.chromium.org/9424009/diff/17001/chrome/common/extensions/ex... chrome/common/extensions/extension.cc:1009: if (is_platform_app()) { This block looks like it should go in LoadLaunchContainer. http://codereview.chromium.org/9424009/diff/17001/chrome/common/extensions/ex... chrome/common/extensions/extension.cc:1010: if (launch_container() != extension_misc::LAUNCH_SHELL) { style nit: Can you change these to launch_container_ here and in LoadLaunchContainer? http://codereview.chromium.org/9424009/diff/17001/chrome/common/extensions/ex... chrome/common/extensions/extension.cc:1203: bool ReadLaunchDimension(const extensions::Manifest* manifest, Functions like this should go in the anonymous namespace. http://codereview.chromium.org/9424009/diff/17001/chrome/common/extensions/ex... chrome/common/extensions/extension.cc:1608: bool Extension::LoadRequirements(string16* error) { Likewise I might rename this one to CheckRequirements. http://codereview.chromium.org/9424009/diff/17001/chrome/common/extensions/ex... chrome/common/extensions/extension.cc:2302: omnibox_keyword_.empty()) { indent -1 The style guide doesn't treat leading unary operators specially. http://codereview.chromium.org/9424009/diff/17001/chrome/common/extensions/ex... chrome/common/extensions/extension.cc:2783: // Loads the required fields for an extension; this must be done first. There seems to be no need to actually load these things first, although I suppose it's nice if the verification fails fast on them. So I would just remove this comment.
All requested changes have been included. https://chromiumcodereview.appspot.com/9424009/diff/17001/chrome/common/exten... File chrome/common/extensions/extension.cc (right): https://chromiumcodereview.appspot.com/9424009/diff/17001/chrome/common/exten... chrome/common/extensions/extension.cc:975: bool Extension::LoadRequiredFeatures(string16* error) { On 2012/03/02 20:20:07, Yoyo Zhou wrote: > So, the required features are actually name and version (description is > optional). Done. https://chromiumcodereview.appspot.com/9424009/diff/17001/chrome/common/exten... chrome/common/extensions/extension.cc:1009: if (is_platform_app()) { On 2012/03/02 20:20:07, Yoyo Zhou wrote: > This block looks like it should go in LoadLaunchContainer. Done. https://chromiumcodereview.appspot.com/9424009/diff/17001/chrome/common/exten... chrome/common/extensions/extension.cc:1010: if (launch_container() != extension_misc::LAUNCH_SHELL) { On 2012/03/02 20:20:07, Yoyo Zhou wrote: > style nit: Can you change these to launch_container_ here and in > LoadLaunchContainer? Done. https://chromiumcodereview.appspot.com/9424009/diff/17001/chrome/common/exten... chrome/common/extensions/extension.cc:1203: bool ReadLaunchDimension(const extensions::Manifest* manifest, On 2012/03/02 20:20:07, Yoyo Zhou wrote: > Functions like this should go in the anonymous namespace. Done. https://chromiumcodereview.appspot.com/9424009/diff/17001/chrome/common/exten... chrome/common/extensions/extension.cc:1608: bool Extension::LoadRequirements(string16* error) { On 2012/03/02 20:20:07, Yoyo Zhou wrote: > Likewise I might rename this one to CheckRequirements. Done. https://chromiumcodereview.appspot.com/9424009/diff/17001/chrome/common/exten... chrome/common/extensions/extension.cc:2302: omnibox_keyword_.empty()) { On 2012/03/02 20:20:07, Yoyo Zhou wrote: > indent -1 > The style guide doesn't treat leading unary operators specially. Done. https://chromiumcodereview.appspot.com/9424009/diff/17001/chrome/common/exten... chrome/common/extensions/extension.cc:2783: // Loads the required fields for an extension; this must be done first. On 2012/03/02 20:20:07, Yoyo Zhou wrote: > There seems to be no need to actually load these things first, although I > suppose it's nice if the verification fails fast on them. So I would just remove > this comment. Done.
LGTM http://codereview.chromium.org/9424009/diff/20001/chrome/common/extensions/ex... File chrome/common/extensions/extension.cc (right): http://codereview.chromium.org/9424009/diff/20001/chrome/common/extensions/ex... chrome/common/extensions/extension.cc:1333: if (!LoadDescription(error) || So pretty. http://codereview.chromium.org/9424009/diff/20001/chrome/common/extensions/ex... chrome/common/extensions/extension.cc:1784: bool Extension::LoadBackgroundAllowJsAccess( Nit: "JS" should be capitalized, or you could also use "Script".
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/9424009/26001
http://codereview.chromium.org/9424009/diff/20001/chrome/common/extensions/ex... File chrome/common/extensions/extension.cc (right): http://codereview.chromium.org/9424009/diff/20001/chrome/common/extensions/ex... chrome/common/extensions/extension.cc:1784: bool Extension::LoadBackgroundAllowJsAccess( On 2012/03/07 21:10:54, Aaron Boodman wrote: > Nit: "JS" should be capitalized, or you could also use "Script". It's changed in the function name; I'll submit a mini-CL with the keys and errors changed appropriately soon (I want to get this one in soon, since it's a large enough change that it requires updating by-hand when extension.cc changes)
Change committed as 125823
On 2012/03/09 10:45:14, I haz the power (commit-bot) wrote: > Change committed as 125823 Any chance this could have caused the failure on the bots for this run... http://build.chromium.org/p/chromium/builders/Mac10.6%20Tests%20%283%29/build... ... the failure started in the build following the build you checked into, but the change that started the following build looks unrelated...
Anyway, joaodasilva is going to clobber the build to see if the error goes away. Check the main waterfall to see if there is still a failure before digging in.
On 2012/03/09 15:08:01, Finnur wrote: > Anyway, joaodasilva is going to clobber the build to see if the error goes away. > Check the main waterfall to see if there is still a failure before digging in. It doesn't appear that this change caused the problem; it was running fine on mac try-bots this morning when I came in, and the problem appears to be resolved. Thanks for the heads-up, though; I appreciate it :) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
