Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10910304:
Cache the object given to the chrome.app.window.create callback (Closed)
|
Created:
8 years, 3 months ago by tapted Modified:
8 years, 2 months ago CC:
chromium-reviews, Aaron Boodman, mihaip-chromium-reviews_chromium.org, darin-cc_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionCache the object given to the chrome.app.window.create callback and use it for later calls to current()
This version also sets the read-only `id` property on the returned appWindow object (accessor for window_key), which can be passed in with the create params.
BUG=148814
TEST=./browser_tests --gtest_filter=PlatformAppBrowserTest.WindowsApi
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158996
Patch Set 1 #Patch Set 2 : No need to index, since we have a unique chromeHidden context. Worry about developer reloads later #Patch Set 3 : style #Patch Set 4 : access to the id property via cache, leave out GetRoutingID for now #
Total comments: 22
Patch Set 5 : first round of comments #
Total comments: 6
Patch Set 6 : comments, still need to get id bootstrapping correct #Patch Set 7 : Pass the window_key back along with the view_id to the custom callback #Patch Set 8 : improve cohesion with the prototype / JS context based on verbal feedback #
Total comments: 5
Patch Set 9 : better arg name, less nesting #Patch Set 10 : rebase to r157512 (conflicts) #
Total comments: 2
Patch Set 11 : reviewer comments: create->initialize, cache->data #Patch Set 12 : stray/unused return statement #Patch Set 13 : rebase due to r158473 #
Total comments: 6
Patch Set 14 : nits #
Messages
Total messages: 20 (0 generated)
https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right): https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... chrome/renderer/resources/extensions/app_window_custom_bindings.js:22: // create failed? If given a callback, trigger it with an undefined object Nit: Start comment with uppercase. https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... chrome/renderer/resources/extensions/app_window_custom_bindings.js:34: viewId: viewId, Does the viewId need to be in the state? https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... chrome/renderer/resources/extensions/app_window_custom_bindings.js:37: var appWindow = view.chrome.app.window.makeAppWindow(state); Nit: it would be nice to add a comment that this runs in the context of view.chrome.app.window. https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... chrome/renderer/resources/extensions/app_window_custom_bindings.js:57: AppWindow.prototype.dom = window; Unless there is a reason to keep it, I'd prefer this stuff to move off prototype / into the makeAppWindow handler. https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... chrome/renderer/resources/extensions/app_window_custom_bindings.js:59: return chromeHidden.appWindow.currentWindow; Would be nice to log an error here if currentWindow is null. https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... chrome/renderer/resources/extensions/app_window_custom_bindings.js:68: var newWindow = new AppWindow; you can get rid of two lines here by removing the vars and just assignint directly to chromeHidden.appWindow.foo https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... chrome/renderer/resources/extensions/app_window_custom_bindings.js:76: // with values from the browser thread not known by create() (or pass them This comment is now invalid I think? https://codereview.chromium.org/10910304/diff/6001/chrome/test/data/extension... File chrome/test/data/extensions/platform_apps/windows_api/test.js (right): https://codereview.chromium.org/10910304/diff/6001/chrome/test/data/extension... chrome/test/data/extensions/platform_apps/windows_api/test.js:18: chrome.test.assertEq(cw, win); You only need one of these asserts. But an extra assert that cw (or win).id == testId would be good :)
https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right): https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... chrome/renderer/resources/extensions/app_window_custom_bindings.js:22: // create failed? If given a callback, trigger it with an undefined object On 2012/09/17 08:33:31, benwells wrote: > Nit: Start comment with uppercase. Done. https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... chrome/renderer/resources/extensions/app_window_custom_bindings.js:34: viewId: viewId, On 2012/09/17 08:33:31, benwells wrote: > Does the viewId need to be in the state? 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 ;-). https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... chrome/renderer/resources/extensions/app_window_custom_bindings.js:37: var appWindow = view.chrome.app.window.makeAppWindow(state); On 2012/09/17 08:33:31, benwells wrote: > Nit: it would be nice to add a comment that this runs in the context of > view.chrome.app.window. Done. https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... chrome/renderer/resources/extensions/app_window_custom_bindings.js:57: AppWindow.prototype.dom = window; On 2012/09/17 08:33:31, benwells wrote: > Unless there is a reason to keep it, I'd prefer this stuff to move off prototype > / into the makeAppWindow handler. 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? https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... chrome/renderer/resources/extensions/app_window_custom_bindings.js:59: return chromeHidden.appWindow.currentWindow; On 2012/09/17 08:33:31, benwells wrote: > Would be nice to log an error here if currentWindow is null. Done. https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... chrome/renderer/resources/extensions/app_window_custom_bindings.js:68: var newWindow = new AppWindow; On 2012/09/17 08:33:31, benwells wrote: > you can get rid of two lines here by removing the vars and just assignint > directly to chromeHidden.appWindow.foo Done. https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... chrome/renderer/resources/extensions/app_window_custom_bindings.js:76: // with values from the browser thread not known by create() (or pass them On 2012/09/17 08:33:31, benwells wrote: > This comment is now invalid I think? 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 https://codereview.chromium.org/10910304/diff/6001/chrome/test/data/extension... File chrome/test/data/extensions/platform_apps/windows_api/test.js (right): https://codereview.chromium.org/10910304/diff/6001/chrome/test/data/extension... chrome/test/data/extensions/platform_apps/windows_api/test.js:18: chrome.test.assertEq(cw, win); On 2012/09/17 08:33:31, benwells wrote: > You only need one of these asserts. But an extra assert that cw (or win).id == > testId would be good :) Done.
https://codereview.chromium.org/10910304/diff/4007/chrome/common/extensions/a... File chrome/common/extensions/api/app_window.idl (right): https://codereview.chromium.org/10910304/diff/4007/chrome/common/extensions/a... chrome/common/extensions/api/app_window.idl:112: [internal, nocompile] static void makeAppWindow(object state); I don't think you really need to list this in the IDL. https://codereview.chromium.org/10910304/diff/4007/chrome/renderer/resources/... File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right): https://codereview.chromium.org/10910304/diff/4007/chrome/renderer/resources/... chrome/renderer/resources/extensions/app_window_custom_bindings.js:67: }); Why not just cache the appWindow as a closure variable? Instead of 'return new AppWindow', do: var appWindow = new AppWindow; apiFunctions.setHandleRequest('current', function() { return appWindow; });
https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right): https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... chrome/renderer/resources/extensions/app_window_custom_bindings.js:34: viewId: viewId, On 2012/09/17 09:22:13, tapted wrote: > On 2012/09/17 08:33:31, benwells wrote: > > Does the viewId need to be in the state? > > 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 ;-). Ah, ok. Let's take it out now, and add it back in when it is needed. https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... chrome/renderer/resources/extensions/app_window_custom_bindings.js:76: // with values from the browser thread not known by create() (or pass them On 2012/09/17 09:22:13, tapted wrote: > On 2012/09/17 08:33:31, benwells wrote: > > This comment is now invalid I think? > > 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 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. https://codereview.chromium.org/10910304/diff/4007/chrome/renderer/resources/... File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right): https://codereview.chromium.org/10910304/diff/4007/chrome/renderer/resources/... chrome/renderer/resources/extensions/app_window_custom_bindings.js:66: // First step is to determine whether we are actually an appWindow via IPC.. Given crbug.com/150033 I think this comment can be removed (and the console log message updated).
https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right): https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... chrome/renderer/resources/extensions/app_window_custom_bindings.js:34: viewId: viewId, On 2012/09/18 03:47:24, benwells wrote: > On 2012/09/17 09:22:13, tapted wrote: > > On 2012/09/17 08:33:31, benwells wrote: > > > Does the viewId need to be in the state? > > > > 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 ;-). > > Ah, ok. Let's take it out now, and add it back in when it is needed. Done. https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... chrome/renderer/resources/extensions/app_window_custom_bindings.js:76: // with values from the browser thread not known by create() (or pass them On 2012/09/18 03:47:24, benwells wrote: > On 2012/09/17 09:22:13, tapted wrote: > > On 2012/09/17 08:33:31, benwells wrote: > > > This comment is now invalid I think? > > > > 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 > > 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. 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.. https://codereview.chromium.org/10910304/diff/4007/chrome/common/extensions/a... File chrome/common/extensions/api/app_window.idl (right): https://codereview.chromium.org/10910304/diff/4007/chrome/common/extensions/a... chrome/common/extensions/api/app_window.idl:112: [internal, nocompile] static void makeAppWindow(object state); On 2012/09/17 17:46:58, jeremya wrote: > I don't think you really need to list this in the IDL. I seem to get errors if I remove it, along the lines of [12391:12391:0918/140748:INFO:CONSOLE(377)] "Error in event handler for 'undefined': Tried to set hook for unknown API "app.window.makeAppWindow" Error: Tried to set hook for unknown API "app.window.makeAppWindow" at APIFunctions._setHook (schema_generated_bindings:50:13) at APIFunctions.setHandleRequest (schema_generated_bindings:55:17) at NamespacedAPIFunctions.self.(anonymous function) [as setHandleRequest] (schema_generated_bindings:96:37) at app.window:71:16 at schema_generated_bindings:430:7 at Array.forEach (native) at schema_generated_bindings:419:20 at chrome.Event.dispatchToListener (event_bindings:387:21) at chrome.Event.dispatch_ (event_bindings:373:27) at chrome.Event.dispatch (event_bindings:393:17)", source: event_bindings (377) https://codereview.chromium.org/10910304/diff/4007/chrome/renderer/resources/... File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right): https://codereview.chromium.org/10910304/diff/4007/chrome/renderer/resources/... chrome/renderer/resources/extensions/app_window_custom_bindings.js:66: // First step is to determine whether we are actually an appWindow via IPC.. On 2012/09/18 03:47:24, benwells wrote: > Given crbug.com/150033 I think this comment can be removed (and the console log > message updated). Done. https://codereview.chromium.org/10910304/diff/4007/chrome/renderer/resources/... chrome/renderer/resources/extensions/app_window_custom_bindings.js:67: }); On 2012/09/17 17:46:58, jeremya wrote: > Why not just cache the appWindow as a closure variable? > > Instead of 'return new AppWindow', do: > > var appWindow = new AppWindow; > apiFunctions.setHandleRequest('current', function() { > return appWindow; > }); Done (detached from chromeHidden).
On 2012/09/18 04:17:24, tapted wrote: > https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... > File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right): > > https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... > chrome/renderer/resources/extensions/app_window_custom_bindings.js:34: viewId: > viewId, > On 2012/09/18 03:47:24, benwells wrote: > > On 2012/09/17 09:22:13, tapted wrote: > > > On 2012/09/17 08:33:31, benwells wrote: > > > > Does the viewId need to be in the state? > > > > > > 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 ;-). > > > > Ah, ok. Let's take it out now, and add it back in when it is needed. > > Done. > > https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... > chrome/renderer/resources/extensions/app_window_custom_bindings.js:76: // with > values from the browser thread not known by create() (or pass them > On 2012/09/18 03:47:24, benwells wrote: > > On 2012/09/17 09:22:13, tapted wrote: > > > On 2012/09/17 08:33:31, benwells wrote: > > > > This comment is now invalid I think? > > > > > > 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 > > > > 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. > > 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.. > > https://codereview.chromium.org/10910304/diff/4007/chrome/common/extensions/a... > File chrome/common/extensions/api/app_window.idl (right): > > https://codereview.chromium.org/10910304/diff/4007/chrome/common/extensions/a... > chrome/common/extensions/api/app_window.idl:112: [internal, nocompile] static > void makeAppWindow(object state); > On 2012/09/17 17:46:58, jeremya wrote: > > I don't think you really need to list this in the IDL. > > I seem to get errors if I remove it, along the lines of > > [12391:12391:0918/140748:INFO:CONSOLE(377)] "Error in event handler for > 'undefined': Tried to set hook for unknown API "app.window.makeAppWindow" Error: > Tried to set hook for unknown API "app.window.makeAppWindow" > at APIFunctions._setHook (schema_generated_bindings:50:13) > at APIFunctions.setHandleRequest (schema_generated_bindings:55:17) > at NamespacedAPIFunctions.self.(anonymous function) [as setHandleRequest] > (schema_generated_bindings:96:37) > at app.window:71:16 > at schema_generated_bindings:430:7 > at Array.forEach (native) > at schema_generated_bindings:419:20 > at chrome.Event.dispatchToListener (event_bindings:387:21) > at chrome.Event.dispatch_ (event_bindings:373:27) > at chrome.Event.dispatch (event_bindings:393:17)", source: event_bindings > (377) > If you take it out of the IDL, you don't need to / cannot set a custom handler via setHandleRequest. That is to set a custom implementation for something defined in IDL - i.e. don't send it through to the browser process as normally happens. Instead you would just define a function in the custom bindings JS and call it. However, if you do this the function would need to be on the right window, so that it is called with the right global context. Keeping it in IDL, as it is now, is probably the simplest way to get the context right. > https://codereview.chromium.org/10910304/diff/4007/chrome/renderer/resources/... > File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right): > > https://codereview.chromium.org/10910304/diff/4007/chrome/renderer/resources/... > chrome/renderer/resources/extensions/app_window_custom_bindings.js:66: // First > step is to determine whether we are actually an appWindow via IPC.. > On 2012/09/18 03:47:24, benwells wrote: > > Given crbug.com/150033 I think this comment can be removed (and the console > log > > message updated). > > Done. > > https://codereview.chromium.org/10910304/diff/4007/chrome/renderer/resources/... > chrome/renderer/resources/extensions/app_window_custom_bindings.js:67: }); > On 2012/09/17 17:46:58, jeremya wrote: > > Why not just cache the appWindow as a closure variable? > > > > Instead of 'return new AppWindow', do: > > > > var appWindow = new AppWindow; > > apiFunctions.setHandleRequest('current', function() { > > return appWindow; > > }); > > Done (detached from chromeHidden).
This version does
scoped_ptr<base::DictionaryValue> result(new base::DictionaryValue);
result->SetInteger("viewId", view_id);
result->SetString("id", shell_window->window_key());
SetResult(result.release());
in app_window_api.cc to "bootstrap" the cache with actual values, without
additional IPC calls (i.e. by passing back a dictionary rather than just the
routing ID).
https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right):
https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/app_window_custom_bindings.js:76: // with
values from the browser thread not known by create() (or pass them
On 2012/09/18 04:17:24, tapted wrote:
> 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..
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.
https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right): https://codereview.chromium.org/10910304/diff/6001/chrome/renderer/resources/... chrome/renderer/resources/extensions/app_window_custom_bindings.js:57: AppWindow.prototype.dom = window; On 2012/09/17 09:22:13, tapted wrote: > On 2012/09/17 08:33:31, benwells wrote: > > Unless there is a reason to keep it, I'd prefer this stuff to move off > prototype > > / into the makeAppWindow handler. > > 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? 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.
lgtm with nit and things to consider please wait for jeremya@'s lgtm before landing https://codereview.chromium.org/10910304/diff/14002/chrome/renderer/resources... File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right): https://codereview.chromium.org/10910304/diff/14002/chrome/renderer/resources... chrome/renderer/resources/extensions/app_window_custom_bindings.js:16: function(name, request, shellWindow) { Nit: windowParams feels like a better name than shellWindow https://codereview.chromium.org/10910304/diff/14002/chrome/renderer/resources... chrome/renderer/resources/extensions/app_window_custom_bindings.js:31: var appWindow = view.chrome.app.window.makeAppWindow(shellWindow); Something to consider: have makeAppWindow just make it, not return it, then call view.chrome.app.window.current() below. https://codereview.chromium.org/10910304/diff/14002/chrome/renderer/resources... chrome/renderer/resources/extensions/app_window_custom_bindings.js:67: chromeHidden.appWindow = { Something else to consider: don't use an object under chromeHidden, use two instead: chromeHidden.appWindowCache = ... chromeHidden.currentAppWindow = new AppWindow
@jeremya - how are we lookin'? https://codereview.chromium.org/10910304/diff/14002/chrome/renderer/resources... File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right): https://codereview.chromium.org/10910304/diff/14002/chrome/renderer/resources... chrome/renderer/resources/extensions/app_window_custom_bindings.js:16: function(name, request, shellWindow) { On 2012/09/19 00:09:21, benwells wrote: > Nit: windowParams feels like a better name than shellWindow Done. https://codereview.chromium.org/10910304/diff/14002/chrome/renderer/resources... chrome/renderer/resources/extensions/app_window_custom_bindings.js:67: chromeHidden.appWindow = { On 2012/09/19 00:09:21, benwells wrote: > Something else to consider: don't use an object under chromeHidden, use two > instead: > > chromeHidden.appWindowCache = ... > chromeHidden.currentAppWindow = new AppWindow Done.
Something still doesn't sit right with me about the makeAppWindow stuff. Here's some thinking: With this patch, there are two goals. One is to make the app.window.current() object the same object every time the function is called. That is accomplishable trivially by constructing the AppWindow once and storing it as a closure variable on the current() function. The other is to pave the way for a cache of information about the window that the browser knows but isn't exposed in the DOM APIs, like its minimized/maximized state and its ID. This goal is complicated slightly by the fact that there's only a very loose binding between a JS context (a RenderView) and a ShellWindow -- really, the only way a shell window RenderView knows it's in a ShellWindow is that we call GetView() on it. We should pass all the initial state needed through GetView() I think. Perhaps GetView() should simply pass on most of that state to another JS function in the child context, but at least that way it's properly private and doesn't need an IDL function. sketch of what i'm thinking: - Context A calls app.window.create() - Context A gets a callback saying 'i created the window, here's the initial state' - Context A calls GetView() and passes in the state - GetView() finds context B from the viewId - GetView() calls a JS function "initializeWindow()" (or something) in context B, passes in the state - Context B creates its AppWindow - Context A ends up with the AppWindow (you could either thread it through as a return value from initialize() or leave it how it is with context A calling current() on context B) Please let me know if none of this makes any sense and we should schedule a VC. Sorry I've been so late in reviewing this code! https://codereview.chromium.org/10910304/diff/18002/chrome/common/extensions/... File chrome/common/extensions/api/app_window.idl (right): https://codereview.chromium.org/10910304/diff/18002/chrome/common/extensions/... chrome/common/extensions/api/app_window.idl:112: [internal, nocompile] static void makeAppWindow(object state); I don't think [internal] means anything when applied to functions; generally it's specified on the namespace and its behaviour is to move it into the chromeHidden.internalAPIs namespace. So as is, client code would be able to call this function. Perhaps a better way to do this would be to pass windowParams through to GetView() whole, and then have GetView() pass it to a method in platform_app.js via module_system()->CallModuleMethod() (as in chrome/renderer/extensions/app_window_custom_bindings.cc). Ask koz about this too :)
On 2012/09/19 16:13:15, jeremya wrote: > Something still doesn't sit right with me about the makeAppWindow stuff. > > Here's some thinking: > > With this patch, there are two goals. One is to make the app.window.current() > object the same object every time the function is called. That is accomplishable > trivially by constructing the AppWindow once and storing it as a closure > variable on the current() function. > > The other is to pave the way for a cache of information about the window that > the browser knows but isn't exposed in the DOM APIs, like its > minimized/maximized state and its ID. This goal is complicated slightly by the > fact that there's only a very loose binding between a JS context (a RenderView) > and a ShellWindow -- really, the only way a shell window RenderView knows it's > in a ShellWindow is that we call GetView() on it. > > We should pass all the initial state needed through GetView() I think. Perhaps > GetView() should simply pass on most of that state to another JS function in the > child context, but at least that way it's properly private and doesn't need an > IDL function. > > sketch of what i'm thinking: > - Context A calls app.window.create() > - Context A gets a callback saying 'i created the window, here's the initial > state' > - Context A calls GetView() and passes in the state > - GetView() finds context B from the viewId > - GetView() calls a JS function "initializeWindow()" (or something) in context > B, passes in the state > - Context B creates its AppWindow > - Context A ends up with the AppWindow (you could either thread it through as a > return value from initialize() or leave it how it is with context A calling > current() on context B) > > Please let me know if none of this makes any sense and we should schedule a VC. > Sorry I've been so late in reviewing this code! > > https://codereview.chromium.org/10910304/diff/18002/chrome/common/extensions/... > File chrome/common/extensions/api/app_window.idl (right): > > https://codereview.chromium.org/10910304/diff/18002/chrome/common/extensions/... > chrome/common/extensions/api/app_window.idl:112: [internal, nocompile] static > void makeAppWindow(object state); > I don't think [internal] means anything when applied to functions; generally > it's specified on the namespace and its behaviour is to move it into the > chromeHidden.internalAPIs namespace. So as is, client code would be able to call > this function. > > Perhaps a better way to do this would be to pass windowParams through to > GetView() whole, and then have GetView() pass it to a method in platform_app.js > via module_system()->CallModuleMethod() (as in > chrome/renderer/extensions/app_window_custom_bindings.cc). Ask koz about this > too :) Jeremy, can you give more details about what doesn't sit right? Is it just that client code can call makeAppWindow()? That doesn't feel like a huge problem, I'm not really sure if it is worth all the effort and extra obfuscation to fix it. If it is, we could do it in another patch (after updating this to log to log to the console if makeAppWindow is ever called twice in a context). Or is it something else? Something that might be cleaner is if we bind the app window as suggested in a closure, and rename makeAppWindow to be something like cacheAppWindowInformation. If this is something you're feeling strongly about but can't express a VC is probably best :)
On 2012/09/20 01:35:29, benwells wrote: > On 2012/09/19 16:13:15, jeremya wrote: > > Something still doesn't sit right with me about the makeAppWindow stuff. > > > > Here's some thinking: > > > > With this patch, there are two goals. One is to make the app.window.current() > > object the same object every time the function is called. That is > accomplishable > > trivially by constructing the AppWindow once and storing it as a closure > > variable on the current() function. > > > > The other is to pave the way for a cache of information about the window that > > the browser knows but isn't exposed in the DOM APIs, like its > > minimized/maximized state and its ID. This goal is complicated slightly by the > > fact that there's only a very loose binding between a JS context (a > RenderView) > > and a ShellWindow -- really, the only way a shell window RenderView knows it's > > in a ShellWindow is that we call GetView() on it. > > > > We should pass all the initial state needed through GetView() I think. Perhaps > > GetView() should simply pass on most of that state to another JS function in > the > > child context, but at least that way it's properly private and doesn't need an > > IDL function. > > > > sketch of what i'm thinking: > > - Context A calls app.window.create() > > - Context A gets a callback saying 'i created the window, here's the initial > > state' > > - Context A calls GetView() and passes in the state > > - GetView() finds context B from the viewId > > - GetView() calls a JS function "initializeWindow()" (or something) in context > > B, passes in the state > > - Context B creates its AppWindow > > - Context A ends up with the AppWindow (you could either thread it through as > a > > return value from initialize() or leave it how it is with context A calling > > current() on context B) > > > > Please let me know if none of this makes any sense and we should schedule a > VC. > > Sorry I've been so late in reviewing this code! > > > > > https://codereview.chromium.org/10910304/diff/18002/chrome/common/extensions/... > > File chrome/common/extensions/api/app_window.idl (right): > > > > > https://codereview.chromium.org/10910304/diff/18002/chrome/common/extensions/... > > chrome/common/extensions/api/app_window.idl:112: [internal, nocompile] static > > void makeAppWindow(object state); > > I don't think [internal] means anything when applied to functions; generally > > it's specified on the namespace and its behaviour is to move it into the > > chromeHidden.internalAPIs namespace. So as is, client code would be able to > call > > this function. > > > > Perhaps a better way to do this would be to pass windowParams through to > > GetView() whole, and then have GetView() pass it to a method in > platform_app.js > > via module_system()->CallModuleMethod() (as in > > chrome/renderer/extensions/app_window_custom_bindings.cc). Ask koz about this > > too :) > > Jeremy, can you give more details about what doesn't sit right? Is it just that > client code can call makeAppWindow()? > > That doesn't feel like a huge problem, I'm not really sure if it is worth all > the effort and extra obfuscation to fix it. If it is, we could do it in another > patch (after updating this to log to log to the console if makeAppWindow is ever > called twice in a context). > > Or is it something else? Something that might be cleaner is if we bind the app > window as suggested in a closure, and rename makeAppWindow to be something like > cacheAppWindowInformation. > > If this is something you're feeling strongly about but can't express a VC is > probably best :) At this point y'all are probably on a plane to here, so we should talk about it when you get here :)
lgtm with makeAppWindow -> initializeAppWindow and appWindowCache -> appWindowData
On 2012/09/24 22:53:33, jeremya wrote: > lgtm with makeAppWindow -> initializeAppWindow and appWindowCache -> > appWindowData (and remove [internal] in the IDL, since it's meaningless)
Reaching out to Mihai for review/feedback (change in extensions/ui -- accessor for window_key ShellWindow data member). Still fighting the win_rel try bot ... out of disk space, and pepper API changes that are causing timeouts.. https://chromiumcodereview.appspot.com/10910304/diff/18002/chrome/common/exte... File chrome/common/extensions/api/app_window.idl (right): https://chromiumcodereview.appspot.com/10910304/diff/18002/chrome/common/exte... chrome/common/extensions/api/app_window.idl:112: [internal, nocompile] static void makeAppWindow(object state); On 2012/09/19 16:13:15, jeremya wrote: > I don't think [internal] means anything when applied to functions; generally > it's specified on the namespace and its behaviour is to move it into the > chromeHidden.internalAPIs namespace. So as is, client code would be able to call > this function. dropped 'internal'
LGTM Given Ben's impending fix for http://crbug.com/150033, the bit in the CL description about reloading should probably be removed. https://chromiumcodereview.appspot.com/10910304/diff/29008/chrome/common/exte... File chrome/common/extensions/api/app_window.idl (right): https://chromiumcodereview.appspot.com/10910304/diff/29008/chrome/common/exte... chrome/common/extensions/api/app_window.idl:112: [nocompile] static void initializeAppWindow(object state); This also needs a [nodoc] https://chromiumcodereview.appspot.com/10910304/diff/29008/chrome/renderer/re... File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right): https://chromiumcodereview.appspot.com/10910304/diff/29008/chrome/renderer/re... chrome/renderer/resources/extensions/app_window_custom_bindings.js:18: // Create failed? If given a callback, trigger it with an undefined object Nit: period at the end of the comment sentence (applies to all new comments). https://chromiumcodereview.appspot.com/10910304/diff/29008/chrome/renderer/re... chrome/renderer/resources/extensions/app_window_custom_bindings.js:63: id: params.id ? params.id : '' Nit: params.id || '' is more idiomatic.
Fixed the idl and nits. Thanks! https://chromiumcodereview.appspot.com/10910304/diff/29008/chrome/common/exte... File chrome/common/extensions/api/app_window.idl (right): https://chromiumcodereview.appspot.com/10910304/diff/29008/chrome/common/exte... chrome/common/extensions/api/app_window.idl:112: [nocompile] static void initializeAppWindow(object state); On 2012/09/26 22:22:23, Mihai Parparita wrote: > This also needs a [nodoc] Done. https://chromiumcodereview.appspot.com/10910304/diff/29008/chrome/renderer/re... File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right): https://chromiumcodereview.appspot.com/10910304/diff/29008/chrome/renderer/re... chrome/renderer/resources/extensions/app_window_custom_bindings.js:18: // Create failed? If given a callback, trigger it with an undefined object On 2012/09/26 22:22:23, Mihai Parparita wrote: > Nit: period at the end of the comment sentence (applies to all new comments). Done. https://chromiumcodereview.appspot.com/10910304/diff/29008/chrome/renderer/re... chrome/renderer/resources/extensions/app_window_custom_bindings.js:63: id: params.id ? params.id : '' On 2012/09/26 22:22:23, Mihai Parparita wrote: > Nit: params.id || '' is more idiomatic. Done (also: cool).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tapted@chromium.org/10910304/44001
Change committed as 158996 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
