Bug 44950 – [WML] Add create functions to WML

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Gyuyoung Kim]

Bug 44851 patch add create function to create WML element. However, the patch didn't add all wml elements. So, I fix them.

[Gyuyoung Kim]

Created attachment 66051 [details]
Patch

[Dirk Schulze]

Comment on attachment 66051 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=66051&action=prettypatch

> WebCore/wml/WMLFormControlElement.h:33
>      WMLFormControlElement(const QualifiedName&, Document*);
should be private now.

Also don't we miss PassRefPtr<WMLFormControlElement> create(..) in the cpp now?

> WebCore/wml/WMLIntrinsicEvent.h:37
> +    static PassRefPtr<WMLIntrinsicEvent> create(const QualifiedName&, Document*);
Dito. Where is this function located? Did you miss it? I didn't see a WMLIntrinsicEvent(const QualifiedName&, Document*) call before. Do we need this here at all?

[Gyuyoung Kim]

Created attachment 66643 [details]
Patch

>> > WebCore/wml/WMLFormControlElement.h:33
>> >      WMLFormControlElement(const QualifiedName&, Document*);
>> should be private now.

Ok, I move create(...) to private.

>> Also don't we miss PassRefPtr<WMLFormControlElement> create(..) in the cpp now?
WMLFormControlElement.cpp already has the create function as below,

  38  PassRefPtr<WMLFormControlElement> WMLFormControlElement::create(const QualifiedName& tagName,   Document* document)
  39 {
  40     return adoptRef(new WMLFormControlElement(tagName, document));
  41 }


>> > WebCore/wml/WMLIntrinsicEvent.h:37
>> > +    static PassRefPtr<WMLIntrinsicEvent> create(const QualifiedName&, Document*);
>> Dito. Where is this function located? Did you miss it? I didn't see a WMLIntrinsicEvent(const >> QualifiedName&, Document*) call before. Do we need this here at all?

WMLIntrinsicEvent.cpp already has create() as below. However, there is no definition for the create(). So, I define the create(...) in WMLIntrinsicEvent.h.

 51 PassRefPtr<WMLIntrinsicEvent> WMLIntrinsicEvent::create(const QualifiedName& tagName, Document* document)
 52 {
 53     return adoptRef(new WMLIntrinsicEvent(tagName, document));
 54 }

In addition, the create() return a WMLIntrinsicEvent instance. But, there is not define the "new WMLIntrinsicEvent(tagName, document)". So, I add the construct function as well.

 +WMLIntrinsicEvent::WMLIntrinsicEvent(const QualifiedName& tagName, Document* document)
 61 +    : m_taskElement(createTaskElement(document))
 62 +{
 63 +}
 64 +

BTW, there is additional wrong code in WMLDocument.h. I fix it together with this patch.

 33      static PassRefPtr<WMLDocument> create(Frame* frame, const KURL& url)
 34      {
 35 -        return adoptRef(adoptRef(new WMLDocument(frame, url))));
 36 +        return adoptRef(new WMLDocument(frame, url));


And, there is a coding style error in WMLIntrinsicEvent.h. I fix it as well.
 
 70 --- a/WebCore/wml/WMLIntrinsicEvent.h
 71 +++ b/WebCore/wml/WMLIntrinsicEvent.h
 72 @@ -22,18 +22,20 @@
 73  #define WMLIntrinsicEvent_h
 74 
 75  #if ENABLE(WML)
 76 +#include "WMLTaskElement.h"
 77 +
 78  #include <wtf/PassRefPtr.h>
 79  #include <wtf/RefCounted.h>
 80  #include <wtf/RefPtr.h>
 81 
 82 -#include "WMLTaskElement.h"

Now, I should go to bed. I will reply your comments tomorrow. :) See you.

[Dirk Schulze]

Comment on attachment 66643 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=66643&action=prettypatch

> WebCore/wml/WMLFormControlElement.h:48
> +    static PassRefPtr<WMLFormControlElement> create(const QualifiedName&, Document*);
My first comment was maybe a bit confusing. 'create' should be public and the CTor should be private :-)

Looks like the WML code is realy bad maintened. Good catches!

Please go a bit more into details in the Changelog. Also mention which commit broke WML and that this patch tries to get WML building again.

