Be more restrictive when adding ScrollableArea's to FrameView's ScrollableArea's map

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Antonio Gomes]

We add all FrameView's at the time they get added to the FrameView thee, and all RenderLayer's whose associated RenderBox has been set as "has overflow clip", no matter if it has a scrollable area de-facto or not.

Lets fix it...

[Antonio Gomes]

ok, patch coming with tests.

[Antonio Gomes]

Back to it

[Antonio Gomes]

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.

[WebKit Review Bot]

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.

[Antonio Gomes]

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.

[Antonio Gomes]

(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.

[Antonio Gomes]

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

[Anders Carlsson]

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?

[Sam Weinig]

(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.

[Antonio Gomes]

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

[Antonio Gomes]

> That is exactly why I went with this mode detailed implementation.

s/mode/more/g

[Antonio Gomes]

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.

[Antonio Gomes]

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

[James Robinson]

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?

[Antonio Gomes]

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.

[Antonio Gomes]

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

[Build Bot]

Comment on attachment 136718 [details]
patch v3

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

[Antonio Gomes]

Created attachment 136729 [details]
patch v3.1 - fixed mac warning

/Volumes/Data/WebKit/Source/WebCore/testing/Internals.cpp:899: warning: unused parameter 'ec

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Antonio Gomes]

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.

[Antonio Gomes]

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.

[James Robinson]

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

Looks great, R=me

[Antonio Gomes]

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