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

Issue 9467017: Password support for NativeTextfieldViews, in the model (Closed)

Created:
8 years, 10 months ago by benrg
Modified:
8 years, 9 months ago
Reviewers:
xji
CC:
chromium-reviews, tfarina, James Su, yusukes+watch_chromium.org, penghuang+watch_chromium.org, brettw-cc_chromium.org, Alexei Svitkine (slow)
Visibility:
Public.

Description

Password support for NativeTextfieldViews, in the model This is an alternate implementation of issue 8747001 (see discussion there). Advantages: * It works in Windows. * It's arguably a simpler design: the abstraction boundary between the obscured and real text is the same as the abstraction boundary between RenderText and TextfieldViewsModel. Problems: * The cursoring code should not be in RenderText, and moving it into the model where it belongs would break the aforementioned abstraction. In other words, I think this design is simpler by accident (the fact that the other version doesn't work on Windows is also an accident). In the long term I think I would rather maintain the other version, though it's possible I'm overlooking something. * It's an ugly change in practice because a lot of calls to RenderText have to be wrapped. The RenderText version touches less code if you ignore the gratuitous identifier renaming. I could try introducing a RenderTextWrapper class instead of patching each call site, but I'm not sure it would help. There are no unit tests yet, which means this code is probably broken. But the basic functionality is there. BUG=105054 TEST=

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+267 lines, -70 lines) Patch
M base/utf_offset_string_conversions.h View 2 chunks +31 lines, -1 line 0 comments Download
M base/utf_offset_string_conversions.cc View 4 chunks +40 lines, -1 line 0 comments Download
M base/utf_offset_string_conversions_unittest.cc View 3 chunks +24 lines, -2 lines 0 comments Download
M ui/views/controls/textfield/native_textfield_views.cc View 6 chunks +26 lines, -26 lines 0 comments Download
M ui/views/controls/textfield/native_textfield_views_unittest.cc View 2 chunks +15 lines, -0 lines 0 comments Download
M ui/views/controls/textfield/textfield_views_model.h View 3 chunks +28 lines, -1 line 0 comments Download
M ui/views/controls/textfield/textfield_views_model.cc View 18 chunks +103 lines, -39 lines 0 comments Download

Messages

Total messages: 1 (0 generated)
xji
8 years, 9 months ago (2012-02-27 23:02:51 UTC) #1
I do not like this approach because the interface is not clean. The visual text
is saved in RenderText while the logical text is saved in TextfieldViewsModel.
The updated code is kind of hacky.

If we do not need to save the original logical text, but only need to save the
variation (such as the LOWERCASE style in
http://code.google.com/p/chromium/issues/detail?id=109308), it is ok to do
transformation in upper layer.

If we need to save and operate on both, what Alexei suggested makes sense. But I
feel that having 2 RenderText is more complicated so that it is worth if this
approach is scalable. For this case, we do not need to save the index mapping
between original text and obscured text because we can use conversions between
UTF16 index and code point offset. So, update the obscured RenderText with the
correct length string and cursor at draw time is easy. But how about we need to
save the mapping between those two? Then, we will need to save it in the model,
plus having 2 RenderText operate on visual and logical level respectively, where
we never need to layout the logical text.

Part of the heaviness of RenderText is that we designed it for editing text
only. Maybe we can split it into display and edit mode (plus the refactor Ben
suggested).

Powered by Google App Engine
This is Rietveld 408576698