Bug 182230 - [CSSOM View] Handle the scrollingElement in Element::scroll(Left/Top/Width/Height/To)
Summary: [CSSOM View] Handle the scrollingElement in Element::scroll(Left/Top/Width/He...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords: InRadar
Depends on: 182053 182241 182287 189241
Blocks: 5991 161612 182289 182290
  Show dependency treegraph
 
Reported: 2018-01-28 23:22 PST by Frédéric Wang (:fredw)
Modified: 2018-09-11 03:11 PDT (History)
24 users (show)

See Also:


Attachments
Patch (WIP) (24.35 KB, patch)
2018-01-28 23:29 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
182053+182230 for EWS (46.97 KB, patch)
2018-01-28 23:30 PST, Frédéric Wang (:fredw)
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.03 MB, application/zip)
2018-01-29 00:44 PST, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.51 MB, application/zip)
2018-01-29 01:07 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-sierra (3.34 MB, application/zip)
2018-01-29 01:24 PST, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-sierra (2.70 MB, application/zip)
2018-01-29 02:32 PST, Build Bot
no flags Details
Patch (182053+182230+182241) for EWS (83.18 KB, patch)
2018-01-30 03:23 PST, Frédéric Wang (:fredw)
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.17 MB, application/zip)
2018-01-30 05:02 PST, Build Bot
no flags Details
Patch (47.89 KB, patch)
2018-01-30 08:00 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (182053+182287+182230) for EWS (69.95 KB, patch)
2018-02-01 01:27 PST, Frédéric Wang (:fredw)
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.61 MB, application/zip)
2018-02-01 02:49 PST, Build Bot
no flags Details
Patch (48.06 KB, patch)
2018-02-01 07:00 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (applies on top of 182053) (48.09 KB, patch)
2018-05-10 02:06 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (applies on top of 182053) (48.12 KB, patch)
2018-07-10 07:42 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (applies on top of 182053) (48.14 KB, patch)
2018-08-30 08:10 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (48.15 KB, patch)
2018-08-31 00:52 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.51 MB, application/zip)
2018-08-31 01:55 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.08 MB, application/zip)
2018-08-31 02:31 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-sierra (3.13 MB, application/zip)
2018-08-31 02:42 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.28 MB, application/zip)
2018-08-31 02:54 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.27 MB, application/zip)
2018-08-31 04:58 PDT, Build Bot
no flags Details
Patch (50.75 KB, patch)
2018-09-03 06:50 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (50.75 KB, patch)
2018-09-03 07:59 PDT, Frédéric Wang (:fredw)
simon.fraser: review-
Details | Formatted Diff | Diff
Patch (50.09 KB, patch)
2018-09-07 00:40 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (55.37 KB, patch)
2018-09-07 11:51 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (55.40 KB, patch)
2018-09-07 12:11 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (follow-up fix) (2.39 KB, patch)
2018-09-10 08:59 PDT, Frédéric Wang (:fredw)
tonikitoo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2018-01-28 23:22:44 PST
.
Comment 1 Frédéric Wang (:fredw) 2018-01-28 23:29:13 PST
Created attachment 332512 [details]
Patch (WIP)
Comment 2 Frédéric Wang (:fredw) 2018-01-28 23:30:11 PST
Created attachment 332513 [details]
182053+182230 for EWS
Comment 3 Build Bot 2018-01-28 23:31:54 PST
Attachment 332513 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Element.cpp:56:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Build Bot 2018-01-29 00:44:24 PST
Comment on attachment 332513 [details]
182053+182230 for EWS

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

