Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
152473008:
More or less implement RenderTextHarfBuzz (Closed)
|
Created:
6 years, 10 months ago by ckocagil Modified:
6 years, 7 months ago CC:
chromium-reviews, tfarina, Alexei Svitkine (slow), Yuki, Andre Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMore or less implement RenderTextHarfBuzz
Skia font lookup functions for HarfBuzz are from Blink. Much of the implementation copied from RenderTextWin. In the future we should either move the copied methods up, or get rid of all the other RenderText implementations in favor of this one.
BUG=321868
TBR=behdad@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272260
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : rebased #Patch Set 4 : rebased; decorations #
Total comments: 188
Patch Set 5 : styles; script runs; comments addressed #Patch Set 6 : more comments #
Total comments: 53
Patch Set 7 : even more comments #Patch Set 8 : merge 252563003 #
Total comments: 38
Patch Set 9 : Alexei's comments #
Total comments: 18
Patch Set 10 : Alexei's comments 2 #
Total comments: 10
Patch Set 11 : comments #
Total comments: 12
Patch Set 12 : nits #
Total comments: 1
Patch Set 13 : compile fix #Patch Set 14 : android fix #Patch Set 15 : actual android fix #
Messages
Total messages: 36 (0 generated)
Just a heads up, this is not ready for a thorough review yet. Higher level comments are welcome.
https://codereview.chromium.org/152473008/diff/60001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/152473008/diff/60001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:605: hb_buffer_guess_segment_properties(text_buffer); You really don't want to do this. We've had a lot of bugs in blink caused by guessing instead of properly determine the script and direction.
Thanks for working on this, Cem; your efforts are greatly appreciated. People have expressed renewed interest in this CL to me recently. I hope Alexei, Yuki, and others might be willing to review this too. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:321: matrix_(canvas->sk_canvas()->getTotalMatrix()), nit: can this just be calculated in DiagonalStrike::Draw? https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:347: ScopedCanvas scoped_canvas(canvas_); Why this is needed over doing Canvas::Save/Restore? https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:413: if (CommandLine::ForCurrentProcess()->HasSwitch("use-harfbuzz-rendertext")) Make "use-harfbuzz-rendertext" a proper ui/gfx/switches.h constant; "enable-harfbuzz-rendertext" would be better. Add a chrome/browser/about_flags.cc entry for the settings and add the corresponding strings to ui/resources/ui_resources.grd. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:46: class RenderTextHarfBuzz; // REMOVE Move this to where it's needed, if anywhere. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:185: int style); nit: document that this is a gfx::Font::FontStyle value. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:441: static RenderText* CreateNativeInstance(); Make this private. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:18: #include "ui/gfx/render_text.h" Remove this include; it's redundant with render_text_harfbuzz.h. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:26: // Font data provider for HarfBuzz using Skia. Copied from Blink. Add TODOs (and file a bug) to de-duplicate code like this. Many of the anon namespace functions have origins in Blink; some may be non-trivial to consolidate, but some could be reasonably easy. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:27: struct HarfBuzzFontData { nit: remove all "HarfBuzz" prefixes in this anonymous namespace. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:40: hb_codepoint_t codepoint, nit: fix indentations. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:43: CHECK(codepoint <= 0xFFFF); nit: CHECK_LE https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:44: paint->setTextEncoding(SkPaint::kGlyphID_TextEncoding); Is it okay to set this here? Should the value be reset after? https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:46: SkScalar skWidth; nit: use chromium's unix_hacker naming convention for these. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:76: paint->setTextEncoding(SkPaint::kUTF32_TextEncoding); Is it okay to set this here? Should the value be reset after? https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:80: *glyph = glyph16; nit: this looks redundant with the assignment outside this block. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:125: // HarfBuzz will use the fallback implementation if they aren't set. nit: Add a TODO to support SkTypeface kerning operations like Blink. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:180: // TODO: This shouldn't grow indefinitely. See the comment above nit: TODO(ckocagil): https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:182: static std::map<SkFontID, GlyphCache> glyph_caches; Could this map to a struct composed of a GlyphCache and a harfbuzz_face? https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:190: unsigned int upem = hb_face_get_upem(harfbuzz_face); Please add a comment explaining what's going on here. e.g. why is this done like the HarfBuzz test code, and not like Blink's HarfBuzzFace::createFont()? https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:192: hb_font_set_scale(harfbuzz_font, upem, upem); Would it make sense to set the scale separately? https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:193: HarfBuzzFontData* hb_font_data = new HarfBuzzFontData(&glyph_caches[font_id]); Could this be cached as well? https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:206: for (size_t i = 0; i < run.glyph_count - 1; ++i) { optional nit: remove braces here and with the for loop below. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:217: DCHECK(false); nit: use NOTREACHED(). https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:218: return -1; nit: should this return 0 to match the equally viable default return value for the LTR case above? I think the DCHECK above is sufficient warning. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:221: Range CharRangeToGlyphRange(const internal::TextRunHarfBuzz& run, nit: Copy/adapt the comment from render_text_win.cc https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:227: size_t first = CharToGlyph(run, range.start()); nit: const for |first| and |last|. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:239: const int kMissingGlyphId = 0; nit: static https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:240: nit: remove blank line https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:257: underline(false) {} nit: initialize |level|, |font_size|, and |font_style| too (in the correct order). https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:265: needs_layout_(true) {} nit: does an empty object need layout? (RenderTextWin doesn't). https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:273: width += runs_[i]->width; I think starting with a single-line implementation is a good idea :) https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:285: internal::TextRunHarfBuzz& run = *runs_[run_index]; nit: use a const ref or a pointer. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:293: run.glyph_to_char[i] + (run.direction == UBIDI_RTL)), nit: I think these should be explicitly "* ? 1 : 0". https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:294: CURSOR_BACKWARD); nit: I think these are opposite from how RenderTextWin handles |trailing|. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:303: bool RenderTextHarfBuzz::IsCursorablePosition(size_t position) { nit: rebase once https://codereview.chromium.org/252563003 lands. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:308: // TODO: Can this be simplified by using |TextRunHarfBuzz::glyph_to_char|? nit: TODO(ckocagil):, and yeah, that ought to work. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:314: return gfx::IsValidCodePointIndex(text(), position) && nit: remove unnecessary gfx:: namespace qualifier. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:373: SelectionModel RenderTextHarfBuzz::AdjacentWordSelectionModel( Having a common implementation (that matches the current Windows implementation) will cause Linux and ChromeOS behavior to change. We'll probably want to make that happen in a separate CL beforehand (like my old WIP <https://codereview.chromium.org/21140002>), or implement separate behavior for Win vs. Posix. Add a TODO with this information for now. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:493: void RenderTextHarfBuzz::EnsureLayout() { I recommend splitting some of this out like RenderTextWin::ItemizeLogicalText. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:495: // TODO: Skip complex processing if text isn't complex. nit: you can remove this TODO here (it's only applicable to RenderTextWin). https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:496: // TODO: Handle styles. nit: TODO(ckocagil): https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:503: UBiDi* line = ubidi_openSized(text.length(), 0, &result); Can you use base::i18n::BiDiLineIterator? https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:504: CHECK(U_SUCCESS(result)); Can you DCHECK or bail gracefully if this fails? Ditto below. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:506: ubidi_setPara(line, text.c_str(), text.length(), UBIDI_DEFAULT_LTR, NULL, This default value should probably come from GetTextDirection. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:518: //const size_t max_run_length = kMaxGlyphs / 2; Remove this and the commented code below too. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:524: nit: remove blank line https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:544: // This avoids using their fallback fonts for more characters than needed, nit: limit to 80 chars here and elsewhere below. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:564: nit: remove blank line https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:570: // TODO: Is there any case where we would prefer defaulting to UBIDI_RTL? nit: TODO(ckocagil):, also the neutral direction should probably inherit the value from the overall string directionality (this is likely what RenderTextWin::ItemizeLogicalText does via "script_state_.uBidiLevel = (GetTextDirection() == base::i18n::RIGHT_TO_LEFT) ? 1 : 0;"). Otherwise, perhaps it could fallback to the UI via base::i18n::IsRTL()... https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:630: void RenderTextHarfBuzz::DrawVisualText(Canvas* canvas) { nit: DCHECK(!needs_layout_); https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:633: internal::SkiaTextRenderer renderer(canvas); Should this apply font smoothing and cleartype settings like RenderTextWin? https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:641: for (size_t i = 0; i < runs_.size(); ++i) { Do you prefer to do this in terms of runs and not segments? I think that's the conservative approach, but on the other hand, using segments would keep the code closer to RenderTextWin and closer to supporting a multi-line implementation. (I think it's fine as is, but hoped you might explain why you chose this route) https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:643: nit: remove blank line https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:649: canvas->Translate(origin); Won't this undesirably apply the vector atop the existing translation? https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:657: DCHECK(!colored_glyphs.is_empty()); See how I've adjusted this in https://codereview.chromium.org/252563003 https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:664: run.width : run.positions[colored_glyphs.end()].x()) - nit: wrap after ":" and indent the following line four more spaces, I think... https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:666: renderer.DrawDecorations(0, 0, width, run.underline, run.strike, nit: don't you need to supply the starting position coordinates? https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:680: nit: remove blank line https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:681: const Font& primary_font = font_list().GetPrimaryFont(); Add a TODO about handling FontList better and CC yukishiino@, we'll probably need to add some sort of font fallback mechanism like RenderTextWin has in ChooseFallbackFont. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:685: /*DeriveFontIfNecessary(run->font.GetFontSize(), run->font.GetHeight(), nit: remove this if the size is properly adjusted via hb_font_set_scale, etc. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:691: hb_buffer_t* text_buffer = hb_buffer_create(); nit: please add some comments for these HarfBuzz function calls. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:695: hb_buffer_guess_segment_properties(text_buffer); Address eae's comment; at least add a TODO and file a bug for now. eae: any advice? I'm guessing we'll need to port shapeHarfBuzzRuns? https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:701: CHECK(run->glyph_count != -1); nit: CHECK_NE, bail more gracefully if possible. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:703: for (size_t j = 0; j < run->glyph_count; ++j) nit: Use |i| or some better name instead of |j| here and below. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:707: for (size_t j = 0; j < run->glyph_count; ++j) nit: would it make more sense to loop over the glyphs once? https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:713: run->positions.reset(new SkPoint[run->glyph_count]); nit: should this use gfx::Point or PointF (for [future] subpixel positioning?) https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:721: run->width += nit: does incrementing the run width like this yield the correct values when you add the run width and the x_offset above in this loop? https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:746: nit: remove blank line https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:748: x += runs_[i]->width; Add TextRunHarfBuzz.preceding_run_widths or similar to RenderTextWin's TextRun? https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:750: const bool last_glyph = text_index == run.range.end(); nit: inline this at its one use below https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:759: int trailing_step = trailing; nit: use explicit "* ? 1 : 0" here. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:760: size_t glyph_pos = glyph_range.start() + Would it be helpful to keep HarfBuzz's glyph advance value for this? https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:761: (run.direction == UBIDI_LTR ? trailing : !trailing); nit: use "* ? trailing_step : (1 - trailing_step)" here. (otherwise trailing_step is unused) https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:787: for (size_t run = 0; run < runs_.size(); ++run) { The header says |runs_| is in logical order, shouldn't this map to visual? I'm confused as to why RenderTextWin doesn't do that either... https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:70: void ShapeRun(internal::TextRunHarfBuzz* run); nit: comment like "Shape the glyphs needed for the text |run|." https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:72: size_t GetRunContainingCaret(const SelectionModel& caret) const; nit: copy/adapt the comments from RenderTextWin for this, [First|Last]SelectionModelInsideRun, and GetRunContainingXCoord. https://codereview.chromium.org/152473008/diff/100001/ui/views/examples/multi... File ui/views/examples/multiline_example.cc (right): https://codereview.chromium.org/152473008/diff/100001/ui/views/examples/multi... ui/views/examples/multiline_example.cc:75: render_text_->ApplyStyle(gfx::BOLD, true, gfx::Range(3, 8)); Handle strings shorter than these hardcoded ranges, like |test_range|. https://codereview.chromium.org/152473008/diff/100001/ui/views/examples/multi... ui/views/examples/multiline_example.cc:104: const wchar_t kTestString[] = L"qwerty" Use base::char16 https://codereview.chromium.org/152473008/diff/100001/ui/views/examples/multi... ui/views/examples/multiline_example.cc:105: L"\x627\x644\x631\x626\x64A\x633\x64A\x629" nit: Is there any reason for changing the content here?
https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:695: hb_buffer_guess_segment_properties(text_buffer); On 2014/04/29 06:24:45, msw wrote: > Address eae's comment; at least add a TODO and file a bug for now. > eae: any advice? I'm guessing we'll need to port shapeHarfBuzzRuns? Ideally we'd be able to share the script/direction resolution code. For now setting the direction explicitly is probably enough.
https://codereview.chromium.org/152473008/diff/60001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/152473008/diff/60001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:605: hb_buffer_guess_segment_properties(text_buffer); On 2014/03/07 18:43:06, eae wrote: > You really don't want to do this. We've had a lot of bugs in blink caused by > guessing instead of properly determine the script and direction. Done, we now split text by script runs taking into account script extensions too. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:321: matrix_(canvas->sk_canvas()->getTotalMatrix()), On 2014/04/29 06:24:45, msw wrote: > nit: can this just be calculated in DiagonalStrike::Draw? No, because for each text run we do a matrix translation in DrawVisualText but we need to remember the initial matrix so that the diagonal strike starts from the initial run, not the last run. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:347: ScopedCanvas scoped_canvas(canvas_); On 2014/04/29 06:24:45, msw wrote: > Why this is needed over doing Canvas::Save/Restore? This is just another way of doing Save/Restore, nothing special. I can change it if you find it obscure. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:413: if (CommandLine::ForCurrentProcess()->HasSwitch("use-harfbuzz-rendertext")) On 2014/04/29 06:24:45, msw wrote: > Make "use-harfbuzz-rendertext" a proper ui/gfx/switches.h constant; > "enable-harfbuzz-rendertext" would be better. Add a > chrome/browser/about_flags.cc entry for the settings and add the corresponding > strings to ui/resources/ui_resources.grd. Done. I added the strings to chrome/app/generated_resources.grd, since that seems to be where most (all?) experiment strings are located. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:46: class RenderTextHarfBuzz; // REMOVE On 2014/04/29 06:24:45, msw wrote: > Move this to where it's needed, if anywhere. Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:185: int style); On 2014/04/29 06:24:45, msw wrote: > nit: document that this is a gfx::Font::FontStyle value. Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:441: static RenderText* CreateNativeInstance(); On 2014/04/29 06:24:45, msw wrote: > Make this private. Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:18: #include "ui/gfx/render_text.h" On 2014/04/29 06:24:45, msw wrote: > Remove this include; it's redundant with render_text_harfbuzz.h. Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:26: // Font data provider for HarfBuzz using Skia. Copied from Blink. On 2014/04/29 06:24:45, msw wrote: > Add TODOs (and file a bug) to de-duplicate code like this. Many of the anon > namespace functions have origins in Blink; some may be non-trivial to > consolidate, but some could be reasonably easy. Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:27: struct HarfBuzzFontData { On 2014/04/29 06:24:45, msw wrote: > nit: remove all "HarfBuzz" prefixes in this anonymous namespace. Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:40: hb_codepoint_t codepoint, On 2014/04/29 06:24:45, msw wrote: > nit: fix indentations. Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:43: CHECK(codepoint <= 0xFFFF); On 2014/04/29 06:24:45, msw wrote: > nit: CHECK_LE Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:44: paint->setTextEncoding(SkPaint::kGlyphID_TextEncoding); On 2014/04/29 06:24:45, msw wrote: > Is it okay to set this here? Should the value be reset after? We call setTextEncoding before doing anything that depends on it, so this is fine. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:46: SkScalar skWidth; On 2014/04/29 06:24:45, msw wrote: > nit: use chromium's unix_hacker naming convention for these. Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:76: paint->setTextEncoding(SkPaint::kUTF32_TextEncoding); On 2014/04/29 06:24:45, msw wrote: > Is it okay to set this here? Should the value be reset after? Addressed above. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:80: *glyph = glyph16; On 2014/04/29 06:24:45, msw wrote: > nit: this looks redundant with the assignment outside this block. Ouch! Fixed! https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:125: // HarfBuzz will use the fallback implementation if they aren't set. On 2014/04/29 06:24:45, msw wrote: > nit: Add a TODO to support SkTypeface kerning operations like Blink. Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:180: // TODO: This shouldn't grow indefinitely. See the comment above On 2014/04/29 06:24:45, msw wrote: > nit: TODO(ckocagil): Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:182: static std::map<SkFontID, GlyphCache> glyph_caches; On 2014/04/29 06:24:45, msw wrote: > Could this map to a struct composed of a GlyphCache and a harfbuzz_face? Done. Good idea! This resolved the harfbuzz face cache TODO above and gave us a nice speed boost :-) https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:190: unsigned int upem = hb_face_get_upem(harfbuzz_face); On 2014/04/29 06:24:45, msw wrote: > Please add a comment explaining what's going on here. e.g. why is this done like > the HarfBuzz test code, and not like Blink's HarfBuzzFace::createFont()? AFAIK this shapes the glyphs at maximum size and doesn't take our display into account. I added a TODO to investigate whether we need to scale this by font size (for hinting and whatnot). https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:192: hb_font_set_scale(harfbuzz_font, upem, upem); On 2014/04/29 06:24:45, msw wrote: > Would it make sense to set the scale separately? Separate from what? https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:193: HarfBuzzFontData* hb_font_data = new HarfBuzzFontData(&glyph_caches[font_id]); On 2014/04/29 06:24:45, msw wrote: > Could this be cached as well? I implement a cache for this, but both FontData and harfbuzz font objects are pretty lightweight so there wasn't much difference in perf. I revert that change. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:206: for (size_t i = 0; i < run.glyph_count - 1; ++i) { On 2014/04/29 06:24:45, msw wrote: > optional nit: remove braces here and with the for loop below. Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:217: DCHECK(false); On 2014/04/29 06:24:45, msw wrote: > nit: use NOTREACHED(). Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:218: return -1; On 2014/04/29 06:24:45, msw wrote: > nit: should this return 0 to match the equally viable default return value for > the LTR case above? I think the DCHECK above is sufficient warning. Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:221: Range CharRangeToGlyphRange(const internal::TextRunHarfBuzz& run, Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:227: size_t first = CharToGlyph(run, range.start()); On 2014/04/29 06:24:45, msw wrote: > nit: const for |first| and |last|. Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:239: const int kMissingGlyphId = 0; On 2014/04/29 06:24:45, msw wrote: > nit: static Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:240: On 2014/04/29 06:24:45, msw wrote: > nit: remove blank line Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:257: underline(false) {} On 2014/04/29 06:24:45, msw wrote: > nit: initialize |level|, |font_size|, and |font_style| too (in the correct > order). Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:265: needs_layout_(true) {} On 2014/04/29 06:24:45, msw wrote: > nit: does an empty object need layout? (RenderTextWin doesn't). Nope. Fixed. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:273: width += runs_[i]->width; On 2014/04/29 06:24:45, msw wrote: > I think starting with a single-line implementation is a good idea :) But this is already single-line. Now I changed it to utilized Line data though. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:285: internal::TextRunHarfBuzz& run = *runs_[run_index]; On 2014/04/29 06:24:45, msw wrote: > nit: use a const ref or a pointer. Done (the former) https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:293: run.glyph_to_char[i] + (run.direction == UBIDI_RTL)), On 2014/04/29 06:24:45, msw wrote: > nit: I think these should be explicitly "* ? 1 : 0". Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:294: CURSOR_BACKWARD); On 2014/04/29 06:24:45, msw wrote: > nit: I think these are opposite from how RenderTextWin handles |trailing|. Hopefully fixed. I think I managed to get them right this time, manual testing works as intended. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:303: bool RenderTextHarfBuzz::IsCursorablePosition(size_t position) { On 2014/04/29 06:24:45, msw wrote: > nit: rebase once https://codereview.chromium.org/252563003 lands. Will do. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:308: // TODO: Can this be simplified by using |TextRunHarfBuzz::glyph_to_char|? On 2014/04/29 06:24:45, msw wrote: > nit: TODO(ckocagil):, and yeah, that ought to work. Done (added my name, didn't resolve it) https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:314: return gfx::IsValidCodePointIndex(text(), position) && On 2014/04/29 06:24:45, msw wrote: > nit: remove unnecessary gfx:: namespace qualifier. Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:373: SelectionModel RenderTextHarfBuzz::AdjacentWordSelectionModel( On 2014/04/29 06:24:45, msw wrote: > Having a common implementation (that matches the current Windows implementation) > will cause Linux and ChromeOS behavior to change. We'll probably want to make > that happen in a separate CL beforehand (like my old WIP > <https://codereview.chromium.org/21140002>), or implement separate behavior for > Win vs. Posix. Add a TODO with this information for now. Added TODO. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:493: void RenderTextHarfBuzz::EnsureLayout() { On 2014/04/29 06:24:45, msw wrote: > I recommend splitting some of this out like RenderTextWin::ItemizeLogicalText. I did some splitting here. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:495: // TODO: Skip complex processing if text isn't complex. On 2014/04/29 06:24:45, msw wrote: > nit: you can remove this TODO here (it's only applicable to RenderTextWin). Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:496: // TODO: Handle styles. On 2014/04/29 06:24:45, msw wrote: > nit: TODO(ckocagil): Resolved! :-) https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:503: UBiDi* line = ubidi_openSized(text.length(), 0, &result); On 2014/04/29 06:24:45, msw wrote: > Can you use base::i18n::BiDiLineIterator? It lacks |ubidi_getBaseDirection| which we need here. I added a TODO to add that functionality to BiDiLineIterator and utilize it. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:504: CHECK(U_SUCCESS(result)); On 2014/04/29 06:24:45, msw wrote: > Can you DCHECK or bail gracefully if this fails? Ditto below. Done, we now NOTREACHED() and return in the case of a failure. Although this results in a stack overflow from recursive gfx::ElideText calls. (I think it expects non-zero string size, but it gets zero). https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:506: ubidi_setPara(line, text.c_str(), text.length(), UBIDI_DEFAULT_LTR, NULL, On 2014/04/29 06:24:45, msw wrote: > This default value should probably come from GetTextDirection. Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:518: //const size_t max_run_length = kMaxGlyphs / 2; On 2014/04/29 06:24:45, msw wrote: > Remove this and the commented code below too. Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:524: On 2014/04/29 06:24:45, msw wrote: > nit: remove blank line Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:544: // This avoids using their fallback fonts for more characters than needed, On 2014/04/29 06:24:45, msw wrote: > nit: limit to 80 chars here and elsewhere below. Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:564: On 2014/04/29 06:24:45, msw wrote: > nit: remove blank line Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:570: // TODO: Is there any case where we would prefer defaulting to UBIDI_RTL? On 2014/04/29 06:24:45, msw wrote: > nit: TODO(ckocagil):, also the neutral direction should probably inherit the > value from the overall string directionality (this is likely what > RenderTextWin::ItemizeLogicalText does via "script_state_.uBidiLevel = > (GetTextDirection() == base::i18n::RIGHT_TO_LEFT) ? 1 : 0;"). Otherwise, perhaps > it could fallback to the UI via base::i18n::IsRTL()... Used GetTextDirection() here. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:630: void RenderTextHarfBuzz::DrawVisualText(Canvas* canvas) { On 2014/04/29 06:24:45, msw wrote: > nit: DCHECK(!needs_layout_); Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:633: internal::SkiaTextRenderer renderer(canvas); On 2014/04/29 06:24:45, msw wrote: > Should this apply font smoothing and cleartype settings like RenderTextWin? Looks like it is needed for disabling cleartype when it's disabled from Windows settings. Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:641: for (size_t i = 0; i < runs_.size(); ++i) { On 2014/04/29 06:24:45, msw wrote: > Do you prefer to do this in terms of runs and not segments? I think that's the > conservative approach, but on the other hand, using segments would keep the code > closer to RenderTextWin and closer to supporting a multi-line implementation. (I > think it's fine as is, but hoped you might explain why you chose this route) Simply because I implemented Lines and Segments after this function. Let's keep it this way until we fix the primary issues and make this the default RenderText. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:643: On 2014/04/29 06:24:45, msw wrote: > nit: remove blank line Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:649: canvas->Translate(origin); On 2014/04/29 06:24:45, msw wrote: > Won't this undesirably apply the vector atop the existing translation? Why would that be undesirable? If the RenderText client gives us a translated Canvas, then it probably wants us to draw at a translated position. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:657: DCHECK(!colored_glyphs.is_empty()); On 2014/04/29 06:24:45, msw wrote: > See how I've adjusted this in https://codereview.chromium.org/252563003 I'll merge that CL here after it's landed. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:664: run.width : run.positions[colored_glyphs.end()].x()) - On 2014/04/29 06:24:45, msw wrote: > nit: wrap after ":" and indent the following line four more spaces, I think... Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:666: renderer.DrawDecorations(0, 0, width, run.underline, run.strike, On 2014/04/29 06:24:45, msw wrote: > nit: don't you need to supply the starting position coordinates? No, we achieve that by translating the canvas above. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:680: On 2014/04/29 06:24:45, msw wrote: > nit: remove blank line Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:681: const Font& primary_font = font_list().GetPrimaryFont(); On 2014/04/29 06:24:45, msw wrote: > Add a TODO about handling FontList better and CC yukishiino@, we'll probably > need to add some sort of font fallback mechanism like RenderTextWin has in > ChooseFallbackFont. Added a TODO for fallback. Better how? https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:685: /*DeriveFontIfNecessary(run->font.GetFontSize(), run->font.GetHeight(), On 2014/04/29 06:24:45, msw wrote: > nit: remove this if the size is properly adjusted via hb_font_set_scale, etc. Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:691: hb_buffer_t* text_buffer = hb_buffer_create(); On 2014/04/29 06:24:45, msw wrote: > nit: please add some comments for these HarfBuzz function calls. Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:695: hb_buffer_guess_segment_properties(text_buffer); On 2014/04/29 16:01:53, eae wrote: > On 2014/04/29 06:24:45, msw wrote: > > Address eae's comment; at least add a TODO and file a bug for now. > > eae: any advice? I'm guessing we'll need to port shapeHarfBuzzRuns? > > Ideally we'd be able to share the script/direction resolution code. For now > setting the direction explicitly is probably enough. I added logic to split runs by scripts which uses Unicode script extensions too. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:701: CHECK(run->glyph_count != -1); On 2014/04/29 06:24:45, msw wrote: > nit: CHECK_NE, bail more gracefully if possible. Actually there isn't anything to check here. Removed. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:703: for (size_t j = 0; j < run->glyph_count; ++j) On 2014/04/29 06:24:45, msw wrote: > nit: Use |i| or some better name instead of |j| here and below. Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:707: for (size_t j = 0; j < run->glyph_count; ++j) On 2014/04/29 06:24:45, msw wrote: > nit: would it make more sense to loop over the glyphs once? Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:713: run->positions.reset(new SkPoint[run->glyph_count]); On 2014/04/29 06:24:45, msw wrote: > nit: should this use gfx::Point or PointF (for [future] subpixel positioning?) Is there any benefit other than using a gfx:: type? https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:721: run->width += On 2014/04/29 06:24:45, msw wrote: > nit: does incrementing the run width like this yield the correct values when you > add the run width and the x_offset above in this loop? It visually does, and I don't see why it shouldn't. (Is your question about ASCII chars or weird chars?) https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:746: On 2014/04/29 06:24:45, msw wrote: > nit: remove blank line Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:748: x += runs_[i]->width; On 2014/04/29 06:24:45, msw wrote: > Add TextRunHarfBuzz.preceding_run_widths or similar to RenderTextWin's TextRun? Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:750: const bool last_glyph = text_index == run.range.end(); On 2014/04/29 06:24:45, msw wrote: > nit: inline this at its one use below Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:759: int trailing_step = trailing; On 2014/04/29 06:24:45, msw wrote: > nit: use explicit "* ? 1 : 0" here. Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:760: size_t glyph_pos = glyph_range.start() + On 2014/04/29 06:24:45, msw wrote: > Would it be helpful to keep HarfBuzz's glyph advance value for this? Which glyph advance value and helpful how? https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:761: (run.direction == UBIDI_LTR ? trailing : !trailing); On 2014/04/29 06:24:45, msw wrote: > nit: use "* ? trailing_step : (1 - trailing_step)" here. (otherwise > trailing_step is unused) Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:787: for (size_t run = 0; run < runs_.size(); ++run) { On 2014/04/29 06:24:45, msw wrote: > The header says |runs_| is in logical order, shouldn't this map to visual? > I'm confused as to why RenderTextWin doesn't do that either... No idea how this worked, maybe we usually don't have very complicated mappings? Anyway I fixed it here. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:70: void ShapeRun(internal::TextRunHarfBuzz* run); On 2014/04/29 06:24:45, msw wrote: > nit: comment like "Shape the glyphs needed for the text |run|." Done. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:72: size_t GetRunContainingCaret(const SelectionModel& caret) const; On 2014/04/29 06:24:45, msw wrote: > nit: copy/adapt the comments from RenderTextWin for this, > [First|Last]SelectionModelInsideRun, and GetRunContainingXCoord. Done. https://codereview.chromium.org/152473008/diff/100001/ui/views/examples/multi... File ui/views/examples/multiline_example.cc (right): https://codereview.chromium.org/152473008/diff/100001/ui/views/examples/multi... ui/views/examples/multiline_example.cc:75: render_text_->ApplyStyle(gfx::BOLD, true, gfx::Range(3, 8)); On 2014/04/29 06:24:45, msw wrote: > Handle strings shorter than these hardcoded ranges, like |test_range|. Done. https://codereview.chromium.org/152473008/diff/100001/ui/views/examples/multi... ui/views/examples/multiline_example.cc:104: const wchar_t kTestString[] = L"qwerty" On 2014/04/29 06:24:45, msw wrote: > Use base::char16 I don't think we can do that (wchar_t isn't always char16) but I added a conversion here. https://codereview.chromium.org/152473008/diff/100001/ui/views/examples/multi... ui/views/examples/multiline_example.cc:105: L"\x627\x644\x631\x626\x64A\x633\x64A\x629" On 2014/04/29 06:24:45, msw wrote: > nit: Is there any reason for changing the content here? For testing RTL runs.
LGTM for harfbuzz/shaping implementation. It's a shame we can't share some of this with blink though.
I'll take a look at the newest patch set soon, just wanted to post some replies. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:321: matrix_(canvas->sk_canvas()->getTotalMatrix()), On 2014/05/01 22:02:01, ckocagil wrote: > On 2014/04/29 06:24:45, msw wrote: > > nit: can this just be calculated in DiagonalStrike::Draw? > > No, because for each text run we do a matrix translation in DrawVisualText but > we need to remember the initial matrix so that the diagonal strike starts from > the initial run, not the last run. This wasn't previously necessary, is it related to the tangential ScopedCanvas changes below, and hopefully avoidable in this CL? https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:347: ScopedCanvas scoped_canvas(canvas_); On 2014/05/01 22:02:01, ckocagil wrote: > On 2014/04/29 06:24:45, msw wrote: > > Why this is needed over doing Canvas::Save/Restore? > > This is just another way of doing Save/Restore, nothing special. I can change it > if you find it obscure. Reducing the amount of churn in this change is highly desirable. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:413: if (CommandLine::ForCurrentProcess()->HasSwitch("use-harfbuzz-rendertext")) On 2014/05/01 22:02:01, ckocagil wrote: > On 2014/04/29 06:24:45, msw wrote: > > Make "use-harfbuzz-rendertext" a proper ui/gfx/switches.h constant; > > "enable-harfbuzz-rendertext" would be better. Add a > > chrome/browser/about_flags.cc entry for the settings and add the corresponding > > strings to ui/resources/ui_resources.grd. > > Done. I added the strings to chrome/app/generated_resources.grd, since that > seems to be where most (all?) experiment strings are located. Good, the file I suggested for strings was wrong, you are right. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:182: static std::map<SkFontID, GlyphCache> glyph_caches; On 2014/05/01 22:02:01, ckocagil wrote: > On 2014/04/29 06:24:45, msw wrote: > > Could this map to a struct composed of a GlyphCache and a harfbuzz_face? > > Done. Good idea! This resolved the harfbuzz face cache TODO above and gave us a > nice speed boost :-) Awesome :D https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:192: hb_font_set_scale(harfbuzz_font, upem, upem); On 2014/05/01 22:02:01, ckocagil wrote: > On 2014/04/29 06:24:45, msw wrote: > > Would it make sense to set the scale separately? > > Separate from what? In a separate function from the creation of the font. (in case we wanted to save fonts independent of size) Maybe it's not a worthwhile idea, ignore it if so. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:193: HarfBuzzFontData* hb_font_data = new HarfBuzzFontData(&glyph_caches[font_id]); On 2014/05/01 22:02:01, ckocagil wrote: > On 2014/04/29 06:24:45, msw wrote: > > Could this be cached as well? > > I implement a cache for this, but both FontData and harfbuzz font objects are > pretty lightweight so there wasn't much difference in perf. I revert that > change. Fair enough, thanks for giving it a shot. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:273: width += runs_[i]->width; On 2014/05/01 22:02:01, ckocagil wrote: > On 2014/04/29 06:24:45, msw wrote: > > I think starting with a single-line implementation is a good idea :) > > But this is already single-line. Now I changed it to utilized Line data though. Right, I was just saying that you chose the right path. I'll take a look at the newest patch set soon. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:504: CHECK(U_SUCCESS(result)); On 2014/05/01 22:02:01, ckocagil wrote: > On 2014/04/29 06:24:45, msw wrote: > > Can you DCHECK or bail gracefully if this fails? Ditto below. > > Done, we now NOTREACHED() and return in the case of a failure. Although this > results in a stack overflow from recursive gfx::ElideText calls. (I think it > expects non-zero string size, but it gets zero). Yikes, please avoid a stack overflow, even on failure, if possible! Clearing the text, faking layout data, etc. is preferable. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:649: canvas->Translate(origin); On 2014/05/01 22:02:01, ckocagil wrote: > On 2014/04/29 06:24:45, msw wrote: > > Won't this undesirably apply the vector atop the existing translation? > > Why would that be undesirable? If the RenderText client gives us a translated > Canvas, then it probably wants us to draw at a translated position. It looks like this continuously applies offsets without ever unapplying them. Given run[0] at x=10, run[1] at x=20, the combo draws run[1] at x=30. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:681: const Font& primary_font = font_list().GetPrimaryFont(); On 2014/05/01 22:02:01, ckocagil wrote: > On 2014/04/29 06:24:45, msw wrote: > > Add a TODO about handling FontList better and CC yukishiino@, we'll probably > > need to add some sort of font fallback mechanism like RenderTextWin has in > > ChooseFallbackFont. > > Added a TODO for fallback. Better how? We should use the FontList's other Fonts besides GetPrimaryFont as fallbacks. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:713: run->positions.reset(new SkPoint[run->glyph_count]); On 2014/05/01 22:02:01, ckocagil wrote: > On 2014/04/29 06:24:45, msw wrote: > > nit: should this use gfx::Point or PointF (for [future] subpixel positioning?) > > Is there any benefit other than using a gfx:: type? I see that the SkPoint values are used later on; this is okay for now. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:721: run->width += On 2014/05/01 22:02:01, ckocagil wrote: > On 2014/04/29 06:24:45, msw wrote: > > nit: does incrementing the run width like this yield the correct values when > you > > add the run width and the x_offset above in this loop? > > It visually does, and I don't see why it shouldn't. (Is your question about > ASCII chars or weird chars?) I figured that each glyph's x_offset would be relative to the start of the run. Further, the x_advance is akin to (but nuanced from) the width of each glyph. If so, run->positions[j] looks like it's double-counting the prior offsets. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:760: size_t glyph_pos = glyph_range.start() + On 2014/05/01 22:02:01, ckocagil wrote: > On 2014/04/29 06:24:45, msw wrote: > > Would it be helpful to keep HarfBuzz's glyph advance value for this? > > Which glyph advance value and helpful how? Could hb_positions[j].x_advance be used to get the trailing x boundry? https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:787: for (size_t run = 0; run < runs_.size(); ++run) { On 2014/05/01 22:02:01, ckocagil wrote: > On 2014/04/29 06:24:45, msw wrote: > > The header says |runs_| is in logical order, shouldn't this map to visual? > > I'm confused as to why RenderTextWin doesn't do that either... > > No idea how this worked, maybe we usually don't have very complicated mappings? > Anyway I fixed it here. I'm scared :p (I'll need to debug this on Windows for my own sanity)
https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:321: matrix_(canvas->sk_canvas()->getTotalMatrix()), On 2014/05/02 05:08:21, msw wrote: > On 2014/05/01 22:02:01, ckocagil wrote: > > On 2014/04/29 06:24:45, msw wrote: > > > nit: can this just be calculated in DiagonalStrike::Draw? > > > > No, because for each text run we do a matrix translation in DrawVisualText but > > we need to remember the initial matrix so that the diagonal strike starts from > > the initial run, not the last run. > > This wasn't previously necessary, is it related to the tangential ScopedCanvas > changes below, and hopefully avoidable in this CL? Actually these changes are intended. When we want to offset the glyphs in RenderTextWin by the line position, baseline, previous runs on the line, etc. we create a second positions array that is translated. Why not just translate the canvas, draw the glyphs as if we were drawing at (0,0) and then restore the canvas? This is faster, simpler, and exactly what we're doing in this patch. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:347: ScopedCanvas scoped_canvas(canvas_); On 2014/05/02 05:08:21, msw wrote: > On 2014/05/01 22:02:01, ckocagil wrote: > > On 2014/04/29 06:24:45, msw wrote: > > > Why this is needed over doing Canvas::Save/Restore? > > > > This is just another way of doing Save/Restore, nothing special. I can change > it > > if you find it obscure. > > Reducing the amount of churn in this change is highly desirable. Done, this is now a Save/Restore pair. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:192: hb_font_set_scale(harfbuzz_font, upem, upem); On 2014/05/02 05:08:21, msw wrote: > On 2014/05/01 22:02:01, ckocagil wrote: > > On 2014/04/29 06:24:45, msw wrote: > > > Would it make sense to set the scale separately? > > > > Separate from what? > > In a separate function from the creation of the font. > (in case we wanted to save fonts independent of size) > Maybe it's not a worthwhile idea, ignore it if so. I'm still not exactly sure what the hb_font_set_scale call actually does. I'll revisit this code when I do. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:504: CHECK(U_SUCCESS(result)); On 2014/05/02 05:08:21, msw wrote: > On 2014/05/01 22:02:01, ckocagil wrote: > > On 2014/04/29 06:24:45, msw wrote: > > > Can you DCHECK or bail gracefully if this fails? Ditto below. > > > > Done, we now NOTREACHED() and return in the case of a failure. Although this > > results in a stack overflow from recursive gfx::ElideText calls. (I think it > > expects non-zero string size, but it gets zero). > > Yikes, please avoid a stack overflow, even on failure, if possible! > Clearing the text, faking layout data, etc. is preferable. Done, we now fallback to fake layout data. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:649: canvas->Translate(origin); On 2014/05/02 05:08:21, msw wrote: > On 2014/05/01 22:02:01, ckocagil wrote: > > On 2014/04/29 06:24:45, msw wrote: > > > Won't this undesirably apply the vector atop the existing translation? > > > > Why would that be undesirable? If the RenderText client gives us a translated > > Canvas, then it probably wants us to draw at a translated position. > > It looks like this continuously applies offsets without ever unapplying them. > Given run[0] at x=10, run[1] at x=20, the combo draws run[1] at x=30. The ScopedCanvas resets it. It's now an explicit Save/Restore pair. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:721: run->width += On 2014/05/02 05:08:21, msw wrote: > On 2014/05/01 22:02:01, ckocagil wrote: > > On 2014/04/29 06:24:45, msw wrote: > > > nit: does incrementing the run width like this yield the correct values when > > you > > > add the run width and the x_offset above in this loop? > > > > It visually does, and I don't see why it shouldn't. (Is your question about > > ASCII chars or weird chars?) > > I figured that each glyph's x_offset would be relative to the start of the run. > Further, the x_advance is akin to (but nuanced from) the width of each glyph. > If so, run->positions[j] looks like it's double-counting the prior offsets. Nope, those offsets are for individual glyphs. e.g a font can make "ü" out of "u" and two periods by offsetting the periods. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:760: size_t glyph_pos = glyph_range.start() + On 2014/05/02 05:08:21, msw wrote: > On 2014/05/01 22:02:01, ckocagil wrote: > > On 2014/04/29 06:24:45, msw wrote: > > > Would it be helpful to keep HarfBuzz's glyph advance value for this? > > > > Which glyph advance value and helpful how? > > Could hb_positions[j].x_advance be used to get the trailing x boundry? See my comment about x_offset above.
This is looking pretty good overall, I'm impressed! https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:321: matrix_(canvas->sk_canvas()->getTotalMatrix()), On 2014/05/06 03:38:40, ckocagil wrote: > On 2014/05/02 05:08:21, msw wrote: > > On 2014/05/01 22:02:01, ckocagil wrote: > > > On 2014/04/29 06:24:45, msw wrote: > > > > nit: can this just be calculated in DiagonalStrike::Draw? > > > > > > No, because for each text run we do a matrix translation in DrawVisualText > but > > > we need to remember the initial matrix so that the diagonal strike starts > from > > > the initial run, not the last run. > > > > This wasn't previously necessary, is it related to the tangential ScopedCanvas > > changes below, and hopefully avoidable in this CL? > > Actually these changes are intended. When we want to offset the glyphs in > RenderTextWin by the line position, baseline, previous runs on the line, etc. we > create a second positions array that is translated. Why not just translate the > canvas, draw the glyphs as if we were drawing at (0,0) and then restore the > canvas? This is faster, simpler, and exactly what we're doing in this patch. Ah, so we could apply canvas translations in RenderTextWin and RenderTextPango DrawVisualText and then not pass a start point here, like in RenderTextHarfBuzz? Does that also simplify the use of internal::SkiaTextRenderer to actually paint the text runs? Please update Win/Pango similarly in a separate CL before or after this one, to have more shared and simplified behavior. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:347: ScopedCanvas scoped_canvas(canvas_); On 2014/05/06 03:38:40, ckocagil wrote: > On 2014/05/02 05:08:21, msw wrote: > > On 2014/05/01 22:02:01, ckocagil wrote: > > > On 2014/04/29 06:24:45, msw wrote: > > > > Why this is needed over doing Canvas::Save/Restore? > > > > > > This is just another way of doing Save/Restore, nothing special. I can > change > > it > > > if you find it obscure. > > > > Reducing the amount of churn in this change is highly desirable. > > Done, this is now a Save/Restore pair. It doesn't look like you updated the latest patch set as such, but I now like the use of ScopedCanvas, so you can leave this change in or make it in a separate CL, whichever you prefer. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:359: Rect(x, end.y() - thickness, pieces_[i].first, clip_height)), I think you'll need to still respect start_.y() for Win/Pango calls. Perhaps this is more reason to land this part of the change (plus |matrix_|, ScopedCanvas, and Win/Pango translation changes) before this HarfBuzz CL. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:649: canvas->Translate(origin); On 2014/05/06 03:38:40, ckocagil wrote: > On 2014/05/02 05:08:21, msw wrote: > > On 2014/05/01 22:02:01, ckocagil wrote: > > > On 2014/04/29 06:24:45, msw wrote: > > > > Won't this undesirably apply the vector atop the existing translation? > > > > > > Why would that be undesirable? If the RenderText client gives us a > translated > > > Canvas, then it probably wants us to draw at a translated position. > > > > It looks like this continuously applies offsets without ever unapplying them. > > Given run[0] at x=10, run[1] at x=20, the combo draws run[1] at x=30. > > The ScopedCanvas resets it. It's now an explicit Save/Restore pair. Ah! Sorry I missed that; ScopedCanvas is fine if you prefer that. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:657: DCHECK(!colored_glyphs.is_empty()); On 2014/05/01 22:02:01, ckocagil wrote: > On 2014/04/29 06:24:45, msw wrote: > > See how I've adjusted this in https://codereview.chromium.org/252563003 > > I'll merge that CL here after it's landed. It has landed, please sync and rebase (in a separate patch set upload, from addressing comments, if possible). https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:77: FontData* hb_font_data = reinterpret_cast<FontData*>(font_data); nit: rename the void* as |data|, and this as |font_data|. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:78: GlyphCache& cache = *hb_font_data->glyph_cache_; nit: make this a const ref or non-const pointer. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:94: FontData* hb_font_data = reinterpret_cast<FontData*>(font_data); nit: rename the void* as |data|, and this as |font_data|. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:107: // Just return true, following the way that HarfBuzz-FreeType implementation nit: "Just return true, like the HarfBuzz-FreeType implementation." https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:117: FontData* hb_font_data = reinterpret_cast<FontData*>(font_data); nit: rename the void* as |data|, and this as |font_data|. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:170: hb_face_t* face = hb_face_create_for_tables(GetFontTable, skia_face, 0); Should this supply a |destroy| argument? Where does |face| get deleted? https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:178: // TODO(ckocagil): This shouldn't grow indefinitely. See the comment above nit: There is no comment above CreateHarfBuzzFace anymore. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:185: if (!cached) { nit: can you check !face_cache->first here instead? https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:264: if (icu_error != U_ZERO_ERROR) nit: Use U_FAILURE(icu_error) https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:268: if (icu_error != U_ZERO_ERROR) nit: Use U_FAILURE(icu_error) https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:273: // Intersects the scx set of |codepoint| with |result| and writes to |result|, nit: scx? https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:275: void ScriptSetIntersect(UChar32 codepoint, Only the first valid script is actually used, afaict. Wouldn't it be clearer if ScriptInterval, ScriptSetIntersect, and GetScriptExtensions only bothered with the one prevailing script value? My idea of the pseudocode: script ScriptInterval(text, in/out length) { script = invalid; for(i = 0; i < length; ++i) { char = text[i]; char_script = GetScript(char); if (!IsValidScript(script)) { script = char_script; } else if (IsValidScript(char_script) && script != char_script) { *length = i - 1; return script; } } // not needed: *length = length; return script; } script GetScript(char) { script = uscript_getScript... if valid(script) return script scripts = uscript_getScriptExtensions... for i = 0:scripts if valid(scripts[i]) return scripts[i]; } bool IsValidScript (script) { return script > USCRIPT_INVALID_CODE && script <= USCRIPT_INHERITED; } https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:298: void ScriptInterval(const base::char16* text, nit: add a comment (perhaps move/adapt the one from the callsite "// Find the length and script of the script run.") https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:301: int* script_length) { Can this return script_length instead? https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:302: DCHECK(length > 0); DCHECK_GT https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:311: UChar32 character = char_iterator.get(); nit: inline this https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:313: if (scripts_size) { nit: explicitly check scripts_size > 0 https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:372: run.glyph_to_char[i] + (run.direction == UBIDI_RTL ? 1 : 0)), nit: maybe cache a const bool is_rtl for the run? https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:811: run->direction == UBIDI_LTR ? HB_DIRECTION_LTR : HB_DIRECTION_RTL); nit: Should TextRunHarfBuzz keep the HarfBuzz direction enum values instead? https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:856: nit: remove blank line https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:867: int trailing_step = trailing ? 1 : 0; nit: const https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:868: size_t glyph_pos = glyph_range.start() + nit: const https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:889: size_t RenderTextHarfBuzz::GetRunContainingXCoord(int x, int* offset) const { nit: move this just below GetRunContainingCaret to match its decl. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:91: void ItemizeText(); nit: Add a comment like "Break the text into lines and runs." https://codereview.chromium.org/152473008/diff/140001/ui/views/examples/multi... File ui/views/examples/multiline_example.cc (right): https://codereview.chromium.org/152473008/diff/140001/ui/views/examples/multi... ui/views/examples/multiline_example.cc:28: return range; nit: you can just return range.Intersect(gfx::Range(0, max));
https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:321: matrix_(canvas->sk_canvas()->getTotalMatrix()), On 2014/05/09 22:55:18, msw wrote: > On 2014/05/06 03:38:40, ckocagil wrote: > > On 2014/05/02 05:08:21, msw wrote: > > > On 2014/05/01 22:02:01, ckocagil wrote: > > > > On 2014/04/29 06:24:45, msw wrote: > > > > > nit: can this just be calculated in DiagonalStrike::Draw? > > > > > > > > No, because for each text run we do a matrix translation in DrawVisualText > > but > > > > we need to remember the initial matrix so that the diagonal strike starts > > from > > > > the initial run, not the last run. > > > > > > This wasn't previously necessary, is it related to the tangential > ScopedCanvas > > > changes below, and hopefully avoidable in this CL? > > > > Actually these changes are intended. When we want to offset the glyphs in > > RenderTextWin by the line position, baseline, previous runs on the line, etc. > we > > create a second positions array that is translated. Why not just translate the > > canvas, draw the glyphs as if we were drawing at (0,0) and then restore the > > canvas? This is faster, simpler, and exactly what we're doing in this patch. > > Ah, so we could apply canvas translations in RenderTextWin and RenderTextPango > DrawVisualText and then not pass a start point here, like in RenderTextHarfBuzz? > Does that also simplify the use of internal::SkiaTextRenderer to actually paint > the text runs? Please update Win/Pango similarly in a separate CL before or > after this one, to have more shared and simplified behavior. It doesn't simplify SkiaTextRenderer. Sure, I will do this for RenderTextWin too. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:359: Rect(x, end.y() - thickness, pieces_[i].first, clip_height)), On 2014/05/09 22:55:18, msw wrote: > I think you'll need to still respect start_.y() for Win/Pango calls. Perhaps > this is more reason to land this part of the change (plus |matrix_|, > ScopedCanvas, and Win/Pango translation changes) before this HarfBuzz CL. We're already respecting that value by using end.y() and clip_height. https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/152473008/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:657: DCHECK(!colored_glyphs.is_empty()); On 2014/05/09 22:55:18, msw wrote: > On 2014/05/01 22:02:01, ckocagil wrote: > > On 2014/04/29 06:24:45, msw wrote: > > > See how I've adjusted this in https://codereview.chromium.org/252563003 > > > > I'll merge that CL here after it's landed. > > It has landed, please sync and rebase (in a separate patch set upload, from > addressing comments, if possible). Done. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:77: FontData* hb_font_data = reinterpret_cast<FontData*>(font_data); On 2014/05/09 22:55:19, msw wrote: > nit: rename the void* as |data|, and this as |font_data|. Done. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:78: GlyphCache& cache = *hb_font_data->glyph_cache_; On 2014/05/09 22:55:19, msw wrote: > nit: make this a const ref or non-const pointer. Done. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:94: FontData* hb_font_data = reinterpret_cast<FontData*>(font_data); On 2014/05/09 22:55:19, msw wrote: > nit: rename the void* as |data|, and this as |font_data|. Done. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:107: // Just return true, following the way that HarfBuzz-FreeType implementation On 2014/05/09 22:55:19, msw wrote: > nit: "Just return true, like the HarfBuzz-FreeType implementation." Done. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:117: FontData* hb_font_data = reinterpret_cast<FontData*>(font_data); On 2014/05/09 22:55:19, msw wrote: > nit: rename the void* as |data|, and this as |font_data|. Done. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:170: hb_face_t* face = hb_face_create_for_tables(GetFontTable, skia_face, 0); On 2014/05/09 22:55:19, msw wrote: > Should this supply a |destroy| argument? Where does |face| get deleted? It currently doesn't get deleted since we never invalidate anything in our cache in |CreateHarfBuzzFont|. I added a ref/unref pair here: even if we don't use it yet, it's bad to assume this object will live forever. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:178: // TODO(ckocagil): This shouldn't grow indefinitely. See the comment above On 2014/05/09 22:55:19, msw wrote: > nit: There is no comment above CreateHarfBuzzFace anymore. Done. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:185: if (!cached) { On 2014/05/09 22:55:19, msw wrote: > nit: can you check !face_cache->first here instead? Done. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:264: if (icu_error != U_ZERO_ERROR) On 2014/05/09 22:55:19, msw wrote: > nit: Use U_FAILURE(icu_error) Done. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:268: if (icu_error != U_ZERO_ERROR) On 2014/05/09 22:55:19, msw wrote: > nit: Use U_FAILURE(icu_error) Done. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:273: // Intersects the scx set of |codepoint| with |result| and writes to |result|, On 2014/05/09 22:55:19, msw wrote: > nit: scx? Done, elaborated https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:275: void ScriptSetIntersect(UChar32 codepoint, What if we have a string of 3 chars with the following script values: {Kana} {Hira,Kana} {Kana} Wouldn't your method only take the first script values into account, thus unnecessarily break the text into 3 items? https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:298: void ScriptInterval(const base::char16* text, On 2014/05/09 22:55:19, msw wrote: > nit: add a comment (perhaps move/adapt the one from the callsite "// Find the > length and script of the script run.") Done. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:301: int* script_length) { On 2014/05/09 22:55:19, msw wrote: > Can this return script_length instead? Done. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:302: DCHECK(length > 0); On 2014/05/09 22:55:19, msw wrote: > DCHECK_GT Done. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:311: UChar32 character = char_iterator.get(); On 2014/05/09 22:55:19, msw wrote: > nit: inline this Done. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:313: if (scripts_size) { On 2014/05/09 22:55:19, msw wrote: > nit: explicitly check scripts_size > 0 Done. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:372: run.glyph_to_char[i] + (run.direction == UBIDI_RTL ? 1 : 0)), On 2014/05/09 22:55:19, msw wrote: > nit: maybe cache a const bool is_rtl for the run? Done. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:811: run->direction == UBIDI_LTR ? HB_DIRECTION_LTR : HB_DIRECTION_RTL); On 2014/05/09 22:55:19, msw wrote: > nit: Should TextRunHarfBuzz keep the HarfBuzz direction enum values instead? I don't think that would change much. If it makes a difference in the future we can change it then. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:856: On 2014/05/09 22:55:19, msw wrote: > nit: remove blank line Done. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:867: int trailing_step = trailing ? 1 : 0; On 2014/05/09 22:55:19, msw wrote: > nit: const Done. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:868: size_t glyph_pos = glyph_range.start() + On 2014/05/09 22:55:19, msw wrote: > nit: const Done. https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:889: size_t RenderTextHarfBuzz::GetRunContainingXCoord(int x, int* offset) const { On 2014/05/09 22:55:19, msw wrote: > nit: move this just below GetRunContainingCaret to match its decl. Done (sorted other methods too) https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:91: void ItemizeText(); On 2014/05/09 22:55:19, msw wrote: > nit: Add a comment like "Break the text into lines and runs." Done. https://codereview.chromium.org/152473008/diff/140001/ui/views/examples/multi... File ui/views/examples/multiline_example.cc (right): https://codereview.chromium.org/152473008/diff/140001/ui/views/examples/multi... ui/views/examples/multiline_example.cc:28: return range; On 2014/05/09 22:55:19, msw wrote: > nit: you can just return range.Intersect(gfx::Range(0, max)); That would return an invalid range when the intersection is empty.
Thanks for working on this, excited to see this happen! https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:360: sk_canvas->clipRect(RectToSkRect( The previous code restored at every iteration, whereas here you keep the clip from previous loop iterations. Is this really correct? Is there a reason this change is part of the harfbuzz CL or can it be a separate change? https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:416: switches::kEnableHarfBuzzRenderText)) Nit: {}'s https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:82: SkPaint paint() const { return paint_; } Is this used somewhere? https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:149: char* buffer = reinterpret_cast<char*>(malloc(table_size)); Can you use scoped_ptr for this instead? If not, add a comment as to why... If this explicitly needs malloc, then perhaps you can follow the FreeDeleter example from scoped_ptr.h? https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:196: unsigned int upem = hb_face_get_upem(face_cache->first); The params to hb_font_set_scale() seem to be ints, so this should be also. https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:211: for (size_t i = 0; i < run.glyph_count - 1; ++i) Nit: {}, since the body is multi-line. https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:217: for (size_t i = 0; i < run.glyph_count; ++i) Nit: {}, since the body is multi-line. https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:246: for (size_t i = 0; i < run.glyph_count; ++i) Nit: {}'s https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:254: (second > USCRIPT_INVALID_CODE && second <= USCRIPT_INHERITED)) Nit: {}'s https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:281: UScriptCode scripts[kMaxScripts] = {USCRIPT_INVALID_CODE}; Nit: Spaces between the {}'s. Note: I think this will initialize only the first element to USCRIPT_INVALID_CODE and the rest to 0. Is that the intention? Same for the function below. https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:374: if (offset < middle) Nit: {}'s https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:378: if (offset < end) Nit: {}'s https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:387: NOTIMPLEMENTED(); It would be good to implement this since it's exercised by tests. I think it should be pretty straight forward - check the other implementations. By the way, what do tests look like when using this RenderText implementation? How many of them work/don't work, etc? https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:475: } else if (iter.pos() >= selection.caret_pos()) { Nit: No else's after break. https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:585: // Precalculate run width information. Nit: Either the indent is wrong here or you're missing {}'s on the loop above.... https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:700: for (size_t run = 0; run < runs_.size(); ++run) Nit: {}'s https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:773: is_rtl ? UBIDI_DEFAULT_RTL : UBIDI_DEFAULT_LTR, NULL, &result); Nit: align https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. Nit: Remove (c) https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:105: }; DISALLOW_COPY_AND_ASSIGN()?
https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:360: sk_canvas->clipRect(RectToSkRect( On 2014/05/12 15:58:31, Alexei Svitkine wrote: > The previous code restored at every iteration, whereas here you keep the clip > from previous loop iterations. Is this really correct? > > Is there a reason this change is part of the harfbuzz CL or can it be a separate > change? We're not keeping the clip here, the SkRegion::kReplace_Op means "make this the new clip region" (as opposed to adding it). It can't be a separate CL without some changes to this CL. I'd like to avoid this and land this CL ASAP. Other RenderText implementations will soon depend on this as well (see the discussion on using canvas |Translate()|). https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:416: switches::kEnableHarfBuzzRenderText)) On 2014/05/12 15:58:31, Alexei Svitkine wrote: > Nit: {}'s Done. https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:82: SkPaint paint() const { return paint_; } On 2014/05/12 15:58:31, Alexei Svitkine wrote: > Is this used somewhere? Not anymore, removed. https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:149: char* buffer = reinterpret_cast<char*>(malloc(table_size)); On 2014/05/12 15:58:31, Alexei Svitkine wrote: > Can you use scoped_ptr for this instead? > > If not, add a comment as to why... > > If this explicitly needs malloc, then perhaps you can follow the FreeDeleter > example from scoped_ptr.h? scoped_ptr is not possible since we pass the ownership to HarfBuzz (which passes it back to the last argument of hb_blob_create). I changed the malloc/free pair to new/delete[]. https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:196: unsigned int upem = hb_face_get_upem(face_cache->first); On 2014/05/12 15:58:31, Alexei Svitkine wrote: > The params to hb_font_set_scale() seem to be ints, so this should be also. Done. https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:211: for (size_t i = 0; i < run.glyph_count - 1; ++i) On 2014/05/12 15:58:31, Alexei Svitkine wrote: > Nit: {}, since the body is multi-line. Done. https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:217: for (size_t i = 0; i < run.glyph_count; ++i) On 2014/05/12 15:58:31, Alexei Svitkine wrote: > Nit: {}, since the body is multi-line. Done. https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:246: for (size_t i = 0; i < run.glyph_count; ++i) On 2014/05/12 15:58:31, Alexei Svitkine wrote: > Nit: {}'s Done. https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:254: (second > USCRIPT_INVALID_CODE && second <= USCRIPT_INHERITED)) On 2014/05/12 15:58:31, Alexei Svitkine wrote: > Nit: {}'s Done. https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:281: UScriptCode scripts[kMaxScripts] = {USCRIPT_INVALID_CODE}; On 2014/05/12 15:58:31, Alexei Svitkine wrote: > Nit: Spaces between the {}'s. > > Note: I think this will initialize only the first element to > USCRIPT_INVALID_CODE and the rest to 0. Is that the intention? Same for the > function below. Done. Yes, none of the values matter, they are rewritten with the GetScriptExtensions call right below. https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:374: if (offset < middle) On 2014/05/12 15:58:31, Alexei Svitkine wrote: > Nit: {}'s Done. https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:378: if (offset < end) On 2014/05/12 15:58:31, Alexei Svitkine wrote: > Nit: {}'s Done. https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:387: NOTIMPLEMENTED(); On 2014/05/12 15:58:31, Alexei Svitkine wrote: > It would be good to implement this since it's exercised by tests. I think it > should be pretty straight forward - check the other implementations. We use Skia fonts in RenderTextHarfBuzz, not gfx::Font. Also the itemizer doesn't handler parentheses yet so RenderTextTest.SameFontForParentheses would fail anyway. I will fix that in a followup CL and find a way to test it. Perhaps some minor refactoring will have to happen to GetFontSpansForTesting. > By the way, what do tests look like when using this RenderText implementation? > How many of them work/don't work, etc? Overall it looks good. 2 tests fail as expected (non-implemented), 1 fails unexpectedly, 2-3 need to be disabled. https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:475: } else if (iter.pos() >= selection.caret_pos()) { On 2014/05/12 15:58:31, Alexei Svitkine wrote: > Nit: No else's after break. Done. https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:585: // Precalculate run width information. On 2014/05/12 15:58:31, Alexei Svitkine wrote: > Nit: Either the indent is wrong here or you're missing {}'s on the loop > above.... Wrong indent, fixed. https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:700: for (size_t run = 0; run < runs_.size(); ++run) On 2014/05/12 15:58:31, Alexei Svitkine wrote: > Nit: {}'s Done. https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:773: is_rtl ? UBIDI_DEFAULT_RTL : UBIDI_DEFAULT_LTR, NULL, &result); On 2014/05/12 15:58:31, Alexei Svitkine wrote: > Nit: align Done. https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/05/12 15:58:31, Alexei Svitkine wrote: > Nit: Remove (c) Done (and done for .cc too) https://codereview.chromium.org/152473008/diff/200001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:105: }; On 2014/05/12 15:58:31, Alexei Svitkine wrote: > DISALLOW_COPY_AND_ASSIGN()? Done.
https://codereview.chromium.org/152473008/diff/220001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/152473008/diff/220001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:61: CHECK_LE(codepoint, 0xFFFFU); Does this need to be a CHECK rather than a DCHECK? (i.e. what happens if this fails?) https://codereview.chromium.org/152473008/diff/220001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:139: font_funcs = hb_font_funcs_create(); Is this intentionally leaked? If so, please add a comment mentioning this is intentional. https://codereview.chromium.org/152473008/diff/220001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:159: char* buffer = new char[table_size]; How about: scoped_ptr<char[]> buffer(new char[table_size]); size_t actual_size = typeface->getTableData(tag, 0, table_size, buffer); if (table_size != actual_size) return 0; // Harbuzz takes ownership of |buffer|. return hb_blob_create(buffer.release(), ...); https://codereview.chromium.org/152473008/diff/220001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:211: size_t CharToGlyph(const internal::TextRunHarfBuzz& run, size_t pos) { Can you add short 1-line comments for all these functions in the anon namespace? https://codereview.chromium.org/152473008/diff/220001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:250: bool HasMissingGlyphs(const internal::TextRunHarfBuzz& run) { Can this be a method on TextRunHarfbuzz rather than a free-standing function? Same for all the other free-standing functions you currently have that take |run| as the first param. https://codereview.chromium.org/152473008/diff/220001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:313: int ScriptInterval(const base::char16* text, Can |text| be passed by const ref? https://codereview.chromium.org/152473008/diff/220001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:314: size_t length, Nit: Align params. https://codereview.chromium.org/152473008/diff/220001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/152473008/diff/220001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:107: bool needs_layout_; Nit: DISALLOW_COPY_AND_ASSIGN() on RenderTextHarfBuzz too?
https://codereview.chromium.org/152473008/diff/220001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/152473008/diff/220001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:61: CHECK_LE(codepoint, 0xFFFFU); On 2014/05/13 15:23:27, Alexei Svitkine wrote: > Does this need to be a CHECK rather than a DCHECK? (i.e. what happens if this > fails?) This is actually supposed to be a DCHECK, I mistranslated the webkit assertion macro. Fixed. I also changed another CHECK to DCHECK below. https://codereview.chromium.org/152473008/diff/220001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:139: font_funcs = hb_font_funcs_create(); On 2014/05/13 15:23:27, Alexei Svitkine wrote: > Is this intentionally leaked? If so, please add a comment mentioning this is > intentional. Done. https://codereview.chromium.org/152473008/diff/220001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:159: char* buffer = new char[table_size]; On 2014/05/13 15:23:27, Alexei Svitkine wrote: > How about: > > scoped_ptr<char[]> buffer(new char[table_size]); > size_t actual_size = typeface->getTableData(tag, 0, table_size, buffer); > if (table_size != actual_size) > return 0; > > // Harbuzz takes ownership of |buffer|. > return hb_blob_create(buffer.release(), ...); Oh, good idea. Done! Sadly I had to assign the released ptr to another variable since the function needs it twice. https://codereview.chromium.org/152473008/diff/220001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:211: size_t CharToGlyph(const internal::TextRunHarfBuzz& run, size_t pos) { On 2014/05/13 15:23:27, Alexei Svitkine wrote: > Can you add short 1-line comments for all these functions in the anon namespace? Done. https://codereview.chromium.org/152473008/diff/220001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:250: bool HasMissingGlyphs(const internal::TextRunHarfBuzz& run) { On 2014/05/13 15:23:27, Alexei Svitkine wrote: > Can this be a method on TextRunHarfbuzz rather than a free-standing function? > > Same for all the other free-standing functions you currently have that take > |run| as the first param. Good idea, but should we make it a proper class with accessors? Google C++ style guide seems to require that. (it says anything more than a collection of data should be a class, and classes should always expose data via accessors) https://codereview.chromium.org/152473008/diff/220001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:313: int ScriptInterval(const base::char16* text, On 2014/05/13 15:23:27, Alexei Svitkine wrote: > Can |text| be passed by const ref? Done. https://codereview.chromium.org/152473008/diff/220001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:314: size_t length, On 2014/05/13 15:23:27, Alexei Svitkine wrote: > Nit: Align params. Done. https://codereview.chromium.org/152473008/diff/220001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/152473008/diff/220001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:107: bool needs_layout_; On 2014/05/13 15:23:27, Alexei Svitkine wrote: > Nit: DISALLOW_COPY_AND_ASSIGN() on RenderTextHarfBuzz too? Oops... Done.
LGTM with minor nits and a comment request; great work! https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:275: void ScriptSetIntersect(UChar32 codepoint, On 2014/05/12 09:53:29, ckocagil wrote: > What if we have a string of 3 chars with the following script values: {Kana} > {Hira,Kana} {Kana} > > Wouldn't your method only take the first script values into account, thus > unnecessarily break the text into 3 items? That's a very good example, can you note it in the ScriptInterval function comment? Also, it'd be great if we could construct a unit test for such cases. https://codereview.chromium.org/152473008/diff/140001/ui/views/examples/multi... File ui/views/examples/multiline_example.cc (right): https://codereview.chromium.org/152473008/diff/140001/ui/views/examples/multi... ui/views/examples/multiline_example.cc:28: return range; On 2014/05/12 09:53:29, ckocagil wrote: > On 2014/05/09 22:55:19, msw wrote: > > nit: you can just return range.Intersect(gfx::Range(0, max)); > > That would return an invalid range when the intersection is empty. Good point, I'm glad you considered that case. https://codereview.chromium.org/152473008/diff/240001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/152473008/diff/240001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:339: DCHECK_GE(length, 0U); nit: This was DCHECK(length > 0) before, is 0 length okay? https://codereview.chromium.org/152473008/diff/240001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:349: if (scripts_size > 0U) nit: reverse the condition to return early, then set script afterwards.
https://codereview.chromium.org/152473008/diff/220001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/152473008/diff/220001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:250: bool HasMissingGlyphs(const internal::TextRunHarfBuzz& run) { On 2014/05/16 13:47:25, ckocagil wrote: > On 2014/05/13 15:23:27, Alexei Svitkine wrote: > > Can this be a method on TextRunHarfbuzz rather than a free-standing function? > > > > Same for all the other free-standing functions you currently have that take > > |run| as the first param. > > Good idea, but should we make it a proper class with accessors? Google C++ style > guide seems to require that. (it says anything more than a collection of data > should be a class, and classes should always expose data via accessors) I think it's OK to have a struct with a few methods. Though, if making it a full-fledged class with accessors, etc isn't super intrusive, then that SGTM to me too. https://codereview.chromium.org/152473008/diff/240001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/152473008/diff/240001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:27: // The maximum number of scripts a Unicode character can belong to. Can you expand this comment to explain where this magic value comes from? (either "5 is the max because spec XYZ says FOOBAR" or "5 is chosen arbitrary here because it's very unlikely that XYZ"... https://codereview.chromium.org/152473008/diff/240001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:30: typedef std::map<uint32_t, uint16_t> GlyphCache; Add a comment explaining what this mapping from and to. https://codereview.chromium.org/152473008/diff/240001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:322: ++out_size; If you're already keeping track of |out_size|, I think you don't need |out|, so you can just do: result[out_size++] = intersection;
https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/152473008/diff/140001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:275: void ScriptSetIntersect(UChar32 codepoint, On 2014/05/16 18:01:30, msw wrote: > On 2014/05/12 09:53:29, ckocagil wrote: > > What if we have a string of 3 chars with the following script values: {Kana} > > {Hira,Kana} {Kana} > > > > Wouldn't your method only take the first script values into account, thus > > unnecessarily break the text into 3 items? > > That's a very good example, can you note it in the ScriptInterval function > comment? Also, it'd be great if we could construct a unit test for such cases. Done, added TODO for a test (sorry for delaying so much stuff, I really want to land this first and then iteratively fix/improve/test) https://codereview.chromium.org/152473008/diff/220001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/152473008/diff/220001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:250: bool HasMissingGlyphs(const internal::TextRunHarfBuzz& run) { Done, moved these to the struct. https://codereview.chromium.org/152473008/diff/240001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/152473008/diff/240001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:27: // The maximum number of scripts a Unicode character can belong to. On 2014/05/16 18:22:53, Alexei Svitkine wrote: > Can you expand this comment to explain where this magic value comes from? > (either "5 is the max because spec XYZ says FOOBAR" or "5 is chosen arbitrary > here because it's very unlikely that XYZ"... Done. https://codereview.chromium.org/152473008/diff/240001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:30: typedef std::map<uint32_t, uint16_t> GlyphCache; On 2014/05/16 18:22:53, Alexei Svitkine wrote: > Add a comment explaining what this mapping from and to. Done. https://codereview.chromium.org/152473008/diff/240001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:322: ++out_size; On 2014/05/16 18:22:53, Alexei Svitkine wrote: > If you're already keeping track of |out_size|, I think you don't need |out|, so > you can just do: > > result[out_size++] = intersection; Done. https://codereview.chromium.org/152473008/diff/240001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:339: DCHECK_GE(length, 0U); On 2014/05/16 18:01:30, msw wrote: > nit: This was DCHECK(length > 0) before, is 0 length okay? Done. https://codereview.chromium.org/152473008/diff/240001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:349: if (scripts_size > 0U) On 2014/05/16 18:01:30, msw wrote: > nit: reverse the condition to return early, then set script afterwards. Done.
Still lgtm with a nit. https://codereview.chromium.org/152473008/diff/260001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/152473008/diff/260001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:32: // Returns whether the given shaped run contains any missing glyphs. nit: indentation.
LGTM % nits Thanks! https://codereview.chromium.org/152473008/diff/260001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/152473008/diff/260001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:227: bool IsUnusualBlockCode(const UBlockCode block_code) { Nit: No need for const https://codereview.chromium.org/152473008/diff/260001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:255: int count = uscript_getScriptExtensions(codepoint, scripts + 1, Nit: Might be good to add a comment explaining the + 1 here, since it's not obvious to me. https://codereview.chromium.org/152473008/diff/260001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:755: int RenderTextHarfBuzz::GetGlyphXBoundary(size_t run_index, Nit: Can this be a member method of TextRunHarfBuzz instead? https://codereview.chromium.org/152473008/diff/260001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:796: bool fake_runs = false; Nit: Add a comment about why we have this. https://codereview.chromium.org/152473008/diff/260001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:884: scoped_ptr<UBiDiLevel[]> levels(new UBiDiLevel[num_runs]); Nit: Can this be a std::vector<UBiDiLevel> instead?
https://codereview.chromium.org/152473008/diff/260001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/152473008/diff/260001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:227: bool IsUnusualBlockCode(const UBlockCode block_code) { On 2014/05/21 07:39:02, Alexei Svitkine wrote: > Nit: No need for const Done. https://codereview.chromium.org/152473008/diff/260001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:255: int count = uscript_getScriptExtensions(codepoint, scripts + 1, On 2014/05/21 07:39:02, Alexei Svitkine wrote: > Nit: Might be good to add a comment explaining the + 1 here, since it's not > obvious to me. Done. https://codereview.chromium.org/152473008/diff/260001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:755: int RenderTextHarfBuzz::GetGlyphXBoundary(size_t run_index, On 2014/05/21 07:39:02, Alexei Svitkine wrote: > Nit: Can this be a member method of TextRunHarfBuzz instead? Done. https://codereview.chromium.org/152473008/diff/260001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:796: bool fake_runs = false; On 2014/05/21 07:39:02, Alexei Svitkine wrote: > Nit: Add a comment about why we have this. Done. https://codereview.chromium.org/152473008/diff/260001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:884: scoped_ptr<UBiDiLevel[]> levels(new UBiDiLevel[num_runs]); On 2014/05/21 07:39:02, Alexei Svitkine wrote: > Nit: Can this be a std::vector<UBiDiLevel> instead? Done. https://codereview.chromium.org/152473008/diff/260001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/152473008/diff/260001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:32: // Returns whether the given shaped run contains any missing glyphs. On 2014/05/20 19:13:44, msw wrote: > nit: indentation. Done.
+sky: ui/views/examples/multiline_example.cc +behdad: please stamp, or take a look at the HarfBuzz usage if you wish. eae already reviewed the HarfBuzz parts.
LGTM https://codereview.chromium.org/152473008/diff/280001/ui/views/examples/multi... File ui/views/examples/multiline_example.cc (right): https://codereview.chromium.org/152473008/diff/280001/ui/views/examples/multi... ui/views/examples/multiline_example.cc:25: gfx::Range ClampRange(gfx::Range range, size_t max) { const gfx::Range&
lgtm
The CQ bit was checked by ckocagil@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckocagil@chromium.org/152473008/280001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/7869)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
The CQ bit was checked by ckocagil@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckocagil@chromium.org/152473008/300001
The CQ bit was unchecked by ckocagil@chromium.org
The CQ bit was checked by ckocagil@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckocagil@chromium.org/152473008/340001
Message was sent while issue was closed.
Change committed as 272260
Message was sent while issue was closed.
After this CL, I am getting the following linking error on Linux (Ubuntu 14.04): ...bgpu_unittest_utils.a obj/ui/gl/libgl_unittest_utils.a obj/ppapi/libppapi_unittest_shared.a obj/ipc/libtest_support_ipc.a obj/ui/aura/libaura_test_support.a obj/ui/compositor/libcompositor_test_support.a obj/ui/wm/libwm.a obj/components/libbreakpad_component.a obj/breakpad/libbreakpad_client.a obj/third_party/WebKit/Source/web/libwebkit_test_support.a obj/third_party/WebKit/Source/core/libwebcore_testing.a obj/third_party/WebKit/Source/modules/libmodules_testing.a obj/components/libbreakpad_host.a obj/ui/views/controls/webview/libwebview.a obj/ui/web_dialogs/libweb_dialogs.a obj/ui/views/libviews.a obj/ui/display/libdisplay_util.a obj/ui/views/libviews_test_support.a lib/libfreetype.so.6 -Wl,--end-group -lrt -ldl -lgmodule-2.0 -lgobject-2.0 -lgthread-2.0 -lglib-2.0 -lnss3 -lnssutil3 -lsmime3 -lplds4 -lplc4 -lnspr4 -lgconf-2 -lgio-2.0 -lresolv -lfontconfig -lfreetype -lpangocairo-1.0 -lcairo -lpangoft2-1.0 -lpango-1.0 -lexpat -lharfbuzz -lX11 -lXi -lXcursor -lXext -lXfixes -lXrender -lXcomposite -lasound -lXdamage -lXtst -lXrandr -lcups -lgssapi_krb5 -lkrb5 -lk5crypto -lcom_err -lgcrypt -lz -lpthread -lm -lcrypt -lcap -lcrypto -ludev -ldbus-1 obj/ui/views/libviews.a(obj/ui/views/window/views.custom_frame_view.o):custom_frame_view.cc:function views::CustomFrameView::LayoutWindowControls(): error: undefined reference to 'views::WindowButtonOrderProvider::GetInstance()' obj/ui/gfx/libgfx.a(obj/ui/gfx/gfx.render_text_harfbuzz.o):render_text_harfbuzz.cc:function gfx::RenderTextHarfBuzz::ShapeRun(gfx::internal::TextRunHarfBuzz*): error: undefined reference to 'hb_icu_script_to_script' collect2: error: ld returned 1 exit status
Message was sent while issue was closed.
@ch.dumez: Does r272355 fix it? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
