All browser types should should open CURRENT_TAB in the current tab

For popup/app windows, even dispositions of CURRENT_TAB are routed to new browser windows, which is overreaching.  Doesn't affect normal user actions since these windows have no address bar and link clicks don't go through this code, but does affect programmatic URL changes.

BUG=30529
TEST=None

[dhg, 2009-12-16 00:46:55]

Hey, so i figured this was the easiest way to start the discussion about this
part of the code. I bounced it off a couple people, and I think this is the
right thing to do...

[Peter Kasting, 2009-12-16 00:58:04]

I have no idea what this is doing, since your description is vague and
non-grammatical, and you don't cite a bug number for context.

Please update with appropriate BUG= and TEST= lines and a more fleshed-out
description, and I'll be happy to take another look.

[sky, 2009-12-16 01:04:29]

I would also make the type_ conditional first as that makes it more
clear this is only for non-tabbed browsers.

  -Scott

On Tue, Dec 15, 2009 at 4:46 PM,  <dhg@chromium.org> wrote:
> Reviewers: pkasting,
>
> Message:
> Hey, so i figured this was the easiest way to start the discussion about
> this
> part of the code. I bounced it off a couple people, and I think this is the
> right thing to do...
>
> Description:
> Changing browser so it opens up in the current tab when its a popup
>
> Please review this at http://codereview.chromium.org/505013
>
> SVN Base: http://src.chromium.org/svn/trunk/src/
>
> Affected files:
>  M     chrome/browser/browser.cc
>
>
> Index: chrome/browser/browser.cc
> ===================================================================
> --- chrome/browser/browser.cc   (revision 34606)
> +++ chrome/browser/browser.cc   (working copy)
> @@ -2973,7 +2973,9 @@
>
>   // If this is not a normal window (such as a popup or an application), we
> can
>   // only have one tab so a new tab always goes into a tabbed browser
> window.
> -  if (disposition != NEW_WINDOW && type_ != TYPE_NORMAL) {
> +  if (disposition != NEW_WINDOW &&
> +      type_ != TYPE_NORMAL &&
> +      disposition != CURRENT_TAB) {
>     // If the disposition is OFF_THE_RECORD we don't want to create a new
>     // browser that will itself create another OTR browser. This will result
> in
>     // a browser leak (and crash below because no tab is created or
> selected).
>
>
>

[dhg, 2009-12-16 01:06:23]

It has no bug.    I have attempted to make the description more descriptive.

On Tue, Dec 15, 2009 at 4:58 PM,  <pkasting@chromium.org> wrote:
> I have no idea what this is doing, since your description is vague and
> non-grammatical, and you don't cite a bug number for context.
>
> Please update with appropriate BUG= and TEST= lines and a more fleshed-out
> description, and I'll be happy to take another look.
>
> http://codereview.chromium.org/505013
>

[dhg, 2009-12-16 01:08:55]

great idea scott. changed.

On Tue, Dec 15, 2009 at 5:04 PM, Scott Violet <sky@chromium.org> wrote:
> I would also make the type_ conditional first as that makes it more
> clear this is only for non-tabbed browsers.
>
>  -Scott
>
> On Tue, Dec 15, 2009 at 4:46 PM,  <dhg@chromium.org> wrote:
>> Reviewers: pkasting,
>>
>> Message:
>> Hey, so i figured this was the easiest way to start the discussion about
>> this
>> part of the code. I bounced it off a couple people, and I think this is the
>> right thing to do...
>>
>> Description:
>> Changing browser so it opens up in the current tab when its a popup
>>
>> Please review this at http://codereview.chromium.org/505013
>>
>> SVN Base: http://src.chromium.org/svn/trunk/src/
>>
>> Affected files:
>>  M     chrome/browser/browser.cc
>>
>>
>> Index: chrome/browser/browser.cc
>> ===================================================================
>> --- chrome/browser/browser.cc   (revision 34606)
>> +++ chrome/browser/browser.cc   (working copy)
>> @@ -2973,7 +2973,9 @@
>>
>>   // If this is not a normal window (such as a popup or an application), we
>> can
>>   // only have one tab so a new tab always goes into a tabbed browser
>> window.
>> -  if (disposition != NEW_WINDOW && type_ != TYPE_NORMAL) {
>> +  if (disposition != NEW_WINDOW &&
>> +      type_ != TYPE_NORMAL &&
>> +      disposition != CURRENT_TAB) {
>>     // If the disposition is OFF_THE_RECORD we don't want to create a new
>>     // browser that will itself create another OTR browser. This will result
>> in
>>     // a browser leak (and crash below because no tab is created or
>> selected).
>>
>>
>>
>

[Peter Kasting, 2009-12-16 01:14:26]

On 2009/12/16 01:06:23, dhg wrote:
> It has no bug.    I have attempted to make the description more descriptive.

The description is now longer, but it just describes the mechanics of the change
itself (i.e. it conveys the exact same information that reading the code does). 
Please describe what the problem and solution are in terms of user-visible
effect, like you would when filing a bug report.  A sample website or use case
that is broken by the current behavior would be ideal.

Also, seems like there should be some kind of non-empty TEST=.

[dhg, 2009-12-16 01:22:42]

How about I put it here.

The problem is, I am working on the usb filebrowser for chromeos.
When the user inserts a usb stick, i pop up a new browser that is a
Popup.  When we finally mount the USB stick, I want to change the URL
of the browser to a filebrowser which points to the directory that the
file was inserted to:

   std::string url = "chrome://filebrowse#";
              url += disks[i].mount_path;
              TabContents* tab = iter->browser->GetSelectedTabContents();
              tab->OpenURL(GURL(url), GURL(), CURRENT_TAB,
                  PageTransition::GENERATED);

As written right now, it causes the url to go to a different browser,
rather than the current one that I am calling into.

I understand that this was out of the blue for you, so let me know if
you need more context.

-Dave
On Tue, Dec 15, 2009 at 5:14 PM,  <pkasting@chromium.org> wrote:
> On 2009/12/16 01:06:23, dhg wrote:
>>
>> It has no bug.    I have attempted to make the description more
>> descriptive.
>
> The description is now longer, but it just describes the mechanics of the
> change
> itself (i.e. it conveys the exact same information that reading the code
> does).
> Please describe what the problem and solution are in terms of user-visible
> effect, like you would when filing a bug report.  A sample website or use
> case
> that is broken by the current behavior would be ideal.
>
> Also, seems like there should be some kind of non-empty TEST=.
>
> http://codereview.chromium.org/505013
>

[Peter Kasting, 2009-12-16 01:28:57]

On 2009/12/16 01:22:42, dhg wrote:
> How about I put it here.

That helps me, but not so much future code spelunkers.  This is why we recommend
that every change be associated with a bug :)

> When the user inserts a usb stick, i pop up a new browser that is a
> Popup.

Is there a particular reason you used a popup?  Why not just open this as a new
tab or window?

>               tab->OpenURL(GURL(url), GURL(), CURRENT_TAB,
>                   PageTransition::GENERATED);

I doubt you want GENERATED here.  Scott, what do you think?

Why not wait until this point to open the window, so you can open it directly on
the correct URL?

> As written right now, it causes the url to go to a different browser,
> rather than the current one that I am calling into.

What is the current behavior and effect of your patch for "normal" popup usage,
e.g. clicking links in popup windows and highlighting text + right
click->"search for"?

[dhg, 2009-12-16 01:32:36]

We use a popup so that it gets created as a mole. (moles are defined
as popups under a certain size)

And I'll run some tests, I was wondering what areas I should try out.
I'll ping this thread when done. (i'll also create a bug on this while
i'm at it, you make a good point)

-Dave

On Tue, Dec 15, 2009 at 5:28 PM,  <pkasting@chromium.org> wrote:
> On 2009/12/16 01:22:42, dhg wrote:
>>
>> How about I put it here.
>
> That helps me, but not so much future code spelunkers.  This is why we
> recommend
> that every change be associated with a bug :)
>
>> When the user inserts a usb stick, i pop up a new browser that is a
>> Popup.
>
> Is there a particular reason you used a popup?  Why not just open this as a
> new
> tab or window?
>
>>               tab->OpenURL(GURL(url), GURL(), CURRENT_TAB,
>>                   PageTransition::GENERATED);
>
> I doubt you want GENERATED here.  Scott, what do you think?
>
> Why not wait until this point to open the window, so you can open it
> directly on
> the correct URL?
>
>> As written right now, it causes the url to go to a different browser,
>> rather than the current one that I am calling into.
>
> What is the current behavior and effect of your patch for "normal" popup
> usage,
> e.g. clicking links in popup windows and highlighting text + right
> click->"search for"?
>
> http://codereview.chromium.org/505013
>

