Clean up use of adjustWindowRect
authorkenneth@webkit.org <kenneth@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 14 Nov 2012 11:05:20 +0000 (11:05 +0000)
committerkenneth@webkit.org <kenneth@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 14 Nov 2012 11:05:20 +0000 (11:05 +0000)
https://bugs.webkit.org/show_bug.cgi?id=102072

Reviewed by Gyuyoung Kim.

Source/WebCore:

Tested by fast/dom/Window/open-window-min-size.html

* loader/FrameLoader.cpp:
(WebCore::createWindow):

    Validate the window size here so that it is not just done for
    .open, but also for .showModalDialog. This is compatible with
    other browsers such as IE and Firefox (though IE > 6, enforces
    a minimum width of 250 instead of 100 as Firefox and us.)

* page/DOMWindow.cpp:
(WebCore):
(WebCore::DOMWindow::adjustWindowRect):

    Make it a static method which only takes page. It was never
    called from anywhere without a valid page, so the page check
    has been turned into an assert, and two of the arguments have
    been removed as they can be accessed via the page.

(WebCore::DOMWindow::moveBy):
(WebCore::DOMWindow::moveTo):
(WebCore::DOMWindow::resizeBy):
(WebCore::DOMWindow::resizeTo):

    Update use of adjustWindowRect.

(WebCore::DOMWindow::open):

    Avoid modifying the WindowFeatures as the WebCore::createWindow
    validates and adjusts the arguments.

* page/DOMWindow.h:
(DOMWindow):

LayoutTests:

Test that minimum sizes are honored.

* fast/dom/Window/open-window-min-size.html: Added.
* fast/dom/Window/open-window-min-size-expected.txt: Added.
* fast/dom/Window/resources/report-size-and-close.html: Added.
* platform/mac/TestExpectations: Skipped as it times out.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@134586 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/dom/Window/open-window-min-size-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/Window/open-window-min-size.html [new file with mode: 0644]
LayoutTests/fast/dom/Window/resources/report-size-and-close.html [new file with mode: 0644]
LayoutTests/platform/mac/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/page/DOMWindow.cpp
Source/WebCore/page/DOMWindow.h

index 5fe7fe0..fb97332 100644 (file)
@@ -1,3 +1,17 @@
+2012-11-14  Kenneth Rohde Christiansen  <kenneth@webkit.org>
+
+        Clean up use of adjustWindowRect
+        https://bugs.webkit.org/show_bug.cgi?id=102072
+
+        Reviewed by Gyuyoung Kim.
+
+        Test that minimum sizes are honored.
+
+        * fast/dom/Window/open-window-min-size.html: Added.
+        * fast/dom/Window/open-window-min-size-expected.txt: Added.
+        * fast/dom/Window/resources/report-size-and-close.html: Added.
+        * platform/mac/TestExpectations: Skipped as it times out.
+
 2012-11-14  Kentaro Hara  <haraken@chromium.org>
 
         Unreviewed. Fixed lint error.
