[Webkit-unassigned] [Bug 20393] Add WML support to WebKit
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Sep 26 08:13:22 PDT 2008
https://bugs.webkit.org/show_bug.cgi?id=20393
eric at webkit.org changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #23850|review? |review-
Flag| |
------- Comment #15 from eric at webkit.org 2008-09-26 08:13 PDT -------
(From update of attachment 23850)
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.
--
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.
More information about the webkit-unassigned
mailing list