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/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/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 {
+// 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
+// OptionsUrlType must be passed to this function to send the correct error
+// message. Returns true on success, false otherwise.
+bool ParseOptionsUrl(Extension* extension,
+                     std::string url_string,

[not at google - send to devlin, 2014/08/29 05:39:24]

const std::string&

[ericzeng, 2014/08/29 22:08:51]

Done.

+                     OptionsPageInfo::OptionsUrlType url_type,

[not at google - send to devlin, 2014/08/29 05:39:24]

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.

[ericzeng, 2014/08/29 16:52:22]

Doesn't that mean moving the errors out of manifest_constants? There some tests
and possibly other code that also need the error strings. 

[ericzeng, 2014/08/29 22:08:51]

Changed from enum to parameterized string

+                     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()) {
+      if (url_type == OptionsPageInfo::OPTIONS_PAGE)
+        *error = base::ASCIIToUTF16(errors::kInvalidOptionsPageInHostedApp);
+      else if (url_type == OptionsPageInfo::OPTIONS_UI_PAGE)
+        *error = base::ASCIIToUTF16(errors::kInvalidOptionsUIPageInHostedApp);

[not at google - send to devlin, 2014/08/29 05:39:24]

This shouldn't even be possible, hosted apps can't specify options_ui at all.

[ericzeng, 2014/08/29 22:08:51]

Removed OptionsUI errors

+      return false;
+    }
+    *result = options_url;

[not at google - send to devlin, 2014/08/29 05:39:25]

You should probably early-return in the hosted app case, particularly since some
of the code isn't necessary.

[ericzeng, 2014/08/29 22:08:51]

Done.

+
+  } else {
+    GURL absolute(url_string);
+    if (absolute.is_valid()) {
+      if (url_type == OptionsPageInfo::OPTIONS_PAGE) {
+        *error =
+            base::ASCIIToUTF16(errors::kInvalidOptionsPageExpectUrlInPackage);
+      } else if (url_type == OptionsPageInfo::OPTIONS_UI_PAGE) {
+        *error =
+            base::ASCIIToUTF16(errors::kInvalidOptionsUIPageExpectUrlInPackage);
+      }
+      return false;
+    }
+
+    GURL resource_url = extension->GetResourceURL(url_string);
+    if (!resource_url.is_valid()) {
+      if (url_type == OptionsPageInfo::OPTIONS_PAGE)
+        *error = base::ASCIIToUTF16(errors::kInvalidOptionsUIPage);
+      else if (url_type == OptionsPageInfo::OPTIONS_UI_PAGE)
+        *error = base::ASCIIToUTF16(errors::kInvalidOptionsUIPage);

[not at google - send to devlin, 2014/08/29 05:39:25]

These are the same?

[ericzeng, 2014/08/29 22:08:50]

Replaced with parameterized error

+      return false;
+    }
+    *result = resource_url;
+  }
+  return true;
+}
+
+OptionsPageInfo* GetOptionsPageInfo(const Extension* extension) {
+  return static_cast<OptionsPageInfo*>(
+      extension->GetManifestData(keys::kOptionsUI));
+}
+
+}  // namespace
+
+OptionsPageInfo::OptionsPageInfo(GURL options_page,
+                                 GURL options_ui_page,
+                                 bool chrome_styles,
+                                 bool open_in_tab)
+    : options_page_(options_page),
+      options_ui_page_(options_ui_page),
+      chrome_styles_(chrome_styles),
+      open_in_tab_(open_in_tab) {
+}
+
+OptionsPageInfo::~OptionsPageInfo() {
+}
+
+// static
+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::ChromeStyle(const Extension* extension) {
+  return GetOptionsPageInfo(extension)->chrome_styles_;

[not at google - send to devlin, 2014/08/29 05:39:24]

Shouldn't you be checking for an empty info here?

[ericzeng, 2014/08/29 22:08:51]

Done.

+}
+
+// static
+bool OptionsPageInfo::OpenInTab(const Extension* extension) {
+  return GetOptionsPageInfo(extension)->open_in_tab_;

[not at google - send to devlin, 2014/08/29 05:39:24]

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.

[ericzeng, 2014/08/29 22:08:51]

Done.

+}
+
+scoped_ptr<OptionsPageInfo> OptionsPageInfo::FromValues(

[not at google - send to devlin, 2014/08/29 05:39:24]

FromValues looks kinda odd, and besides this is a Parse method, so Parse()?

[ericzeng, 2014/08/29 22:08:50]

Done.

+    Extension* extension,
+    const base::Value* options_ui_value,
+    const base::Value* options_page_value,
+    std::vector<InstallWarning>* install_warnings,
+    base::string16* error) {
+  GURL options_page;
+  GURL options_ui_page;
+  bool chrome_style = false;
+  bool open_in_tab = false;
+
+  // 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,
+                           OptionsUrlType::OPTIONS_UI_PAGE,
+                           error,
+                           &options_ui_page)) {
+        return scoped_ptr<OptionsPageInfo>();
+      }
+      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;
+    } else {
+      *error = base::ASCIIToUTF16(errors::kInvalidOptionsUIPage);
+      return scoped_ptr<OptionsPageInfo>();
+    }
+  }
+
+  // Parse the options_page entry
+  if (options_page_value) {
+    std::string options_page_string;
+    if (!options_page_value->GetAsString(&options_page_string)) {
+      *error = ASCIIToUTF16(errors::kInvalidOptionsPage);
+      return scoped_ptr<OptionsPageInfo>();
+    }
+    if (!ParseOptionsUrl(extension,
+                         options_page_string,
+                         OptionsUrlType::OPTIONS_PAGE,
+                         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,

[not at google - send to devlin, 2014/08/29 05:39:24]

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.

[ericzeng, 2014/08/29 16:52:22]

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. 

+                                       base::string16* error) {
+  const base::Value* options_ui_value = NULL;
+  const base::Value* options_page_value = NULL;
+  if (!extension->manifest()->Get(keys::kOptionsUI, &options_ui_value) &&
+      !extension->manifest()->Get(keys::kOptionsPage, &options_page_value)) {
+    // Note: extension->manifest()->Get returns false both when the value is
+    // malformed or missing. Therefore we can't determine which one is at fault,
+    // so for now return the error message for just options_page.
+    *error = base::ASCIIToUTF16(errors::kInvalidOptionsPage);
+    return false;
+  }

[not at google - send to devlin, 2014/08/29 05:39:24]

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.

[ericzeng, 2014/08/29 22:08:51]

Done.

+
+  std::vector<InstallWarning> install_warnings;
+
+  scoped_ptr<OptionsPageInfo> info =
+      OptionsPageInfo::FromValues(extension,
+                                  options_ui_value,
+                                  options_page_value,
+                                  &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::GetOptionsPage(extension).is_empty() &&

[not at google - send to devlin, 2014/08/29 05:39:24]

HasOptionsPage would look so much nicer in places like this :(

Early-exit might look better here as well.

[ericzeng, 2014/08/29 22:08:51]

Done.

+      !extension->is_hosted_app()) {
+    const base::FilePath options_path =
+        extensions::file_util::ExtensionURLToRelativeFilePath(
+            OptionsPageInfo::GetOptionsPage(extension));
+    const base::FilePath path =

[not at google - send to devlin, 2014/08/29 05:39:24]

const base::FilePath&

[ericzeng, 2014/08/29 22:08:51]

Done.

+        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