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

[bugzilla-daemon at webkit.org]

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


eric at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #24139|review?                     |review-
               Flag|                            |




------- Comment #21 from eric at webkit.org  2008-10-06 23:42 PDT -------
(From update of attachment 24139)
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.


-- 
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.