Bug 82641 - remove unneeded copies of SkPaths, remove unneeded save/restore
: remove unneeded copies of SkPaths, remove unneeded save/restore
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Mike Reed
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-29 12:28 PDT by Mike Reed
Modified: 2012-03-30 09:33 PDT (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Reed 2012-03-29 12:28:13 PDT
avoid unneeded copies of SkPaths, avoid unneeded save/restore
Comment 1 Mike Reed 2012-03-29 12:29:55 PDT
Created attachment 134645 [details]
Patch
Comment 2 WebKit Review Bot 2012-03-29 12:34:05 PDT
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 Mike Reed 2012-03-29 12:49:41 PDT
Created attachment 134648 [details]
Patch
Comment 4 Eric Seidel 2012-03-29 13:12:02 PDT
Comment on attachment 134648 [details]
Patch

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 Mike Reed 2012-03-29 13:22:16 PDT
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 Mike Reed 2012-03-29 13:30:32 PDT
Created attachment 134655 [details]
Patch
Comment 7 Mike Reed 2012-03-29 13:31:23 PDT
patch #3 updated the comment. PTAL
Comment 8 Stephen White 2012-03-29 13:37:32 PDT
(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 Mike Reed 2012-03-29 14:00:21 PDT
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 Stephen White 2012-03-29 14:16:22 PDT
(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 Mike Reed 2012-03-29 14:28:55 PDT
Gotcha.

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

If the EWS goes green, can the CL get a + ?
Comment 12 Stephen White 2012-03-30 08:10:16 PDT
Comment on attachment 134655 [details]
Patch

OK.  r=me
Comment 13 WebKit Review Bot 2012-03-30 09:33:27 PDT
Comment on attachment 134655 [details]
Patch

Clearing flags on attachment: 134655

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