Summary: | Move makeSecure from WTF::String to renderText | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chang Shu <cshu> | ||||||||||
Component: | JavaScriptCore | Assignee: | Chang Shu <cshu> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ap, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | All | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 32509 | ||||||||||||
Attachments: |
|
Description
Chang Shu
2011-04-21 10:33:37 PDT
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. |