Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1163913002:
Improve outline bounds estimation in InlinePainter (Closed)
|
Created:
5 years, 6 months ago by fs Modified:
5 years, 6 months ago Reviewers:
chrishtr CC:
blink-reviews, blink-reviews-paint_chromium.org, dshwang, slimming-paint-reviews_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionImprove outline bounds estimation in InlinePainter
Use LayoutInline::linesVisualOverflowBoundingBox to get bounds that
includes the outline of the inline.
TEST=fast/css/focus-ring-outline-color.html (S.P. strict cull rect)
BUG=495368
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196722
Patch Set 1 #Patch Set 2 : Use linesVisualOverflowBoundingBox. #
Total comments: 2
Patch Set 3 : Don't double-inflate. #Patch Set 4 : Fix continuation outlines. #Messages
Total messages: 20 (2 generated)
fs@opera.com changed reviewers: + chrishtr@chromium.org
Ping?
Should we be using linesVisualOverflowBoundingBox() instead?
On 2015/06/03 17:20:44, chrishtr wrote: > Should we be using linesVisualOverflowBoundingBox() instead? Based on LayoutInline::clippedOverflowRect it doesn't look that way.
On 2015/06/03 at 21:00:08, fs wrote: > On 2015/06/03 17:20:44, chrishtr wrote: > > Should we be using linesVisualOverflowBoundingBox() instead? > > Based on LayoutInline::clippedOverflowRect it doesn't look that way. That function does use linesVisualOverflowBoundingBox right? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
On 2015/06/03 21:04:00, chrishtr wrote: > On 2015/06/03 at 21:00:08, fs wrote: > > On 2015/06/03 17:20:44, chrishtr wrote: > > > Should we be using linesVisualOverflowBoundingBox() instead? > > > > Based on LayoutInline::clippedOverflowRect it doesn't look that way. > > That function does use linesVisualOverflowBoundingBox right? > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Yes, which it inflates with outlineSize. Looks kinda expensive too, considering we've already collected the focus/outline rects.
On 2015/06/03 at 21:10:45, fs wrote: > On 2015/06/03 21:04:00, chrishtr wrote: > > On 2015/06/03 at 21:00:08, fs wrote: > > > On 2015/06/03 17:20:44, chrishtr wrote: > > > > Should we be using linesVisualOverflowBoundingBox() instead? > > > > > > Based on LayoutInline::clippedOverflowRect it doesn't look that way. > > > > That function does use linesVisualOverflowBoundingBox right? > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Yes, which it inflates with outlineSize. Looks kinda expensive too, considering we've already collected the focus/outline rects. So performance is the reason not do that? If so it would be good to look at storing the visual overflow rect on the LayoutInline. Not sure why we don't do that.
On 2015/06/03 22:22:34, chrishtr wrote: > On 2015/06/03 at 21:10:45, fs wrote: > > On 2015/06/03 21:04:00, chrishtr wrote: > > > On 2015/06/03 at 21:00:08, fs wrote: > > > > On 2015/06/03 17:20:44, chrishtr wrote: > > > > > Should we be using linesVisualOverflowBoundingBox() instead? > > > > > > > > Based on LayoutInline::clippedOverflowRect it doesn't look that way. > > > > > > That function does use linesVisualOverflowBoundingBox right? > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > Yes, which it inflates with outlineSize. Looks kinda expensive too, > considering we've already collected the focus/outline rects. > > So performance is the reason not do that? If so it would be good to look at > storing the visual overflow rect on the LayoutInline. Not sure why we don't do > that. Switched to using linesVisualOverflowBoundingBox(). Still feels like overkill to me given that the drawing ops here seem pretty well-defined in terms of bounds.
https://codereview.chromium.org/1163913002/diff/20001/Source/core/paint/Inlin... File Source/core/paint/InlinePainter.cpp (right): https://codereview.chromium.org/1163913002/diff/20001/Source/core/paint/Inlin... Source/core/paint/InlinePainter.cpp:35: int outlineOutset; Why does linesVisualOverflowBoundingBox() not already include the bounding box? If not then this seems to imply to me that this code is wrong also, since it should include outline: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... So include outline in linesVisualOverflowBoundingBox()?
https://codereview.chromium.org/1163913002/diff/20001/Source/core/paint/Inlin... File Source/core/paint/InlinePainter.cpp (right): https://codereview.chromium.org/1163913002/diff/20001/Source/core/paint/Inlin... Source/core/paint/InlinePainter.cpp:35: int outlineOutset; On 2015/06/04 17:08:12, chrishtr wrote: > Why does linesVisualOverflowBoundingBox() not already include the bounding box? > If not then this seems to imply to me that this code is wrong also, since it > should include outline: > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > So include outline in linesVisualOverflowBoundingBox()? Hmm, yes... the inflate(outlineSize) in LayoutInline::clippedOverflowRect confused me - now it looks rather redundant to me.
On 2015/06/05 at 10:19:53, fs wrote: > https://codereview.chromium.org/1163913002/diff/20001/Source/core/paint/Inlin... > File Source/core/paint/InlinePainter.cpp (right): > > https://codereview.chromium.org/1163913002/diff/20001/Source/core/paint/Inlin... > Source/core/paint/InlinePainter.cpp:35: int outlineOutset; > On 2015/06/04 17:08:12, chrishtr wrote: > > Why does linesVisualOverflowBoundingBox() not already include the bounding box? > > If not then this seems to imply to me that this code is wrong also, since it > > should include outline: > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > So include outline in linesVisualOverflowBoundingBox()? > > Hmm, yes... the inflate(outlineSize) in LayoutInline::clippedOverflowRect confused me - now it looks rather redundant to me. clippedOverflowRect is not what you're calling here. Should the outline code in clippedOverflowRect be moved into linesVisualOverflowBoundingBox?
On 2015/06/05 16:41:15, chrishtr wrote: > On 2015/06/05 at 10:19:53, fs wrote: > > > https://codereview.chromium.org/1163913002/diff/20001/Source/core/paint/Inlin... > > File Source/core/paint/InlinePainter.cpp (right): > > > > > https://codereview.chromium.org/1163913002/diff/20001/Source/core/paint/Inlin... > > Source/core/paint/InlinePainter.cpp:35: int outlineOutset; > > On 2015/06/04 17:08:12, chrishtr wrote: > > > Why does linesVisualOverflowBoundingBox() not already include the bounding > box? > > > If not then this seems to imply to me that this code is wrong also, since it > > > should include outline: > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > So include outline in linesVisualOverflowBoundingBox()? > > > > Hmm, yes... the inflate(outlineSize) in LayoutInline::clippedOverflowRect > confused me - now it looks rather redundant to me. > > clippedOverflowRect is not what you're calling here. Should the outline code in > clippedOverflowRect be moved into linesVisualOverflowBoundingBox? I was initially looking at clippedOverflowRect (or rather clippedOverflowRectForPaintInvalidation, but that just calls the other one) to see how the paint inv. rect was computed - and it had that inflation - which is why I included that in PS2 (dropped in PS3). Looking closer though wondered if that inflate() didn't cause the outline to be added twice, i.e. it should rather be something like: https://codereview.chromium.org/1164533005/ - either that or move it into linesVisualOverflowBoundingBox as you suggest.
On 2015/06/05 at 16:54:46, fs wrote: > On 2015/06/05 16:41:15, chrishtr wrote: > > On 2015/06/05 at 10:19:53, fs wrote: > > > > > https://codereview.chromium.org/1163913002/diff/20001/Source/core/paint/Inlin... > > > File Source/core/paint/InlinePainter.cpp (right): > > > > > > > > https://codereview.chromium.org/1163913002/diff/20001/Source/core/paint/Inlin... > > > Source/core/paint/InlinePainter.cpp:35: int outlineOutset; > > > On 2015/06/04 17:08:12, chrishtr wrote: > > > > Why does linesVisualOverflowBoundingBox() not already include the bounding > > box? > > > > If not then this seems to imply to me that this code is wrong also, since it > > > > should include outline: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > > > So include outline in linesVisualOverflowBoundingBox()? > > > > > > Hmm, yes... the inflate(outlineSize) in LayoutInline::clippedOverflowRect > > confused me - now it looks rather redundant to me. > > > > clippedOverflowRect is not what you're calling here. Should the outline code in > > clippedOverflowRect be moved into linesVisualOverflowBoundingBox? > > I was initially looking at clippedOverflowRect (or rather clippedOverflowRectForPaintInvalidation, but that just calls the other one) to see how the paint inv. rect was computed - and it had that inflation - which is why I included that in PS2 (dropped in PS3). Looking closer though wondered if that inflate() didn't cause the outline to be added twice, i.e. it should rather be something like: https://codereview.chromium.org/1164533005/ - either that or move it into linesVisualOverflowBoundingBox as you suggest. Ok... one of those two approaches though, righT?
On 2015/06/05 16:56:28, chrishtr wrote: > On 2015/06/05 at 16:54:46, fs wrote: > > On 2015/06/05 16:41:15, chrishtr wrote: > > > On 2015/06/05 at 10:19:53, fs wrote: > > > > > > > > https://codereview.chromium.org/1163913002/diff/20001/Source/core/paint/Inlin... > > > > File Source/core/paint/InlinePainter.cpp (right): > > > > > > > > > > > > https://codereview.chromium.org/1163913002/diff/20001/Source/core/paint/Inlin... > > > > Source/core/paint/InlinePainter.cpp:35: int outlineOutset; > > > > On 2015/06/04 17:08:12, chrishtr wrote: > > > > > Why does linesVisualOverflowBoundingBox() not already include the > bounding > > > box? > > > > > If not then this seems to imply to me that this code is wrong also, > since it > > > > > should include outline: > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > > > > > So include outline in linesVisualOverflowBoundingBox()? > > > > > > > > Hmm, yes... the inflate(outlineSize) in LayoutInline::clippedOverflowRect > > > confused me - now it looks rather redundant to me. > > > > > > clippedOverflowRect is not what you're calling here. Should the outline code > in > > > clippedOverflowRect be moved into linesVisualOverflowBoundingBox? > > > > I was initially looking at clippedOverflowRect (or rather > clippedOverflowRectForPaintInvalidation, but that just calls the other one) to > see how the paint inv. rect was computed - and it had that inflation - which is > why I included that in PS2 (dropped in PS3). Looking closer though wondered if > that inflate() didn't cause the outline to be added twice, i.e. it should rather > be something like: https://codereview.chromium.org/1164533005/ - either that or > move it into linesVisualOverflowBoundingBox as you suggest. > > Ok... one of those two approaches though, righT? Yeah, need to see what the try-bots say about that CL (and if it doesn't say much, then maybe make some more tests). The fact that this CL seem to fix the outline painting kind of suggests that the former (double inflate) is true.
On 2015/06/05 16:59:46, fs wrote: > On 2015/06/05 16:56:28, chrishtr wrote: > > On 2015/06/05 at 16:54:46, fs wrote: > > > On 2015/06/05 16:41:15, chrishtr wrote: > > > > On 2015/06/05 at 10:19:53, fs wrote: > > > > > > > > > > > > https://codereview.chromium.org/1163913002/diff/20001/Source/core/paint/Inlin... > > > > > File Source/core/paint/InlinePainter.cpp (right): > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1163913002/diff/20001/Source/core/paint/Inlin... > > > > > Source/core/paint/InlinePainter.cpp:35: int outlineOutset; > > > > > On 2015/06/04 17:08:12, chrishtr wrote: > > > > > > Why does linesVisualOverflowBoundingBox() not already include the > > bounding > > > > box? > > > > > > If not then this seems to imply to me that this code is wrong also, > > since it > > > > > > should include outline: > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > > > > > > > So include outline in linesVisualOverflowBoundingBox()? > > > > > > > > > > Hmm, yes... the inflate(outlineSize) in > LayoutInline::clippedOverflowRect > > > > confused me - now it looks rather redundant to me. > > > > > > > > clippedOverflowRect is not what you're calling here. Should the outline > code > > in > > > > clippedOverflowRect be moved into linesVisualOverflowBoundingBox? > > > > > > I was initially looking at clippedOverflowRect (or rather > > clippedOverflowRectForPaintInvalidation, but that just calls the other one) to > > see how the paint inv. rect was computed - and it had that inflation - which > is > > why I included that in PS2 (dropped in PS3). Looking closer though wondered if > > that inflate() didn't cause the outline to be added twice, i.e. it should > rather > > be something like: https://codereview.chromium.org/1164533005/ - either that > or > > move it into linesVisualOverflowBoundingBox as you suggest. > > > > Ok... one of those two approaches though, righT? > > Yeah, need to see what the try-bots say about that CL (and if it doesn't say > much, then maybe make some more tests). The fact that this CL seem to fix the > outline painting kind of suggests that the former (double inflate) is true. Apparently using linesVisualOverflowBoundingBox() misses continuations (noticed while looking at some of the other failing tests in the bug.) Rolled back to something resembling PS1.
The CQ bit was checked by chrishtr@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1163913002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196722 |
