Pass result of blockage across content API when new tab blocked.

Pass result of blockage across content API when new tab blocked. This change allows the proper blockage of foreground tabs created without user-gestures for rel=noreferrer links.

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

[Tom Sepez, 2012-08-27 22:00:10]

Hi John,

This patch isn't quite ready, but I wanted to pull you in early because of the
need to change the API in content/public.  

See the diffs in:

content/public/browser/web_contents.h
content/public/browser/web_contents_delegate.h

The associated bug lays out the context, but without returning this info back to
content, WebContentsImpl::CreateNewWindow() isn't aware that no new tab was
created, and mistakenly navigates the (wrong) current tab.

[jam, 2012-08-29 00:07:23]

content/public is reasonable, just a comment:

https://codereview.chromium.org/10868116/diff/1/content/public/browser/web_co...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/10868116/diff/1/content/public/browser/web_co...
content/public/browser/web_contents.h:230: virtual bool
AddNewContents(WebContents* new_contents,
can you get rid of this function altogether? most places that call it go
directly through the delegate. so we can convert the remaining callsites and get
rid of this

https://codereview.chromium.org/10868116/diff/1/content/public/browser/web_co...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/10868116/diff/1/content/public/browser/web_co...
content/public/browser/web_contents_delegate.h:305: WebContents* new_contents) {
return true; }
nit: you will get clang errors for this, you need to put this in the cc file

[Tom Sepez, 2012-09-04 19:03:56]

Hi John, please re-review. Thanks.

[jam, 2012-09-04 19:50:46]

https://codereview.chromium.org/10868116/diff/28016/chrome/browser/ui/views/e...
File chrome/browser/ui/views/external_tab_container_win.cc (right):

https://codereview.chromium.org/10868116/diff/28016/chrome/browser/ui/views/e...
chrome/browser/ui/views/external_tab_container_win.cc:540: return false;
can you explain this? I'm not sure I understand why it's this way?