New failing tests:
tiled-drawing/scrolling/frames/fixed-inside-frame.html
tiled-drawing/scrolling/fast-scroll-mainframe-zoom.html
http/tests/navigation/anchor-frames.html
fast/dom/window-scroll-scaling.html
fast/multicol/scrolling-overflow.html
fast/dom/Element/body-scrollLeft.html
fast/dom/Element/documentElement-scrollTop.html
http/tests/navigation/anchor-frames-gbk.html
imported/w3c/web-platform-tests/html/browsers/browsing-the-web/scroll-to-fragid/003.html
fast/dom/Element/scrollTop.html
fast/scrolling/latching/scroll-div-no-latching.html
fast/dom/Element/body-scrollTop.html
fast/events/scroll-in-scaled-page-with-overflow-hidden.html
fast/dom/Element/scrollLeft.html
tiled-drawing/tiled-drawing-scroll-position-page-cache-restoration.html
fast/scrolling/latching/scroll-nested-iframe.html
fast/dom/Element/scrolling-funtions-on-body.html
tiled-drawing/scrolling/iframe_in_iframe.html
fast/dom/Element/documentElement-scrollLeft.html
fast/scrolling/latching/iframe_in_iframe.html
fast/scrolling/latching/scroll-latched-nested-div.html
Comment 5 Build Bot 2018-01-29 00:44:25 PST
Created attachment 332516 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 6 Build Bot 2018-01-29 01:07:35 PST
Comment on attachment 332513 [details]
182053+182230 for EWS

Attachment 332513 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6246462

New failing tests:
fast/dom/Element/scrollLeft.html
fast/multicol/scrolling-overflow.html
fast/dom/Element/body-scrollLeft.html
platform/ios/ios/fast/coordinates/page-offsets.html
fast/dom/Element/documentElement-scrollTop.html
fast/dom/Element/scrollTop.html
fast/dom/Element/body-scrollTop.html
fast/dom/Element/scrolling-funtions-on-body.html
imported/w3c/web-platform-tests/cssom-view/scrolling-quirks-vs-nonquirks.html
imported/w3c/web-platform-tests/html/browsers/browsing-the-web/scroll-to-fragid/003.html
fast/dom/Element/documentElement-scrollLeft.html
Comment 7 Build Bot 2018-01-29 01:07:37 PST
Created attachment 332519 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 8 Build Bot 2018-01-29 01:24:53 PST
Comment on attachment 332513 [details]
182053+182230 for EWS

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

New failing tests:
http/tests/navigation/anchor-frames-same-origin.html
http/tests/navigation/anchor-frames.html
fast/dom/window-scroll-scaling.html
fast/multicol/scrolling-overflow.html
fast/dom/Element/body-scrollLeft.html
fast/dom/Element/documentElement-scrollTop.html
http/tests/navigation/anchor-frames-gbk.html
imported/w3c/web-platform-tests/html/browsers/browsing-the-web/scroll-to-fragid/003.html
fast/scrolling/latching/scroll-div-no-latching.html
fast/dom/Element/body-scrollTop.html
fast/events/scroll-in-scaled-page-with-overflow-hidden.html
fast/dom/Element/scrollLeft.html
fast/scrolling/latching/scroll-nested-iframe.html
fast/dom/Element/scrolling-funtions-on-body.html
fast/dom/Element/scrollTop.html
fast/dom/Element/documentElement-scrollLeft.html
fast/scrolling/latching/iframe_in_iframe.html
fast/scrolling/latching/scroll-latched-nested-div.html
Comment 9 Build Bot 2018-01-29 01:24:55 PST
Created attachment 332520 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 10 Build Bot 2018-01-29 02:32:08 PST
Comment on attachment 332513 [details]
182053+182230 for EWS

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

New failing tests:
http/tests/navigation/anchor-frames-same-origin.html
http/tests/navigation/anchor-frames.html
fast/dom/window-scroll-scaling.html
fast/multicol/scrolling-overflow.html
fast/dom/Element/body-scrollLeft.html
fast/dom/Element/documentElement-scrollTop.html
http/tests/navigation/anchor-frames-gbk.html
imported/w3c/web-platform-tests/html/browsers/browsing-the-web/scroll-to-fragid/003.html
fast/scrolling/latching/scroll-div-no-latching.html
fast/dom/Element/body-scrollTop.html
fast/events/scroll-in-scaled-page-with-overflow-hidden.html
fast/dom/Element/scrollLeft.html
fast/scrolling/latching/scroll-nested-iframe.html
fast/dom/Element/scrolling-funtions-on-body.html
fast/dom/Element/scrollTop.html
fast/dom/Element/documentElement-scrollLeft.html
fast/scrolling/latching/iframe_in_iframe.html
fast/scrolling/latching/scroll-latched-nested-div.html
Comment 11 Build Bot 2018-01-29 02:32:09 PST
Created attachment 332529 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 12 Frédéric Wang (:fredw) 2018-01-30 03:23:44 PST
Created attachment 332645 [details]
Patch (182053+182230+182241) for EWS
Comment 13 Build Bot 2018-01-30 05:02:45 PST
Comment on attachment 332645 [details]
Patch (182053+182230+182241) for EWS

