Bug 79611

Summary: Be more restrictive when adding ScrollableArea's to FrameView's ScrollableArea's map
Product: WebKit Reporter: Antonio Gomes <tonikitoo>
Component: Layout and RenderingAssignee: Antonio Gomes <tonikitoo>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, bdakin, dglazkov, efidler, jamesr, jchaffraix, leo.yang, manyoso, sam, simon.fraser, staikos, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 73144    
Attachments:
Description Flags
patch v1
tonikitoo: commit-queue-
patch v1.1 - fixed style errors
andersca: review-
patch v2
jamesr: review-
patch v3
buildbot: commit-queue-
patch v3.1 - fixed mac warning
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-03
none
(committed r114170, r=jamesr) patch v4 none

Description Antonio Gomes 2012-02-26 18:30:36 PST
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...
Comment 1 Antonio Gomes 2012-02-27 13:26:23 PST
ok, patch coming with tests.
Comment 2 Antonio Gomes 2012-03-11 21:49:31 PDT
Back to it
Comment 3 Antonio Gomes 2012-03-11 21:55:08 PDT
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.
Comment 4 WebKit Review Bot 2012-03-11 21:57:49 PDT
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 5 Antonio Gomes 2012-03-11 22:01:32 PDT
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.
Comment 6 Antonio Gomes 2012-03-12 07:27:06 PDT
(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.
Comment 7 Antonio Gomes 2012-03-12 10:28:27 PDT
Created attachment 131345 [details]
patch v1.1 - fixed style errors
Comment 8 Anders Carlsson 2012-03-12 18:08:12 PDT
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?
Comment 9 Sam Weinig 2012-03-12 18:14:59 PDT
(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.
Comment 10 Antonio Gomes 2012-03-12 20:15:30 PDT

> > 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.
Comment 11 Antonio Gomes 2012-03-12 20:39:24 PDT
> That is exactly why I went with this mode detailed implementation.

s/mode/more/g
Comment 12 Antonio Gomes 2012-03-12 22:02:36 PDT
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.
Comment 13 Antonio Gomes 2012-03-13 15:58:03 PDT
@Anders/Sam: Would you guys have a moment for another review round?
Comment 14 James Robinson 2012-03-18 17:27:47 PDT
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?
Comment 15 Antonio Gomes 2012-04-11 12:04:36 PDT
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.
Comment 16 Antonio Gomes 2012-04-11 12:16:49 PDT
Keeping Julien in the loop as he's been changing related RenderLayer bits.
Comment 17 Build Bot 2012-04-11 12:40:59 PDT
Comment on attachment 136718 [details]
patch v3

Attachment 136718 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12389054
Comment 18 Antonio Gomes 2012-04-11 12:54:53 PDT
Created attachment 136729 [details]
patch v3.1 - fixed mac warning

/Volumes/Data/WebKit/Source/WebCore/testing/Internals.cpp:899: warning: unused parameter 'ec
Comment 19 WebKit Review Bot 2012-04-11 15:18:46 PDT
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
Comment 20 WebKit Review Bot 2012-04-11 15:18:53 PDT
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 21 Antonio Gomes 2012-04-11 22:09:57 PDT
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.
Comment 22 Antonio Gomes 2012-04-13 09:31:43 PDT
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 23 James Robinson 2012-04-13 13:29:01 PDT
Comment on attachment 137094 [details]
(committed r114170, r=jamesr) patch v4

Looks great, R=me
Comment 24 Antonio Gomes 2012-04-13 14:33:43 PDT
Committed <http://trac.webkit.org/changeset/114170>