diff --git a/LayoutTests/fast/dom/Window/open-window-min-size-expected.txt b/LayoutTests/fast/dom/Window/open-window-min-size-expected.txt
new file mode 100644 (file)
index 0000000..59dced9
--- /dev/null
@@ -0,0 +1,8 @@
+PASS successfullyParsed is true
+
+TEST COMPLETE
+PASS 100 is >= 100
+PASS 100 is >= 100
+PASS 100 is >= 100
+PASS 100 is >= 100
+
diff --git a/LayoutTests/fast/dom/Window/open-window-min-size.html b/LayoutTests/fast/dom/Window/open-window-min-size.html
new file mode 100644 (file)
index 0000000..45a75dc
--- /dev/null
@@ -0,0 +1,34 @@
+<html>
+<head>
+<script src="../../js/resources/js-test-pre.js"></script>
+<script>
+function validate(msg) {
+    shouldBeGreaterThanOrEqual(msg["width"].toString(), "100");
+    shouldBeGreaterThanOrEqual(msg["height"].toString(), "100");
+}
+
+function test() {
+    var data = window.showModalDialog("resources/report-size-and-close.html", "", "dialogWidth:10; dialogHeight:10");
+    validate(data);
+
+    window.addEventListener('message', function(e) {
+        validate(e.data);
+        e.source.close();
+        testRunner.notifyDone();
+    }, false);
+    window.open("resources/report-size-and-close.html", "non-empty-argument", "width=10, height=10");
+}
+
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.waitUntilDone();
+    testRunner.setCanOpenWindows();
+    testRunner.setCloseRemainingWindowsWhenComplete(true);
+}
+
+</script>
+<script src="../../js/resources/js-test-post.js"></script>
+</head>
+<body onload="test()">
+</body>
+</html>
diff --git a/LayoutTests/fast/dom/Window/resources/report-size-and-close.html b/LayoutTests/fast/dom/Window/resources/report-size-and-close.html
new file mode 100644 (file)
index 0000000..a92480d
--- /dev/null
@@ -0,0 +1,12 @@
+<body onload="loaded()">
+<script>
+function loaded() {
+    var msg = { "width": window.outerWidth, "height": window.outerHeight };
+    if (window.dialogArguments != undefined) {
+        window.returnValue = msg;
+        window.close();
+    } else
+        window.opener.postMessage(msg, '*');
+}
+</script>
+</body>
index 214fe22..063df99 100644 (file)
@@ -510,6 +510,9 @@ fast/forms/search-placeholder-value-changed.html
 # https://bugs.webkit.org/show_bug.cgi?id=72435
 fast/dom/Window/window-postmessage-arrays.html
 
+# Timeout.
+fast/dom/Window/open-window-min-size.html
+
 # https://bugs.webkit.org/show_bug.cgi?id=73148
 fast/canvas/webgl/webgl-texture-binding-preserved.html
 
index 4f486b3..9c7bd39 100644 (file)
@@ -1,3 +1,44 @@
+2012-11-14  Kenneth Rohde Christiansen  <kenneth@webkit.org>
+
+        Clean up use of adjustWindowRect
+        https://bugs.webkit.org/show_bug.cgi?id=102072
+
+        Reviewed by Gyuyoung Kim.
+
+        Tested by fast/dom/Window/open-window-min-size.html
+
+        * loader/FrameLoader.cpp:
+        (WebCore::createWindow):
+
+            Validate the window size here so that it is not just done for
+            .open, but also for .showModalDialog. This is compatible with
+            other browsers such as IE and Firefox (though IE > 6, enforces
+            a minimum width of 250 instead of 100 as Firefox and us.)
+
+        * page/DOMWindow.cpp:
+        (WebCore):
+        (WebCore::DOMWindow::adjustWindowRect):
+
+            Make it a static method which only takes page. It was never
+            called from anywhere without a valid page, so the page check
+            has been turned into an assert, and two of the arguments have
+            been removed as they can be accessed via the page.
+
+        (WebCore::DOMWindow::moveBy):
+        (WebCore::DOMWindow::moveTo):
+        (WebCore::DOMWindow::resizeBy):
+        (WebCore::DOMWindow::resizeTo):
+
+            Update use of adjustWindowRect.
+
+        (WebCore::DOMWindow::open):
+
+            Avoid modifying the WindowFeatures as the WebCore::createWindow
+            validates and adjusts the arguments.
+
+        * page/DOMWindow.h:
+        (DOMWindow):
+
 2012-11-14  Takashi Sakamoto  <tasak@google.com>
 
         Crash when replacing parts of text inputs with content: url(...)
index cd7feae..ffc2283 100644 (file)
@@ -3321,21 +3321,25 @@ Frame* createWindow(Frame* openerFrame, Frame* lookupFrame, const FrameLoadReque
     page->chrome()->setResizable(features.resizable);
 
     // 'x' and 'y' specify the location of the window, while 'width' and 'height'
-    // specify the size of the page. We can only resize the window, so
-    // adjust for the difference between the window size and the page size.
+    // specify the size of the viewport. We can only resize the window, so adjust
+    // for the difference between the window size and the viewport size.
 
     FloatRect windowRect = page->chrome()->windowRect();
-    FloatSize pageSize = page->chrome()->pageRect().size();
+    FloatSize viewportSize = page->chrome()->pageRect().size();
+
     if (features.xSet)
         windowRect.setX(features.x);
     if (features.ySet)
         windowRect.setY(features.y);
     if (features.widthSet)
-        windowRect.setWidth(features.width + (windowRect.width() - pageSize.width()));
+        windowRect.setWidth(features.width + (windowRect.width() - viewportSize.width()));
     if (features.heightSet)
-        windowRect.setHeight(features.height + (windowRect.height() - pageSize.height()));
-    page->chrome()->setWindowRect(windowRect);
+        windowRect.setHeight(features.height + (windowRect.height() - viewportSize.height()));
+
+    // Ensure non-NaN values, minimum size as well as being within valid screen area.
+    FloatRect newWindowRect = DOMWindow::adjustWindowRect(page, windowRect);
 
+    page->chrome()->setWindowRect(newWindowRect);
     page->chrome()->show();
 
     created = true;
index a84fec5..acdec95 100644 (file)
@@ -308,16 +308,18 @@ void DOMWindow::dispatchAllPendingUnloadEvents()
 }
 
 // This function:
