Created attachment 334454 [details] Patch

Comment on attachment 334454 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334454&action=review @Simon, Daniel: This patch just extracts the scrollOffset from offsetFromRender, but I'm not sure it is what was meant in the FIXME nor whether it's really an improvement... WDYT? > Source/WebCore/rendering/RenderLayerBacking.cpp:2471 > // but the repaint rect is computed without taking the scroll position into account (see shouldApplyClipAndScrollPositionForRepaint()). I'm a bit confused by this comment. The first line (from Daniel, bug 125239) seems to say it is done because scrollOffset is included in the offsetFromRenderer in RenderLayerBacking::updateGeometry(), which actually happens on non-iOS platforms too and what I'm trying to remove here. The second line (from Simon, bug 159186) seems to say it is done because scrollPosition in applyCachedClipAndScrollPositionForRepaint (rather than shouldApplyClipAndScrollPositionForRepaint). So it does not seem we can get rid of this...

Created attachment 340071 [details] Patch Rebasing

Created attachment 348912 [details] Patch

Created attachment 354929 [details] Patch Rebasing...

Comment on attachment 354929 [details] Patch Attachment 354929 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10004230 New failing tests: compositing/overflow/scrolling-without-painting.html compositing/overflow/textarea-scroll-touch.html

Created attachment 354943 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6

Comment on attachment 354929 [details] Patch Attachment 354929 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10005225 New failing tests: compositing/overflow/textarea-scroll-touch.html compositing/overflow/scrolling-without-painting.html

Created attachment 354956 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6

Created attachment 355336 [details] Patch

Created attachment 355474 [details] Patch

@smfr: review ping?

Comment on attachment 355474 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355474&action=review > Source/WebCore/ChangeLog:8 > + Reviewed by NOBODY (OOPS!). > + > + No new tests, already covered by existing tests. Can you explain here why the scroll offset needs to be store separately from the paint offset?

Comment on attachment 355474 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355474&action=review >> Source/WebCore/ChangeLog:8 >> + No new tests, already covered by existing tests. > > Can you explain here why the scroll offset needs to be store separately from the paint offset? I'm not exactly sure why it needs to be that way (seem comment 2) but I see it makes the API cleaner and maybe help to optimize re-display. Can you please explain the rationale of this FIXME?

View in context: https://bugs.webkit.org/attachment.cgi?id=355474&action=review >> Source/WebCore/ChangeLog:8 >> + No new tests, already covered by existing tests. > > Can you explain here why the scroll offset needs to be store separately from the paint offset? I'm not exactly sure why it needs to be that way (seem comment 2) but I see it makes the API cleaner and maybe help to optimize re-display. Can you please explain the rationale of this FIXME?

Comment on attachment 355474 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355474&action=review >>> Source/WebCore/ChangeLog:8 >>> + No new tests, already covered by existing tests. >> >> Can you explain here why the scroll offset needs to be store separately from the paint offset? > > I'm not exactly sure why it needs to be that way (seem comment 2) but I see it makes the API cleaner and maybe help to optimize re-display. Can you please explain the rationale of this FIXME? Oh ha, that was my fixme. I mean, it makes sense, but do we need to do it to avoid repaints on composited scroll, for example?

Comment on attachment 355474 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355474&action=review >>>> Source/WebCore/ChangeLog:8 >>>> + No new tests, already covered by existing tests. >>> >>> Can you explain here why the scroll offset needs to be store separately from the paint offset? >> >> I'm not exactly sure why it needs to be that way (seem comment 2) but I see it makes the API cleaner and maybe help to optimize re-display. Can you please explain the rationale of this FIXME? > > Oh ha, that was my fixme. I mean, it makes sense, but do we need to do it to avoid repaints on composited scroll, for example? That was what I mean by optimizing redisplay but I'm not sure at all to be honest... As a reminder, this originates from the discussion about iOS frame scrolling on 12/02/2018: 23:21:32 - fredw: I also tried to copy the stuff with offsetFromRenderer, but again, I'm not sure about that 23:21:34 - smfr: right, that's to match how UIScrollVIew works 23:21:41 - smfr: don't use offsetFromRenderer! it's special 23:21:55 - smfr: it's to size layers to take account of borders, shadows etc 23:22:18 - fredw: mmh, but I think it was used for more... 23:23:00 - smfr: maybe you could draw some ASCII art in a bug to show how the web process layer tree maps into UIScrollVIews for a subframe, across the frame boundary 23:23:11 - smfr: it's critical to get that part right 23:24:44 - fredw: This is only taking padding for m_scrollingLayer: https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/rendering/RenderLayerBacking.cpp#L1140 23:25:13 - fredw: but this is taking offset into account for the content layer: https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/rendering/RenderLayerBacking.cpp#L1163 23:25:42 - smfr: "FIXME: The paint offset and the scroll offset should really be separate concepts." yes 23:26:44 - fredw: So for now I just tried to copy the logic of overflow node, but maybe that was not a good idea... 23:27:04 - smfr: i think it will be different in some ways 23:27:54 - fredw: I think I just ignored the padding stuff in this code, since it is taken into account somewhere else 23:28:20 - smfr: be sure to test content with iframes which have padding, thick borders and box shadows 23:28:40 - fredw: Yes, I already did did that in my patch. Not box shadows, though 23:30:08 - fredw: So do you think I should keep "setBoundsOrigin" like for overflow node or try to use "setPosition" instead? 23:30:29 - smfr: you'll have to use bounds origin since that's what UIScrollView uses 23:30:44 - smfr: maybe we should try to get that distinction out of the WebCore codeethough 23:30:53 - smfr: would be nice to hide that difference in the scrolling tree for WK2 23:31:01 - smfr: harder with iOS WK1 23:31:46 - fredw: and what about these offsetFromRenderer's? 23:32:49 - fredw: IIUC, I need it unless i do the FIXME you mentioned 23:32:52 - smfr: we should address the FIXME :) 23:33:09 - smfr: i'd have to study more to see what that involves 23:33:37 - fredw: OK, maybe I could do that for overflow node first too. Although, I don't really understand it :-) 23:34:10 - fredw: it seems it's not specific to iOS, though, there is another one in the #ekse 23:34:18 - smfr: so, for normal layers, offsetFromRenderer is the offset between the top left of the GraphicsLayer (the backing store) to the top left of the renderer's border box 23:34:36 - smfr: basically if you add box-shadow or something, offsetFromRenderer accounts for the extra space you need

Created attachment 356130 [details] Patch for landing

The commit-queue encountered the following flaky tests while processing attachment 356130 [details]: workers/bomb.html bug 171985 (author: fpizlo@apple.com) webgl/2.0.0/conformance/more/functions/vertexAttribPointerBadArgs.html bug 192218 (author: justin_fan@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 356130 [details] Patch for landing Clearing flags on attachment: 356130 Committed r238727: <https://trac.webkit.org/changeset/238727>

All reviewed patches have been landed. Closing bug.

<rdar://problem/46365397>