Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2241683005:
SkPDF: unify drawText and drawPosText (Closed)
|
Created:
4 years, 4 months ago by hal.canary Modified:
4 years, 4 months 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. |
DescriptionSkPDF: unify drawText and drawPosText
Motivation: a later CL will add drawTextBlob() (after
https://crrev.com/2084533004 lands). This CL is designed
with that change in mind. Also fewer redundant lines of
code.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2241683005
Committed: https://skia.googlesource.com/skia/+/6059dc32fe36358175cb81541c91e74a2a7e771a
Committed: https://skia.googlesource.com/skia/+/4ed2f01cf6f3a63d5185ea5b442549d20ce2ec16
Patch Set 1 : compile on win #
Total comments: 8
Patch Set 2 : 2016-08-15 (Monday) 14:13:45 EDT #Patch Set 3 : stop assuming typeface has glyphs #
Messages
Total messages: 30 (21 generated)
Description was changed from ========== SkPDF: unify drawText and drawPosText Motivation: a later CL will add drawTextBlob() (after https://crrev.com/2084533004 lands). This CL is designed with that change in mind. BUG=skia: ========== to ========== SkPDF: unify drawText and drawPosText Motivation: a later CL will add drawTextBlob() (after https://crrev.com/2084533004 lands). This CL is designed with that change in mind. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2241683005 ==========
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Description was changed from ========== SkPDF: unify drawText and drawPosText Motivation: a later CL will add drawTextBlob() (after https://crrev.com/2084533004 lands). This CL is designed with that change in mind. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2241683005 ========== to ========== SkPDF: unify drawText and drawPosText Motivation: a later CL will add drawTextBlob() (after https://crrev.com/2084533004 lands). This CL is designed with that change in mind. Also fewer redundant lines of code. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2241683005 ==========
halcanary@google.com changed reviewers: + bungeman@google.com
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2241683005/diff/20001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/2241683005/diff/20001/src/pdf/SkPDFDevice.cpp... src/pdf/SkPDFDevice.cpp:1193: SkScalar y = 2 == scalarsPerPos ? *pos++ : 0; This is apparently the only place this 'scalarsPerPos' is used, why not just replace it with 'positioning' and avoid the ugly, ugly cast where 'scalarsPerPos' is currently set? https://codereview.chromium.org/2241683005/diff/20001/src/pdf/SkPDFDevice.cpp... src/pdf/SkPDFDevice.cpp:1219: this->drawTextInternal(d, text, len, pos, (SkTextBlob::GlyphPositioning)scalarsPerPos, ugly cast, and then 'uncast' in the callee. https://codereview.chromium.org/2241683005/diff/20001/src/pdf/SkPDFDevice.h File src/pdf/SkPDFDevice.h (right): https://codereview.chromium.org/2241683005/diff/20001/src/pdf/SkPDFDevice.h#n... src/pdf/SkPDFDevice.h:126: void drawTextInternal(const SkDraw&, const void*, size_t, const SkScalar pos[], Any particular reason for this to be public? https://codereview.chromium.org/2241683005/diff/20001/src/pdf/SkPDFDevice.h#n... src/pdf/SkPDFDevice.h:197: sk_sp<SkSurface> makeSurface(const SkImageInfo&, const SkSurfaceProps&) override; Since I was looking at visibility, these are protected, but the class is final, so these are private anyway. Not sure if there's anything one may wish to do about that though.
also renamed new method to keep with existing private methdos. https://codereview.chromium.org/2241683005/diff/20001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/2241683005/diff/20001/src/pdf/SkPDFDevice.cpp... src/pdf/SkPDFDevice.cpp:1193: SkScalar y = 2 == scalarsPerPos ? *pos++ : 0; On 2016/08/15 17:03:37, bungeman-skia wrote: > This is apparently the only place this 'scalarsPerPos' is used, why not just > replace it with 'positioning' and avoid the ugly, ugly cast where > 'scalarsPerPos' is currently set? Done. I was using it for direct indexing into the pos[] array, but then cleaned it up this far. https://codereview.chromium.org/2241683005/diff/20001/src/pdf/SkPDFDevice.cpp... src/pdf/SkPDFDevice.cpp:1219: this->drawTextInternal(d, text, len, pos, (SkTextBlob::GlyphPositioning)scalarsPerPos, On 2016/08/15 17:03:37, bungeman-skia wrote: > ugly cast, and then 'uncast' in the callee. I agree. But this will work nicely with SkDevice::drawTextBlob(). https://codereview.chromium.org/2241683005/diff/20001/src/pdf/SkPDFDevice.h File src/pdf/SkPDFDevice.h (right): https://codereview.chromium.org/2241683005/diff/20001/src/pdf/SkPDFDevice.h#n... src/pdf/SkPDFDevice.h:126: void drawTextInternal(const SkDraw&, const void*, size_t, const SkScalar pos[], On 2016/08/15 17:03:37, bungeman-skia wrote: > Any particular reason for this to be public? no. fixed. https://codereview.chromium.org/2241683005/diff/20001/src/pdf/SkPDFDevice.h#n... src/pdf/SkPDFDevice.h:197: sk_sp<SkSurface> makeSurface(const SkImageInfo&, const SkSurfaceProps&) override; On 2016/08/15 17:03:37, bungeman-skia wrote: > Since I was looking at visibility, these are protected, but the class is final, > so these are private anyway. Not sure if there's anything one may wish to do > about that though. Acknowledged.
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by halcanary@google.com
lgtm
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== SkPDF: unify drawText and drawPosText Motivation: a later CL will add drawTextBlob() (after https://crrev.com/2084533004 lands). This CL is designed with that change in mind. Also fewer redundant lines of code. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2241683005 ========== to ========== SkPDF: unify drawText and drawPosText Motivation: a later CL will add drawTextBlob() (after https://crrev.com/2084533004 lands). This CL is designed with that change in mind. Also fewer redundant lines of code. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2241683005 Committed: https://skia.googlesource.com/skia/+/6059dc32fe36358175cb81541c91e74a2a7e771a ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://skia.googlesource.com/skia/+/6059dc32fe36358175cb81541c91e74a2a7e771a
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:40001) has been created in https://codereview.chromium.org/2248923002/ by robertphillips@google.com. The reason for reverting is: I believe this is breaking the Google3 roll.
Message was sent while issue was closed.
Description was changed from ========== SkPDF: unify drawText and drawPosText Motivation: a later CL will add drawTextBlob() (after https://crrev.com/2084533004 lands). This CL is designed with that change in mind. Also fewer redundant lines of code. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2241683005 Committed: https://skia.googlesource.com/skia/+/6059dc32fe36358175cb81541c91e74a2a7e771a ========== to ========== SkPDF: unify drawText and drawPosText Motivation: a later CL will add drawTextBlob() (after https://crrev.com/2084533004 lands). This CL is designed with that change in mind. Also fewer redundant lines of code. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2241683005 Committed: https://skia.googlesource.com/skia/+/6059dc32fe36358175cb81541c91e74a2a7e771a ==========
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bungeman@google.com Link to the patchset: https://codereview.chromium.org/2241683005/#ps60001 (title: "stop assuming typeface has glyphs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== SkPDF: unify drawText and drawPosText Motivation: a later CL will add drawTextBlob() (after https://crrev.com/2084533004 lands). This CL is designed with that change in mind. Also fewer redundant lines of code. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2241683005 Committed: https://skia.googlesource.com/skia/+/6059dc32fe36358175cb81541c91e74a2a7e771a ========== to ========== SkPDF: unify drawText and drawPosText Motivation: a later CL will add drawTextBlob() (after https://crrev.com/2084533004 lands). This CL is designed with that change in mind. Also fewer redundant lines of code. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2241683005 Committed: https://skia.googlesource.com/skia/+/6059dc32fe36358175cb81541c91e74a2a7e771a Committed: https://skia.googlesource.com/skia/+/4ed2f01cf6f3a63d5185ea5b442549d20ce2ec16 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://skia.googlesource.com/skia/+/4ed2f01cf6f3a63d5185ea5b442549d20ce2ec16 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
