[webkit-reviews] review denied: [Bug 31265] Find a new home for setUseSecureKeyboardEntry (and Mac implementation) : [Attachment 42810] Incremental improvement

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Stuart Morgan
<stuartmorgan at chromium.org>'s request for review:
Bug 31265: Find a new home for setUseSecureKeyboardEntry (and Mac
implementation)
https://bugs.webkit.org/show_bug.cgi?id=31265

Attachment 42810: Incremental improvement
https://bugs.webkit.org/attachment.cgi?id=42810&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    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.