Reviewed by darin.
authorggaren <ggaren@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 7 Feb 2006 21:32:55 +0000 (21:32 +0000)
committerggaren <ggaren@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 7 Feb 2006 21:32:55 +0000 (21:32 +0000)
        - Fixed <rdar://problem/4425269> REGRESSION: wrong size pop-up when
        downloading attachment at webmail.mac.com (6882)

        The bug was that the feature string parser didn't always initialize the
        *Set variables, so WebCore assumed a height was set when it wasn't,
        and used a garbage height variable.

        While there, I fixed two other bugs: (1) The parser wasn't chewing up
        as many characters as I thought it was. (Chewing up extra characters
        is necessary to match Win IE.) (2) We considered \t and \f to be
        whitespace, but Win IE doesn't.

        * manual-tests/window-open-features-parsing.html: Added these cases,
        fixed up comments.

        * bridge/BrowserExtension.h: Cleaned up declaration order a bit.
        * khtml/ecma/kjs_window.cpp:
        (KJS::isSeparator): New function, tells you if a character is a
        separator
        (KJS::parseWindowFeatures): Always initialize *Set to false. Drive
        parsing based on invalid characters ('separators') rather than valid
        ones, to match Win IE.

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

WebCore/ChangeLog
WebCore/bridge/BrowserExtension.h
WebCore/khtml/ecma/kjs_window.cpp
WebCore/manual-tests/window-open-features-parsing.html

index b9bb3e83ea5b160b434a7e2ed6f027c1df08f814..ea5a9a27cafcd36ebb1cc108e3f8bb8ac9e7dca9 100644 (file)
@@ -1,3 +1,30 @@
+2006-02-07  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by darin.
+
+        - Fixed <rdar://problem/4425269> REGRESSION: wrong size pop-up when 
+        downloading attachment at webmail.mac.com (6882)
+
+        The bug was that the feature string parser didn't always initialize the
+        *Set variables, so WebCore assumed a height was set when it wasn't,
+        and used a garbage height variable.
+
+        While there, I fixed two other bugs: (1) The parser wasn't chewing up 
+        as many characters as I thought it was. (Chewing up extra characters 
+        is necessary to match Win IE.) (2) We considered \t and \f to be 
+        whitespace, but Win IE doesn't.
+
+        * manual-tests/window-open-features-parsing.html: Added these cases,
+        fixed up comments.
+
+        * bridge/BrowserExtension.h: Cleaned up declaration order a bit.
+        * khtml/ecma/kjs_window.cpp:
+        (KJS::isSeparator): New function, tells you if a character is a
+        separator
+        (KJS::parseWindowFeatures): Always initialize *Set to false. Drive
+        parsing based on invalid characters ('separators') rather than valid 
+        ones, to match Win IE.
+
 2006-02-07  Alexey Proskuryakov  <ap@nypop.com>
 
         Reviewed by Timothy.
index 7773b7c3aa0a2ff3f622abce1c8935af221090af..0ce3f162491f163af0dd185a95c6eb3312cf50f9 100644 (file)
@@ -68,20 +68,22 @@ private:
 
 struct WindowArgs {
     int x;
+    bool xSet;
     int y;
+    bool ySet;
     int width;
+    bool widthSet;
     int height;
+    bool heightSet;
+
     bool menuBarVisible;
     bool statusBarVisible;
     bool toolBarVisible;
     bool locationBarVisible;
     bool scrollBarsVisible;
     bool resizable;
+
     bool fullscreen;
-    bool xSet;
-    bool ySet;
-    bool widthSet;
-    bool heightSet;
     bool dialog;
 };
 
index ab7ea2b40d82fb24a0de42d159c8a6c25c750523..2732149c4c28472d3db1cfb1014e2943b55d7193 100644 (file)
@@ -1445,6 +1445,12 @@ static void setWindowFeature(const DOMString& keyString, const DOMString& valueS
         windowArgs.scrollBarsVisible = value;
 }
 
