Cache the object given to the chrome.app.window.create callback

chrome/renderer/resources/extensions/app_window_custom_bindings.js

Index: chrome/renderer/resources/extensions/app_window_custom_bindings.js
diff --git a/chrome/renderer/resources/extensions/app_window_custom_bindings.js b/chrome/renderer/resources/extensions/app_window_custom_bindings.js
index 04283fec5a0ed4059a0a36f007f8decec9d2bf11..3fea8af558ed38288875d10af225cbde0b5cb81e 100644
--- a/chrome/renderer/resources/extensions/app_window_custom_bindings.js
+++ b/chrome/renderer/resources/extensions/app_window_custom_bindings.js
@@ -11,30 +11,70 @@ var forEach = require('utils').forEach;
 var GetView = appWindowNatives.GetView;
 
 chromeHidden.registerCustomHook('app.window', function(bindingsAPI) {
+  chromeHidden.appWindow = {
+    windowCache: {},
+    currentWindow: null
+  };
+
   var apiFunctions = bindingsAPI.apiFunctions;
   apiFunctions.setCustomCallback('create', function(name, request, viewId) {
-    var view = null;
-    if (viewId) {
-      var shouldShowFrame = !request.args[1] || request.args[1].frame != 'none';
-      view = GetView(viewId, !!shouldShowFrame);
+    if (!viewId) {
+      // create failed? If given a callback, trigger it with an undefined object

[benwells, 2012/09/17 08:33:31]

Nit: Start comment with uppercase.

[tapted, 2012/09/17 09:22:13]

Done.

+      if (request.callback) {
+        request.callback()
+        delete request.callback;
+      }
+      return;
     }
+
+    var params = request.args[1];
+    var shouldShowFrame = !params || params.frame != 'none';
+    var view = GetView(viewId, !!shouldShowFrame);
+    var state = {
+      viewId: viewId,

[benwells, 2012/09/17 08:33:31]

Does the viewId need to be in the state?

[tapted, 2012/09/17 09:22:13]

a case for it might be to index a collection of AppWindow objects held in some
shared area (background page?), or to later expose it to the developer (but I
think Jeremy is against that ;-).

[benwells, 2012/09/18 03:47:24]

Ah, ok. Let's take it out now, and add it back in when it is needed. 

[tapted, 2012/09/18 04:17:24]

Done.

+      id: params.id ? params.id : ''
+    };
+    var appWindow = view.chrome.app.window.makeAppWindow(state);

[benwells, 2012/09/17 08:33:31]

Nit: it would be nice to add a comment that this runs in the context of
view.chrome.app.window.

[tapted, 2012/09/17 09:22:13]

Done.

+
     if (request.callback) {
-      request.callback(view.chrome.app.window.current());
+      request.callback(appWindow);
       delete request.callback;
     }
-  })
+  });
   var AppWindow = function() {};
   forEach(chromeHidden.internalAPIs.app.currentWindowInternal, function(fn) {
     AppWindow.prototype[fn] =
         chromeHidden.internalAPIs.app.currentWindowInternal[fn];
   });
   AppWindow.prototype.moveTo = window.moveTo.bind(window);
+  Object.defineProperty(AppWindow.prototype, 'id', {get: function() {
+    return chromeHidden.appWindow.windowCache.id;
+  }});
   AppWindow.prototype.resizeTo = window.resizeTo.bind(window);
   AppWindow.prototype.contentWindow = window;
   // So as not to break apps that use .dom. http://crbug.com/147668
   // TODO(jeremya): remove this once M23 has branched.
   AppWindow.prototype.dom = window;

[benwells, 2012/09/17 08:33:31]

Unless there is a reason to keep it, I'd prefer this stuff to move off prototype
/ into the makeAppWindow handler.

[tapted, 2012/09/17 09:22:13]

I am for this - the cache means that the prototype should only be used once, and
we can avoid some logic in the api bindings. Jeremy?

[tapted, 2012/09/18 07:47:32]

Turns out trying to ditch the prototype will break DCHECKs generated from

  callback CreateWindowCallback =
      void ([instanceOf=AppWindow] object created_window);


makeAppWindow is more cohesive in the latest revision, but still uses a
prototype.

   apiFunctions.setHandleRequest('current', function() {
-    return new AppWindow;
-  })
+    return chromeHidden.appWindow.currentWindow;

[benwells, 2012/09/17 08:33:31]

Would be nice to log an error here if currentWindow is null.

[tapted, 2012/09/17 09:22:13]

Done.

+    // That should "always" work in production. However, a right-click reload
+    // in developer mode will clear the cache, so we need to bootstrap it again.
+    // First step is to determine whether we are actually an appWindow via IPC..
+  });
+
+  // TODO(tapted) this should be "internal only" but needs to be called on a
+  // different JS context -- is there a neater way?
+  apiFunctions.setHandleRequest('makeAppWindow', function(params) {
+    var newWindow = new AppWindow;

[benwells, 2012/09/17 08:33:31]

you can get rid of two lines here by removing the vars and just assignint
directly to chromeHidden.appWindow.foo

[tapted, 2012/09/17 09:22:13]

Done.

+    var cache = {
+      viewId: params.viewId,
+      id: params.id ? params.id : ''
+    };
+    chromeHidden.appWindow.windowCache = cache;
+    chromeHidden.appWindow.currentWindow = newWindow;
+    // TODO(tapted) we need to fire-off a mechanism here to bootstrap the cache
+    // with values from the browser thread not known by create() (or pass them

[benwells, 2012/09/17 08:33:31]

This comment is now invalid I think?

[tapted, 2012/09/17 09:22:13]

There might be some properties we want to store in the cache that 'create()'
doesn't know about, and we can't get from DOM window. E.g. the defaults for
{min,max}{Width,Height} -- a developer might want to know what defaults got
chosen for them. We could hardcode a default (like for id**), but that's not
very cohesive (e.g. when defaults change in the C++ code).

** except app_window_api.cc has "// TODO(mek): use URL if no id specified?" so
the assumption that id = '' if none is specified might be invalid

[benwells, 2012/09/18 03:47:24]

TODOs have a habit of sticking around, so I like them to be pretty clearly
defined and known. If there is some 'might' involved let's remove the todo. You
can file bugs on yourself instead.

Having said that, I think we should figure out the id behavior and get it right
before landing this change.

[tapted, 2012/09/18 04:17:24]

I can see if I can convince browser::create to pass an object back to the custom
callback with this info on it, without major IDL changes..

[tapted, 2012/09/18 05:07:46]

Done for `id` without touching the IDL (but with touching shell_window.h).
Alternative: just touch app_window_api.cc and pull window_key from create_params
instead of ShellWindow. Extension: do touch the IDL and use the
create-dictionary helpers generated by the IDL script.

+    // in to the custom callback, and ignore developer-reloads)
+    return newWindow;
+  });
 });