Fix callback for chrome.tabs.update with javascript URLs.

Fix callback for chrome.tabs.update with javascript URLs.

UpdateTabFunction is asynchronous when updating a tab with a javascript URL. In this situation, the method was returning early before generating the callback result. This fixes the ExecuteScriptApiTest.NavigationRaceJavaScriptUrl test.

BUG=89731
TEST=ExecuteScriptApiTest.NavigationRaceJavaScriptUrl

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

[jstritar, 2012-01-27 15:38:43]

Actually, can you hold off on reviewing this? I'm going to add a test that
doesn't rely on a race condition (having trouble reproducing the original bug
now).

[jstritar, 2012-01-27 20:16:13]

Okay, you can take a look. I re-enabled a test that was disabled a while ago.
There are a bunch of chrome.tabs.update({url:...}) tests that have been
disabled... not sure how we'd know if they no longer crash.

[Mihai Parparita -not on Chrome, 2012-01-27 23:03:20]

LGTM

https://chromiumcodereview.appspot.com/9225010/diff/3003/chrome/browser/exten...
File chrome/browser/extensions/extension_tabs_module.cc (right):

https://chromiumcodereview.appspot.com/9225010/diff/3003/chrome/browser/exten...
chrome/browser/extensions/extension_tabs_module.cc:1072: web_contents_ =
contents->web_contents();
Is it safe to hold on to this pointer? I'm thinking of scenarios where the tab
gets closed by the time ExecutedCodeFinished returns. You may want to consider
overriding WebContentsDestroyed to null out web_contents_ and check for that in
PopulateResponse.

[jstritar, 2012-01-30 16:28:05]

http://codereview.chromium.org/9225010/diff/3003/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/9225010/diff/3003/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_module.cc:1072: web_contents_ =
contents->web_contents();
On 2012/01/27 23:03:20, Mihai Parparita wrote:
> Is it safe to hold on to this pointer? I'm thinking of scenarios where the tab
> gets closed by the time ExecutedCodeFinished returns. You may want to consider
> overriding WebContentsDestroyed to null out web_contents_ and check for that
in
> PopulateResponse.

Done. I also updated it so we don't need to hold on to the tab_strip_ pointer by
using the ExtensionTabUtil::CreateTabValue(contents) method.

[Mihai Parparita -not on Chrome, 2012-01-30 16:33:00]

Still LGTM

http://codereview.chromium.org/9225010/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tabs_module.h (right):

http://codereview.chromium.org/9225010/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_module.h:21: class TabStripModel;
Nit: This can be removed now.

http://codereview.chromium.org/9225010/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_module.h:123: content::WebContents*
web_contents_;
Nit: mind adding an empty line between the methods and members (to aid
readability)?

[jstritar, 2012-01-30 16:40:23]

http://codereview.chromium.org/9225010/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tabs_module.h (right):

http://codereview.chromium.org/9225010/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_module.h:21: class TabStripModel;
On 2012/01/30 16:33:00, Mihai Parparita wrote:
> Nit: This can be removed now.

Done.

http://codereview.chromium.org/9225010/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_module.h:123: content::WebContents*
web_contents_;
On 2012/01/30 16:33:00, Mihai Parparita wrote:
> Nit: mind adding an empty line between the methods and members (to aid
> readability)?

Done.

[commit-bot: I haz the power, 2012-01-30 16:40:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jstritar@chromium.org/9225010/13002

[commit-bot: I haz the power, 2012-01-30 17:59:18]

Change committed as 119690