Bug 31265 - Find a new home for setUseSecureKeyboardEntry (and Mac implementation)
Summary: Find a new home for setUseSecureKeyboardEntry (and Mac implementation)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Normal
Assignee: Stuart Morgan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-09 11:20 PST by Stuart Morgan
Modified: 2010-03-08 11:22 PST (History)
8 users (show)

See Also:


Attachments
Incremental improvement (17.40 KB, patch)
2009-11-09 15:22 PST, Stuart Morgan
darin: review-
Details | Formatted Diff | Diff
Incremental improvement, v2 (17.74 KB, patch)
2009-11-10 13:34 PST, Stuart Morgan
darin: review-
Details | Formatted Diff | Diff
Incremental improvement, v3 (17.49 KB, patch)
2009-12-02 13:25 PST, Stuart Morgan
no flags Details | Formatted Diff | Diff
Incremental improvement, v4 (17.47 KB, patch)
2009-12-03 08:55 PST, Stuart Morgan
mjs: review-
Details | Formatted Diff | Diff
Incremental improvement, v5 (18.45 KB, patch)
2010-02-17 11:57 PST, Stuart Morgan
no flags Details | Formatted Diff | Diff
v6 (18.45 KB, patch)
2010-02-17 12:55 PST, Stuart Morgan
no flags Details | Formatted Diff | Diff
v7 (18.45 KB, patch)
2010-02-17 15:11 PST, Stuart Morgan
eric: review-
Details | Formatted Diff | Diff
v7, respun (18.50 KB, patch)
2010-03-08 08:19 PST, Stuart Morgan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stuart Morgan 2009-11-09 11:20:35 PST
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.
Comment 1 Stuart Morgan 2009-11-09 15:22:01 PST
Created attachment 42810 [details]
Incremental improvement

This does most of what was requested, if I understood correctly.
Comment 2 Stuart Morgan 2009-11-09 15:22:25 PST
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?
Comment 3 Darin Adler 2009-11-09 16:29:55 PST
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.
Comment 4 Darin Adler 2009-11-09 16:30:04 PST
Thanks for taking this on, by the way!
Comment 5 Stuart Morgan 2009-11-10 13:34:02 PST
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?
Comment 6 Darin Adler 2009-11-11 13:20:26 PST
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
Comment 7 Stuart Morgan 2009-12-02 13:25:19 PST
Created attachment 44177 [details]
Incremental improvement, v3

Addresses review comments.
Comment 8 WebKit Review Bot 2009-12-02 21:11:48 PST
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
Comment 9 Adam Barth 2009-12-02 21:34:25 PST
Comment on attachment 44177 [details]
Incremental improvement, v3

Would you mind fixing the namespace indent style issue since this is a new file?
Comment 10 Stuart Morgan 2009-12-03 08:55:36 PST
Created attachment 44244 [details]
Incremental improvement, v4

Done
Comment 11 Maciej Stachowiak 2009-12-28 18:21:03 PST
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.
Comment 12 Stuart Morgan 2010-02-17 11:57:39 PST
Created attachment 48920 [details]
Incremental improvement, v5

Switched to USE(CARBON_SECURE_INPUT_MODE), and updated for bitrot.
Comment 13 WebKit Review Bot 2010-02-17 12:02:55 PST
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.
Comment 14 Stuart Morgan 2010-02-17 12:55:05 PST
Created attachment 48927 [details]
v6
Comment 15 WebKit Review Bot 2010-02-17 13:01:51 PST
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.
Comment 16 Eric Seidel (no email) 2010-02-17 14:49:00 PST
Comment on attachment 48927 [details]
v6

Why would these want to be free functions instead of at least static class functions?
Comment 17 Stuart Morgan 2010-02-17 15:10:40 PST
(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.
Comment 18 Stuart Morgan 2010-02-17 15:11:34 PST
Created attachment 48942 [details]
v7

Sigh; trying again since I misunderstood the style bot.
Comment 19 Eric Seidel (no email) 2010-03-05 13:04:50 PST
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.
Comment 20 Stuart Morgan 2010-03-08 08:19:33 PST
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.
Comment 21 WebKit Commit Bot 2010-03-08 11:22:51 PST
Comment on attachment 50224 [details]
v7, respun

Clearing flags on attachment: 50224

Committed r55673: <http://trac.webkit.org/changeset/55673>
Comment 22 WebKit Commit Bot 2010-03-08 11:22:57 PST
All reviewed patches have been landed.  Closing bug.