Bug 64305 - Move XML errors handling out from XMLDocumentParser (refactoring).
Summary: Move XML errors handling out from XMLDocumentParser (refactoring).
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Vsevolod Vlasov
URL:
Keywords:
Depends on:
Blocks: 55233
  Show dependency treegraph
 
Reported: 2011-07-11 12:19 PDT by Vsevolod Vlasov
Modified: 2011-07-14 10:15 PDT (History)
2 users (show)

See Also:


Attachments
Patch (33.08 KB, patch)
2011-07-11 12:33 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (comments addressed + qt build fix) (33.76 KB, patch)
2011-07-11 13:09 PDT, Vsevolod Vlasov
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vsevolod Vlasov 2011-07-11 12:19:02 PDT
Move XML errors handling out from XMLDocumentParser.

This is a part of http://webk.it/55233 (XSL transformations do not handle errors).
Comment 1 Vsevolod Vlasov 2011-07-11 12:33:03 PDT
Created attachment 100350 [details]
Patch
Comment 2 Adam Barth 2011-07-11 12:39:12 PDT
Comment on attachment 100350 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100350&action=review

Sounds like a good idea.  I didn't verify that the moved code was identical, but I did find at least one difference, which makes me feel like I need to look more carefully in order to review the patch thoroughly.

> Source/WebCore/xml/XMLErrors.cpp:72
> +        case tip:
> +            break;

Where did this case come from?  It doesn't appear to have existed before.
Comment 3 Vsevolod Vlasov 2011-07-11 12:44:15 PDT
> > Source/WebCore/xml/XMLErrors.cpp:72
> > +        case tip:
> > +            break;
> 
> Where did this case come from?  It doesn't appear to have existed before.
I just added that for myself and forgot to remove, thanks for spotting it.

I'll fix that together with other comments if any.
Comment 4 Vsevolod Vlasov 2011-07-11 12:48:36 PDT
Actually this "tip" type is coming from libxml2 and will be used later (as it is used currently currently for XSLT error handling), but this is out of scope of this bug.
Comment 5 Early Warning System Bot 2011-07-11 12:55:41 PDT
Comment on attachment 100350 [details]
Patch

Attachment 100350 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9010580
Comment 6 Vsevolod Vlasov 2011-07-11 13:09:39 PDT
Created attachment 100354 [details]
Patch (comments addressed + qt build fix)
Comment 7 Adam Barth 2011-07-13 15:01:52 PDT
Comment on attachment 100354 [details]
Patch (comments addressed + qt build fix)

View in context: https://bugs.webkit.org/attachment.cgi?id=100354&action=review

> Source/WebCore/xml/XMLErrors.cpp:85
> +    RefPtr<Element> h3 = doc->createElement(h3Tag, false);
> +    reportElement->appendChild(h3.get(), ec);

Can there be mutation event registered in doc to screw us over?

> Source/WebCore/xml/XMLErrors.h:59
> +    String m_errorMessages;

Consider using StringBuilder instead.  (See Darin's recent post on webkit-dev.)
Comment 8 Adam Barth 2011-07-13 15:02:16 PDT
Neither of those things are your fault, but the StringBuilder one should be easy to fix while you're here.
Comment 9 Vsevolod Vlasov 2011-07-14 08:52:13 PDT
I'll fix StringBuilder thing before committing.

I have also filed https://bugs.webkit.org/show_bug.cgi?id=64532 for the other problem (with an example where it fails)
Comment 10 Vsevolod Vlasov 2011-07-14 10:15:45 PDT
Committed r91008: <http://trac.webkit.org/changeset/91008>