Bug 59114

Created attachment 90740 [details] fix patch

Comment on attachment 90740 [details] fix patch View in context: https://bugs.webkit.org/attachment.cgi?id=90740&action=review A good cleanup, but the isNull() check in makeSecure is wrong. > Source/JavaScriptCore/JavaScriptCore.order:-1686 > -__ZN7WebCore10StringImpl6secureEt You don't need to change order files. It's no problem if you do, but it's a waste of time. > Source/WebCore/rendering/RenderText.cpp:95 > +void makeSecure(String* string, UChar mask, LastCharacterBehavior behavior) I suggest making the string argument a reference instead of a pointer, to make it obvious that it can't be null. > Source/WebCore/rendering/RenderText.cpp:98 > + if (string->isNull()) > + return; You iterate up to length - 1 below, which will be no good if the string is empty. Why didn't you get crashes in testing? This should check for isEmpty() - or if there is some reason why empty strings can't get here, we should assert that, and/or restructure the code to make it more clear. > Source/WebCore/rendering/RenderText.h:211 > +void makeSecure(String*, UChar mask, LastCharacterBehavior = ObscureLastCharacter); I don't know if this is the best place for this function - RenderText.h doesn't currently have utility functions like this. Seems acceptable, but you could ask a rendering expert.

The dependency from 32509 is removed. No emergency to fix this issue.

Created attachment 100121 [details] patch 2

found a solution.

Comment on attachment 100121 [details] patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=100121&action=review > Source/JavaScriptCore/wtf/text/WTFString.h:226 > void makeUpper() { if (m_impl) m_impl = m_impl->upper(); } > - void makeSecure(UChar aChar) { if (m_impl) m_impl = m_impl->secure(aChar); } > + void fill(UChar aChar) > + { > + if (m_impl) > + m_impl = m_impl->fill(aChar); > + } Why does this use a different style than functions above? I'd have named the variable "character" or "c", not "aChar".

(In reply to comment #6) > (From update of attachment 100121 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100121&action=review > > > Source/JavaScriptCore/wtf/text/WTFString.h:226 > > void makeUpper() { if (m_impl) m_impl = m_impl->upper(); } > > - void makeSecure(UChar aChar) { if (m_impl) m_impl = m_impl->secure(aChar); } > > + void fill(UChar aChar) > > + { > > + if (m_impl) > > + m_impl = m_impl->fill(aChar); > > + } > > Why does this use a different style than functions above? > > I'd have named the variable "character" or "c", not "aChar". The style script reports this: More than one command on the same line in if [whitespace/parens] [4] So I had to change it to multi-line. All others have the same issue but they were there before the style rule, I believe. Maybe we should change the script.

Created attachment 100125 [details] patch 3: minor changes after r+

> The style script reports this: The script is not a reviewer :)

I think that consistency is more important in this case than style script suggestions.

(In reply to comment #10) > I think that consistency is more important in this case than style script suggestions. Would the patch be committed if not passing style check?

Created attachment 100129 [details] patch 4: minor changes after r+

Attachment 100129 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/wtf/text/WTFString.h:222: More than one command on the same line in if [whitespace/parens] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 100129 [details] patch 4: minor changes after r+ Clearing flags on attachment: 100129 Committed r90643: <http://trac.webkit.org/changeset/90643>

All reviewed patches have been landed. Closing bug.