Create windows for new app window bounds API

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/apps_client.h"
 #include "apps/ui/native_app_window.h"
 #include "base/command_line.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.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/api/extension_action/extension_action_api.h"
 #include "chrome/browser/extensions/window_controller.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/extensions/api/app_window.h"
 #include "chrome/common/extensions/features/feature_channel.h"
 #include "content/public/browser/notification_registrar.h"
@@ skipping 25 matching lines @@
 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.";
+const char kInvalidChannelForBounds[] =
+    "innerBounds and outerBounds are only available in dev channel.";
 }  // namespace app_window_constants
 
 const char kNoneFrameOption[] = "none";
   // TODO(benwells): Remove HTML titlebar injection.
 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
@@ skipping 17 matching lines @@
   ~DevToolsRestorer() {}
 
   void LoadCompleted() {
     delayed_create_function_->SendDelayedResponse();
     Release();
   }
 
   scoped_refptr<AppWindowCreateFunction> delayed_create_function_;
 };
 
+// If the same property is specified for the inner and outer bounds, raise an
+// error.
+bool CheckBoundsConflict(const scoped_ptr<int>& inner_property,
+                         const scoped_ptr<int>& outer_property,
+                         const std::string& property_name,
+                         std::string* error) {
+  if (inner_property.get() && outer_property.get()) {
+    std::vector<std::string> subst;
+    subst.push_back(property_name);
+    *error = ReplaceStringPlaceholders(
+        "The $1 property cannot be specified for both inner and outer bounds.",

[benwells, 2014/03/05 02:33:30]

Nit: put this in kConflictingBlahBlah up with the other errors.

+        subst,
+        NULL);
+    return false;
+  }
+
+  return true;
+}
+
+// Copy over the bounds specification properties from the API to the
+// AppWindow::CreateParams.
+void CopyBoundsSpec(
+    const extensions::api::app_window::BoundsSpecification* input_spec,
+    apps::AppWindow::BoundsSpecification* create_spec) {
+  if (input_spec && input_spec->left.get())

[tapted, 2014/03/05 03:14:03]

nit: 

if (!input_spec)
  return;

+    create_spec->bounds.set_x(*input_spec->left);
+  if (input_spec && input_spec->top.get())
+    create_spec->bounds.set_y(*input_spec->top);
+  if (input_spec && input_spec->width.get())
+    create_spec->bounds.set_width(*input_spec->width);
+  if (input_spec && input_spec->height.get())
+    create_spec->bounds.set_height(*input_spec->height);
+  if (input_spec && input_spec->min_width.get())
+    create_spec->minimum_size.set_width(*input_spec->min_width);
+  if (input_spec && input_spec->min_height.get())
+    create_spec->minimum_size.set_height(*input_spec->min_height);
+  if (input_spec && input_spec->max_width.get())
+    create_spec->maximum_size.set_width(*input_spec->max_width);
+  if (input_spec && input_spec->max_height.get())
+    create_spec->maximum_size.set_height(*input_spec->max_height);
+}
+
 }  // namespace
 
 AppWindowCreateFunction::AppWindowCreateFunction()
     : inject_html_titlebar_(false) {}
 
 void AppWindowCreateFunction::SendDelayedResponse() {
   SendResponse(true);
 }
 
 bool AppWindowCreateFunction::RunImpl() {
@@ skipping 60 matching lines @@
           result->SetBoolean("existingWindow", true);
           // TODO(benwells): Remove HTML titlebar injection.
           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.
-    if (options->default_width.get())
-      create_params.bounds.set_width(*options->default_width.get());
-    if (options->default_height.get())
-      create_params.bounds.set_height(*options->default_height.get());
-    if (options->default_left.get())
-      create_params.bounds.set_x(*options->default_left.get());
-    if (options->default_top.get())
-      create_params.bounds.set_y(*options->default_top.get());
-
-    if (options->width.get())
-      create_params.bounds.set_width(*options->width.get());
-    if (options->height.get())
-      create_params.bounds.set_height(*options->height.get());
-    if (options->left.get())
-      create_params.bounds.set_x(*options->left.get());
-    if (options->top.get())
-      create_params.bounds.set_y(*options->top.get());
-
-    if (options->bounds.get()) {
-      app_window::ContentBounds* bounds = options->bounds.get();
-      if (bounds->width.get())
-        create_params.bounds.set_width(*bounds->width.get());
-      if (bounds->height.get())
-        create_params.bounds.set_height(*bounds->height.get());
-      if (bounds->left.get())
-        create_params.bounds.set_x(*bounds->left.get());
-      if (bounds->top.get())
-        create_params.bounds.set_y(*bounds->top.get());
-    }
+    if (!ParseBoundsSpec(*options, &create_params, &error_))

[benwells, 2014/03/05 02:33:30]

Yay for making this monster function shorter. Maybe call it GetBoundsSpec to be
consistent with GetFrameOptions (or rename GetFrameOptions to ParseFrameOptions,
I don't mind).

+      return false;
 
     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 (!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())
-      minimum_size.set_width(*options->min_width);
-    if (options->min_height.get())
-      minimum_size.set_height(*options->min_height);
-    gfx::Size& maximum_size = create_params.maximum_size;
-    if (options->max_width.get())
-      maximum_size.set_width(*options->max_width);
-    if (options->max_height.get())
-      maximum_size.set_height(*options->max_height);
-
     if (options->hidden.get())
       create_params.hidden = *options->hidden.get();
 
     if (options->resizable.get())
       create_params.resizable = *options->resizable.get();
 
     if (options->always_on_top.get() &&
         GetExtension()->HasAPIPermission(APIPermission::kAlwaysOnTopWindows))
       create_params.always_on_top = *options->always_on_top.get();
 
@@ skipping 48 matching lines @@
   if (apps::AppWindowRegistry::Get(GetProfile())
           ->HadDevToolsAttached(created_view)) {
     new DevToolsRestorer(this, created_view);
     return true;
   }
 
   SendResponse(true);
   return true;
 }
 
+bool AppWindowCreateFunction::ParseBoundsSpec(
+    const extensions::api::app_window::CreateWindowOptions& options,
+    apps::AppWindow::CreateParams* params,
+    std::string* error) {
+  DCHECK(params);
+  DCHECK(error);
+
+  if (options.inner_bounds.get() || options.outer_bounds.get()) {
+    // Parse the inner and outer bounds specifications. If developers use the
+    // new API, the deprecated fields will be ignored - do not allow them to
+    // be mixed.

[tapted, 2014/03/05 03:14:03]

nit: I read this thinking it would be an error if a developer specified both
(but that wouldn't really work). Maybe

"..will be ignored - do not allow them to be mixed." -> "..will be ignored -
there is no attempt to merge them."

+
+    if (GetCurrentChannel() > chrome::VersionInfo::CHANNEL_DEV) {
+      *error = app_window_constants::kInvalidChannelForBounds;
+      return false;
+    }
+
+    const extensions::api::app_window::BoundsSpecification* inner_bounds =
+        options.inner_bounds.get();
+    const extensions::api::app_window::BoundsSpecification* outer_bounds =
+        options.outer_bounds.get();
+    if (inner_bounds && outer_bounds &&

[benwells, 2014/03/05 02:33:30]

You can put all of these conflict checks into one if (innerBounds &&
outer_bounds) check to make the coder a little saner.

Maybe you could also put this into a kind of:
all_good = true;
all_good = all_good || CheckBoundsConflict(...);
all_good = all_good || CheckBoundsConflict(...);
if (all_good)
  return false;

chunk, but I'll leave that up to you.

+        !CheckBoundsConflict(
+             inner_bounds->left, outer_bounds->left, "left", error)) {
+      return false;
+    }
+    if (inner_bounds && outer_bounds &&
+        !CheckBoundsConflict(
+             inner_bounds->top, outer_bounds->top, "top", error)) {
+      return false;
+    }
+    if (inner_bounds && outer_bounds &&
+        !CheckBoundsConflict(
+             inner_bounds->width, outer_bounds->width, "width", error)) {
+      return false;
+    }
+    if (inner_bounds && outer_bounds &&
+        !CheckBoundsConflict(
+             inner_bounds->height, outer_bounds->height, "height", error)) {
+      return false;
+    }
+    if (inner_bounds && outer_bounds &&
+        !CheckBoundsConflict(inner_bounds->min_width,
+                             outer_bounds->min_width,
+                             "minWidth",
+                             error)) {
+      return false;
+    }
+    if (inner_bounds && outer_bounds &&
+        !CheckBoundsConflict(inner_bounds->min_height,
+                             outer_bounds->min_height,
+                             "minHeight",
+                             error)) {
+      return false;
+    }
+    if (inner_bounds && outer_bounds &&
+        !CheckBoundsConflict(inner_bounds->max_width,
+                             outer_bounds->max_width,
+                             "maxWidth",
+                             error)) {
+      return false;
+    }
+    if (inner_bounds && outer_bounds &&
+        !CheckBoundsConflict(inner_bounds->max_height,
+                             outer_bounds->max_height,
+                             "maxHeight",
+                             error)) {
+      return false;
+    }
+
+    CopyBoundsSpec(inner_bounds, &(params->content_spec));
+    CopyBoundsSpec(outer_bounds, &(params->window_spec));
+  } else {
+    // Parse deprecated fields.
+
+    // TODO(jeremya): remove these, since they do the same thing as

[benwells, 2014/03/05 02:33:30]

I don't think we have a plan or bug to remove these, can you remove the TODO?

+    // left/top/width/height.
+    if (options.default_width.get())
+      params->content_spec.bounds.set_width(*options.default_width.get());
+    if (options.default_height.get())
+      params->content_spec.bounds.set_height(*options.default_height.get());
+    if (options.default_left.get())
+      params->content_spec.bounds.set_x(*options.default_left.get());

[benwells, 2014/03/05 02:33:30]

My understanding is that previously left / top in this fiedl and in bounds
referred to the window left / top.

[tmdiep, 2014/03/05 03:06:47]

Ugh. bounds.left and bounds.top were actually intended to be content, but due to
a bug in NativeAppWindowViews::GetFrameInsets(), ended up being window left/top.
Mac and Linux GTK got it right. But it really doesn't make sense to position
content wrt to the screen anyway, so maybe I'll make the deprecated left/top all
set the window position.

[tmdiep, 2014/03/06 01:06:49]

I will make top/left be interpreted as top/left of the frame during window
creation, set bounds, get bounds, etc as it seems apps really have started to
rely on the bug: crbug.com/349397.

It won't be consistent with the documentation, but we will deprecate bounds and
change the doco soon anyway.

[benwells, 2014/03/06 22:40:16]

Awesome, love it. You should add a comment briefly explaining why, or people
might think this is a bug and 'fix' it.

+    if (options.default_top.get())
+      params->content_spec.bounds.set_y(*options.default_top.get());
+
+    if (options.width.get())
+      params->content_spec.bounds.set_width(*options.width.get());
+    if (options.height.get())
+      params->content_spec.bounds.set_height(*options.height.get());
+    if (options.left.get())
+      params->content_spec.bounds.set_x(*options.left.get());
+    if (options.top.get())
+      params->content_spec.bounds.set_y(*options.top.get());
+
+    if (options.bounds.get()) {
+      app_window::ContentBounds* bounds = options.bounds.get();
+      if (bounds->width.get())
+        params->content_spec.bounds.set_width(*bounds->width.get());
+      if (bounds->height.get())
+        params->content_spec.bounds.set_height(*bounds->height.get());
+      if (bounds->left.get())
+        params->content_spec.bounds.set_x(*bounds->left.get());
+      if (bounds->top.get())
+        params->content_spec.bounds.set_y(*bounds->top.get());
+    }
+
+    gfx::Size& minimum_size = params->content_spec.minimum_size;
+    if (options.min_width.get())
+      minimum_size.set_width(*options.min_width);
+    if (options.min_height.get())
+      minimum_size.set_height(*options.min_height);
+    gfx::Size& maximum_size = params->content_spec.maximum_size;
+    if (options.max_width.get())
+      maximum_size.set_width(*options.max_width);
+    if (options.max_height.get())
+      maximum_size.set_height(*options.max_height);
+  }
+
+  return true;
+}
+
 AppWindow::Frame AppWindowCreateFunction::GetFrameFromString(
     const std::string& frame_string) {
    if (frame_string == kHtmlFrameOption &&
        (GetExtension()->HasAPIPermission(APIPermission::kExperimental) ||
         CommandLine::ForCurrentProcess()->HasSwitch(
             switches::kEnableExperimentalExtensionApis))) {
      inject_html_titlebar_ = true;
      return AppWindow::FRAME_NONE;
    }
 
@@ skipping 51 matching lines @@
       GetCurrentChannel() > chrome::VersionInfo::CHANNEL_DEV) {
     // If not on trunk or dev channel, always use the standard white frame.
     // TODO(benwells): Remove this code 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