-// 1) Validates the pending changes are not changing to NaN
-// 2) Constrains the window rect to the minimum window size and no bigger than
-//    the the float rect's dimensions.
-// 3) Constrain window rect to within the top and left boundaries of the screen rect
-// 4) Constraint the window rect to within the bottom and right boundaries of the
-//    screen rect.
-// 5) Translate the window rect coordinates to be within the coordinate space of
-//    the screen rect.
-void DOMWindow::adjustWindowRect(const FloatRect& screen, FloatRect& window, const FloatRect& pendingChanges) const
+// 1) Validates the pending changes are not changing any value to NaN; in that case keep original value.
+// 2) Constrains the window rect to the minimum window size and no bigger than the float rect's dimensions.
+// 3) Constrains the window rect to within the top and left boundaries of the available screen rect.
+// 4) Constrains the window rect to within the bottom and right boundaries of the available screen rect.
+// 5) Translate the window rect coordinates to be within the coordinate space of the screen.
+FloatRect DOMWindow::adjustWindowRect(Page* page, const FloatRect& pendingChanges)
 {
+    ASSERT(page);
+
+    FloatRect screen = screenAvailableRect(page->mainFrame()->view());
+    FloatRect window = page->chrome()->windowRect();
+
     // Make sure we're in a valid state before adjusting dimensions.
     ASSERT(isfinite(screen.x()));
     ASSERT(isfinite(screen.y()));
@@ -327,7 +329,7 @@ void DOMWindow::adjustWindowRect(const FloatRect& screen, FloatRect& window, con
     ASSERT(isfinite(window.y()));
     ASSERT(isfinite(window.width()));
     ASSERT(isfinite(window.height()));
-    
+
     // Update window values if new requested values are not NaN.
     if (!isnan(pendingChanges.x()))
         window.setX(pendingChanges.x());
@@ -338,15 +340,15 @@ void DOMWindow::adjustWindowRect(const FloatRect& screen, FloatRect& window, con
     if (!isnan(pendingChanges.height()))
         window.setHeight(pendingChanges.height());
 
-    ASSERT(m_frame);
-    Page* page = m_frame->page();
-    FloatSize minimumSize = page ? m_frame->page()->chrome()->client()->minimumWindowSize() : FloatSize(100, 100);
+    FloatSize minimumSize = page->chrome()->client()->minimumWindowSize();
     window.setWidth(min(max(minimumSize.width(), window.width()), screen.width()));
     window.setHeight(min(max(minimumSize.height(), window.height()), screen.height()));
 
-    // Constrain the window position to the screen.
+    // Constrain the window position within the valid screen area.
     window.setX(max(screen.x(), min(window.x(), screen.maxX() - window.width())));
     window.setY(max(screen.y(), min(window.y(), screen.maxY() - window.height())));
+
+    return window;
 }
 
 // FIXME: We can remove this function once V8 showModalDialog is changed to use DOMWindow.
@@ -1450,8 +1452,7 @@ void DOMWindow::moveBy(float x, float y) const
     FloatRect update = fr;
     update.move(x, y);
     // Security check (the spec talks about UniversalBrowserWrite to disable this check...)
-    adjustWindowRect(screenAvailableRect(page->mainFrame()->view()), fr, update);
-    page->chrome()->setWindowRect(fr);
+    page->chrome()->setWindowRect(adjustWindowRect(page, update));
 }
 
 void DOMWindow::moveTo(float x, float y) const
@@ -1470,10 +1471,9 @@ void DOMWindow::moveTo(float x, float y) const
     FloatRect sr = screenAvailableRect(page->mainFrame()->view());
     fr.setLocation(sr.location());
     FloatRect update = fr;
-    update.move(x, y);     
+    update.move(x, y);
     // Security check (the spec talks about UniversalBrowserWrite to disable this check...)
-    adjustWindowRect(sr, fr, update);
-    page->chrome()->setWindowRect(fr);
+    page->chrome()->setWindowRect(adjustWindowRect(page, update));
 }
 
 void DOMWindow::resizeBy(float x, float y) const
