Bug 19727 – Return bool from GraphicsContext::getShadow() so the tests aren't duplicated so many times in Cairo and Qt ports

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Jonathon Jongsma (jonner)]

The Cairo and Qt ports dont' support platform shadows, so text shadows are drawn 'manually'.  Before drawing the shadows, they perform a test to see if there is a valid shadow, e.g.:
bool hasShadow = textDrawingMode() == cTextFill && shadowColor.isValid() && (shadowSize.width() || shadowSize.height());

This test must be replicated anywhere we draw something that should have a shadow (e.g. text, underlines, etc).  Instead of copying and pasting this test every time, it would be nice to have a convenience function such as GraphicsContext::hasShadow() that could tell us whether a shadow should be drawn or not.

(Suggested in Bug #18459)

[Jonathon Jongsma (jonner)]

note that if this was implemented, the patch for Bug #19676 should be changed to use it as well.

[Jonathon Jongsma (jonner)]

Created attachment 21884 [details]
add GraphicsContext::hasShadow()

I didn't include the textDrawingMode() test since that didn't feel like it fit in hasShadow(), but if others feel it belongs there, I can add it.

[Darin Adler]

Comment on attachment 21884 [details]
add GraphicsContext::hasShadow()

I think a better way to do this would be to make the getShadow function return a boolean, so you can easily check if you can entirely skip the shadow drawing while getting the specification for drawing it if you do need to.

You can see this would work because the calls to hasShadow both have calls to getShadow immediately afterward.

+    return m_common->state.shadowColor.isValid() &&
+        (m_common->state.shadowSize.width() ||
+         m_common->state.shadowSize.height());

This should also check shadowColor.alpha() for a non-zero value.

I'm going to say r=me, but I think it would be better to do this without adding a new function.

Comment on attachment 21884 [details]
add GraphicsContext::hasShadow()

This function will return false for a 0-offset, positive-blur shadow. At the very least, this makes the name "hasShadow()" misleading.

[Jonathon Jongsma (jonner)]

(In reply to comment #4)
> (From update of attachment 21884 [details] [edit])
> This function will return false for a 0-offset, positive-blur shadow. At the
> very least, this makes the name "hasShadow()" misleading.
> 

Good point, but I think you've found a bug rather and this function should probably return true for this case since it indicates that the backend should draw something (In this case, the cairo backend would draw a shadow that is completely obscured since it doesn't yet support blur, but I don't see a big problem with that since I'd assume that it will get blur support eventually).  To me, this indicates why it would be beneficial to separate the logic into a function like this -- a bug in the logic only needs to be fixed once rather than several places.  

I like darin's suggestion of returning a bool for getShadow.  I'll attach an updated patch with that change (and check for non-zero alpha and blur values)

[Jonathon Jongsma (jonner)]

Created attachment 21887 [details]
Updated patch which returns bool from getShadow()

This patch should address the comments from both darin and mitz.  Also, just in case it's not obvious, this patch depends on the patch in Bug #18459 which adds text-shadow support to the Qt port.  This patch was submitted separately since it affeccts more than just the Qt port, but it can't really be applied until the other patch is applied.

[Jonathon Jongsma (jonner)]

updated summary to match new patch.

[Dirk Schulze]

You have to be carefull with 
+    bool hasShadow = context->textDrawingMode() == cTextFill &&
+        context->getShadow(shadowSize, shadowBlur, shadowColor);

you won't be able to draw shadows on stroked texts. See changes for FontCairo in https://bugs.webkit.org/attachment.cgi?id=20845

[Darin Adler]

Comment on attachment 21887 [details]
Updated patch which returns bool from getShadow()

+    return color.isValid() && color.alpha() &&
+        (blur || m_common->state.shadowSize.width() || m_common->state.shadowSize.height());

Heh, I just realized that invalid colors are guaranteed to have an alpha of 0, so you don't need the isValid check at all.

+        (blur || m_common->state.shadowSize.width() || m_common->state.shadowSize.height());

Why not use size here instead of m_common->state.shadowSize?

r=me

[Jonathon Jongsma (jonner)]

(In reply to comment #9)
> (From update of attachment 21887 [details] [edit])
> +    return color.isValid() && color.alpha() &&
> +        (blur || m_common->state.shadowSize.width() ||
> m_common->state.shadowSize.height());
> 
> Heh, I just realized that invalid colors are guaranteed to have an alpha of 0,
> so you don't need the isValid check at all.
> 
> +        (blur || m_common->state.shadowSize.width() ||
> m_common->state.shadowSize.height());
> 
> Why not use size here instead of m_common->state.shadowSize?

oops, I guess because I just re-used code from my previous hasShadow() patch that didn't have locals.  It would be cleaner if it just used the local, I admit.  

Would you like me to resubmit a new patch?

[Jonathon Jongsma (jonner)]

Created attachment 21891 [details]
Updated patch: use local, fix build failure in Qt

in fact, I need to submit an updated patch anyway since I accidentally moved a variable out of scope and broke compilation for the Qt port.  Sorry for all of the noise.  Hopefully this one is final.

[Darin Adler]

Comment on attachment 21891 [details]
Updated patch: use local, fix build failure in Qt

Please don't check in your ChangeLog with:

         WARNING: NO TEST CASES ADDED OR CHANGED

Please add an entry to the ChangeLog with the bug number in it and a summary description of the changes.

Code changes still good. r=me

[Jonathon Jongsma (jonner)]

(In reply to comment #12)
> (From update of attachment 21891 [details] [edit])
> Please don't check in your ChangeLog with:
> 
>          WARNING: NO TEST CASES ADDED OR CHANGED

Sorry, just to clarify, are you saying that I should delete this line before attaching a patch to bugzilla?  I usually leave that in when I don't have tests just to make it obvious to the reviewer that there are no tests with this patch.  But I can remove it from future patches if you'd like.  On the other hand, if by 'check in', you meant 'commit to svn', then I'm afraid I don't have commit access yet.

> Please add an entry to the ChangeLog with the bug number in it and a summary
> description of the changes.

Good point, I will try to remember to do so in the future.  Or I can upload a modified patch if it would be helpful.  

[Simon Hausmann]

Comment on attachment 21891 [details]
Updated patch: use local, fix build failure in Qt

Landed in r34765