Bug 140041 – Modernize and tighten up HTMLDocumentParser

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Darin Adler]

Modernize and tighten up HTMLDocumentParser

[Darin Adler]

Created attachment 243897 [details]
Patch

[Darin Adler]

Created attachment 243899 [details]
Patch

[Build Bot]

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]

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]

Created attachment 243933 [details]
Patch

[Darin Adler]

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 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 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]

Committed r177883: <http://trac.webkit.org/changeset/177883>

[Darin Adler]

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]

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

[Alexey Proskuryakov]

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]

(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]

Skipped it in <http://trac.webkit.org/r177913>.

[Darin Adler]

Fixed the regression and reenabled the test in <http://trac.webkit.org/r177951>.