Added populate parameter to chrome.windows.get, chrome.windows.getCurrent,

Added populate parameter to chrome.windows.get, chrome.windows.getCurrent,
chrome.windows.getLastFocused

BUG=53516
TEST=Run chrome.windows.get(id, {"populate":true}, fun) from an extension

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

[Aaron Boodman, 2011-12-16 20:41:24]

Sigh, sadly, I don't think it's quite so simple.

We need to support extensions that call the old APIs that are already in the
wild (one of the core principles of the extension platform is that we don't
break backward compatibility).

In many cases, we thought ahead and made the APIs more naturally extensible by
putting a 'details' object as a second param from the beginning. But it looks
like not in these cases.

You can fix this by adding code to
chrome/renderer/resources/extensions/schema_generated_bindings.js. There is a
section near the bottom where we have many custom hooks for massaging the
arguments of various APIs. Take a look at browserAction.setIcon, for an example.

You can do something like this to support both versions of the API. You'll leave
the C++ the way it is, and then catch calls using the old signature and update
them to the new one.

Another, perhaps easier, approach would be to use the choice element in the JSON
schema. Make the second argument a choice between function and object, and the
final argument an optional function. Then in the C++, check which one you got.

Be sure to run the API tests to ensure that old code continues to work. Build
the browser_tests binary, and run ./out/Release/browser_tests
--gtest_filter=ExtensionApiTest.Tab*

[Matt Tytel, 2011-12-16 21:20:31]

I was just realizing this. I think I may have accidentally tested the old
functionality on an installed version of Chrome :/

So I'm confused on what "optional" means in extension_api.json. I thought it
meant omit-able but now my current understanding is:
If there is an "optional" value defined in "parameters", that function can
accept null in place of it but CAN'T omit that parameter.
If there is an "optional" value defined in "properties", that object CAN omit
that property.
Is this correct?

[Matt Tytel, 2011-12-16 23:54:27]

Well, I see what the situation is after looking at bug 29215.

I've uploaded the fix to make it backwards compatible but I'd like to see 29215
implemented soon so the code I added to schema_generated_bindings.js doesn't
stay there.

[Aaron Boodman, 2011-12-17 07:12:58]

http://codereview.chromium.org/8969011/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/8969011/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_module.cc:194: DictionaryValue* args;
Always initialize primitives (to NULL in this case).

http://codereview.chromium.org/8969011/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tabs_test.cc (right):

http://codereview.chromium.org/8969011/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_test.cc:72: ListValue* tabs;
*twitch*

http://codereview.chromium.org/8969011/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_test.cc:140: // id should match the
window that was passed to RunFunction
Comment doesn't make sense. You mean something like: returned id should match
new_browser's id. Same comment multiple places in this file.

http://codereview.chromium.org/8969011/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_test.cc:140: // id should match the
window that was passed to RunFunction
Also, nit, but comments should generally be complete sentences, with
capitalization and periods (except in the case of end-of-line comments that
document a parameter).

http://codereview.chromium.org/8969011/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_test.cc:142: ListValue* tabs;
!

http://codereview.chromium.org/8969011/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_test.cc:160: // Note that "last
focused" means the "top" most window
Missing final period.

http://codereview.chromium.org/8969011/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_test.cc:162: int focus_id =
ExtensionTabUtil::GetWindowId(new_browser);
focused_window_id?

http://codereview.chromium.org/8969011/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_test.cc:172: ListValue* tabs;
!! (I'm going to stop pointing these out now)

http://codereview.chromium.org/8969011/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_test.cc:194: Browser* bro =
CreateBrowser(browser()->profile());
Avoid abbreviations. Just use the term "new_browser" here.

http://codereview.chromium.org/8969011/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_test.cc:217:
std::sort(window_ids.begin(), window_ids.end());
You should just use std::set. They come with an equality operator, so you don't
need to do this by hand. And you also don't need to sort them.

[Matt Tytel, 2011-12-17 07:43:32]

http://codereview.chromium.org/8969011/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tabs_test.cc (right):

http://codereview.chromium.org/8969011/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_test.cc:140: // id should match the
window that was passed to RunFunction
On 2011/12/17 07:12:58, Aaron Boodman wrote:
> Comment doesn't make sense. You mean something like: returned id should match
> new_browser's id. Same comment multiple places in this file.

Here I want to distinguish between the functionality of GetLastFocusedWindow.
That it should match whatever browser we "called" the GetCurrentWindowFunction
from.

http://codereview.chromium.org/8969011/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_test.cc:194: Browser* bro =
CreateBrowser(browser()->profile());
On 2011/12/17 07:12:58, Aaron Boodman wrote:
> Avoid abbreviations. Just use the term "new_browser" here.

You got it bro ;)

[Aaron Boodman, 2011-12-17 08:02:38]

http://codereview.chromium.org/8969011/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tabs_test.cc (right):

http://codereview.chromium.org/8969011/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_test.cc:140: // id should match the
window that was passed to RunFunction
On 2011/12/17 07:43:32, mtytel wrote:
> On 2011/12/17 07:12:58, Aaron Boodman wrote:
> > Comment doesn't make sense. You mean something like: returned id should
match
> > new_browser's id. Same comment multiple places in this file.
> 
> Here I want to distinguish between the functionality of GetLastFocusedWindow.
> That it should match whatever browser we "called" the GetCurrentWindowFunction
> from.

Ok, fair enough. In that case, it might be a little more clear to say something
like "...should match the window_id of the Browser instance that was passed to
RunFunction"  - to tie things back to the Browser class and new_browser
variable.

[Matt Tytel, 2011-12-19 02:26:03]

The changes you have requested are complete. What do you think?

