Add the "options_ui" extension manifest field.

extensions/common/manifest_handlers/options_page_info.cc

Index: extensions/common/manifest_handlers/options_page_info.cc
diff --git a/extensions/common/manifest_handlers/options_page_info.cc b/extensions/common/manifest_handlers/options_page_info.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0d2d41a312d1094067d18d6afd544b74e2ac32d4
--- /dev/null
+++ b/extensions/common/manifest_handlers/options_page_info.cc
@@ -0,0 +1,236 @@
+// 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/lazy_instance.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;
+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;
+  }
+
+  GURL absolute(url_string);

[not at google - send to devlin, 2014/08/29 22:33:37]

Inline this?

if (GURL(url_string).is_valid()) { .. }

[Yoyo Zhou, 2014/08/29 23:06:27]

Add a comment here along the lines of: Otherwise, the options URL should be
inside the extension.

[ericzeng, 2014/08/29 23:54:04]

Done.

+  if (absolute.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,
+                                 const GURL& options_ui_page,
+                                 bool chrome_styles,
+                                 bool open_in_tab)
+    : options_page_(options_page),

[Yoyo Zhou, 2014/08/29 23:06:27]

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.

[ericzeng, 2014/09/02 18:23:37]

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. 

+      options_ui_page_(options_ui_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) {
+    return GURL::EmptyGURL();
+  }
+
+  if (info->options_ui_page_.is_valid()) {
+    return info->options_ui_page_;
+  }
+  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);
+  if (!info)
+    return false;
+  return info->chrome_styles_;

[not at google - send to devlin, 2014/08/29 22:33:37]

You could do

return info && info->chrome_styles_;

[ericzeng, 2014/08/29 23:54:04]

Done.

+}
+
+// static
+bool OptionsPageInfo::ShouldOpenInTab(const Extension* extension) {
+  OptionsPageInfo* info = GetOptionsPageInfo(extension);
+  if (!info)
+    return false;
+  return info->open_in_tab_;

[not at google - send to devlin, 2014/08/29 22:33:37]

return info && info->open_in_tab_;

[ericzeng, 2014/08/29 23:54:03]

Done.

+}
+
+scoped_ptr<OptionsPageInfo> OptionsPageInfo::Parse(
+    Extension* extension,
+    const base::Value* options_ui_value,
+    const std::string& options_page_string,
+    std::vector<InstallWarning>* install_warnings,
+    base::string16* error) {

[not at google - send to devlin, 2014/08/29 22:33:37]

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).

[ericzeng, 2014/08/29 23:54:04]

I would do that but since options_page is also handled in here, I still need to
support errors. 

+  GURL options_page;
+  GURL options_ui_page;
+  bool chrome_style = false;
+  bool open_in_tab = !FeatureSwitch::embedded_extension_options()->IsEnabled();

[not at google - send to devlin, 2014/08/29 22:33:37]

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()...) {
  ...
}

[ericzeng, 2014/08/29 23:54:03]

Done.

[ericzeng, 2014/09/02 21:08:46]

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.

+
+  // Parse the options_ui object
+  if (options_ui_value) {
+    scoped_ptr<OptionsUI> options_ui =
+        OptionsUI::FromValue(*options_ui_value, error);
+    if (options_ui) {
+      if (!ParseOptionsUrl(extension,
+                           options_ui->page,
+                           keys::kOptionsUI,
+                           error,

[Yoyo Zhou, 2014/08/29 23:06:27]

Why not pass in a different string from error, so you don't have to overwrite
and then clear it?

[ericzeng, 2014/09/02 18:23:37]

Done.

+                           &options_ui_page)) {
+        // Use an install warning instead of an error for options_ui.page

[Yoyo Zhou, 2014/08/29 23:06:27]

nit: comments end in .

+        install_warnings->push_back(InstallWarning(base::UTF16ToASCII(*error)));
+        *error = base::string16();
+      }
+      chrome_style = options_ui->chrome_style.get() &&
+                     *options_ui->chrome_style &&
+                     FeatureSwitch::embedded_extension_options()->IsEnabled();
+      open_in_tab |= options_ui->open_in_tab.get() && *options_ui->open_in_tab;

[not at google - send to devlin, 2014/08/29 22:33:37]

All this logic can be simplified if open_in_tab were initialized to true and the
outer condition check the FeatureSwitch.

[Yoyo Zhou, 2014/08/29 23:06:27]

Why does this look different from the chrome_style handling? They seem to be
about the same.

[ericzeng, 2014/08/29 23:54:04]

Done.

[ericzeng, 2014/09/02 18:23:37]

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. 

+    } else {
+      install_warnings->push_back(InstallWarning(ErrorUtils::FormatErrorMessage(
+          errors::kInvalidOptionsPage, keys::kOptionsUI)));

[not at google - send to devlin, 2014/08/29 22:33:37]

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) {
  ...
}

[ericzeng, 2014/08/29 23:54:03]

Done.

+    }
+  }
+
+  // Parse the options_page entry

[Yoyo Zhou, 2014/08/29 23:06:27]

Say "legacy" in here

[ericzeng, 2014/09/02 18:23:37]

Done.

+  if (!options_page_string.empty()) {
+    if (!ParseOptionsUrl(extension,
+                         options_page_string,
+                         keys::kOptionsPage,
+                         error,
+                         &options_page)) {
+      return scoped_ptr<OptionsPageInfo>();
+    }
+  }
+  return make_scoped_ptr(new OptionsPageInfo(
+      options_page, options_ui_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;
+  if (manifest->HasPath(keys::kOptionsUI) &&
+      !manifest->Get(keys::kOptionsUI, &options_ui_value)) {

[not at google - send to devlin, 2014/08/29 22:33:37]

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));

[ericzeng, 2014/08/29 23:54:04]

Done.

+    install_warnings.push_back(InstallWarning(ErrorUtils::FormatErrorMessage(
+        errors::kInvalidOptionsPage, keys::kOptionsUI)));
+  }
+
+  scoped_ptr<OptionsPageInfo> info = OptionsPageInfo::Parse(extension,
+                                                            options_ui_value,
+                                                            options_page_string,
+                                                            &install_warnings,
+                                                            error);
+  if (!info)
+    return false;
+
+  extension->AddInstallWarnings(install_warnings);
+  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 =
+      extensions::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