Update ExtensionApiTest.Tabs and re-enable

Update ExtensionApiTest.Tabs and re-enable

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

[asargent_no_longer_on_chrome, 2009-09-11 17:54:29]

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

http://codereview.chromium.org/199091/diff/1/5#newcode101
Line 101: 
nit: consider removing blank line

http://codereview.chromium.org/199091/diff/1/5#newcode276
Line 276: this.doneListening();
I think "this.doneListening()" feels a little magical here, requiring
understanding of the fact that we use a custom 'this' object when calling apply
on the function arg to chrome.test.listenForever. It might be a little more
clear to instead have a chrome.test.doneListening function instead.

I'm not totally opposed to the way you've done it, but maybe get a second
opinion from aa or erikkay?

[Erik does not do reviews, 2009-09-11 18:12:52]

http://codereview.chromium.org/199091/diff/1/4
File chrome/renderer/resources/extension_apitest.js (right):

http://codereview.chromium.org/199091/diff/1/4#newcode179
Line 179: that.doneListening = function() {
can't you just do func.doneListening = function() {...?
That way doneListening will be injected into the scope of the function directly.

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

http://codereview.chromium.org/199091/diff/1/5#newcode194
Line 194: chrome.tabs.move(moveTabIds['d'], {"windowId": moveWindow2, "index":
2},
80 cols

http://codereview.chromium.org/199091/diff/1/5#newcode276
Line 276: this.doneListening();
On 2009/09/11 17:54:29, Antony Sargent wrote:
> I think "this.doneListening()" feels a little magical here, requiring
> understanding of the fact that we use a custom 'this' object when calling
apply
> on the function arg to chrome.test.listenForever. It might be a little more
> clear to instead have a chrome.test.doneListening function instead.
> 
> I'm not totally opposed to the way you've done it, but maybe get a second
> opinion from aa or erikkay?

You can't use chrome.test.doneListening because you need the closure to
unregister the listener.  I added a comment to doneListening on another way to
do this.

http://codereview.chromium.org/199091/diff/1/5#newcode293
Line 293: chrome.test.listenOnce(chrome.tabs.onSelectionChanged, function(tabid,
info) {
80 cols

[rafaelw, 2009-09-11 19:05:05]

I tried both variants that Erik suggested and neither works. I don't believe
there's anyway to inject variables into a called functions scope. The callstack
and scope chain are independant.

func.doneListening() would have required the callback calling

arguments.callee.doneListening().

simply defining a closer in the stack above the safeFunctionApply doesn't leave
the closer in the called functions scope, because it's scope was bound earlier,
at the time the function was created (in test.js). 

I talked with Aaron about it and we both felt that this.doneListening() and
arguments.callee.doneListening() were both too sketchy, so we settled on what's
included.

Take a look.

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

http://codereview.chromium.org/199091/diff/1/5#newcode101
Line 101: 
On 2009/09/11 17:54:29, Antony Sargent wrote:
> nit: consider removing blank line

Done.

http://codereview.chromium.org/199091/diff/1/5#newcode194
Line 194: chrome.tabs.move(moveTabIds['d'], {"windowId": moveWindow2, "index":
2},
On 2009/09/11 18:12:52, Erik Kay wrote:
> 80 cols

Done.

http://codereview.chromium.org/199091/diff/1/5#newcode293
Line 293: chrome.test.listenOnce(chrome.tabs.onSelectionChanged, function(tabid,
info) {
On 2009/09/11 18:12:52, Erik Kay wrote:
> 80 cols

Done.

[asargent_no_longer_on_chrome, 2009-09-11 19:28:57]

this seems ok to me

[Erik does not do reviews, 2009-09-11 19:46:06]

LGTM

[Paweł Hajdan Jr., 2009-09-11 20:09:06]

Enforcing order of events seems Really Good, always. Good luck!

http://codereview.chromium.org/199091/diff/4001/4002
File chrome/browser/extensions/extension_tabs_apitest.cc (right):

http://codereview.chromium.org/199091/diff/4001/4002#newcode7
Line 7: // Flaky, http://crbug.com/20828. Please consult phajdan.jr before
re-enabling.
Please remove this comment. If all goes well, it won't be needed.

[rafaelw, 2009-09-11 20:12:09]

http://codereview.chromium.org/199091/diff/4001/4002
File chrome/browser/extensions/extension_tabs_apitest.cc (right):

http://codereview.chromium.org/199091/diff/4001/4002#newcode7
Line 7: // Flaky, http://crbug.com/20828. Please consult phajdan.jr before
re-enabling.
On 2009/09/11 20:09:06, Paweł Hajdan Jr. wrote:
> Please remove this comment. If all goes well, it won't be needed.

Done.