Bug 82641 – remove unneeded copies of SkPaths, remove unneeded save/restore

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Mike Reed]

avoid unneeded copies of SkPaths, avoid unneeded save/restore

[Mike Reed]

Created attachment 134645 [details]
Patch

[WebKit Review Bot]

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.

[Mike Reed]

Created attachment 134648 [details]
Patch

[Eric Seidel (no email)]

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?

[Mike Reed]

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.

[Mike Reed]

Created attachment 134655 [details]
Patch

[Mike Reed]

patch #3 updated the comment. PTAL

[Stephen White]

(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).

[Mike Reed]

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...

[Stephen White]

(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?).

[Mike Reed]

Gotcha.

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

If the EWS goes green, can the CL get a + ?

[Stephen White]

Comment on attachment 134655 [details]
Patch

OK.  r=me

[WebKit Review Bot]

Comment on attachment 134655 [details]
Patch

Clearing flags on attachment: 134655

Committed r112676: <http://trac.webkit.org/changeset/112676>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.