Bug 20393 – Add WML support to WebKit

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[George Staikos]

Upcoming patch adds WML document type and nodes to WebKit.  It is disabled by default, but can be enabled with ENABLE(WML).  Should work for Safari (Mac/Win) and Qt ports at least.  The patch probably has quite some formatting violations but given the independence of the patch I think that as long as they don't affect core code, it should go in and then be fixed in SVN.  The patch is large and impact from changes in SVN is non-trivial.  Work was done by Charles Wei, Yichao Yin of Torch Mobile, and me.

[George Staikos]

Created attachment 22813 [details]
Patch to implement WML

Implements WML.  Changelog will be included when patch is checked in.

[Mark Rowe (bdash)]

Comment on attachment 22813 [details]
Patch to implement WML

I've just skimmed over the patch briefly, but have a few comments.

ASCIICType.h currently uses character literals for character codes of printable characters, and hex literals for non-printable characters.  New code should follow this style.

The naming on some new functions and variables uses inconsistent case.  For example, defaultWmlStyle vs isWMLDocument.  The latter style is preferred.

It seems bad that knowledge of WML is sprinkled throughout XMLTokenizer.  There also appears to be code (lines 2134 through 2143) outside of any functions, which would not compile.

Including code that requires a third-party library (WBXML) directly in ResourceLoader doesn't seem like a great idea either.

Destructors in new classes appear to be inconsistently declared as virtual, and many are declared and then left empty.  Empty destructors should be omitted.

There are numerous coding style issues with the patch as well.  I would prefer to see them addressed before the code is landed, primarily because I do not foresee this code getting as much love and maintenance as the rest of the code base going forward.

The primary issues that I see are:

1) Missing space between "if" and the opening parenthesis.
2) Unnecessary parentheses around some expressions in the conditions of "if" statements.
3) Many new .cpp files have incorrect ordering of include files.  The order should be config.h, the associated header file, a blank line, and then the rest of the includes in sorted order.  See existing .cpp files for an example.
4) One-line "if" statements should not have braces.
5) The placement of braces in function declarations is inconsistent.  Braces should be placed on their own line in function declarations.
6) The constant 0 should be used rather than NULL.
7) Enum members should user InterCaps with an initial capital letter, rather than ALLCAPS.

All that said, I love the pile of layout tests!  Are expected results also going to be included?

[Mark Rowe (bdash)]

I should mention that the coding style guidelines are available at <http://webkit.org/coding/coding-style.html>.  They cover most of the points I raised, plus some others that I may have missed.

[Timothy Hatcher]

