Modernize and tighten up HTMLDocumentParser
Created attachment 243897 [details] Patch
Created attachment 243899 [details] Patch
Comment on attachment 243899 [details] Patch Attachment 243899 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6353429383348224 New failing tests: editing/pasteboard/drag-and-drop-objectimage-contenteditable.html
Created attachment 243902 [details] Archive of layout-test-results from ews103 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 243933 [details] Patch
I think this patch truly is now ready for review with no failures on EWS. The red bubble on Mac seems to be due to some problem with the Mac EWS bot, not a problem with this patch.
Comment on attachment 243933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243933&action=review > Source/WebCore/ChangeLog:12 > + * html/FTPDirectoryDocument.cpp: Removed unneeded includes, made more We should probably get rid of FTP directory support at some point. I don't believe we are using it anywhere. > Source/WebCore/dom/DocumentFragment.cpp:86 > + ASSERT(contextElement); > + HTMLDocumentParser::parseDocumentFragment(source, *this, *contextElement, parserContentPolicy); What makes this assertion correct? From looking at callers of this (of which there are only a few), this seems safe, but I think it would be better to pass contextElement in by reference to here. > Source/WebCore/dom/DocumentFragment.cpp:91 > + ASSERT(contextElement); Same thoughts as above.
Comment on attachment 243933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243933&action=review >> Source/WebCore/ChangeLog:12 >> + * html/FTPDirectoryDocument.cpp: Removed unneeded includes, made more > > We should probably get rid of FTP directory support at some point. I don't believe we are using it anywhere. Good idea. Why didn’t I think of that? >> Source/WebCore/dom/DocumentFragment.cpp:86 >> + HTMLDocumentParser::parseDocumentFragment(source, *this, *contextElement, parserContentPolicy); > > What makes this assertion correct? From looking at callers of this (of which there are only a few), this seems safe, but I think it would be better to pass contextElement in by reference to here. I completely agree. I just tried to hold back and not do all the refactoring in one go. >> Source/WebCore/dom/DocumentFragment.cpp:91 >> + ASSERT(contextElement); > > Same thoughts as above. Yes. Will do.
Committed r177883: <http://trac.webkit.org/changeset/177883>
Comment on attachment 243933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243933&action=review >>> Source/WebCore/dom/DocumentFragment.cpp:86 >>> + HTMLDocumentParser::parseDocumentFragment(source, *this, *contextElement, parserContentPolicy); >> >> What makes this assertion correct? From looking at callers of this (of which there are only a few), this seems safe, but I think it would be better to pass contextElement in by reference to here. > > I completely agree. I just tried to hold back and not do all the refactoring in one go. Turns out this one is correct and I changed the argument to Element&. >>> Source/WebCore/dom/DocumentFragment.cpp:91 >>> + ASSERT(contextElement); >> >> Same thoughts as above. > > Yes. Will do. But this assertion is not correct, and I removed it.
This change made editing/pasteboard/drag-and-drop-objectimage-contenteditable.html time out: https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=editing%2Fpasteboard%2Fdrag-and-drop-objectimage-contenteditable.html
EWS warned about this for the previous version of the patch, but it failed to build the latest one because of some kind of gperf failure on WebCore/css/makeprop.pl. These build failures happen frequently, I wonder what's up with them.
(In reply to comment #11) > This change made > editing/pasteboard/drag-and-drop-objectimage-contenteditable.html time out: OK. I’ll tackle that this evening. Would you be willing to put it on Skip for me until I get to it? I’ll make it my top programming priority, so should be done in a day or two at most.
Skipped it in <http://trac.webkit.org/r177913>.
Fixed the regression and reenabled the test in <http://trac.webkit.org/r177951>.