Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1403573002:
Simplify draw one glyph (Closed)
|
Created:
5 years, 2 months ago by herb_g Modified:
5 years ago Reviewers:
bungeman-skia CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionReplace D1G with a simpler implementation.
Committed: https://skia.googlesource.com/skia/+/001e74426672e00f3f2783ccf728031662d4a358
Patch Set 1 #Patch Set 2 : merge simplified draw one glyph in. #
Total comments: 10
Patch Set 3 : Address comments. #Messages
Total messages: 22 (9 generated)
merge simplified draw one glyph in.
Description was changed from ========== Simplify D1g. BUG=skia: ========== to ========== Replace D1G with a simpler implementation. BUG=skia: ==========
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1403573002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1403573002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
herb@google.com changed reviewers: + bungeman@google.com
https://codereview.chromium.org/1403573002/diff/20001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/1403573002/diff/20001/src/core/SkDraw.cpp#new... src/core/SkDraw.cpp:1430: SkBlitter* blitter) nit: Will this fit on the previous line? It looks really close... https://codereview.chromium.org/1403573002/diff/20001/src/core/SkDraw.cpp#new... src/core/SkDraw.cpp:1446: void drawOneGlyph(const SkGlyph& glyph, SkPoint position) { Is there a reason not to make this void operator()(const SkGlyph& glyph, SkPoint position) it looks strange that everywhere this is called it looks like 'drawOneGlyph.drawOneGlyph'. https://codereview.chromium.org/1403573002/diff/20001/src/core/SkDraw.cpp#new... src/core/SkDraw.cpp:1470: if (fUseRegionToDraw) { So essentially, this is trading off a predictable call through a function pointer with a predictable branch? https://codereview.chromium.org/1403573002/diff/20001/src/core/SkDraw.cpp#new... src/core/SkDraw.cpp:1499: static bool UseRegionDrawOneGlyph(const SkRasterClip* rClip) { Can this just be named 'UseRegionToDraw' so that it is named similar to what it initializes? Also, if we have this one, why not a 'SkIRect ClipBounds(const SkRasterClip* rClip)' so that fClipBounds can also be marked const? https://codereview.chromium.org/1403573002/diff/20001/src/core/SkDraw.cpp#new... src/core/SkDraw.cpp:1563: DrawOneGlyph drawOneGlyph(*this, paint, cache, wrapper.getBlitter()); nit: Any reason for the spacing here? It seems to come from the spacing where the same line is used below, but here it doesn't line up with anything, so it looks a little odd.
Address comments.
https://codereview.chromium.org/1403573002/diff/20001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/1403573002/diff/20001/src/core/SkDraw.cpp#new... src/core/SkDraw.cpp:1430: SkBlitter* blitter) On 2015/11/30 21:42:29, bungeman1 wrote: > nit: Will this fit on the previous line? It looks really close... Done. https://codereview.chromium.org/1403573002/diff/20001/src/core/SkDraw.cpp#new... src/core/SkDraw.cpp:1446: void drawOneGlyph(const SkGlyph& glyph, SkPoint position) { On 2015/11/30 21:42:29, bungeman1 wrote: > Is there a reason not to make this > > void operator()(const SkGlyph& glyph, SkPoint position) > > it looks strange that everywhere this is called it looks like > 'drawOneGlyph.drawOneGlyph'. Done - made it obey the FindAndPlaceGlyph interface so no lambda is needed. https://codereview.chromium.org/1403573002/diff/20001/src/core/SkDraw.cpp#new... src/core/SkDraw.cpp:1470: if (fUseRegionToDraw) { On 2015/11/30 21:42:29, bungeman1 wrote: > So essentially, this is trading off a predictable call through a function > pointer with a predictable branch? That is one of two reasons. The second is that they share much of the setup code. https://codereview.chromium.org/1403573002/diff/20001/src/core/SkDraw.cpp#new... src/core/SkDraw.cpp:1499: static bool UseRegionDrawOneGlyph(const SkRasterClip* rClip) { On 2015/11/30 21:42:29, bungeman1 wrote: > Can this just be named 'UseRegionToDraw' so that it is named similar to what it > initializes? > > Also, if we have this one, why not a 'SkIRect ClipBounds(const SkRasterClip* > rClip)' so that fClipBounds can also be marked const? Done. https://codereview.chromium.org/1403573002/diff/20001/src/core/SkDraw.cpp#new... src/core/SkDraw.cpp:1563: DrawOneGlyph drawOneGlyph(*this, paint, cache, wrapper.getBlitter()); On 2015/11/30 21:42:29, bungeman1 wrote: > nit: Any reason for the spacing here? It seems to come from the spacing where > the same line is used below, but here it doesn't line up with anything, so it > looks a little odd. Done.
PTAL
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1403573002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1403573002/40001
Description was changed from ========== Replace D1G with a simpler implementation. BUG=skia: ========== to ========== Replace D1G with a simpler implementation. ==========
The CQ bit was unchecked by bungeman@google.com
lgtm
The CQ bit was checked by herb@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1403573002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1403573002/40001
Message was sent while issue was closed.
Description was changed from ========== Replace D1G with a simpler implementation. ========== to ========== Replace D1G with a simpler implementation. Committed: https://skia.googlesource.com/skia/+/001e74426672e00f3f2783ccf728031662d4a358 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/001e74426672e00f3f2783ccf728031662d4a358
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1486723004/ by herb@google.com. The reason for reverting is: This seems to break nexus 9 release.. |
