Rename overrideContentLogicalXXX() to overrideContentAndScrollbarLogicalXXX()
Created attachment 403086 [details] Patch
There was a rename for these methods in Blink some time ago, but the solution there was different (instead of changing the name, the method was changed so it actually returned the content size and not the content + scrollbar). Maybe that can be an useful thing to do in WebKit too: https://chromium-review.googlesource.com/c/chromium/src/+/1024171/ A method that returns content size + scrollbar, but doesn't include the border size doesn't look like a very useful thing to me, and quite confusing if you play with borders, scrollbars and so on.
(In reply to Manuel Rego Casasnovas from comment #2) > There was a rename for these methods in Blink some time ago, but the > solution there was different (instead of changing the name, the method was > changed so it actually returned the content size and not the content + > scrollbar). Maybe that can be an useful thing to do in WebKit too: > https://chromium-review.googlesource.com/c/chromium/src/+/1024171/ Nice to know. > A method that returns content size + scrollbar, but doesn't include the > border size doesn't look like a very useful thing to me, and quite confusing > if you play with borders, scrollbars and so on. Well maybe doesn't look useful but it's what it has been doing for years :). The point of this patch was not fixing that, but making it explicit so that we don't commit mistakes while the methods are there.
Comment on attachment 403086 [details] Patch The scrollbar is outside the padding box (see paddingBoxRectIncludingScrollbar), so I'd like a clearer vision of how this relates to contentLogicalWidth/paddingBoxRectIncludingScrollbar.
(In reply to Simon Fraser (smfr) from comment #4) > Comment on attachment 403086 [details] > Patch > > The scrollbar is outside the padding box (see > paddingBoxRectIncludingScrollbar), so I'd like a clearer vision of how this > relates to contentLogicalWidth/paddingBoxRectIncludingScrollbar. Right the situation is pretty convoluted right now as the code is breaking the css box model in the sense that it's storing the content+scrollbar bypassing the padding. I think it'd be better to follow Manuel's advice and revamp the whole override thing.
Comment on attachment 403086 [details] Patch Removing the r?. Handled in another bug.