@@ -1491,8 +1491,7 @@ void DOMWindow::resizeBy(float x, float y) const
     FloatRect fr = page->chrome()->windowRect();
     FloatSize dest = fr.size() + FloatSize(x, y);
     FloatRect update(fr.location(), dest);
-    adjustWindowRect(screenAvailableRect(page->mainFrame()->view()), fr, update);
-    page->chrome()->setWindowRect(fr);
+    page->chrome()->setWindowRect(adjustWindowRect(page, update));
 }
 
 void DOMWindow::resizeTo(float width, float height) const
@@ -1510,8 +1509,7 @@ void DOMWindow::resizeTo(float width, float height) const
     FloatRect fr = page->chrome()->windowRect();
     FloatSize dest = FloatSize(width, height);
     FloatRect update(fr.location(), dest);
-    adjustWindowRect(screenAvailableRect(page->mainFrame()->view()), fr, update);
-    page->chrome()->setWindowRect(fr);
+    page->chrome()->setWindowRect(adjustWindowRect(page, update));
 }
 
 int DOMWindow::setTimeout(PassOwnPtr<ScheduledAction> action, int timeout, ExceptionCode& ec)
@@ -1930,15 +1928,6 @@ PassRefPtr<DOMWindow> DOMWindow::open(const String& urlString, const AtomicStrin
     }
 
     WindowFeatures windowFeatures(windowFeaturesString);
-    FloatRect windowRect(windowFeatures.xSet ? windowFeatures.x : 0, windowFeatures.ySet ? windowFeatures.y : 0,
-        windowFeatures.widthSet ? windowFeatures.width : 0, windowFeatures.heightSet ? windowFeatures.height : 0);
-    Page* page = m_frame->page();
-    DOMWindow::adjustWindowRect(screenAvailableRect(page ? page->mainFrame()->view() : 0), windowRect, windowRect);
-    windowFeatures.x = windowRect.x();
-    windowFeatures.y = windowRect.y();
-    windowFeatures.height = windowRect.height();
-    windowFeatures.width = windowRect.width();
-
     Frame* result = createWindow(urlString, frameName, windowFeatures, activeWindow, firstFrame, m_frame);
     return result ? result->document()->domWindow() : 0;
 }
@@ -1958,7 +1947,8 @@ void DOMWindow::showModalDialog(const String& urlString, const String& dialogFea
     if (!canShowModalDialogNow(m_frame) || !firstWindow->allowPopUp())
         return;
 
-    Frame* dialogFrame = createWindow(urlString, emptyAtom, WindowFeatures(dialogFeaturesString, screenAvailableRect(m_frame->view())),
+    WindowFeatures windowFeatures(dialogFeaturesString, screenAvailableRect(m_frame->view()));
+    Frame* dialogFrame = createWindow(urlString, emptyAtom, windowFeatures,
         activeWindow, firstFrame, m_frame, function, functionContext);
     if (!dialogFrame)
         return;
index 95f6ef1..424f58a 100644 (file)
@@ -120,7 +120,7 @@ namespace WebCore {
         static bool dispatchAllPendingBeforeUnloadEvents();
         static void dispatchAllPendingUnloadEvents();
 
-        void adjustWindowRect(const FloatRect& screen, FloatRect& window, const FloatRect& pendingChanges) const;
+        static FloatRect adjustWindowRect(Page*, const FloatRect& pendingChanges);
 
         // FIXME: We can remove this function once V8 showModalDialog is changed to use DOMWindow.
         static void parseModalDialogFeatures(const String&, HashMap<String, String>&);