[dhg, 2009-12-16 01:53:14]

Just tried out cnn, and a couple other sites.   clicking on a link on
the popup opened in the correct location (meaning not in the popup).
as did right clicking on an image and saying "open this image in a new
tab". Also highlighting text and saying "search for blah" worked as
well.  I also added a new bug (if we want to move the conversation
there)

http://code.google.com/p/chromium/issues/detail?id=30529



On Tue, Dec 15, 2009 at 5:32 PM, David Garcia <dhg@chromium.org> wrote:
> We use a popup so that it gets created as a mole. (moles are defined
> as popups under a certain size)
>
> And I'll run some tests, I was wondering what areas I should try out.
> I'll ping this thread when done. (i'll also create a bug on this while
> i'm at it, you make a good point)
>
> -Dave
>
> On Tue, Dec 15, 2009 at 5:28 PM,  <pkasting@chromium.org> wrote:
>> On 2009/12/16 01:22:42, dhg wrote:
>>>
>>> How about I put it here.
>>
>> That helps me, but not so much future code spelunkers.  This is why we
>> recommend
>> that every change be associated with a bug :)
>>
>>> When the user inserts a usb stick, i pop up a new browser that is a
>>> Popup.
>>
>> Is there a particular reason you used a popup?  Why not just open this as a
>> new
>> tab or window?
>>
>>>               tab->OpenURL(GURL(url), GURL(), CURRENT_TAB,
>>>                   PageTransition::GENERATED);
>>
>> I doubt you want GENERATED here.  Scott, what do you think?
>>
>> Why not wait until this point to open the window, so you can open it
>> directly on
>> the correct URL?
>>
>>> As written right now, it causes the url to go to a different browser,
>>> rather than the current one that I am calling into.
>>
>> What is the current behavior and effect of your patch for "normal" popup
>> usage,
>> e.g. clicking links in popup windows and highlighting text + right
>> click->"search for"?
>>
>> http://codereview.chromium.org/505013
>>
>

