[Webkit-unassigned] [Bug 23452] Add XHTMLMP support to Webkit
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Feb 11 06:31:15 PST 2009
https://bugs.webkit.org/show_bug.cgi?id=23452
------- Comment #13 from darin at apple.com 2009-02-11 06:31 PDT -------
(In reply to comment #8)
> > > +#if ENABLE(XHTMLMP)
> > > + if (settings())
> > > + m_shouldProcessNoScriptElement = !settings()->isJavaScriptEnabled();
> > > +#endif
> > Member initialization is preferred over assignment. Also, this seems to leave
> > m_shouldProcessNoScriptElement uninitialized if settings() is 0. Also, when can
> > settings() be 0? I don't think this is correct.
>
> I am not sure when the settings() will be 0. but I think we'd better use
> ASSERT(settings()) to ensure save.
> will remove the if segment to make sure m_shouldProcessNoScriptElement is
> initialized. is that ok?
As I said in my review, I don't think the assert is valuable. If we need a null
check, we can include one, but if not we don't want to assert the pointer. It
doesn't really add much.
When I said "member initialization", I meant this syntax:
, m_shouldProcessNoScriptElement(xxx)
As opposed to assignment.
> As for the MIMEType of "application/xhtml+xml" support is for following the
> user agent conformance
> requirement stated in the section 7.2 of specificatoin
> OMA-WAP-XHTMLMP-V1_1-20061020-A.pdf.
> This method is just used to judge what document is XHTMLMP document.
I don't get it. How can the same browser support both XHTML and XHTMLMP then? I
think this is highly suspect.
> Sorry, I am not sure how to name the error is better. how about this one:
> "DOCTYPE declaration lack." ?
Maybe someone else, perhaps Hyatt, can help you with this naming.
> > The check before casting scriptElement to HTMLScriptElement* is a little bit
> > non-obvious. I think that function should be on ScriptElement not on
> > HTMLScriptElement. It can simply return false for non-HTML script elements.
> > That's more efficient than what you're doing here and also safer because it
> > doesn't require a cast.
>
> sounds better. but if that we have to change the code of ScriptElement and
> SVGScriptElement.
Yes.
> I think it's not very good for this patch to touch much none-XHTMLMP code in
> the WebCore. No?
I understand your desire to not touch other code, but that's not a good
strategy long term. We shouldn't add things without considering the overall
design. The change I suggested has very little, or no, cost.
I think it's absolutely fine to touch the non-XHTMLMP code if it makes things
cleaner. Ideally, though, we should do that in a separate small patch first to
prepare, and not do it as part of a large XHTMLMP patch.
--
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