Attachment 332645 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6259872

New failing tests:
imported/w3c/web-platform-tests/cssom-view/scrolling-quirks-vs-nonquirks.html
Comment 14 Build Bot 2018-01-30 05:02:46 PST
Created attachment 332650 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 15 Frédéric Wang (:fredw) 2018-01-30 08:00:28 PST
Created attachment 332653 [details]
Patch

This patch applies after the dependencies.
Comment 16 Frédéric Wang (:fredw) 2018-01-31 03:13:11 PST
Comment on attachment 332653 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332653&action=review

> LayoutTests/fast/events/scroll-in-scaled-page-with-overflow-hidden.html:21
> +        internals.settings.setCSSOMViewScrollingAPIEnabled(false);

This does not work well. I handled it better in bug 182287 instead.
Comment 17 Frédéric Wang (:fredw) 2018-02-01 01:27:47 PST
Created attachment 332857 [details]
Patch (182053+182287+182230) for EWS

Rebasing over bug 182287.
Comment 18 Build Bot 2018-02-01 02:49:00 PST
Comment on attachment 332857 [details]
Patch (182053+182287+182230) for EWS

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

New failing tests:
tiled-drawing/scrolling/fast-scroll-div-latched-mainframe.html
Comment 19 Build Bot 2018-02-01 02:49:02 PST
Created attachment 332864 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 20 Frédéric Wang (:fredw) 2018-02-01 07:00:46 PST
Created attachment 332874 [details]
Patch
Comment 21 Frédéric Wang (:fredw) 2018-05-10 02:06:18 PDT
Created attachment 340079 [details]
Patch (applies on top of 182053)

Rebasing...
Comment 22 Frédéric Wang (:fredw) 2018-07-10 07:42:57 PDT
Created attachment 344695 [details]
Patch (applies on top of 182053)

Rebasing...
Comment 23 Frédéric Wang (:fredw) 2018-08-30 08:10:47 PDT
Created attachment 348498 [details]
Patch (applies on top of 182053)

Rebase...
Comment 24 Frédéric Wang (:fredw) 2018-08-31 00:52:36 PDT
Created attachment 348619 [details]
Patch

Rebasing...
Comment 25 Build Bot 2018-08-31 01:55:25 PDT
Comment on attachment 348619 [details]
Patch

Attachment 348619 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9047905

New failing tests:
imported/w3c/web-platform-tests/css/cssom-view/HTMLBody-ScrollArea_quirksmode.html
imported/w3c/web-platform-tests/css/cssom-view/scrollingElement.html
imported/w3c/web-platform-tests/css/cssom-view/scrolling-quirks-vs-nonquirks.html
Comment 26 Build Bot 2018-08-31 01:55:27 PDT
Created attachment 348621 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 27 Build Bot 2018-08-31 02:31:26 PDT
Comment on attachment 348619 [details]
Patch

Attachment 348619 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9048026

New failing tests:
imported/w3c/web-platform-tests/css/cssom-view/scrollingElement.html
imported/w3c/web-platform-tests/css/cssom-view/HTMLBody-ScrollArea_quirksmode.html
tiled-drawing/scrolling/fast-scroll-div-latched-mainframe.html
imported/w3c/web-platform-tests/css/cssom-view/scrolling-quirks-vs-nonquirks.html
Comment 28 Build Bot 2018-08-31 02:31:28 PDT
Created attachment 348623 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 29 Build Bot 2018-08-31 02:42:57 PDT
Comment on attachment 348619 [details]
Patch

Attachment 348619 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9047989

