[webkit-reviews] review denied: [Bug 20393] Add WML support to WebKit : [Attachment 24139] All the bits that integrate with existing code

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied George Staikos <staikos at kde.org>'s
request for review:
Bug 20393: Add WML support to WebKit
https://bugs.webkit.org/show_bug.cgi?id=20393

Attachment 24139: All the bits that integrate with existing code
https://bugs.webkit.org/attachment.cgi?id=24139&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.