Add frame color option to packaged app windows.

chrome/browser/extensions/api/app_window/app_window_api.cc

Index: chrome/browser/extensions/api/app_window/app_window_api.cc
diff --git a/chrome/browser/extensions/api/app_window/app_window_api.cc b/chrome/browser/extensions/api/app_window/app_window_api.cc
index 7f0006d1a03bbe6a92c0a2ac709224f780704a9f..bcdbaac617744d0c2742a99784620a5252ec9a91 100644
--- a/chrome/browser/extensions/api/app_window/app_window_api.cc
+++ b/chrome/browser/extensions/api/app_window/app_window_api.cc
@@ -9,6 +9,7 @@
 #include "apps/app_window_registry.h"
 #include "apps/ui/native_app_window.h"
 #include "base/command_line.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "chrome/browser/app_mode/app_mode_utils.h"
@@ -26,6 +27,7 @@
 #include "content/public/common/url_constants.h"
 #include "extensions/browser/extensions_browser_client.h"
 #include "extensions/common/switches.h"
+#include "third_party/skia/include/core/SkColor.h"
 #include "ui/base/ui_base_types.h"
 #include "ui/gfx/rect.h"
 #include "url/gurl.h"
@@ -46,10 +48,16 @@ namespace extensions {
 namespace app_window_constants {
 const char kInvalidWindowId[] =
     "The window id can not be more than 256 characters long.";
-}
+const char kInvalidColorSpecification[] =
+    "The color specification could not be parsed.";
+const char kInvalidChannelForFrameOptions[] =
+    "frameOptions is only available in dev channel.";
+const char kFrameOptionsAndFrame[] =
+    "Only one of frame and frameOptions can be supplied.";
+const char kColorWithFrameNone[] = "Windows with no frame cannot have a color.";
+}  // namespace app_window_constants
 
 const char kNoneFrameOption[] = "none";
