[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
bugzilla-daemon at webkit.org
Wed Nov 11 13:20:25 PST 2009
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
More information about the webkit-reviews
mailing list