Add the "options_ui" extension manifest field.

extensions/common/manifest_handlers/options_page_info.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "extensions/common/manifest_handlers/options_page_info.h"
+
+#include "base/file_util.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/strings/utf_string_conversions.h"
+#include "extensions/common/api/extensions_manifest_types.h"
+#include "extensions/common/error_utils.h"
+#include "extensions/common/feature_switch.h"
+#include "extensions/common/file_util.h"
+#include "extensions/common/manifest_constants.h"
+#include "extensions/strings/grit/extensions_strings.h"
+#include "ui/base/l10n/l10n_util.h"
+
+using base::ASCIIToUTF16;
+using base::DictionaryValue;
+
+namespace extensions {
+
+namespace keys = manifest_keys;
+namespace values = manifest_values;

[Devlin, 2014/09/04 21:16:04]

Is this used?

[ericzeng, 2014/09/04 22:22:07]

No, must have left it there when refactoring in the OptionsPageHandler from
ManifestURL.

+namespace errors = manifest_errors;
+
+using core_api::extensions_manifest_types::OptionsUI;
+
+namespace {
+
+OptionsPageInfo* GetOptionsPageInfo(const Extension* extension) {
+  return static_cast<OptionsPageInfo*>(
+      extension->GetManifestData(keys::kOptionsUI));
+}
+
+// Parses |url_string| into a GURL |result| if it is a valid options page for
+// this app/extension. If not, it returns the reason in |error|. Because this
+// handles URLs for both "options_page" and "options_ui.page", the name of the
+// manifest field must be provided in |manifest_field_name|.
+bool ParseOptionsUrl(Extension* extension,
+                     const std::string& url_string,
+                     const std::string& manifest_field_name,
+                     base::string16* error,
+                     GURL* result) {
+  if (extension->is_hosted_app()) {
+    // Hosted apps require an absolute URL.
+    GURL options_url(url_string);
+    if (!options_url.is_valid() || !options_url.SchemeIsHTTPOrHTTPS()) {
+      *error = base::ASCIIToUTF16(errors::kInvalidOptionsPageInHostedApp);
+      return false;
+    }
+    *result = options_url;
+    return true;
+  }
+
+  // Otherwise the options URL should be inside the extension.
+  if (GURL(url_string).is_valid()) {
+    *error = base::ASCIIToUTF16(errors::kInvalidOptionsPageExpectUrlInPackage);
+    return false;
+  }
+
+  GURL resource_url = extension->GetResourceURL(url_string);
+  if (!resource_url.is_valid()) {
+    *error = ErrorUtils::FormatErrorMessageUTF16(errors::kInvalidOptionsPage,
+                                                 manifest_field_name);
+    return false;
+  }
+  *result = resource_url;
+  return true;
+}
+
+}  // namespace
+
+OptionsPageInfo::OptionsPageInfo(const GURL& options_page,
+                                 bool chrome_styles,
+                                 bool open_in_tab)
+    : options_page_(options_page),
+      chrome_styles_(chrome_styles),
+      open_in_tab_(open_in_tab) {
+}
+
+OptionsPageInfo::~OptionsPageInfo() {
+}
+
+// static
+const GURL& OptionsPageInfo::GetOptionsPage(const Extension* extension) {
+  OptionsPageInfo* info = GetOptionsPageInfo(extension);
+  if (!info)

[Yoyo Zhou, 2014/09/04 21:15:09]

Use ternary operator as Devlin suggests.

[Devlin, 2014/09/04 21:16:04]

inline:
info ? info->options_page_ : GURL::EmptyGURL();

[ericzeng, 2014/09/04 22:22:07]

Done.

+    return GURL::EmptyGURL();
+  return info->options_page_;
+}
+
+// static
+bool OptionsPageInfo::HasOptionsPage(const Extension* extension) {
+  return !OptionsPageInfo::GetOptionsPage(extension).is_empty();
+}
+
+// static
+bool OptionsPageInfo::ShouldUseChromeStyle(const Extension* extension) {
+  OptionsPageInfo* info = GetOptionsPageInfo(extension);
+  return info && info->chrome_styles_;
+}
+
+// static
+bool OptionsPageInfo::ShouldOpenInTab(const Extension* extension) {
+  OptionsPageInfo* info = GetOptionsPageInfo(extension);
+  return info && info->open_in_tab_;
+}
+
+scoped_ptr<OptionsPageInfo> OptionsPageInfo::Create(
+    Extension* extension,
+    const base::Value* options_ui_value,
+    const std::string& options_page_string,
+    std::vector<InstallWarning>* install_warnings,
+    base::string16* error) {
+  GURL options_page;
+  bool chrome_style = false;
+  bool open_in_tab = !FeatureSwitch::embedded_extension_options()->IsEnabled();

[Devlin, 2014/09/04 21:16:04]

This looks wrong.  We default to opening in a tab *unless* the switch is on, in
which case we default to not?

[ericzeng, 2014/09/04 22:22:07]

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. 

[Devlin, 2014/09/04 22:45:32]

Apparently I'm not thinking straight today - the whole time doing this review, I
was interpreting "open_in_tab" as "open_embedded".

+
+  // Parse the options_ui object.
+  if (options_ui_value &&
+      FeatureSwitch::embedded_extension_options()->IsEnabled()) {
+    base::string16 options_ui_error;
+
+    scoped_ptr<OptionsUI> options_ui =
+        OptionsUI::FromValue(*options_ui_value, &options_ui_error);
+    if (!options_ui_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(base::UTF16ToASCII(options_ui_error)));
+    }
+
+    if (options_ui) {
+      base::string16 options_parse_error;
+      if (!ParseOptionsUrl(extension,
+                           options_ui->page,
+                           keys::kOptionsUI,
+                           &options_parse_error,
+                           &options_page)) {
+        install_warnings->push_back(
+            InstallWarning(base::UTF16ToASCII(options_parse_error)));
+      }
+      chrome_style =
+          options_ui->chrome_style.get() && *options_ui->chrome_style;
+      open_in_tab = options_ui->open_in_tab.get() && *options_ui->open_in_tab;

[Devlin, 2014/09/04 21:16:04]

This will override your default set above, because it will always be false if
options_ui doesn't specify an open_in_tab value.

[ericzeng, 2014/09/04 22:22:07]

I think that is the intended behavior, since we want the popup UI to be opt-out.

[Devlin, 2014/09/04 22:45:32]

Right, see above.

+    }
+  }
+
+  // Parse the legacy options_page entry if there was no entry for
+  // options_ui.page.
+  if (!options_page_string.empty() && !options_page.is_valid()) {
+    if (!ParseOptionsUrl(extension,
+                         options_page_string,
+                         keys::kOptionsPage,
+                         error,
+                         &options_page)) {
+      return scoped_ptr<OptionsPageInfo>();
+    }
+  }
+
+  return make_scoped_ptr(
+      new OptionsPageInfo(options_page, chrome_style, open_in_tab));
+}
+
+OptionsPageManifestHandler::OptionsPageManifestHandler() {
+}
+
+OptionsPageManifestHandler::~OptionsPageManifestHandler() {
+}
+
+bool OptionsPageManifestHandler::Parse(Extension* extension,
+                                       base::string16* error) {
+  std::vector<InstallWarning> install_warnings;
+  const Manifest* manifest = extension->manifest();
+
+  std::string options_page_string;
+  if (manifest->HasPath(keys::kOptionsPage) &&
+      !manifest->GetString(keys::kOptionsPage, &options_page_string)) {
+    *error = ErrorUtils::FormatErrorMessageUTF16(errors::kInvalidOptionsPage,
+                                                 keys::kOptionsPage);
+    return false;
+  }
+
+  const base::Value* options_ui_value = NULL;
+  ignore_result(manifest->Get(keys::kOptionsUI, &options_ui_value));
+
+  scoped_ptr<OptionsPageInfo> info =
+      OptionsPageInfo::Create(extension,
+                              options_ui_value,
+                              options_page_string,
+                              &install_warnings,
+                              error);
+  if (!info)
+    return false;
+
+  extension->AddInstallWarnings(install_warnings);

[Devlin, 2014/09/04 21:16:04]

Do we want an install warning if the extension specifies both options_page and
options_ui?

[ericzeng, 2014/09/04 22:22:07]

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. 

+  extension->SetManifestData(keys::kOptionsUI, info.release());
+  return true;
+}
+
+bool OptionsPageManifestHandler::Validate(
+    const Extension* extension,
+    std::string* error,
+    std::vector<InstallWarning>* warnings) const {
+  // Validate path to the options page.  Don't check the URL for hosted apps,
+  // because they are expected to refer to an external URL.
+  if (!OptionsPageInfo::HasOptionsPage(extension) || extension->is_hosted_app())
+    return true;
+
+  base::FilePath options_path = file_util::ExtensionURLToRelativeFilePath(
+      OptionsPageInfo::GetOptionsPage(extension));
+  base::FilePath path = extension->GetResource(options_path).GetFilePath();
+  if (path.empty() || !base::PathExists(path)) {
+    *error = l10n_util::GetStringFUTF8(IDS_EXTENSION_LOAD_OPTIONS_PAGE_FAILED,
+                                       options_path.LossyDisplayName());
+    return false;
+  }
+  return true;
+}
+
+const std::vector<std::string> OptionsPageManifestHandler::Keys() const {
+  static const char* keys[] = {keys::kOptionsPage, keys::kOptionsUI};
+  return std::vector<std::string>(keys, keys + arraysize(keys));
+}
+
+}  // namespace extensions