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

Issue 1130683005: Nix Windows-specific textfield move-by-word behavior; enable tests.

Created:
5 years, 7 months ago by msw
Modified:
5 years, 7 months ago
CC:
chromium-reviews, derat+watch_chromium.org, oshima, ckocagil, sadrul
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Nix Windows-specific textfield move-by-word behavior; enable tests. Use the existing code for Linux and Chrome OS on Windows too. It's more well tested and doesn't suffer from known RTL defects. We originally intended to mimic platform-specific nuances. The current Win-specific code does not seem worth keeping: 1) It suffers from a long-standing RTL left/right reversal defect. 2) It lacks automated testing (existing tests were disabled on Windows). 3) It doesn't match Windows perfectly: <http://crbug.com/196326#c5>; 4) The benefits of copying quicks are dubious: <http://crbug.com/196326#c6>; We can invest in matching Windows behavior correctly later, if that's desirable. The HarfBuzz code was added in https://codereview.chromium.org/421053002 It was based off of the corresponding Pango code at the time: https://chromium.googlesource.com/chromium/src/+/925fea1e3bece4b2e8909c5731cc4874a8cab80d/ui/gfx/render_text_pango.cc TEST=Cursor move-by-word on Windows Omnibox, etc. works well. BUG=486485, 196326 R=asvitkine@chromium.org

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+0 lines, -39 lines) Patch
M ui/gfx/render_text_harfbuzz.cc View 2 chunks +0 lines, -36 lines 0 comments Download
M ui/gfx/render_text_unittest.cc View 2 chunks +0 lines, -3 lines 0 comments Download

Messages

Total messages: 13 (5 generated)
msw
Hey Alexei, please take a look; thanks!
5 years, 7 months ago (2015-05-13 00:32:31 UTC) #2
Alexei Svitkine (slow)
code lgtm, though I guess I don't know the context here - why did we ...
5 years, 7 months ago (2015-05-13 14:51:51 UTC) #3
msw
On 2015/05/13 14:51:51, Alexei Svitkine wrote: > code lgtm, though I guess I don't know ...
5 years, 7 months ago (2015-05-13 21:29:30 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1130683005/1
5 years, 7 months ago (2015-05-13 21:30:02 UTC) #8
Peter Kasting
I'm concerned about this CL. Saying we don't currently match Windows perfectly doesn't necessarily justify ...
5 years, 7 months ago (2015-05-13 22:35:28 UTC) #9
msw
On 2015/05/13 22:35:28, Peter Kasting wrote: > I'm concerned about this CL. Saying we don't ...
5 years, 7 months ago (2015-05-13 23:13:56 UTC) #11
Peter Kasting
On 2015/05/13 23:13:56, msw wrote: > On 2015/05/13 22:35:28, Peter Kasting wrote: > > I'm ...
5 years, 7 months ago (2015-05-13 23:16:00 UTC) #12
msw
5 years, 7 months ago (2015-05-14 00:16:48 UTC) #13
On 2015/05/13 23:16:00, Peter Kasting wrote:
> On 2015/05/13 23:13:56, msw wrote:
> > On 2015/05/13 22:35:28, Peter Kasting wrote:
> > > I'm concerned about this CL.  Saying we don't currently match Windows
> > perfectly
> > > doesn't necessarily justify moving further away from Windows native
> behavior. 
> > > What Windows-native behaviors are we losing here?
> > > 
> > > I don't think we should just throw this away and say maybe we'll implement
> > > Windows-native behavior someday if someone wants it.
> > 
> > I don't recall what Windowisms were actually implemented.
> > Do you want to take a closer look? I'm not terribly concerned.
> 
> I don't want this code disabled if you're not even sure what the consequences
of
> disabling it are.  Please look more closely.
> 
> If you can provide a list of the behavioral consequences of this it may be
that
> we'll want to make the change, but it needs to be clear what we're actually
> doing.

Perhaps I'll fix the RTL defect in the Windows code path before pursuing this
change any further. Thoughts?

Otherwise, I'll take a quick closer look tomorrow. Hopefully comparing these
code paths' differences to some quick tests of Windows native behavior will
suffice. Otherwise, it'd be unreasonable to perform an exhaustive manual audit
of three word breaking patterns over various cursor/selection states with
permutations of text from all supported character sets and IME scenarios.

FYI, code archeology takes me through my initial RenderText impl
<https://codereview.chromium.org/7265011>, where I based
RenderText::Get[Left|Right]CursorPosition off of
TextfieldViewsModel::MoveCursorTo[Previous|Next]Word, which Oshima originally
implemented in <https://codereview.chromium.org/5857002> and Sadrul refined with
some prescriptive behavior in <https://codereview.chromium.org/5972008>.

Powered by Google App Engine
This is Rietveld 408576698