Bug 82641 - remove unneeded copies of SkPaths, remove unneeded save/restore
: remove unneeded copies of SkPaths, remove unneeded save/restore
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-03-29 12:28 PST by
Modified: 2012-03-30 09:33 PST (History)


Attachments
Patch (4.17 KB, patch)
2012-03-29 12:29 PST, Mike Reed
no flags Review Patch | Details | Formatted Diff | Diff
Patch (4.15 KB, patch)
2012-03-29 12:49 PST, Mike Reed
no flags Review Patch | Details | Formatted Diff | Diff
Patch (4.15 KB, patch)
2012-03-29 13:30 PST, Mike Reed
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-03-29 12:28:13 PST
avoid unneeded copies of SkPaths, avoid unneeded save/restore
------- Comment #1 From 2012-03-29 12:29:55 PST -------
Created an attachment (id=134645) [details]
Patch
------- Comment #2 From 2012-03-29 12:34:05 PST -------
Attachment 134645 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #3 From 2012-03-29 12:49:41 PST -------
Created an attachment (id=134648) [details]
Patch
------- Comment #4 From 2012-03-29 13:12:02 PST -------
(From update of attachment 134648 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=134648&action=review

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:398
> +    // we must make a copy of the path, to mark it as inverse-filled

I believe comments are supposed to be sentences in WebKit with capitals and periods.

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:422
> +    if (path->getFillType() != ftype) {
> +        storage = *path;
> +        storage.setFillType(ftype);
> +        path = &storage;
> +    }

I'm confused by this idium.  You're copying the path to change its fill type?

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:-831
> -    platformContext()->save();

setupPaintForFilling does not affect the context I take it?
------- Comment #5 From 2012-03-29 13:22:16 PST -------
I have to copy the path, which is a pointer to a const path, if I want to change anything. The point of this CL is to perform those expensive copies only if they are actually needed. The previous version always did an implicit copy, since the data-type was SkPath, and not SkPath*.

All of the setupPaint[forFilling,ForStroking] methods are const, and do not affect the context.

I will fix the comment.
------- Comment #6 From 2012-03-29 13:30:32 PST -------
Created an attachment (id=134655) [details]
Patch
------- Comment #7 From 2012-03-29 13:31:23 PST -------
patch #3 updated the comment. PTAL
------- Comment #8 From 2012-03-29 13:37:32 PST -------
(In reply to comment #5)
> I have to copy the path, which is a pointer to a const path, if I want to change anything. The point of this CL is to perform those expensive copies only if they are actually needed. The previous version always did an implicit copy, since the data-type was SkPath, and not SkPath*.

Actually (not Mike's fault), it looks like Path::platformPath() is const, but returns a non-const PlatformPath* (in this case, an SkPath*).  That's pretty bad.  IMHO, we should have two accessors:  a const one that returns a const PlatformPath*, and a non-const one that returns a non-const PlatformPath* (if absolutely necessary).
------- Comment #9 From 2012-03-29 14:00:21 PST -------
In these cases (I think), the Path object is always const, so I really can't mutate the underlying impl. I think my (deferred) copies are still needed...
------- Comment #10 From 2012-03-29 14:16:22 PST -------
(In reply to comment #9)
> In these cases (I think), the Path object is always const, so I really can't mutate the underlying impl. I think my (deferred) copies are still needed...

Your code is correct.  What I meant was that Path::platformPath() is const, but returns a non-const SkPath* (I think?).
------- Comment #11 From 2012-03-29 14:28:55 PST -------
Gotcha.

Agreed. Certain 'platformPath() const' should return a const native*.

If the EWS goes green, can the CL get a + ?
------- Comment #12 From 2012-03-30 08:10:16 PST -------
(From update of attachment 134655 [details])
OK.  r=me
------- Comment #13 From 2012-03-30 09:33:27 PST -------
(From update of attachment 134655 [details])
Clearing flags on attachment: 134655

Committed r112676: <http://trac.webkit.org/changeset/112676>
------- Comment #14 From 2012-03-30 09:33:32 PST -------
All reviewed patches have been landed.  Closing bug.