avoid unneeded copies of SkPaths, avoid unneeded save/restore
Created attachment 134645 [details] Patch
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.
Created attachment 134648 [details] Patch
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?
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.
Created attachment 134655 [details] Patch
patch #3 updated the comment. PTAL
(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).
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...
(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?).
Gotcha. Agreed. Certain 'platformPath() const' should return a const native*. If the EWS goes green, can the CL get a + ?
Comment on attachment 134655 [details] Patch OK. r=me
Comment on attachment 134655 [details] Patch Clearing flags on attachment: 134655 Committed r112676: <http://trac.webkit.org/changeset/112676>
All reviewed patches have been landed. Closing bug.