[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