Add browser tests for extensions tab API.

Add browser tests for extensions tab API.

BUG=18442
TEST=none


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

[Erik does not do reviews, 2009-08-28 00:30:56]

+rafaelw (who knows this API better than I)

http://codereview.chromium.org/174633/diff/1/3
File chrome/browser/extensions/extension_test_api.cc (right):

http://codereview.chromium.org/174633/diff/1/3#newcode35
Line 35: printf("%s\n", message.c_str());
should this be LOG(INFO)?  I forget if that also prints to stdout by default. 
If not, you could do both.

http://codereview.chromium.org/174633/diff/1/9
File chrome/test/data/extensions/api_test/tabs/test.js (right):

http://codereview.chromium.org/174633/diff/1/9#newcode56
Line 56: 
remove extra newline

http://codereview.chromium.org/174633/diff/1/9#newcode67
Line 67: function moreSetup() {
this seems weird to me.  why not just put these inside of the create callback of
the previous setup function?

http://codereview.chromium.org/174633/diff/1/9#newcode81
Line 81: // The most recent tab should be selected
newline above comment

http://codereview.chromium.org/174633/diff/1/9#newcode104
Line 104: chrome.tabs.update(testTabId, {"selected":true, "url": "chrome://c"},
why not just move this inline into the next test?

http://codereview.chromium.org/174633/diff/1/9#newcode126
Line 126: chrome.windows.remove(firstWindowId, function() { assertNoLastError();
});
just use assertNoLastError by itself like you do later

http://codereview.chromium.org/174633/diff/1/9#newcode127
Line 127: chrome.windows.remove(secondWindowId, testFunction());
the fact that these two are written differently makes me think that
testFunction() isn't necessarily the best model.

http://codereview.chromium.org/174633/diff/1/9#newcode222
Line 222: // We don't use assertEq since these strings are huge and we don't
newline above comment

http://codereview.chromium.org/174633/diff/1/9#newcode260
Line 260: assertNoLastError());
no () on assertNoLastError

http://codereview.chromium.org/174633/diff/1/9#newcode266
Line 266: assertEq(moveTabIds['a'], tabid);
don't all of these need to call succeed() in order to do the next test?

[rafaelw, 2009-08-28 21:16:34]

http://codereview.chromium.org/174633/diff/1/3
File chrome/browser/extensions/extension_test_api.cc (right):

http://codereview.chromium.org/174633/diff/1/3#newcode35
Line 35: printf("%s\n", message.c_str());
It seems like console.log already prints to LOG(INFO). If I run matt's messaging
browser tests inside VS, I see all the console.log output. Is there a flag you
can pass during runtime that would output LOG(INFO) to stdout and avoid needing
this api addition?


On 2009/08/28 00:30:57, Erik Kay wrote:
> should this be LOG(INFO)?  I forget if that also prints to stdout by default. 
> If not, you could do both.

http://codereview.chromium.org/174633/diff/1/9
File chrome/test/data/extensions/api_test/tabs/test.js (right):

http://codereview.chromium.org/174633/diff/1/9#newcode17
Line 17: // event name, and do nothing if they don't find one.
see event_bindings.js. removeListener() is implemented

http://codereview.chromium.org/174633/diff/1/9#newcode22
Line 22: function validateTab(tab) {
this validation already happens automatically by the json schema for callbacks
and events. no need to duplicate this testing. the assertNoLastError seems to
have already via testFunction(). consider removing validateTab()

http://codereview.chromium.org/174633/diff/1/9#newcode46
Line 46: function openNewTab() {
add TODOs for needed cases: 1) two windows open. create tab in "other" window.
2) don't pass windowId. confirm created in "this" window. 3) pass index. confirm
placed at correct index position. 4) pass selected. confirm is selected.

http://codereview.chromium.org/174633/diff/1/9#newcode58
Line 58: // Setup a new window for later tests.
add TODOs for needed cases: 1) window sizing/positioning. passed url (relative &
absolute)