New failing tests:
imported/w3c/web-platform-tests/css/cssom-view/HTMLBody-ScrollArea_quirksmode.html
imported/w3c/web-platform-tests/css/cssom-view/scrollingElement.html
imported/w3c/web-platform-tests/css/cssom-view/scrolling-quirks-vs-nonquirks.html
Comment 30 Build Bot 2018-08-31 02:42:59 PDT
Created attachment 348625 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 31 Build Bot 2018-08-31 02:54:17 PDT
Comment on attachment 348619 [details]
Patch

Attachment 348619 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9048008

New failing tests:
imported/w3c/web-platform-tests/css/cssom-view/HTMLBody-ScrollArea_quirksmode.html
imported/w3c/web-platform-tests/css/cssom-view/scrollingElement.html
imported/w3c/web-platform-tests/css/cssom-view/scrolling-quirks-vs-nonquirks.html
Comment 32 Build Bot 2018-08-31 02:54:19 PDT
Created attachment 348627 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 33 Build Bot 2018-08-31 04:58:40 PDT
Comment on attachment 348619 [details]
Patch

Attachment 348619 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9048802

New failing tests:
imported/w3c/web-platform-tests/css/cssom-view/HTMLBody-ScrollArea_quirksmode.html
imported/w3c/web-platform-tests/css/cssom-view/scrollingElement.html
imported/w3c/web-platform-tests/css/cssom-view/scrolling-quirks-vs-nonquirks.html
Comment 34 Build Bot 2018-08-31 04:58:42 PDT
Created attachment 348634 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 35 Frédéric Wang (:fredw) 2018-09-03 03:48:54 PDT
The new failures are due to the fact that the cssom-view test suite has been moved into wpt/css (I did not notice it because the old version is still present). See bug 189241.
Comment 36 Frédéric Wang (:fredw) 2018-09-03 06:50:15 PDT
Created attachment 348769 [details]
Patch
Comment 37 Frédéric Wang (:fredw) 2018-09-03 07:57:48 PDT
Comment on attachment 348769 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348769&action=review

> LayoutTests/tiled-drawing/scrolling/fast-scroll-div-latched-mainframe.html:53
> +    var pageScrollPositionAfter = document.scrollingELement.scrollTop;

oops, this should be s/scrollingELement/scrollingElement/
Comment 38 Frédéric Wang (:fredw) 2018-09-03 07:59:00 PDT
Created attachment 348772 [details]
Patch
Comment 39 Simon Fraser (smfr) 2018-09-06 10:59:39 PDT
Comment on attachment 348772 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348772&action=review

> Source/WebCore/dom/Document.cpp:1466
> +    if (settings().CSSOMViewScrollingAPIEnabled() && inQuirksMode())

inQuirksModeI() is the more efficient check: put it first.

> Source/WebCore/dom/Document.h:453
>      WEBCORE_EXPORT Element* scrollingElement();
> +    // When calling from C++ code, use this method. scrollingElement() is just for the web IDL implementation.
> +    Element* scrollingElementNoLayout();

It might be clearer to flip the naming here; rename scrollingElement() to scrollingElementForAPI or scrollingElementUpdatingLayout() or something, and make the no-layout version just be scrollingElement().

> Source/WebCore/dom/Element.cpp:817
> +static int adjustForZoom(int value, const Frame& frame)

This should say what is being adjusted. What is 'value'? Is it a scroll position or offset?

> Source/WebCore/dom/Element.cpp:824
> +    // Needed because of truncation (rather than rounding) when scaling up.
> +    if (zoomFactor > 1)
> +        value++;

Weird. Why not just floor/ceil after the division by zoomFactor?

> Source/WebCore/dom/Element.cpp:979
> +        RefPtr<Frame> frame = document().frame();
> +        if (!frame)
> +            return 0;
> +        RefPtr<FrameView> view = frame->view();
> +        if (!view)
> +            return 0;

