Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
518653002:
Add the "options_ui" extension manifest field. (Closed)
|
Created:
6 years, 3 months ago by ericzeng Modified:
6 years, 3 months ago Reviewers:
Avi (use Gerrit), benwells, Andrew T Wilson (Slow), Yoyo Zhou, Devlin, Fady Samuel, not at google - send to devlin CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, chromium-apps-reviews_chromium.org, tfarina, arv+watch_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionCommitted: https://crrev.com/09a7e00df7e8316a01cda0288577a293d7d126a3
Cr-Commit-Position: refs/heads/master@{#294019}
Patch Set 1 #
Total comments: 56
Patch Set 2 : Fix everything #
Total comments: 42
Patch Set 3 : Address kalman's comments #
Total comments: 2
Patch Set 4 : Rebase #Patch Set 5 : Address yoz's comments #
Total comments: 6
Patch Set 6 : Fix flag logic and define directive #
Total comments: 46
Patch Set 7 : Address comments #
Total comments: 19
Patch Set 8 : comments #Patch Set 9 : Rebase and replace another instance of ManifestURL::GetOptionsPage #Patch Set 10 : Rebase #Patch Set 11 : Rebase #Patch Set 12 : Fix build issue? #Messages
Total messages: 57 (12 generated)
ericzeng@chromium.org changed reviewers: + fsamuel@chromium.org, kalman@chromium.org, rdevlin.cronin@chromium.org, yoz@chromium.org
PTAL. Sorry about the massive patch, here's what's going on in this CL implementation-wise: - Add "options_ui" manifest entry - Add a new ManifestHandler and ManifestData: OptionsPageManifestHandler and OptionsPageInfo. - Refactor ManifestURL::OptionsPageHandler into OptionsPageManifestHandler - Move ManifestURL::GetOptionsPage to OptionsPageInfo::GetOptionsPage (and refactor EVERYWHERE) - Add tests for "options_ui" - Modify Extension Options code to check "options_ui.open_new_tab" fsamuel@chromium.org: Please review changes in chrome/browser/guest_view/extension_options rdevlin.cronin@chromium.org, yoz@chromium.org, kalman@chromium.org: Please review changes in extensions/, chrome/browser/extensions, and chrome/common/extensions
Lotsa comments, but looks great. https://codereview.chromium.org/518653002/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/518653002/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_context_menu_model.cc:147: .length() > 0; Weird. You can write (url.spec().length() > 0) as just (!url.is_empty()) - mind changing that? (as is already written below for whatever reason) https://codereview.chromium.org/518653002/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/518653002/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_tab_util.cc:560: DCHECK(!OptionsPageInfo::GetOptionsPage(extension).is_empty()); At this point I realise there should be an OptionsPageInfo::HasOptionsPage method. Dunno, up to you if you want to do that cleanup, I think it'd be nice. https://codereview.chromium.org/518653002/diff/1/chrome/browser/resources/ext... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/518653002/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:275: if (this.data_.enableEmbeddedExtensionOptions && The flag check should be unnecessary now, so long as OpenInTab returns true when the flag is off. Which it should. https://codereview.chromium.org/518653002/diff/1/chrome/browser/ui/webui/exte... File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right): https://codereview.chromium.org/518653002/diff/1/chrome/browser/ui/webui/exte... chrome/browser/ui/webui/extensions/extension_settings_handler.cc:274: extension_data->SetBoolean("options_chrome_style", When would you need to access this? https://codereview.chromium.org/518653002/diff/1/chrome/common/extensions/man... File chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc (right): https://codereview.chromium.org/518653002/diff/1/chrome/common/extensions/man... chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc:63: extensions::OptionsPageInfo::GetOptionsPage(extension.get()).path()); Odd to be comparing the parts separately. Can you just do EXPECT_EQ(GURL("chrome-extension://options.html"), extensions::OptionsPageInfo::GetOptionsPage(extension.get())); ? https://codereview.chromium.org/518653002/diff/1/chrome/common/extensions/man... chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc:94: } Could you add tests with the feature switch turned on/off? https://codereview.chromium.org/518653002/diff/1/extensions/common/api/extens... File extensions/common/api/extensions_manifest_types.json (right): https://codereview.chromium.org/518653002/diff/1/extensions/common/api/extens... extensions/common/api/extensions_manifest_types.json:47: "description": "<p>The path to your options page.</p>", No need for the surrounding <p>, we can do that ourselves. Same elsewhere. You only need the <p>s if you have multiple paragraphs. https://codereview.chromium.org/518653002/diff/1/extensions/common/api/extens... extensions/common/api/extensions_manifest_types.json:48: "optional": false, "optional" is false by default. https://codereview.chromium.org/518653002/diff/1/extensions/common/api/extens... extensions/common/api/extensions_manifest_types.json:57: "description": "<p>If <code>true</code>, the options page will be opened in a new tab instead of in an embedded popup in chrome://extensions. The default value is <code>false</code.</p>", </code> not </code https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... File extensions/common/manifest_handlers/options_page_info.cc (right): https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:35: std::string url_string, const std::string& https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:36: OptionsPageInfo::OptionsUrlType url_type, Rather than having this enum type, you could parameterise all of your error messages with the path (they're basically copied from each other anyway). Then you could pass the path into here. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:46: *error = base::ASCIIToUTF16(errors::kInvalidOptionsUIPageInHostedApp); This shouldn't even be possible, hosted apps can't specify options_ui at all. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:49: *result = options_url; You should probably early-return in the hosted app case, particularly since some of the code isn't necessary. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:69: *error = base::ASCIIToUTF16(errors::kInvalidOptionsUIPage); These are the same? https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:112: return GetOptionsPageInfo(extension)->chrome_styles_; Shouldn't you be checking for an empty info here? https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:117: return GetOptionsPageInfo(extension)->open_in_tab_; And here? And for both of the above: we should make sure that these behave as you'd expect when the feature switch is *off*. However that should be - whether you parse differently, or implement these methods differently (the former I guess?). But yeah - ChromeStyle should always be false if the flag is off, OpenInTab should always be true. In fact you might be able to clean up some of the places that check the feature flag to just rely on the logic in here being correct instead. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:120: scoped_ptr<OptionsPageInfo> OptionsPageInfo::FromValues( FromValues looks kinda odd, and besides this is a Parse method, so Parse()? https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:177: bool OptionsPageManifestHandler::Parse(Extension* extension, Generally speaking: everywhere, try to show install warnings rather than errors. This isn't a pattern that most other manifest handlers use, but I wish it was. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:188: } Keeping the parse logic in OptionsPageInfo is good, as you've done, but in this case I think you could do a bit up front just to give OptionsPageInfo sane values and make this check more meaningful (i.e. address your "Note..." comment). Manifest* manifest = extension->manifest(); std::string options_page; if (manifest->HasPath(keys::kOptionsPage) && manifest->GetAsString(keys::kOptionsPage, &options_page)) { install_warnings.push_back("invalid value for options_page or whatever"); return; } const base::Value* options_ui = NULL; if (manifest->HasPath(keys::kOptionsUI) && !manifest->Get(...)) { install_warnings.push_back(...); return; } ... etc. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:213: if (!OptionsPageInfo::GetOptionsPage(extension).is_empty() && HasOptionsPage would look so much nicer in places like this :( Early-exit might look better here as well. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:218: const base::FilePath path = const base::FilePath& https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... File extensions/common/manifest_handlers/options_page_info.h (right): https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.h:21: enum OptionsUrlType { OPTIONS_PAGE, OPTIONS_UI_PAGE }; When is this used? https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.h:23: OptionsPageInfo(GURL options_page, const GURL& and below. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.h:32: static GURL GetOptionsPage(const Extension* extension); Put line breaks between the method declaration and the next method's comment. Also this method should return a const GURL&. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.h:35: static bool ChromeStyle(const Extension* extension); IsChromeStyle?
The guest_view change seems trivial and I'm not too familiar with the other stuff, so I'll let other reviewers give their approval then I'll add a guest_view rubberstamp.
Initial round of responses from the bus https://codereview.chromium.org/518653002/diff/1/chrome/browser/resources/ext... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/518653002/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:275: if (this.data_.enableEmbeddedExtensionOptions && On 2014/08/29 05:39:23, kalman wrote: > The flag check should be unnecessary now, so long as OpenInTab returns true when > the flag is off. Which it should. Hold your horses, I haven't removed the feature flag quite yet. https://codereview.chromium.org/518653002/diff/1/chrome/browser/ui/webui/exte... File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right): https://codereview.chromium.org/518653002/diff/1/chrome/browser/ui/webui/exte... chrome/browser/ui/webui/extensions/extension_settings_handler.cc:274: extension_data->SetBoolean("options_chrome_style", On 2014/08/29 05:39:23, kalman wrote: > When would you need to access this? I'm not sure, if the UA stylesheet is handled all in C++ then I suppose I don't need it here. https://codereview.chromium.org/518653002/diff/1/chrome/common/extensions/man... File chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc (right): https://codereview.chromium.org/518653002/diff/1/chrome/common/extensions/man... chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc:63: extensions::OptionsPageInfo::GetOptionsPage(extension.get()).path()); On 2014/08/29 05:39:23, kalman wrote: > Odd to be comparing the parts separately. Can you just do > > EXPECT_EQ(GURL("chrome-extension://options.html"), > extensions::OptionsPageInfo::GetOptionsPage(extension.get())); > > ? I'll need the extension id, but yes that should work. I was modeling my tests off of the existing ones. https://codereview.chromium.org/518653002/diff/1/chrome/common/extensions/man... chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc:94: } On 2014/08/29 05:39:23, kalman wrote: > Could you add tests with the feature switch turned on/off? The feature switch doesn't affect any of the manifest code, plus I thought we were removing it soon? https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... File extensions/common/manifest_handlers/options_page_info.cc (right): https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:36: OptionsPageInfo::OptionsUrlType url_type, On 2014/08/29 05:39:24, kalman wrote: > Rather than having this enum type, you could parameterise all of your error > messages with the path (they're basically copied from each other anyway). Then > you could pass the path into here. Doesn't that mean moving the errors out of manifest_constants? There some tests and possibly other code that also need the error strings. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:177: bool OptionsPageManifestHandler::Parse(Extension* extension, On 2014/08/29 05:39:24, kalman wrote: > Generally speaking: everywhere, try to show install warnings rather than errors. > This isn't a pattern that most other manifest handlers use, but I wish it was. Does you mean that in places where I set *error = errors::kError I should do install_warnings->push_back(errors::kError)? I try to do it where I can but a lot of the options_page manifest tests are checking for the errors instead of install warnings. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... File extensions/common/manifest_handlers/options_page_info.h (right): https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.h:21: enum OptionsUrlType { OPTIONS_PAGE, OPTIONS_UI_PAGE }; On 2014/08/29 05:39:25, kalman wrote: > When is this used? It's used in OptionsPageInfo::FromValues to determine which kind of error to throw, the options_page error or the options_ui error. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.h:35: static bool ChromeStyle(const Extension* extension); On 2014/08/29 05:39:25, kalman wrote: > IsChromeStyle? I thought that just naming the method the same thing as the field would be easiest, I would also prefer something that captured the intent like "ShouldUseChromeStyle"
https://codereview.chromium.org/518653002/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/518653002/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_context_menu_model.cc:147: .length() > 0; On 2014/08/29 05:39:23, kalman wrote: > Weird. You can write (url.spec().length() > 0) as just (!url.is_empty()) - mind > changing that? > > (as is already written below for whatever reason) Done. https://codereview.chromium.org/518653002/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/518653002/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_tab_util.cc:560: DCHECK(!OptionsPageInfo::GetOptionsPage(extension).is_empty()); On 2014/08/29 05:39:23, kalman wrote: > At this point I realise there should be an OptionsPageInfo::HasOptionsPage > method. Dunno, up to you if you want to do that cleanup, I think it'd be nice. Done. https://codereview.chromium.org/518653002/diff/1/chrome/browser/resources/ext... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/518653002/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:275: if (this.data_.enableEmbeddedExtensionOptions && On 2014/08/29 16:52:22, ericzeng wrote: > On 2014/08/29 05:39:23, kalman wrote: > > The flag check should be unnecessary now, so long as OpenInTab returns true > when > > the flag is off. Which it should. > > Hold your horses, I haven't removed the feature flag quite yet. Done. https://codereview.chromium.org/518653002/diff/1/chrome/browser/ui/webui/exte... File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right): https://codereview.chromium.org/518653002/diff/1/chrome/browser/ui/webui/exte... chrome/browser/ui/webui/extensions/extension_settings_handler.cc:274: extension_data->SetBoolean("options_chrome_style", On 2014/08/29 16:52:22, ericzeng wrote: > On 2014/08/29 05:39:23, kalman wrote: > > When would you need to access this? > > I'm not sure, if the UA stylesheet is handled all in C++ then I suppose I don't > need it here. Done. https://codereview.chromium.org/518653002/diff/1/chrome/common/extensions/man... File chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc (right): https://codereview.chromium.org/518653002/diff/1/chrome/common/extensions/man... chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc:63: extensions::OptionsPageInfo::GetOptionsPage(extension.get()).path()); On 2014/08/29 16:52:22, ericzeng wrote: > On 2014/08/29 05:39:23, kalman wrote: > > Odd to be comparing the parts separately. Can you just do > > > > EXPECT_EQ(GURL("chrome-extension://options.html"), > > extensions::OptionsPageInfo::GetOptionsPage(extension.get())); > > > > ? > > I'll need the extension id, but yes that should work. I was modeling my tests > off of the existing ones. Done. https://codereview.chromium.org/518653002/diff/1/chrome/common/extensions/man... chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc:94: } On 2014/08/29 16:52:22, ericzeng wrote: > On 2014/08/29 05:39:23, kalman wrote: > > Could you add tests with the feature switch turned on/off? > > The feature switch doesn't affect any of the manifest code, plus I thought we > were removing it soon? Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/api/extens... File extensions/common/api/extensions_manifest_types.json (right): https://codereview.chromium.org/518653002/diff/1/extensions/common/api/extens... extensions/common/api/extensions_manifest_types.json:47: "description": "<p>The path to your options page.</p>", On 2014/08/29 05:39:24, kalman wrote: > No need for the surrounding <p>, we can do that ourselves. Same elsewhere. You > only need the <p>s if you have multiple paragraphs. Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/api/extens... extensions/common/api/extensions_manifest_types.json:48: "optional": false, On 2014/08/29 05:39:24, kalman wrote: > "optional" is false by default. Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/api/extens... extensions/common/api/extensions_manifest_types.json:57: "description": "<p>If <code>true</code>, the options page will be opened in a new tab instead of in an embedded popup in chrome://extensions. The default value is <code>false</code.</p>", On 2014/08/29 05:39:24, kalman wrote: > </code> not </code Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... File extensions/common/manifest_handlers/options_page_info.cc (right): https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:35: std::string url_string, On 2014/08/29 05:39:24, kalman wrote: > const std::string& Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:36: OptionsPageInfo::OptionsUrlType url_type, On 2014/08/29 16:52:22, ericzeng wrote: > On 2014/08/29 05:39:24, kalman wrote: > > Rather than having this enum type, you could parameterise all of your error > > messages with the path (they're basically copied from each other anyway). Then > > you could pass the path into here. > > Doesn't that mean moving the errors out of manifest_constants? There some tests > and possibly other code that also need the error strings. Changed from enum to parameterized string https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:46: *error = base::ASCIIToUTF16(errors::kInvalidOptionsUIPageInHostedApp); On 2014/08/29 05:39:24, kalman wrote: > This shouldn't even be possible, hosted apps can't specify options_ui at all. Removed OptionsUI errors https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:49: *result = options_url; On 2014/08/29 05:39:25, kalman wrote: > You should probably early-return in the hosted app case, particularly since some > of the code isn't necessary. Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:69: *error = base::ASCIIToUTF16(errors::kInvalidOptionsUIPage); On 2014/08/29 05:39:25, kalman wrote: > These are the same? Replaced with parameterized error https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:112: return GetOptionsPageInfo(extension)->chrome_styles_; On 2014/08/29 05:39:24, kalman wrote: > Shouldn't you be checking for an empty info here? Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:117: return GetOptionsPageInfo(extension)->open_in_tab_; On 2014/08/29 05:39:24, kalman wrote: > And here? > > And for both of the above: we should make sure that these behave as you'd expect > when the feature switch is *off*. However that should be - whether you parse > differently, or implement these methods differently (the former I guess?). > > But yeah - ChromeStyle should always be false if the flag is off, OpenInTab > should always be true. > > In fact you might be able to clean up some of the places that check the feature > flag to just rely on the logic in here being correct instead. Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:120: scoped_ptr<OptionsPageInfo> OptionsPageInfo::FromValues( On 2014/08/29 05:39:24, kalman wrote: > FromValues looks kinda odd, and besides this is a Parse method, so Parse()? Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:188: } On 2014/08/29 05:39:24, kalman wrote: > Keeping the parse logic in OptionsPageInfo is good, as you've done, but in this > case I think you could do a bit up front just to give OptionsPageInfo sane > values and make this check more meaningful (i.e. address your "Note..." > comment). > > > Manifest* manifest = extension->manifest(); > > std::string options_page; > if (manifest->HasPath(keys::kOptionsPage) && > manifest->GetAsString(keys::kOptionsPage, &options_page)) { > install_warnings.push_back("invalid value for options_page or whatever"); > return; > } > > const base::Value* options_ui = NULL; > if (manifest->HasPath(keys::kOptionsUI) && !manifest->Get(...)) { > install_warnings.push_back(...); > return; > } > > ... etc. Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:213: if (!OptionsPageInfo::GetOptionsPage(extension).is_empty() && On 2014/08/29 05:39:24, kalman wrote: > HasOptionsPage would look so much nicer in places like this :( > > Early-exit might look better here as well. Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:218: const base::FilePath path = On 2014/08/29 05:39:24, kalman wrote: > const base::FilePath& Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... File extensions/common/manifest_handlers/options_page_info.h (right): https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.h:21: enum OptionsUrlType { OPTIONS_PAGE, OPTIONS_UI_PAGE }; On 2014/08/29 16:52:22, ericzeng wrote: > On 2014/08/29 05:39:25, kalman wrote: > > When is this used? > > It's used in OptionsPageInfo::FromValues to determine which kind of error to > throw, the options_page error or the options_ui error. Removed (as discussed in person) https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.h:32: static GURL GetOptionsPage(const Extension* extension); On 2014/08/29 05:39:25, kalman wrote: > Put line breaks between the method declaration and the next method's comment. > > Also this method should return a const GURL&. Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.h:35: static bool ChromeStyle(const Extension* extension); On 2014/08/29 16:52:22, ericzeng wrote: > On 2014/08/29 05:39:25, kalman wrote: > > IsChromeStyle? > > I thought that just naming the method the same thing as the field would be > easiest, I would also prefer something that captured the intent like > "ShouldUseChromeStyle" Changed to ShouldUseChromeStyle as discussed in person.
https://codereview.chromium.org/518653002/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_context_menu_model.cc (right): https://codereview.chromium.org/518653002/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_context_menu_model.cc:147: .length() > 0; On 2014/08/29 05:39:23, kalman wrote: > Weird. You can write (url.spec().length() > 0) as just (!url.is_empty()) - mind > changing that? > > (as is already written below for whatever reason) Done. https://codereview.chromium.org/518653002/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/518653002/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/extension_tab_util.cc:560: DCHECK(!OptionsPageInfo::GetOptionsPage(extension).is_empty()); On 2014/08/29 05:39:23, kalman wrote: > At this point I realise there should be an OptionsPageInfo::HasOptionsPage > method. Dunno, up to you if you want to do that cleanup, I think it'd be nice. Done. https://codereview.chromium.org/518653002/diff/1/chrome/browser/resources/ext... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/518653002/diff/1/chrome/browser/resources/ext... chrome/browser/resources/extensions/extension_list.js:275: if (this.data_.enableEmbeddedExtensionOptions && On 2014/08/29 16:52:22, ericzeng wrote: > On 2014/08/29 05:39:23, kalman wrote: > > The flag check should be unnecessary now, so long as OpenInTab returns true > when > > the flag is off. Which it should. > > Hold your horses, I haven't removed the feature flag quite yet. Done. https://codereview.chromium.org/518653002/diff/1/chrome/browser/ui/webui/exte... File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right): https://codereview.chromium.org/518653002/diff/1/chrome/browser/ui/webui/exte... chrome/browser/ui/webui/extensions/extension_settings_handler.cc:274: extension_data->SetBoolean("options_chrome_style", On 2014/08/29 16:52:22, ericzeng wrote: > On 2014/08/29 05:39:23, kalman wrote: > > When would you need to access this? > > I'm not sure, if the UA stylesheet is handled all in C++ then I suppose I don't > need it here. Done. https://codereview.chromium.org/518653002/diff/1/chrome/common/extensions/man... File chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc (right): https://codereview.chromium.org/518653002/diff/1/chrome/common/extensions/man... chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc:63: extensions::OptionsPageInfo::GetOptionsPage(extension.get()).path()); On 2014/08/29 16:52:22, ericzeng wrote: > On 2014/08/29 05:39:23, kalman wrote: > > Odd to be comparing the parts separately. Can you just do > > > > EXPECT_EQ(GURL("chrome-extension://options.html"), > > extensions::OptionsPageInfo::GetOptionsPage(extension.get())); > > > > ? > > I'll need the extension id, but yes that should work. I was modeling my tests > off of the existing ones. Done. https://codereview.chromium.org/518653002/diff/1/chrome/common/extensions/man... chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc:94: } On 2014/08/29 16:52:22, ericzeng wrote: > On 2014/08/29 05:39:23, kalman wrote: > > Could you add tests with the feature switch turned on/off? > > The feature switch doesn't affect any of the manifest code, plus I thought we > were removing it soon? Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/api/extens... File extensions/common/api/extensions_manifest_types.json (right): https://codereview.chromium.org/518653002/diff/1/extensions/common/api/extens... extensions/common/api/extensions_manifest_types.json:47: "description": "<p>The path to your options page.</p>", On 2014/08/29 05:39:24, kalman wrote: > No need for the surrounding <p>, we can do that ourselves. Same elsewhere. You > only need the <p>s if you have multiple paragraphs. Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/api/extens... extensions/common/api/extensions_manifest_types.json:48: "optional": false, On 2014/08/29 05:39:24, kalman wrote: > "optional" is false by default. Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/api/extens... extensions/common/api/extensions_manifest_types.json:57: "description": "<p>If <code>true</code>, the options page will be opened in a new tab instead of in an embedded popup in chrome://extensions. The default value is <code>false</code.</p>", On 2014/08/29 05:39:24, kalman wrote: > </code> not </code Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... File extensions/common/manifest_handlers/options_page_info.cc (right): https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:35: std::string url_string, On 2014/08/29 05:39:24, kalman wrote: > const std::string& Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:36: OptionsPageInfo::OptionsUrlType url_type, On 2014/08/29 16:52:22, ericzeng wrote: > On 2014/08/29 05:39:24, kalman wrote: > > Rather than having this enum type, you could parameterise all of your error > > messages with the path (they're basically copied from each other anyway). Then > > you could pass the path into here. > > Doesn't that mean moving the errors out of manifest_constants? There some tests > and possibly other code that also need the error strings. Changed from enum to parameterized string https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:46: *error = base::ASCIIToUTF16(errors::kInvalidOptionsUIPageInHostedApp); On 2014/08/29 05:39:24, kalman wrote: > This shouldn't even be possible, hosted apps can't specify options_ui at all. Removed OptionsUI errors https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:49: *result = options_url; On 2014/08/29 05:39:25, kalman wrote: > You should probably early-return in the hosted app case, particularly since some > of the code isn't necessary. Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:69: *error = base::ASCIIToUTF16(errors::kInvalidOptionsUIPage); On 2014/08/29 05:39:25, kalman wrote: > These are the same? Replaced with parameterized error https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:112: return GetOptionsPageInfo(extension)->chrome_styles_; On 2014/08/29 05:39:24, kalman wrote: > Shouldn't you be checking for an empty info here? Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:117: return GetOptionsPageInfo(extension)->open_in_tab_; On 2014/08/29 05:39:24, kalman wrote: > And here? > > And for both of the above: we should make sure that these behave as you'd expect > when the feature switch is *off*. However that should be - whether you parse > differently, or implement these methods differently (the former I guess?). > > But yeah - ChromeStyle should always be false if the flag is off, OpenInTab > should always be true. > > In fact you might be able to clean up some of the places that check the feature > flag to just rely on the logic in here being correct instead. Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:120: scoped_ptr<OptionsPageInfo> OptionsPageInfo::FromValues( On 2014/08/29 05:39:24, kalman wrote: > FromValues looks kinda odd, and besides this is a Parse method, so Parse()? Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:188: } On 2014/08/29 05:39:24, kalman wrote: > Keeping the parse logic in OptionsPageInfo is good, as you've done, but in this > case I think you could do a bit up front just to give OptionsPageInfo sane > values and make this check more meaningful (i.e. address your "Note..." > comment). > > > Manifest* manifest = extension->manifest(); > > std::string options_page; > if (manifest->HasPath(keys::kOptionsPage) && > manifest->GetAsString(keys::kOptionsPage, &options_page)) { > install_warnings.push_back("invalid value for options_page or whatever"); > return; > } > > const base::Value* options_ui = NULL; > if (manifest->HasPath(keys::kOptionsUI) && !manifest->Get(...)) { > install_warnings.push_back(...); > return; > } > > ... etc. Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:213: if (!OptionsPageInfo::GetOptionsPage(extension).is_empty() && On 2014/08/29 05:39:24, kalman wrote: > HasOptionsPage would look so much nicer in places like this :( > > Early-exit might look better here as well. Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.cc:218: const base::FilePath path = On 2014/08/29 05:39:24, kalman wrote: > const base::FilePath& Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... File extensions/common/manifest_handlers/options_page_info.h (right): https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.h:21: enum OptionsUrlType { OPTIONS_PAGE, OPTIONS_UI_PAGE }; On 2014/08/29 16:52:22, ericzeng wrote: > On 2014/08/29 05:39:25, kalman wrote: > > When is this used? > > It's used in OptionsPageInfo::FromValues to determine which kind of error to > throw, the options_page error or the options_ui error. Removed (as discussed in person) https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.h:32: static GURL GetOptionsPage(const Extension* extension); On 2014/08/29 05:39:25, kalman wrote: > Put line breaks between the method declaration and the next method's comment. > > Also this method should return a const GURL&. Done. https://codereview.chromium.org/518653002/diff/1/extensions/common/manifest_h... extensions/common/manifest_handlers/options_page_info.h:35: static bool ChromeStyle(const Extension* extension); On 2014/08/29 16:52:22, ericzeng wrote: > On 2014/08/29 05:39:25, kalman wrote: > > IsChromeStyle? > > I thought that just naming the method the same thing as the field would be > easiest, I would also prefer something that captured the intent like > "ShouldUseChromeStyle" Changed to ShouldUseChromeStyle as discussed in person.
Basically lg. https://codereview.chromium.org/518653002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/518653002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_tab_util.cc:571: if (FeatureSwitch::embedded_extension_options()->IsEnabled() && Remove this FeatureSwitch check? https://codereview.chromium.org/518653002/diff/20001/chrome/common/extensions... File chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc (right): https://codereview.chromium.org/518653002/diff/20001/chrome/common/extensions... chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc:94: TEST_F(OptionsPageManifestTest, OptionsUIChromeStyleAndOpenInTabNoFlag) { You should ASSERT_TRUE that the flag is actually off here. https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... File extensions/common/manifest_handlers/options_page_info.cc (right): https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.cc:57: GURL absolute(url_string); Inline this? if (GURL(url_string).is_valid()) { .. } https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.cc:111: return info->chrome_styles_; You could do return info && info->chrome_styles_; https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.cc:119: return info->open_in_tab_; return info && info->open_in_tab_; https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.cc:127: base::string16* error) { I wouldn't even pass |error| in here, to avoid the temptation to use it. Only set warnings. When you need an error like in the OptionsUI::FromValue(..) call create a local variable and create an install warning from it (like you already do, kind of). https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.cc:131: bool open_in_tab = !FeatureSwitch::embedded_extension_options()->IsEnabled(); I think it would be simpler to initialise this to its default value (true). Then change the condition below to if (options_ui_value && FeatureSwitch::embedded_extension_options()...) { ... } https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.cc:150: open_in_tab |= options_ui->open_in_tab.get() && *options_ui->open_in_tab; All this logic can be simplified if open_in_tab were initialized to true and the outer condition check the FeatureSwitch. https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.cc:153: errors::kInvalidOptionsPage, keys::kOptionsUI))); I think that the OptionsUI::FromValue call generates an error for you? So you could do (above) like: std::string error; options_ui = OptionsUI::FromValue(*options_ui_value, &error); if (!error.empty()) { // OptionsUI::FromValue populates |error| both when there are // errors (in which case |options_ui| will be NULL) and warnings // (in which case |options_ui| will be valid). Either way, show it // as an install warning. install_warnings->push_back(InstallWarning(error)); } if (options_ui) { ... } https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.cc:192: !manifest->Get(keys::kOptionsUI, &options_ui_value)) { Like we discussed (i.e. like you noticed) the HasPath() check here is unnecessary given that getting a Value can't possibly be because it's an invalid type. So it'll only return false if it's not actually there. In fact this error handling case shouldn't even be necessary? You can just Get the manifest key and ignore the result. You might even want to use the ignore_result function (https://code.google.com/p/chromium/codesearch#chromium/src/base/macros.h&q=ig...) const base::Value* options_ui_value = NULL; // There might not be an "options_ui" key if there was an "options_page" key. ignore_result(manifest->Get(keys::kOptionsUI, &options_ui_value)); https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... File extensions/common/manifest_handlers/options_page_info.h (right): https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.h:32: static bool HasOptionsPage(const Extension* extension); Comment. https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.h:42: static scoped_ptr<OptionsPageInfo> Parse( Oh hey. Now that I'm reading this in daylight, it should be called Create() because it's creating a new OptionsPageInfo instance.
Mostly nits. https://chromiumcodereview.appspot.com/518653002/diff/20001/chrome/common/ext... File chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc (right): https://chromiumcodereview.appspot.com/518653002/diff/20001/chrome/common/ext... chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc:48: // Tests for the options_ui.page manifest field nit: comments should end in . https://chromiumcodereview.appspot.com/518653002/diff/20001/extensions/common... File extensions/common/api/_manifest_features.json (right): https://chromiumcodereview.appspot.com/518653002/diff/20001/extensions/common... extensions/common/api/_manifest_features.json:100: "extension_types": ["extension"] Do we want legacy_packaged_app too? Those also show up in chrome://extensions. https://chromiumcodereview.appspot.com/518653002/diff/20001/extensions/common... File extensions/common/manifest_handlers/options_page_info.cc (right): https://chromiumcodereview.appspot.com/518653002/diff/20001/extensions/common... extensions/common/manifest_handlers/options_page_info.cc:57: GURL absolute(url_string); Add a comment here along the lines of: Otherwise, the options URL should be inside the extension. https://chromiumcodereview.appspot.com/518653002/diff/20001/extensions/common... extensions/common/manifest_handlers/options_page_info.cc:79: : options_page_(options_page), Do we really need to store both options_page and options_ui_page if only one of them will ever be returned? The info struct, as a parsed representation, doesn't need to exactly match the manifest data. https://chromiumcodereview.appspot.com/518653002/diff/20001/extensions/common... extensions/common/manifest_handlers/options_page_info.cc:141: error, Why not pass in a different string from error, so you don't have to overwrite and then clear it? https://chromiumcodereview.appspot.com/518653002/diff/20001/extensions/common... extensions/common/manifest_handlers/options_page_info.cc:143: // Use an install warning instead of an error for options_ui.page nit: comments end in . https://chromiumcodereview.appspot.com/518653002/diff/20001/extensions/common... extensions/common/manifest_handlers/options_page_info.cc:150: open_in_tab |= options_ui->open_in_tab.get() && *options_ui->open_in_tab; Why does this look different from the chrome_style handling? They seem to be about the same. https://chromiumcodereview.appspot.com/518653002/diff/20001/extensions/common... extensions/common/manifest_handlers/options_page_info.cc:157: // Parse the options_page entry Say "legacy" in here https://chromiumcodereview.appspot.com/518653002/diff/20001/extensions/common... File extensions/common/manifest_handlers/options_page_info.h (right): https://chromiumcodereview.appspot.com/518653002/diff/20001/extensions/common... extensions/common/manifest_handlers/options_page_info.h:50: bool LoadOptionsPage(const Extension* extension, base::string16* error); This looks unused.
https://chromiumcodereview.appspot.com/518653002/diff/20001/extensions/common... File extensions/common/api/_manifest_features.json (right): https://chromiumcodereview.appspot.com/518653002/diff/20001/extensions/common... extensions/common/api/_manifest_features.json:100: "extension_types": ["extension"] On 2014/08/29 23:06:27, Yoyo Zhou wrote: > Do we want legacy_packaged_app too? Those also show up in chrome://extensions. I was thinking no, since we don't want to give legacy packaged apps any new features. However now that I think about it... perhaps this should be an exception. It's a bit mean to potentially break legacy packaged apps without the ability for them to opt out. The deprecation process can happen elsewhere.
https://codereview.chromium.org/518653002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/518653002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_tab_util.cc:571: if (FeatureSwitch::embedded_extension_options()->IsEnabled() && On 2014/08/29 22:33:37, kalman wrote: > Remove this FeatureSwitch check? Done. https://codereview.chromium.org/518653002/diff/20001/chrome/common/extensions... File chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc (right): https://codereview.chromium.org/518653002/diff/20001/chrome/common/extensions... chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc:94: TEST_F(OptionsPageManifestTest, OptionsUIChromeStyleAndOpenInTabNoFlag) { On 2014/08/29 22:33:37, kalman wrote: > You should ASSERT_TRUE that the flag is actually off here. Done. https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... File extensions/common/manifest_handlers/options_page_info.cc (right): https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.cc:57: GURL absolute(url_string); On 2014/08/29 22:33:37, kalman wrote: > Inline this? > > if (GURL(url_string).is_valid()) { .. } Done. https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.cc:111: return info->chrome_styles_; On 2014/08/29 22:33:37, kalman wrote: > You could do > > return info && info->chrome_styles_; Done. https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.cc:119: return info->open_in_tab_; On 2014/08/29 22:33:37, kalman wrote: > return info && info->open_in_tab_; Done. https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.cc:127: base::string16* error) { On 2014/08/29 22:33:37, kalman wrote: > I wouldn't even pass |error| in here, to avoid the temptation to use it. Only > set warnings. When you need an error like in the OptionsUI::FromValue(..) call > create a local variable and create an install warning from it (like you already > do, kind of). I would do that but since options_page is also handled in here, I still need to support errors. https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.cc:131: bool open_in_tab = !FeatureSwitch::embedded_extension_options()->IsEnabled(); On 2014/08/29 22:33:37, kalman wrote: > I think it would be simpler to initialise this to its default value (true). > > Then change the condition below to > > if (options_ui_value && FeatureSwitch::embedded_extension_options()...) { > ... > } Done. https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.cc:150: open_in_tab |= options_ui->open_in_tab.get() && *options_ui->open_in_tab; On 2014/08/29 22:33:37, kalman wrote: > All this logic can be simplified if open_in_tab were initialized to true and the > outer condition check the FeatureSwitch. Done. https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.cc:153: errors::kInvalidOptionsPage, keys::kOptionsUI))); On 2014/08/29 22:33:37, kalman wrote: > I think that the OptionsUI::FromValue call generates an error for you? So you > could do (above) like: > > std::string error; > options_ui = OptionsUI::FromValue(*options_ui_value, &error); > if (!error.empty()) { > // OptionsUI::FromValue populates |error| both when there are > // errors (in which case |options_ui| will be NULL) and warnings > // (in which case |options_ui| will be valid). Either way, show it > // as an install warning. > install_warnings->push_back(InstallWarning(error)); > } > if (options_ui) { > ... > } Done. https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.cc:192: !manifest->Get(keys::kOptionsUI, &options_ui_value)) { On 2014/08/29 22:33:37, kalman wrote: > Like we discussed (i.e. like you noticed) the HasPath() check here is > unnecessary given that getting a Value can't possibly be because it's an invalid > type. So it'll only return false if it's not actually there. > > In fact this error handling case shouldn't even be necessary? You can just Get > the manifest key and ignore the result. You might even want to use the > ignore_result function > (https://code.google.com/p/chromium/codesearch#chromium/src/base/macros.h&q=ig...) > > const base::Value* options_ui_value = NULL; > // There might not be an "options_ui" key if there was an "options_page" key. > ignore_result(manifest->Get(keys::kOptionsUI, &options_ui_value)); Done. https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... File extensions/common/manifest_handlers/options_page_info.h (right): https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.h:32: static bool HasOptionsPage(const Extension* extension); On 2014/08/29 22:33:37, kalman wrote: > Comment. Done. https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.h:42: static scoped_ptr<OptionsPageInfo> Parse( On 2014/08/29 22:33:37, kalman wrote: > Oh hey. Now that I'm reading this in daylight, it should be called Create() > because it's creating a new OptionsPageInfo instance. Done.
lgtm https://codereview.chromium.org/518653002/diff/40001/extensions/common/manife... File extensions/common/manifest_handlers/options_page_info.cc (right): https://codereview.chromium.org/518653002/diff/40001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.cc:139: &options_ui_error, This overrides any existing value for |options_ui_error|. I think it's better to show both if there are 2 errors. I like the code I wrote below; do the ::FromValue, add a warning if there was a warning, continue parsing if there was a value.
https://codereview.chromium.org/518653002/diff/20001/chrome/common/extensions... File chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc (right): https://codereview.chromium.org/518653002/diff/20001/chrome/common/extensions... chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc:48: // Tests for the options_ui.page manifest field On 2014/08/29 23:06:27, Yoyo Zhou wrote: > nit: comments should end in . Done. https://codereview.chromium.org/518653002/diff/20001/extensions/common/api/_m... File extensions/common/api/_manifest_features.json (right): https://codereview.chromium.org/518653002/diff/20001/extensions/common/api/_m... extensions/common/api/_manifest_features.json:100: "extension_types": ["extension"] On 2014/08/29 23:26:42, kalman (OOO until 6th Oct) wrote: > On 2014/08/29 23:06:27, Yoyo Zhou wrote: > > Do we want legacy_packaged_app too? Those also show up in chrome://extensions. > > I was thinking no, since we don't want to give legacy packaged apps any new > features. However now that I think about it... perhaps this should be an > exception. It's a bit mean to potentially break legacy packaged apps without the > ability for them to opt out. The deprecation process can happen elsewhere. Adding legacy packaged apps to the list. https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... File extensions/common/manifest_handlers/options_page_info.cc (right): https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.cc:79: : options_page_(options_page), On 2014/08/29 23:06:27, Yoyo Zhou wrote: > Do we really need to store both options_page and options_ui_page if only one of > them will ever be returned? The info struct, as a parsed representation, doesn't > need to exactly match the manifest data. As is I don't think both need to be stored. I thought that there might be a case where we would want to check the legacy options_page field, so I put it in the class, but so far I haven't encountered one. https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.cc:141: error, On 2014/08/29 23:06:27, Yoyo Zhou wrote: > Why not pass in a different string from error, so you don't have to overwrite > and then clear it? Done. https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.cc:150: open_in_tab |= options_ui->open_in_tab.get() && *options_ui->open_in_tab; On 2014/08/29 23:06:27, Yoyo Zhou wrote: > Why does this look different from the chrome_style handling? They seem to be > about the same. I changed how both booleans are handled, but they were different because chrome_style is false in all cases by default, while open_in_tab is false with the switch on, and true with the switch off. https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.cc:157: // Parse the options_page entry On 2014/08/29 23:06:27, Yoyo Zhou wrote: > Say "legacy" in here Done. https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... File extensions/common/manifest_handlers/options_page_info.h (right): https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.h:50: bool LoadOptionsPage(const Extension* extension, base::string16* error); On 2014/08/29 23:06:27, Yoyo Zhou wrote: > This looks unused. Done. https://codereview.chromium.org/518653002/diff/40001/extensions/common/manife... File extensions/common/manifest_handlers/options_page_info.cc (right): https://codereview.chromium.org/518653002/diff/40001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.cc:139: &options_ui_error, On 2014/08/30 00:48:11, kalman (OOO until 6th Oct) wrote: > This overrides any existing value for |options_ui_error|. I think it's better to > show both if there are 2 errors. > > I like the code I wrote below; do the ::FromValue, add a warning if there was a > warning, continue parsing if there was a value. Done.
https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... File extensions/common/manifest_handlers/options_page_info.cc (right): https://codereview.chromium.org/518653002/diff/20001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.cc:131: bool open_in_tab = !FeatureSwitch::embedded_extension_options()->IsEnabled(); On 2014/08/29 23:54:03, ericzeng wrote: > On 2014/08/29 22:33:37, kalman wrote: > > I think it would be simpler to initialise this to its default value (true). > > > > Then change the condition below to > > > > if (options_ui_value && FeatureSwitch::embedded_extension_options()...) { > > ... > > } > > Done. I just realized that this doesn't work, because it couples the feature with the existence of the options_ui field. Basically it makes the popup an opt-out feature instead of an opt-in feature.
https://codereview.chromium.org/518653002/diff/100001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/518653002/diff/100001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:275: if (!extension.options_open_in_tab) { nit: no unix_hacker_style in js. https://codereview.chromium.org/518653002/diff/100001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/app_list_controller_delegate.cc (right): https://codereview.chromium.org/518653002/diff/100001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/app_list_controller_delegate.cc:162: !extensions::OptionsPageInfo::GetOptionsPage(extension).is_empty(); HasOptionsPage()? https://codereview.chromium.org/518653002/diff/100001/chrome/common/extension... File chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc (right): https://codereview.chromium.org/518653002/diff/100001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc:15: using extensions::FeatureSwitch; just chuck the whole file into namespace extensions https://codereview.chromium.org/518653002/diff/100001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc:29: EXPECT_TRUE(OptionsPageInfo::GetOptionsPage(extension.get()).is_empty()); !HasOptionsPage()? https://codereview.chromium.org/518653002/diff/100001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc:50: scoped_ptr<FeatureSwitch::ScopedOverride> enable_flag( No need for a scoped_ptr here. https://codereview.chromium.org/518653002/diff/100001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc:56: extension = LoadAndExpectSuccess("options_ui_page_basic.json"); inline: scoped_refptr<const Extension> extension = LoadAndExpectSuccess("..."); https://codereview.chromium.org/518653002/diff/100001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc:71: // Tests for the options_ui.chrome_style and options_ui.open_in_tab fields nit: period. https://codereview.chromium.org/518653002/diff/100001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc:87: // If chrome_style and open_in_tab are not set, they should be false nit: period. https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... File extensions/common/manifest_handlers/options_page_info.cc (right): https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:8: #include "base/lazy_instance.h" need? https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:89: if (!info) { nit: no brackets https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:90: return GURL::EmptyGURL(); better yet, return info ? info->options_url_ : GURL::EmptyGURL(); Might be worth briefly combing through the uses, and see if EmptyGURL is actually necessary (instead of just GURL). WDYT, Yoyo? https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:121: bool open_in_tab = !FeatureSwitch::embedded_extension_options()->IsEnabled(); Do we want to default this to true with the switch on? https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:130: if (options_ui_error.empty()) { Do you mean if (!empty())? https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:156: if (!options_page_string.empty()) { Why not instead do something like: GURL options_page; if (options_ui_value && switch->enabled()) options_page = <parse> if (!options_page.is_valid()) options_page = <parse> Then options page will already be what you want, and you only parse the legacy if you need to. https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:223: extensions::file_util::ExtensionURLToRelativeFilePath( remove extensions:: prefix. https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... File extensions/common/manifest_handlers/options_page_info.h (right): https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.h:11: #include "base/values.h" forward declare values needed. https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.h:12: #include "extensions/common/extension.h" Also forward declare https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.h:31: // Returns true if the given extension has an options page, false otherwise. nit: omit "false otherwise" https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.h:56: bool chrome_styles_; nit: at minimum, newlines between these. https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.h:57: bool open_in_tab_; DISALLOW_COPY_AND_ASSIGN? Or do you expect to be copying/assigning? https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.h:60: // Parses the "options_ui" manifest key. And the legacy "options_page" key
https://chromiumcodereview.appspot.com/518653002/diff/80001/extensions/common... File extensions/common/manifest_handlers/options_page_info.cc (right): https://chromiumcodereview.appspot.com/518653002/diff/80001/extensions/common... extensions/common/manifest_handlers/options_page_info.cc:131: // OptionsUI::FromValue populates |error| both when there are |error| -> |options_ui_error| This is somewhat surprising. How can we have both warnings and a valid options_ui? https://chromiumcodereview.appspot.com/518653002/diff/80001/extensions/common... extensions/common/manifest_handlers/options_page_info.cc:166: // Prefer the options_ui.page url over the options_page url. nit: URL https://chromiumcodereview.appspot.com/518653002/diff/80001/extensions/common... File extensions/common/manifest_handlers/options_page_info.h (right): https://chromiumcodereview.appspot.com/518653002/diff/80001/extensions/common... extensions/common/manifest_handlers/options_page_info.h:52: // The url to the options page of this extension. We only store one options nit: URL https://chromiumcodereview.appspot.com/518653002/diff/100001/extensions/commo... File extensions/common/manifest_handlers/options_page_info.cc (right): https://chromiumcodereview.appspot.com/518653002/diff/100001/extensions/commo... extensions/common/manifest_handlers/options_page_info.cc:90: return GURL::EmptyGURL(); On 2014/09/02 21:42:30, Devlin wrote: > better yet, > return info ? info->options_url_ : GURL::EmptyGURL(); > > Might be worth briefly combing through the uses, and see if EmptyGURL is > actually necessary (instead of just GURL). WDYT, Yoyo? Hmm, // This object is for callers // who return references but don't have anything to return in some cases. Are you referring to something else?
https://chromiumcodereview.appspot.com/518653002/diff/100001/extensions/commo... File extensions/common/manifest_handlers/options_page_info.cc (right): https://chromiumcodereview.appspot.com/518653002/diff/100001/extensions/commo... extensions/common/manifest_handlers/options_page_info.cc:90: return GURL::EmptyGURL(); On 2014/09/02 21:54:35, Yoyo Zhou wrote: > On 2014/09/02 21:42:30, Devlin wrote: > > better yet, > > return info ? info->options_url_ : GURL::EmptyGURL(); > > > > Might be worth briefly combing through the uses, and see if EmptyGURL is > > actually necessary (instead of just GURL). WDYT, Yoyo? > > Hmm, > // This object is for callers > // who return references but don't have anything to return in some cases. > > Are you referring to something else? Nope, that's it. Just that, IIRC, C++ extends the life of a temporary to the current scope, so something like: const GURL& GetURL() { return GURL(); } void Foo() { const GURL& url = GetURL(); <do_stuff_with_url> } is safe, even though |url| doesn't really have an owning object. That, coupled with the impressive warnings on the similar EmptyString() functions, make me wonder if we should be returning EmptyGURL() vs GURL() here. Not sure if the risk of someone actually keeping around a GURL reference is better or worse than the extra overhead for the EmptyGURL() call.
https://chromiumcodereview.appspot.com/518653002/diff/100001/extensions/commo... File extensions/common/manifest_handlers/options_page_info.cc (right): https://chromiumcodereview.appspot.com/518653002/diff/100001/extensions/commo... extensions/common/manifest_handlers/options_page_info.cc:90: return GURL::EmptyGURL(); On 2014/09/02 22:08:19, Devlin wrote: > On 2014/09/02 21:54:35, Yoyo Zhou wrote: > > On 2014/09/02 21:42:30, Devlin wrote: > > > better yet, > > > return info ? info->options_url_ : GURL::EmptyGURL(); > > > > > > Might be worth briefly combing through the uses, and see if EmptyGURL is > > > actually necessary (instead of just GURL). WDYT, Yoyo? > > > > Hmm, > > // This object is for callers > > // who return references but don't have anything to return in some cases. > > > > Are you referring to something else? > > Nope, that's it. Just that, IIRC, C++ extends the life of a temporary to the > current scope, so something like: > > const GURL& GetURL() { > return GURL(); > } > > void Foo() { > const GURL& url = GetURL(); > <do_stuff_with_url> > } > > is safe, even though |url| doesn't really have an owning object. > > That, coupled with the impressive warnings on the similar EmptyString() > functions, make me wonder if we should be returning EmptyGURL() vs GURL() here. > Not sure if the risk of someone actually keeping around a GURL reference is > better or worse than the extra overhead for the EmptyGURL() call. Even so, none of the scary warnings in EmptyString apply here; this is exactly the situation where we would return EmptyString if it were a string.
https://chromiumcodereview.appspot.com/518653002/diff/100001/extensions/commo... File extensions/common/manifest_handlers/options_page_info.cc (right): https://chromiumcodereview.appspot.com/518653002/diff/100001/extensions/commo... extensions/common/manifest_handlers/options_page_info.cc:90: return GURL::EmptyGURL(); On 2014/09/02 22:10:29, Yoyo Zhou wrote: > On 2014/09/02 22:08:19, Devlin wrote: > > On 2014/09/02 21:54:35, Yoyo Zhou wrote: > > > On 2014/09/02 21:42:30, Devlin wrote: > > > > better yet, > > > > return info ? info->options_url_ : GURL::EmptyGURL(); > > > > > > > > Might be worth briefly combing through the uses, and see if EmptyGURL is > > > > actually necessary (instead of just GURL). WDYT, Yoyo? > > > > > > Hmm, > > > // This object is for callers > > > // who return references but don't have anything to return in some cases. > > > > > > Are you referring to something else? > > > > Nope, that's it. Just that, IIRC, C++ extends the life of a temporary to the > > current scope, so something like: > > > > const GURL& GetURL() { > > return GURL(); > > } > > > > void Foo() { > > const GURL& url = GetURL(); > > <do_stuff_with_url> > > } > > > > is safe, even though |url| doesn't really have an owning object. > > > > That, coupled with the impressive warnings on the similar EmptyString() > > functions, make me wonder if we should be returning EmptyGURL() vs GURL() > here. > > Not sure if the risk of someone actually keeping around a GURL reference is > > better or worse than the extra overhead for the EmptyGURL() call. > > Even so, none of the scary warnings in EmptyString apply here; this is exactly > the situation where we would return EmptyString if it were a string. Mmkay, just confirming. :)
https://chromiumcodereview.appspot.com/518653002/diff/80001/extensions/common... File extensions/common/manifest_handlers/options_page_info.cc (right): https://chromiumcodereview.appspot.com/518653002/diff/80001/extensions/common... extensions/common/manifest_handlers/options_page_info.cc:131: // OptionsUI::FromValue populates |error| both when there are On 2014/09/02 21:54:35, Yoyo Zhou wrote: > |error| -> |options_ui_error| > > This is somewhat surprising. How can we have both warnings and a valid > options_ui? This is better for forwards compatibility. Say we add a new (optional) property "awesome" in Chrome 41. Extensions start using it. We wouldn't want that to stop an extension from installing, just show a warning. https://chromiumcodereview.appspot.com/518653002/diff/100001/extensions/commo... File extensions/common/manifest_handlers/options_page_info.cc (right): https://chromiumcodereview.appspot.com/518653002/diff/100001/extensions/commo... extensions/common/manifest_handlers/options_page_info.cc:90: return GURL::EmptyGURL(); On 2014/09/02 22:14:57, Devlin wrote: > On 2014/09/02 22:10:29, Yoyo Zhou wrote: > > On 2014/09/02 22:08:19, Devlin wrote: > > > On 2014/09/02 21:54:35, Yoyo Zhou wrote: > > > > On 2014/09/02 21:42:30, Devlin wrote: > > > > > better yet, > > > > > return info ? info->options_url_ : GURL::EmptyGURL(); > > > > > > > > > > Might be worth briefly combing through the uses, and see if EmptyGURL is > > > > > actually necessary (instead of just GURL). WDYT, Yoyo? > > > > > > > > Hmm, > > > > // This object is for callers > > > > // who return references but don't have anything to return in some > cases. > > > > > > > > Are you referring to something else? > > > > > > Nope, that's it. Just that, IIRC, C++ extends the life of a temporary to > the > > > current scope, so something like: > > > > > > const GURL& GetURL() { > > > return GURL(); > > > } > > > > > > void Foo() { > > > const GURL& url = GetURL(); > > > <do_stuff_with_url> > > > } > > > > > > is safe, even though |url| doesn't really have an owning object. > > > > > > That, coupled with the impressive warnings on the similar EmptyString() > > > functions, make me wonder if we should be returning EmptyGURL() vs GURL() > > here. > > > Not sure if the risk of someone actually keeping around a GURL reference is > > > better or worse than the extra overhead for the EmptyGURL() call. > > > > Even so, none of the scary warnings in EmptyString apply here; this is exactly > > the situation where we would return EmptyString if it were a string. > > Mmkay, just confirming. :) Yes this is exactly what GURL::EmptyGURL is for, it allows the caller to assign to a const& rather than needing to take a copy.
https://codereview.chromium.org/518653002/diff/80001/extensions/common/manife... File extensions/common/manifest_handlers/options_page_info.cc (right): https://codereview.chromium.org/518653002/diff/80001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.cc:166: // Prefer the options_ui.page url over the options_page url. On 2014/09/02 21:54:35, Yoyo Zhou wrote: > nit: URL This block was removed per Devlin's comments. https://codereview.chromium.org/518653002/diff/80001/extensions/common/manife... File extensions/common/manifest_handlers/options_page_info.h (right): https://codereview.chromium.org/518653002/diff/80001/extensions/common/manife... extensions/common/manifest_handlers/options_page_info.h:52: // The url to the options page of this extension. We only store one options On 2014/09/02 21:54:35, Yoyo Zhou wrote: > nit: URL Done. https://codereview.chromium.org/518653002/diff/100001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/518653002/diff/100001/chrome/browser/resource... chrome/browser/resources/extensions/extension_list.js:275: if (!extension.options_open_in_tab) { On 2014/09/02 21:42:29, Devlin wrote: > nit: no unix_hacker_style in js. Done. https://codereview.chromium.org/518653002/diff/100001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/app_list_controller_delegate.cc (right): https://codereview.chromium.org/518653002/diff/100001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/app_list_controller_delegate.cc:162: !extensions::OptionsPageInfo::GetOptionsPage(extension).is_empty(); On 2014/09/02 21:42:29, Devlin wrote: > HasOptionsPage()? Done. https://codereview.chromium.org/518653002/diff/100001/chrome/common/extension... File chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc (right): https://codereview.chromium.org/518653002/diff/100001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc:15: using extensions::FeatureSwitch; On 2014/09/02 21:42:29, Devlin wrote: > just chuck the whole file into namespace extensions Done. https://codereview.chromium.org/518653002/diff/100001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc:29: EXPECT_TRUE(OptionsPageInfo::GetOptionsPage(extension.get()).is_empty()); On 2014/09/02 21:42:29, Devlin wrote: > !HasOptionsPage()? Done. https://codereview.chromium.org/518653002/diff/100001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc:50: scoped_ptr<FeatureSwitch::ScopedOverride> enable_flag( On 2014/09/02 21:42:29, Devlin wrote: > No need for a scoped_ptr here. Done. https://codereview.chromium.org/518653002/diff/100001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc:56: extension = LoadAndExpectSuccess("options_ui_page_basic.json"); On 2014/09/02 21:42:29, Devlin wrote: > inline: > scoped_refptr<const Extension> extension = LoadAndExpectSuccess("..."); Done. https://codereview.chromium.org/518653002/diff/100001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc:71: // Tests for the options_ui.chrome_style and options_ui.open_in_tab fields On 2014/09/02 21:42:29, Devlin wrote: > nit: period. Done. https://codereview.chromium.org/518653002/diff/100001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifests_options_unittest.cc:87: // If chrome_style and open_in_tab are not set, they should be false On 2014/09/02 21:42:29, Devlin wrote: > nit: period. Done. https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... File extensions/common/manifest_handlers/options_page_info.cc (right): https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:8: #include "base/lazy_instance.h" On 2014/09/02 21:42:29, Devlin wrote: > need? Not anymore https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:89: if (!info) { On 2014/09/02 21:42:29, Devlin wrote: > nit: no brackets Done. https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:121: bool open_in_tab = !FeatureSwitch::embedded_extension_options()->IsEnabled(); On 2014/09/02 21:42:30, Devlin wrote: > Do we want to default this to true with the switch on? I believe so, Ben and I discussed that we wanted chrome styles to be opt-in in all cases, but the popup to be opt-out. https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:130: if (options_ui_error.empty()) { On 2014/09/02 21:42:30, Devlin wrote: > Do you mean if (!empty())? Oops, yes https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:156: if (!options_page_string.empty()) { On 2014/09/02 21:42:30, Devlin wrote: > Why not instead do something like: > GURL options_page; > if (options_ui_value && switch->enabled()) > options_page = <parse> > > if (!options_page.is_valid()) > options_page = <parse> > > Then options page will already be what you want, and you only parse the legacy > if you need to. Done. https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:223: extensions::file_util::ExtensionURLToRelativeFilePath( On 2014/09/02 21:42:30, Devlin wrote: > remove extensions:: prefix. Done. https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... File extensions/common/manifest_handlers/options_page_info.h (right): https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.h:11: #include "base/values.h" On 2014/09/02 21:42:30, Devlin wrote: > forward declare values needed. Done. https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.h:12: #include "extensions/common/extension.h" On 2014/09/02 21:42:30, Devlin wrote: > Also forward declare Done. https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.h:31: // Returns true if the given extension has an options page, false otherwise. On 2014/09/02 21:42:30, Devlin wrote: > nit: omit "false otherwise" Done. https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.h:56: bool chrome_styles_; On 2014/09/02 21:42:30, Devlin wrote: > nit: at minimum, newlines between these. Done. https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.h:57: bool open_in_tab_; On 2014/09/02 21:42:30, Devlin wrote: > DISALLOW_COPY_AND_ASSIGN? Or do you expect to be copying/assigning? Done. https://codereview.chromium.org/518653002/diff/100001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.h:60: // Parses the "options_ui" manifest key. On 2014/09/02 21:42:30, Devlin wrote: > And the legacy "options_page" key Done.
LGTM https://chromiumcodereview.appspot.com/518653002/diff/120001/extensions/commo... File extensions/common/manifest_handlers/options_page_info.cc (right): https://chromiumcodereview.appspot.com/518653002/diff/120001/extensions/commo... extensions/common/manifest_handlers/options_page_info.cc:88: if (!info) Use ternary operator as Devlin suggests.
https://codereview.chromium.org/518653002/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/518653002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_tab_util.cc:572: // If embedded extension options are enabled, open chrome://extensions nit: tweak the wording slightly (something more like "If the extension opted for embedded options" or "If we should embed the options page for the extension"). https://codereview.chromium.org/518653002/diff/120001/extensions/common/api/_... File extensions/common/api/_manifest_features.json (right): https://codereview.chromium.org/518653002/diff/120001/extensions/common/api/_... extensions/common/api/_manifest_features.json:100: "extension_types": ["extension", "legacy_packaged_app"] no hosted app? I would expect everything with an options page to be able to use options_ui. https://codereview.chromium.org/518653002/diff/120001/extensions/common/manif... File extensions/common/manifest_handlers/options_page_info.cc (right): https://codereview.chromium.org/518653002/diff/120001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:24: namespace values = manifest_values; Is this used? https://codereview.chromium.org/518653002/diff/120001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:88: if (!info) inline: info ? info->options_page_ : GURL::EmptyGURL(); https://codereview.chromium.org/518653002/diff/120001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:118: bool open_in_tab = !FeatureSwitch::embedded_extension_options()->IsEnabled(); This looks wrong. We default to opening in a tab *unless* the switch is on, in which case we default to not? https://codereview.chromium.org/518653002/diff/120001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:148: open_in_tab = options_ui->open_in_tab.get() && *options_ui->open_in_tab; This will override your default set above, because it will always be false if options_ui doesn't specify an open_in_tab value. https://codereview.chromium.org/518653002/diff/120001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:199: extension->AddInstallWarnings(install_warnings); Do we want an install warning if the extension specifies both options_page and options_ui? https://codereview.chromium.org/518653002/diff/120001/extensions/common/manif... File extensions/common/manifest_handlers/options_page_info.h (right): https://codereview.chromium.org/518653002/diff/120001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.h:22: class Extension; If you include the .h, you don't need to forward-declare. :)
https://codereview.chromium.org/518653002/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/extension_tab_util.cc (right): https://codereview.chromium.org/518653002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/extension_tab_util.cc:572: // If embedded extension options are enabled, open chrome://extensions On 2014/09/04 21:16:04, Devlin wrote: > nit: tweak the wording slightly (something more like "If the extension opted for > embedded options" or "If we should embed the options page for the extension"). Done. https://codereview.chromium.org/518653002/diff/120001/extensions/common/api/_... File extensions/common/api/_manifest_features.json (right): https://codereview.chromium.org/518653002/diff/120001/extensions/common/api/_... extensions/common/api/_manifest_features.json:100: "extension_types": ["extension", "legacy_packaged_app"] On 2014/09/04 21:16:04, Devlin wrote: > no hosted app? I would expect everything with an options page to be able to use > options_ui. Ben had some reason for keeping legacy_packaged_app and hosted_app out but I don't really remember. Also, looking at the hosted app documentation, it looks like options_page isn't an valid entry? https://codereview.chromium.org/518653002/diff/120001/extensions/common/manif... File extensions/common/manifest_handlers/options_page_info.cc (right): https://codereview.chromium.org/518653002/diff/120001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:24: namespace values = manifest_values; On 2014/09/04 21:16:04, Devlin wrote: > Is this used? No, must have left it there when refactoring in the OptionsPageHandler from ManifestURL. https://codereview.chromium.org/518653002/diff/120001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:88: if (!info) On 2014/09/04 21:16:04, Devlin wrote: > inline: > info ? info->options_page_ : GURL::EmptyGURL(); Done. https://codereview.chromium.org/518653002/diff/120001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:118: bool open_in_tab = !FeatureSwitch::embedded_extension_options()->IsEnabled(); On 2014/09/04 21:16:04, Devlin wrote: > This looks wrong. We default to opening in a tab *unless* the switch is on, in > which case we default to not? So if the embedded extension options flag is turned off, options pages should be opened in a tab as usual, so it this should be true. If embedded extension options is turned on, we want the popup UI to be an opt-out feature, so this should be false by default. I think all of the FeatureSwitch logic for the embedded options page has been refactored into OptionsPageInfo::ShouldOpenInTab, rather than having checks everywhere options pages are opened, so I think this is the correct way to handle it. https://codereview.chromium.org/518653002/diff/120001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:148: open_in_tab = options_ui->open_in_tab.get() && *options_ui->open_in_tab; On 2014/09/04 21:16:04, Devlin wrote: > This will override your default set above, because it will always be false if > options_ui doesn't specify an open_in_tab value. I think that is the intended behavior, since we want the popup UI to be opt-out. https://codereview.chromium.org/518653002/diff/120001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:199: extension->AddInstallWarnings(install_warnings); On 2014/09/04 21:16:04, Devlin wrote: > Do we want an install warning if the extension specifies both options_page and > options_ui? Maybe not? An extension might want to keep the old options_page around for older versions of Chrome. Also, an extension could have different values for options_page and options_ui.page, the options_page value would be a tabbed options page for old Chrome versions, and option_ui.page would be an options page that works with the popup. https://codereview.chromium.org/518653002/diff/120001/extensions/common/manif... File extensions/common/manifest_handlers/options_page_info.h (right): https://codereview.chromium.org/518653002/diff/120001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.h:22: class Extension; On 2014/09/04 21:16:04, Devlin wrote: > If you include the .h, you don't need to forward-declare. :) Done.
https://codereview.chromium.org/518653002/diff/120001/extensions/common/manif... File extensions/common/manifest_handlers/options_page_info.cc (right): https://codereview.chromium.org/518653002/diff/120001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:118: bool open_in_tab = !FeatureSwitch::embedded_extension_options()->IsEnabled(); On 2014/09/04 22:22:07, ericzeng wrote: > On 2014/09/04 21:16:04, Devlin wrote: > > This looks wrong. We default to opening in a tab *unless* the switch is on, > in > > which case we default to not? > > So if the embedded extension options flag is turned off, options pages should be > opened in a tab as usual, so it this should be true. If embedded extension > options is turned on, we want the popup UI to be an opt-out feature, so this > should be false by default. > > I think all of the FeatureSwitch logic for the embedded options page has been > refactored into OptionsPageInfo::ShouldOpenInTab, rather than having checks > everywhere options pages are opened, so I think this is the correct way to > handle it. Apparently I'm not thinking straight today - the whole time doing this review, I was interpreting "open_in_tab" as "open_embedded". https://codereview.chromium.org/518653002/diff/120001/extensions/common/manif... extensions/common/manifest_handlers/options_page_info.cc:148: open_in_tab = options_ui->open_in_tab.get() && *options_ui->open_in_tab; On 2014/09/04 22:22:07, ericzeng wrote: > On 2014/09/04 21:16:04, Devlin wrote: > > This will override your default set above, because it will always be false if > > options_ui doesn't specify an open_in_tab value. > > I think that is the intended behavior, since we want the popup UI to be opt-out. > Right, see above.
lgtm
guest_view lgtm
ericzeng@chromium.org changed reviewers: + atwilson@chromium.org, avi@chromium.org, benwells@chromium.org
atwilson@chromium.org: Please review changes in chrome/browser/background/background_mode_manager.cc avi@chromium.org: Please review changes in chrome/browser/ui/cocoa/extensions/extension_action_context_menu_controller.mm benwells@chromium.org: Please review changes in chrome/browser/ui/extensions/application_launch.cc and chrome/browser/ui/app_list/app_list_controller_delegate.cc I've replaced instances of ManifestURL::GetOptionsPage in these files with OptionsPageInfo::GetOptionsPage - everything should work the same as before.
chrome/browser/ui/cocoa LGTM
ping benwells/atwilson
ericzeng@chromium.org changed reviewers: + calamity@chromium.org - atwilson@chromium.org
Swapping atwilson@ for calamity@
ericzeng@chromium.org changed reviewers: + atwilson@chromium.org
Or not... I misread the OWNERS file. calamity: could you still take a look at chrome/browser/ui/app_list/app_list_controller_delegate.cc?
background lgtm
c/b/ui lgtm
The CQ bit was checked by ericzeng@chromium.org
ericzeng@chromium.org changed reviewers: - calamity@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ericzeng@chromium.org/518653002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
The CQ bit was checked by ericzeng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ericzeng@chromium.org/518653002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
The CQ bit was checked by ericzeng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ericzeng@chromium.org/518653002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to commit the patch.
The CQ bit was checked by ericzeng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ericzeng@chromium.org/518653002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for chrome/app/generated_resources.grd:
While running git apply --index -p1;
error: patch failed: chrome/app/generated_resources.grd:4556
error: chrome/app/generated_resources.grd: patch does not apply
Patch: chrome/app/generated_resources.grd
Index: chrome/app/generated_resources.grd
diff --git a/chrome/app/generated_resources.grd
b/chrome/app/generated_resources.grd
index
7a96688df1b8d9c6558e310dbe79a879a6718ac1..78531cf5e3fd5a14bfaacb62e6d51e400d1c6032
100644
--- a/chrome/app/generated_resources.grd
+++ b/chrome/app/generated_resources.grd
@@ -4556,9 +4556,6 @@ Make sure you do not expose any sensitive information.
<message name="IDS_EXTENSION_LOAD_ICON_FOR_BROWSER_ACTION_FAILED"
desc="">
Could not load icon '<ph name="ICON">$1<ex>icon.png</ex></ph>' for
browser action.
</message>
- <message name="IDS_EXTENSION_LOAD_OPTIONS_PAGE_FAILED" desc="">
- Could not load options page '<ph
name="OPTIONS_PAGE">$1<ex>page.html</ex></ph>'.
- </message>
<message name="IDS_EXTENSION_LOAD_ABOUT_PAGE_FAILED" desc="">
Could not load about page '<ph
name="ABOUT_PAGE">$1<ex>page.html</ex></ph>'.
</message>
It looks like this was committed as https://chromium.googlesource.com/chromium/src.git/+/09a7e00df7e8316a01cda028... even though it says it couldn't apply the patch. That particular file looks ok though.
On 2014/09/09 23:21:04, ericzeng wrote: > It looks like this was committed as > https://chromium.googlesource.com/chromium/src.git/+/09a7e00df7e8316a01cda028... > even though it says it couldn't apply the patch. That particular file looks ok > though. Wow. That seems very wrong. I filed crbug.com/412609 for this.
Patchset 12 (id:??) landed as https://crrev.com/09a7e00df7e8316a01cda0288577a293d7d126a3 Cr-Commit-Position: refs/heads/master@{#294019} |