You'll get r=me after you uploaded a new version with the changes above.

[Gyuyoung Kim]

Created attachment 66676 [details]
Patch

Dirk Schulze , Thank you for your review. I modify this patch according to your comment.

When patches are landed to mainline, it seems the patch didn't consider WML. So, there are many build breaks when WML is enabled. In addition, it seems to me that nobody maintains WML. 

However, I need to use WML in my project. So, I am considering if WebKit's WML can be used or not.
I continue to maintain WML. :) Thank you.

Please review one more bug. Bug 44954

[Dirk Schulze]

Comment on attachment 66676 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=66676&action=prettypatch

> WebCore/wml/WMLFormControlElement.h:34
>  public:
> +    static PassRefPtr<WMLFormControlElement> create(const QualifiedName&, Document*);
> +
>      WMLFormControlElement(const QualifiedName&, Document*);
>      virtual ~WMLFormControlElement();
sorry, I thougt it was clear now. The Ctor: WMLFormControlElement(const QualifiedName&, Document*); should be private now! The same way like you did it for WMLIntrinsicEvent.

Rest looks fine.

[Gyuyoung Kim]

All other files defines the create(...) as public. It seems to me that we also define the create(...) as public. I think the create() function make a instance from other class. So, the create need to be set public.

How do you think about it ?

[Dirk Schulze]

(In reply to comment #7)
> All other files defines the create(...) as public. It seems to me that we also define the create(...) as public. I think the create() function make a instance from other class. So, the create need to be set public.
> 
> How do you think about it ?

Sure. 'create' as public, the Ctor as private:

public:
     static PassRefPtr<WMLFormControlElement> create(const QualifiedName&, Document*);
     ....

private:
    WMLFormControlElement(const QualifiedName&, Document*);

[Gyuyoung Kim]

Created attachment 66688 [details]
Patch

Ok, I fix it. :)

[Dirk Schulze]

Comment on attachment 66688 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=66688&action=prettypatch

> WebCore/wml/WMLFormControlElement.h:33
> +    static PassRefPtr<WMLFormControlElement> create(const QualifiedName&, Document*);
> +
>      WMLFormControlElement(const QualifiedName&, Document*);
Did you upload the wrong patch? Still no change here!

[Gyuyoung Kim]

