[webkit-reviews] review denied: [Bug 79611] Be more restrictive when adding ScrollableArea's to FrameView's ScrollableArea's map : [Attachment 131543] patch v2
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Mar 18 17:27:46 PDT 2012
James Robinson <jamesr at chromium.org> has denied Antonio Gomes
<tonikitoo at webkit.org>'s request for review:
Bug 79611: Be more restrictive when adding ScrollableArea's to FrameView's
ScrollableArea's map
https://bugs.webkit.org/show_bug.cgi?id=79611
Attachment 131543: patch v2
https://bugs.webkit.org/attachment.cgi?id=131543&action=review
------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=131543&action=review
Logic looks reasonable but the testing needs to be better to feel confident
with this patch.
> LayoutTests/scrollbars/resources/hidden-scrollable-textarea.html:1
> +<textarea id="textarea1" style="visibility:hidden;" rows='5' cols='20'>
<!DOCTYPE html>
> LayoutTests/scrollbars/resources/scrollable-divs.html:1
> +<html>
<!DOCTYPE html>
> LayoutTests/scrollbars/resources/scrollable-textarea.html:1
> +<textarea rows='5' cols='20'>
<!DOCTYPE html>
> LayoutTests/scrollbars/scrollable-areas-count-expected.txt:2
> +WARN: shouldBe() expects string arguments
the script thinks you are misusing shouldBe() and I think it's right - you
should be doing something like shouldBe("result", "36"); the script eval()s
both sides and then compares them.
> LayoutTests/scrollbars/scrollable-areas-count.html:1
> +<head>
<!DOCTYPE html>
> LayoutTests/scrollbars/scrollable-areas-count.html:16
> + shouldBeTrue(stringify(result) == "36");
this test is very fragile - if you have an even number of off-by-one errors in
the calculations used below, this test would still pass. could you break it out
into one test per interesting condition (for instance, one dedicated test for a
visibility:hidden iframe with normal content and another for visibility:visible
iframe with hidden content)
> Source/WebCore/page/FrameView.cpp:2585
> + // Cover #3 and #4.
what's 4?
what happened to 2?
More information about the webkit-reviews
mailing list