Bug 41117 – Move Document* down onto DocumentParser, since every DocumentParser needs one.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

Move Document* down onto DocumentParser, since every DocumentParser needs one.

[Eric Seidel (no email)]

Created attachment 59589 [details]
Patch

[WebKit Review Bot]

Attachment 59589 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/dom/DocumentParser.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 59589 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3301694

[Adam Barth]

Comment on attachment 59589 [details]
Patch

WebCore/dom/DocumentParser.h:32
 +      class Document;
You should fix the indent here while you're mucking with things.

WebCore/dom/XMLDocumentParserLibxml2.cpp:525
 +  XMLDocumentParser::XMLDocumentParser(Document* _doc, FrameView* _view)
Can you fix these parameter names too?

WebCore/dom/XMLDocumentParserLibxml2.cpp:578
 +      // FIXME: The Document owns the Parser, we shouldn't need to ref the Document.
Please remove this in a follow-up patch.

WebCore/loader/ImageDocument.cpp:97
 +          return static_cast<ImageDocument*>(document());
Yuck.

WebCore/loader/PluginDocument.cpp:54
 +      }
Missing blank line after this line.

WebCore/loader/SinkDocument.cpp:35
 +      SinkDocumentParser(Document* document) : DocumentParser(document) { }
Please split into multiple lines for consistency.

[Darin Adler]

Comment on attachment 59589 [details]
Patch

> +// If move DocumentParser() to a DocumentParser.cpp, this include can go away.
> +#include <wtf/Assertions.h>

I think this comment is overkill.

> +    // FIXME: The Document owns the Parser, we shouldn't need to ref the Document.
> +    m_document->ref();

This comment seems oversimplified. Sure, the document owns the parser, but if the document is destroyed the parser may be abruptly destroyed, and there may be some need to postpone that. There are times when something owned, like the FrameLoader, may ref something that owns it, like the Frame. This could have been one of those times. On the other hand the code could well be wrong, but it's not sufficient explanation to say that the document owns the parser. Because of that, I think this comment is misleading.

> +    // FIXME: We should not need to ref/deref the Document, even when parsing fragments.
> +    if (m_parsingFragment)
> +        document()->deref();

Are you sure about this?