(In reply to comment #10)
> (From update of attachment 66688 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=66688&action=prettypatch
> 
> > WebCore/wml/WMLFormControlElement.h:33
> > +    static PassRefPtr<WMLFormControlElement> create(const QualifiedName&, Document*);
> > +
> >      WMLFormControlElement(const QualifiedName&, Document*);
> Did you upload the wrong patch? Still no change here!

As mentioned in Comment #7, almost create() function is set as public. Should I set the create as private ?

[Dirk Schulze]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 66688 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=66688&action=prettypatch
> > 
> > > WebCore/wml/WMLFormControlElement.h:33
> > > +    static PassRefPtr<WMLFormControlElement> create(const QualifiedName&, Document*);
> > > +
> > >      WMLFormControlElement(const QualifiedName&, Document*);
> > Did you upload the wrong patch? Still no change here!
> 
> As mentioned in Comment #7, almost create() function is set as public. Should I set the create as private ?

Please read https://bugs.webkit.org/show_bug.cgi?id=44950#c8 again. 'create()' should be public and WMLFormControlElement(const QualifiedName&, Document*); should be private. I even wrote the code down for you. Just copy paste it if you want.

[Gyuyoung Kim]

Created attachment 66693 [details]
Patch

I am sorry :(. I only focused on create() functions. ㅜ.ㅜ. 

I fixed it again.
Thanks.

[Dirk Schulze]

Comment on attachment 66693 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=66693&action=prettypatch

> WebCore/wml/WMLIntrinsicEvent.h:38
> +    static PassRefPtr<WMLIntrinsicEvent> create(const QualifiedName&, Document*);
> +
I'm realy sorry, I looked at Maciejs patch again. I think the new constructor doesn't make sense. Maciej may made a mistake here. The new create function and the new constructor are wrong in this context. Please undo the changes except of the coding style change and delete PassRefPtr<WMLIntrinsicEvent> create(const QualifiedName&, Document*); from WebCore/wml/WMLIntrinsicEvent.cpp instead.

[Gyuyoung Kim]

Created attachment 66697 [details]
Patch

No Problem, I fix it again. BTW, when I change " WMLFormControlElement(const QualifiedName&, Document*);" with private, there are build breaks as below,

==================================================================================================
/home/gyuyoung/webkit/WebKit-WML/WebCore/wml/WMLFormControlElement.h: In constructor ‘WebCore::WMLOptGroupElement::WMLOptGroupElement(const WebCore::QualifiedName&, WebCore::Document*)’:
/home/gyuyoung/webkit/WebKit-WML/WebCore/wml/WMLFormControlElement.h:49: error: ‘WebCore::WMLFormControlElement::WMLFormControlElement(const WebCore::QualifiedName&, WebCore::Document*)’ is private
/home/gyuyoung/webkit/WebKit-WML/WebCore/wml/WMLOptGroupElement.cpp:39: error: within this context
[  9%] Building CXX object WebCore/CMakeFiles/webcore_efl.dir/wml/WMLPrevElement.cpp.o
make[2]: *** [WebCore/CMakeFiles/webcore_efl.dir/wml/WMLOptGroupElement.cpp.o] 오류 1
make[2]: *** 끝나지 않은 작업을 기다리고 있습니다....

/home/gyuyoung/webkit/WebKit-WML/WebCore/wml/WMLFormControlElement.h: In constructor ‘WebCore::WMLInputElement::WMLInputElement(const WebCore::QualifiedName&, WebCore::Document*)’:
/home/gyuyoung/webkit/WebKit-WML/WebCore/wml/WMLFormControlElement.h:49: error: ‘WebCore::WMLFormControlElement::WMLFormControlElement(const WebCore::QualifiedName&, WebCore::Document*)’ is private
/home/gyuyoung/webkit/WebKit-WML/WebCore/wml/WMLInputElement.cpp:44: error: within this context
make[2]: *** [WebCore/CMakeFiles/webcore_efl.dir/wml/WMLInputElement.cpp.o] Error 1
/home/gyuyoung/webkit/WebKit-WML/WebCore/wml/WMLFormControlElement.h: In constructor ‘WebCore::WMLOptionElement::WMLOptionElement(const WebCore::QualifiedName&, WebCore::Document*)’:
/home/gyuyoung/webkit/WebKit-WML/WebCore/wml/WMLFormControlElement.h:49: error: ‘WebCore::WMLFormControlElement::WMLFormControlElement(const WebCore::QualifiedName&, WebCore::Document*)’ is private
/home/gyuyoung/webkit/WebKit-WML/WebCore/wml/WMLOptionElement.cpp:38: error: within this context
make[2]: *** [WebCore/CMakeFiles/webcore_efl.dir/wml/WMLOptionElement.cpp.o] Error 1
==================================================================================================

I think WMLOptGroupElement and WMLInputElement invoke the Ctor as below. So, I move the Ctor to public again. Is this OK ?

==================================================================================================
WMLOptGroupElement::WMLOptGroupElement(const QualifiedName& tagName, Document* doc)
    : WMLFormControlElement(tagName, doc)

WMLInputElement::WMLInputElement(const QualifiedName& tagName, Document* doc)
    : WMLFormControlElement(tagName, doc)
==================================================================================================

[Dirk Schulze]

(In reply to comment #15)
> Created an attachment (id=66697) [details]
> Patch
> 
> No Problem, I fix it again. BTW, when I change " WMLFormControlElement(const QualifiedName&, Document*);" with private, there are build breaks as below,
> 
Make it protected.

[Gyuyoung Kim]

Created attachment 66830 [details]
Patch

Sorry for my late upload patch. I had private promise with my friend.
Ok, I set the constructor as protected. :) 

Thanks.

[Dirk Schulze]

Comment on attachment 66830 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=66830&action=prettypatch

LGTM. Thanks for working on WML! r=me.

[WebKit Commit Bot]

Comment on attachment 66830 [details]
Patch

Clearing flags on attachment: 66830

Committed r66977: <http://trac.webkit.org/changeset/66977>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.