Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(38)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 185123002: Init BidiResolver with Neutral directionality in RenderText (Closed)

Created:
6 years, 9 months ago by eae
Modified:
6 years, 9 months ago
CC:
blink-reviews, bemjb+rendering_chromium.org, dsinclair, zoltan1, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, pdr., cbiesinger
Visibility:
Public.

Description

Init BidiResolver with Neutral directionality in RenderText Set the initial directionality to Neutral in RenderText::computePreferredLogicalWidths. This ensures that leading and trailing SCRIPT_COMMON characters gets the correct directionality and thus the correct width. TEST=fast/text/international/rtl-space-in-ltr-element.html BUG=333004 R=leviw@chromium.org Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168339

Patch Set 1 : #

Total comments: 1

Patch Set 2 : addressing leviws concerns #

Patch Set 3 : w/fix for nbsp #

Patch Set 4 : re-uploading for 500 #

Total comments: 1

Patch Set 5 : patch for landing #

Patch Set 6 : patch for landing (trying again, stupid 500s) #

Patch Set 7 : land #

Unified diffs Side-by-side diffs Delta from patch set Stats (+40 lines, -4 lines) Patch
A LayoutTests/fast/text/international/rtl-space-in-ltr-element.html View 1 chunk +16 lines, -0 lines 0 comments Download
A LayoutTests/fast/text/international/rtl-space-in-ltr-element-expected.html View 1 chunk +16 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderText.cpp View 1 2 3 4 5 6 2 chunks +8 lines, -4 lines 0 comments Download

Messages

Total messages: 17 (0 generated)
eae
6 years, 9 months ago (2014-03-01 00:39:36 UTC) #1
leviw_travelin_and_unemployed
https://codereview.chromium.org/185123002/diff/20001/Source/core/rendering/RenderText.cpp File Source/core/rendering/RenderText.cpp (right): https://codereview.chromium.org/185123002/diff/20001/Source/core/rendering/RenderText.cpp#newcode946 Source/core/rendering/RenderText.cpp:946: textRun.setDirection(bidiResolver.determineParagraphDirectionality(&hasStrongDirectionality)); This is confusing since we're only running on ...
6 years, 9 months ago (2014-03-01 01:37:09 UTC) #2
leviw_travelin_and_unemployed
This it great! LGTM!
6 years, 9 months ago (2014-03-03 19:21:05 UTC) #3
leviw_travelin_and_unemployed
The CQ bit was checked by leviw@chromium.org
6 years, 9 months ago (2014-03-03 19:22:00 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eae@chromium.org/185123002/40001
6 years, 9 months ago (2014-03-03 19:22:36 UTC) #5
leviw_travelin_and_unemployed
https://codereview.chromium.org/185123002/diff/80001/Source/core/rendering/RenderText.cpp File Source/core/rendering/RenderText.cpp (right): https://codereview.chromium.org/185123002/diff/80001/Source/core/rendering/RenderText.cpp#newcode955 Source/core/rendering/RenderText.cpp:955: while (i > run->stop() || (run->next() && run->next()->direction() == ...
6 years, 9 months ago (2014-03-03 19:27:45 UTC) #6
eae
The CQ bit was checked by eae@chromium.org
6 years, 9 months ago (2014-03-03 19:37:10 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eae@chromium.org/185123002/120001
6 years, 9 months ago (2014-03-03 19:38:24 UTC) #8
eae
The CQ bit was unchecked by eae@chromium.org
6 years, 9 months ago (2014-03-03 21:51:33 UTC) #9
eae
The CQ bit was checked by eae@chromium.org
6 years, 9 months ago (2014-03-03 23:08:52 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eae@chromium.org/185123002/140001
6 years, 9 months ago (2014-03-03 23:09:21 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eae@chromium.org/185123002/140001
6 years, 9 months ago (2014-03-03 23:24:10 UTC) #12
eae
Committed patchset #7 manually as r168339 (presubmit successful).
6 years, 9 months ago (2014-03-03 23:30:43 UTC) #13
cbiesinger
On 2014/03/03 23:30:43, eae wrote: > Committed patchset #7 manually as r168339 (presubmit successful). Hmm, ...
6 years, 9 months ago (2014-03-04 01:06:58 UTC) #14
eae
On 2014/03/04 01:06:58, cbiesinger wrote: > On 2014/03/03 23:30:43, eae wrote: > > Committed patchset ...
6 years, 9 months ago (2014-03-04 01:56:02 UTC) #15
cbiesinger
On 2014/03/04 01:56:02, eae wrote: > On 2014/03/04 01:06:58, cbiesinger wrote: > > On 2014/03/03 ...
6 years, 9 months ago (2014-03-04 04:40:16 UTC) #16
eae
6 years, 9 months ago (2014-03-04 16:38:12 UTC) #17
Message was sent while issue was closed. 
On 2014/03/04 04:40:16, cbiesinger wrote:
> On 2014/03/04 01:56:02, eae wrote:
> > On 2014/03/04 01:06:58, cbiesinger wrote:
> > > On 2014/03/03 23:30:43, eae wrote:
> > > > Committed patchset #7 manually as r168339 (presubmit successful).
> > > 
> > > Hmm, did this cause fast/text/international/rtl-space-in-ltr-element.html
to
> > > fail on mac?
> > 
> > It is a new test added by this change and it passed both mac bots.
> 
> well it's failing on the bots:
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fas...

Our infrastructure truely does suck. Let's suppress it for now, it'll start
working when we move mac over to the new font stack.

Powered by Google App Engine
This is Rietveld 408576698