Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
472083002:
Move the shadows out of paintTextWithShadows. (Closed)
|
Created:
6 years, 4 months ago by jbroman Modified:
6 years, 4 months ago CC:
blink-reviews, blink-reviews-rendering, zoltan1, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, pdr., rune+blink Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionMove the shadows out of paintTextWithShadows.
It unnecessarily entangles one aspect of style with the actual text painting.
Also rename it paintText since it no longer deals with shadows.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180741
Patch Set 1 #
Total comments: 10
Patch Set 2 : more aggressive refactor - DO NOT SUBMIT 10% regression #Patch Set 3 : #
Total comments: 8
Patch Set 4 : review comments #
Total comments: 2
Patch Set 5 : GraphicsContextStateSaver& #
Messages
Total messages: 20 (0 generated)
Dealing with the shadows inside of paintTextWithShadows and everything else outside hasn't made sense since I removed the loop in paintTextWithShadows over a year ago. This seems more logical, and (I hope) makes it easier to replace the paintText step with one that uses other primitives, like text blobs.
+schenney, who may care and I neglected to add as a reviewer initially
I like the idea, but this shadow business makes my head spin :) https://codereview.chromium.org/472083002/diff/1/Source/core/rendering/Inline... File Source/core/rendering/InlineTextBox.cpp (right): https://codereview.chromium.org/472083002/diff/1/Source/core/rendering/Inline... Source/core/rendering/InlineTextBox.cpp:357: context->clearDrawLooper(); Is clearing required here? I don't think we were doing it before. https://codereview.chromium.org/472083002/diff/1/Source/core/rendering/Inline... Source/core/rendering/InlineTextBox.cpp:436: context->clearShadow(); This feels unbalanced: we're setting up shadows in updateGC but clearing them in the parent frame. Is it possible to push the setup logic up to the same level (in paint() maybe)? https://codereview.chromium.org/472083002/diff/1/Source/core/rendering/Inline... Source/core/rendering/InlineTextBox.cpp:436: context->clearShadow(); Also, is it correct to do this unconditionally (can we ever inherit a shadow looper and accidentally clear it here)? Not really introduced in this patch, but at least previously we were only clearing when we had a local shadow. https://codereview.chromium.org/472083002/diff/1/Source/core/rendering/Inline... Source/core/rendering/InlineTextBox.cpp:724: // It might be cleaner to use a GraphicsContextStateSaver. If shadows require nesting semantics we should definitely use a saver. I'm not sure they do though... https://codereview.chromium.org/472083002/diff/1/Source/core/rendering/Inline... Source/core/rendering/InlineTextBox.cpp:725: context->clearDrawLooper(); This may need to happen before paint decorations above -- looks like now we could leave a shadow looper on during paintDecoration() whereas before we did not (also, paintDecoration seems to do its own shadow voodoo and overrides the looper anyway -- would be nice to consolidate but not sure it's doable. maybe that's the reason why this works).
drastic changes; PTAL https://codereview.chromium.org/472083002/diff/1/Source/core/rendering/Inline... File Source/core/rendering/InlineTextBox.cpp (right): https://codereview.chromium.org/472083002/diff/1/Source/core/rendering/Inline... Source/core/rendering/InlineTextBox.cpp:357: context->clearDrawLooper(); On 2014/08/15 02:01:06, Florin Malita wrote: > Is clearing required here? I don't think we were doing it before. It's a little finicky. I've refactored to just use a state saver in a way that doesn't seem to have a significant adverse performance impact. https://codereview.chromium.org/472083002/diff/1/Source/core/rendering/Inline... Source/core/rendering/InlineTextBox.cpp:436: context->clearShadow(); On 2014/08/15 02:01:06, Florin Malita wrote: > This feels unbalanced: we're setting up shadows in updateGC but clearing them in > the parent frame. > > Is it possible to push the setup logic up to the same level (in paint() maybe)? I tried to move this, but it's logic common to painting the text and the decorations, and I couldn't find a clean way. What I have done is refactored to use GraphicsContextStateSaver. I'd originally put the exact condition for saving (textStroke || shadow) in there, but in the spirit of this comment, I'm now passing the GCSS pointer and letting updateGraphicsContext choose whether a reset will be required. I'm not quite thrilled about the latter; what do you think? https://codereview.chromium.org/472083002/diff/1/Source/core/rendering/Inline... Source/core/rendering/InlineTextBox.cpp:436: context->clearShadow(); On 2014/08/15 02:01:06, Florin Malita wrote: > Also, is it correct to do this unconditionally (can we ever inherit a shadow > looper and accidentally clear it here)? Not really introduced in this patch, but > at least previously we were only clearing when we had a local shadow. We'd have a number of issues were this possible. I'm not aware of any way this can happen in the HTML case, at least. I don't think it can for SVG either, but I don't know SVG well. (I do wish GraphicsContext were less stateful so we didn't even have to ask questions like these.) https://codereview.chromium.org/472083002/diff/1/Source/core/rendering/Inline... Source/core/rendering/InlineTextBox.cpp:724: // It might be cleaner to use a GraphicsContextStateSaver. On 2014/08/15 02:01:06, Florin Malita wrote: > If shadows require nesting semantics we should definitely use a saver. I'm not > sure they do though... I don't think they do. There's certainly no layout test that includes such a case. https://codereview.chromium.org/472083002/diff/1/Source/core/rendering/Inline... Source/core/rendering/InlineTextBox.cpp:725: context->clearDrawLooper(); On 2014/08/15 02:01:06, Florin Malita wrote: > This may need to happen before paint decorations above -- looks like now we > could leave a shadow looper on during paintDecoration() whereas before we did > not (also, paintDecoration seems to do its own shadow voodoo and overrides the > looper anyway -- would be nice to consolidate but not sure it's doable. maybe > that's the reason why this works). We couldn't, only because of the explicit clear in updateGraphicsContext. I've removed the shadow voodoo in the decoration code (and adjusted TestExpectations). It has a small change in how shadows of text and decorations overlap (but within what the spec permits, and this is a super-weird case anyhow). If we wanted to make shadows of text decorations work more pleasantly, that would be a larger, separate change.
Great cleanup! https://codereview.chromium.org/472083002/diff/40001/Source/core/rendering/In... File Source/core/rendering/InlineTextBox.cpp (right): https://codereview.chromium.org/472083002/diff/40001/Source/core/rendering/In... Source/core/rendering/InlineTextBox.cpp:515: static void updateGraphicsContext(GraphicsContext* context, const TextPaintingStyle& textStyle, bool horizontal, GraphicsContextStateSaver* stateSaver = 0) Nit: this could live in the anonymous namespace above. https://codereview.chromium.org/472083002/diff/40001/Source/core/rendering/In... Source/core/rendering/InlineTextBox.cpp:519: if (stateSaver) { Can we skip this when mode == newMode (move it inside the conditional below)? https://codereview.chromium.org/472083002/diff/40001/Source/core/rendering/In... Source/core/rendering/InlineTextBox.cpp:545: stateSaver->save(); We should save before touching the looper. https://codereview.chromium.org/472083002/diff/40001/Source/core/rendering/In... Source/core/rendering/InlineTextBox.cpp:720: GraphicsContextStateSaver stateSaver(*context); Can't this save be deferred also? Why weren't we saving before?
Done. Apologies for diff ugliness due to code movement. https://codereview.chromium.org/472083002/diff/40001/Source/core/rendering/In... File Source/core/rendering/InlineTextBox.cpp (right): https://codereview.chromium.org/472083002/diff/40001/Source/core/rendering/In... Source/core/rendering/InlineTextBox.cpp:515: static void updateGraphicsContext(GraphicsContext* context, const TextPaintingStyle& textStyle, bool horizontal, GraphicsContextStateSaver* stateSaver = 0) On 2014/08/20 18:55:45, Florin Malita wrote: > Nit: this could live in the anonymous namespace above. Okay. At this point, I might as well push paintText and paintEmphasisMark in as well. https://codereview.chromium.org/472083002/diff/40001/Source/core/rendering/In... Source/core/rendering/InlineTextBox.cpp:519: if (stateSaver) { On 2014/08/20 18:55:46, Florin Malita wrote: > Can we skip this when mode == newMode (move it inside the conditional below)? Done. https://codereview.chromium.org/472083002/diff/40001/Source/core/rendering/In... Source/core/rendering/InlineTextBox.cpp:545: stateSaver->save(); On 2014/08/20 18:55:46, Florin Malita wrote: > We should save before touching the looper. Err, yes, of course. Hazard of "p" and "P" being so similar in vim. Good catch. :) https://codereview.chromium.org/472083002/diff/40001/Source/core/rendering/In... Source/core/rendering/InlineTextBox.cpp:720: GraphicsContextStateSaver stateSaver(*context); On 2014/08/20 18:55:45, Florin Malita wrote: > Can't this save be deferred also? Why weren't we saving before? In the combinedText case, there's also the CTM rotations. But in the interest of consistency and non-surprise, I've done as you asked (and restored the CCW rotation).
LGTM, assuming perf looks good :) https://codereview.chromium.org/472083002/diff/60001/Source/core/rendering/In... File Source/core/rendering/InlineTextBox.cpp (right): https://codereview.chromium.org/472083002/diff/60001/Source/core/rendering/In... Source/core/rendering/InlineTextBox.cpp:465: if (stateSaver) { Nit: I think all callers pass a statesaver now. Instead of a pointer, we could use a ref and update this conditional (and the other one at the bottom) to something like if (!stateSaver.saved()) stateSaver.save();
The CQ bit was checked by jbroman@chromium.org
On 2014/08/20 20:55:40, Florin Malita wrote: > LGTM, assuming perf looks good :) > > https://codereview.chromium.org/472083002/diff/60001/Source/core/rendering/In... > File Source/core/rendering/InlineTextBox.cpp (right): > > https://codereview.chromium.org/472083002/diff/60001/Source/core/rendering/In... > Source/core/rendering/InlineTextBox.cpp:465: if (stateSaver) { > Nit: I think all callers pass a statesaver now. Instead of a pointer, we could > use a ref and update this conditional (and the other one at the bottom) to > something like > > if (!stateSaver.saved()) > stateSaver.save(); The performance impact on Wikipedia is less than my ability to measure it, in my tests.
https://codereview.chromium.org/472083002/diff/60001/Source/core/rendering/In... File Source/core/rendering/InlineTextBox.cpp (right): https://codereview.chromium.org/472083002/diff/60001/Source/core/rendering/In... Source/core/rendering/InlineTextBox.cpp:465: if (stateSaver) { On 2014/08/20 20:55:40, Florin Malita wrote: > Nit: I think all callers pass a statesaver now. Instead of a pointer, we could > use a ref and update this conditional (and the other one at the bottom) to > something like > > if (!stateSaver.saved()) > stateSaver.save(); > Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jbroman@chromium.org/472083002/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/23734) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)
The CQ bit was checked by jbroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jbroman@chromium.org/472083002/80001
The CQ bit was unchecked by jbroman@chromium.org
The CQ bit was checked by jbroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jbroman@chromium.org/472083002/80001
Message was sent while issue was closed.
Committed patchset #5 (80001) as 180741 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