(In reply to comment #1)
> Implements WML.  Changelog will be included when patch is checked in.

ChangeLogs usually help the review process, especially for huge code dumps like this! So I encourage you to write the ChangeLog now to help the review along.

[Rob Buis]

Comment on attachment 22813 [details]
Patch to implement WML

I think in general this patch is too large as-is to review. Therefore I only looked at coding style issues and not at what it exactly does. Maybe the patch can be split up in wml specific, changes to the non-wml code and tests?

The coding style does not seem consistent, which is wrong per definition. I assume the webkit style must be followed, which means all if(, for( and while( uses should be if (, for ( and while (,
Here are some coding style issues:

Example of if( use where if ( should be used:

>+
>+void WMLAElement::defaultEventHandler(Event* evt) 
>+{
>+    bool shouldHandle = false;
>+    if(evt->type() == clickEvent) {
>+        shouldHandle = true;

Example of while( use where while ( should be used:

>+void WMLCardElement::registerDoElement(WMLDoElement* e)
>+{
>+    if(e) {
>+        Vector<WMLDoElement*>::iterator it = m_doVecs.begin();
>+        WMLDoElement* doe = NULL;
>+        while(it != m_doVecs.end()) {

Example of for( use where for ( should be used:

>+
>+    String templDoName;
>+    String cardDoName;
>+    WMLDoElement* templDo = 0;
>+    for(; templIt != templDoNameVecs.end(); ++templIt){
>+        templDo = *templIt;

Put { on a new line after functions:

>+void WMLTemplateElement::setHasIntrEvent(bool has) {
>+    m_hasIntrEvent = has;
>+}
>+
>+bool WMLTemplateElement::hasIntrEvent() {
>+    return m_hasIntrEvent;
>+}

In general my advise is to code all sources to match webkit coding style. Until then I think it makes sense to r- this, so that is what I do now.

[George Staikos]

I agree, but I think the concern is the parts that touch existing code.  The new files should also be fixed, but I believe the patch can go in and they can be fixed after.  It's quite a pain to work on this outside of svn.  It took days to merge to a point where it made sense to post the patch, and apparently there were even some monumental merge errors as mentioned by Mark.

If the size of the patch is a concern, how about we check in the tests, then check in the wml/ directory, then post the rest for review?  It's totally harmless like that.

[Sam Weinig]

(In reply to comment #6)
> I agree, but I think the concern is the parts that touch existing code.  The
> new files should also be fixed, but I believe the patch can go in and they can
> be fixed after.  It's quite a pain to work on this outside of svn.  It took
> days to merge to a point where it made sense to post the patch, and apparently
> there were even some monumental merge errors as mentioned by Mark.

While I understand the pain of working outside Trunk, I don't think it is a good idea to commit even the new files without fixing obvious style issues and other errors.  Since we don't expect this to be turned on in the mainline WebKit (or at least I see no reason to), the code is more prone to bit rot due to a lack of attention.

> 
> If the size of the patch is a concern, how about we check in the tests, then
> check in the wml/ directory, then post the rest for review?  It's totally
> harmless like that.
> 

This doesn't seem like the most ideal way to add it to the codebase, though perhaps the easiest.  We certainly need to review the new files in the wml/ directory as well, and that will be easier if it is in chunks.

[George Staikos]

To all who reviewed, instead of just saying what you don't like, can you suggest a way for us to review this and break it into pieces then?  I am the only one of the three of us with a Mac or Windows and have basically no time to spend reworking code formatting.  I spent 3 full days reworking the patch to make it work in trunk already and that's about it for me.  Our Qt environment is also very different from trunk.  The longer we play this debate game here the bigger chance you will just kill this patch though.  If it just can't go in because there are spaces missing in front of if() statements in the wml/ directory then I guess we'll have to live with that until someone writes a script to reformat the code or does it manually.

[George Staikos]

Created attachment 22955 [details]
Patch to implement WML

Updated to fix lots of if( and NULL vs 0 and other such things.

[Eric Seidel (no email)]

Well, certainly the layout tests could be split out into another patch.  As could the new files, as could the changes to core, as can the build files.

m_evntHandler should be m_eventHandler and should be an OwnPtr. :)

It should be possible to write a regexp to fix the { style violations:

bool WMLTemplateElement::hasIntrEvent() {

I sense that whoever wrote the patch hates vowels. :)

Obviously you all would prefer not to fork.  We'd prefer you not fork.  I'd certainly be happy to help review any patches under 50k... 20k would be of course preferred. :)  I'm also happy to chat about any of this over IRC.

[Eric Seidel (no email)]

Comment on attachment 22955 [details]
Patch to implement WML

Thank you very much for the patch George!  Unfortunately it's really unreviewable as-is.  We can't by-pass the normal review mechanisms even for a change off-by default.  However, we can certainly land this in pieces.  Even landing the individual new files w/o them being compiled in or being able to run.

There are really two questions here:
1.  Does WebKit.org want WML support?  I haven't heard any complaints against such (and I'm myself not against adding WML support.).

2.  Is this patch OK?

The answer to the second is "it might be", but we'll be able to tell better when split into smaller pieces which can be individually reviewed and landed (even if not built or tested initially).

Thanks again for the patch, but I'm going to mark this large one as r-.  I look forward to reviewing this in pieces.

[George Staikos]

Created attachment 23849 [details]
Layout tests

[Eric Seidel (no email)]

Comment on attachment 23849 [details]
Layout tests

The layout tests look fine.  Assuming they are available under the standard webkit licenses.  This will need a ChangeLog, and some edits to the Skipped files on the various platforms which do not yet have wml support.  I'm gonna clear the flag, and we can land this once the other pieces are reviewed.

[George Staikos]

Created attachment 23850 [details]
WML directory patch

These are the WML nodes.  There might be some style issues but I would really rather fix these once in trunk so we can avoid the constant merge errors that pop up on us.  We can do those style fixes immediately right in svn once the code is in, but delaying it further because of style will really delay it a long long time.

[Eric Seidel (no email)]

Comment on attachment 23850 [details]
WML directory patch

Ok, here are a few thoughts.

I'm not really sure how we go about landing such a large hunk of code.  Obviously the parts that touch the core code will need to be wrapped in ENABLE(WML).

I'm not "sure" that WebKit wants WML support, but I also don't see reasons to suggest that it does not.  I sorta thought WML was dead.

200k is too large of a patch for me to really review fully, but I'll at least provide some thoughts on the code.

Minor style violations:
69     }
 70     else
(I hate that we even have to care about these!  Tools should take care of this for us!)

I'm not really a fan of the "Helper" paradigm.  Helpers tend to be misc-buckets.  It seems this one is too.

WMLHelper::tokenizer()->
So I guess this is basically just a global shared tokenizer?  We haven't handled error reporting for SVG either, but I think normally the way you access the tokenizer is through your document object.  Which would mean we don't need this global here.

WMLHelper::currentBrwContext()->
What's a BrwContext?  It's not clear from the code.  I'm not sure I found the right WML spec, but I searched for Brw and didn't find it mentioned anywhere.

I'm not sure this belongs on "WMLHelper" either:
WMLHelper::hasVariableRef(
Why not WMLElement?

Minor style:
 43     : HTMLAnchorElement(name, doc), m_task(0)
Those are generally 1-per-line.  Again, its *stupid* that you, the programmer, should have to care about this... a script should handle this...

Whoever wrote this seems to like to add extra lines at the end of classes too:
 private:
 42 
 43     WMLTaskElement* m_task;
 44 
 45 };

FYI, there is a patch posted to change this syntax:
4 generateFactory="1"
 5 generateWrapperFactory="1"

I would think normal assignment should be sufficient here?
    for (VariableMap::iterator it = vars.begin(); it != vars.end(); ++it) { 
 77         m_variables.set(it->first, it->second); 
 78

So does WMLBrwContext have the same lifetime as the page?  aka, the WebView?
 41 WMLBrwContext::WMLBrwContext(Page* page)

Why is this safe?
void WMLBrwContext::setNeedCheckDeckAccess(bool need)
 124 {
 125     if (m_hasDeckAccess && need) {
 126         WMLHelper::tokenizer()->reportError(MultiAccessElementsError);
 127
Are we guarenteed to still be parsing at that time?  In the HTML/XML world, your tokenizer might not still be alive once you're done parsing.

So it seems Brw is actually an abbreviation for "Browser" and Brw is actually just an extension of Page, to handle a few WML-specific attributes.  Seems this all should go on Page as part of some WMLController sorta thing.

 57     , m_hasIntrEvent(false)
no need to abbreviate here.

Seems this should be an OwnPtr:
 73     WMLIntrinsicEventHandler* m_evntHandler;

Generally we encourage longer/more-descriptive naming:
 75     WMLIntrEventType etype = Unknown;

eventType in this case.

WebKit, for better or worse, is against { } on single line ifs:
    if (attr->name() == titleAttr) {
 78         m_title = WMLHelper::substituteVarRef(attr->value(), document());
 79     } else if (attr->name() == onenterforwardAttr) {
 80
again, it's stupid that you should even have to think about that... since a script shoudl do that for you.

Instead of using an enum and then an if:
 73 void WMLCardElement::parseMappedAttribute(MappedAttribute* attr)

should be changed to have 
 92     if (etype != Unknown) {

be a function which is called. 

 95         Element* go = WMLElementFactory::createWMLElement(qName, document(), false); 
Should use a RefPtr, no?  createWMLElement shoudl return a PassRefPtr

 94         QualifiedName qName("go", "go", "");
I don't think you meant for that to be "go:go", also why not use:
WMLNames::goTag?

 97         go->setAttributeNS("", "href", href, ec);
setAttribute?

 100         ev.task = static_cast<WMLGoElement*>(go);
One could have called WMLElementFactory::createGoElement() directly, or?

Why is visability tracked by hand?
 120 void WMLCardElement::setVisibility(bool isVisible) 
instead of using css styling?

Seems odd to clear the old timer on error:
75     // only one timer is allowed in a card 
 176     if (!m_evtTimer) {
 177         m_evtTimer = timer;
 178     } else {
 179         m_evtTimer = 0;
 180         WMLHelper::tokenizer()->reportError(MultiTimerElementsError);
 181     }

Please use early-return instead of a long if:
void WMLCardElement::registerDoElement(WMLDoElement* e)
 190 {
 191     if (e) {
 192         Vector<WMLDoElement*>::iterator it = m_doVecs.begin();
 193

Please use more descriptive variable names:
 193         WMLDoElement* doe = 0;

Same issue as before about safely of accessing tokenizer, and that it probably should be accessed through the document() instead of via this global:
            if (doe->doName() == e->doName()) {
 198                 WMLHelper::tokenizer()->reportError(SameNameDoElementsError);
 199


We use full english sentence names instead of abbreviations when possible:
 235 void WMLCardElement::handleIntrEventIfNeeded()

eventType:
 242     WMLIntrEventType etype;

Same as before:
 235 void WMLCardElement::handleIntrEventIfNeeded()

instead of using an enum, and then if's this should have used function calls.  Seems like a strange paradigm to me.

I got about 1/5th of the way through teh patch,but already there are plenty of comments to act on.

Thanks for the patch, but I think we should digest it in smaller chunks.  Landing large sections of code which were developed outside of the main tree is *hard*  (I know, believe me... I'm doing ti righ tnow for Chrome), but I do think there is value in the review process.

If you wanted to land this all on a branch and then work to update the branch over a set of reviews, that would be fine with me too.

[Eric Seidel (no email)]

I talked about this with mjs.  He and I are OK with corner-cutting to get WML landed.  Obviously it will have some issues, but as long as the parts which touch core-code are sane, and well locked off in ENABLE(WML) I think it's OK to land a not-quite-perfect "wml" directory.

How about you post the patches necessary to all of the WebKit core code and we get that reviewed and landed?

[George Staikos]

What is more desired for the rest of the patches?  One bit at a time checked in, or all of it in one shot?  None of it is really useful on its own but it shouldn't break the build or interfere with anything if it's checked in piecewise.  I could break it down per-directory, per-file, or perhaps some other way?

[Eric Seidel (no email)]

(In reply to comment #17)
> What is more desired for the rest of the patches?  One bit at a time checked
> in, or all of it in one shot?  None of it is really useful on its own but it
> shouldn't break the build or interfere with anything if it's checked in
> piecewise.  I could break it down per-directory, per-file, or perhaps some
> other way?
> 

Most interesting to me is to see the core-changes first.  We could check in the wml directory alone... but that seems a little silly since it's easy for you all to add those locally (check out another svn repo inside an svn checkout), but the core changes are what you'll actually need in over time, and the core changes are most of what we care about.  Maybe we can talk this out on IRC today, and get the core stuff reviewed today?

[George Staikos]

(In reply to comment #18)
> Most interesting to me is to see the core-changes first.  We could check in the
> wml directory alone... but that seems a little silly since it's easy for you
> all to add those locally (check out another svn repo inside an svn checkout),
> but the core changes are what you'll actually need in over time, and the core
> changes are most of what we care about.  Maybe we can talk this out on IRC
> today, and get the core stuff reviewed today?

Okay, I will generate the diff in the next day or two.  Question is, in one piece, broken down by directory, or some attempt to do it by sub-feature?  It's kind of hard to do the last one though.

[George Staikos]

Created attachment 24139 [details]
All the bits that integrate with existing code

[Eric Seidel (no email)]

Comment on attachment 24139 [details]
All the bits that integrate with existing code

Changes to DerivedSources.make need to be conditional based on ENABLE_WML.

Seems like you're using an old version of Xcode.  Which is OK, but causes 2 needless changes to the project file.

The big if in CSSStyleSelector::matchUARules(int& firstUARule, int& lastUARule) is kinda nasty.

* The default style sheet used to render HTML.
should be WML
Are you sure you want all those styles?  If you want to be so close to HTML, why not just override HTML styles instead of being totally separate?

I'm surprised the WML stuff needs its own error message insertion.  Wouldn't the XML case work?

Why is this needed in XMLTokenizer?
Document *document() const { return m_doc; }

And what private methods does friend class WMLTokenizer; need?  We've certainly found friend classes to cause crashes by calling private methods which weren't expected to be called...  If WMLTokenizer is a subclass of XMLTokenizer (as later code would suggest) then methods should be protected instead of using a friend class.

Globals are a bad idea here:
WMLHelper::currentBrwContext()->setNeedCheckDeckAccess(true);
as I mentioned in my review of the wml/ directory.

this is the wrong place to make this change:
#if ENABLE(WML)
 750     if (m_doc->isWMLDocument())
 751         newElement = WMLElementFactory::createWMLElement(qName, m_doc, true);
 752     else
 753 #endif
 754         newElement = m_doc->createElement(qName, true, ec);

There is a unified createElementNS which handles farming out element creation to different factories.  I take it that having a WML element inside an XML document would be bad news?  And vice-versa?

void BackForwardList::clearState()
is a bad method name.  What is "state" in this context?


Why?
#if ENABLE(WML)
 123     friend class WMLImageElement;
 124 #endif

There is already an abstraction for SVGImageLoader and HTMLImageLoader (at least I think there is), maybe you need WML image loader instead of hackign into HTMLImageLoader?

So it seems like you're subclassing HTML elements in order to add WML-specific behavior?  Could CSS be used for any of this?  Subclasses don't need friend declarations.  Instead things should be moved to "protected" to allow subclasses to access them.

TorchMobile is the only copyright I see adding a URL. :)

Why does WML need "Force reload"?

ResourceLoader didRecieveData is the wrong place to hack in WBXML decoding, IMO.  I'm not sure what the right place is thought, but probably in whatever handles decoding of the main resource.  For example in sub-resources, the CachedResource subclasses all handle their own decoding in their add data methods.

Why does "up" and "down" mean something on a WML page that's different from on any other page?  It seems that you're using ENABLE(WML) To mean PLATFORM(TORCH_MOBILE) in this case.

WMLBrwContext really needs a better name.  It's WMLPageData or something like that.  It's data you want to hang off of the page object which is WML specific.  It probably shouldn't be ref-counted, but should instead be OwnPtr on the Page, but I'm not sure about it's lifetime.

[yichao.yin]

Created attachment 24708 [details]
WML module

update patch for the WML implementaion. including
1) fix some code styles incompliant, such as braces,names, #include statements sorted order.
2) According to Eric's good advices, rededsign the implemention in part.

[yichao.yin]

Created attachment 24709 [details]
WebCore touched by WML

As per Eric's good suggestion, updated the WebCore-changes related to WML implementaion.

[yichao.yin]

Created attachment 24710 [details]
The whole submit for Adding WML

It is generated based on the latest version of Webkit: 37922. and has been verified.

[yichao.yin]

(In reply to comment #21)
> (From update of attachment 24139 [details] [edit])
> Changes to DerivedSources.make need to be conditional based on ENABLE_WML.
> Seems like you're using an old version of Xcode.  Which is OK, but causes 2
> needless changes to the project file.
> The big if in CSSStyleSelector::matchUARules(int& firstUARule, int& lastUARule)
> is kinda nasty.

You're right, it should not affect the old code. Has fixed.

> * The default style sheet used to render HTML.
> should be WML
> Are you sure you want all those styles?  If you want to be so close to HTML,
> why not just override HTML styles instead of being totally separate?

Actually, some style is WML specific, I think we'd better use WML own css file.

> I'm surprised the WML stuff needs its own error message insertion.  Wouldn't
> the XML case work?

Yes, It doesn't work.

> Why is this needed in XMLTokenizer?
> Document *document() const { return m_doc; }
> And what private methods does friend class WMLTokenizer; need?  We've certainly
> found friend classes to cause crashes by calling private methods which weren't
> expected to be called...  If WMLTokenizer is a subclass of XMLTokenizer (as
> later code would suggest) then methods should be protected instead of using a
> friend class.

All the friend class related to WML has been removed.

> Globals are a bad idea here:
> WMLHelper::currentBrwContext()->setNeedCheckDeckAccess(true);
> as I mentioned in my review of the wml/ directory.

It has been fixed.

> this is the wrong place to make this change:
> #if ENABLE(WML)
>  750     if (m_doc->isWMLDocument())
>  751         newElement = WMLElementFactory::createWMLElement(qName, m_doc,
> true);
>  752     else
>  753 #endif
>  754         newElement = m_doc->createElement(qName, true, ec);
> There is a unified createElementNS which handles farming out element creation
> to different factories.  I take it that having a WML element inside an XML
> document would be bad news?  And vice-versa?

It has been fixed.

> void BackForwardList::clearState()
> is a bad method name.  What is "state" in this context?

has updated the name.

> Why?
> #if ENABLE(WML)
>  123     friend class WMLImageElement;
>  124 #endif
> There is already an abstraction for SVGImageLoader and HTMLImageLoader (at
> least I think there is), maybe you need WML image loader instead of hackign
> into HTMLImageLoader?

Using new image loader for WML is big cost since its primary work is the same with HTMLImageloader. After all, WML Image is a HTML image. 


> So it seems like you're subclassing HTML elements in order to add WML-specific
> behavior?  Could CSS be used for any of this?  Subclasses don't need friend
> declarations.  Instead things should be moved to "protected" to allow
> subclasses to access them.

According to my original design principle, I don't want to touch WebCore's code too much, and try to reuse the HTML code. But what you said is right, I have modified them. 

> TorchMobile is the only copyright I see adding a URL. :)
> Why does WML need "Force reload"?
> ResourceLoader didRecieveData is the wrong place to hack in WBXML decoding,
> IMO.  I'm not sure what the right place is thought, but probably in whatever
> handles decoding of the main resource.  For example in sub-resources, the
> CachedResource subclasses all handle their own decoding in their add data
> methods.
I have move the WBXML decoding to MainResoureLoader. Maybe it's better.

> Why does "up" and "down" mean something on a WML page that's different from on
> any other page?  It seems that you're using ENABLE(WML) To mean
> PLATFORM(TORCH_MOBILE) in this case.
Since it's Platform specific, I have removed it.

> WMLBrwContext really needs a better name.  It's WMLPageData or something like
> that.  It's data you want to hang off of the page object which is WML specific.
>  It probably shouldn't be ref-counted, but should instead be OwnPtr on the
> Page, but I'm not sure about it's lifetime.
Has renamed it.

[Nikolas Zimmermann]

Comment on attachment 24709 [details]
WebCore touched by WML

Patch looks great, almost r+. Some minors issues:

DOMImplementation.cpp
> +
> +#if ENABLE(WML)
> +    if ((type == "text/vnd.wap.wml")
> +#if ENABLE(WBXML)
> +        || (type == "application/vnd.wap.wmlc")
> +#endif
> +        )

Extra parenthesis around (type == ..) are superflous. Please remove.

XMLTokenizer.h:
> +        Document *document() const { return m_doc; }
> +

Should read "Document* document()..."

HTMLSelectElement:
>  void HTMLSelectElement::setMultiple(bool multiple)
>  {
> +#if ENABLE(WML)
> +    m_multiple = multiple;
> +#else
>      setAttribute(multipleAttr, multiple ? "" : 0);
> +#endif
>  }

When enabling WML, this would affect HTML as well, is altering m_multiple really desired? When looking through the code, m_multiple is set through parseMappedAttribute, and in general most HTMLElement derived classes exposing setXXXValue / xxxValue functions are supposed to modify the corresponding attribute values through setAttribute. Alterin m_multiple violates this pattern. What's the reason?

MainResourceLoader.cpp:
> +#if ENABLE(WBXML)
> +    if (length && m_response.mimeType() == "application/vnd.wap.wmlc") {
> +        printf(">>>>>>>>>>>>>  decode wbxml document\n");
> +        printf(">>>>>>>>>>>>>  decode wbxml document\n");
> +        printf(">>>>>>>>>>>>>  decode wbxml document\n");
> +        WB_UTINY *wbxml = (WB_UTINY*)data;
> +        WB_ULONG wbxml_len = length;
> +        WB_ULONG xml_len = 0;
> +        WB_UTINY *xml = 0;
> +        WBXMLError ret = WBXML_OK; 
> +        WBXMLGenXMLParams params;
> + 
> +        // Init Default Parameters
> +        params.lang = WBXML_LANG_UNKNOWN;
> +        params.gen_type = WBXML_GEN_XML_INDENT;
> +        params.indent = 1;
> +        params.keep_ignorable_ws = FALSE;
> + 
> +        ret = wbxml_conv_wbxml2xml_withlen(wbxml, wbxml_len, &xml, &xml_len, &params);
> +        if (ret != WBXML_OK)
> +            return;
> +        data = (char*)xml;
> +        length = xml_len;
> +    }
> +#endif
That looks hackish overall. Please move stars -> "WB_UTINY *wbxml" -> "WB_UTINY* wbxml".
C style casts should be changed to static/reinterpret cast.

Anything else looks sane!

[Nikolas Zimmermann]

Created attachment 25098 [details]
WebCore touched by WML (v2)

Fixed all issues I outlined in the previous comment. r?

[yichao.yin]

Thank zimmermann for your comments.


(In reply to comment #26)
> HTMLSelectElement:
> >  void HTMLSelectElement::setMultiple(bool multiple)
> >  {
> > +#if ENABLE(WML)
> > +    m_multiple = multiple;
> > +#else
> >      setAttribute(multipleAttr, multiple ? "" : 0);
> > +#endif
> >  }
> When enabling WML, this would affect HTML as well, is altering m_multiple
> really desired? When looking through the code, m_multiple is set through
> parseMappedAttribute, and in general most HTMLElement derived classes exposing
> setXXXValue / xxxValue functions are supposed to modify the corresponding
> attribute values through setAttribute. Alterin m_multiple violates this
> pattern. What's the reason?

* Yes, we'd better set value of class member with setXXXX() method by convetion.
In some cases WMLSelectElement needs to update the HTMLSelectElement::m_multiple but HTMLSelectElement::setMultiple() doesn't support it. I think we needn't to add new method for HTMLSelectElement and WMLSelectElement to implement that. 
Of cource, you are right. we should  not affect HTML while WML is enabled. I have fixed it in my new patch.


> MainResourceLoader.cpp:
> > +#if ENABLE(WBXML)
> > +    if (length && m_response.mimeType() == "application/vnd.wap.wmlc") {
> > +        WB_UTINY *wbxml = (WB_UTINY*)data;
> > +        WB_ULONG wbxml_len = length;
> > +        WB_ULONG xml_len = 0;
> > +        WB_UTINY *xml = 0;
> > +        WBXMLError ret = WBXML_OK; 
> > +        WBXMLGenXMLParams params;
> > + 
> > +        // Init Default Parameters
> > +        params.lang = WBXML_LANG_UNKNOWN;
> > +        params.gen_type = WBXML_GEN_XML_INDENT;
> > +        params.indent = 1;
> > +        params.keep_ignorable_ws = FALSE;
> > + 
> > +        ret = wbxml_conv_wbxml2xml_withlen(wbxml, wbxml_len, &xml, &xml_len, &params);
> > +        if (ret != WBXML_OK)
> > +            return;
> > +        data = (char*)xml;
> > +        length = xml_len;
> > +    }
> > +#endif
> That looks hackish overall. Please move stars -> "WB_UTINY *wbxml" ->
> "WB_UTINY* wbxml".
> C style casts should be changed to static/reinterpret cast.

* Thanks for your advice and new patch. I did slight change for your update
about this issue because (const char*)data can't be casted to WB_UTINY* directly.

[yichao.yin]

Created attachment 25165 [details]
WebCore touched by WML(v3)

This mew patch includes Zimmermann's update and the fixes for all the issues the comments #26 mentioned.

[yichao.yin]

Created attachment 25166 [details]
WML module (v2)

Because EventNames namespace has been discarded in the version 38361 of Webkit, WML module needs updating.

[yichao.yin]

Created attachment 25167 [details]
The whole submit for Adding WML (v2)

this patch includes all the changes for adding WML to webkit(25165, 25166 and layouttests), which is generated based on the revision: 38361

[yichao.yin]

Created attachment 25168 [details]
Layout test cases for wml

layout test cases for WML. it adds some new test cases and resoure files comparing with the last version

[yichao.yin]

(In reply to comment #31)
> Created an attachment (id=25167) [review]
> The whole submit for Adding WML (v2)
> this patch includes all the changes for adding WML to webkit(25165, 25166 and
> layouttests), which is generated based on the revision: 38361

correction: patch "The whole submit for Adding WML (v2)" doesn't contain the layout test cases.

[Nikolas Zimmermann]

Hi Yichao,

I'm working on a whole new revision of this patch, actually working in non-Qt environments, most noticeable: Mac/Cocoa. WMLTokenizer is tied to Qt at the moment, I'll fix that.
 
> * Yes, we'd better set value of class member with setXXXX() method by
> convetion.
> In some cases WMLSelectElement needs to update the
> HTMLSelectElement::m_multiple but HTMLSelectElement::setMultiple() doesn't
> support it. I think we needn't to add new method for HTMLSelectElement and
> WMLSelectElement to implement that. 
> Of cource, you are right. we should  not affect HTML while WML is enabled. I
> have fixed it in my new patch.

I think you don't need to change setMultiple in any way, because the setAttribute() call will immediately call parseMappedAttribute, which takes care of setting m_multiple to the correct value.
 
> * Thanks for your advice and new patch. I did slight change for your update
> about this issue because (const char*)data can't be casted to WB_UTINY*
> directly.

Great, I'll take that into account.

[yichao.yin]

(In reply to comment #34)
> Hi Yichao,
> I'm working on a whole new revision of this patch, actually working in non-Qt
> environments, most noticeable: Mac/Cocoa. WMLTokenizer is tied to Qt at the
> moment, I'll fix that.

* Thanks zimmermann! Yes, what I submitted this time is just for Qt. Actually, For Mac, we are using libxml2 to parse the WML document. The related code is in XMLTokenizerLibxml2.cpp. Because I have no the Mac envrionment, George has ever planned to verify the code. But I guess he is too busy to do that. Anyway, I will upload the patch to bugzilla for your reivew later.

> > * Yes, we'd better set value of class member with setXXXX() method by
> > convetion.
> > In some cases WMLSelectElement needs to update the
> > HTMLSelectElement::m_multiple but HTMLSelectElement::setMultiple() doesn't
> > support it. I think we needn't to add new method for HTMLSelectElement and
> > WMLSelectElement to implement that. 
> > Of cource, you are right. we should  not affect HTML while WML is enabled. I
> > have fixed it in my new patch.
> I think you don't need to change setMultiple in any way, because the
> setAttribute() call will immediately call parseMappedAttribute, which takes
> care of setting m_multiple to the correct value.

* You are right. But WMLSelectElement::parseMappedAttribute can't update the m_multiple directly because it is private. thus we have to make m_multiple protected or let HTMLSelectElement class to offer new method setXXXX to let its subclass to set m_multiple. Obviously, the latter is better. but it't not easy to name the new method properly, you know, setMultiple() exists and it's supposed to be used to update the m_multiple member of HTMLSelectElement class in general. That's why I decide to hack it in HTMLSelectElement::setMultple() for WMLSelectElement. How do you think about it?

[yichao.yin]

Created attachment 25205 [details]
XMLTokenizerLibxml2.patch

this xml tokenizer is used to parse WML document with libxml2 for non-Qt platform

[Nikolas Zimmermann]

(In reply to comment #35)
> (In reply to comment #34)
> > Hi Yichao,
> > I'm working on a whole new revision of this patch, actually working in non-Qt
> > environments, most noticeable: Mac/Cocoa. WMLTokenizer is tied to Qt at the
> > moment, I'll fix that.
> 
> * Thanks zimmermann! Yes, what I submitted this time is just for Qt. Actually,
> For Mac, we are using libxml2 to parse the WML document. The related code is in
> XMLTokenizerLibxml2.cpp. Because I have no the Mac envrionment, George has ever
> planned to verify the code. But I guess he is too busy to do that. Anyway, I
> will upload the patch to bugzilla for your reivew later.
> 
> > > * Yes, we'd better set value of class member with setXXXX() method by
> > > convetion.
> > > In some cases WMLSelectElement needs to update the
> > > HTMLSelectElement::m_multiple but HTMLSelectElement::setMultiple() doesn't
> > > support it. I think we needn't to add new method for HTMLSelectElement and
> > > WMLSelectElement to implement that. 
> > > Of cource, you are right. we should  not affect HTML while WML is enabled. I
> > > have fixed it in my new patch.
> > I think you don't need to change setMultiple in any way, because the
> > setAttribute() call will immediately call parseMappedAttribute, which takes
> > care of setting m_multiple to the correct value.
> 
> * You are right. But WMLSelectElement::parseMappedAttribute can't update the
> m_multiple directly because it is private. thus we have to make m_multiple
> protected or let HTMLSelectElement class to offer new method setXXXX to let its
> subclass to set m_multiple. Obviously, the latter is better. but it't not easy
> to name the new method properly, you know, setMultiple() exists and it's
> supposed to be used to update the m_multiple member of HTMLSelectElement class
> in general. That's why I decide to hack it in HTMLSelectElement::setMultple()
> for WMLSelectElement. How do you think about it?

Hi Yichao,

as we discussed in private mails, the idea is that WMLSelectElement doesn't inherit from HTMLSelectElement at all, but instead we should have a common SelectElement base-class for both of them. That'll resolve all these problems, patch upcoming soon.

Greetings,
Niko

[Nikolas Zimmermann]

Cleared all review flaqgs. The WML support will be added in chunks, to ease reviewing.
WMLTokenizer shouldn't be needed and the integration within WebCore can be done much better - as has been discussed in private mails with Yichao & George.

The patch "The whole submit for Adding WML (v2)" will be converted to the new design, step-by-step. The first bug covering this work is 22324. It adds the new design (not relying on a special WMLTokenizer) and some WML elements (card/p/a). More patches following after 22324 landed.

[yichao.yin]

(In reply to comment #37)
> (In reply to comment #35)
> > (In reply to comment #34)
> > > Hi Yichao,
> > > I'm working on a whole new revision of this patch, actually working in non-Qt
> > > environments, most noticeable: Mac/Cocoa. WMLTokenizer is tied to Qt at the
> > > moment, I'll fix that.
> > 
> > * Thanks zimmermann! Yes, what I submitted this time is just for Qt. Actually,
> > For Mac, we are using libxml2 to parse the WML document. The related code is in
> > XMLTokenizerLibxml2.cpp. Because I have no the Mac envrionment, George has ever
> > planned to verify the code. But I guess he is too busy to do that. Anyway, I
> > will upload the patch to bugzilla for your reivew later.
> > 
> > > > * Yes, we'd better set value of class member with setXXXX() method by
> > > > convetion.
> > > > In some cases WMLSelectElement needs to update the
> > > > HTMLSelectElement::m_multiple but HTMLSelectElement::setMultiple() doesn't
> > > > support it. I think we needn't to add new method for HTMLSelectElement and
> > > > WMLSelectElement to implement that. 
> > > > Of cource, you are right. we should  not affect HTML while WML is enabled. I
> > > > have fixed it in my new patch.
> > > I think you don't need to change setMultiple in any way, because the
> > > setAttribute() call will immediately call parseMappedAttribute, which takes
> > > care of setting m_multiple to the correct value.
> > 
> > * You are right. But WMLSelectElement::parseMappedAttribute can't update the
> > m_multiple directly because it is private. thus we have to make m_multiple
> > protected or let HTMLSelectElement class to offer new method setXXXX to let its
> > subclass to set m_multiple. Obviously, the latter is better. but it't not easy
> > to name the new method properly, you know, setMultiple() exists and it's
> > supposed to be used to update the m_multiple member of HTMLSelectElement class
> > in general. That's why I decide to hack it in HTMLSelectElement::setMultple()
> > for WMLSelectElement. How do you think about it?
> Hi Yichao,
> as we discussed in private mails, the idea is that WMLSelectElement doesn't
> inherit from HTMLSelectElement at all, but instead we should have a common
> SelectElement base-class for both of them. That'll resolve all these problems,
> patch upcoming soon.
> Greetings,
> Niko

Hi Niko,
Yes, this problem won't appear for the new design any more.

[Nikolas Zimmermann]

Marked all but two patches as obsolete.
'The whole submit for Adding WML (v2)' should be left as is, until I've completed the merge/rewrite.
'XMLTokenizerLibxml2.patch' is a reminder, that we don't bail on a missing doctype at the moment, not yet sure if we really want that.

[zaheer]

wbxml_conv_wbxml2xml_withlen does not support chunked incremetal decoding (pls correct me). If this is true, invoking this from addData where the data is received incrementally may be incorrect. 

[Arun Patole]

Guys, I would like to know if the stable basic wml support is now available with the latest webkit revision as I still see the status of this issue as "NEW".

Thanks in advance,
Arun

[yichao.yin]

(In reply to comment #41)
> wbxml_conv_wbxml2xml_withlen does not support chunked incremetal decoding (pls
> correct me). If this is true, invoking this from addData where the data is
> received incrementally may be incorrect. 

Yeah, actually, this issue has already been fixed in my private branch. But it's in discussion whether to integrate wbxml2 into WebKit. The support for wbxml is not included in the latest implementation of WML.

[yichao.yin]

(In reply to comment #42)
> Guys, I would like to know if the stable basic wml support is now available
> with the latest webkit revision as I still see the status of this issue as
> "NEW".
> Thanks in advance,
> Arun

Yes, the basic wml implementation, reimplemented by Niko, has already entered Webkit repository.

[Nikolas Zimmermann]

Comment on attachment 25205 [details]
XMLTokenizerLibxml2.patch

Cleared all patches on this bug. Most of the original code has been rewritten and landed over the past months. The remaining work happens in the child-bugs of this master WML bug.

Maybe add dependency on the following build failures?

https://bugs.webkit.org/show_bug.cgi?id=45956
https://bugs.webkit.org/show_bug.cgi?id=42943

[Adam Barth]

WebKit has WML support.

[Laszlo Gombos]

WML support is removed - http://trac.webkit.org/changeset/85256.