+// Though isspace() considers \t and \v to be whitespace, Win IE doesn't.
+static bool isSeparator(QChar c)
+{
+    return c == ' ' || c == '\t' || c == '\n' || c == '\r' || c == '=' || c == ',' || c == '\0';
+}
+
 static void parseWindowFeatures(const DOMString& features, WindowArgs& windowArgs)
 {
     /*
@@ -1458,6 +1464,11 @@ static void parseWindowFeatures(const DOMString& features, WindowArgs& windowArg
     windowArgs.dialog = false;
     windowArgs.fullscreen = false;
     
+    windowArgs.xSet = false;
+    windowArgs.ySet = false;
+    windowArgs.widthSet = false;
+    windowArgs.heightSet = false;
+    
     if (features.length() == 0) {
         windowArgs.menuBarVisible = true;
         windowArgs.statusBarVisible = true;
@@ -1466,11 +1477,6 @@ static void parseWindowFeatures(const DOMString& features, WindowArgs& windowArg
         windowArgs.scrollBarsVisible = true;
         windowArgs.resizable = true;
         
-        windowArgs.xSet = false;
-        windowArgs.ySet = false;
-        windowArgs.widthSet = false;
-        windowArgs.heightSet = false;
-        
         return;
     }
     
@@ -1489,16 +1495,16 @@ static void parseWindowFeatures(const DOMString& features, WindowArgs& windowArg
     int length = features.length();
     DOMString buffer = features.lower();
     while (i < length) {
-        // skip to first letter or number, but don't skip past the end of the string
-        while (!buffer[i].isLetterOrNumber()) {
+        // skip to first non-separator, but don't skip past the end of the string
+        while (isSeparator(buffer[i])) {
             if (i >= length)
                 break;
             i++;
         }
         keyBegin = i;
         
-        // skip to first non-letter, non-number
-        while (buffer[i].isLetterOrNumber())
+        // skip to first separator
+        while (!isSeparator(buffer[i]))
             i++;
         keyEnd = i;
         
@@ -1509,18 +1515,20 @@ static void parseWindowFeatures(const DOMString& features, WindowArgs& windowArg
             i++;
         }
         
-        // skip to first letter or number, but don't skip past a ',' or the end of the string
-        while (!buffer[i].isLetterOrNumber()) {
+        // skip to first non-separator, but don't skip past a ',' or the end of the string
+        while (isSeparator(buffer[i])) {
             if (buffer[i] == ',' || i >= length)
                 break;
             i++;
         }
         valueBegin = i;
         
-        // skip to first non-letter, non-number
-        while (buffer[i].isLetterOrNumber())
+        // skip to first separator
+        while (!isSeparator(buffer[i]))
             i++;
         valueEnd = i;
+        
+        assert(i <= length);
 
         DOMString keyString(buffer.substring(keyBegin, keyEnd - keyBegin));
         DOMString valueString(buffer.substring(valueBegin, valueEnd - valueBegin));
index 31839859993171add989aed84460e98c300a332f..cd686abf384407aac34d704f748cc53d1d75fe7f 100644 (file)
@@ -1,15 +1,16 @@
 <html><head><script>
 function test() {
     /* The lowdown on this feature string:
-        - width: reads as 0200|0, which, after strtol, gives you 200
-        - height: reads as "", which means yes, which means 1, but the minimum size is 100
-        - width1: reads as width1, an invalid key, so it doesn't override width
-        - left: reads as no, which means 0, which means aligned to the left side of the screen
-        - toolBAR: reads as yes
-        - resizable: reads as yess, which is invalid, which means no
-        - status: reads as "", which means yes
+        - ,=\twidth: reads as key:width value:0200px|0, which, after strtol/toInt, gives you 200
+        - =height: reads as key:height value:"", which means yes, which means 1, but the minimum size is 100, so 100
+        - 1width: reads as key:1width, an invalid key, so it doesn't override the previous width
+        - left: reads as key:left value:no, which means 0, which means aligned to the left side of the screen
+        - \ntoolBAR: reads as key:toolbar value:yes
+        - resizable: reads as key:resizable value:yess, which is invalid, which means no
+        - \rstatus: reads as key:status value:"", which means yes
+        - the trailing comma catches a previous mistake i made reading past the end of the string
     */    
-    var sFeatures = "  ,=width ==      = =     0200px|0=height  400,1width=400,left=nO toolBAR=yeS,resizable=yess,  \t\v\f\r\nstatus= ,"; 
+    var sFeatures = "  ,=\twidth ==    = =     0200px|0=height  400,1width=400,left=nO \ntoolBAR=yeS,resizable=yess, \rstatus= ,"; 
     var w = window.open("resources/popup200x100.html", undefined, sFeatures); 
     w.focus();
 }
@@ -18,12 +19,15 @@ function test() {
 <p>This test checks whether parsing of the 'features' argument to window.open matches Win IE's behavior.</p>
 <p>The link below should open a window with the following attributes:</p>
 <ul>
-<li> A WebView exactly 200x100, such that you can see a red 1 pixel border around the edge of the whole WebView.
+<li> A WebView exactly 200x100, such that you can see a red 1 pixel border along each edge of the WebView.
 <li> A window aligned to the left hand side of the screen.
 <li> Toolbar visible.
 <li> Statusbar visible.
 <li> Not resizable.
 </ul>
+<a href="" onclick="test(); return false;">Click to test</a>
 <hr>
-<a href="" onclick="test(); return false;">Click to test.</a>
+<p>The link below should open a window whose dimensions and positioning match
+what you would get from file->New Window.</p>
+<a href="" onclick='window.open("", "", "status,resizable");'>Click to test</a>
 </body></html>