When creating a tab contents for a browser in the RVHDelegateHelper, create a wrapper immediately.

When creating a tab contents for a browser in the RVHDelegateHelper, create a wrapper immediately.

BUG=100456
TEST=ExtensionApiTest.WebRequestNewTab

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

[jochen (gone - plz use gerrit), 2011-10-27 15:44:06]

Scott, before I continue to add a test to this, could you please have look
whether the general approach is ok?

[sky, 2011-10-27 16:41:42]

In theory this seems fine. But presumably we have some existing place that does
this too. Does it need to change as well?

  -Scott

[jochen (gone - plz use gerrit), 2011-10-27 19:11:27]

On 2011/10/27 16:41:42, sky wrote:
> In theory this seems fine. But presumably we have some existing place that
does
> this too. Does it need to change as well?
> 
>   -Scott

My understanding is that the tab will be added in Browser::AddNewContents which
first checks whether the tab already has a wrapper, and only if no wrapper
exists, creates a new one.

[Avi (use Gerrit), 2011-10-27 19:32:58]

If you're always adding a wrapper, is it redundant?

BTW, don't forget that TCWs change the ownership story. If you make a TCW, it
takes ownership of the TC, and whoever owns the TC is supposed to own the TCW
instead. If the TC gets destroyed rather than the TCW, the TCW attached will
DCHECK pretty hard.

[jochen (gone - plz use gerrit), 2011-10-27 19:42:12]

On 2011/10/27 19:32:58, Avi wrote:
> If you're always adding a wrapper, is it redundant?

The TCW is added to late, i.e. after the corresponding network requests started.

> 
> BTW, don't forget that TCWs change the ownership story. If you make a TCW, it
> takes ownership of the TC, and whoever owns the TC is supposed to own the TCW
> instead. If the TC gets destroyed rather than the TCW, the TCW attached will
> DCHECK pretty hard.

I'll fire off tryjobs, but my take on this is that at this point the TC will
always end up being added to the browser.

[sky, 2011-10-27 19:49:53]

On 2011/10/27 19:42:12, jochen wrote:
> On 2011/10/27 19:32:58, Avi wrote:
> > If you're always adding a wrapper, is it redundant?
> 
> The TCW is added to late, i.e. after the corresponding network requests
started.
> 
> > 
> > BTW, don't forget that TCWs change the ownership story. If you make a TCW,
it
> > takes ownership of the TC, and whoever owns the TC is supposed to own the
TCW
> > instead. If the TC gets destroyed rather than the TCW, the TCW attached will
> > DCHECK pretty hard.
> 
> I'll fire off tryjobs, but my take on this is that at this point the TC will
> always end up being added to the browser.

Make sure that's true for prerendering, blocked popups and instant too.

  -Scott

[jochen (gone - plz use gerrit), 2011-10-28 13:03:53]

ok, now with tests

Scott, plz review chrome/browser/ui bits
Dominic, rest

[battre, 2011-10-28 13:27:50]

Awesome. Thanks a lot!

LGTM

http://codereview.chromium.org/8404046/diff/3003/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/webrequest/framework.js (right):

http://codereview.chromium.org/8404046/diff/3003/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/webrequest/framework.js:167: if
(!(details.tabId in tabIdMap)) {
Nit/optional: Add comment to not open two new tabs in short succession to
prevent race conditions?

[battre, 2011-10-28 13:28:21]

nit: TEST=tdb in issue description

[sky, 2011-10-28 17:40:59]

LGTM

[commit-bot: I haz the power, 2011-10-28 18:04:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/8404046/9002

[commit-bot: I haz the power, 2011-10-28 18:04:30]

Can't apply patch for file chrome/browser/extensions/extension_tab_id_map.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/extension_tab_id_map.cc
Hunk #1 FAILED at 12.
1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/extensions/extension_tab_id_map.cc.rej

[commit-bot: I haz the power, 2011-10-28 18:51:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/8404046/3014

[commit-bot: I haz the power, 2011-10-28 19:59:11]

Change committed as 107777