Bug 31265 – Find a new home for setUseSecureKeyboardEntry (and Mac implementation)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Stuart Morgan]

In order to give Chromium access to the secure text entry code, it was moved to Frame.cpp, which isn't ideal. From bug 31083 comment 6:
> Clearly all the ports that don't compile on Mac OS X don't want this code.
> Since Mac is taken to mean the native Mac WebKit, we may just need a filename
> suffix that means anything running on the Mac platform, which could include Qt
> or GTK or Chromium when compiling on Mac OS X.
> 
> One of the cleanest ways to refactor this would be to move the code into
> WebCore/platform and just call it from Frame or SelectionController, since it's
> definitely a bunch of "talking to the underlying platform" code just inline
> there because it was written *so* long ago.
> 
> Side note: If we are touching this anyway, it would be good to move it to
> SelectionController. As you can see in Frame.h, this is supposed to be moved
> out of the Frame class.

[Stuart Morgan]

Created attachment 42810 [details]
Incremental improvement

This does most of what was requested, if I understood correctly.

[Stuart Morgan]

I can do a second patch to create a new file suffix for "everything on the Mac", but I wanted to get the easy stuff done now. Before I actually make a patch that adds a new file convention though, there should be buy-in on what that should be (what suffix, and what exactly should it mean? Should there be a corresponding PLATFORM?). Is the mailing list the best place to bring that up?

[Darin Adler]

Comment on attachment 42810 [details]
Incremental improvement

> +    void updateSecureKeyboardEntryIfActive();
> +    void setUseSecureKeyboardEntry(bool enable);

Since if you take my advice below it will only be used within the SelectionController class, setUseSecureKeyboardEntry should be private rather than public.

>      if (m_doc && selection()->isFocusedAndActive())
> -        setUseSecureKeyboardEntry(m_doc->useSecureKeyboardEntryWhenActive());
> +        selection()->setUseSecureKeyboardEntry(m_doc->useSecureKeyboardEntryWhenActive());

This should call updateSecureKeyboardEntryIfActive instead.

Even the null check of the document could be done inside updateSecureKeyboardEntryIfActive, I think.

> +#include "SecureTextInput.h"

You need to include "config.h" first in any .cpp file.

> +#if PLATFORM(MAC) || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN))
> +#import <Carbon/Carbon.h>
> +#endif

PLATFORM(MAC) doesn't need this include due to the prefix header file for WebCore. Does Chromium really need it?

> +#if PLATFORM(MAC) || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN))
> +const short enableRomanKeyboardsOnly = -23;
> +void enableSecureTextInput()

I think we should stick this Tiger-only constant, enableRomanKeyboardsOnly, inside an #ifdef with a bit of whitespace around it. No need to crowd it next to the function the way it was before.

> +{
> +    if (IsSecureEventInputEnabled())
> +        return;
> +    EnableSecureEventInput();
> +#ifdef BUILDING_ON_TIGER
> +    KeyScript(enableRomanKeyboardsOnly);
> +#else
> +    // WebKit substitutes nil for input context when in password field, which corresponds to null TSMDocument. So, there is
> +    // no need to call TSMGetActiveDocument(), which may return an incorrect result when selection hasn't been yet updated
> +    // after focusing a node.
> +    CFArrayRef inputSources = TISCreateASCIICapableInputSourceList();
> +    TSMSetDocumentProperty(0, kTSMDocumentEnabledInputSourcesPropertyTag, sizeof(CFArrayRef), &inputSources);
> +    CFRelease(inputSources);
> +#endif
> +}
> +
> +void disableSecureTextInput()
> +{
> +    if (!IsSecureEventInputEnabled())
> +        return;
> +    DisableSecureEventInput();
> +#ifdef BUILDING_ON_TIGER
> +    KeyScript(smKeyEnableKybds);
> +#else
> +    TSMRemoveDocumentProperty(0, kTSMDocumentEnabledInputSourcesPropertyTag);
> +#endif
> +}
> +#endif
> +
> +} // namespace WebCore

It's wrong to take code originally written by an Apple engineer in 2006, put it in a new source file, and label that file with Copyright 2009 Google. Please fix that.

> Property changes on: WebCore/platform/SecureTextInput.cpp
> ___________________________________________________________________
> Added: svn:eol-style
>    + LF

Other source files have eol-style set to native, not LF. You should try to be consistent with the others, or perhaps we should change our default.

> +#if !(PLATFORM(MAC) || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN)))
> +    inline void enableSecureTextInput() { }
> +    inline void disableSecureTextInput() { }
> +#endif

Since this condition is already in this header, we might want to define something so we don't have to repeat the condition inside the .cpp file as well.

