[webkit-reviews] review requested: [Bug 23499] Add WML specific logic for WML <input> : [Attachment 26994] updated patch

[bugzilla-daemon at webkit.org]

yichao.yin <yichao.yin at torchmobile.com.cn> has asked  for review:
Bug 23499: Add WML specific logic for WML <input>
https://bugs.webkit.org/show_bug.cgi?id=23499

Attachment 26994: updated patch
https://bugs.webkit.org/attachment.cgi?id=26994&action=review

------- Additional Comments from yichao.yin <yichao.yin at torchmobile.com.cn>
updated patch which fixes some issues Niko points out.


Addtional explain for Niko's comments

> > +	 static const String s_formatCodes;
> I'm not sure wheter it's wise to declare a static const String.
> What you could do is add a private function: "static const AtomicString&
> formatCodes();"
> const AtomicString& WMLInputElement::formatCodes()
> {
>     DEFINE_STATIC_LOCAL(AtomicString, codes, ("AaNnXxMm"));
>     return codes;
> }

Since formatCodes() is private and independent, I think it's better add the
function "static const AtomicString& formatCodes() const" in the source file
instead of defining it as a member of HTMLInputElement.



> > @@ -215,6 +235,9 @@ void WMLInputElement::attach()
> > +	 // Here we initialize the WML <input> element
> > +	 init();
> I'm not sure if it's desired to call init() from attach().
> How about calling it from 'finishedParsingChildren()' ?
> The drawback using attach() would be that I could force the renderer to be
> attached, detach and re-attached.
> This way we'd store the variable state twice, that's not what we want I fear.


>From functionality, placing it in finishedParsingChildren() is ok under most
situations. but the run-webkit-tests script can't get the expected rendering
result similar with that QtLauncher gets. As a result some cases will fail. But
if I place it in attach(), both of them works well. It seems the layout test
frameworks for WML needs calling init() multiple times to make sure the
rendering tree is right. exactly, ensure the vaiable value is correct. Anyway I
still keep it in attach() to make everthing is ok for now.