Make 'options' and callback arg to chrome.appWindow.create optional.

Make 'options' and callback arg to chrome.appWindow.create optional.

R=asargent@chromium.org


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139274

[jeremya, 2012-05-28 04:28:36]

asargent- IDL changes
mihaip- shell window changes
kalman- sanity check

Thanks!

[not at google - send to devlin, 2012-05-28 04:46:43]

lgtm

https://chromiumcodereview.appspot.com/10456003/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://chromiumcodereview.appspot.com/10456003/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/app_window/app_window_api.cc:34: if
(params->options.get()) {
nit: save a reference to "params->options.get()" rather than typing it
repeatedly.

https://chromiumcodereview.appspot.com/10456003/diff/1/chrome/renderer/resour...
File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right):

https://chromiumcodereview.appspot.com/10456003/diff/1/chrome/renderer/resour...
chrome/renderer/resources/extensions/app_window_custom_bindings.js:20:
request.callback = null;
nits: no space in "function (", rename variable "response" to "view_id" and
"dom" to "view", put the "request.callback = null" inside the "if
(request.callback)".  Consider using "delete" instead of = null.

[jeremya, 2012-05-28 06:49:01]

https://chromiumcodereview.appspot.com/10456003/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://chromiumcodereview.appspot.com/10456003/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/app_window/app_window_api.cc:34: if
(params->options.get()) {
On 2012/05/28 04:46:43, kalman wrote:
> nit: save a reference to "params->options.get()" rather than typing it
> repeatedly.

Done.

https://chromiumcodereview.appspot.com/10456003/diff/1/chrome/renderer/resour...
File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right):

https://chromiumcodereview.appspot.com/10456003/diff/1/chrome/renderer/resour...
chrome/renderer/resources/extensions/app_window_custom_bindings.js:20:
request.callback = null;
On 2012/05/28 04:46:43, kalman wrote:
> nits: no space in "function (", rename variable "response" to "view_id" and
> "dom" to "view", put the "request.callback = null" inside the "if
> (request.callback)".  Consider using "delete" instead of = null.

Done. Why delete instead of = null?

[not at google - send to devlin, 2012-05-28 06:52:55]

https://chromiumcodereview.appspot.com/10456003/diff/1/chrome/renderer/resour...
File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right):

https://chromiumcodereview.appspot.com/10456003/diff/1/chrome/renderer/resour...
chrome/renderer/resources/extensions/app_window_custom_bindings.js:20:
request.callback = null;
On 2012/05/28 06:49:01, jeremya wrote:
> On 2012/05/28 04:46:43, kalman wrote:
> > nits: no space in "function (", rename variable "response" to "view_id" and
> > "dom" to "view", put the "request.callback = null" inside the "if
> > (request.callback)".  Consider using "delete" instead of = null.
> 
> Done. Why delete instead of = null?

Personal preference, I think it's neater in this context, but it's effectively
the same (that or the more typical = undefined). Don't feel strongly at all
though.

[jeremya, 2012-05-28 07:06:45]

https://chromiumcodereview.appspot.com/10456003/diff/1/chrome/renderer/resour...
File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right):

https://chromiumcodereview.appspot.com/10456003/diff/1/chrome/renderer/resour...
chrome/renderer/resources/extensions/app_window_custom_bindings.js:20:
request.callback = null;
On 2012/05/28 06:52:55, kalman wrote:
> On 2012/05/28 06:49:01, jeremya wrote:
> > On 2012/05/28 04:46:43, kalman wrote:
> > > nits: no space in "function (", rename variable "response" to "view_id"
and
> > > "dom" to "view", put the "request.callback = null" inside the "if
> > > (request.callback)".  Consider using "delete" instead of = null.
> > 
> > Done. Why delete instead of = null?
> 
> Personal preference, I think it's neater in this context, but it's effectively
> the same (that or the more typical = undefined). Don't feel strongly at all
> though.

. o O ( I wonder if one is more or less optimal for v8 than the other. Does
`delete` cause it to repack data? Obviously irrelevant here, but interesting :)
)