-const char kHtmlFrameOption[] = "experimental-html";
 
 namespace {
 
@@ -82,6 +90,40 @@ class DevToolsRestorer : public base::RefCounted<DevToolsRestorer> {
   scoped_refptr<AppWindowCreateFunction> delayed_create_function_;
 };
 
+bool ParseCSSColorString(const std::string& color_string, SkColor* result) {

[Matt Giuca, 2014/02/18 06:03:18]

It's kind of surprising that there isn't already such a method in the Chrome
code base. I searched for one and couldn't find one either. If there really is
none, do you think it's worth putting this somewhere more central (base?).

It should also have a proper unit test (independent of JavaScript and the
windowing API) since it has some hairy edge cases.

[benwells, 2014/02/18 07:47:25]

Yeah I forgot I had copy pasted this :-/

Using the original one now. I'll apply your review comments to that one.

Done.

+  std::string formatted_color = "#";

[Matt Giuca, 2014/02/18 06:03:18]

Nit: Why does formatted_color need to start with a #?

In the #FFF case, this only gets in the way. In the #FFFFFF case, you can just
copy a substring excluding the #.

It just simplifies the arithmetic a bit later (no need for "1 +").

+  // Check the string for incorrect formatting.
+  if (color_string[0] != '#')

[Matt Giuca, 2014/02/18 06:03:18]

if (color_string.empty() || ...) (potential buffer overflow).

[benwells, 2014/02/18 07:47:25]

Done.

+    return false;
+
+  // Convert the string from #FFF format to #FFFFFF format.
+  if (color_string.length() == 4) {
+    for (size_t i = 1; i < color_string.length(); i++) {

[Matt Giuca, 2014/02/18 06:03:18]

Why not: i < 4
Also: ++i

[benwells, 2014/02/18 07:47:25]

Done.

+      formatted_color += color_string[i];
+      formatted_color += color_string[i];
+    }
+  } else {

[Matt Giuca, 2014/02/18 06:03:18]

How about:
// You could even move this first, since it is more common.
else if (color_string.length() == 7) {
  formatted_color = color_string;
} else {
  return false;
}

DCHECK(formatted_color.length() == 7);

Just feels a bit more natural. Also, in the #FFF case, your code currently
builds a string of length 7, then returns false if it's length is not 7. A
DCHECK is more appropriate there.

[benwells, 2014/02/18 07:47:25]

Done. I think a DCHECK is unwarranted.

+    formatted_color = color_string;
+  }
+
+  if (formatted_color.length() != 7)
+    return false;
+
+  // Convert the string to an integer and make sure it is in the correct value
+  // range.
+  int color_ints[3] = {0};

[Matt Giuca, 2014/02/18 06:03:18]

Use HexStringToBytes instead of manually looping.

Aside from simplifying the code, it avoids a couple of weird cases where
HexStringToInt accepts "0x" as well as + and - signs. (For example, the string
"#+F+F+F" is a valid parse because "+F" maps to 15.)

Also, since SkColorSetARGB accepts uint8, your color_ints array should be uint8,
not int, which means you don't need to do range checks. (HexStringToBytes gives
you a vector of uint8 so this is good). Change this comment, removing "and make
sure it is in the correct value range".

[benwells, 2014/02/18 07:47:25]

Done.

+  for (int i = 0; i < 3; i++) {

[Matt Giuca, 2014/02/18 06:03:18]

Nit: ++i

[benwells, 2014/02/18 07:47:25]

Done.

+    if (!base::HexStringToInt(formatted_color.substr(1 + (2 * i), 2),
+                              color_ints + i))

[Matt Giuca, 2014/02/18 06:03:18]

Nit: &color_ints[i]

+      return false;
+    if (color_ints[i] > 255 || color_ints[i] < 0)

[Matt Giuca, 2014/02/18 06:03:18]

This should be a DCHECK, since it is not physically possible for two hex digits
to be outside this range.

+      return false;
+  }
+
+  *result = SkColorSetARGB(255, color_ints[0], color_ints[1], color_ints[2]);
+  return true;
+}
+
 }  // namespace
 
 void AppWindowCreateFunction::SendDelayedResponse() {
@@ -105,8 +147,6 @@ bool AppWindowCreateFunction::RunImpl() {
       url = absolute;
   }
 
-  bool inject_html_titlebar = false;
-
   // TODO(jeremya): figure out a way to pass the opening WebContents through to
   // AppWindow::Create so we can set the opener at create time rather than
   // with a hack in AppWindowCustomBindings::GetView().
@@ -152,6 +192,7 @@ bool AppWindowCreateFunction::RunImpl() {
           result->Set("viewId", new base::FundamentalValue(view_id));
           window->GetSerializedState(result);
           result->SetBoolean("existingWindow", true);
+          // TODO(benwells): Remove HTML titlebar injection.

[Matt Giuca, 2014/02/18 06:03:18]

I think that removing HTML titlebar injection should be a separate CL from this
one (either before or after; it doesn't matter). It seems like a lot of the
changes in this CL are about removing this unrelated* feature.

*I realise that it's related in that this feature makes that one obsolete, but
the changes aren't dependent on one another.

[benwells, 2014/02/18 07:47:25]

Done.

           result->SetBoolean("injectTitlebar", false);
           SetResult(result);
           SendResponse(true);
@@ -199,19 +240,8 @@ bool AppWindowCreateFunction::RunImpl() {
       }
     }
 
-    if (options->frame.get()) {
-      if (*options->frame == kHtmlFrameOption &&
-          (GetExtension()->HasAPIPermission(APIPermission::kExperimental) ||
-           CommandLine::ForCurrentProcess()->HasSwitch(
-               switches::kEnableExperimentalExtensionApis))) {
-        create_params.frame = AppWindow::FRAME_NONE;
-        inject_html_titlebar = true;
-      } else if (*options->frame == kNoneFrameOption) {
-        create_params.frame = AppWindow::FRAME_NONE;
-      } else {
-        create_params.frame = AppWindow::FRAME_CHROME;
-      }
-    }
+    if (!GetFrameOptions(*options, &create_params))
+      return false;
 
     if (options->transparent_background.get() &&
         (GetExtension()->HasAPIPermission(APIPermission::kExperimental) ||
@@ -262,6 +292,8 @@ bool AppWindowCreateFunction::RunImpl() {
     }
   }
 
+  UpdateFrameOptionsForChannel(&create_params);
+
   create_params.creator_process_id =
       render_view_host_->GetProcess()->GetID();
 
@@ -281,8 +313,8 @@ bool AppWindowCreateFunction::RunImpl() {
 
   base::DictionaryValue* result = new base::DictionaryValue;
   result->Set("viewId", new base::FundamentalValue(view_id));
-  result->Set("injectTitlebar",
-      new base::FundamentalValue(inject_html_titlebar));
+  // TODO(benwells): Remove HTML titlebar injection.
+  result->Set("injectTitlebar", new base::FundamentalValue(false));
   result->Set("id", new base::StringValue(app_window->window_key()));
   app_window->GetSerializedState(result);
   SetResult(result);
@@ -297,4 +329,65 @@ bool AppWindowCreateFunction::RunImpl() {
   return true;
 }
 
+AppWindow::Frame AppWindowCreateFunction::GetFrameFromString(
+    const std::string& frame_string) {
+  if (frame_string == kNoneFrameOption)
+    return AppWindow::FRAME_NONE;
+
+  return AppWindow::FRAME_CHROME;
+}
+
+bool AppWindowCreateFunction::GetFrameOptions(
+    const app_window::CreateWindowOptions& options,
+    AppWindow::CreateParams* create_params) {
+  if (options.frame.get() && options.frame_options.get()) {
+    error_ = app_window_constants::kFrameOptionsAndFrame;
+    return false;
+  }
+
+  if (options.frame.get())
+    create_params->frame = GetFrameFromString(*options.frame);
+
+  if (options.frame_options.get()) {
+    if (GetCurrentChannel() <= chrome::VersionInfo::CHANNEL_DEV) {
+      app_window::FrameOptions* frame_options = options.frame_options.get();
+
+      if (frame_options->type.get())
+        create_params->frame = GetFrameFromString(*frame_options->type);
+
+      if (frame_options->color.get()) {
+        if (create_params->frame != AppWindow::FRAME_CHROME) {
+          error_ = app_window_constants::kColorWithFrameNone;
+          return false;
+        }
+
+        if (ParseCSSColorString(*frame_options->color,
+                                &create_params->frame_color)) {
+          create_params->has_frame_color = true;
+        } else {
+          error_ = app_window_constants::kInvalidColorSpecification;

[Matt Giuca, 2014/02/18 06:03:18]

Where does this error get displayed or sent? In your sample app, an invalid
colour specification produces no JS console output, and the following Chrome
console output:

[6376:5176:0218/051747:INFO:CONSOLE(0)] "Error in response to app.window.create:
TypeError: Cannot read property 'viewId' of undefined
    at HTMLInputElement.white
(chrome-extension://bcepbjddpgkemenmofbnkinhlohnbjhf/test.js:7:21)", source:
chrome-extension://bcepbjddpgkemenmofbnkinhlohnbjhf/_generated_background_page.html
(0)
[6376:5176:0218/051747:INFO:CONSOLE(0)] "Error in response to app.window.create:
TypeError: Cannot read property 'contentWindow' of undefined
    at HTMLInputElement.white
(chrome-extension://bcepbjddpgkemenmofbnkinhlohnbjhf/test.js:7:21)", source:
chrome-extension://bcepbjddpgkemenmofbnkinhlohnbjhf/_generated_background_page.html
(0)

It's like it just sets the window to null and doesn't print your error message
at all.

[benwells, 2014/02/18 07:47:25]

It gets put in chrome.runtime.lastError. For some reason we stopped logging
these errors in general. Maybe we should keep logging them.

+          return false;
+        }
+      }
+    } else {
+      error_ = app_window_constants::kInvalidChannelForFrameOptions;
+      return false;
+    }
+  }
+
+  return true;
+}
+
+void AppWindowCreateFunction::UpdateFrameOptionsForChannel(
+    apps::AppWindow::CreateParams* create_params) {
+  if (create_params->frame == AppWindow::FRAME_CHROME &&
+      GetCurrentChannel() > chrome::VersionInfo::CHANNEL_DEV) {
+    // If not on trunk or dev channel, always use the standard white frame. This

[Matt Giuca, 2014/02/18 06:03:18]

Can you change the second sentence into a TODO(benwells).

[benwells, 2014/02/18 07:47:25]

Done.

+    // code will be remove once we get agreement to use the new native style
+    // frame.
+    create_params->has_frame_color = true;
+    create_params->frame_color = SK_ColorWHITE;
+  }
+}
+
 }  // namespace extensions