[webkit-reviews] review denied: [Bug 3591] Frames only use integers as its length : [Attachment 22819] Interpret fractional-percentage MultiLength values.

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Brad Garcia <bgarcia at google.com>'s
request for review:
Bug 3591: Frames only use integers as its length
https://bugs.webkit.org/show_bug.cgi?id=3591

Attachment 22819: Interpret fractional-percentage MultiLength values.
https://bugs.webkit.org/attachment.cgi?id=22819&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
You'll need to use run-webkit-tests to generate the expected results and add
them to your patch.  Your test is layout-dependant, but not font-dependent. 
Since it's layout dependent, run-webkit-tests will automatically generate the
results under:
LayoutTests/platform/mac/fast/frames/frame-length-fractional-expected.txt
However since your test doesn't depend on the user's fonts, you can move this
file into LayoutTests/fast/frames/rame-length-fractional-expected.txt before
calling svn add (and adding it to your patch).

I wondered first if your backup case here:
+	 if (ok)
+	     return Length(r, Percent);
+	 return Length(1, Relative);

should try to do an int conversion first before returning Length(1, Relative)? 
But it appears that charactersToDouble returns !ok for the same cases
charactersToInt does.  It would be nice to validate this with a test case, but
such could be relatively complicated for such a simple fix.


I think generally we use:
unsigned intLength = i;
instead of:
unsigned intLength(i);
But I don't have strong opinions on that subject.


We could make a fancier test case for this, but I think this is OK-as is.

Hyatt should probably sanity check this, but assuming the layout tests pass
this looks fine to me.

r- due to lack of expected results for you test case (if this wasn't your first
patch, or you were a commiter, I'd just r+ and leave you to make the missing
results as you landed).



Oh, if you're interested in knowing how I might have made a fancier test case:

We have a nifty system for creating js-only test cases using some custom
assert-like macros.  Basically you copy this file:
http://trac.webkit.org/browser/trunk/LayoutTests/fast/js/resources/TEMPLATE.htm
l
into fast/frame/resources/

and then add a frame-length-fractional.js file to fast/frame/resources/ with
the following content:

description("My fancy test case for bug 3591");

var iframe = document.createElement("iframe");
iframe.style.width = "1000px";
iframe.style.height = "200px;
document.body.appendChild(iframe);

var frameset = document.createElement("frameset");
iframe.contentDocument.appendChild(frameset);
var frame1 = document.createElement("frame");
frameset.appendChild(frame1);
var frame2 = document.createElement("frame");
frameset.appendChild(frame2);


frameset.setAttribute("rows", "50%,*");
shouldBe(frame1.contentWidth, 500);

frameset.setAttribute("rows", "50.3%,*");
shouldBe(frame1.contentWidth, 503);

frameset.setAttribute("rows", "50.7%,*");
shouldBe(frame1.contentWidth, 507);

frameset.setAttribute("rows", "51%,*");
shouldBe(frame1.contentWidth, 510);

frameset.setAttribute("rows", "51 %,*");
shouldBe(frame1.contentWidth, 510);

// And then you could add a bunch of failure cases which touch the code
relating to your change:

frameset.setAttribute("rows", "51.3*");
shouldBe(frame1.contentWidth, 51/52 * 1000);

... etc.


Heck, I'd probably even have made my own shoudBe abstraction:

fuction testParsing(string, expectedWidth)
{
frameset.setAttribute("rows", string);
shouldBe(frame1.contentWidth, expectedWidth);
}

If you're interested in trying to add such a test case, be my guest.  It's not
required for this patch, but I figure the information is useful to you for
future patches anyway.