Duplicate popup window should have same size and scroll ...

Duplicate popup window should have same size and scroll 
position as original popup.
BUG=5632
(http://crbug.com/5632)

Patch by Mohamed Mansour.

[Mohamed Mansour (USE mhm), 2008-12-17 22:41:59]

added Elliot for review since Ben is on vacation

[Peter Kasting, 2008-12-17 22:47:46]

http://codereview.chromium.org/14812/diff/1/2
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/14812/diff/1/2#newcode1332
Line 1332: // The new popup window should have the same size as the original
popup.
* Wouldn't we want this for app windows too?
* Doesn't this put the popup directly atop the other popup, obscuring it?  (Or
does it just make the size the same?)
* Do we really care very much about addressing this issue?  What use case does
it make better?

[Mohamed Mansour (USE mhm), 2008-12-17 22:57:55]

> * Wouldn't we want this for app windows too?
Normal windows duplicate it by adding a new tab, what do 
you mean App windows? Are App windows windows with no
tabs?


> * Doesn't this put the popup directly atop the other popup, 
> obscuring it?  (Or does it just make the size the same?)
Yes it does, should I apply a offset, like size of 
the frame?


> * Do we really care very much about addressing this issue?  
> What use case does it make better?
When you duplicate a popup, many websites designed the 
popup to be fixed size, so if the user wishes to duplicate,
we should make sure it is the same size.

So, I should update it for both TYPE_APP and TYPE_POPUP,
and apply an offset which is the frame height for top-left

[Peter Kasting, 2008-12-17 23:01:28]

On 2008/12/17 22:57:55, Mohamed Mansour wrote:
> > * Wouldn't we want this for app windows too?
> Normal windows duplicate it by adding a new tab, what do 
> you mean App windows? Are App windows windows with no
> tabs?

Yes (TYPE_APP).

> > * Doesn't this put the popup directly atop the other popup, 
> > obscuring it?  (Or does it just make the size the same?)
> Yes it does, should I apply a offset, like size of 
> the frame?

Normally the OS is supposed to handle this sort of thing.  It is probably risky
to do it ourselves since we could e.g. offset the window straight off the bottom
of the screen.

> > What use case does it make better?
> When you duplicate a popup, many websites designed the 
> popup to be fixed size, so if the user wishes to duplicate,
> we should make sure it is the same size.

But the issue is, why does the user want to duplicate it?  I dunno, this just
seems like a total non-issue to me.  Maybe somebody else can comment.

[Mohamed Mansour (USE mhm), 2008-12-17 23:34:39]

Okay, if you check it now, I received the bounds of the browser window, and just
set the width and height. Now it doesn't popup the window directly on top of the
other popup.

[Evan Stade, 2008-12-17 23:55:26]

http://codereview.chromium.org/14812/diff/6/205
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/14812/diff/6/205#newcode1336
Line 1336: browser_bounds.set_width(window()->GetNormalBounds().width());
um, what? delete lines 1336-1337

[Evan Stade, 2008-12-17 23:56:33]

sorry, ignore my comment. I am confused by this code though, can you add a
comment that clarifies what you are doing.

[Mohamed Mansour (USE mhm), 2008-12-18 00:12:33]

I added more comments.

[Peter Kasting, 2008-12-18 00:22:01]

http://codereview.chromium.org/14812/diff/206/207
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/14812/diff/206/207#newcode1334
Line 1334: // The new popup window should have the same size as the original
popup.
This code doesn't run for just popups, so let's not say "popup".

I think the confusion is more due to the names in the code than the lack of
comments.  How about:

// Preserve the size of the original window.  The new window has already been
given an offset by the OS, so we shouldn't copy the old bounds wholesale.
const Window* const new_window = browser->window();
new_window->SetBounds(gfx::Rect(new_window->GetNormalBounds().origin(),
window()->GetNormalBounds().size());

[Elliot Glaysher, 2008-12-18 00:28:46]

On 2008/12/18 00:12:33, Mohamed Mansour wrote:
> I added more comments.

I agree with Peter that this is a total non-issue simply because of how unused
duplicating a window w/o a tabstrip is, but this behaviour is better then how
things were previously.

Fix Peter's naming issue and I'll go ahead and commit it.

[Mohamed Mansour (USE mhm), 2008-12-18 00:41:09]

> Fix Peter's naming issue and I'll go ahead and commit it.
done.