https://codereview.chromium.org/10868116/diff/28016/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/10868116/diff/28016/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1279: not_blocked =
delegate_->AddNewContents(
it's confusing to read this block of code, since the delegate is asked twice
whether they want to block this or not? why isn't the first call sufficient?

[Tom Sepez, 2012-09-04 20:46:16]

John, Reverting CreateWebContents to type void removes the code on which you
previously commented.  Thanks,

http://codereview.chromium.org/10868116/diff/28016/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/external_tab_container_win.cc (right):

http://codereview.chromium.org/10868116/diff/28016/chrome/browser/ui/views/ex...
chrome/browser/ui/views/external_tab_container_win.cc:540: return false;
On 2012/09/04 19:50:46, John Abd-El-Malek wrote:
> can you explain this? I'm not sure I understand why it's this way?
Not sure there's any upside to returning false here. I'll change it to be true.

http://codereview.chromium.org/10868116/diff/28016/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.cc (right):

http://codereview.chromium.org/10868116/diff/28016/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.cc:1279: not_blocked =
delegate_->AddNewContents(
You're right.  I think the first call need not return bool and that this is the
only place that checks for blocking. I'll make the changes

[Tom Sepez, 2012-09-05 00:37:30]

John, please re-re-review.  Thanks.

[jam, 2012-09-05 06:12:14]

lgtm with nit

After looking at this change a few times, it seems that there's a lot of code
that has to return a boolean, and it's not very clear what that means. What do
you think of keeping the return parameter to be a void, and to have a |blocked|
out parameter? That way only the places that need to block the tab bother with
that parameter.

https://codereview.chromium.org/10868116/diff/28031/ui/web_dialogs/web_dialog...
File ui/web_dialogs/web_dialog_web_contents_delegate.h (right):

https://codereview.chromium.org/10868116/diff/28031/ui/web_dialogs/web_dialog...
ui/web_dialogs/web_dialog_web_contents_delegate.h:31: virtual bool
AddNewContents(content::BrowserContext* context,
the only implementation of this is in chrome_web_contents_handler.cc, which
doesn't need this blocking mechanism. seems unnecessary to add it here?

[Tom Sepez, 2012-09-05 16:58:41]

On 2012/09/05 06:12:14, John Abd-El-Malek wrote:
> lgtm with nit
> 
> After looking at this change a few times, it seems that there's a lot of code
> that has to return a boolean, and it's not very clear what that means. What do
> you think of keeping the return parameter to be a void, and to have a
|blocked|
> out parameter? That way only the places that need to block the tab bother with
> that parameter.

My personal preference is to avoid out parameters when possible.
> 
>
https://codereview.chromium.org/10868116/diff/28031/ui/web_dialogs/web_dialog...
> File ui/web_dialogs/web_dialog_web_contents_delegate.h (right):
> 
>
https://codereview.chromium.org/10868116/diff/28031/ui/web_dialogs/web_dialog...
> ui/web_dialogs/web_dialog_web_contents_delegate.h:31: virtual bool
> AddNewContents(content::BrowserContext* context,
> the only implementation of this is in chrome_web_contents_handler.cc, which
> doesn't need this blocking mechanism. seems unnecessary to add it here?

Yep.  Will revert to void.

[jam, 2012-09-05 16:59:56]

On Wed, Sep 5, 2012 at 9:58 AM, <tsepez@chromium.org> wrote:

> On 2012/09/05 06:12:14, John Abd-El-Malek wrote:
>
>> lgtm with nit
>>
>
>  After looking at this change a few times, it seems that there's a lot of
>> code
>> that has to return a boolean, and it's not very clear what that means.
>> What do
>> you think of keeping the return parameter to be a void, and to have a
>>
> |blocked|
>
>> out parameter? That way only the places that need to block the tab bother
>> with
>> that parameter.
>>
>
> My personal preference is to avoid out parameters when possible.


I'm curious, why?


>
>
> https://codereview.chromium.**org/10868116/diff/28031/ui/**
>
web_dialogs/web_dialog_web_**contents_delegate.h<https://codereview.chromium.org/10868116/diff/28031/ui/web_dialogs/web_dialog_web_contents_delegate.h>
>
>> File ui/web_dialogs/web_dialog_web_**contents_delegate.h (right):
>>
>
>
> https://codereview.chromium.**org/10868116/diff/28031/ui/**
>
web_dialogs/web_dialog_web_**contents_delegate.h#newcode31<https://codereview.chromium.org/10868116/diff/28031/ui/web_dialogs/web_dialog_web_contents_delegate.h#newcode31>
>
>> ui/web_dialogs/web_dialog_web_**contents_delegate.h:31: virtual bool
>> AddNewContents(content::**BrowserContext* context,
>> the only implementation of this is in chrome_web_contents_handler.**cc,
>> which
>> doesn't need this blocking mechanism. seems unnecessary to add it here?
>>
>
> Yep.  Will revert to void.
>
>
>
http://codereview.chromium.**org/10868116/<http://codereview.chromium.org/108...
>

[Tom Sepez, 2012-09-05 17:14:44]

It generally makes writing expressions containing the function easier,
particularly if you're fond of writing macros as one would have to do in the old
days before compilers got smart about inlining. For scalar types, there isn't a
performance hit like there would be for copying a class.

[jam, 2012-09-05 17:38:30]

I highly doubt that performance here will be an issue. I personally think
it would be more readable as an output parameter, but this is just personal
preference, so up to you since you wrote this patch.

On Wed, Sep 5, 2012 at 10:23 AM, Thomas Sepez <tsepez@google.com> wrote:

>
>
> I guess that there's also that aliasing conundrum, wherein if I do
>
> bool x = foo(); bar(); bam(x);
>
> vs.
>
> bool x; foo(&x); bar(); bam(x);
>
> the compiler can't optimize x into a register across the call to bar
> because foo might have stashed that address somewhere and bar might have
> modified it.
>
>
> ---------- Forwarded message ----------
> From: <tsepez@chromium.org>
> Date: Wed, Sep 5, 2012 at 10:14 AM
> Subject: Re: Pass result of blockage across content API when new tab
> blocked. (issue 10868116)
> To: jam@chromium.org
> Cc: chromium-reviews@chromium.org, avi@chromium.org,
> mihaip-chromium-reviews@chromium.org, creis+watch@chromium.org,
> ajwong+watch@chromium.org, tfarina@chromium.org,
> jeremya+watch@chromium.org, jam@chromium.org, dcheng@chromium.org,
> joi+watch-content@chromium.org, aa@chromium.org, groby+watch@chromium.org,
> darin-cc@chromium.org, brettw-cc@chromium.org, gbillock+watch@chromium.org,
> smckay+watch@chromium.org
>
>
> It generally makes writing expressions containing the function easier,
> particularly if you're fond of writing macros as one would have to do in
> the old
> days before compilers got smart about inlining. For scalar types, there
> isn't a
> performance hit like there would be for copying a class.
>
>
http://codereview.chromium.**org/10868116/<http://codereview.chromium.org/108...
>
>
>
> On Wed, Sep 5, 2012 at 10:14 AM, <tsepez@chromium.org> wrote:
>
>> It generally makes writing expressions containing the function easier,
>> particularly if you're fond of writing macros as one would have to do in
>> the old
>> days before compilers got smart about inlining. For scalar types, there
>> isn't a
>> performance hit like there would be for copying a class.
>>
>>
http://codereview.chromium.**org/10868116/<http://codereview.chromium.org/108...
>>
>
>

[tsepez (do not use), 2012-09-05 17:43:41]

Clearly there is no performance issue in this case, I'm just in the habit
of writing it that way.


On Wed, Sep 5, 2012 at 10:38 AM, John Abd-El-Malek <jam@chromium.org> wrote:

> I highly doubt that performance here will be an issue. I personally think
> it would be more readable as an output parameter, but this is just personal
> preference, so up to you since you wrote this patch.
>
>
> On Wed, Sep 5, 2012 at 10:23 AM, Thomas Sepez <tsepez@google.com> wrote:
>
>>
>>
>> I guess that there's also that aliasing conundrum, wherein if I do
>>
>> bool x = foo(); bar(); bam(x);
>>
>> vs.
>>
>> bool x; foo(&x); bar(); bam(x);
>>
>> the compiler can't optimize x into a register across the call to bar
>> because foo might have stashed that address somewhere and bar might have
>> modified it.
>>
>>
>> ---------- Forwarded message ----------
>> From: <tsepez@chromium.org>
>> Date: Wed, Sep 5, 2012 at 10:14 AM
>> Subject: Re: Pass result of blockage across content API when new tab
>> blocked. (issue 10868116)
>> To: jam@chromium.org
>> Cc: chromium-reviews@chromium.org, avi@chromium.org,
>> mihaip-chromium-reviews@chromium.org, creis+watch@chromium.org,
>> ajwong+watch@chromium.org, tfarina@chromium.org,
>> jeremya+watch@chromium.org, jam@chromium.org, dcheng@chromium.org,
>> joi+watch-content@chromium.org, aa@chromium.org, groby+watch@chromium.org,
>> darin-cc@chromium.org, brettw-cc@chromium.org,
>> gbillock+watch@chromium.org, smckay+watch@chromium.org
>>
>>
>> It generally makes writing expressions containing the function easier,
>> particularly if you're fond of writing macros as one would have to do in
>> the old
>> days before compilers got smart about inlining. For scalar types, there
>> isn't a
>> performance hit like there would be for copying a class.
>>
>>
http://codereview.chromium.**org/10868116/<http://codereview.chromium.org/108...
>>
>>
>>
>> On Wed, Sep 5, 2012 at 10:14 AM, <tsepez@chromium.org> wrote:
>>
>>> It generally makes writing expressions containing the function easier,
>>> particularly if you're fond of writing macros as one would have to do in
>>> the old
>>> days before compilers got smart about inlining. For scalar types, there
>>> isn't a
>>> performance hit like there would be for copying a class.
>>>
>>>
http://codereview.chromium.**org/10868116/<http://codereview.chromium.org/108...
>>>
>>
>>
>

[commit-bot: I haz the power, 2012-09-05 18:48:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tsepez@chromium.org/10868116/24023

[commit-bot: I haz the power, 2012-09-05 18:48:43]

Presubmit check for 10868116-24023 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ui/views
    ui

Presubmit checks took 3.7s to calculate.

[Tom Sepez, 2012-09-05 18:50:52]

Ben, please review as owner for ui, ui/views. Thanks.

[Ben Goodger (Google), 2012-09-06 21:19:24]

ui lgtm, but for the record, I am similarly confused by the bool return. seems
like a confusing api. we generally like to avoid bool for things like this, or
do as jam suggests. bool return values are best for things where the function
name is descriptive (e.g. bool IsMaximized()) or there is some convention that
return true/false mean something. that's not really the case here.

[commit-bot: I haz the power, 2012-09-06 21:35:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tsepez@chromium.org/10868116/24023

[jam, 2012-09-06 21:42:56]

On 2012/09/06 21:19:24, Ben Goodger (Google) wrote:
> ui lgtm, but for the record, I am similarly confused by the bool return. seems
> like a confusing api. we generally like to avoid bool for things like this, or
> do as jam suggests. bool return values are best for things where the function
> name is descriptive (e.g. bool IsMaximized()) or there is some convention that
> return true/false mean something. that's not really the case here.

given I'm not alone in finding this not readable, and Ben has written a more
explanatory version of why a bool return is confusing in this case, can we make
it an out parameter?

[Tom Sepez, 2012-09-06 22:05:24]

Will do.

[Tom Sepez, 2012-09-07 00:35:02]

John, please take a look at this CL which uses output parameter.

[jam, 2012-09-07 01:08:19]

lgtm, thanks

[commit-bot: I haz the power, 2012-09-07 19:19:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tsepez@chromium.org/10868116/26122

[commit-bot: I haz the power, 2012-09-07 23:44:03]

Change committed as 155535

[jochen (gone - plz use gerrit), 2012-11-20 10:50:44]

https://chromiumcodereview.appspot.com/10868116/diff/26122/chrome/browser/ext...
File chrome/browser/extensions/extension_host.cc (left):

https://chromiumcodereview.appspot.com/10868116/diff/26122/chrome/browser/ext...
chrome/browser/extensions/extension_host.cc:614: return;
why did you delete this?