Bug 81486 - [BlackBerry] Use GraphicsContext::fillPath() and strokePath instead of drawPath() in RenderThemeBlackBerry
: [BlackBerry] Use GraphicsContext::fillPath() and strokePath instead of drawPa...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Leo Yang
:
Depends on:
Blocks: 73144
  Show dependency treegraph
 
Reported: 2012-03-18 20:52 PDT by Leo Yang
Modified: 2012-04-01 06:40 PDT (History)
7 users (show)

See Also:


Attachments
Patch (4.05 KB, patch)
2012-03-18 21:03 PDT, Leo Yang
rwlbuis: review-
Details | Formatted Diff | Diff
Patch v2 (4.63 KB, patch)
2012-03-31 20:04 PDT, Leo Yang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Leo Yang 2012-03-18 20:52:53 PDT
RenderThemeBlackBerry.cpp is now using GraphicsContext::drawPath().
Comment 1 Leo Yang 2012-03-18 21:03:03 PDT
Created attachment 132537 [details]
Patch
Comment 2 Rob Buis 2012-03-19 07:02:06 PDT
Comment on attachment 132537 [details]
Patch

We should work on getting rid of using drawPath, it can be replaced by using fillPath/strokePath. This means RenderThemeBlackBerry has to be adapted to use those instead.
Comment 3 Leo Yang 2012-03-19 22:52:48 PDT
(In reply to comment #2)
> (From update of attachment 132537 [details])
> We should work on getting rid of using drawPath, it can be replaced by using fillPath/strokePath. This means RenderThemeBlackBerry has to be adapted to use those instead.

If we replace drawPath with fillPath and strokePath, there is some overhead:

826     if (paintingDisabled())
 827         return;
 828 
 829     SkPath path = *pathToFill.platformPath();
 830     if (!isPathSkiaSafe(getCTM(), path))
 831       return;
 832 
 833     const GraphicsContextState& state = m_state;
 834     path.setFillType(state.fillRule == RULE_EVENODD ?
 835         SkPath::kEvenOdd_FillType : SkPath::kWinding_FillType);

This block of code will be unnecessarily called twice.
Comment 4 Leo Yang 2012-03-26 20:08:27 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 132537 [details] [details])
> > We should work on getting rid of using drawPath, it can be replaced by using fillPath/strokePath. This means RenderThemeBlackBerry has to be adapted to use those instead.
> 
> If we replace drawPath with fillPath and strokePath, there is some overhead:
> 
> 826     if (paintingDisabled())
>  827         return;
>  828 
>  829     SkPath path = *pathToFill.platformPath();
>  830     if (!isPathSkiaSafe(getCTM(), path))
>  831       return;
>  832 
>  833     const GraphicsContextState& state = m_state;
>  834     path.setFillType(state.fillRule == RULE_EVENODD ?
>  835         SkPath::kEvenOdd_FillType : SkPath::kWinding_FillType);
> 
> This block of code will be unnecessarily called twice.

@rob: what do you think? I can move drawPath implementation to a separated new file or RenderThemeBlackBerry if you agree.
Comment 5 Leo Yang 2012-03-31 19:54:03 PDT
Change bug title
Comment 6 Leo Yang 2012-03-31 20:04:41 PDT
Created attachment 134985 [details]
Patch v2

Substituting drawPath() by fillPath() and strokePath()
Comment 7 Rob Buis 2012-04-01 05:57:00 PDT
Comment on attachment 134985 [details]
Patch v2

Looks fine.
Comment 8 Leo Yang 2012-04-01 06:03:37 PDT
Thanks for your review.
Comment 9 WebKit Review Bot 2012-04-01 06:40:49 PDT
Comment on attachment 134985 [details]
Patch v2

Clearing flags on attachment: 134985

Committed r112814: <http://trac.webkit.org/changeset/112814>
Comment 10 WebKit Review Bot 2012-04-01 06:40:54 PDT
All reviewed patches have been landed.  Closing bug.