Bug 34477 – [Qt] Canvas createPattern with no-repeat not working

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chang Shu]

QtLauncher fails on the following test case while Safari works.
http://waplabdc.nokia-boston.com/browser/users/ligman/canvas%20tests/colors%20and%20styles/pattern_norepeat.html

[qi]

1.	\WebCore\html\canvas\CanvasPattern.cpp parse and save the “repeat” information.
2.	\WebCore\platform\graphics\qt\GraphicsContextQt.cpp take the responsibility to draw the Rect (using QBrush):
p->fillRect(rect, QBrush(m_common->state.fillPattern->createPlatformPattern(affine)));
3.	\WebCore\platform\graphics\qt\PatternQt.cpp, creating the QBrush, where we only passed the image, we didn’t pass the “repeat” information, since QBrush don’t support it.


 4.  By the way, today, I found the similar issue with “blur”. The “blur” is one property of “shadow”. Currently, “shadow” is implemented in GraphicsContextQt.cpp. Since, everything (text, image, line, rect, etc) can have “shadow”, we draw “shadow” for each one, but we didn’t apply the “blur” pattern for “shadow” now. I am not sure, GraphicsContextQt should draw the “shadow” or let lower level like QBrush to draw “shadow”. Anyway, currently I find it is difficult to draw a “blur” “shadow” now in GraphicsContextQt.

[Simon Hausmann]

Ariya, do you think it's easier to implement this in Qt or in WebKit?

[qi]

1. The m_repeatX and m_repeatY is private variable in Pattern.h. If we need to implement the "repeat" in QtGraphicContext.cpp, it is difficult for us to get this information. What we can do is using "#if" in Pattern.h to expose a new API for "repeat".

[qi]

Does QT have a "blur" or "shadow" function?
In this case, "canvas" can draw a image, rect, line, text, etc. And, each of them can have a "shadow" and "shadow" can be "blur".
I searched in QT, I found QGraphicsBlurEffect and QGraphicsDropShadowEffect (QGraphicsEffect)looks for this purpose. But they suppose to be taken by QGraphicsItem, it looks heavy and not for this purpose. 
Not sure, QT has function for our purpose.

[Tor Arne Vestbø]

Please follow the QtWebKit bug reporting guidelines when reporting bugs.

See http://trac.webkit.org/wiki/QtWebKitBugs

Specifically:

  - The 'QtWebKit' component should only be used for bugs/features in the
    public QtWebKit API layer, not to signify that the bug is specific to
    the Qt port of WebKit

      http://trac.webkit.org/wiki/QtWebKitBugs#Component

  - Add the keyword 'Qt' to signal that it's a Qt-related bug

      http://trac.webkit.org/wiki/QtWebKitBugs#Keywords

[qi]

I found class "QPixmapDropShadowFilter" pretty much meet our requirement for shadow and blur, but unfortunately it is private class.

[qi]

Copy the comments about "blur" and "shadow" to bug 34479. This bug only focus on "repeat" issue.

[qi]

Created attachment 50432 [details]
patch file for bug fixing

1. Created 2 new API in Pattern.h for QT to expose "repeat" information.
2. In GraphicsContextQT, using "repeat" information to control fillRect.

[WebKit Review Bot]

Attachment 50432 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[qi]

Created attachment 50495 [details]
fix bug 34477 (canvas fillRect with "repeat")

1. create the new patch to replace old one for ChangeLog style issue.
2. Qt brush do not support "norepeat" or "repeatx" or "repeaty", it only support "repeat". This fix handle the "repeat" information at GraphicsContextQt.
3. To get the "repeat" information from Pattern (Since it is a private information), I create new API to expose the "repeat" information for Qt.

[Laszlo Gombos]

Comment on attachment 50495 [details]
fix bug 34477 (canvas fillRect with "repeat")

> Index: WebCore/platform/graphics/Pattern.h
> ===================================================================
> --- WebCore/platform/graphics/Pattern.h	(revision 55541)
> +++ WebCore/platform/graphics/Pattern.h	(working copy)
> @@ -86,7 +86,10 @@ public:
>  #endif
>      void setPatternSpaceTransform(const AffineTransform& patternSpaceTransformation);
>      void setPlatformPatternSpaceTransform();
> -
> +#if PLATFORM(QT)
> +    bool getRepeatX() const { return m_repeatX; }
> +    bool getRepeatY() const { return m_repeatY; }
> +#endif
>  private:
>      Pattern(Image*, bool repeatX, bool repeatY);
>  

Instead of exposing m_repeatX from Pattern, the information can be held in a WebKit class that derives from QBrush (e.g. BrushQt) . You can change the typedef from QBrush to BrushQt in Pattern.h.

[Eric Seidel (no email)]

Comment on attachment 50495 [details]
fix bug 34477 (canvas fillRect with "repeat")

We don't use "get" in getters.  The style bot should have caught that.  Looks like there are Tabs in the ChagneLog, also a style-bot failure.

I'm confused why Qt is the only platform which needs these exposed.

[qi]

I decide to follow Laszlo's comments to create a new class which inherited from QBrush. I will put the "repeat" information into the new class. Then we don't need to expose the API in "Pattern.h".

For Eric's question: The "repeat" information is saved in "Pattern.h" but it is private. When we call "createPlatformPattern" which will return a "QBrush", the "repeat" information suppose to pass into "QBrush" to let it handle it (if "QBrush" can handle it, we don't need expose it). But, "QBrush" don't support it, so we need to implement it at webkit level, that's why I try to expose it.

[qi]

Created attachment 50805 [details]
Patch for comments

1> Created new class BrushQt which inherited from QBrush to save the "repeat" info
2> Stop exposing the "repeat" info at the Pattern.h

[Simon Hausmann]

(In reply to comment #14)
> Created an attachment (id=50805) [details]
> Patch for comments
> 
> 1> Created new class BrushQt which inherited from QBrush to save the "repeat"
> info
> 2> Stop exposing the "repeat" info at the Pattern.h

I admit I prefer the previous solution (without the "get" prefix for the getters). Subclassing a primitive value-based type just for this one quirk feels like overkill to me, given that it's perfectly fine to have the getters in Pattern.

Laszlo, what do you think?

[Laszlo Gombos]

Simon, I have no strong preference on the "subclassing QBrush" vs. "exposing the getters in Pattern" issue. 

I think in the long run it is beneficial to minimize the usage of PLATFORM(QT) in the platform independent part of WebKit. One option is to introduce the getters for all platforms in Pattern.

[qi]

Created attachment 51348 [details]
new patch

Based on Laszlo's new comments, I expose the repeatX and repeatY in Pattern.h which is not only for Qt but everyone.

[Simon Hausmann]

Comment on attachment 51348 [details]
new patch


> +    bool repeatX() { return m_repeatX; }
> +    bool repeatY() { return m_repeatY; }

Both methods should be const. Please fix before landing.

The rest looks good to me.

[qi]

Created attachment 51416 [details]
add const

Add "const" to the getter.

[Laszlo Gombos]

Comment on attachment 51416 [details]
add const

Landed manually with the suggested change - http://trac.webkit.org/changeset/56391.