Create windows for new app window bounds API

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 856d82db410a3a1b52675ec816a9e37e29697376..68d6ec71bbbb4a9c02cecc2f658cdc460d4537b0 100644
--- a/chrome/browser/extensions/api/app_window/app_window_api.cc
+++ b/chrome/browser/extensions/api/app_window/app_window_api.cc
@@ -11,6 +11,7 @@
 #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"
@@ -56,6 +57,8 @@ const char kInvalidChannelForFrameOptions[] =
 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";
@@ -93,6 +96,48 @@ class DevToolsRestorer : public base::RefCounted<DevToolsRestorer> {
   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()
@@ -173,37 +218,8 @@ bool AppWindowCreateFunction::RunImpl() {
       }
     }
 
-    // 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) {
@@ -222,17 +238,6 @@ bool AppWindowCreateFunction::RunImpl() {
       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();
 
@@ -301,6 +306,128 @@ bool AppWindowCreateFunction::RunImpl() {
   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 &&