Issue 151135: Pop-up chrome....

Issue 151135: Pop-up chrome.... (Closed)

Created:
11 years, 5 months ago by John Grabowski

Modified:
9 years, 5 months ago

Reviewers:
pink (ping after 24hrs)

CC:
chromium-reviews_googlegroups.com, John Grabowski, Ben Goodger (Google), dglazkov

Base URL:
svn://chrome-svn/chrome/trunk/src/

Visibility:
Public.

Description

Pop-up chrome. BUG=http://crbug.com/15727 TEST=Load a web page that has a popup. Example: <!DOCTYPE html> Click button to open a new window. <br> <button onclick="w = window.open('http://www.google.com', 'New Window', 'width=512,height=512'); w.moveTo(300,300);">Open with w,h</button> Now click on the button to get a pop-up. In the new window, make sure there is no "tab" area above the URL bar, and no "new tab" button. Hit Cmd-T to create a new tab, and make sure it gets created in the OTHER window. More work is needed to minimize pop-up chrome more, but this'll prevent the most brutal failures (e.g. team meeting "demo" today). Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=19837

Patch Set 1 #

Total comments: 6

Created: 11 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+54 lines, -6 lines)			Patch
M	chrome/browser/cocoa/browser_window_controller.mm	View	2 chunks	+12 lines, -0 lines	0 comments	Download
M	chrome/browser/cocoa/browser_window_controller_unittest.mm	View	1 chunk	+18 lines, -0 lines	2 comments	Download
M	chrome/browser/cocoa/tab_window_controller.h	View	1 chunk	+6 lines, -0 lines	2 comments	Download
M	chrome/browser/cocoa/tab_window_controller.mm	View	2 chunks	+18 lines, -6 lines	2 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

John Grabowski

Note general behavior will become the same as Windows (e.g. can't create new tabs on ...

11 years, 5 months ago (2009-07-01 03:40:43 UTC) #1

pink (ping after 24hrs)

LGTM with some questions. http://codereview.chromium.org/151135/diff/1/2 File chrome/browser/cocoa/browser_window_controller_unittest.mm (right): http://codereview.chromium.org/151135/diff/1/2#newcode95 Line 95: controller_.release(); why is this ...

11 years, 5 months ago (2009-07-01 12:53:00 UTC) #2

John Grabowski

11 years, 5 months ago (2009-07-02 20:28:24 UTC) #3

http://codereview.chromium.org/151135/diff/1/2
File chrome/browser/cocoa/browser_window_controller_unittest.mm (right):

http://codereview.chromium.org/151135/diff/1/2#newcode95
Line 95: controller_.release();
On 2009/07/01 12:53:00, pink wrote:
> why is this necessary? won't it do that when the test finishes?

Somehow the controller gets autoreleased.  I can confirm this by seeing it's
dealloc called when an autorelease pool gets drained.  I could not, however,
figure out when it gets autoreleased.  My best guess is [BrowserWindowController
destroyBrowser], but a breakpoint there wasn't hit.  Maybe gdb doesn't like me
today.

I can also confirm that valgrind is happy with this change, and that an extra
release of this object blows up (dbl-free).  

I wish I had a better answer.

http://codereview.chromium.org/151135/diff/1/5
File chrome/browser/cocoa/tab_window_controller.h (right):

http://codereview.chromium.org/151135/diff/1/5#newcode92
Line 92: - (BOOL)isNormalWindow;
On 2009/07/01 12:53:00, pink wrote:
> javascript has a full set of window chrome flags that you can enable and
disable
> on a browser (toolbar, bookmark bar, status bar). Does WinChrome wrap all that
> up into the types, or do we need to deal with that as well?

The answer is both.  POPUP windows (not normal) on windows don't show bookmark
bars, don't allow you to 'location=0', etc.  

However, in the bigger picture, we really want a new nib for popups, not just a
mod to an existing nib.  (Location bar on Windows pop-ups is slightly different
than the location bar on normal windows).  So the normal check will be adequate,
but needs to cause more change.

http://codereview.chromium.org/151135/diff/1/3
File chrome/browser/cocoa/tab_window_controller.mm (right):

http://codereview.chromium.org/151135/diff/1/3#newcode22
Line 22: // different window).
On 2009/07/01 12:53:00, pink wrote:
> does the content area need adjusting up? I think it's positioned lower than
> "normal" in the nib assuming a tab strip will go above it. Does it look too
low
> in the window w/out it?

Yes, it is too low.  The right answer is a new nib for pop-ups.  However, in the
immediate term, I want to prevent the most brutal of problems when trying to use
Mac Chrome with Presently.  I'll save the new nib for our UI polish phase, and
will leave the bug open.

Expand Messages | Collapse Messages