Bug 59114 – Move makeSecure from WTF::String to renderText

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Chang Shu]

makeSecure is not a general-purpose string utility but a particular operation intended for rendertext only. We should move it to the right place to avoid layering violation.

[Chang Shu]

Created attachment 90740 [details]
fix patch

[Alexey Proskuryakov]

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.

[Chang Shu]

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

[Chang Shu]

Created attachment 100121 [details]
patch 2

[Chang Shu]

found a solution.

[Alexey Proskuryakov]

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".

[Chang Shu]

(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.

[Chang Shu]

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

[Alexey Proskuryakov]

> The style script reports this:

The script is not a reviewer :)

[Alexey Proskuryakov]

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

[Chang Shu]

(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?

[Chang Shu]

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

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

Clearing flags on attachment: 100129

Committed r90643: <http://trac.webkit.org/changeset/90643>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.