Composition input method lacks character echo in password input fields

• Component: Forms
• Status: RESOLVED FIXED

[Petri Ojala]

This bug report originated from issue QTBUG-6704
http://bugreports.qt.nokia.com/browse/QTBUG-6704

For non-touch screen phones, the input character is shown as *, which is correct of course. But i have no idea what i have typed in, in small letter or capital letter. For the touch-sreen input method, it will show you the letter that you tpyed in and then displays it as *.

My suggestion is to improve the input method for non-touch screens, while using the keyboard to fill in the letters, the letter filled in should be displayed, when confirmed, it shows *, exactly as how it is working on the touch-screen.

So, it should have character echo before echoing *.

[Petri Ojala]

Created attachment 44787 [details]
testpage

[Petri Ojala]

Can be reproduced with N97 in landscape mode

[Janne Koskinen]

This is serious usability issue when using T9 as if you can't see the character being echoed you need to count the presses to know which character you entered.

[Simon Hausmann]

I wonder if this could be solved inside qcoefepinputcontext_s60.cpp, so that it would also work with QLineEdit's password mode and the code wouldn't have to be duplicated.

 Do you think that's possible/feasible/makes sense?

[Janne Koskinen]

(In reply to comment #4)
> I wonder if this could be solved inside qcoefepinputcontext_s60.cpp, so that it
> would also work with QLineEdit's password mode and the code wouldn't have to be
> duplicated.
> 
>  Do you think that's possible/feasible/makes sense?

A common place for echoing would be good idea indeed. I tried simple QLineEdit and the feature is completely missing. I'll change the Qt bug to better reflect the situation.

[Simon Hausmann]

(In reply to comment #5)
> (In reply to comment #4)
> > I wonder if this could be solved inside qcoefepinputcontext_s60.cpp, so that it
> > would also work with QLineEdit's password mode and the code wouldn't have to be
> > duplicated.
> > 
> >  Do you think that's possible/feasible/makes sense?
> 
> A common place for echoing would be good idea indeed. I tried simple QLineEdit
> and the feature is completely missing. I'll change the Qt bug to better reflect
> the situation.

Hmm, I actually can't think of a way to achieve this from just the input method, as it merely affects the rendering of the character.

That means it is a feature of the rendering container (WebKit and QLineEdit) and not a feature of the input method.

[Joseph Ligman]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > I wonder if this could be solved inside qcoefepinputcontext_s60.cpp, so that it
> > > would also work with QLineEdit's password mode and the code wouldn't have to be
> > > duplicated.
> > > 
> > >  Do you think that's possible/feasible/makes sense?
> > 
> > A common place for echoing would be good idea indeed. I tried simple QLineEdit
> > and the feature is completely missing. I'll change the Qt bug to better reflect
> > the situation.
> 
> Hmm, I actually can't think of a way to achieve this from just the input
> method, as it merely affects the rendering of the character.
> 
> That means it is a feature of the rendering container (WebKit and QLineEdit)
> and not a feature of the input method.


What we did for S60 Browser in the past: (with a few details omitted)

In RenderText.cpp 


#if PLATFORM(SYMBIAN)

void RenderText::securityTimerFired(Timer<RenderText>*)
{
    switch (style()->textSecurity()) {
        case TSCIRCLE:
            m_text = m_text->secure(whiteBullet);
            break;
        case TSDISC:
            m_text = m_text->secure(bullet);
            break;
        case TSSQUARE:
            m_text = m_text->secure(blackSquare);
    }
    setNeedsLayoutAndPrefWidthsRecalc();
}

#endif

#if PLATFORM(SYMBIAN)
	
        // We use the same characters here as for list markers.
        // See the listMarkerText function in RenderListMarker.cpp.
        switch (style()->textSecurity()) {
            case TSNONE:
                break;
            case TSCIRCLE:
                m_text = m_text->secureShowOffset(bullet, m_offset);
                break;
            case TSDISC:
                if(backspace){
                        m_text = m_text->secure(bullet);
                }
                else{
                    m_text = m_text->secureShowOffset(bullet, m_offset);
                }
                       
               break;
            case TSSQUARE:
                m_text = m_text->secureShowOffset(blackSquare, m_offset);
        }
       
        if (style()->textSecurity() != TSNONE) {
            m_securityTimer.stop();
            m_securityTimer.startOneShot(KSecureTimerTimeout);
        }
       
#else 
...

[Petri Ojala]

Created attachment 46124 [details]
proposed patch

Attaching patch.

[WebKit Review Bot]

Attachment 46124 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/RenderText.h:183:  Should have a space between // and comment  [whitespace/comments] [4]
WebCore/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:18:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4

[Petri Ojala]

Created attachment 46125 [details]
Updated patch

Updated patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46125 [details] without any errors.

[Kristian Amlie]

I think a much more correct fix for this problem is to implement the timer logic in the input methods of Qt, by having it sending preedit text to the widget and only commit it after the timer expires. That would be a generic fix that would work for all Qt widgets and webkit as well. It would render this bug moot, so I'm going to try that next week.

[Simon Hausmann]

Comment on attachment 46125 [details]
Updated patch

At the least this should not be just be PLATFORM(SYMBIAN) #ifdef'fed. The same feature is needed for example on Maemo 5, too.

However if Kristian can solve this inside Qt's input method, then that's of course the cleanest solution, as it would fix all occurrences of this problem in one place.

[Kristian Amlie]

Ok, this has been solved for general Qt widgets by introducing a temporary 1-second preedit text in the input methods that Qt uses (commit 87ee066fc86 in the Qt repository).

This bug is still valid though, since WebKit does not display preedit text as clear text, but masks that also as password text. This is IMO a bug, since this makes any kind of advanced input into a password field nearly impossible. The suggested fix is now to fix WebKit to display this preedit text also in password fields. Whether or not you want to reuse this bug report for that issue or not, I leave to you.

[Samuel Nevala]

Created attachment 53669 [details]
patch

[WebKit Review Bot]

Attachment 53669 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/editing/ReplaceTextInNodeCommand.h:20:  #ifndef header guard has wrong style, please use: ReplaceTextInNodeCommand_h  [build/header_guard] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Samuel Nevala]

Created attachment 53670 [details]
patch

[WebKit Review Bot]

Attachment 53670 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/editing/ReplaceTextInNodeCommand.h:20:  #ifndef header guard has wrong style, please use: ReplaceTextInNodeCommand_h  [build/header_guard] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Samuel Nevala]

Created attachment 53671 [details]
patch

[WebKit Review Bot]

Attachment 53669 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1602587

[WebKit Review Bot]

Attachment 53671 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1569608

[Antonio Gomes]

(In reply to comment #19)
> Created an attachment (id=53671) [details]
> patch

I think the approach looks good, but unless you improve the test covarage of this feature/fix, and make it build all perrs plaftorms/ports (including Mac and chromium), it can be harder to be accepted, so I'd work on LayoutTests for this.

Also Mac would likely fail to build your patch as well.

[Simon Hausmann]

Comment on attachment 53671 [details]
patch

I can do an initial review on the generics, but for exact workings of the editing code
I'm not the right one to do the review.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index c27fce0..c3c01e1 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,25 @@
> [phone]  Samuel Nevala  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [Qt] 3rd edition phones lack character echo in web input fields
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=32509
> +

Please explain your change in the changelog.

> --- a/WebCore/WebCore.pro
> +++ b/WebCore/WebCore.pro
> @@ -568,6 +568,7 @@ SOURCES += \
>      editing/RemoveNodePreservingChildrenCommand.cpp \
>      editing/ReplaceNodeWithSpanCommand.cpp \
>      editing/ReplaceSelectionCommand.cpp \
> +    editing/ReplaceTextInNodeCommand.cpp \
>      editing/SelectionController.cpp \
>      editing/SetNodeAttributeCommand.cpp \
>      editing/SmartReplace.cpp \
> @@ -1289,6 +1290,7 @@ HEADERS += \
>      editing/RemoveNodePreservingChildrenCommand.h \
>      editing/ReplaceNodeWithSpanCommand.h \
>      editing/ReplaceSelectionCommand.h \
> +    editing/ReplaceTextInNodeCommand.h \
>      editing/SelectionController.h \
>      editing/SetNodeAttributeCommand.h \
>      editing/SmartReplace.h \

When adding new files to the build I think you'll have to add them to the
other build systems, too.

> +UChar CharacterData::secureTextChar()

Shouldn't this function be const?

[Samuel Nevala]

>>I think the approach looks good, but unless you improve the test covarage of
>>this feature/fix, and make it build all perrs plaftorms/ports (including Mac
>>and chromium), it can be harder to be accepted, so I'd work on LayoutTests for
>>this.
>>Also Mac would likely fail to build your patch as well.

I don't have mac in use. It is bit difficult to verify mac build and I would appreciate help on that side.

About layout tests, is it possible to acquire secured pass text using test environment? By secured pass text I mean text value converted to secure chars.

>>When adding new files to the build I think you'll have to add them to the
>>other build systems, too.

I assuming that you mean adding new files also to Android.mk & GNUmakefile.am. I there other places where new file should be added?

>>Shouldn't this function be const?
Yes, I'll correct that.

[Samuel Nevala]

Created attachment 56235 [details]
Latest_Patch

[WebKit Review Bot]

Attachment 56235 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "WebKit/qt/tests/qwebpage/tst_qwebpage.cpp"
WebCore/dom/CharacterData.cpp:164:  Missing spaces around ==  [whitespace/operators] [3]
WebCore/dom/CharacterData.cpp:229:  Missing space before ( in if(  [whitespace/parens] [5]
WebCore/dom/CharacterData.cpp:229:  Mismatching spaces inside () in if  [whitespace/parens] [5]
WebCore/rendering/RenderText.cpp:1077:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 4 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Samuel Nevala]

Created attachment 56319 [details]
La

[Samuel Nevala]

Created attachment 56320 [details]
Latest_Patch

Fixed style.

[WebKit Review Bot]

Attachment 56320 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/2258404

[Janne Koskinen]

buildbot has false positive on this?

Can someone review this as again without the patch in Symbian devices you cannot enter password protected websites.

[Simon Hausmann]

I've briefly spoken with Samuel also on irc, but here's my take on this:

As far as I can see the information about whether the text should be shown plain or "secured" (the text security) is stored in the render style. The logic for displaying the text is in the render object. Therefore I think the logic for implementing this feature should also be in the render object (RenderText, RenderTextControlSingleLine, etc.).

I think adding this feature specific preedit flag to WebCore::String is not a good idea.

[Simon Hausmann]

Comment on attachment 56320 [details]
Latest_Patch

AFAIK Samuel is working on a new revision of the same patch but in a much less intrusive way. Therefore I'm clearing the review for this patch.

[Samuel Nevala]

Created attachment 59086 [details]
Latest patch

Layout test run on Linux. Qt auto test run on 5.0 sdk emulator. Fix also testes on Nokia XpressMusic as described on bug report. No regression detected.

Linux:
- qt 4.6.2
- git://gitorious.org/webkit/webkit.git head=ef989587e6fc7dba53992b0fc2f45703333061a7

s60 emulator & hw:
- git://gitorious.org/+qt-s60-developers/qt/qt-s60.git head=442784de39c8128f418354fdcfdb3988bb599104
- git://gitorious.org/webkit/webkit.git head=9d9ada9106cb31e6ea00e7eacb9c49aa90652769

[Simon Hausmann]

Antti, could you help reviewing this patch?

The basic context is that for password fields on devices with virtual keyboards we want to display the last entered character temporarily before it turns into a * as password character.

Qt's input method - for password fields - will submit the last entered character as part of the composition and a second later it'll submit it as committed string. With this patch Samuel is trying to make sure that the password character that is part of the composition is shown in clear text.

I like this patch for being small and less intrusive, but I see that it is technically a layer violation. Do you think the violation is acceptible or is there maybe a better way of doing it?


Thanks :)

[Kenneth Rohde Christiansen]

(In reply to comment #34)
> Antti, could you help reviewing this patch?
> 
> The basic context is that for password fields on devices with virtual keyboards we want to display the last entered character temporarily before it turns into a * as password character.
> 
> Qt's input method - for password fields - will submit the last entered character as part of the composition and a second later it'll submit it as committed string. With this patch Samuel is trying to make sure that the password character that is part of the composition is shown in clear text.
> 
> I like this patch for being small and less intrusive, but I see that it is technically a layer violation. Do you think the violation is acceptible or is there maybe a better way of doing it?
> 
> 
> Thanks :)

We should really avoid adding mor layer violations. Anyway, this seems to be the same behaviour on Maemo/MeeGo, so I guess this bug is not really Symbian specific.

[Antti Koivisto]

I don't understand the symbian editing code (or editing in general) well enough to really give useful comments. However I find it surprising that the changelog talks about symbian only yet all the code changes are in the generic code? How is this going to affect other platforms? Is this what you mean by layer violation here?

[Simon Hausmann]

(In reply to comment #36)
> I don't understand the symbian editing code (or editing in general) well enough to really give useful comments. However I find it surprising that the changelog talks about symbian only yet all the code changes are in the generic code? How is this going to affect other platforms? Is this what you mean by layer violation here?

The bug itself is not Symbian specific, even though it was ported as such. Other platforms that implement virtual keyboards through input methods suffer from the same problem I believe.

[Samuel Nevala]

if (!document()->page()->focusController()->focusedOrMainFrame()->editor()->hasComposition())    m_text.makeSecure(secureChar);

can be replaced with 

if(!(node() && document()->frame()->editor()->compositionNode() == node()))                m_text.makeSecure(secureChar);

and #include "FocusController.h" can be removed. Layer violation is there already if it is about document()->frame()->editor(). To me it seem pretty minor.

[Chang Shu]

This "character echo" feature should only be valid for mobile devices and ITUT keyboard. For other cases that user knows exactly what he types, we should not expose the character due to security reasons. For example, on desktop, you don't want to echo password as some other guys may look over your shoulder. :)
So I think we should either have the code in platform-specific places or make it configurable and enable it only on certain scenarios.

[Kristian Amlie]

(In reply to comment #39)
> This "character echo" feature should only be valid for mobile devices and ITUT keyboard. For other cases that user knows exactly what he types, we should not expose the character due to security reasons. For example, on desktop, you don't want to echo password as some other guys may look over your shoulder. :)
> So I think we should either have the code in platform-specific places or make it configurable and enable it only on certain scenarios.

Why? I think it makes sense even on desktop to allow input method composed characters to be visible. I will admit I am not a user of any advanced input methods (like Chinese or similar), so I cannot speak on their behalf obviously, but I have the impression that fairly advanced compositions more or less require you to be able to see what you type.

And of course, typing in English or any other regular alphabets would not show the text.

[Chang Shu]

(In reply to comment #40)
> (In reply to comment #39)
> > This "character echo" feature should only be valid for mobile devices and ITUT keyboard. For other cases that user knows exactly what he types, we should not expose the character due to security reasons. For example, on desktop, you don't want to echo password as some other guys may look over your shoulder. :)
> > So I think we should either have the code in platform-specific places or make it configurable and enable it only on certain scenarios.
> 
> Why? I think it makes sense even on desktop to allow input method composed characters to be visible. I will admit I am not a user of any advanced input methods (like Chinese or similar), so I cannot speak on their behalf obviously, but I have the impression that fairly advanced compositions more or less require you to be able to see what you type.
> 
> And of course, typing in English or any other regular alphabets would not show the text.

I see your point of showing composed characters even on desktop. It's a trade off but to me, exposing password in public is much more serious than lacking character echo. I am not sure if we have a real case here, either. For example, in Chinese, passwords are still in English alphabets.

In addition, is it so that the patch submitted only deals with composed characters?

[Kristian Amlie]

(In reply to comment #41)
> I see your point of showing composed characters even on desktop. It's a trade off but to me, exposing password in public is much more serious than lacking character echo. I am not sure if we have a real case here, either. For example, in Chinese, passwords are still in English alphabets.

In that case they will probably never see the effects of this patch.

> In addition, is it so that the patch submitted only deals with composed characters?

Yes.

[Janne Koskinen]

Bump. You can reproduce this issue on N8 using portrait VKB.

[Prasad]

Related QtWRT Bugzilla error ID: http://bugs.nokia-boston.com/bugzilla/show_bug.cgi?id=7298

[Antonio Gomes]

I do not see how this bug is Qt-only (see [Qt] prefix in bug title). It only touches cross platform code.

[Chang Shu]

I believe it's just historical reason that we set the [qt] flag.

[Ryosuke Niwa]

Comment on attachment 59086 [details]
Latest patch

View in context: https://bugs.webkit.org/attachment.cgi?id=59086&action=review

> LayoutTests/editing/input/secure-text.html:56
> +        eventSender.keyDown("rightArrow");
> +    }

You should probably log("DONE") at the end.

> WebCore/editing/Editor.cpp:1487
> +           RenderText* textRenderer = static_cast<RenderText*>(baseNode->renderer());
> +           if (textRenderer && textRenderer->style() && textRenderer->style()->textSecurity() != TSNONE) {
> +               String originalText = textRenderer->text();
> +               originalText.replace(baseOffset, text.length(), text);
> +               textRenderer->setText(originalText.impl());
> +           }

Why aren't you fixing TypingCommand::insertText instead (e.g. add an argument)?  It doesn't make much sense to me that you're inserting text, and then replacing it.  This code may affect selection; e.g. clears selection.

> WebCore/rendering/RenderText.cpp:1098
> +            if (!document()->page()->focusController()->focusedOrMainFrame()->editor()->hasComposition())
> +                m_text.makeSecure(secureChar);

How do you know that this text node is the one focused?  Or even that m_text is on the focused frame? r- because of this condition.

[Alexey Proskuryakov]

+        [Qt] 3rd edition phones lack character echo in web input fields

Is this bug about password inputs, or more general?

Displaying the latest character typed into a password field sounds like something that already works on iOS. Did you check how that's implemented? Perhaps the code is already in open source WebCore, and no changes are necessary?

+    function seucreText(textLength)

Typo: seucre.

+            if (style()->textSecurity() == TSCIRCLE)
+                secureChar = whiteBullet;
+            else if (style()->textSecurity() == TSDISC)
+                secureChar = bullet;
+            else if (style()->textSecurity() == TSSQUARE)
+                secureChar = blackSquare;

Replacing a switch with an if/else sequence is not great, because that way, the compiler won't complain if more options are added to TextSecurity enum, and we forget to update this code.

[Janne Koskinen]

(In reply to comment #48)
> Is this bug about password inputs, or more general?

It is about password input echoing with multi-state-key keyboard. Issue exists also if language which combines characters to make syllables or words is being entered into password field.

> Displaying the latest character typed into a password field sounds like something that already works on iOS. Did you check how that's implemented? 

AFAIK iOS devices don't have ITU-T keyboard so unlikely this has become an issue there.

There is workarounds for this bug all over platforms, least the trackers that I can access.

Anyways, Samuel has not worked on webkit for 6 months and thus you most likely won't get reply from him unless he feels generous. So, if someone wants this to get fixed better pick it up and continue.

[Alexey Proskuryakov]

I don't even know what an ITU-T keyboard is, but iOS just always displays the last character in the password field for a second or two. This sounds like what this patch tries to make possible, too.

[Kristian Amlie]

> I don't even know what an ITU-T keyboard is, but iOS just always displays the last character in the password field for a second or two. This sounds like what this patch tries to make possible, too.

How does it display the last character? Is it just a property of the editor, or does the platform code have to send any specific events to show and hide the character?

From the Qt side it does not matter whether it's an ITU-T keyboard or not. All keyboards, when used in password fields, will send characters of the "preedit" type, which means they should be displayed, even in password fields. Then after a second, it erases the character, and sends a new "committed" character.

[Janne Koskinen]

(In reply to comment #50)
> I don't even know what an ITU-T keyboard is, but iOS just always displays the last character in the password field for a second or two. This sounds like what this patch tries to make possible, too.

Right, that is exactly what this patch is trying to make.

ITU-T E.161 http://en.wikipedia.org/wiki/E.161 ; commonly referred as ITU-T keyboard.

The original patch was based on S60 browser changes also posted on this bug report #c7 and from there it started to live as now Qt does the correct thing on passing input events to webkit.

[Chang Shu]

> Anyways, Samuel has not worked on webkit for 6 months and thus you most likely won't get reply from him unless he feels generous. So, if someone wants this to get fixed better pick it up and continue.

I talked to Samuel through email before and I will pick up the rest of the work unless he's available now.

[David Kilzer (:ddkilzer)]

(In reply to comment #51)
> > I don't even know what an ITU-T keyboard is, but iOS just always displays the last character in the password field for a second or two. This sounds like what this patch tries to make possible, too.
> 
> How does it display the last character? Is it just a property of the editor, or does the platform code have to send any specific events to show and hide the character?

The iOS port of WebKit uses a timer to determine when to obscure the last character in a text field.  The code is mostly self-contained within WebCore/rendering/RenderText.cpp (as I recall).

[Chang Shu]

> 
> The iOS port of WebKit uses a timer to determine when to obscure the last character in a text field.  The code is mostly self-contained within WebCore/rendering/RenderText.cpp (as I recall).

Right, I just checked the iOS code and want to confirm this. Function CharacterData::insertData() calls RenderText::momentarilyRevealLastCharacter(), which starts a timer to do the text replacement.

Personally, I prefer Samuel's approach. I will work on the next patch based on Niwa's review.

[Chang Shu]

I forgot to ask: does Samuel's patch work for all platforms? If it's a Qt-only solution, we'd better go for the iOS timer approach.

[Kristian Amlie]

It will probably not work for all platforms, because I don't expect that all of them use composition text in their password fields to temporarily display the character. Instead, I would assume they just send normal text, and it is up to RenderText.cpp to display the temporary character.

If you do use the iOS approach, keep in mind that the first version of the character that arrives from Qt will be composition (preedit) text, and after a second or so it is replaced by real text, so in that case you will have two timers running.

[Prasad]

Nokia Internal Bugzilla error ID: 
http://bugs.nokia-boston.com/bugzilla/show_bug.cgi?id=7298

[Alexey Proskuryakov]

> Personally, I prefer Samuel's approach.

Are you suggesting that we add new code to WebCore for another way to do something that's already supported by it?

[Chang Shu]

(In reply to comment #59)
> > Personally, I prefer Samuel's approach.
> 
> Are you suggesting that we add new code to WebCore for another way to do something that's already supported by it?

It seems Samuel's patch won't work across all platforms so I will proceed with iOS approach.

[Kenneth Rohde Christiansen]

Please do this in a way that it will work with Qt/WebKit2. Our development branch in gitorious has support for input methods.

[Janne Koskinen]

(In reply to comment #60)
> (In reply to comment #59)
> > > Personally, I prefer Samuel's approach.
> > 
> > Are you suggesting that we add new code to WebCore for another way to do something that's already supported by it?
> 
> It seems Samuel's patch won't work across all platforms so I will proceed with iOS approach.

If you look at the patches there is the iOS approach https://bugs.webkit.org/attachment.cgi?id=46125 .

[Chang Shu]

> 
> If you look at the patches there is the iOS approach https://bugs.webkit.org/attachment.cgi?id=46125 .

Thanks, Janne. I found the iOS source code from the web. The implementation is slightly different. I will provide a new patch soon.

[Chang Shu]

I am working on the test case right now. However, I couldn't figure out how composition mode was turned on in the test case in Samuel's patch. It looks to me the password echo is always on in iOS source code. It may not be an issue in iOS but we need a flag for webkit trunk. Using editor()->hasComposition() seems to be a good choice but I don't know how we can enable it through layout tests. I am not able to do it manually on any desktop browsers either because IME is not allowed for password field. The only place to make it work is a phone device with ITU-T keyboard. Another option is to use a flag in page/settings.h. Any inputs, everybody? Thanks!

[Chang Shu]

Created attachment 79219 [details]
fix patch

[Antonio Gomes]

Comment on attachment 79219 [details]
fix patch

This goes against what was discussed here: http://old.nabble.com/Bools-are-strictly-worse-than-enums-td30371586.html

[Ryosuke Niwa]

Comment on attachment 79219 [details]
fix patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79219&action=review

Although I'm not qualified to r+ a patch for this bug, I'll still give you my feedback.

> Source/WebCore/dom/CharacterData.h:37
> -    void insertData(unsigned offset, const String&, ExceptionCode&);
> +    void insertData(unsigned offset, const String&, ExceptionCode&, bool canShowLastCharacterIfSecure = false);

Nit: I'm not sure if canShowLastCharacterIfSecure is the right name. We certainly show last character when it's not secure.  Maybe canShowSecureLastCharacter?

> Source/WebCore/rendering/RenderText.cpp:1544
> +    Timer<RenderText>* secureLastCharacterTimer;
> +    if (m_hasSecureLastCharacterTimer) {
> +        secureLastCharacterTimer = gSecureLastCharacterTimers->get(this);
> +        secureLastCharacterTimer->stop();
> +    } else {
> +        secureLastCharacterTimer = new Timer<RenderText>(this, &RenderText::secureLastCharacter);
> +        gSecureLastCharacterTimers->add(this, secureLastCharacterTimer);
> +        m_hasSecureLastCharacterTimer = true;
> +    }

Mn... what happens if Timer fires after RenderText is destroyed?  Or is there an guarantee that it won't happen?

> LayoutTests/editing/input/secure-text.html:65
> +    if (testIdx >= 0) {
> +        expectedSecureTextLen = tests[testIdx][2];
> +        textInputController.doCommand("moveForward:");
> +        assert(tests[testIdx][4], window.find(secureText(expectedSecureTextLen), false, true), "secured after delay.");
> +    }

It'll be nice if you could put this verification in a closure or something so that the verification code appears after the test code.  You can have two functions test & verify and make them mutually "recursive" via setTimeouts.

> LayoutTests/editing/input/secure-text.html:78
> +    for(var i = 0; i < charSequence.length - 1; i++) {

Nit: space between for and (

> LayoutTests/editing/input/secure-text.html:87
> +    window.setTimeout(run, 1200);

1200 seems like an awfully long time.  Can we shorten it to something like 500?  Or will that break the test / make the test flaky?

> LayoutTests/editing/input/secure-text.html:96
> +    if (window.layoutTestController) {
> +        layoutTestController.dumpAsText();
> +        layoutTestController.waitUntilDone();
> +        run();
> +    }

Note that textInputController isn't implemented on all platforms so you might want to check window.textInputController and print out appropriate message here.

[Kenneth Rohde Christiansen]

Comment on attachment 79219 [details]
fix patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79219&action=review

> Source/WebCore/dom/CharacterData.h:37
> +    void insertData(unsigned offset, const String&, ExceptionCode&, bool canShowLastCharacterIfSecure = false);

Ah I see you use MomentarilyRevealLastCharacter above.

Maybe, enum SecureMode { AlwaysHideCharacters, MomentarilyRevealLastCharacter }

> Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:53
> +    m_node->insertData(m_offset, m_text, ec, canShowLastCharacterIfSecure);

Is it really "canShow...", it feels more like a "doShow". Maybe I would make make it clear that it is temporarily. ie showLastCharTemporarilyIfSecure.

> Source/WebCore/rendering/RenderText.h:122
> +    void secureLastCharacter(Timer<RenderText> * aTimer);

There should be no space before *, also you can remove aTimer.

[Chang Shu]

Thanks for the reviews and comments. Please see my comments below.

> Source/WebCore/dom/CharacterData.h:37
> +    void insertData(unsigned offset, const String&, ExceptionCode&, bool canShowLastCharacterIfSecure = false);

From Niwa: Nit: I'm not sure if canShowLastCharacterIfSecure is the right name. We certainly show last character when it's not secure.  Maybe canShowSecureLastCharacter?

From Kenneth: Ah I see you use MomentarilyRevealLastCharacter above.

Maybe, enum SecureMode { AlwaysHideCharacters, MomentarilyRevealLastCharacter }

Chang: I will use enum then.

> Source/WebCore/rendering/RenderText.cpp:1544
> +        secureLastCharacterTimer = new Timer<RenderText>(this, &RenderText::secureLastCharacter);
> +        gSecureLastCharacterTimers->add(this, secureLastCharacterTimer);

Mn... what happens if Timer fires after RenderText is destroyed?  Or is there an guarantee that it won't happen?

Chang: Good question. I will stop timer in the RenderText destructor.

> LayoutTests/editing/input/secure-text.html:65
> +    if (testIdx >= 0) {
> +        expectedSecureTextLen = tests[testIdx][2];
> +        textInputController.doCommand("moveForward:");
> +        assert(tests[testIdx][4], window.find(secureText(expectedSecureTextLen), false, true), "secured after delay.");
> +    }

It'll be nice if you could put this verification in a closure or something so that the verification code appears after the test code.  You can have two functions test & verify and make them mutually "recursive" via setTimeouts.

Chang: Can you show me details? I have to do two verifications, one before timeout and one after timeout.

> LayoutTests/editing/input/secure-text.html:78
> +    for(var i = 0; i < charSequence.length - 1; i++) {

Nit: space between for and (

Chang: Sure.

> LayoutTests/editing/input/secure-text.html:87
> +    window.setTimeout(run, 1200);

1200 seems like an awfully long time.  Can we shorten it to something like 500?  Or will that break the test / make the test flaky?

Chang: The reveal password time period is hardcoded to 1 sec (it is 2 seconds in iOs code!). So 1200ms is almost the minimum. I am thinking add a setting for this so in the layout test, we can set it to a much smaller value. Besides, different platforms may want different values based on their user experience. But I will do this in a separate patch.

> LayoutTests/editing/input/secure-text.html:96
> +    if (window.layoutTestController) {
> +        layoutTestController.dumpAsText();
> +        layoutTestController.waitUntilDone();
> +        run();
> +    }

Note that textInputController isn't implemented on all platforms so you might want to check window.textInputController and print out appropriate message here.

Chang: Sure. I will add the sanity check.

------- Comment #68 From Kenneth Rohde Christiansen 2011-01-17 15:41:04 PST (-) [reply] -------

(From update of attachment 79219 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=79219&action=review

> Source/WebCore/rendering/RenderText.h:122
> +    void secureLastCharacter(Timer<RenderText> * aTimer);

There should be no space before *, also you can remove aTimer.

Chang: will do.

[Ryosuke Niwa]

(In reply to comment #69)
> > LayoutTests/editing/input/secure-text.html:87
> > +    window.setTimeout(run, 1200);
> 
> 1200 seems like an awfully long time.  Can we shorten it to something like 500?  Or will that break the test / make the test flaky?
> 
> Chang: The reveal password time period is hardcoded to 1 sec (it is 2 seconds in iOs code!). So 1200ms is almost the minimum. I am thinking add a setting for this so in the layout test, we can set it to a much smaller value. Besides, different platforms may want different values based on their user experience. But I will do this in a separate patch.

Yes, we definitely want to be able to adjust the time interval.  I'd imagine some users (e.g. people with eyesight problems) want to adjust it to be a longer period of time.

[Chang Shu]

Created attachment 79460 [details]
fix patch 2

2nd patch after 1st round review.

[Mike Fenton]

This is a really useful patch for virtual keyboards.

Would it be possible to make it more configurable and driven by the editor by adding an editor function delayBeforeSecureLastCharacter() which returns the length of time to unmask the character for?  A value of 0 could then be used to immediately mask the character.

This would allow the timing to be tuned based on individual fields as well as the state of the device.  Devices that have multiple input methods may only want to unmask for the virtual keyboard, not in all cases.

[Chang Shu]

(In reply to comment #72)
> This is a really useful patch for virtual keyboards.
> 
> Would it be possible to make it more configurable and driven by the editor by adding an editor function delayBeforeSecureLastCharacter() which returns the length of time to unmask the character for?  A value of 0 could then be used to immediately mask the character.
> 
> This would allow the timing to be tuned based on individual fields as well as the state of the device.  Devices that have multiple input methods may only want to unmask for the virtual keyboard, not in all cases.

Yes, yes, I definitely want to do that (see previous comments above). I plan to add this setting in a separate patch so not to make this one too complicated.

[Chang Shu]

ping reviewer. thanks!

[Eric Seidel (no email)]

Comment on attachment 79460 [details]
fix patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=79460&action=review

Are we sure adding a destructor to RenderText isn't a perf hit?  I'm not sure if it is or not.

> Source/JavaScriptCore/wtf/text/WTFString.h:216
> +    void makeSecure(UChar aChar, bool shouldSecureLastCharacter)

Why shouldn't this take an enum like the StringImpl one does?  We could even move the enum onto WTFString if that's needed.

> Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:52
> +    CharacterData::SecureMode secureMode = (m_node->document()->frame() && m_node->document()->frame()->editor()->ignoreCompositionSelectionChange()) ? CharacterData::MomentarilyRevealLastCharacter : CharacterData::AlwaysHideCharacters;

Seems this needs a helper function.

[Chang Shu]

Created attachment 80492 [details]
fix patch 3

Updated patch based on Eric's comments.

[Kenneth Rohde Christiansen]

Comment on attachment 80492 [details]
fix patch 3

View in context: https://bugs.webkit.org/attachment.cgi?id=80492&action=review

> Source/WebCore/rendering/RenderText.cpp:56
> +static const float revealLastCharacterDurationInSeconds = 1.0f;

I believe that our style dictates to not use 1.0f but just 1. Could you check?

[Chang Shu]

> > +static const float revealLastCharacterDurationInSeconds = 1.0f;
> 
> I believe that our style dictates to not use 1.0f but just 1. Could you check?

You mean the webkit style check? The style bubble above is green.

[Janne Koskinen]

(In reply to comment #78)
> > > +static const float revealLastCharacterDurationInSeconds = 1.0f;
> > 
> > I believe that our style dictates to not use 1.0f but just 1. Could you check?
> 
> You mean the webkit style check? The style bubble above is green.

I don't think it is in check-webkit style, but indeed it is good style.
This proactively prevents erroneous statement like
>static const double foo = 1.0f; // oops
if precision is changed later on.

[Chang Shu]

> I don't think it is in check-webkit style, but indeed it is good style.
> This proactively prevents erroneous statement like
> >static const double foo = 1.0f; // oops
> if precision is changed later on.

Sure. I will make the change along with other changes if necessary (ping reviewers) or do it in the patch that supports configurable echo timeout.

[Suresh Voruganti]

Adding to Qtwebkit 2.1 Master bug

[Andreas Kling]

@Ryosuke: Could you have a look at this? It feels like something bordering on your alley :)

[Ryosuke Niwa]

(In reply to comment #82)
> @Ryosuke: Could you have a look at this? It feels like something bordering on your alley :)

I wish I could but I can't review RenderText change because I'm not familiar with the rendering engine.  I'd imagine eseidel, darin, or mitz would be able to review this patch.

[Ademar Reis]

Patch 3 added to qtwebkit-2.1 branch as 45527cd4cb9e2d7c00da2c72ae9f04172e36a4b3. This bug is now part of the "pending trunk inclusion" set.

[Hajime Morrita]

Comment on attachment 80492 [details]
fix patch 3

View in context: https://bugs.webkit.org/attachment.cgi?id=80492&action=review

> Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:35
> +static CharacterData::SecureMode getTextNodeSecureMode(PassRefPtr<Text> node)

This logic could be a part of CharacterData class. How about to move this to CharacterData and add CharacterData:insertSecurableData() or something?

> Source/WebCore/rendering/RenderText.h:188
>  };

Is it possible to push down these to some new subclass like RenderSecureText?
Almost all RenderText objects are never edited and having special securely-editable class might be good separation.
I'm not sure if this is really good idea though. This is just an idea and you can freely ignore...

[Alexey Proskuryakov]

I think that "secure mode" may not be a good name, as it collides with a different "secure input mode" that still lives in WebCore/SecureTextInput.h for Chromium (but should be in WebKit/chromium).

Something more straightforward like "drawsWithBullets" could work, except that the character is configurable.

> +static CharacterData::SecureMode getTextNodeSecureMode(PassRefPtr<Text> node)

I didn't read the patch itself, but this caught my eye as a coding style violation. Functions that return a value directly don't have a "get" prefix in WebKit.

[Alexey Proskuryakov]

Also, with over 80 comments in the bug, I can't remember why we decided that this needs WTF and WebCore changes, and can't be done by the WebKit client.

The existing WTF::String::makeSecure() feels like an incredibly ugly layering violation to me. Perhaps the best approach would be to clean up the code first.

[Chang Shu]

(In reply to comment #87)
> Also, with over 80 comments in the bug, I can't remember why we decided that this needs WTF and WebCore changes, and can't be done by the WebKit client.

Even I need refresh my memory. I think the changes over there is just to reuse the String::makeSecure code.
> 
> The existing WTF::String::makeSecure() feels like an incredibly ugly layering violation to me. Perhaps the best approach would be to clean up the code first.

Right. We should do this first. Over all, I feel the implementation has some flaws. For example, the echoing only happens on the last char. If you insert/delete chars in the middle, no echoing happens.

Good that this patch is drawing some attention again. I will find time work on it.

[Eric Seidel (no email)]

Comment on attachment 80492 [details]
fix patch 3

The last comment was over a month ago.  Looks like Chang plans to revise this anyway.  Marking r-.

[Chang Shu]

(In reply to comment #89)
> (From update of attachment 80492 [details])
> The last comment was over a month ago.  Looks like Chang plans to revise this anyway.  Marking r-.

Yes. I will revisit this bug when I have time. For now, the patch should be out-of-date anyway. Thanks.

[Chang Shu]

Created attachment 96239 [details]
fix patch 3: rebaseline + minor changes

[Chang Shu]

(In reply to comment #91)
> Created an attachment (id=96239) [details]
> fix patch 3: rebaseline + minor changes

This patch is NOT for review but for cherry-picking.

[Antonio Gomes]

(In reply to comment #91)
> Created an attachment (id=96239) [details]
> fix patch 3: rebaseline + minor changes

Plans to put it up for revieW?

[Chang Shu]

Comment on attachment 96239 [details]
fix patch 3: rebaseline + minor changes

>> fix patch 3: rebaseline + minor changes
> Plans to put it up for revieW?

Ok, why not give it a shot? :)
In the new patch, I removed the change in JavascriptCore so it doesn't have to depend on the cleanup of String::makeSecure.
In terms of WebKit vs. WebCore approach, I think the latter provides a cross-platform solution which will benefit all ports.

[Antonio Gomes]

Comment on attachment 96239 [details]
fix patch 3: rebaseline + minor changes

View in context: https://bugs.webkit.org/attachment.cgi?id=96239&action=review

> Source/WebCore/rendering/RenderText.h:121
> +    void momentarilyRevealLastCharacter();

Is this for the last char in the text OR last typed char?

Like if you write some text, then move cursor to a different position other than the last position, and type some thing, does it work? If so, it sounds like the method name could be better?

[Chang Shu]

(In reply to comment #95)
> (From update of attachment 96239 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96239&action=review
> 
> > Source/WebCore/rendering/RenderText.h:121
> > +    void momentarilyRevealLastCharacter();
> 
> Is this for the last char in the text OR last typed char?
> 
> Like if you write some text, then move cursor to a different position other than the last position, and type some thing, does it work? If so, it sounds like the method name could be better?

No, it won't work. What revealed is the last char in the text. I understand it's less desired behavior. At the time, I just tried to match the iOS behavior. It would be good to fix it with probably some big changes to the current implementation.

[Mike Fenton]

(In reply to comment #96)
> > Like if you write some text, then move cursor to a different position other than the last position, and type some thing, does it work? If so, it sounds like the method name could be better?
> 
> No, it won't work. What revealed is the last char in the text. I understand it's less desired behavior. At the time, I just tried to match the iOS behavior. It would be good to fix it with probably some big changes to the current implementation.

If the input point isn't at the end of the text, should it reveal any characters at all then?

I'm also all for enhancing it to be the last character typed in the future.

[Chang Shu]

(In reply to comment #97)
> (In reply to comment #96)
> > > Like if you write some text, then move cursor to a different position other than the last position, and type some thing, does it work? If so, it sounds like the method name could be better?
> > 
> > No, it won't work. What revealed is the last char in the text. I understand it's less desired behavior. At the time, I just tried to match the iOS behavior. It would be good to fix it with probably some big changes to the current implementation.
> 
> If the input point isn't at the end of the text, should it reveal any characters at all then?
> 
> I'm also all for enhancing it to be the last character typed in the future.

I prefer revealing the last typed char because the purpose of it is to give the user a confirmation(echo) of what just typed. But it's a rare case anyway.

[Kenneth Rohde Christiansen]

pressing backspace and then revealing the last typed char, can be a security issue

[Mike Fenton]

(In reply to comment #98)
> > If the input point isn't at the end of the text, should it reveal any characters at all then?
> > 
> > I'm also all for enhancing it to be the last character typed in the future.
> 
> I prefer revealing the last typed char because the purpose of it is to give the user a confirmation(echo) of what just typed. But it's a rare case anyway.

Yes, that's exactly what I was suggesting.  The last typed character should be revealed regardless of it's position in the text.

[Chang Shu]

Created attachment 99840 [details]
patch 4

This patch echoes the last typed char and also does some code refactory.

[WebKit Review Bot]

Comment on attachment 99840 [details]
patch 4

Attachment 99840 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8988789

New failing tests:
editing/input/secure-text.html

[WebKit Review Bot]

Created attachment 99850 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Chang Shu]

(In reply to comment #102)
> (From update of attachment 99840 [details])
> Attachment 99840 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/8988789
> 
> New failing tests:
> editing/input/secure-text.html

The failure is due to a short timeout tolerance on chromium. I will add a setting to adjust password echo delay in the patch.

[Chang Shu]

Created attachment 100021 [details]
patch 5

split tests to cut down the running time. The current tests are slow because the echo duration is hard-coded. I will work on a follow-up patch to make it configurable.

[Alexey Proskuryakov]

Comment on attachment 100021 [details]
patch 5

View in context: https://bugs.webkit.org/attachment.cgi?id=100021&action=review

> Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:36
> +static CharacterData::SecureMode getTextNodeSecureMode(PassRefPtr<Text> node)

The argument should be a plain pointer, there is no ownership passing.

> Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:38
> +    return (node->document()->frame() && node->document()->frame()->editor()->ignoreCompositionSelectionChange()) ? CharacterData::MomentarilyRevealLastTypedCharacter : CharacterData::AlwaysHideCharacters;

I don't understand why ignoreCompositionSelectionChange() is a indicator of whether the last character should be revealed.

> Source/WebCore/rendering/RenderText.cpp:55
> +static const float revealLastTypedCharacterDurationInSeconds = 0.5f;

startOneShot() takes a double argument - why is this a float?

Also, "static" doesn't mean anything with "const" in C++.

> Source/WebCore/rendering/RenderText.cpp:207
> +            secureLastTypedCharacterTimer->stop();
> +            delete secureLastTypedCharacterTimer;

No need to stop a timer before deleting it.

> Source/WebCore/rendering/RenderText.h:178
> +    unsigned m_lastTypedCharacterOffsetToReveal;

I don't know if it's OK to add an unsigned and a bool (bitfield) to all RenderTexts. I suspect that it can be bad for memory use.

[Chang Shu]

> > Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:38
> > +    return (node->document()->frame() && node->document()->frame()->editor()->ignoreCompositionSelectionChange()) ? CharacterData::MomentarilyRevealLastTypedCharacter : CharacterData::AlwaysHideCharacters;
> 
> I don't understand why ignoreCompositionSelectionChange() is a indicator of whether the last character should be revealed.

It seems we can only figure out whether the current input is composition type through this function. 
I also plan to add a flag in page settings to enable password echo even for qwerty keyboard. E.g. iOS echoes password even for full keyboard.

> > Source/WebCore/rendering/RenderText.h:178
> > +    unsigned m_lastTypedCharacterOffsetToReveal;
> 
> I don't know if it's OK to add an unsigned and a bool (bitfield) to all RenderTexts. I suspect that it can be bad for memory use.

I can probably pass the offset to the timer. but that also involves more code in setText and setTextInternal. I will give it a try.

Thanks for the review.

[Alexey Proskuryakov]

> It seems we can only figure out whether the current input is composition type through this function. 
> I also plan to add a flag in page settings to enable password echo even for qwerty keyboard. E.g. iOS echoes password even for full keyboard.

Right - what's surprising to me is that password input is somehow related to composition. Now that I look at this more, I also suspect that this code will trigger the timer when typing Japanese or Chinese.

[Chang Shu]

(In reply to comment #108)
> > It seems we can only figure out whether the current input is composition type through this function. 
> > I also plan to add a flag in page settings to enable password echo even for qwerty keyboard. E.g. iOS echoes password even for full keyboard.
> 
> Right - what's surprising to me is that password input is somehow related to composition. Now that I look at this more, I also suspect that this code will trigger the timer when typing Japanese or Chinese.

It should not. Inside function CharacterData::insertData, it also checks if the text isSecure. I think Japanese or Chinese characters can never be password.

[Chang Shu]

Created attachment 100346 [details]
patch 6

[Antonio Gomes]

Comment on attachment 100346 [details]
patch 6

View in context: https://bugs.webkit.org/attachment.cgi?id=100346&action=review

I think we are very close. Some minor comments and nits.

> Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:36
> +static CharacterData::SecureMode getTextNodeSecureMode(Text* node)

name it secureModeForTextNode maybe.

> Source/WebCore/rendering/RenderText.cpp:72
> +        m_renderText->setText(m_renderText->text(), true);

what is true here? maybe add a /*XXX*/ comment?

> Source/WebCore/rendering/RenderText.cpp:1670
> +    if (!gSecureTextTimers)
> +        gSecureTextTimers = new SecureTextTimerMap;

Is it ok to leak this guy here?

> Source/WebCore/rendering/RenderText.h:118
> +    bool isSecure() { return style()->textSecurity() != TSNONE; }

const method?

[Chang Shu]

> > Source/WebCore/rendering/RenderText.cpp:1670
> > +    if (!gSecureTextTimers)
> > +        gSecureTextTimers = new SecureTextTimerMap;
> 
> Is it ok to leak this guy here?

I think some existing code also leaks the global variable. We can do better to clean it up while the size of the map decreases to 0. But we may end up creating and deleting it too often.

[Balazs Kelemen]

(In reply to comment #112)
> > > Source/WebCore/rendering/RenderText.cpp:1670
> > > +    if (!gSecureTextTimers)
> > > +        gSecureTextTimers = new SecureTextTimerMap;
> > 
> > Is it ok to leak this guy here?
> 
> I think some existing code also leaks the global variable. We can do better to clean it up while the size of the map decreases to 0. But we may end up creating and deleting it too often.

It is a policy in WebKit to use heap for globals and never dealloc them.  This makes us shutting down faster ;) The preferred way is to define the variable with the DEFINE_STATIC_LOCAL macro at function scope.

[Chang Shu]

> > I think some existing code also leaks the global variable. We can do better to clean it up while the size of the map decreases to 0. But we may end up creating and deleting it too often.
> 
> It is a policy in WebKit to use heap for globals and never dealloc them.  This makes us shutting down faster ;) The preferred way is to define the variable with the DEFINE_STATIC_LOCAL macro at function scope.

Thanks for the clarification!

[Chang Shu]

Created attachment 100363 [details]
patch 7: follow up Antonio's comments

[Alexey Proskuryakov]

Comment on attachment 100363 [details]
patch 7: follow up Antonio's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=100363&action=review

> Source/WebCore/dom/CharacterData.cpp:112
> +        RenderText* renderText = toRenderText(renderer());
> +        if (renderText->isSecure())

What guarantees that it's a RenderText?

> Source/WebCore/rendering/RenderText.cpp:63
> +    SecureTextTimer(RenderText* renderText)
> +    : m_renderText(renderText), m_lastTypedCharacterOffset(0) { }

Wrong formatting - if it's more than one line, it should be formatted like regular code (each brach on its own line, 4 spaces more for initializer list).

> Source/WebCore/rendering/RenderText.cpp:75
> +        ASSERT(gSecureTextTimers->get(m_renderText));

IIRC there is a contains() function for this.

[Chang Shu]

> > Source/WebCore/dom/CharacterData.cpp:112
> > +        RenderText* renderText = toRenderText(renderer());
> > +        if (renderText->isSecure())
> 
> What guarantees that it's a RenderText?

I think it's safe with the current code. The only two classes derive from CharacterData are Comment and Text and only Text has renderer. There is also existing code in CharacterData.cpp that casts renderer() to RenderText in updateRenderer().

> > Source/WebCore/rendering/RenderText.cpp:75
> > +        ASSERT(gSecureTextTimers->get(m_renderText));
> 
> IIRC there is a contains() function for this.

The input to contains() is the value. m_renderText is the key. But you are right that I probably don't need to save m_renderText since I can use find() and contains(). I had concern on the performance but the map should be really small.

[Chang Shu]

> > > Source/WebCore/rendering/RenderText.cpp:75
> > > +        ASSERT(gSecureTextTimers->get(m_renderText));
> > 
> > IIRC there is a contains() function for this.
> 
> The input to contains() is the value. m_renderText is the key. But you are right that I probably don't need to save m_renderText since I can use find() and contains(). I had concern on the performance but the map should be really small.

Sorry, the above comments were wrong. I was looking at the Vector api. :(

[Chang Shu]

Created attachment 100530 [details]
patch 8: addresses ap's review, etc.

[Alexey Proskuryakov]

Comment on attachment 100530 [details]
patch 8: addresses ap's review, etc.

View in context: https://bugs.webkit.org/attachment.cgi?id=100530&action=review

> Source/WebCore/dom/CharacterData.cpp:114
> +    if ((secureMode == MomentarilyRevealLastTypedCharacter) && renderer()) {
> +        RenderText* renderText = toRenderText(renderer());
> +        if (renderText->isSecure())
> +            renderText->momentarilyRevealLastTypedCharacter(offset + data.length() - 1);
> +    }

I think that this would read better as:

 110    if (secureMode == MomentarilyRevealLastTypedCharacter) {
 111        RenderText* renderText = toRenderText(renderer());
 112        if (renderText && renderText->isSecure())
 113            renderText->momentarilyRevealLastTypedCharacter(offset + data.length() - 1);
 114    }

> Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:38
> +    return (node->document()->frame() && node->document()->frame()->editor()->ignoreCompositionSelectionChange()) ? CharacterData::MomentarilyRevealLastTypedCharacter : CharacterData::AlwaysHideCharacters;

I still don't understand how password input is related to composition. Seems like we should have a client call (or a preference) that specifies this policy directly.

> Source/WebCore/rendering/RenderText.cpp:65
> +    { }

These should be on separate lines.

[Chang Shu]

> > Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:38
> > +    return (node->document()->frame() && node->document()->frame()->editor()->ignoreCompositionSelectionChange()) ? CharacterData::MomentarilyRevealLastTypedCharacter : CharacterData::AlwaysHideCharacters;
> 
> I still don't understand how password input is related to composition. Seems like we should have a client call (or a preference) that specifies this policy directly.
> 
Thanks for the fine tune. For the question above, I think the use case is that we'd like to reveal the last-typed character only when all three conditions are true:
1. it's a password field
2. the input method is composition type.
3. it's entering new character, not deleting.

The reason behind this is only when we entering password using composition input method, we need some degree of conformance of what we have typed. It's not necessary when we type password using full keyboard.

Note that when 1 is true and 2 is false, we mask the whole password. And also note that the current iOS behavior is different: it always reveal last-typed character without considering condition 2. We cannot do the same as iOS because the same code is used for desktop and exposing the last character is not preferred. I plan to add the page setting to allow user enable password echo even the input method is not composition. But it will be in a separate patch.

But I do agree that making the setting work without considering composition seems simplify the case. Then it seems we should land the setting patch first before landing this patch. Otherwise, we will enable password echo even for desktop browser.

[Alexey Proskuryakov]

Comment on attachment 100530 [details]
patch 8: addresses ap's review, etc.

So, for each port it would be:
- iOS doesn't need to consider composition status;
- Mobile Qt does need that;
- Every other port never reveals the last character.

As you are saying that making that more explicit will simplify the logic, let's do that.

[Antonio Gomes]

(In reply to comment #122)
> (From update of attachment 100530 [details])
> So, for each port it would be:
> - iOS doesn't need to consider composition status;

Does iOS momentarily reveal the last character typed even with a physical keyboard (paired over bluetooth)?

[Kenneth Rohde Christiansen]

> Thanks for the fine tune. For the question above, I think the use case is that we'd like to reveal the last-typed character only when all three conditions are true:
> 1. it's a password field
> 2. the input method is composition type.
> 3. it's entering new character, not deleting.

I am not sure that I will agree to number 2. On the N950 for instance (or via bluotooth keyboard on the N9 when that is supported) you can very well input passwords without having compositing. I am not convinces that that should have a different behavior than when inputting via the virtual keyboard.

[Chang Shu]

Thanks for the review and comments, guys. I finally get access to internet as I have been travelling these days in my vacation. I will follow ap's suggestion using an option to enable/disable password echo and this should resolve some conflicts and confusion from different needs and platforms.

[Chang Shu]

Created attachment 104347 [details]
patch 9

[Alexey Proskuryakov]

Comment on attachment 104347 [details]
patch 9

Looks good to me. But as discussed on IRC, the code to reveal the last typed character should be in InsertIntoTextNodeCommand::doApply or even higher up the stack, not in CharacterData::insertData.

[Alexey Proskuryakov]

Comment on attachment 104347 [details]
patch 9

View in context: https://bugs.webkit.org/attachment.cgi?id=104347&action=review

> Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:58
> +    m_node->insertData(m_offset, m_text, ec, secureModeForTextNode(m_node.get()));

Also, it's not great to have ExceptionCode not be the last argument.

> Source/WebCore/rendering/RenderText.cpp:65
> +    { }

These should be on separate lines.

[Chang Shu]

Created attachment 104387 [details]
patch 10

[Alexey Proskuryakov]

Comment on attachment 104387 [details]
patch 10

View in context: https://bugs.webkit.org/attachment.cgi?id=104387&action=review

> Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:53
> +    if (m_node->document()->settings() && m_node->document()->settings()->passwordEchoEnabled() && m_node->renderer()) {

Is the renderer() check needed, given that we checked rendererIsEditable() above?

> Source/WebCore/rendering/RenderText.h:120
> +    void secureText(UChar mask);

This shouldn't be public, should it?

[Chang Shu]

Created attachment 104434 [details]
patch 11: r+ed with minor changes

[WebKit Review Bot]

Comment on attachment 104434 [details]
patch 11: r+ed with minor changes

Attachment 104434 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9426670

New failing tests:
editing/inserting/4875189-1.html

[WebKit Review Bot]

Comment on attachment 104434 [details]
patch 11: r+ed with minor changes

Attachment 104434 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9438022

New failing tests:
editing/inserting/4875189-1.html

[Chang Shu]

> New failing tests:
> editing/inserting/4875189-1.html

I don't get why the above test failed with my patch. I have tried to build the latest chromium with my patch on linux and it passed. Besides, there's not much difference between patch 11 and patch 10 but patch 10 passed chromium. Can anyone give me a clue? thanks!

[Chang Shu]

Comment on attachment 104434 [details]
patch 11: r+ed with minor changes

from the buldbot, i see r93392 is red. retry my patch.

[WebKit Review Bot]

Comment on attachment 104434 [details]
patch 11: r+ed with minor changes

Rejecting attachment 104434 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 2

Last 500 characters of output:
 svg/custom/svg-fonts-word-spacing.html = IMAGE+TEXT

Regressions: Unexpected image mismatch : (5)
  fast/text/atsui-multiple-renderers.html = IMAGE
  fast/text/international/danda-space.html = IMAGE
  fast/text/international/thai-baht-space.html = IMAGE
  fast/text/international/thai-line-breaks.html = IMAGE
  platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE

Regressions: Unexpected DumpRenderTree crashes : (1)
  editing/inserting/4875189-1.html = CRASH



Full output: http://queues.webkit.org/results/9434376

[WebKit Review Bot]

Comment on attachment 104434 [details]
patch 11: r+ed with minor changes

Rejecting attachment 104434 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 2

Last 500 characters of output:
 svg/custom/svg-fonts-word-spacing.html = IMAGE+TEXT

Regressions: Unexpected image mismatch : (5)
  fast/text/atsui-multiple-renderers.html = IMAGE
  fast/text/international/danda-space.html = IMAGE
  fast/text/international/thai-baht-space.html = IMAGE
  fast/text/international/thai-line-breaks.html = IMAGE
  platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE

Regressions: Unexpected DumpRenderTree crashes : (1)
  editing/inserting/4875189-1.html = CRASH



Full output: http://queues.webkit.org/results/9467897

[Chang Shu]

Created attachment 104908 [details]
patch 12: fix the crash on chromium

The root cause of the crash on chromium was some silly coding in Internals functions.

[WebKit Review Bot]

Comment on attachment 104908 [details]
patch 12: fix the crash on chromium

Clearing flags on attachment: 104908

Committed r93656: <http://trac.webkit.org/changeset/93656>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r93656 cherry-picked into qtwebkit-2.2 with commit 3502319 <http://gitorious.org/webkit/qtwebkit/commit/3502319>