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

[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 42890: Incremental improvement, v2
https://bugs.webkit.org/attachment.cgi?id=42890&action=review

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