added windows.getWindows() and tabs.moveTab()

added windows.getWindows() and tabs.moveTab()


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

[rafaelw, 2009-04-16 22:13:34]

two more api functions

[Aaron Boodman, 2009-04-16 22:25:10]

lgtm

http://codereview.chromium.org/79026/diff/1/5
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/79026/diff/1/5#newcode225
Line 225: const DictionaryValue *args_hash = static_cast<const
DictionaryValue*>(args_);
Nit: I think just 'args' is fine for this variable name.

http://codereview.chromium.org/79026/diff/1/6
File chrome/renderer/renderer_resources.grd (left):

http://codereview.chromium.org/79026/diff/1/6#oldcode3
Line 3: without changes to the corresponding grd file.  -->
:)

[Matt Perry, 2009-04-16 22:25:40]

http://codereview.chromium.org/79026/diff/1/5
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/79026/diff/1/5#newcode13
Line 13: // Forward declare static helper functions defined below.
forward declare wouldn't be necessary if the helpers were defined at the top of
the file.  I vote for moving them.  Aaron?

http://codereview.chromium.org/79026/diff/1/5#newcode43
Line 43: // schema validation should prevent this.
indent

http://codereview.chromium.org/79026/diff/1/5#newcode63
Line 63: if (window_ids.size() == 0) {
nit: rewrite as 'bool all_windows = (window_ids.size() == 0);'

http://codereview.chromium.org/79026/diff/1/5#newcode245
Line 245: (new_index < tab_strip->count())) {
it might make sense to auto clamp new_index to the max possible.

http://codereview.chromium.org/79026/diff/1/7
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/79026/diff/1/7#newcode76
Line 76: // TODO(rafaelw):  support windowQuery==null.
that'll come for free with some json schema changes, so I think this TODO can be
removed.

http://codereview.chromium.org/79026/diff/1/7#newcode78
Line 78: validate(arguments, arguments.callee.params);
nit: maybe validate should default to validating against firstarg.callee.params?
 Aaron?

[Aaron Boodman, 2009-04-16 22:58:54]

http://codereview.chromium.org/79026/diff/1/7
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/79026/diff/1/7#newcode78
Line 78: validate(arguments, arguments.callee.params);
On 2009/04/16 22:25:40, Matt Perry wrote:
> nit: maybe validate should default to validating against
firstarg.callee.params?
>  Aaron?

Ok, I give. Let's just let sendRequest validate by default. You can write it
like this:

chromium.tabs.getWindows = sendRequest.partial(GetWindows);

Then add this line:
validate(Array.prototype.slice.apply(arguments, 1), 
         request.params)

to sendRequest().

...or something...

[rafaelw, 2009-04-16 23:11:26]

http://codereview.chromium.org/79026/diff/1/5
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/79026/diff/1/5#newcode43
Line 43: // schema validation should prevent this.
On 2009/04/16 22:25:40, Matt Perry wrote:
> indent

Done.

http://codereview.chromium.org/79026/diff/1/5#newcode63
Line 63: if (window_ids.size() == 0) {
On 2009/04/16 22:25:40, Matt Perry wrote:
> nit: rewrite as 'bool all_windows = (window_ids.size() == 0);'

Done.

http://codereview.chromium.org/79026/diff/1/5#newcode225
Line 225: const DictionaryValue *args_hash = static_cast<const
DictionaryValue*>(args_);
On 2009/04/16 22:25:11, Aaron Boodman wrote:
> Nit: I think just 'args' is fine for this variable name.

Done.

http://codereview.chromium.org/79026/diff/1/5#newcode245
Line 245: (new_index < tab_strip->count())) {
On 2009/04/16 22:25:40, Matt Perry wrote:
> it might make sense to auto clamp new_index to the max possible.

Done.

http://codereview.chromium.org/79026/diff/1/7
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/79026/diff/1/7#newcode76
Line 76: // TODO(rafaelw):  support windowQuery==null.
On 2009/04/16 22:25:40, Matt Perry wrote:
> that'll come for free with some json schema changes, so I think this TODO can
be
> removed.

Done.

[Matt Perry, 2009-04-16 23:20:22]

LGTM