Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
924543004:
adding baseline options for super/sub scripting (Closed)
Descriptionadding baseline options for super/sub scripting
This change list adds four options for a smaller font to be used as a style within the render text.
Option Example
------ -------
SUPERSCRIPT a mathematical exponent would be superscript
SUPERIOR 8th, the "th" would be superior script
INFERIOR 1/2, the "2" would be inferior ("1" is superior)
SUBSCRIPT H2O, the "2" would be subscript
Some imagination is needed to interpret the examples above. To see a clearer references to what is meant, this wikipedia article may be helpful: http://en.wikipedia.org/wiki/Subscript_and_superscript
These options may be set in the same way options like BOLD and ITALIC can currently be set, e.g. with a call to my_render_text->ApplyStyle(gfx::SUPERSCRIPT, true, gfx::Range(1, 3));
BUG=459812
Committed: https://crrev.com/eefeef59e4b44727e735e99733799a489b8a6efa
Cr-Commit-Position: refs/heads/master@{#319492}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Changing baseline styles from bool list to breaklist #
Total comments: 22
Patch Set 3 : restoring baselines after call to SetText #Patch Set 4 : manual merge to help try bots #
Total comments: 25
Patch Set 5 : Moved RestoreBreakList to file local function #
Total comments: 4
Patch Set 6 : merge update; adding test cases #Patch Set 7 : changed to ToRoundedInt #
Total comments: 4
Patch Set 8 : added rough bounds checking that is not yet correct #
Total comments: 12
Patch Set 9 : Refined DoesntClip and DoesClip tests #
Total comments: 9
Patch Set 10 : added debug print in test buffer #Patch Set 11 : changed draw rect test; added bug numbers #
Total comments: 16
Patch Set 12 : removed debug prints; cleaned up comments #
Total comments: 4
Patch Set 13 : Removed failing strings #Patch Set 14 : Removed another string #Patch Set 15 : commented out string failing on Mac #Patch Set 16 : added space test (for Win8 build) #
Total comments: 2
Patch Set 17 : enabling more tests depending on the platform #Patch Set 18 : unit test adjustment for mac text smoothing #
Total comments: 2
Patch Set 19 : changed ifdef #
Total comments: 6
Patch Set 20 : Adding SCOPED_TRACE #Patch Set 21 : Change for Win XP unittest #Messages
Total messages: 55 (10 generated)
dschuyler@chromium.org changed reviewers: + msw@chromium.org
https://codereview.chromium.org/924543004/diff/1/ui/gfx/render_text_harfbuzz.cc File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/924543004/diff/1/ui/gfx/render_text_harfbuzz.... ui/gfx/render_text_harfbuzz.cc:1167: // Calculate a slightly smaller font. nit: remove the line break in the comment. https://codereview.chromium.org/924543004/diff/1/ui/gfx/render_text_harfbuzz.... ui/gfx/render_text_harfbuzz.cc:1169: // look pretty good. Feel free to change this if someone on the UX team nit: omit the "Feel free" sentence. https://codereview.chromium.org/924543004/diff/1/ui/gfx/text_constants.h File ui/gfx/text_constants.h (right): https://codereview.chromium.org/924543004/diff/1/ui/gfx/text_constants.h#newc... ui/gfx/text_constants.h:35: // Figure of font metrics: nit: vertically center "height", "ascent", and "cap height". https://codereview.chromium.org/924543004/diff/1/ui/gfx/text_constants.h#newc... ui/gfx/text_constants.h:52: SUPERSCRIPT, // e.g. a mathematical exponent would be superscript I don't think you'll want to merge these directly with TextStyle. Instead add something like a BreakList<ScriptBaseline> RenderText::baseline_types_; or similar for a new enum of these script baseline types, including a NORMAL type. Otherwise, using a bool value for each style would allow users to set overlapping ranges with different script baseline styles. I imagine we don't (yet) want to handle combined/nested script baseline types (eg. a range marked true for both subscript and superscript, etc.).
Thanks for the feedback. I tried changing the baseline styles to a break list by mimicking the color break lists. https://codereview.chromium.org/924543004/diff/1/ui/gfx/render_text_harfbuzz.cc File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/924543004/diff/1/ui/gfx/render_text_harfbuzz.... ui/gfx/render_text_harfbuzz.cc:1167: // Calculate a slightly smaller font. On 2015/02/17 23:32:06, msw wrote: > nit: remove the line break in the comment. Done. https://codereview.chromium.org/924543004/diff/1/ui/gfx/render_text_harfbuzz.... ui/gfx/render_text_harfbuzz.cc:1169: // look pretty good. Feel free to change this if someone on the UX team On 2015/02/17 23:32:06, msw wrote: > nit: omit the "Feel free" sentence. Done. https://codereview.chromium.org/924543004/diff/1/ui/gfx/text_constants.h File ui/gfx/text_constants.h (right): https://codereview.chromium.org/924543004/diff/1/ui/gfx/text_constants.h#newc... ui/gfx/text_constants.h:35: // Figure of font metrics: On 2015/02/17 23:32:06, msw wrote: > nit: vertically center "height", "ascent", and "cap height". Done. https://codereview.chromium.org/924543004/diff/1/ui/gfx/text_constants.h#newc... ui/gfx/text_constants.h:52: SUPERSCRIPT, // e.g. a mathematical exponent would be superscript On 2015/02/17 23:32:06, msw wrote: > I don't think you'll want to merge these directly with TextStyle. Instead add > something like a BreakList<ScriptBaseline> RenderText::baseline_types_; or > similar for a new enum of these script baseline types, including a NORMAL type. > > Otherwise, using a bool value for each style would allow users to set > overlapping ranges with different script baseline styles. I imagine we don't > (yet) want to handle combined/nested script baseline types (eg. a range marked > true for both subscript and superscript, etc.). I'm a little less confident for the number of places I ended up adding baseline changes. Something I still haven't worked out the implications of is this, "Use an empty color BreakList to avoid breaking runs at color boundaries." I guess I'm wondering if I should do something similar with the baseline styles or not. Done.
https://codereview.chromium.org/924543004/diff/1/ui/gfx/text_constants.h File ui/gfx/text_constants.h (right): https://codereview.chromium.org/924543004/diff/1/ui/gfx/text_constants.h#newc... ui/gfx/text_constants.h:52: SUPERSCRIPT, // e.g. a mathematical exponent would be superscript On 2015/02/18 01:37:27, dschuyler wrote: > On 2015/02/17 23:32:06, msw wrote: > > I don't think you'll want to merge these directly with TextStyle. Instead add > > something like a BreakList<ScriptBaseline> RenderText::baseline_types_; or > > similar for a new enum of these script baseline types, including a NORMAL > type. > > > > Otherwise, using a bool value for each style would allow users to set > > overlapping ranges with different script baseline styles. I imagine we don't > > (yet) want to handle combined/nested script baseline types (eg. a range marked > > true for both subscript and superscript, etc.). > > I'm a little less confident for the number of places I ended up adding baseline > changes. Something I still haven't worked out the implications of is this, "Use > an empty color BreakList to avoid breaking runs at color boundaries." I guess > I'm wondering if I should do something similar with the baseline styles or not. > > Done. What you have looks pretty good. That comment is saying that we shouldn't break runs for text color ranges, so it doesn't use the color breaklist. Since we *should* break runs for baseline changes (because it uses a different font size), we'll use the actual baseline style breaklist there (as you currently do). https://codereview.chromium.org/924543004/diff/20001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/924543004/diff/20001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:431: // Adjust ranged styles and colors to accommodate a new text length. nit: Adjust ranged styles, baselines, and colors... https://codereview.chromium.org/924543004/diff/20001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:436: baselines_.SetMax(text_length); You'll need to clear the baseline values here, like we do for |styles_| below. Changes in text content need to clear any ranged styling that breaks runs, otherwise the old ranges might attempt to split multi-character graphemes (surrogate pairs, etc.) into different runs. https://codereview.chromium.org/924543004/diff/20001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1302: render_text->baselines_ = baselines_; With the change to clear the baseline values on SetText, you'll need to restore baselines below, like we do for |styles_|, ensuring that the breaks fall on valid cursor positions... I suggest making an improvement here: Create a helper function (from the contents of the style loop below) to fix-up a breaklist given a render text instance, ensuring the breaks fall on valid cursor indices. Then we can just call that helper for each of the |styles_| breaklists, and for the new |baselines_| breaklist. That said, this helper code is a bit naive and overly-cautious, the ranges should take the specific eliding into account. We should adjust ranges for ELIDE_MIDDLE and ELIDE_HEAD (shifting indices for text that was removed, so the ranges continue applying to the same characters)... But fixing that is a bit beyond what I'd ask of you in this CL. https://codereview.chromium.org/924543004/diff/20001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/924543004/diff/20001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:112: // Internal helper class used by derived classes to iterate colors and styles. nit: // Internal helper class used to iterate colors, baselines, and styles. https://codereview.chromium.org/924543004/diff/20001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:122: BaselineStyle baseline() const { return (BaselineStyle)baseline_->second; } nit: is the cast needed? if so, use static_cast instead of a C-style cast. https://codereview.chromium.org/924543004/diff/20001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:343: // Set the text baseline style over the entire text or a logical character nit: remove "text" for cleaner comment wrapping. https://codereview.chromium.org/924543004/diff/20001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:686: // Color and style breaks, used to color and stylize ranges of text. nit: "// Color, baseline, and style breaks, used to modify ranges of text." https://codereview.chromium.org/924543004/diff/20001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/924543004/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1155: if (run->baseline_type) { nit: explicitly compare if (run->baseline_type != NORMAL_BASELINE) { https://codereview.chromium.org/924543004/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1161: switch (run->baseline_type) { nit: I think you'll need a default case or explicitly handle each of the enum values (including NUM_BASELINE_STYLES), or you'll probably get clang warnings or similar compiler errors on some bots. https://codereview.chromium.org/924543004/diff/20001/ui/gfx/text_constants.h File ui/gfx/text_constants.h (right): https://codereview.chromium.org/924543004/diff/20001/ui/gfx/text_constants.h#... ui/gfx/text_constants.h:45: // Text baseline offset types nit: add trailing periods here and to the eg. comments below. https://codereview.chromium.org/924543004/diff/20001/ui/gfx/text_constants.h#... ui/gfx/text_constants.h:53: // | |--------+------------+-----------| SUBSCRIPT | nit q: should "SUBSCRIPT" be on the line below?
https://codereview.chromium.org/924543004/diff/20001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/924543004/diff/20001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:431: // Adjust ranged styles and colors to accommodate a new text length. On 2015/02/18 17:07:23, msw wrote: > nit: Adjust ranged styles, baselines, and colors... Done. https://codereview.chromium.org/924543004/diff/20001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:436: baselines_.SetMax(text_length); On 2015/02/18 17:07:23, msw wrote: > You'll need to clear the baseline values here, like we do for |styles_| below. > Changes in text content need to clear any ranged styling that breaks runs, > otherwise the old ranges might attempt to split multi-character graphemes > (surrogate pairs, etc.) into different runs. Done. https://codereview.chromium.org/924543004/diff/20001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1302: render_text->baselines_ = baselines_; On 2015/02/18 17:07:23, msw wrote: > With the change to clear the baseline values on SetText, you'll need to restore > baselines below, like we do for |styles_|, ensuring that the breaks fall on > valid cursor positions... I suggest making an improvement here: Create a helper > function (from the contents of the style loop below) to fix-up a breaklist given > a render text instance, ensuring the breaks fall on valid cursor indices. Then > we can just call that helper for each of the |styles_| breaklists, and for the > new |baselines_| breaklist. > > That said, this helper code is a bit naive and overly-cautious, the ranges > should take the specific eliding into account. We should adjust ranges for > ELIDE_MIDDLE and ELIDE_HEAD (shifting indices for text that was removed, so the > ranges continue applying to the same characters)... But fixing that is a bit > beyond what I'd ask of you in this CL. I moved the restore break list code out to a separate function. I left the last bit that sounds like some decent optimization opportunities for a later day. :) Done. https://codereview.chromium.org/924543004/diff/20001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/924543004/diff/20001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:112: // Internal helper class used by derived classes to iterate colors and styles. On 2015/02/18 17:07:23, msw wrote: > nit: // Internal helper class used to iterate colors, baselines, and styles. Done. https://codereview.chromium.org/924543004/diff/20001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:122: BaselineStyle baseline() const { return (BaselineStyle)baseline_->second; } On 2015/02/18 17:07:23, msw wrote: > nit: is the cast needed? if so, use static_cast instead of a C-style cast. Done. https://codereview.chromium.org/924543004/diff/20001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:343: // Set the text baseline style over the entire text or a logical character On 2015/02/18 17:07:23, msw wrote: > nit: remove "text" for cleaner comment wrapping. Done. https://codereview.chromium.org/924543004/diff/20001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:686: // Color and style breaks, used to color and stylize ranges of text. On 2015/02/18 17:07:23, msw wrote: > nit: "// Color, baseline, and style breaks, used to modify ranges of text." Done. https://codereview.chromium.org/924543004/diff/20001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/924543004/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1155: if (run->baseline_type) { On 2015/02/18 17:07:23, msw wrote: > nit: explicitly compare if (run->baseline_type != NORMAL_BASELINE) { Done. https://codereview.chromium.org/924543004/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1161: switch (run->baseline_type) { On 2015/02/18 17:07:23, msw wrote: > nit: I think you'll need a default case or explicitly handle each of the enum > values (including NUM_BASELINE_STYLES), or you'll probably get clang warnings or > similar compiler errors on some bots. I moved the INFERIOR case to fall through to the new default case. Done. https://codereview.chromium.org/924543004/diff/20001/ui/gfx/text_constants.h File ui/gfx/text_constants.h (right): https://codereview.chromium.org/924543004/diff/20001/ui/gfx/text_constants.h#... ui/gfx/text_constants.h:45: // Text baseline offset types On 2015/02/18 17:07:23, msw wrote: > nit: add trailing periods here and to the eg. comments below. Done. https://codereview.chromium.org/924543004/diff/20001/ui/gfx/text_constants.h#... ui/gfx/text_constants.h:53: // | |--------+------------+-----------| SUBSCRIPT | On 2015/02/18 17:07:23, msw wrote: > nit q: should "SUBSCRIPT" be on the line below? Done.
Fix up Mac (no-op is probably okay for now), add unit tests, and it would be nice if you added a way to try this via Views Examples (but maybe that can wait if you're planning to expose this via views::Label). https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1287: void RenderText::RestoreBreakList(RenderText* render_text, Make this a file-local function in the anon namespace, it doesn't use any private/protected RenderText members (if you use text() instead of text_). https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1288: T& break_list) { Make this a BreakList<T>, other types shouldn't be handled here. https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1378: // Restore styles. Make sure style ranges don't break new text graphemes. nit: combine comments: "Restore styles and baselines without breaking multi-character graphemes." https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1381: BreakList<bool>& break_list = render_text->styles_[style]; nit: inline render_text->styles_[style] below, remove curly braces. https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:112: // Internal helper class used by derived classes to iterate colors, baselines, nit: remove "by derived classes" for a one-liner. https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:338: // Set the baseline style over the entire text or a logical character range. nit: remove the extra space before "baseline" https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:641: // does break a grapheme in |render_text|, the range will be slightly nit: remove the double space before "grapheme". https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1133: // Build the list of runs from the script items and ranged styles. Use an nit: "Build the run list from the script items and ranges styles and baselines." https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1218: run->font_size = round(primary_font.GetFontSize() * ratio); nit: I don't think round is commonly used in the Chromium codebase... Perhaps use std::ceil(foo + 0.5) here and for SUPERIOR below? https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1232: case INFERIOR: // fall through nit: trailing period. https://codereview.chromium.org/924543004/diff/60001/ui/gfx/text_constants.h File ui/gfx/text_constants.h (right): https://codereview.chromium.org/924543004/diff/60001/ui/gfx/text_constants.h#... ui/gfx/text_constants.h:62: NUM_BASELINE_STYLES, nit: this isn't actually needed, you can safely remove this for now.
Also, don't forget to set BUG=
https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1287: void RenderText::RestoreBreakList(RenderText* render_text, On 2015/02/18 23:32:08, msw wrote: > Make this a file-local function in the anon namespace, it doesn't use any > private/protected RenderText members (if you use text() instead of text_). Done. https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1288: T& break_list) { On 2015/02/18 23:32:08, msw wrote: > Make this a BreakList<T>, other types shouldn't be handled here. Done. https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1378: // Restore styles. Make sure style ranges don't break new text graphemes. On 2015/02/18 23:32:08, msw wrote: > nit: combine comments: "Restore styles and baselines without breaking > multi-character graphemes." Done. https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1381: BreakList<bool>& break_list = render_text->styles_[style]; On 2015/02/18 23:32:08, msw wrote: > nit: inline render_text->styles_[style] below, remove curly braces. Done. https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:112: // Internal helper class used by derived classes to iterate colors, baselines, On 2015/02/18 23:32:08, msw wrote: > nit: remove "by derived classes" for a one-liner. Done. https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:338: // Set the baseline style over the entire text or a logical character range. On 2015/02/18 23:32:08, msw wrote: > nit: remove the extra space before "baseline" Done. https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:641: // does break a grapheme in |render_text|, the range will be slightly On 2015/02/18 23:32:08, msw wrote: > nit: remove the double space before "grapheme". Done. https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1133: // Build the list of runs from the script items and ranged styles. Use an On 2015/02/18 23:32:08, msw wrote: > nit: "Build the run list from the script items and ranges styles and baselines." Done. https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1218: run->font_size = round(primary_font.GetFontSize() * ratio); On 2015/02/18 23:32:08, msw wrote: > nit: I don't think round is commonly used in the Chromium codebase... > Perhaps use std::ceil(foo + 0.5) here and for SUPERIOR below? That gets a bit confusing/less readable because it needs to be floor if less than zero and ceil if greater or equal to zero. Also, with ceil, the 0.5 would need to be subtracted (which is weird looking). Would you consider std::round as a replacement for round()? That way it's using the C++ call (and I find it easier to read that ceil). https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1232: case INFERIOR: // fall through On 2015/02/18 23:32:08, msw wrote: > nit: trailing period. Done. https://codereview.chromium.org/924543004/diff/60001/ui/gfx/text_constants.h File ui/gfx/text_constants.h (right): https://codereview.chromium.org/924543004/diff/60001/ui/gfx/text_constants.h#... ui/gfx/text_constants.h:62: NUM_BASELINE_STYLES, On 2015/02/18 23:32:08, msw wrote: > nit: this isn't actually needed, you can safely remove this for now. Done.
Let me know when you've fixed Mac, added unit tests, and optionally added a way to demo this through Views Examples. https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1218: run->font_size = round(primary_font.GetFontSize() * ratio); On 2015/02/19 00:29:15, dschuyler wrote: > On 2015/02/18 23:32:08, msw wrote: > > nit: I don't think round is commonly used in the Chromium codebase... > > Perhaps use std::ceil(foo + 0.5) here and for SUPERIOR below? > > That gets a bit confusing/less readable because it needs to be floor if less > than zero and ceil if greater or equal to zero. Also, with ceil, the 0.5 would > need to be subtracted (which is weird looking). Would you consider std::round > as a replacement for round()? That way it's using the C++ call (and I find it > easier to read that ceil). std::round only seems to be used in 4 places in the codebase. I'm not sure why that isn't used more widely, but it makes me hesitant to recommend its use... Maybe use gfx::ToRoundedInt from ui/gfx/geometry/safe_integer_conversions.h. I wonder what other places use that naive code snippet and have bugs for not handling negative numbers correctly... +CC pkasting for advice. https://codereview.chromium.org/924543004/diff/80001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/924543004/diff/80001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1286: namespace { Don't declare an anonymous namespace in the middle of the file, move this function to the existing anonymous namespace at the top of the file. https://codereview.chromium.org/924543004/diff/80001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/924543004/diff/80001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1133: // Build the run list from the script items and ranges styles and baselines. nit: d'oh sorry, s/ranges/ranged/... my fault!
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1218: run->font_size = round(primary_font.GetFontSize() * ratio); On 2015/02/19 02:50:57, msw wrote: > On 2015/02/19 00:29:15, dschuyler wrote: > > On 2015/02/18 23:32:08, msw wrote: > > > nit: I don't think round is commonly used in the Chromium codebase... > > > Perhaps use std::ceil(foo + 0.5) here and for SUPERIOR below? > > > > That gets a bit confusing/less readable because it needs to be floor if less > > than zero and ceil if greater or equal to zero. Also, with ceil, the 0.5 > would > > need to be subtracted (which is weird looking). Would you consider std::round > > as a replacement for round()? That way it's using the C++ call (and I find it > > easier to read that ceil). > > std::round only seems to be used in 4 places in the codebase. I'm not sure why > that isn't used more widely, but it makes me hesitant to recommend its use... > Maybe use gfx::ToRoundedInt from ui/gfx/geometry/safe_integer_conversions.h. I > wonder what other places use that naive code snippet and have bugs for not > handling negative numbers correctly... +CC pkasting for advice. std::round is C++11, and since we can't yet use the C++11 standard library, it's out. round(), without the std:: qualifier, is also C++11 AFAICT, but was in C99 and thus likely has such broad compiler support that the couple dozen places in the codebase that reference it "happen to work". ToRoundedInt() is probably the best answer, and is nicer than round() anyway in that it clamps to an int for you. Incidentally, some places in our code round away from zero (the default behavior of round() and friends) while some round toward positive infinity ("floor(x + 0.5)"). This distinction may look like a bug, but at least in the spots I've tried to "fix" it in the past, it turned out we were doing what we were because of explicit specs to that effect, e.g. I guess CSS rounding is specified as always being toward positive infinity. Dumb.
On 2015/02/19 07:27:46, Peter Kasting wrote: > https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text_harfb... > File ui/gfx/render_text_harfbuzz.cc (right): > > https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text_harfb... > ui/gfx/render_text_harfbuzz.cc:1218: run->font_size = > round(primary_font.GetFontSize() * ratio); > On 2015/02/19 02:50:57, msw wrote: > > On 2015/02/19 00:29:15, dschuyler wrote: > > > On 2015/02/18 23:32:08, msw wrote: > > > > nit: I don't think round is commonly used in the Chromium codebase... > > > > Perhaps use std::ceil(foo + 0.5) here and for SUPERIOR below? > > > > > > That gets a bit confusing/less readable because it needs to be floor if less > > > than zero and ceil if greater or equal to zero. Also, with ceil, the 0.5 > > would > > > need to be subtracted (which is weird looking). Would you consider > std::round > > > as a replacement for round()? That way it's using the C++ call (and I find > it > > > easier to read that ceil). > > > > std::round only seems to be used in 4 places in the codebase. I'm not sure why > > that isn't used more widely, but it makes me hesitant to recommend its use... > > Maybe use gfx::ToRoundedInt from ui/gfx/geometry/safe_integer_conversions.h. I > > wonder what other places use that naive code snippet and have bugs for not > > handling negative numbers correctly... +CC pkasting for advice. > > std::round is C++11, and since we can't yet use the C++11 standard library, it's > out. round(), without the std:: qualifier, is also C++11 AFAICT, but was in C99 > and thus likely has such broad compiler support that the couple dozen places in > the codebase that reference it "happen to work". ToRoundedInt() is probably the > best answer, and is nicer than round() anyway in that it clamps to an int for > you. > > Incidentally, some places in our code round away from zero (the default behavior > of round() and friends) while some round toward positive infinity ("floor(x + > 0.5)"). This distinction may look like a bug, but at least in the spots I've > tried to "fix" it in the past, it turned out we were doing what we were because > of explicit specs to that effect, e.g. I guess CSS rounding is specified as > always being toward positive infinity. Dumb. Awesome, thanks for the description of the issues. I'm switching to ToRoundedInt now. On the unit tests: I'm looking at render_text_unittest.cc and thinking of patterning the baselines tests after the color break list tests (since the code pattern is also very similar).
On 2015/02/19 19:47:11, dschuyler wrote: > On the unit tests: I'm looking at render_text_unittest.cc and thinking of > patterning the baselines tests after the color break list tests (since the code > pattern is also very similar). That might be an okay start, but the baselines will break runs, unlike the colors, so you might want to test that explicitly. It'd also be nice to roughly test the effects of using the baseline types; setting a range of text to use any of the baseline types should not affect the overall height or baseline of the RenderText [line] (right?), but it should have a width smaller than the same text with a normal baseline style. You could further check the relative size/position of glyph bounds for each style, but maybe that's a bit excessive.
Hi Mike, I've got some more changes up. There was a delay while I worked through some errors I made merging. I think I've got it straightened out. I have some unit test code added, yet there is another test that is not part of this check in. That test checks that pixels are not written outside of the reported text bounds. That test fails. The code does write above the font ascent, but not above the top of the font. There is a comment related to the font height at https://code.google.com/p/chromium/issues/detail?id=459812#c2 Also, the width of the rendered text can be slightly less than the reported width when a superscript or subscript is used. I may be misreading the comment about what would be excessive to do prior to landing this change list. Can you help me know whether the two issues above are show stoppers for this change list https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/924543004/diff/60001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1218: run->font_size = round(primary_font.GetFontSize() * ratio); On 2015/02/19 07:27:46, Peter Kasting wrote: > On 2015/02/19 02:50:57, msw wrote: > > On 2015/02/19 00:29:15, dschuyler wrote: > > > On 2015/02/18 23:32:08, msw wrote: > > > > nit: I don't think round is commonly used in the Chromium codebase... > > > > Perhaps use std::ceil(foo + 0.5) here and for SUPERIOR below? > > > > > > That gets a bit confusing/less readable because it needs to be floor if less > > > than zero and ceil if greater or equal to zero. Also, with ceil, the 0.5 > > would > > > need to be subtracted (which is weird looking). Would you consider > std::round > > > as a replacement for round()? That way it's using the C++ call (and I find > it > > > easier to read that ceil). > > > > std::round only seems to be used in 4 places in the codebase. I'm not sure why > > that isn't used more widely, but it makes me hesitant to recommend its use... > > Maybe use gfx::ToRoundedInt from ui/gfx/geometry/safe_integer_conversions.h. I > > wonder what other places use that naive code snippet and have bugs for not > > handling negative numbers correctly... +CC pkasting for advice. > > std::round is C++11, and since we can't yet use the C++11 standard library, it's > out. round(), without the std:: qualifier, is also C++11 AFAICT, but was in C99 > and thus likely has such broad compiler support that the couple dozen places in > the codebase that reference it "happen to work". ToRoundedInt() is probably the > best answer, and is nicer than round() anyway in that it clamps to an int for > you. > > Incidentally, some places in our code round away from zero (the default behavior > of round() and friends) while some round toward positive infinity ("floor(x + > 0.5)"). This distinction may look like a bug, but at least in the spots I've > tried to "fix" it in the past, it turned out we were doing what we were because > of explicit specs to that effect, e.g. I guess CSS rounding is specified as > always being toward positive infinity. Dumb. Done. https://codereview.chromium.org/924543004/diff/80001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/924543004/diff/80001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1286: namespace { On 2015/02/19 02:50:57, msw wrote: > Don't declare an anonymous namespace in the middle of the file, move this > function to the existing anonymous namespace at the top of the file. Done. https://codereview.chromium.org/924543004/diff/80001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/924543004/diff/80001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1133: // Build the run list from the script items and ranges styles and baselines. On 2015/02/19 02:50:57, msw wrote: > nit: d'oh sorry, s/ranges/ranged/... my fault! Done.
On 2015/02/25 02:23:06, dschuyler wrote: > I have some unit test code added, yet there is another test that is not part of > this check in. That test checks that pixels are not written outside of the > reported text bounds. That test fails. The code does write above the font > ascent, but not above the top of the font. There is a comment related to the > font height at https://code.google.com/p/chromium/issues/detail?id=459812#c2 > > Also, the width of the rendered text can be slightly less than the reported > width when a superscript or subscript is used. > > I may be misreading the comment about what would be excessive to do prior to > landing this change list. Can you help me know whether the two issues above are > show stoppers for this change list You should try to get the line heights and widths correct with styled baselines, but they aren't necessarily strict blockers. Take a look at mukai's CL to address the height issues you pointed out for <http://crbug.com/459812>: <http://codereview.chromium.org/943093002>. You might need to modify HarfBuzzLineBreaker to add the baseline offset to the font height or similar. And I'm not sure why the width would be incorrect... can you investigate? https://codereview.chromium.org/924543004/diff/120001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/924543004/diff/120001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:1352: render_text->baselines_ = baselines_; I think you can remove this with the call to RestoreBreakList below. https://codereview.chromium.org/924543004/diff/120001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/924543004/diff/120001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:247: // resulting breaks (INT_MAX should be used instead of the text length for nit: remove the extra space before the parens. You can optionally leave this comment, and the one at line 237, as they were.
Thanks again for looking that over.
Ah, the comment on the text width was a misunderstanding on my part. There
seems to always be one or two pixel columns after the text. I thought that was
the superscript miscalculating. Here's an enlargement of a couple of WW's for
example ('-' represents the background color):
----------------------------
----------------------------
----------------------------
@@@--@@@--@@@@@@--@@@--@@@--
@@@--@@@--@@@@@@--@@@--@@@--
@@@-@@@@--@@--@@--@@@@-@@@--
-@@-@@@@@@@@--@@@@@@@@-@@---
-@@@@@-@@@@@--@@@@@-@@@@@---
-@@@@@-@@@@@--@@@@@-@@@@@---
-@@@@@-@@@@---@@@@@-@@@@@---
-@@@@@-@@@@----@@@@-@@@@@---
--@@@--@@@@----@@@@--@@@----
----------------------------
----------------------------
----------------------------
^pixels that are in the rect, though past all text
pixels.
^iff my math is correct, this is the vertical line
that is allowed for "one column of anti-aliased pixels past the expected width."
The W's above are bold, by the way.
The extra vertical pixels seem to be there regardless of whether the string
contains super/subscript. So it looks like that may be normal.
I've also checked in that other unit test. There is a comment in there on a
SetDisplayRect that should be removed to really do the test. With the
SetDisplayRect the pixels will be clipped by the display rect (rather than
having the text really fit in the space). Which rect should I be using to
determine the correct height to measure against?
https://codereview.chromium.org/924543004/diff/120001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):
https://codereview.chromium.org/924543004/diff/120001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1352: render_text->baselines_ = baselines_;
On 2015/02/25 16:02:33, msw wrote:
> I think you can remove this with the call to RestoreBreakList below.
Done.
https://codereview.chromium.org/924543004/diff/120001/ui/gfx/render_text_unit...
File ui/gfx/render_text_unittest.cc (right):
https://codereview.chromium.org/924543004/diff/120001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:247: // resulting breaks (INT_MAX should be used
instead of the text length for
On 2015/02/25 16:02:33, msw wrote:
> nit: remove the extra space before the parens. You can optionally leave this
> comment, and the one at line 237, as they were.
Done.
https://codereview.chromium.org/924543004/diff/140001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/924543004/diff/140001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2569: surface->peekPixels(NULL, NULL)); nit: nullptr https://codereview.chromium.org/924543004/diff/140001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2587: TEST_F(RenderTextTest, TextBaselineStyleDoesntClip) { It might be nice to combine the two tests, or maybe extract some helpers. https://codereview.chromium.org/924543004/diff/140001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2604: render_text->SetHorizontalAlignment(gfx::ALIGN_LEFT); nit: you can remove all gfx:: qualifiers in this test. https://codereview.chromium.org/924543004/diff/140001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2616: Rect(kTestSize, 0, kCanvasSize.width() - kTestSize, Shouldn't the width be render_text->GetStringSize().width() + kTestSize * 2? https://codereview.chromium.org/924543004/diff/140001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2629: static_cast<const uint32*>(surface->peekPixels(NULL, NULL)); nit: nullptr https://codereview.chromium.org/924543004/diff/140001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2632: for (int y = 0; y < kTestSize; ++y) { I don't think you can assume the text will be vertically centered like this because RenderText uses cap-height centering. If the internal leading and the descent (descender + external leading?) are not equal, the text's vertical position will not match these expectations, afaik. Maybe try something like: // Allow the RenderText to paint outside of its display rect. (you should verify that this really still works as advertised, maybe set a super small display rect and ensure text is painted outside that rect) render_text->set_clip_to_display_rect(false); // Set the text size to exactly what the RenderText requests (maybe smaller or 0 size to ensure the above flag works). Rect text_rect(kTestSize, kTestSize, render_text->GetStringSize().width(), render_text->GetStringSize().height()) render_text->SetDisplayRect(text_rect); // Ensure nothing is painted outside of the following rects (x, y, w, h): EnsureSolidRect(SK_ColorWHITE, 0, 0, kCanvasSize.width(), kTestSize); // top EnsureSolidRect(SK_ColorWHITE, 0, text_rect.bottom(), kCanvasSize.width(), kTestSize); // bottom EnsureSolidRect(SK_ColorWHITE, 0, 0, kTestSize, kCanvasSize.height()); // left EnsureSolidRect(SK_ColorWHITE, text_rect.right() + 1?, 0, kTestSize, kCanvasSize.height()); // right (with 1 px for AA) // Ensure the pixel just right of the rect only contains AA pixels...
While reworking the clip tests, I've removed the test for the vertical pixel column of text smoothing/anti-aliasing because it appears that there is already extra space at the end of the string rectangle to allow for that. I'd like to test whether the extra allowance in the unit test was an artifact from a previous version of the code and may no longer apply. I'm not sure if this is the right approach to test this though (so I'd like to call this out for suggestions). https://codereview.chromium.org/924543004/diff/140001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/924543004/diff/140001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2569: surface->peekPixels(NULL, NULL)); On 2015/02/26 03:36:54, msw wrote: > nit: nullptr Done. https://codereview.chromium.org/924543004/diff/140001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2587: TEST_F(RenderTextTest, TextBaselineStyleDoesntClip) { On 2015/02/26 03:36:53, msw wrote: > It might be nice to combine the two tests, or maybe extract some helpers. I realized that TextDoesntClipt was a subset of TextBaselineDoesntClip while looking at consolidating them. A test for TextDoesClip has also been added. https://codereview.chromium.org/924543004/diff/140001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2604: render_text->SetHorizontalAlignment(gfx::ALIGN_LEFT); On 2015/02/26 03:36:53, msw wrote: > nit: you can remove all gfx:: qualifiers in this test. Done. https://codereview.chromium.org/924543004/diff/140001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2616: Rect(kTestSize, 0, kCanvasSize.width() - kTestSize, On 2015/02/26 03:36:54, msw wrote: > Shouldn't the width be render_text->GetStringSize().width() + kTestSize * 2? If the intent were to clip to the rect, yes. The intent was just to avoid going off of the canvas. It's good for this test to allow freedom to expose errors. The reason I was using the * 2 on the height is because I don't know a way to control the vertical alignment other than manipulating the rect. It would be much nicer if I could simply vertically align top, for example. https://codereview.chromium.org/924543004/diff/140001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2629: static_cast<const uint32*>(surface->peekPixels(NULL, NULL)); On 2015/02/26 03:36:53, msw wrote: > nit: nullptr Done. https://codereview.chromium.org/924543004/diff/140001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2632: for (int y = 0; y < kTestSize; ++y) { On 2015/02/26 03:36:53, msw wrote: > I don't think you can assume the text will be vertically centered like this > because RenderText uses cap-height centering. If the internal leading and the > descent (descender + external leading?) are not equal, the text's vertical > position will not match these expectations, afaik. Maybe try something like: > // Allow the RenderText to paint outside of its display rect. (you should > verify that this really still works as advertised, maybe set a super small > display rect and ensure text is painted outside that rect) > render_text->set_clip_to_display_rect(false); > // Set the text size to exactly what the RenderText requests (maybe smaller or > 0 size to ensure the above flag works). > Rect text_rect(kTestSize, kTestSize, render_text->GetStringSize().width(), > render_text->GetStringSize().height()) > render_text->SetDisplayRect(text_rect); > > // Ensure nothing is painted outside of the following rects (x, y, w, h): > EnsureSolidRect(SK_ColorWHITE, 0, 0, kCanvasSize.width(), kTestSize); // top > EnsureSolidRect(SK_ColorWHITE, 0, text_rect.bottom(), kCanvasSize.width(), > kTestSize); // bottom > EnsureSolidRect(SK_ColorWHITE, 0, 0, kTestSize, kCanvasSize.height()); // left > EnsureSolidRect(SK_ColorWHITE, text_rect.right() + 1?, 0, kTestSize, > kCanvasSize.height()); // right (with 1 px for AA) > // Ensure the pixel just right of the rect only contains AA pixels... Thanks for pointing out the clip to display rect false call. Done.
On 2015/02/26 23:15:56, dschuyler wrote: > While reworking the clip tests, I've removed the test for the vertical pixel > column of text smoothing/anti-aliasing because it appears that there is already > extra space at the end of the string rectangle to allow for that. I'd like to > test whether the extra allowance in the unit test was an artifact from a > previous version of the code and may no longer apply. I'm not sure if this is > the right approach to test this though (so I'd like to call this out for > suggestions). Is that because your test clips to the tight display rect, unlike the old one? https://codereview.chromium.org/924543004/diff/160001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/924543004/diff/160001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:39: class TestRectangleBuffer { nit: move to the end of the namespace, or at least below conts, add a comment. https://codereview.chromium.org/924543004/diff/160001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:61: }; nit: disallow copy and assign https://codereview.chromium.org/924543004/diff/160001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:65: void TestRectangleBuffer::EnsureSolidRect(SkColor color, nit: define this inline with the declaration above. https://codereview.chromium.org/924543004/diff/160001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2623: // ADD(dschuyler): This should be added because without it there is a fake We shouldn't land this as-is... If we can't make a proper fix, we should just test strings that pass with set_clip_to_display_rect(false), commenting others out with TODOs citing the sizing bugs # (the a-ring and underscore bug, and a bug to track styled baseline sizing). I imagine the cases from the old TextDoesntClip would pass that test (at least with normal baselines).
https://codereview.chromium.org/924543004/diff/160001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/924543004/diff/160001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:39: class TestRectangleBuffer { On 2015/02/27 01:28:19, msw wrote: > nit: move to the end of the namespace, or at least below conts, add a comment. Done. https://codereview.chromium.org/924543004/diff/160001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:61: }; On 2015/02/27 01:28:19, msw wrote: > nit: disallow copy and assign Done. https://codereview.chromium.org/924543004/diff/160001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:65: void TestRectangleBuffer::EnsureSolidRect(SkColor color, On 2015/02/27 01:28:19, msw wrote: > nit: define this inline with the declaration above. Done. https://codereview.chromium.org/924543004/diff/160001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2623: // ADD(dschuyler): This should be added because without it there is a fake On 2015/02/27 01:28:19, msw wrote: > We shouldn't land this as-is... If we can't make a proper fix, we should just > test strings that pass with set_clip_to_display_rect(false), commenting others > out with TODOs citing the sizing bugs # (the a-ring and underscore bug, and a > bug to track styled baseline sizing). I imagine the cases from the old > TextDoesntClip would pass that test (at least with normal baselines). I've added in some test print code for this CL (which we can leave in or I can remove it, no problem). Using the debug print it's easier to see what is printing out of the GetStringSize bounds. I also commented out the strings that show the bugs and put a comment with the bug number. Done.
https://codereview.chromium.org/924543004/diff/160001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/924543004/diff/160001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2623: // ADD(dschuyler): This should be added because without it there is a fake On 2015/02/27 03:05:35, dschuyler wrote: > On 2015/02/27 01:28:19, msw wrote: > > We shouldn't land this as-is... If we can't make a proper fix, we should just > > test strings that pass with set_clip_to_display_rect(false), commenting others > > out with TODOs citing the sizing bugs # (the a-ring and underscore bug, and a > > bug to track styled baseline sizing). I imagine the cases from the old > > TextDoesntClip would pass that test (at least with normal baselines). > > I've added in some test print code for this CL (which we can leave in or I can > remove it, no problem). > Using the debug print it's easier to see what is printing out of the > GetStringSize bounds. > I also commented out the strings that show the bugs and put a comment with the > bug number. > > Done. Remove print debugging code, but feel free to share that with mukai. https://codereview.chromium.org/924543004/diff/200001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/924543004/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:424: line->size.set_width(line->size.width() + width); nit: add a comment like: // TODO(dschuyler): Account for stylized baselines in string sizing. https://codereview.chromium.org/924543004/diff/200001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/924543004/diff/200001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:178: // Test whether any values in the rectangular area are anything other than nit: s/whether/if/ for a one-liner. https://codereview.chromium.org/924543004/diff/200001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2618: // BUG=459812 The underscore will draw outside of the GetStringSize rect. // TODO(mukai): Underscores draw outside GetStringSize; crbug.com/459812 https://codereview.chromium.org/924543004/diff/200001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2623: // BUG=459812 The A-Ring will draw outside of the GetStringSize rect. // TODO(mukai): A-Ring draws outside GetStringSize; crbug.com/459812 https://codereview.chromium.org/924543004/diff/200001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2635: render_text->SetDisplayRect(Rect(kCanvasSize)); I think you can remove this and rely on the calls in the loop below. https://codereview.chromium.org/924543004/diff/200001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2648: render_text->GetStringSize().width(), nit: cache a Size string_size for use throughout. https://codereview.chromium.org/924543004/diff/200001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2698: render_text->SetDisplayRect(Rect(kCanvasSize)); ditto
https://codereview.chromium.org/924543004/diff/200001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/924543004/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:424: line->size.set_width(line->size.width() + width); On 2015/02/27 03:48:05, msw wrote: > nit: add a comment like: // TODO(dschuyler): Account for stylized baselines in > string sizing. I may have been confusing with my mentioning that I thought the width calculation had an error. Upon further research it looks like it is good. I'm guessing this comment comes from that confusion on my part. If it's talking about something else, could you help me with more detail about the thing to fix? https://codereview.chromium.org/924543004/diff/200001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/924543004/diff/200001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:178: // Test whether any values in the rectangular area are anything other than On 2015/02/27 03:48:06, msw wrote: > nit: s/whether/if/ for a one-liner. Done. https://codereview.chromium.org/924543004/diff/200001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2618: // BUG=459812 The underscore will draw outside of the GetStringSize rect. On 2015/02/27 03:48:06, msw wrote: > // TODO(mukai): Underscores draw outside GetStringSize; crbug.com/459812 Done. https://codereview.chromium.org/924543004/diff/200001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2623: // BUG=459812 The A-Ring will draw outside of the GetStringSize rect. On 2015/02/27 03:48:06, msw wrote: > // TODO(mukai): A-Ring draws outside GetStringSize; crbug.com/459812 Done. https://codereview.chromium.org/924543004/diff/200001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2635: render_text->SetDisplayRect(Rect(kCanvasSize)); On 2015/02/27 03:48:06, msw wrote: > I think you can remove this and rely on the calls in the loop below. Done. https://codereview.chromium.org/924543004/diff/200001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2648: render_text->GetStringSize().width(), On 2015/02/27 03:48:06, msw wrote: > nit: cache a Size string_size for use throughout. Done. https://codereview.chromium.org/924543004/diff/200001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2698: render_text->SetDisplayRect(Rect(kCanvasSize)); On 2015/02/27 03:48:06, msw wrote: > ditto Done.
lgtm with nits. https://codereview.chromium.org/924543004/diff/200001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/924543004/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:424: line->size.set_width(line->size.width() + width); On 2015/02/27 21:45:20, dschuyler wrote: > On 2015/02/27 03:48:05, msw wrote: > > nit: add a comment like: // TODO(dschuyler): Account for stylized baselines in > > string sizing. > > I may have been confusing with my mentioning that I thought the width > calculation had an error. Upon further research it looks like it is good. I'm > guessing this comment comes from that confusion on my part. If it's talking > about something else, could you help me with more detail about the thing to fix? It seems like we don't account for the baseline offset in calculating the line height... should we be doing that? If so, this deserves a comment. https://codereview.chromium.org/924543004/diff/220001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/924543004/diff/220001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:199: protected: nit: these could all be private, right? https://codereview.chromium.org/924543004/diff/220001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2679: int width = string_size.width(); nit: remove |width|, just inline string_size.width() below.
It's looking like the Windows version is drawing some smoothing before (left of) the rectangle. My guess is this wasn't seen before because the font system was different on windows previously and/or the prior testing left justified the text in the canvas (so it was clipped from being off-canvas). The Mac version seems to be going off of the right edge (not just smoothing, but black pixels). I'm trying to work out whether this is a recent thing. https://codereview.chromium.org/924543004/diff/200001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/924543004/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:424: line->size.set_width(line->size.width() + width); On 2015/02/27 22:05:14, msw wrote: > On 2015/02/27 21:45:20, dschuyler wrote: > > On 2015/02/27 03:48:05, msw wrote: > > > nit: add a comment like: // TODO(dschuyler): Account for stylized baselines > in > > > string sizing. > > > > I may have been confusing with my mentioning that I thought the width > > calculation had an error. Upon further research it looks like it is good. > I'm > > guessing this comment comes from that confusion on my part. If it's talking > > about something else, could you help me with more detail about the thing to > fix? > > It seems like we don't account for the baseline offset in calculating the line > height... should we be doing that? If so, this deserves a comment. Ah, it sounds like this will depend on the outcome from the reasoning on how bounds are calculated (in bug 459812). I put the comment in. Done. https://codereview.chromium.org/924543004/diff/220001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/924543004/diff/220001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:199: protected: On 2015/02/27 22:05:15, msw wrote: > nit: these could all be private, right? Done. https://codereview.chromium.org/924543004/diff/220001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2679: int width = string_size.width(); On 2015/02/27 22:05:15, msw wrote: > nit: remove |width|, just inline string_size.width() below. Done.
Patchset #17 (id:320001) has been deleted
Hi, this is an invitation to re-LGTM this change list :)
https://codereview.chromium.org/924543004/diff/300001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/924543004/diff/300001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2642: L" ", This test now has one case, which only "paints" whitespace, so even that is only trivially passing... I like that you've made this test more rigorous, but it's useless as-is. Instead of completely skipping all the cases below, we should make the test tolerant to their failed expectations. So, always allow 1px (or some other very small amount) of painting outside the expected area (perhaps verifying a capped luminance there, like the old test), or add top/bottom/left/right inset values with each test case, allowing more specific error margins for each string. That way, we're at least certain that the defect is fairly negligible and know it's not painting 10px beyond the expected text size. You should also wait for mukai to approve all these TODOs...
Patchset #17 (id:340001) has been deleted
https://codereview.chromium.org/924543004/diff/300001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/924543004/diff/300001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2642: L" ", On 2015/03/02 23:05:35, msw wrote: > This test now has one case, which only "paints" whitespace, so even that is only > trivially passing... I like that you've made this test more rigorous, but it's > useless as-is. Instead of completely skipping all the cases below, we should > make the test tolerant to their failed expectations. So, always allow 1px (or > some other very small amount) of painting outside the expected area (perhaps > verifying a capped luminance there, like the old test), or add > top/bottom/left/right inset values with each test case, allowing more specific > error margins for each string. That way, we're at least certain that the defect > is fairly negligible and know it's not painting 10px beyond the expected text > size. > > You should also wait for mukai to approve all these TODOs... Done.
I suppose this lgtm with a nit. Ping Jun about the TODOs you're adding for him. https://codereview.chromium.org/924543004/diff/380001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/924543004/diff/380001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2702: #ifndef OS_MACOSX nit: "#if !defined(OS_MACOSX)"
dschuyler@chromium.org changed reviewers: + mukai@chromium.org - msw@chromium.org, pkasting@chromium.org
Hi Jun, In the render_text_unittest.cc, could you give me your thoughts on some TODO entries I put in with your name on them :) I'm hoping you could give a LGTM to this CL.
https://codereview.chromium.org/924543004/diff/380001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/924543004/diff/380001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2702: #ifndef OS_MACOSX On 2015/03/03 23:18:28, msw wrote: > nit: "#if !defined(OS_MACOSX)" Done.
https://codereview.chromium.org/924543004/diff/400001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/924543004/diff/400001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2664: for (auto string : kTestStrings) { nit: add SCOPED_TRACE to reveal which test case fails.
https://codereview.chromium.org/924543004/diff/400001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/924543004/diff/400001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2664: for (auto string : kTestStrings) { On 2015/03/05 22:32:45, Jun Mukai wrote: > nit: add SCOPED_TRACE to reveal which test case fails. I added some SCOPED_TRACE calls. (If I did it incorrectly, please let me know). Done.
https://codereview.chromium.org/924543004/diff/400001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/924543004/diff/400001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2664: for (auto string : kTestStrings) { On 2015/03/05 23:19:41, dschuyler wrote: > On 2015/03/05 22:32:45, Jun Mukai wrote: > > nit: add SCOPED_TRACE to reveal which test case fails. > > I added some SCOPED_TRACE calls. (If I did it incorrectly, please let me know). > > Done. I believe SCOPED_TRACE is helpful to be added at the top of a loop like this. Here is the point: when an assert or expect fails, the gtest usually reports the failure with the line number and the file name, but it isn't much clear which test data causes the failure. When SCOPED_TRACE is specified, the data is printed out along with the failure report, so adding it with the test data string is beneficial in case of the same assertions are checked multiple times.
The CQ bit was checked by dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/924543004/#ps420001 (title: "Adding SCOPED_TRACE")
The CQ bit was unchecked by dschuyler@chromium.org
https://codereview.chromium.org/924543004/diff/400001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/924543004/diff/400001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2664: for (auto string : kTestStrings) { On 2015/03/05 22:32:45, Jun Mukai wrote: > nit: add SCOPED_TRACE to reveal which test case fails. Done. https://codereview.chromium.org/924543004/diff/400001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2664: for (auto string : kTestStrings) { On 2015/03/06 03:02:25, Jun Mukai wrote: > On 2015/03/05 23:19:41, dschuyler wrote: > > On 2015/03/05 22:32:45, Jun Mukai wrote: > > > nit: add SCOPED_TRACE to reveal which test case fails. > > > > I added some SCOPED_TRACE calls. (If I did it incorrectly, please let me > know). > > > > Done. > > I believe SCOPED_TRACE is helpful to be added at the top of a loop like this. > > Here is the point: when an assert or expect fails, the gtest usually reports the > failure with the line number and the file name, but it isn't much clear which > test data causes the failure. > When SCOPED_TRACE is specified, the data is printed out along with the failure > report, so adding it with the test data string is beneficial in case of the same > assertions are checked multiple times. Thanks! So, it sounds like the current comments are being extra clear by saying whether the drawing is off the top, bottom, left, or right. Acknowledged.
The CQ bit was checked by dschuyler@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/924543004/420001
Message was sent while issue was closed.
Committed patchset #20 (id:420001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/eefeef59e4b44727e735e99733799a489b8a6efa Cr-Commit-Position: refs/heads/master@{#319492}
Message was sent while issue was closed.
dgrogan@chromium.org changed reviewers: + dgrogan@chromium.org
Message was sent while issue was closed.
Failure on XP: http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20(1)/builds/361... Many variants of c:\b\build\slave\win_builder\build\src\ui\gfx\render_text_unittest.cc(194): error: Value of: color Actual: 4278190080 Expected: 0xFFFFFFFF Which is: 4294967295 ????????????? at 2, 14 Google Test trace: c:\b\build\slave\win_builder\build\src\ui\gfx\render_text_unittest.cc(2715): TextDoesntClip Left Side c:\b\build\slave\win_builder\build\src\ui\gfx\render_text_unittest.cc(194): error: Value of: color Actual: 4278190080 Expected: 0xFFFFFFFF Which is: 4294967295 ????????????? at 2, 15 Google Test trace: c:\b\build\slave\win_builder\build\src\ui\gfx\render_text_unittest.cc(2715): TextDoesntClip Left Side
Message was sent while issue was closed.
On 2015/03/07 00:28:53, dgrogan wrote: > Failure on XP: > > http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20(1)/builds/361... > > Many variants of > > c:\b\build\slave\win_builder\build\src\ui\gfx\render_text_unittest.cc(194): > error: Value of: color > Actual: 4278190080 > Expected: 0xFFFFFFFF > Which is: 4294967295 > ????????????? at 2, 14 > Google Test trace: > c:\b\build\slave\win_builder\build\src\ui\gfx\render_text_unittest.cc(2715): > TextDoesntClip Left Side > c:\b\build\slave\win_builder\build\src\ui\gfx\render_text_unittest.cc(194): > error: Value of: color > Actual: 4278190080 > Expected: 0xFFFFFFFF > Which is: 4294967295 > ????????????? at 2, 15 > Google Test trace: > c:\b\build\slave\win_builder\build\src\ui\gfx\render_text_unittest.cc(2715): > TextDoesntClip Left Side Darn, you'll need to revert, and the scoped trace and expectations don't even tell you which string is broken...
Message was sent while issue was closed.
On 2015/03/07 00:34:39, msw wrote: > On 2015/03/07 00:28:53, dgrogan wrote: > > Failure on XP: > > > > > http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20(1)/builds/361... > > > > Many variants of > > > > c:\b\build\slave\win_builder\build\src\ui\gfx\render_text_unittest.cc(194): > > error: Value of: color > > Actual: 4278190080 > > Expected: 0xFFFFFFFF > > Which is: 4294967295 > > ????????????? at 2, 14 > > Google Test trace: > > c:\b\build\slave\win_builder\build\src\ui\gfx\render_text_unittest.cc(2715): > > TextDoesntClip Left Side > > c:\b\build\slave\win_builder\build\src\ui\gfx\render_text_unittest.cc(194): > > error: Value of: color > > Actual: 4278190080 > > Expected: 0xFFFFFFFF > > Which is: 4294967295 > > ????????????? at 2, 15 > > Google Test trace: > > c:\b\build\slave\win_builder\build\src\ui\gfx\render_text_unittest.cc(2715): > > TextDoesntClip Left Side > > Darn, you'll need to revert, and the scoped trace and expectations don't even > tell you which string is broken... The ????????????? is telling us the string. It looks like the output isn't printing unicode. On my local log output, I'll see the localized unicode characters.
Message was sent while issue was closed.
A revert of this CL (patchset #20 id:420001) has been created in https://codereview.chromium.org/985923002/ by dschuyler@chromium.org. The reason for reverting is: Windows XP appears to be drawing outside of the expected draw area for a font. The short term fix will be to disable the test for Windows XP and put in a TODO to look into it..
Message was sent while issue was closed.
I found this try bot entry for XP: win_chromium_xp_rel_ng That is green with the change at https://codereview.chromium.org/988763005/ Are there other XP tests I should/could run? On Fri, Mar 6, 2015 at 4:44 PM, <dschuyler@chromium.org> wrote: > A revert of this CL (patchset #20 id:420001) has been created in > https://codereview.chromium.org/985923002/ by dschuyler@chromium.org. > > The reason for reverting is: Windows XP appears to be drawing outside of > the > expected draw area for a font. > > The short term fix will be to disable the test for Windows XP and put in a > TODO > to look into it.. > > https://codereview.chromium.org/924543004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Hm, there's also a ChromeOS failure: https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... RenderTextTest.TextDoesntClip (run #1): [ RUN ] RenderTextTest.TextDoesntClip ../../ui/gfx/render_text_unittest.cc:194: Failure Value of: color Actual: 4287006342 Expected: 0xFFFFFFFF Which is: 4294967295 ????????????? at 73, 9 Google Test trace: ../../ui/gfx/render_text_unittest.cc:2704: TextDoesntClip Top Side ../../ui/gfx/render_text_unittest.cc:194: Failure Value of: color Actual: 4292796126 Expected: 0xFFFFFFFF Which is: 4294967295 ????????????? at 74, 9 Google Test trace: ../../ui/gfx/render_text_unittest.cc:2704: TextDoesntClip Top Side [ FAILED ] RenderTextTest.TextDoesntClip (151 ms)
Message was sent while issue was closed.
It sounds like I'm not using the correct try bots... Is there a better way for me to know which try bots to check? On Fri, Mar 6, 2015 at 5:27 PM, <dgrogan@chromium.org> wrote: > Hm, there's also a ChromeOS failure: > https://build.chromium.org/p/chromium.memory/builders/ > Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/ > builds/7266/steps/gfx_unittests/logs/RenderTextTest.TextDoesntClip > > RenderTextTest.TextDoesntClip (run #1): > [ RUN ] RenderTextTest.TextDoesntClip > ../../ui/gfx/render_text_unittest.cc:194: Failure > Value of: color > Actual: 4287006342 > Expected: 0xFFFFFFFF > Which is: 4294967295 > ????????????? at 73, 9 > Google Test trace: > ../../ui/gfx/render_text_unittest.cc:2704: TextDoesntClip Top Side > ../../ui/gfx/render_text_unittest.cc:194: Failure > Value of: color > Actual: 4292796126 > Expected: 0xFFFFFFFF > Which is: 4294967295 > ????????????? at 74, 9 > Google Test trace: > ../../ui/gfx/render_text_unittest.cc:2704: TextDoesntClip Top Side > [ FAILED ] RenderTextTest.TextDoesntClip (151 ms) > > https://codereview.chromium.org/924543004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Please do not ignore my comments... https://codereview.chromium.org/924543004/diff/400001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/924543004/diff/400001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2664: for (auto string : kTestStrings) { On 2015/03/06 20:31:56, dschuyler wrote: > On 2015/03/05 22:32:45, Jun Mukai wrote: > > nit: add SCOPED_TRACE to reveal which test case fails. > > Done. I don't believe you've done what I meant. I meant: for (auto string : kTestStrings) { SCOPED_TRACE(string); ... } That way, the failed string is printed out.
Message was sent while issue was closed.
The scoped string is printed out. I'm confused by the goal. If I add that line, the string will be printed twice for each error. I took your comment to be explaining the intent of the minimum to be done, and since the change was including more information, a super-set, that would be acceptable. On Fri, Mar 6, 2015 at 6:22 PM, <mukai@chromium.org> wrote: > Please do not ignore my comments... > > > https://codereview.chromium.org/924543004/diff/400001/ui/ > gfx/render_text_unittest.cc > File ui/gfx/render_text_unittest.cc (right): > > https://codereview.chromium.org/924543004/diff/400001/ui/ > gfx/render_text_unittest.cc#newcode2664 > ui/gfx/render_text_unittest.cc:2664: for (auto string : kTestStrings) { > On 2015/03/06 20:31:56, dschuyler wrote: > >> On 2015/03/05 22:32:45, Jun Mukai wrote: >> > nit: add SCOPED_TRACE to reveal which test case fails. >> > > Done. >> > > I don't believe you've done what I meant. > > I meant: > for (auto string : kTestStrings) { > SCOPED_TRACE(string); > ... > > } > > That way, the failed string is printed out. > > https://codereview.chromium.org/924543004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
