Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2416603002:
Add ability to compute text intercepts to Font (Closed)
|
Created:
4 years, 2 months ago by drott Modified:
4 years, 2 months ago CC:
ajuma+watch_chromium.org, blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, eae, f(malita), jbroman, Justin Novosad, pdr+graphicswatchlist_chromium.org, reed1, rwlbuis, Stephen Chennney Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd ability to compute text intercepts to Font
For text-decoration-skip: ink we need to have access to text intercepts,
i.e. figuring out where the line decoration intersects with the glyph
shapes. Skia offers this functionality and we need to integrate it with
our text and graphics code.
BUG=655166
Committed: https://crrev.com/b1221f6b6b59f582f07f7042bd3780e6b375c733
Cr-Commit-Position: refs/heads/master@{#425394}
Patch Set 1 #Patch Set 2 : Add ability to compute text intercepts to GraphicsContext #Patch Set 3 : Rebased #
Total comments: 10
Patch Set 4 : Addressing fmalita's review comments #
Total comments: 7
Patch Set 5 : Fix unused variable build error #Patch Set 6 : Nits addressed #
Messages
Total messages: 47 (31 generated)
The CQ bit was checked by drott@chromium.org to run a CQ dry run
drott@chromium.org changed reviewers: + fmalita@chromium.org, pdr@chromium.org
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: CLs for remote refs other than refs/pending/heads/master must contain NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them
The CQ bit was checked by drott@chromium.org 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: This issue passed the CQ dry run.
The CQ bit was checked by drott@chromium.org 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: This issue passed the CQ dry run.
Rebased
The CQ bit was checked by drott@chromium.org 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...
https://codereview.chromium.org/2416603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/Font.cpp (right): https://codereview.chromium.org/2416603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/Font.cpp:417: return false; false -> 0? https://codereview.chromium.org/2416603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/Font.h (right): https://codereview.chromium.org/2416603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/Font.h:111: int getTextIntercepts(SkCanvas*, SkCanvas param doesn't seem needed. https://codereview.chromium.org/2416603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/Font.h:116: SkScalar* intervals) const; I'm not necessarily opposed to following Skia's signature here, but it would also make sense to consider a higher-level, Blink-oriented API. How about something like struct Intercept { float mBegin, mEnd; }; bool getTextIntercepts(const TextRunPaintInfo&, float deviceScaleFactor, const SkPaint&, Vector<Intercept>& intercepts, std::tuple<float, float>& bounds); Then the caller wouldn't have to invoke twice, but instead the impl can hide that detail and resize the out vector as needed. https://codereview.chromium.org/2416603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/GraphicsContext.h (right): https://codereview.chromium.org/2416603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/GraphicsContext.h:266: int getTextIntercepts(const Font&, Is there much value in exposing this helper on GraphicsContext? In general, we've been trying to move state out of it for a while, so e.g. I'd like to no longer push the text paint to GC but instead use a local SkPaint at some point. I would think the Font API should suffice, even if for now we're going to grab the fill paint & DSF from GraphicsContext explicitly.
Description was changed from ========== Add ability to compute text intercepts to GraphicsContext For text-decoration-skip: ink we need to have access to text intercepts, i.e. figuring out where the line decoration intersects with the glyph shapes. Skia offers this functionality and we need to integrate it with our text and graphics code. BUG=655166 ========== to ========== Add ability to compute text intercepts to GraphicsContext For text-decoration-skip: ink we need to have access to text intercepts, i.e. figuring out where the line decoration intersects with the glyph shapes. Skia offers this functionality and we need to integrate it with our text and graphics code. BUG=655166 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm not the most familiar with this code but this seems reasonable % Florin's comments https://codereview.chromium.org/2416603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/Font.h (right): https://codereview.chromium.org/2416603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/Font.h:107: // two, and is at most the number of glyphs * 2 in TextRun part of Super nit: Add "the", remove the second period. "...number of glyphs * 2 in [the] TextRun part of TextRunPaintInfo."
Description was changed from ========== Add ability to compute text intercepts to GraphicsContext For text-decoration-skip: ink we need to have access to text intercepts, i.e. figuring out where the line decoration intersects with the glyph shapes. Skia offers this functionality and we need to integrate it with our text and graphics code. BUG=655166 ========== to ========== Add ability to compute text intercepts to Font For text-decoration-skip: ink we need to have access to text intercepts, i.e. figuring out where the line decoration intersects with the glyph shapes. Skia offers this functionality and we need to integrate it with our text and graphics code. BUG=655166 ==========
Addressing fmalita's review comments
The CQ bit was checked by drott@chromium.org 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...
Thanks for the thorough review. Moved the API to Font only, changed method signature to have a Vector<TextIntercepts> reference param and a tuple for bounds. Skia allocation pattern is now hidden in Font.cpp. https://codereview.chromium.org/2416603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/Font.cpp (right): https://codereview.chromium.org/2416603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/Font.cpp:417: return false; On 2016/10/13 at 18:09:19, f(malita) wrote: > false -> 0? Removed the return type. https://codereview.chromium.org/2416603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/Font.h (right): https://codereview.chromium.org/2416603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/Font.h:111: int getTextIntercepts(SkCanvas*, On 2016/10/13 at 18:09:19, f(malita) wrote: > SkCanvas param doesn't seem needed. Ah true, thanks - I initially thought I needed it, that's also why I placed this on GraphicsContext. Good then. https://codereview.chromium.org/2416603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/Font.h:116: SkScalar* intervals) const; On 2016/10/13 at 18:09:19, f(malita) wrote: > I'm not necessarily opposed to following Skia's signature here, but it would also make sense to consider a higher-level, Blink-oriented API. Good idea, looks better and less alien on the Blink side after changing that. https://codereview.chromium.org/2416603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/GraphicsContext.h (right): https://codereview.chromium.org/2416603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/GraphicsContext.h:266: int getTextIntercepts(const Font&, On 2016/10/13 at 18:09:19, f(malita) wrote: > Is there much value in exposing this helper on GraphicsContext? In general, we've been trying to move state out of it for a while, so e.g. I'd like to no longer push the text paint to GC but instead use a local SkPaint at some point. No, as mentioned above, I initially thought, I'd need the SkCanvas, but I don't. > I would think the Font API should suffice, even if for now we're going to grab the fill paint & DSF from GraphicsContext explicitly. Yes, sounds good to me, changed accordingly.
Thanks, pdr@, since the method signature changed, the "super nit" has been addressed. https://codereview.chromium.org/2416603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/Font.h (right): https://codereview.chromium.org/2416603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/Font.h:107: // two, and is at most the number of glyphs * 2 in TextRun part of On 2016/10/14 at 04:40:56, pdr. wrote: > Super nit: Add "the", remove the second period. > "...number of glyphs * 2 in [the] TextRun part of TextRunPaintInfo." I changed the signature of this method and the comment. So this is implicitly addressed.
Florin, could you submit to CQ if it looks good to you? Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
Thanks Dominik, LGTM w/ a couple of nits. https://codereview.chromium.org/2416603002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/Font.cpp (right): https://codereview.chromium.org/2416603002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/Font.cpp:449: int numIntervals = 0; nit: no need to decl/initialize numIntervals here AFAICT. https://codereview.chromium.org/2416603002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/Font.cpp:457: paint.getTextBlobIntercepts(runInfo.cachedTextBlob->get(), boundsArray, nit: not sure how common this is in practice, but it's a cheap test -- if numIntervals == 0, we can skip the second getTextBlobIntercepts call. (it's probably more interesting to skip the second getInterceptsFromBloberizer call below) https://codereview.chromium.org/2416603002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/Font.cpp:473: getInterceptsFromBloberizer(glyphBuffer, this, paint, deviceScaleFactor, nit: ditto
https://codereview.chromium.org/2416603002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/Font.cpp (right): https://codereview.chromium.org/2416603002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/Font.cpp:436: interceptsBuffer); Ah, the bots caught this one: I think we want offsetInterceptsBuffer here :)
Fix unused variable build error
The CQ bit was checked by drott@chromium.org 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...
Nits addressed
The CQ bit was checked by drott@chromium.org 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...
Thanks for the quick review, updated accordingly. https://codereview.chromium.org/2416603002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/fonts/Font.cpp (right): https://codereview.chromium.org/2416603002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/Font.cpp:449: int numIntervals = 0; On 2016/10/14 at 15:36:48, f(malita) wrote: > nit: no need to decl/initialize numIntervals here AFAICT. Yes, moved the second declaration down to line 468. https://codereview.chromium.org/2416603002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/Font.cpp:457: paint.getTextBlobIntercepts(runInfo.cachedTextBlob->get(), boundsArray, On 2016/10/14 at 15:36:48, f(malita) wrote: > nit: not sure how common this is in practice, but it's a cheap test -- if numIntervals == 0, we can skip the second getTextBlobIntercepts call. Done. https://codereview.chromium.org/2416603002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/fonts/Font.cpp:473: getInterceptsFromBloberizer(glyphBuffer, this, paint, deviceScaleFactor, On 2016/10/14 at 15:36:48, f(malita) wrote: > nit: ditto Done here as well.
The CQ bit was unchecked by drott@chromium.org
The CQ bit was checked by drott@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org Link to the patchset: https://codereview.chromium.org/2416603002/#ps100001 (title: "Nits addressed")
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.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add ability to compute text intercepts to Font For text-decoration-skip: ink we need to have access to text intercepts, i.e. figuring out where the line decoration intersects with the glyph shapes. Skia offers this functionality and we need to integrate it with our text and graphics code. BUG=655166 ========== to ========== Add ability to compute text intercepts to Font For text-decoration-skip: ink we need to have access to text intercepts, i.e. figuring out where the line decoration intersects with the glyph shapes. Skia offers this functionality and we need to integrate it with our text and graphics code. BUG=655166 Committed: https://crrev.com/b1221f6b6b59f582f07f7042bd3780e6b375c733 Cr-Commit-Position: refs/heads/master@{#425394} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b1221f6b6b59f582f07f7042bd3780e6b375c733 Cr-Commit-Position: refs/heads/master@{#425394} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
