Add frame color option to packaged app windows.

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

 // Copyright (c) 2012 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 "chrome/browser/extensions/api/app_window/app_window_api.h"
 
 #include "apps/app_window.h"
 #include "apps/app_window_contents.h"
 #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"
 #include "chrome/browser/devtools/devtools_window.h"
 #include "chrome/browser/extensions/window_controller.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/apps/chrome_shell_window_delegate.h"
 #include "chrome/common/extensions/api/app_window.h"
 #include "chrome/common/extensions/features/feature_channel.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #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"
 
 #if defined(USE_ASH)
 #include "ash/shell.h"
 #include "ui/aura/root_window.h"
 #include "ui/aura/window.h"
 #endif
 
 using apps::AppWindow;
 
 namespace app_window = extensions::api::app_window;
 namespace Create = app_window::Create;
 
 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 {
 
 // Opens an inspector window and delays the response to the
 // AppWindowCreateFunction until the DevToolsWindow has finished loading, and is
 // ready to stop on breakpoints in the callback.
 class DevToolsRestorer : public base::RefCounted<DevToolsRestorer> {
  public:
   DevToolsRestorer(AppWindowCreateFunction* delayed_create_function,
                    content::RenderViewHost* created_view)
@@ skipping 12 matching lines @@
   ~DevToolsRestorer() {}
 
   void LoadCompleted() {
     delayed_create_function_->SendDelayedResponse();
     Release();
   }
 
   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() {
   SendResponse(true);
 }
 
 bool AppWindowCreateFunction::RunImpl() {
   // Don't create app window if the system is shutting down.
   if (extensions::ExtensionsBrowserClient::Get()->IsShuttingDown())
     return false;
 
   scoped_ptr<Create::Params> params(Create::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params.get());
 
   GURL url = GetExtension()->GetResourceURL(params->url);
   // Allow absolute URLs for component apps, otherwise prepend the extension
   // path.
   if (GetExtension()->location() == extensions::Manifest::COMPONENT) {
     GURL absolute = GURL(params->url);
     if (absolute.has_scheme())
       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().
   AppWindow::CreateParams create_params;
   app_window::CreateWindowOptions* options = params->options.get();
   if (options) {
     if (options->id.get()) {
       // TODO(mek): use URL if no id specified?
       // Limit length of id to 256 characters.
       if (options->id->length() > 256) {
@@ skipping 25 matching lines @@
 
           if (options->focused.get() && !*options->focused.get())
             window->Show(AppWindow::SHOW_INACTIVE);
           else
             window->Show(AppWindow::SHOW_ACTIVE);
 
           base::DictionaryValue* result = new base::DictionaryValue;
           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);
           return true;
         }
       }
     }
 
     // TODO(jeremya): remove these, since they do the same thing as
     // left/top/width/height.
@@ skipping 27 matching lines @@
         create_params.bounds.set_y(*bounds->top.get());
     }
 
     if (GetCurrentChannel() <= chrome::VersionInfo::CHANNEL_DEV ||
         GetExtension()->location() == extensions::Manifest::COMPONENT) {
       if (options->type == extensions::api::app_window::WINDOW_TYPE_PANEL) {
         create_params.window_type = AppWindow::WINDOW_TYPE_PANEL;
       }
     }
 
-    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) ||
          CommandLine::ForCurrentProcess()->HasSwitch(
              switches::kEnableExperimentalExtensionApis))) {
       create_params.transparent_background = *options->transparent_background;
     }
 
     gfx::Size& minimum_size = create_params.minimum_size;
     if (options->min_width.get())
@@ skipping 30 matching lines @@
         case extensions::api::app_window::STATE_MAXIMIZED:
           create_params.state = ui::SHOW_STATE_MAXIMIZED;
           break;
         case extensions::api::app_window::STATE_MINIMIZED:
           create_params.state = ui::SHOW_STATE_MINIMIZED;
           break;
       }
     }
   }
 
+  UpdateFrameOptionsForChannel(&create_params);
+
   create_params.creator_process_id =
       render_view_host_->GetProcess()->GetID();
 
   AppWindow* app_window = new AppWindow(
       GetProfile(), new ChromeShellWindowDelegate(), GetExtension());
   app_window->Init(
       url, new apps::AppWindowContentsImpl(app_window), create_params);
 
   if (chrome::IsRunningInForcedAppMode())
     app_window->ForcedFullscreen();
 
   content::RenderViewHost* created_view =
       app_window->web_contents()->GetRenderViewHost();
   int view_id = MSG_ROUTING_NONE;
   if (create_params.creator_process_id == created_view->GetProcess()->GetID())
     view_id = created_view->GetRoutingID();
 
   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);
 
   if (apps::AppWindowRegistry::Get(GetProfile())
           ->HadDevToolsAttached(created_view)) {
     new DevToolsRestorer(this, created_view);
     return true;
   }
 
   SendResponse(true);
   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