This appears 6 times. Share it.
Comment 40 Frédéric Wang (:fredw) 2018-09-06 11:26:17 PDT
(In reply to Simon Fraser (smfr) from comment #39)
> inQuirksModeI() is the more efficient check: put it first.
> 
> It might be clearer to flip the naming here; rename scrollingElement() to
> scrollingElementForAPI or scrollingElementUpdatingLayout() or something, and
> make the no-layout version just be scrollingElement().

Good points, I'll do it.

> This should say what is being adjusted. What is 'value'? Is it a scroll
> position or offset?
>
> Weird. Why not just floor/ceil after the division by zoomFactor?
> 
> This appears 6 times. Share it.

Note that I only moved existing code from HTMLBodyElement.cpp. Maybe these changes should be handled separately to make them clearer in the history.
Comment 41 Simon Fraser (smfr) 2018-09-06 12:38:18 PDT
(In reply to Frédéric Wang (:fredw) from comment #40)
> (In reply to Simon Fraser (smfr) from comment #39)
> > inQuirksModeI() is the more efficient check: put it first.
> > 
> > It might be clearer to flip the naming here; rename scrollingElement() to
> > scrollingElementForAPI or scrollingElementUpdatingLayout() or something, and
> > make the no-layout version just be scrollingElement().
> 
> Good points, I'll do it.
> 
> > This should say what is being adjusted. What is 'value'? Is it a scroll
> > position or offset?
> >
> > Weird. Why not just floor/ceil after the division by zoomFactor?
> > 
> > This appears 6 times. Share it.
> 
> Note that I only moved existing code from HTMLBodyElement.cpp. Maybe these
> changes should be handled separately to make them clearer in the history.

I think it's OK to do a refactor when moving code.
Comment 42 Frédéric Wang (:fredw) 2018-09-06 23:32:40 PDT
Comment on attachment 348772 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348772&action=review

>> Source/WebCore/dom/Element.cpp:817
>> +static int adjustForZoom(int value, const Frame& frame)
> 
> This should say what is being adjusted. What is 'value'? Is it a scroll position or offset?

The current code uses this for both offset and size.

>> Source/WebCore/dom/Element.cpp:824
>> +        value++;
> 
> Weird. Why not just floor/ceil after the division by zoomFactor?

I've opened bug 189397 for this one as there are several zooming functions with different implementation. I prefer not risk unrelated behavior changes in this bug.

>> Source/WebCore/dom/Element.cpp:979
>> +            return 0;
> 
> This appears 6 times. Share it.

I can do that one in this bug.
Comment 43 Frédéric Wang (:fredw) 2018-09-07 00:40:29 PDT
Created attachment 349118 [details]
Patch
Comment 44 Simon Fraser (smfr) 2018-09-07 11:17:14 PDT
Comment on attachment 349118 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349118&action=review

Makes sure it builds!

> Source/WebCore/dom/Element.cpp:861
> +static int adjustForZoom(int value, const Frame& frame)

Still needs a better name.

> Source/WebCore/dom/Element.cpp:866
> +    // FIXME(https://webkit.org/b/189397): Why can't we just ceil/floor?

Space after FIXME, and you can just say webkit.org/b/189397.
Comment 45 Frédéric Wang (:fredw) 2018-09-07 11:51:16 PDT
Created attachment 349170 [details]
Patch
Comment 46 Build Bot 2018-09-07 11:54:08 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 47 Frédéric Wang (:fredw) 2018-09-07 12:11:21 PDT
Created attachment 349174 [details]
Patch
Comment 48 WebKit Commit Bot 2018-09-07 14:12:50 PDT
Comment on attachment 349174 [details]
Patch

Clearing flags on attachment: 349174

Committed r235806: <https://trac.webkit.org/changeset/235806>
Comment 49 WebKit Commit Bot 2018-09-07 14:12:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 50 Radar WebKit Bug Importer 2018-09-07 14:13:57 PDT
<rdar://problem/44237945>
Comment 51 Truitt Savell 2018-09-10 08:27:11 PDT
Looks like this patch https://trac.webkit.org/changeset/235806/webkit

has caused https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=tiled-drawing%2Fscrolling%2Ffast-scroll-iframe-latched-mainframe.html

to become flakey timeout. 

Test History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=tiled-drawing%2Fscrolling%2Ffast-scroll-iframe-latched-mainframe.html

I reproduced this flakiness using command:

run-webkit-tests tiled-drawing/scrolling/fast-scroll-iframe-latched-mainframe.html --iterations 500 -f

on a build of 235805 and no timeouts occurred. running this on 235806 and the failure reproduces easily.
Comment 52 Frédéric Wang (:fredw) 2018-09-10 08:43:04 PDT
(In reply to Truitt Savell from comment #51)
> Looks like this patch https://trac.webkit.org/changeset/235806/webkit

Thanks, I wonder whether it's related to the current implementation of Document::isBodyPotentiallyScrollable (see bug 182292).

I guess a workaround for now would be to use documentElement instead of scrollingElement in LayoutTests/tiled-drawing/scrolling/fast-scroll-div-latched-mainframe.html
Comment 53 Frédéric Wang (:fredw) 2018-09-10 08:59:31 PDT
Created attachment 349311 [details]
Patch (follow-up fix)

(In reply to Frédéric Wang (:fredw) from comment #52)
> (In reply to Truitt Savell from comment #51)
> > Looks like this patch https://trac.webkit.org/changeset/235806/webkit
> 
> Thanks, I wonder whether it's related to the current implementation of
> Document::isBodyPotentiallyScrollable (see bug 182292).
> 
> I guess a workaround for now would be to use documentElement instead of
> scrollingElement in
> LayoutTests/tiled-drawing/scrolling/fast-scroll-div-latched-mainframe.html

I stand corrected. I thought the issue was with tiled-drawing/scrolling/fast-scroll-div-latched-mainframe.html...
For fast-scroll-iframe-latched-mainframe.html, just replacing document.body with document.scrollingElement avoids the flackiness for me.
Comment 54 Frédéric Wang (:fredw) 2018-09-10 12:39:17 PDT
Committed r235855: <https://trac.webkit.org/changeset/235855>
Comment 55 Truitt Savell 2018-09-10 13:22:59 PDT
Looks like there were a total of 3 tests effected by this change all with similar names. 

tiled-drawing/scrolling/fast-scroll-iframe-latched-mainframe-with-handler.html 
tiled-drawing/scrolling/fast-scroll-select-latched-mainframe.html 
tiled-drawing/scrolling/fast-scroll-iframe-latched-mainframe.html

Combined History:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=tiled-drawing%2Fscrolling%2Ffast-scroll-iframe-latched-mainframe-with-handler.html%20%20tiled-drawing%2Fscrolling%2Ffast-scroll-select-latched-mainframe.html%20%20tiled-drawing%2Fscrolling%2Ffast-scroll-iframe-latched-mainframe.html
Comment 56 Truitt Savell 2018-09-10 13:49:51 PDT
Another Timeout:
tiled-drawing/scrolling/fast-scroll-div-latched-mainframe.html

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=tiled-drawing%2Fscrolling%2Ffast-scroll-div-latched-mainframe-with-handler.html
Comment 57 Frédéric Wang (:fredw) 2018-09-10 22:28:13 PDT
(In reply to Truitt Savell from comment #56)
> Another Timeout:
> tiled-drawing/scrolling/fast-scroll-div-latched-mainframe.html
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=tiled-drawing%2Fscrolling%2Ffast-scroll-div-
> latched-mainframe-with-handler.html

This is the history tiled-drawing/scrolling/fast-scroll-div-latched-mainframe-with-handler.html ; tiled-drawing/scrolling/fast-scroll-div-latched-mainframe.html had already been fixed in the initial patch and does not look flaky.

(In reply to Truitt Savell from comment #55)
> tiled-drawing/scrolling/fast-scroll-iframe-latched-mainframe-with-handler.
> html 
> tiled-drawing/scrolling/fast-scroll-select-latched-mainframe.html 
> tiled-drawing/scrolling/fast-scroll-iframe-latched-mainframe.html

OK, actually it seems a bunch of new tests using document.body instead of document.scrolingElement have been added since the patch for bug 182241 landed. I'll take care of it.
Comment 58 Frédéric Wang (:fredw) 2018-09-11 03:11:28 PDT
(In reply to Frédéric Wang (:fredw) from comment #57)
> OK, actually it seems a bunch of new tests using document.body instead of
> document.scrolingElement have been added since the patch for bug 182241
> landed. I'll take care of it.

Hopefully, bug 189495 will address the remaining flaky results.