[webkit-reviews] review requested: [Bug 79611] Be more restrictive when adding ScrollableArea's to FrameView's ScrollableArea's map : [Attachment 136718] patch v3

Wed Apr 11 12:04:35 PDT 2012

Antonio Gomes <tonikitoo at webkit.org> has asked	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 136718: patch v3
https://bugs.webkit.org/attachment.cgi?id=136718&action=review

------- Additional Comments from Antonio Gomes <tonikitoo at webkit.org>
(In reply to comment #14)
> (From update of attachment 131543 [details])
> 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.

Made each relevant case a test.

> > LayoutTests/scrollbars/resources/hidden-scrollable-textarea.html:1
> > +<textarea id="textarea1" style="visibility:hidden;" rows='5' cols='20'>
> 
> <!DOCTYPE html>

Fixed throughout the new patch.
> 
> > 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.

Fixed.

> > 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)

Fixed by making each "use case" a test.

> > Source/WebCore/page/FrameView.cpp:2585
> > +	 // Cover #3 and #4.
> 
> what's 4?
> what happened to 2?

Fixed.