http://codereview.chromium.org/174633/diff/1/9#newcode75
Line 75: chrome.tabs.getAllInWindow(firstWindowId, testFunction(function(tabs) {
TODOs: null windowId for getAllInWindow should default to "current" window.

[asargent_no_longer_on_chrome, 2009-08-28 22:56:57]

http://codereview.chromium.org/174633/diff/1/3
File chrome/browser/extensions/extension_test_api.cc (right):

http://codereview.chromium.org/174633/diff/1/3#newcode35
Line 35: printf("%s\n", message.c_str());
On 2009/08/28 21:16:34, rafaelw wrote:
> It seems like console.log already prints to LOG(INFO). If I run matt's
messaging
> browser tests inside VS, I see all the console.log output. Is there a flag you
> can pass during runtime that would output LOG(INFO) to stdout and avoid
needing
> this api addition?
> 
> 
> On 2009/08/28 00:30:57, Erik Kay wrote:
> > should this be LOG(INFO)?  I forget if that also prints to stdout by
default. 
> > If not, you could do both.
> 
> 

Ok, I changed this to both printf and LOG(INFO).

http://codereview.chromium.org/174633/diff/1/9
File chrome/test/data/extensions/api_test/tabs/test.js (right):

http://codereview.chromium.org/174633/diff/1/9#newcode17
Line 17: // event name, and do nothing if they don't find one.
On 2009/08/28 21:16:34, rafaelw wrote:
> see event_bindings.js. removeListener() is implemented

Good catch. I reworked things so that I now use removeListener.

http://codereview.chromium.org/174633/diff/1/9#newcode22
Line 22: function validateTab(tab) {
On 2009/08/28 21:16:34, rafaelw wrote:
> this validation already happens automatically by the json schema for callbacks
> and events. no need to duplicate this testing. the assertNoLastError seems to
> have already via testFunction(). consider removing validateTab()

Ok, I've removed this to simplify the tests.

http://codereview.chromium.org/174633/diff/1/9#newcode46
Line 46: function openNewTab() {
On 2009/08/28 21:16:34, rafaelw wrote:
> add TODOs for needed cases: 1) two windows open. create tab in "other" window.
> 2) don't pass windowId. confirm created in "this" window. 3) pass index.
confirm
> placed at correct index position. 4) pass selected. confirm is selected.

Added TODO

http://codereview.chromium.org/174633/diff/1/9#newcode56
Line 56: 
On 2009/08/28 00:30:57, Erik Kay wrote:
> remove extra newline

Done.

http://codereview.chromium.org/174633/diff/1/9#newcode58
Line 58: // Setup a new window for later tests.
On 2009/08/28 21:16:34, rafaelw wrote:
> add TODOs for needed cases: 1) window sizing/positioning. passed url (relative
&
> absolute)

Added TODO

http://codereview.chromium.org/174633/diff/1/9#newcode67
Line 67: function moreSetup() {
On 2009/08/28 00:30:57, Erik Kay wrote:
> this seems weird to me.  why not just put these inside of the create callback
of
> the previous setup function?

Done.

http://codereview.chromium.org/174633/diff/1/9#newcode75
Line 75: chrome.tabs.getAllInWindow(firstWindowId, testFunction(function(tabs) {
On 2009/08/28 21:16:34, rafaelw wrote:
> TODOs: null windowId for getAllInWindow should default to "current" window.

Added TODO

http://codereview.chromium.org/174633/diff/1/9#newcode81
Line 81: // The most recent tab should be selected
On 2009/08/28 00:30:57, Erik Kay wrote:
> newline above comment

Done.

http://codereview.chromium.org/174633/diff/1/9#newcode104
Line 104: chrome.tabs.update(testTabId, {"selected":true, "url": "chrome://c"},
On 2009/08/28 00:30:57, Erik Kay wrote:
> why not just move this inline into the next test?

Done.

http://codereview.chromium.org/174633/diff/1/9#newcode126
Line 126: chrome.windows.remove(firstWindowId, function() { assertNoLastError();
});
On 2009/08/28 00:30:57, Erik Kay wrote:
> just use assertNoLastError by itself like you do later

Done.

http://codereview.chromium.org/174633/diff/1/9#newcode127
Line 127: chrome.windows.remove(secondWindowId, testFunction());
On 2009/08/28 00:30:57, Erik Kay wrote:
> the fact that these two are written differently makes me think that
> testFunction() isn't necessarily the best model.

Yeah, testFunction sort of breaks down for these more complicated scenarios
where you want to make sure that multiple callbacks fired successfully. For now
I've changed the second one to just use assertNoLastError/succeed. Perhaps I can
take a second pass at some point and try and come up with some better
infrastructure to keep the actual tests simple.

http://codereview.chromium.org/174633/diff/1/9#newcode222
Line 222: // We don't use assertEq since these strings are huge and we don't
On 2009/08/28 00:30:57, Erik Kay wrote:
> newline above comment

Done.

http://codereview.chromium.org/174633/diff/1/9#newcode260
Line 260: assertNoLastError());
On 2009/08/28 00:30:57, Erik Kay wrote:
> no () on assertNoLastError

Good catch. Done.

http://codereview.chromium.org/174633/diff/1/9#newcode266
Line 266: assertEq(moveTabIds['a'], tabid);
On 2009/08/28 00:30:57, Erik Kay wrote:
> don't all of these need to call succeed() in order to do the next test?

I was wrapping the listener functions with testFunction(), but I've changed that
in favor of being more explicit by calling succeed() directly.

[Erik does not do reviews, 2009-08-28 23:05:56]

http://codereview.chromium.org/174633/diff/2002/1007
File chrome/chrome.gyp (right):

http://codereview.chromium.org/174633/diff/2002/1007#newcode58
Line 58: 'browser/extensions/extension_tabs_apitest.cc',
should this be moved out of win_specific? (if so, also add it to exclude_on_mac)

http://codereview.chromium.org/174633/diff/2002/1010
File chrome/test/data/extensions/api_test/tabs/test.js (right):

http://codereview.chromium.org/174633/diff/2002/1010#newcode210
Line 210: // equal to either the first or second window.
Add a comment explaining why this is non-deterministic (only in some cases).

http://codereview.chromium.org/174633/diff/2002/1010#newcode211
Line 211: chrome.tabs.captureVisibleTab(null, function(url) {
shouldn't this just use testFunction()?

http://codereview.chromium.org/174633/diff/2002/1010#newcode228
Line 228: "selected": true}, function(tab) {
why not testFunction()?

[rafaelw, 2009-08-28 23:38:06]

lgtm.

consider altering testFunction() to take a bool param that says whether it'll
succeed() on exit. so you can always use it.

http://codereview.chromium.org/174633/diff/2002/1007
File chrome/chrome.gyp (right):

http://codereview.chromium.org/174633/diff/2002/1007#newcode58
Line 58: 'browser/extensions/extension_tabs_apitest.cc',
On 2009/08/28 23:05:57, Erik Kay wrote:
> should this be moved out of win_specific? (if so, also add it to
exclude_on_mac)

yes. try putting this in both browser_test_sources and
browser_test_exclude_on_mac. that'll enable it on linux.

[asargent_no_longer_on_chrome, 2009-08-30 23:03:46]

I switched the signature of "testFunction(func)" to
"testCallback(succeedWhenDone, func)", and I think this makes the code a little
clearer. Let me know what you guys think. 

Also, note the comment I made on Linux.

http://codereview.chromium.org/174633/diff/2002/1007
File chrome/chrome.gyp (right):

http://codereview.chromium.org/174633/diff/2002/1007#newcode58
Line 58: 'browser/extensions/extension_tabs_apitest.cc',
On 2009/08/28 23:38:06, rafaelw wrote:
> On 2009/08/28 23:05:57, Erik Kay wrote:
> > should this be moved out of win_specific? (if so, also add it to
> exclude_on_mac)
> 
> yes. try putting this in both browser_test_sources and
> browser_test_exclude_on_mac. that'll enable it on linux.
> 
> 

It turns out there are some weird issues with linux and the tabs API that I
haven't been able to track down yet, so I'm going to leave it windows-only for
now. (I'll create bugs for these, but there are at least 2 linux blockers so
far. The first is that sometimes if you call chrome.tabs.create and don't
specify an url, it goes to "http://www.google.com" instead of "chrome://newtab".
Second, the detectLanguage function seems to never call its callback.)

http://codereview.chromium.org/174633/diff/2002/1010
File chrome/test/data/extensions/api_test/tabs/test.js (right):

http://codereview.chromium.org/174633/diff/2002/1010#newcode210
Line 210: // equal to either the first or second window.
On 2009/08/28 23:05:57, Erik Kay wrote:
> Add a comment explaining why this is non-deterministic (only in some cases).

Done.

http://codereview.chromium.org/174633/diff/2002/1010#newcode211
Line 211: chrome.tabs.captureVisibleTab(null, function(url) {
On 2009/08/28 23:05:57, Erik Kay wrote:
> shouldn't this just use testFunction()?

Done.

http://codereview.chromium.org/174633/diff/2002/1010#newcode228
Line 228: "selected": true}, function(tab) {
On 2009/08/28 23:05:57, Erik Kay wrote:
> why not testFunction()?

Done.

[Erik does not do reviews, 2009-08-31 17:20:48]

LGTM