[Webkit-unassigned] [Bug 20393] Add WML support to WebKit

[bugzilla-daemon at webkit.org]

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





------- Comment #25 from yichao.yin at torchmobile.com.cn  2008-10-28 04:26 PDT -------
(In reply to comment #21)
> (From update of attachment 24139 [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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.