Bug 34477 - [Qt] Canvas createPattern with no-repeat not working
Summary: [Qt] Canvas createPattern with no-repeat not working
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: qi
Keywords: Qt
Depends on:
Reported: 2010-02-02 07:49 PST by Chang Shu
Modified: 2010-03-23 08:00 PDT (History)
7 users (show)

See Also:

patch file for bug fixing (55.81 KB, patch)
2010-03-10 13:33 PST, qi
no flags Details | Formatted Diff | Diff
fix bug 34477 (canvas fillRect with "repeat") (55.83 KB, patch)
2010-03-11 06:33 PST, qi
eric: review-
Details | Formatted Diff | Diff
Patch for comments (59.03 KB, patch)
2010-03-16 10:19 PDT, qi
no flags Details | Formatted Diff | Diff
new patch (55.60 KB, patch)
2010-03-22 13:55 PDT, qi
no flags Details | Formatted Diff | Diff
add const (55.67 KB, patch)
2010-03-23 06:32 PDT, qi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chang Shu 2010-02-02 07:49:22 PST
QtLauncher fails on the following test case while Safari works.
Comment 1 qi 2010-03-08 13:30:28 PST
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.
Comment 2 Simon Hausmann 2010-03-09 00:58:17 PST
Ariya, do you think it's easier to implement this in Qt or in WebKit?
Comment 3 qi 2010-03-09 11:52:01 PST
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".
Comment 4 qi 2010-03-09 12:55:53 PST
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.
Comment 5 Tor Arne Vestbø 2010-03-10 06:39:59 PST
Please follow the QtWebKit bug reporting guidelines when reporting bugs.

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


  - 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


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

Comment 6 qi 2010-03-10 08:03:55 PST
I found class "QPixmapDropShadowFilter" pretty much meet our requirement for shadow and blur, but unfortunately it is private class.
Comment 7 qi 2010-03-10 13:28:14 PST
Copy the comments about "blur" and "shadow" to bug 34479. This bug only focus on "repeat" issue.
Comment 8 qi 2010-03-10 13:33:37 PST
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.
Comment 9 WebKit Review Bot 2010-03-10 13:39:20 PST
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.
Comment 10 qi 2010-03-11 06:33:12 PST
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.
Comment 11 Laszlo Gombos 2010-03-15 10:24:32 PDT
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();
> -
> +    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.
Comment 12 Eric Seidel (no email) 2010-03-15 13:27:56 PDT
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.
Comment 13 qi 2010-03-15 14:00:13 PDT
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.
Comment 14 qi 2010-03-16 10:19:27 PDT
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
Comment 15 Simon Hausmann 2010-03-21 15:09:18 PDT
(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?
Comment 16 Laszlo Gombos 2010-03-22 09:51:32 PDT
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.
Comment 17 qi 2010-03-22 13:55:03 PDT
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.
Comment 18 Simon Hausmann 2010-03-22 14:40:24 PDT
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.
Comment 19 qi 2010-03-23 06:32:42 PDT
Created attachment 51416 [details]
add const

Add "const" to the getter.
Comment 20 Laszlo Gombos 2010-03-23 07:59:49 PDT
Comment on attachment 51416 [details]
add const

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