[webkit-reviews] review denied: [Bug 136801] REGRESSION: Text with a zero offset, zero blur shadow vanishes : [Attachment 238236] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 136801: REGRESSION: Text with a zero offset, zero blur shadow vanishes
https://bugs.webkit.org/show_bug.cgi?id=136801

Attachment 238236: Patch
https://bugs.webkit.org/attachment.cgi?id=238236&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=238236&action=review


It’s OK to rearrange like this. No need to have this live in InlineTextBox. But
this is unnecessarily tricky to use, and the applyShadowToGraphicsContext
function has two confusing boolean arguments!

I’m going to say review- because I think we can factor this better.

> Source/WebCore/rendering/TextPainter.cpp:103
> +    bool lastShadowIterationDrawsText = !stroked && opaque;

This should be named lastShadowIterationShouldDrawText, I think. But maybe it
won’t exist at all after some more refactoring.

> Source/WebCore/rendering/TextPainter.cpp:108
> +	   bool emptyShadow = TextPainter::isEmptyShadow(shadow, opaque);

It’s not great for a local variable to be named "emptyShadow" when it's not a
shadow, but rather is a boolean.

> Source/WebCore/rendering/TextPainter.cpp:109
> +	   bool onlyDrawsShadow = TextPainter::isLastShadowIteration(shadow) &&
lastShadowIterationDrawsText;

I am deeply puzzled by the logic here. It says that when we want to draw text,
then we should only draw a shadow. That can’t be right so I must be reading it
wrong.

> Source/WebCore/rendering/TextPainter.cpp:136
> +	   if (!onlyDrawsShadow)
> +	       context.restore(); // Matching call to context.save() in
applyShadowToGraphicsContext
>	   else
>	       context.clearShadow();

Seems to me this would be clearer if these four lines of code were a function
like applyShadowToGraphicsContext, maybe called removeShadowFromGraphicsContext
or unapplyShadowFromGraphicsContext. Another way to do it would be to use a
class for this and have the destructor do this work. That would concentrate the
code to set things up and clean them up in one place.

> Source/WebCore/rendering/TextPainter.h:59
> +    static inline bool isLastShadowIteration(const ShadowData* shadow)
> +    {
> +	   return shadow && !shadow->next();
> +    }
> +    static inline bool isEmptyShadow(const ShadowData* shadow, bool
textIsOpaque = false)
> +    {
> +	   return textIsOpaque && shadow && shadow->location() == IntPoint() &&
!shadow->radius();
> +    }

I understand these need for these two functions to be defined in the header
file since they are inlined. But I suggest putting them outside the class
definition, at the end of the header file, rather than having them inline. I
think this makes the header easier to read.

I don’t understand what an “empty shadow” is, which makes the isEmptyShadow
quite unpleasant. It’s something different from “no shadow at all”. I think
it’s a shadow that is completely covered by opaque text perhaps, but then it’s
not really a kind of shadow, so the function name is not good. It really seems
bad to have to pass a “text is opaque” boolean to a function that claims to be
doing something to a shadow.

> Source/WebCore/rendering/TextPainter.h:61
> +    static FloatSize applyShadowToGraphicsContext(GraphicsContext&, const
ShadowData&, const FloatRect& textRect, bool onlyDrawsShadow, bool
shadowIsEmpty, FontOrientation = Horizontal);

I don’t like the separation of responsibilities here. I think we should have a
single helper function that does everything, or perhaps two, one to start and
one to end, not three functions that have to be used carefully with boolean
parameters. Maybe this set of three functions should be a class; that could
help us correctly unapply the shadow from the graphics context in exactly the
right cases without having to be so careful about how we write the code at the
call sites.

It seems to me that applyShadowToGraphicsContext can return some indication
that we should skip the shadow entirely, rather than requiring a relatively
subtle and complex check at both call sites.