>      ImageDocument* m_doc;
>  };
>      
> -class ImageTokenizer : public DocumentParser {
> +class ImageDocumentParser : public DocumentParser {
>  public:
> -    ImageTokenizer(ImageDocument* doc) : m_doc(doc) {}
> +    ImageDocumentParser(ImageDocument* doc)
> +        : DocumentParser(doc)
> +    {
> +    }

If we're going to rename this class, then I suggest we also rename the abbreviated argument name.

> +    ImageDocument* imageDocument() const
> +    {
> +        return static_cast<ImageDocument*>(document());
> +    }

I suggest naming this just "document()", and calling through to DocumentParser::document(). There's no reason to call the underlying document() function directly. It's nice to call the function by its normal name and get the more specific type.

> +    MediaDocumentParser(Document* doc)
> +        : DocumentParser(doc)

Again, if we are going so far as to reformat, then I'd like to rename "doc" to "document".

> +    PluginDocumentParser(Document* doc)
> +        : DocumentParser(doc)
> +        , m_embedElement(0)
> +    {
> +    }

And again.

[Eric Seidel (no email)]

(In reply to comment #4)
> (From update of attachment 59589 [details])
> WebCore/dom/DocumentParser.h:32
>  +      class Document;
> You should fix the indent here while you're mucking with things.

I intentionally left that for a separate patch. :)

> WebCore/dom/XMLDocumentParserLibxml2.cpp:525
>  +  XMLDocumentParser::XMLDocumentParser(Document* _doc, FrameView* _view)
> Can you fix these parameter names too?

Sure.

> WebCore/dom/XMLDocumentParserLibxml2.cpp:578
>  +      // FIXME: The Document owns the Parser, we shouldn't need to ref the Document.
> Please remove this in a follow-up patch.

Sure.  I didn't want to make things any more likely to break.

> WebCore/loader/ImageDocument.cpp:97
>  +          return static_cast<ImageDocument*>(document());
> Yuck.

Why is this yuck?  We know that document() is an ImageDocument() because we were constructed with one.  Would you like a comment to explain such?  I think it's better than the sub classes know what subclass the root storage is and have fooDocument() type-specific accessors, than that each subclass has to store their own type-specific pointer.

> WebCore/loader/PluginDocument.cpp:54
>  +      }
> Missing blank line after this line.

OK.

> WebCore/loader/SinkDocument.cpp:35
>  +      SinkDocumentParser(Document* document) : DocumentParser(document) { }
> Please split into multiple lines for consistency.

OK.

[Eric Seidel (no email)]

(In reply to comment #5)
> (From update of attachment 59589 [details])
> > +// If move DocumentParser() to a DocumentParser.cpp, this include can go away.
> > +#include <wtf/Assertions.h>
> 
> I think this comment is overkill.

OK, will remove.  I just hate adding more headers to common header includes.

> > +    // FIXME: The Document owns the Parser, we shouldn't need to ref the Document.
> > +    m_document->ref();
> 
> This comment seems oversimplified. Sure, the document owns the parser, but if the document is destroyed the parser may be abruptly destroyed, and there may be some need to postpone that. There are times when something owned, like the FrameLoader, may ref something that owns it, like the Frame. This could have been one of those times. On the other hand the code could well be wrong, but it's not sufficient explanation to say that the document owns the parser. Because of that, I think this comment is misleading.

OK, I can explain better.  I still think the code is going to be wrong here.  In the fragment parsing case the Document* is the Document* owned by the Fragment, which is never destoryed during fragment parsing, so it makes no sense that it would need to ref the Doc.  But I could be missing something.

In the DocumentFragment parsing case the DocumentParser is on the stack, and the DocumentFrament* is passed to it (and not owned by it).  No script execution takes place during DocumentFragment parsing (which is synchronous).  I'm happy to explain all of this in a comment.

> > +    // FIXME: We should not need to ref/deref the Document, even when parsing fragments.
> > +    if (m_parsingFragment)
> > +        document()->deref();
> 
> Are you sure about this?

See above.

> > -class ImageTokenizer : public DocumentParser {
> > +class ImageDocumentParser : public DocumentParser {
> >  public:
> > -    ImageTokenizer(ImageDocument* doc) : m_doc(doc) {}
> > +    ImageDocumentParser(ImageDocument* doc)
> > +        : DocumentParser(doc)
> > +    {
> > +    }
> 
> If we're going to rename this class, then I suggest we also rename the abbreviated argument name.

You mean "doc"?  Happy to.

> > +    ImageDocument* imageDocument() const
> > +    {
> > +        return static_cast<ImageDocument*>(document());
> > +    }
> 
> I suggest naming this just "document()", and calling through to DocumentParser::document(). There's no reason to call the underlying document() function directly. It's nice to call the function by its normal name and get the more specific type.

Ok.  Sounds good to me.

> > +    MediaDocumentParser(Document* doc)
> > +        : DocumentParser(doc)
> 
> Again, if we are going so far as to reformat, then I'd like to rename "doc" to "document".
> 
> > +    PluginDocumentParser(Document* doc)
> > +        : DocumentParser(doc)
> > +        , m_embedElement(0)
> > +    {
> > +    }
> 
> And again.

[Adam Barth]

I'd be surprised if the ref()/deref() were needed.  However, I think we can skip the comment and just try removing them in a subsequent patch.

[Eric Seidel (no email)]

Created attachment 59613 [details]
Address Darin and Adam's feedback

[WebKit Review Bot]

Attachment 59613 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/dom/DocumentParser.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 59613 [details]
Address Darin and Adam's feedback

WebCore/dom/DocumentParser.h:27
 +  // If move DocumentParser() to a DocumentParser.cpp, this include can go away.
Darin asked you to remove this comment.

WebCore/dom/XMLDocumentParserLibxml2.cpp: 
 +          m_doc->ref();
Nice.

[Early Warning System Bot]

Attachment 59613 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3289635

[Eric Seidel (no email)]

Committed r61737: <http://trac.webkit.org/changeset/61737>

[WebKit Review Bot]

http://trac.webkit.org/changeset/61737 might have broken Qt Linux Release minimal and Qt Linux ARMv5 Release

[Eric Seidel (no email)]

Committed r61738: <http://trac.webkit.org/changeset/61738>

[Darin Adler]

Comment on attachment 59613 [details]
Address Darin and Adam's feedback

Can m_document be private instead of protected?

[Eric Seidel (no email)]

Yes, It could be.