> Property changes on: WebCore/platform/SecureTextInput.h
> ___________________________________________________________________
> Added: svn:eol-style
>    + LF

Other source files have eol-style set to native, not LF. You should try to be consistent with the others, or perhaps we should change our default.

[Darin Adler]

Thanks for taking this on, by the way!

[Stuart Morgan]

Created attachment 42890 [details]
Incremental improvement, v2

Addresses review comments (sorry for the WebKit-n00b mistakes!)

Two questions:
- This uses the FrameMac.cpp copyright block used for the moved code (except with the date changed to 2006, since as you said the code predates that file). I've also noticed some hybrid copyrights that list both Apple, with one date, and Google, with another, but wasn't sure when that was called for; since the non-moved code here was negligible I asume it doesn't need that, but let me know if there's a convention I should follow.
- Regarding the Carbon include, build-webkit --debug fails for me, choking on all the Carbon symbols in that file, if I don't include it, so it does seem to be necessary for PLATFORM(MAC). Is there something I have configured wrong?

[Darin Adler]

Comment on attachment 42890 [details]
Incremental improvement, v2

> +    void setUseSecureKeyboardEntry(bool enable);

Normally we'd leave out the argument when it doesn't make things any clearer. I would leave it out here.

> -    m_doc = newDoc;
> -    if (m_doc && selection()->isFocusedAndActive())
> -        setUseSecureKeyboardEntry(m_doc->useSecureKeyboardEntryWhenActive());
> +    selection()->updateSecureKeyboardEntryIfActive();
>  
> +    m_doc = newDoc;

Doesn’t this need to be in the other order? Can updateSecureKeyboardEntryIfActive do the right thing if m_doc is not set to the new document?

> + * Copyright (C) 2006 Apple Inc. All rights reserved.

Better. Could list any other years where substantial changes were made to the code, since they count as publication dates. I think 2006, 2007 would be right, but maybe there are interesting 2008 and 2009 changes.

Ideally this would have the newer version of the license: the one in WebCore/LICENSE-APPLE.

> +    // Once enableSecureTextInput is called, secure text input mode in set

"is set", not "in set".

review- because of the ordering issue in Frame::setDocument

[Stuart Morgan]

Created attachment 44177 [details]
Incremental improvement, v3

Addresses review comments.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/SecureTextInput.h:44:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1

[Adam Barth]

Comment on attachment 44177 [details]
Incremental improvement, v3

Would you mind fixing the namespace indent style issue since this is a new file?

[Stuart Morgan]

Created attachment 44244 [details]
Incremental improvement, v4

Done

[Maciej Stachowiak]

Comment on attachment 44244 [details]
Incremental improvement, v4

> #if PLATFORM(MAC) || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN))
> #define USES_CARBON_SECURE_INPUT_MODE 1
> #endif

I think the right way to do this is to make a WTF-style USE macro as in Platform.h, so at the use site it's #if USE(CARBON_SECURE_INPUT_MODE)

r- to consider this suggestion. Otherwise looks ok to me.

[Stuart Morgan]

Created attachment 48920 [details]
Incremental improvement, v5

Switched to USE(CARBON_SECURE_INPUT_MODE), and updated for bitrot.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/SecureTextInput.cpp:70:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1


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

[Stuart Morgan]

Created attachment 48927 [details]
v6

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/SecureTextInput.cpp:70:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/SecureTextInput.cpp:72:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 2


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

[Eric Seidel (no email)]

Comment on attachment 48927 [details]
v6

Why would these want to be free functions instead of at least static class functions?

[Stuart Morgan]

(In reply to comment #16)
> Why would these want to be free functions instead of at least static class
> functions?

For the same reason that SuddenTermination.h, which I modelled this on since it's conceptually very similar, doesn't use a class. You'd have to ask Darin why that is, but I would guess it's because there's no particular reason to do so.

[Stuart Morgan]

Created attachment 48942 [details]
v7

Sigh; trying again since I misunderstood the style bot.

[Eric Seidel (no email)]

Comment on attachment 48942 [details]
v7

This patch does not apply.  Please click on one of the purple bubbles to see why.  It's more difficult to review patches which do not apply because the reviewers don't get to see the EWS results.

[Stuart Morgan]

Created attachment 50224 [details]
v7, respun

The last version failed to apply because someone touched the project file in the window between me spinning the patch and the bot processing it. Hopefully I'll get luckier this time.

[WebKit Commit Bot]

Comment on attachment 50224 [details]
v7, respun

Clearing flags on attachment: 50224

Committed r55673: <http://trac.webkit.org/changeset/55673>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.