Bug 140041

Summary: Modernize and tighten up HTMLDocumentParser
Product: WebKit Reporter: Darin Adler <darin>
Component: Page LoadingAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, esprehn+autocc, gyuyoung.kim, kangil.han, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews103 for mac-mountainlion
none
Patch sam: review+

Darin Adler
Reported 2015-01-02 09:48:53 PST
Modernize and tighten up HTMLDocumentParser
Attachments
Patch (52.63 KB, patch)
2015-01-02 10:08 PST, Darin Adler
no flags
Patch (47.76 KB, patch)
2015-01-02 10:19 PST, Darin Adler
no flags
Archive of layout-test-results from ews103 for mac-mountainlion (512.42 KB, application/zip)
2015-01-02 11:06 PST, Build Bot
no flags
Patch (47.55 KB, patch)
2015-01-04 11:51 PST, Darin Adler
sam: review+
Darin Adler
Comment 1 2015-01-02 10:08:40 PST
Darin Adler
Comment 2 2015-01-02 10:19:28 PST
Build Bot
Comment 3 2015-01-02 11:06:07 PST
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
Build Bot
Comment 4 2015-01-02 11:06:12 PST
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
Darin Adler
Comment 5 2015-01-04 11:51:34 PST
Darin Adler
Comment 6 2015-01-04 16:23:11 PST
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.
Sam Weinig
Comment 7 2015-01-04 17:30:19 PST
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.
Darin Adler
Comment 8 2015-01-04 18:22:29 PST
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.
Darin Adler
Comment 9 2015-01-04 18:24:32 PST
Darin Adler
Comment 10 2015-01-04 21:07:37 PST
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.
Alexey Proskuryakov
Comment 11 2015-01-05 09:58:59 PST
Alexey Proskuryakov
Comment 12 2015-01-05 10:01:47 PST
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.
Darin Adler
Comment 13 2015-01-05 10:06:23 PST
(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.
Alexey Proskuryakov
Comment 14 2015-01-05 10:16:30 PST
Darin Adler
Comment 15 2015-01-05 23:46:37 PST
Fixed the regression and reenabled the test in <http://trac.webkit.org/r177951>.
Note You need to log in before you can comment on or make changes to this bug.