[not at google - send to devlin, 2012-05-28 08:05:53]

https://chromiumcodereview.appspot.com/10456003/diff/1/chrome/renderer/resour...
File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right):

https://chromiumcodereview.appspot.com/10456003/diff/1/chrome/renderer/resour...
chrome/renderer/resources/extensions/app_window_custom_bindings.js:20:
request.callback = null;
On 2012/05/28 07:06:45, jeremya wrote:
> On 2012/05/28 06:52:55, kalman wrote:
> > On 2012/05/28 06:49:01, jeremya wrote:
> > > On 2012/05/28 04:46:43, kalman wrote:
> > > > nits: no space in "function (", rename variable "response" to "view_id"
> and
> > > > "dom" to "view", put the "request.callback = null" inside the "if
> > > > (request.callback)".  Consider using "delete" instead of = null.
> > > 
> > > Done. Why delete instead of = null?
> > 
> > Personal preference, I think it's neater in this context, but it's
effectively
> > the same (that or the more typical = undefined). Don't feel strongly at all
> > though.
> 
> . o O ( I wonder if one is more or less optimal for v8 than the other. Does
> `delete` cause it to repack data? Obviously irrelevant here, but interesting
:)
> )

Probably not worth thinking about too much :)

[asargent_no_longer_on_chrome, 2012-05-28 16:22:50]

lgtm

https://chromiumcodereview.appspot.com/10456003/diff/5/chrome/common/extensio...
File chrome/common/extensions/api/app_window.idl (right):

https://chromiumcodereview.appspot.com/10456003/diff/5/chrome/common/extensio...
chrome/common/extensions/api/app_window.idl:30: CreateWindowCallback callback);
any reason not to have the create callback optional as well?

[jeremya, 2012-05-28 23:11:38]

https://chromiumcodereview.appspot.com/10456003/diff/5/chrome/common/extensio...
File chrome/common/extensions/api/app_window.idl (right):

https://chromiumcodereview.appspot.com/10456003/diff/5/chrome/common/extensio...
chrome/common/extensions/api/app_window.idl:30: CreateWindowCallback callback);
On 2012/05/28 16:22:50, Antony Sargent wrote:
> any reason not to have the create callback optional as well?

Seems not :) Done.

[commit-bot: I haz the power, 2012-05-28 23:11:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/10456003/9001

[commit-bot: I haz the power, 2012-05-28 23:11:58]

Failed to apply patch for
chrome/browser/extensions/api/app_window/app_window_api.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/api/app_window/app_window_api.cc
Hunk #2 FAILED at 32.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/extensions/api/app_window/app_window_api.cc.rej

[commit-bot: I haz the power, 2012-05-28 23:17:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/10456003/5002

[commit-bot: I haz the power, 2012-05-28 23:33:19]

Try job failure for 10456003-5002 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-05-29 00:07:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/10456003/1010

[commit-bot: I haz the power, 2012-05-29 01:48:49]

Change committed as 139274

[Mihai Parparita -not on Chrome, 2012-05-29 06:14:42]

https://chromiumcodereview.appspot.com/10456003/diff/1010/chrome/browser/exte...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://chromiumcodereview.appspot.com/10456003/diff/1010/chrome/browser/exte...
chrome/browser/extensions/api/app_window/app_window_api.cc:21: namespace Create
= app_window::Create;
Nit: would using app_window::Create have worked here?

https://chromiumcodereview.appspot.com/10456003/diff/1010/chrome/renderer/res...
File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right):

https://chromiumcodereview.appspot.com/10456003/diff/1010/chrome/renderer/res...
chrome/renderer/resources/extensions/app_window_custom_bindings.js:14:
apiFunctions.setCustomCallback('create', function(name, request, view_id) {
Nit: view_id should be called viewId per the JS style guide.

https://chromiumcodereview.appspot.com/10456003/diff/1010/chrome/renderer/res...
chrome/renderer/resources/extensions/app_window_custom_bindings.js:18: if
(request.callback) {
Nit: This can become an early return at the start of the function (no point in
trying to get the view if no one wants it).