[Aaron Boodman, 2011-12-19 02:40:00]

LGTM - nice work.

http://codereview.chromium.org/8969011/diff/18001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/8969011/diff/18001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_module.cc:189: int window_id = -1;
Thanks for fixing bad older code. There was a lot of it in this file!

[commit-bot: I haz the power, 2011-12-19 02:40:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtytel@chromium.org/8969011/18001

[commit-bot: I haz the power, 2011-12-19 02:40:14]

Can't apply patch for file chrome/common/extensions/api/extension_api.json.
While running patch -p0 --forward --force;
patching file chrome/common/extensions/api/extension_api.json
Hunk #1 FAILED at 961.
Hunk #2 FAILED at 977.
Hunk #3 FAILED at 993.
3 out of 3 hunks FAILED -- saving rejects to file
chrome/common/extensions/api/extension_api.json.rej

[Aaron Boodman, 2011-12-19 02:48:55]

http://bit.ly/5gsQlt

Try syncing, resolving, and uploading a new patch.

- a

On Sun, Dec 18, 2011 at 6:40 PM,  <commit-bot@chromium.org> wrote:
> Can't apply patch for file chrome/common/extensions/api/extension_api.json.
> While running patch -p0 --forward --force;
> patching file chrome/common/extensions/api/extension_api.json
> Hunk #1 FAILED at 961.
> Hunk #2 FAILED at 977.
> Hunk #3 FAILED at 993.
> 3 out of 3 hunks FAILED -- saving rejects to file
> chrome/common/extensions/api/extension_api.json.rej
>
>
> http://codereview.chromium.org/8969011/

[Matt Tytel, 2011-12-19 02:59:54]

aw man..

[Matt Tytel, 2011-12-19 03:03:26]

On 2011/12/19 02:59:54, mtytel wrote:
> aw man..

Oop, ignore this one, file was renamed.

[clintstaley_gmail.com, 2011-12-19 03:12:42]

Aaron,

Matt would *love* to see this thing landed tonight, but he is taking off 
for vacation in about one hour.  IIRC, we don't land changes unless the 
author is prepared to stand by in the event of tree closure.  How long 
must he stick around if the tree doesn't go red?

Thanks,

Clint

On 12/18/2011 6:40 PM, aa@chromium.org wrote:
> LGTM - nice work.
>
>
>
http://codereview.chromium.org/8969011/diff/18001/chrome/browser/extensions/e...

>
> File chrome/browser/extensions/extension_tabs_module.cc (right):
>
>
http://codereview.chromium.org/8969011/diff/18001/chrome/browser/extensions/e...

>
> chrome/browser/extensions/extension_tabs_module.cc:189: int window_id =
> -1;
> Thanks for fixing bad older code. There was a lot of it in this file!
>
> http://codereview.chromium.org/8969011/
>

[Matt Tytel, 2011-12-19 03:44:10]

This code is ready for another go. Can it still go through tonight?

[Aaron Boodman, 2011-12-19 05:07:11]

I'm landing these w/ the commit queue. It's OK to land changes this way if you
aren't around. The tradeoff is that sometimes the commit queue fails. So cross
your fingers.

[commit-bot: I haz the power, 2011-12-19 05:07:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtytel@chromium.org/8969011/22004

[commit-bot: I haz the power, 2011-12-19 05:07:25]

Can't apply patch for file
chrome/common/extensions/docs/examples/api/devtools/network/chrome-firephp.zip.
While running patch -p0 --forward --force;
patching file
chrome/common/extensions/docs/examples/api/devtools/network/chrome-firephp.zip
Hunk #1 FAILED at 1.
Hunk #2 FAILED at 7.
Hunk #3 FAILED at 23.
Hunk #4 FAILED at 218.
Hunk #5 FAILED at 233.
5 out of 5 hunks FAILED -- saving rejects to file
chrome/common/extensions/docs/examples/api/devtools/network/chrome-firephp.zip.rej

[Aaron Boodman, 2011-12-19 05:08:29]

Oh, it's because the patch contains binary files. Can't land those via CQ. Will
try a normal one.

[Matt Tytel, 2011-12-19 05:15:37]

I don't get updates on the normal ones do I?

[Aaron Boodman, 2011-12-19 05:20:57]

Hm, I'm not even sure how to land a change like this. None of the tools based on
patch will accept it since it includes a binary file. So I can't even pull it
into my client and do it for you. This is where being a provisional committer
comes in handy :).

What changed about the zip files? Perhaps we can land those separately, or you
can send them to me separately? If you're still around, I'm in IRC for awhile
tonight.

[Matt Tytel, 2011-12-19 05:39:09]

No more generated files.

[commit-bot: I haz the power, 2011-12-19 05:39:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtytel@chromium.org/8969011/24005

[Aaron Boodman, 2011-12-19 06:09:03]

http://codereview.chromium.org/8969011/diff/24005/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tabs_test.cc (right):

http://codereview.chromium.org/8969011/diff/24005/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_test.cc:197: for (uint i = 0; i <
NUM_WINDOWS - 1; ++i) {
Dang, bad reviewer. This should be size_t to match NUM_WINDOWS. Also, we don't
use uint or many of the other integral types in Chrome code. Just boolean, byte,
int, int64, and size_t.

[Matt Tytel, 2011-12-19 06:22:40]

uint -> size_t
And I'm ready for another go.

[Matt Tytel, 2011-12-19 06:53:15]

Deleting the wrong file from a change list makes a bad first impression..

[commit-bot: I haz the power, 2011-12-19 06:56:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtytel@chromium.org/8969011/27012

[commit-bot: I haz the power, 2011-12-19 08:51:19]

Change committed as 114963