[sky, 2009-12-16 17:04:57]

>>               tab->OpenURL(GURL(url), GURL(), CURRENT_TAB,
>>                   PageTransition::GENERATED);
>
> I doubt you want GENERATED here.  Scott, what do you think?

Oh what a mess. If we're keeping GENERATED to mean just from the
Omnibox, then this should probably be LINK as nothing else is good.

   -Scott

[dhg, 2009-12-16 17:12:03]

k changed it in my other code, not a big deal.

On Wed, Dec 16, 2009 at 9:04 AM, Scott Violet <sky@chromium.org> wrote:
>>>               tab->OpenURL(GURL(url), GURL(), CURRENT_TAB,
>>>                   PageTransition::GENERATED);
>>
>> I doubt you want GENERATED here.  Scott, what do you think?
>
> Oh what a mess. If we're keeping GENERATED to mean just from the
> Omnibox, then this should probably be LINK as nothing else is good.
>
>   -Scott
>

[Peter Kasting, 2009-12-16 18:50:03]

I updated the description to try and be clearer.

LGTM

http://codereview.chromium.org/505013/diff/7001/7002
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/505013/diff/7001/7002#newcode2978
chrome/browser/browser.cc:2978: disposition != CURRENT_TAB) {
Nit: I'd put this on the previous line

[dhg, 2009-12-16 20:25:21]

done and uploaded. (i'm not a comitter)

On Wed, Dec 16, 2009 at 10:50 AM,  <pkasting@chromium.org> wrote:
> I updated the description to try and be clearer.
>
> LGTM
>
>
> http://codereview.chromium.org/505013/diff/7001/7002
> File chrome/browser/browser.cc (right):
>
> http://codereview.chromium.org/505013/diff/7001/7002#newcode2978
> chrome/browser/browser.cc:2978: disposition != CURRENT_TAB) {
> Nit: I'd put this on the previous line
>
> http://codereview.chromium.org/505013
>

[Peter Kasting, 2009-12-19 01:12:51]

Landed in r35017.