Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9705083:
Unknown options in extension manifest file are silently ignored (Closed)
|
Created:
8 years, 9 months ago by mitchellwrosen Modified:
8 years, 7 months ago Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionUnknown options in extension manifest file are silently ignored
Added a red warning beneath "Inspect active views" (when in developer mode on
chrome://settings/extensions) if a loaded extension contains an unrecognized
manifest key.
BUG=73693
TEST=Manual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=136519
Patch Set 1 #Patch Set 2 : Removed an accedentally-included file #
Total comments: 29
Patch Set 3 : Changes per comments #
Total comments: 2
Patch Set 4 : Reduced to a couple lines #
Total comments: 2
Patch Set 5 : Fixed unit test #
Messages
Total messages: 24 (0 generated)
https://chromiumcodereview.appspot.com/9705083/diff/2001/chrome/browser/resou... File chrome/browser/resources/extensions/extension_list.js (right): https://chromiumcodereview.appspot.com/9705083/diff/2001/chrome/browser/resou... chrome/browser/resources/extensions/extension_list.js:235: // Then unrecognized manifest keys nit: Please turn this into a complete sentence. https://chromiumcodereview.appspot.com/9705083/diff/2001/chrome/browser/resou... chrome/browser/resources/extensions/extension_list.js:235: // Then unrecognized manifest keys nit: Add a period at the end of the sentence. https://chromiumcodereview.appspot.com/9705083/diff/2001/chrome/browser/resou... chrome/browser/resources/extensions/extension_list.js:241: for (var i = 1; i < extension.unrecognizedKeys.length; ++i) { nit: i++ https://chromiumcodereview.appspot.com/9705083/diff/2001/chrome/browser/resou... chrome/browser/resources/extensions/extension_list.js:241: for (var i = 1; i < extension.unrecognizedKeys.length; ++i) { nit: No braces for single-line blocks. https://chromiumcodereview.appspot.com/9705083/diff/2001/chrome/browser/resou... File chrome/browser/resources/extensions/extensions.html (right): https://chromiumcodereview.appspot.com/9705083/diff/2001/chrome/browser/resou... chrome/browser/resources/extensions/extensions.html:91: <span i18n-content="extensionSettingsExtensionUnrecognizedKeys"></span> nit: 80 cols. https://chromiumcodereview.appspot.com/9705083/diff/2001/chrome/browser/resou... chrome/browser/resources/extensions/extensions.html:92: <span></span> What's with the empty span? https://chromiumcodereview.appspot.com/9705083/diff/2001/chrome/browser/ui/we... File chrome/browser/ui/webui/options/extension_settings_handler.cc (right): https://chromiumcodereview.appspot.com/9705083/diff/2001/chrome/browser/ui/we... chrome/browser/ui/webui/options/extension_settings_handler.cc:696: // Add unrecognized manifest keys nit: Add period.
just looked at a bit of the C++, gotta run, will look at the rest tomorrow http://codereview.chromium.org/9705083/diff/2001/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options/extension_settings_handler.cc (right): http://codereview.chromium.org/9705083/diff/2001/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/extension_settings_handler.cc:703: ++iter) { nit: call iterators "it" or "i", it's more concise, and if it can fit on 2 lines then do that (put the ++i on the line above) http://codereview.chromium.org/9705083/diff/2001/chrome/common/extensions/ext... File chrome/common/extensions/extension.h (right): http://codereview.chromium.org/9705083/diff/2001/chrome/common/extensions/ext... chrome/common/extensions/extension.h:527: // Unrecognized keys are filled in when Manifest::ValidateManifest is called. Why need this method? Seems adequate to call manifest()->HasUnrecognizedManifestKeys(). http://codereview.chromium.org/9705083/diff/2001/chrome/common/extensions/man... File chrome/common/extensions/manifest.h (right): http://codereview.chromium.org/9705083/diff/2001/chrome/common/extensions/man... chrome/common/extensions/manifest.h:85: const std::vector<const std::string*>** unrecognized_keys) const; The unrecognised keys are unlikely to be a lot of data, so it'd be a cleaner interface to just return a new std::vector<std::string> here rather than taking it as an out-parameter. Also, call it GetUnrecognizedKeys. http://codereview.chromium.org/9705083/diff/2001/chrome/common/extensions/man... chrome/common/extensions/manifest.h:105: std::vector<const std::string*> unrecognized_keys_; Make this a scoped_ptr<std::vector<std::string> > and return copies of it in GetUnrecognizedKeys() (CHECKing that it exists first).
rest of the comments. http://codereview.chromium.org/9705083/diff/2001/chrome/browser/resources/ext... File chrome/browser/resources/extensions/extension_list.js (right): http://codereview.chromium.org/9705083/diff/2001/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:235: // Then unrecognized manifest keys On 2012/04/08 20:33:23, James Hawkins wrote: > nit: Please turn this into a complete sentence. Note that the existing comments above are already of this strange non-complete-sentence form, so if you're going to do this then you should probably update those too. http://codereview.chromium.org/9705083/diff/2001/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:239: var keys = unrecognizedKeys.querySelector('span:nth-of-type(2)'); Suggestion: make this an unordered list rather than a flat list of names. http://codereview.chromium.org/9705083/diff/2001/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:241: for (var i = 1; i < extension.unrecognizedKeys.length; ++i) { nit: use forEach here rather than a for loop http://codereview.chromium.org/9705083/diff/2001/chrome/browser/resources/ext... File chrome/browser/resources/extensions/extensions.html (right): http://codereview.chromium.org/9705083/diff/2001/chrome/browser/resources/ext... chrome/browser/resources/extensions/extensions.html:92: <span></span> On 2012/04/08 20:33:23, James Hawkins wrote: > What's with the empty span? Looks like this is a target for the content. If you follow my suggestion in the JS, make this an <ul> with a quick comment explaining that? http://codereview.chromium.org/9705083/diff/2001/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options/extension_settings_handler.cc (right): http://codereview.chromium.org/9705083/diff/2001/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/extension_settings_handler.cc:697: const std::vector<const std::string*>* unrecognized_keys_vec; just call it "unrecognized_keys" or even "keys". http://codereview.chromium.org/9705083/diff/2001/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/extension_settings_handler.cc:697: const std::vector<const std::string*>* unrecognized_keys_vec; also, a bit of a nit, but since this is a really long method and this variable is just temporary, perhaps surround the whole block in a { } scope? http://codereview.chromium.org/9705083/diff/2001/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/extension_settings_handler.cc:699: ListValue* unrecognized_keys = new ListValue; nit: ListValue not ListValue()
https://chromiumcodereview.appspot.com/9705083/diff/2001/chrome/browser/resou... File chrome/browser/resources/extensions/extension_list.js (right): https://chromiumcodereview.appspot.com/9705083/diff/2001/chrome/browser/resou... chrome/browser/resources/extensions/extension_list.js:235: // Then unrecognized manifest keys On 2012/04/08 20:33:23, James Hawkins wrote: > nit: Please turn this into a complete sentence. Done. https://chromiumcodereview.appspot.com/9705083/diff/2001/chrome/browser/resou... chrome/browser/resources/extensions/extension_list.js:235: // Then unrecognized manifest keys On 2012/04/08 20:33:23, James Hawkins wrote: > nit: Add a period at the end of the sentence. Done. https://chromiumcodereview.appspot.com/9705083/diff/2001/chrome/browser/resou... chrome/browser/resources/extensions/extension_list.js:235: // Then unrecognized manifest keys On 2012/04/10 02:02:32, kalman wrote: > On 2012/04/08 20:33:23, James Hawkins wrote: > > nit: Please turn this into a complete sentence. > > Note that the existing comments above are already of this strange > non-complete-sentence form, so if you're going to do this then you should > probably update those too. Done. https://chromiumcodereview.appspot.com/9705083/diff/2001/chrome/browser/resou... chrome/browser/resources/extensions/extension_list.js:239: var keys = unrecognizedKeys.querySelector('span:nth-of-type(2)'); On 2012/04/10 02:02:32, kalman wrote: > Suggestion: make this an unordered list rather than a flat list of names. Done. https://chromiumcodereview.appspot.com/9705083/diff/2001/chrome/browser/resou... chrome/browser/resources/extensions/extension_list.js:241: for (var i = 1; i < extension.unrecognizedKeys.length; ++i) { On 2012/04/10 02:02:32, kalman wrote: > nit: use forEach here rather than a for loop Done. https://chromiumcodereview.appspot.com/9705083/diff/2001/chrome/browser/resou... File chrome/browser/resources/extensions/extensions.html (right): https://chromiumcodereview.appspot.com/9705083/diff/2001/chrome/browser/resou... chrome/browser/resources/extensions/extensions.html:91: <span i18n-content="extensionSettingsExtensionUnrecognizedKeys"></span> On 2012/04/08 20:33:23, James Hawkins wrote: > nit: 80 cols. Done. https://chromiumcodereview.appspot.com/9705083/diff/2001/chrome/browser/resou... chrome/browser/resources/extensions/extensions.html:92: <span></span> On 2012/04/10 02:02:32, kalman wrote: > On 2012/04/08 20:33:23, James Hawkins wrote: > > What's with the empty span? > > Looks like this is a target for the content. If you follow my suggestion in the > JS, make this an <ul> with a quick comment explaining that? Done. https://chromiumcodereview.appspot.com/9705083/diff/2001/chrome/browser/ui/we... File chrome/browser/ui/webui/options/extension_settings_handler.cc (right): https://chromiumcodereview.appspot.com/9705083/diff/2001/chrome/browser/ui/we... chrome/browser/ui/webui/options/extension_settings_handler.cc:696: // Add unrecognized manifest keys On 2012/04/08 20:33:23, James Hawkins wrote: > nit: Add period. Done. https://chromiumcodereview.appspot.com/9705083/diff/2001/chrome/browser/ui/we... chrome/browser/ui/webui/options/extension_settings_handler.cc:697: const std::vector<const std::string*>* unrecognized_keys_vec; On 2012/04/10 02:02:32, kalman wrote: > just call it "unrecognized_keys" or even "keys". Done. https://chromiumcodereview.appspot.com/9705083/diff/2001/chrome/browser/ui/we... chrome/browser/ui/webui/options/extension_settings_handler.cc:699: ListValue* unrecognized_keys = new ListValue; On 2012/04/10 02:02:32, kalman wrote: > nit: ListValue not ListValue() Done. https://chromiumcodereview.appspot.com/9705083/diff/2001/chrome/browser/ui/we... chrome/browser/ui/webui/options/extension_settings_handler.cc:703: ++iter) { On 2012/04/09 13:01:54, kalman wrote: > nit: call iterators "it" or "i", it's more concise, and if it can fit on 2 lines > then do that (put the ++i on the line above) Done.
I think we should be just reusing the install_warnings property from Extension, it makes sense to centralise all install warnings in there (and these are install warnings). It would also mean much fewer code changes in this patch. The most sensible place for this is probably in Extension::InitFromValue somewhere. Just add all of those keys from Manifest::unrecognized_keys() into a sensible warning to add to install_warnings_. http://codereview.chromium.org/9705083/diff/12002/chrome/common/extensions/ma... File chrome/common/extensions/manifest.cc (right): http://codereview.chromium.org/9705083/diff/12002/chrome/common/extensions/ma... chrome/common/extensions/manifest.cc:37: // checking to let developers know when they screw up. You can remove the TODO now. Also, the comment above needs to be updated. http://codereview.chromium.org/9705083/diff/12002/chrome/common/extensions/ma... File chrome/common/extensions/manifest.h (right): http://codereview.chromium.org/9705083/diff/12002/chrome/common/extensions/ma... chrome/common/extensions/manifest.h:84: std::vector<std::string> GetUnrecognizedKeys() const { Just call this unrecognized_keys(); this is the standard way to write cheap getters. And to make it cheap, return a "const std::vector<std::string>&"
Hey, looks like code keeps changing around you :) Have a look at https://chromiumcodereview.appspot.com/10217017
lgtm https://chromiumcodereview.appspot.com/9705083/diff/16003/chrome/common/exten... File chrome/common/extensions/manifest.cc (right): https://chromiumcodereview.appspot.com/9705083/diff/16003/chrome/common/exten... chrome/common/extensions/manifest.cc:54: (*key).c_str())); key->c_str()
https://chromiumcodereview.appspot.com/9705083/diff/16003/chrome/common/exten... File chrome/common/extensions/manifest.cc (right): https://chromiumcodereview.appspot.com/9705083/diff/16003/chrome/common/exten... chrome/common/extensions/manifest.cc:54: (*key).c_str())); On 2012/05/01 04:04:01, kalman wrote: > key->c_str() But key's a key_iterator, not a std::string*
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/9705083/16003
Try job failure for 9705083-16003 (retry) on mac_rel for step "unit_tests". It's a second try, previously, step "unit_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
On 2012/05/01 05:06:48, I haz the power (commit-bot) wrote: > Try job failure for 9705083-16003 (retry) on mac_rel for step "unit_tests". > It's a second try, previously, step "unit_tests" failed. > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu... Right, so this is failing unit tests, but now that I think of it you should add a unit test for this. Also, you'll need to update the patch description.
On 2012/05/01 05:06:48, I haz the power (commit-bot) wrote: > Try job failure for 9705083-16003 (retry) on mac_rel for step "unit_tests". > It's a second try, previously, step "unit_tests" failed. > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu... Right, so this is failing unit tests, but now that I think of it you should add a unit test for this. Also, you'll need to update the patch description.
On 2012/05/03 03:14:01, kalman wrote: > On 2012/05/01 05:06:48, I haz the power (commit-bot) wrote: > > Try job failure for 9705083-16003 (retry) on mac_rel for step "unit_tests". > > It's a second try, previously, step "unit_tests" failed. > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu... > > Right, so this is failing unit tests, but now that I think of it you should add > a unit test for this. > > Also, you'll need to update the patch description. Would it be a sufficient unit test to change the two lines that were failing in the existing test ManifestTest.Extension? Line 53: change EXPECT_TRUE(warnings.empty()) to EXPECT_EQ(warnings.size(), 1), because an unknown key is set on line 47. Line 75: Assert that there are 2 warnings instead of 1 (for the same reason as above)
(referring to chrome/common/extensions/manifest_unittest.cc, by the way)
yep, sgtm.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/9705083/31002
Try job failure for 9705083-31002 (retry) on win_rel for step "update". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... Step "update" is always a major failure. Look at the try server FAQ for more details.
The CQ has been having problems. I'll try to dcommit this when I have the time.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/9705083/31002
Change committed as 136519 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
