Bug 64305

Summary: Move XML errors handling out from XMLDocumentParser (refactoring).
Product: WebKit Reporter: Vsevolod Vlasov <vsevik>
Component: XMLAssignee: Vsevolod Vlasov <vsevik>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, jeffrey+webkit
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 55233    
Attachments:
Description Flags
Patch
none
Patch (comments addressed + qt build fix) abarth: review+

Vsevolod Vlasov
Reported 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).
Attachments
Patch (33.08 KB, patch)
2011-07-11 12:33 PDT, Vsevolod Vlasov 		no flags
DetailsFormatted DiffDiff
Patch (comments addressed + qt build fix) (33.76 KB, patch)
2011-07-11 13:09 PDT, Vsevolod Vlasov 		abarth: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Vsevolod Vlasov
Comment 1 2011-07-11 12:33:03 PDT
Created attachment 100350 [details] Patch
Adam Barth
Comment 2 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.
Vsevolod Vlasov
Comment 3 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.
Vsevolod Vlasov
Comment 4 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.
Early Warning System Bot
Comment 5 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
Vsevolod Vlasov
Comment 6 2011-07-11 13:09:39 PDT
Created attachment 100354 [details] Patch (comments addressed + qt build fix)
Adam Barth
Comment 7 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.)
Adam Barth
Comment 8 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.
Vsevolod Vlasov
Comment 9 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)
Vsevolod Vlasov
Comment 10 2011-07-14 10:15:45 PDT
Committed r91008: <http://trac.webkit.org/changeset/91008>
Note You need to log in before you can comment on or make changes to this bug.