Bug 79611

ok, patch coming with tests.

Back to it

Created attachment 131279 [details] patch v1 Patch introduces a way to only cache actually scrollable ScrollableArea-derivated instances (FrameView and RenderLayer). For example, the added layout test counts for 36 scrollable areas with the patch and around 90 without it. For real world webpages, I tested many ones, including cnn.com main page, where it went from 11 scrollable areas cached to 0 (given that it has nothing actually scrollable, other than the mainframe). This level of accuracy, allows ports to use this information for in-region scroll target detection logic, etc.

Attachment 131279 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/scro..." exit_code: 1 Source/WebCore/page/FrameView.h:283: The parameter name "strategy" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/testing/Internals.h:124: The parameter name "document" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 131279 [details] patch v1 > Source/WebCore/page/FrameView.h:283: The parameter name "strategy" adds no information, so it should be removed. [readability/parameter_name] [5] I believe this is a bug in the style checker. Will file... > Source/WebCore/testing/Internals.h:124: The parameter name "document" adds no information, so it should be removed. [readability/parameter_name] [5] Will fix.

(In reply to comment #5) > (From update of attachment 131279 [details]) > > > Source/WebCore/page/FrameView.h:283: The parameter name "strategy" adds no information, so it should be removed. [readability/parameter_name] [5] > > I believe this is a bug in the style checker. Will file... bug 80835.

Created attachment 131345 [details] patch v1.1 - fixed style errors

Comment on attachment 131345 [details] patch v1.1 - fixed style errors View in context: https://bugs.webkit.org/attachment.cgi?id=131345&action=review I think ScrollableAreasSet should be ScrollableAreaSet, and the update function sshould be named accordingly. Also, the same thing goes for scrollableAreasCount. > Source/WebCore/page/FrameView.cpp:2564 > +void FrameView::updateScrollableAreasSet() > +{ This function seems overly complicated, can't we just check that the frame view has either a vertical or horizontal scrollbar? > Source/WebCore/rendering/RenderLayer.cpp:4416 > +void RenderLayer::updateScrollableAreasSet() > +{ Same question here, can't we just check for scrollbars?

(In reply to comment #8) > (From update of attachment 131345 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131345&action=review > > .... Also, the same thing goes for scrollableAreasCount. I like numberOfScrollableAreas for this kind of thing.

> > Source/WebCore/page/FrameView.cpp:2564 > > +void FrameView::updateScrollableAreasSet() > > +{ > > This function seems overly complicated, can't we just check that the frame view has either a vertical or horizontal scrollbar? That is exactly why I went with this mode detailed implementation. Ports can avoid scrollbars completely at FrameView creation time. So checking the presence of scrollbars would not be enough. That is why in the frameview method I explicitly checked for: - #1 contentsize > visiblesize - if #1 is true, we then check #2 if the web content specifies anything that prevents scrolling, for example overflow:hidden, scrolling=no, etc. Although, I agree the logic could be simpler. > > Source/WebCore/rendering/RenderLayer.cpp:4416 > > +void RenderLayer::updateScrollableAreasSet() > > +{ > > Same question here, can't we just check for scrollbars? Here, simplifying is easier.

> That is exactly why I went with this mode detailed implementation. s/mode/more/g

Created attachment 131543 [details] patch v2 @Sam/Anders: Made both updateScrollableAreaSet simpler, but as I explained in comment #10, just checking scrollbars is not enough, since ports can disable them at FrameView creation. Also made the renames you guys suggested.

@Anders/Sam: Would you guys have a moment for another review round?

Comment on attachment 131543 [details] patch v2 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?

Created attachment 136718 [details] patch v3 (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.

Keeping Julien in the loop as he's been changing related RenderLayer bits.

Comment on attachment 136718 [details] patch v3 Attachment 136718 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12389054

Created attachment 136729 [details] patch v3.1 - fixed mac warning /Volumes/Data/WebKit/Source/WebCore/testing/Internals.cpp:899: warning: unused parameter 'ec

Comment on attachment 136729 [details] patch v3.1 - fixed mac warning Attachment 136729 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12389149 New failing tests: fast/reflections/table-cell.html fast/table/overflowHidden.html fast/transforms/transform-table-row.html fast/table/table-anonymous-block-destroy-crash.html fast/block/positioning/offsetLeft-relative-td.html tables/mozilla_expected_failures/marvin/table_overflow_hidden_tr.html fast/css/webkit-mask-crash-td-2.html tables/mozilla/marvin/backgr_layers-opacity.html fast/table/relative-position-stacking.html fast/multicol/negativeColumnWidth.html fast/css/nested-layers-with-hover.html fast/repaint/scroll-relative-table-inside-table-cell.html fast/table/relative-position-containment.html fast/repaint/scroll-inside-table-cell.html fast/table/relative-position-offsets.html tables/mozilla_expected_failures/bugs/bug106966.html

Created attachment 136765 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 136729 [details] patch v3.1 - fixed mac warning clearing flags to check the bot results, and implement a request from jamesr on IRc.

Created attachment 137094 [details] (committed r114170, r=jamesr) patch v4 Changes from v3.1: -Fixed chromium reported issues with layout tests. -added a test to check that dynamic changes to the overflow proper properly updates the set. -added change in FrameView::updateScrollableAreaSet to remove itself from its parent when it fails to the following condition: if (!owner || !owner->renderer() || !owner->renderer()->visibleToHitTesting()) previously it was just bailing our earlier.

Comment on attachment 137094 [details] (committed r114170, r=jamesr) patch v4 Looks great, R=me

Committed <http://trac.webkit.org/changeset/114170>