Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
915383003:
Fix alignment format in multiline mode (Closed)
DescriptionFix alignment format in multiline mode
BUG=461182
TEST=covered by RenderTextTest.Multiline_HorizontalAlignemnt
Committed: https://crrev.com/403e56062fff5bdbca47745201e075b2916152ba
Cr-Commit-Position: refs/heads/master@{#317750}
Patch Set 1 : #
Total comments: 7
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 10
Patch Set 5 : #Patch Set 6 : disable test on mac #
Total comments: 2
Messages
Total messages: 27 (13 generated)
Patchset #5 (id:70001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:60001) has been deleted
oshima@chromium.org changed reviewers: + mukai@chromium.org
fyi
mukai@chromium.org changed reviewers: + msw@chromium.org
+msw for the review. It looks good to me though.
https://codereview.chromium.org/915383003/diff/90001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/915383003/diff/90001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1090: #if defined(OS_WIN) || defined(OS_CHROMEOS) This should actually check if the current instance is a RenderTextHarfBuzz instance... RenderTextMac doesn't support multiline, but Mac also creates RenderTextHarfBuzz instances via CreateInstanceForEditing. It'd be nice if you can solve that elegantly, otherwise, I suppose you can just make this "if !defined(OS_MACOSX)" for now; ditto below. https://codereview.chromium.org/915383003/diff/90001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1096: #if !defined(OS_WIN) || defined(OS_CHROMEOS) This is *definitely* wrong with the "!" added for OS_WIN; see above. https://codereview.chromium.org/915383003/diff/90001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/915383003/diff/90001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:2037: // Second string should be always shorter, thus nit: consider: "The second line in the test string is always shorter," https://codereview.chromium.org/915383003/diff/90001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:2038: // the seond line has longer alignment. nit: "second"
mukai@, sorry if it wasn't clear. This wasn't for code review. I just wanted to let you know that there is missing piece in your multiline CL for RTHB, so that you can include this in your RTL format CL. https://codereview.chromium.org/915383003/diff/90001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/915383003/diff/90001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1090: #if defined(OS_WIN) || defined(OS_CHROMEOS) On 2015/02/19 01:13:50, msw wrote: > This should actually check if the current instance is a RenderTextHarfBuzz > instance... RenderTextMac doesn't support multiline, but Mac also creates > RenderTextHarfBuzz instances via CreateInstanceForEditing. It'd be nice if you > can solve that elegantly, otherwise, I suppose you can just make this "if > !defined(OS_MACOSX)" for now; ditto below. That means this is already broken and should be fixed separately. This change was missing piece in mukai's@ multiline support CL for RTHB.
looks like this wasn't in your RTL fix? can you take over this CL and update the test? (test didn't work as intended due to the bug you fixed)
Patchset #4 (id:150001) has been deleted
oshima@chromium.org changed reviewers: - mukai@chromium.org
Ok, I'm back and uploaded the new patch with updated test and cleanups. msw@, PTAL. https://codereview.chromium.org/915383003/diff/90001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/915383003/diff/90001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1090: #if defined(OS_WIN) || defined(OS_CHROMEOS) On 2015/02/19 04:40:14, oshima (OOO Fab 17 - Feb 20) wrote: > On 2015/02/19 01:13:50, msw wrote: > > This should actually check if the current instance is a RenderTextHarfBuzz > > instance... RenderTextMac doesn't support multiline, but Mac also creates > > RenderTextHarfBuzz instances via CreateInstanceForEditing. It'd be nice if you > > can solve that elegantly, otherwise, I suppose you can just make this "if > > !defined(OS_MACOSX)" for now; ditto below. > > That means this is already broken and should be fixed separately. > > This change was missing piece in mukai's@ multiline support CL for RTHB. Used !defined(OS_MACOSX) in this CL. I uploaded the solution w/o ifdef in patch set #3, but want to do that in separate CL as I need to merge this to 42. https://codereview.chromium.org/915383003/diff/90001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1096: #if !defined(OS_WIN) || defined(OS_CHROMEOS) On 2015/02/19 01:13:50, msw wrote: > This is *definitely* wrong with the "!" added for OS_WIN; see above. Sorry, this was from my experiment. (I was playing with this on/off). I did clean up as you suggested.
lgtm with nits. https://codereview.chromium.org/915383003/diff/170001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/915383003/diff/170001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:1095: // TODO(ckocagil): Enable |lines_| usage on mac. nit: update this comment to say RenderTextMac instead of just "mac" https://codereview.chromium.org/915383003/diff/170001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/915383003/diff/170001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:606: FRIEND_TEST_ALL_PREFIXES(RenderTextTest, Multiline_HorizontalAlignment); nit: Isn't being a friend test of rthb enough? Is this needed? https://codereview.chromium.org/915383003/diff/170001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/915383003/diff/170001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2147: // Ensure RTL alignment works. nit: Is this really specific to RTL or just multi-line alignment? https://codereview.chromium.org/915383003/diff/170001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2153: { L"abcdefghij\nhijkl", gfx::ALIGN_LEFT}, nit: add spaces before closing curly braces throughout. https://codereview.chromium.org/915383003/diff/170001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2153: { L"abcdefghij\nhijkl", gfx::ALIGN_LEFT}, nit: maybe add an english, 2nd line longer case? (I guess that doesn't affect the tested outcomes...)
https://codereview.chromium.org/915383003/diff/170001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/915383003/diff/170001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:1095: // TODO(ckocagil): Enable |lines_| usage on mac. On 2015/02/23 19:30:06, msw wrote: > nit: update this comment to say RenderTextMac instead of just "mac" Done. https://codereview.chromium.org/915383003/diff/170001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/915383003/diff/170001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:606: FRIEND_TEST_ALL_PREFIXES(RenderTextTest, Multiline_HorizontalAlignment); On 2015/02/23 19:30:06, msw wrote: > nit: Isn't being a friend test of rthb enough? Is this needed? Thanks for catching . This was added to access GetAlignmentOffset, but no longer needed as I added one in RTHB to access set_glyph_witdth_for_test. https://codereview.chromium.org/915383003/diff/170001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/915383003/diff/170001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2147: // Ensure RTL alignment works. On 2015/02/23 19:30:07, msw wrote: > nit: Is this really specific to RTL or just multi-line alignment? I added RTL case for assertion purpose, but updated the comment anyway. https://codereview.chromium.org/915383003/diff/170001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2153: { L"abcdefghij\nhijkl", gfx::ALIGN_LEFT}, On 2015/02/23 19:30:07, msw wrote: > nit: maybe add an english, 2nd line longer case? > (I guess that doesn't affect the tested outcomes...) Done. https://codereview.chromium.org/915383003/diff/170001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2153: { L"abcdefghij\nhijkl", gfx::ALIGN_LEFT}, On 2015/02/23 19:30:07, msw wrote: > nit: add spaces before closing curly braces throughout. Done.
The CQ bit was checked by oshima@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/915383003/#ps210001 (title: "disable test on mac")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/915383003/210001
The CQ bit was unchecked by oshima@chromium.org
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/915383003/210001
still lgtm, but I'd appreciate an extra comment for the test. https://codereview.chromium.org/915383003/diff/210001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/915383003/diff/210001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2147: #if !defined(OS_MACOSX) nit: Oh darn, this should work on Mac (using RTHB), except for the overly-broad macros in GetAlignmentOffset... Add a TODO (or note this test in that function).
Message was sent while issue was closed.
Committed patchset #6 (id:210001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/403e56062fff5bdbca47745201e075b2916152ba Cr-Commit-Position: refs/heads/master@{#317750}
Message was sent while issue was closed.
https://codereview.chromium.org/915383003/diff/210001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/915383003/diff/210001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2147: #if !defined(OS_MACOSX) On 2015/02/24 03:27:59, msw wrote: > nit: Oh darn, this should work on Mac (using RTHB), except for the overly-broad > macros in GetAlignmentOffset... Add a TODO (or note this test in that function). This won't work on mac as is. This should work with patch set #3, and will re-enable there. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
