Bug 119633

Treating (0, 0) as default size falls apart because windowRect height ends up being 24 here: if (features.heightSet) windowRect.setHeight(features.height + (windowRect.height() - viewportSize.height()));

window.resizeTo is broken too.

I'm thinking that r142755 should be rolled out entirely, and replaced with a fix in createWindow() function in FrameLoader.cpp: - if (features.widthSet) + if (features.widthSet && features.width) windowRect.setWidth(features.width + (windowRect.width() - viewportSize.width())); - if (features.heightSet) + if (features.heightSet && features.height) windowRect.setHeight(features.height + (windowRect.height() - viewportSize.height())); Allan, does this make sense?

Created attachment 208451 [details] proposed fix

Comment on attachment 208451 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=208451&action=review > Source/WebCore/ChangeLog:3 > + REGRESSION (r142755): window.open creates an invisible window when width and height are 0 Seems like the bug is when width *or* height is zero. What about small widths and heights that are not zero?

Comment on attachment 208451 [details] proposed fix Attachment 208451 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/1410299

I don't understand how the browser is supposed to be able to guess if default size was requested when the get an openWindow call. The issue is this: In many browsers windows that request a specific size are opened in a separate window, but windows that request default size are opened in a tab. This is decided outside of WebKit, and thus WebKit needs to tell the browser that default was requested. If 0x0 is not used, what is?

Looks like Gtk bot is broken now. > Seems like the bug is when width *or* height is zero. What about small widths and heights that are not zero? These are handled in DOMWindow::adjustWindowRect, which enforces a minimum size. Width and height are adjusted to be at least 100. > In many browsers windows that request a specific size are opened in a separate window, but windows that request default size are opened in a tab. When WebKit creates a window, it's before any of this code is run (oldPage->chrome().createWindow() in FrameLoader.cpp). The window is always created with a default size, by definition of default size, and is then resized by the code this patch touches. Afterwards, it's displayed with page->chrome().show(). This has to work in all WebKit based browsers as long as window.open(url, "", "") works with no explicit size. We just read the current size from client using page->chrome().windowRect() and page->chrome().pageRect(), adjust it to be on screen and to not be too small, and feed back to the client. I find it a bit strange how this logic is split between WebKit and client, but it works. Are you saying that there are WebKit based browsers that make the decision on whether to put the window in tab based on requested size? Presumably they make the decision in their show() implementation then.

Comment on attachment 208451 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=208451&action=review > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:131 > - if (!ewk_view_setting_enable_auto_resize_window_get(m_view) || rect.isEmpty()) > + if (!ewk_view_setting_enable_auto_resize_window_get(m_view)) Actually, I think that this code used to break moving the window to screen if it had default size, but an offscreen position. But perhaps that can never happen in practice.

> I don't understand how the browser is supposed to be able to guess if default size was requested when the get an openWindow call. To answer your question more directly, WebCore does pass all the requested features to the client, so it can do anything even upfront in createWindow(). It is kind of strange that WebCore enforces additional constraints after the fact, I don't know why we do this.

(In reply to comment #10) > Are you saying that there are WebKit based browsers that make the decision on whether to put the window in tab based on requested size? Presumably they make the decision in their show() implementation then. Yes all of them except Safari apparently.

(In reply to comment #11) > (In reply to comment #10) > > Are you saying that there are WebKit based browsers that make the decision on whether to put the window in tab based on requested size? Presumably they make the decision in their show() implementation then. > > Yes all of them except Safari apparently. Well, specifically the classic desktop browsers implementing tabs. Default size windows opening in tabs is expected behavior on a lot of sites that can control whether a new window should popup or not that way. WebKit for years requsted a window of size (0,0) if no size was set, so that became how the browsers implemented the standard behavior. The minimum size for unset window size was only enforced for a few months before I modified it to let 0 through again. Did something change in Safari to rely on the minimum in the timespan it was enforced?

> WebKit for years requsted a window of size (0,0) if no size was set The windows is created by this method: Page* Chrome::createWindow(Frame* frame, const FrameLoadRequest& request, const WindowFeatures& features, const NavigationAction& action) const { Page* newPage = m_client->createWindow(frame, request, features, action); ... } How was the (0, 0) size passed? The size is in WindowFeatures, but neither your not my patch change anything about that structure content. We only change later calls that resize the already created window. > The minimum size for unset window size was only enforced for a few months before I modified it to let 0 through again. Do you know when this was added? As mentioned before, I'm surprised that any of this logic is in WebCore.

Comment on attachment 208451 [details] proposed fix Clearing flags on attachment: 208451 Committed r153913: <http://trac.webkit.org/changeset/153913>

All reviewed patches have been landed. Closing bug.

(In reply to comment #13) > Do you know when this was added? As mentioned before, I'm surprised that any of this logic is in WebCore. The adjustment was there before but the part that overwrote default 0,0 with minimum came from http://trac.webkit.org/changeset/134586

Thank you for the pointer. Based on what I learned looking at this code today, it's probably wrong to call adjustWindowRect when creating a new window, and in general, it seems that logic added in r134586 should be client's responsibility.