Demistify DocumentParser::append

Source/core/dom/DocumentParser.h

 /*
  * Copyright (C) 2000 Peter Kelly (pmk@post.com)
  * Copyright (C) 2005, 2006 Apple Computer, Inc.
  * Copyright (C) 2007 Samuel Weinig (sam@webkit.org)
  * Copyright (C) 2010 Google, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
@@ skipping 38 matching lines @@
     // insert is used by document.write.
     virtual void insert(const SegmentedString&) = 0;
 
     // The below functions are used by DocumentWriter (the loader).
     virtual void appendBytes(const char* bytes, size_t length) = 0;
     virtual bool needsDecoder() const { return false; }
     virtual void setDecoder(PassOwnPtr<TextResourceDecoder>);
     virtual TextResourceDecoder* decoder();
     virtual void setHasAppendedData() { }
 
-    // FIXME: append() should be private, but DocumentLoader::replaceDocumentWhileExecutingJavaScriptURL uses it for now.

[kouhei (in TOK), 2014/12/18 04:34:35]

Why should this FIXME be removed?

[kbalazs, 2014/12/18 23:28:28]

I answered that in the changelog :)

"Also remove outdated comment that it should not be public. Both DocumentWriter
and DOMPatchSupport uses it and there is nothing particularly wrong with that."

So first of all the comment says it's only used by DocumentWriter but in fact
it's used by DOMPatchsSupport as well. Second now that append is only used on
sync path I think there is nothing wrong with it.

[kouhei (in TOK), 2014/12/19 04:44:27]

I think this comment is still relevant. So would you keep it for this CL?

DocumentParser has 3 methods (appendBytes, append, insert) to add content, so
I'm a bit concerned about the current state. I know that each method does things
a bit differently, but I feel this is overly complicated and hard to understand.
DOMPatchSupport and DocumentWriter should really be using appendBytes here.

-    // FIXME: This really should take a PassOwnPtr to signify that it expects to take
-    // ownership of the buffer. The parser expects the PassRefPtr to hold the only ref of the StringImpl.

[kouhei (in TOK), 2014/12/18 04:34:35]

I'm ok with removing this FIXME.

-    virtual void append(PassRefPtr<StringImpl>) = 0;
+    virtual void append(const String&) = 0;
 
     virtual void finish() = 0;
 
     // FIXME: processingData() is only used by DocumentLoader::isLoadingInAPISense
     // and is very unclear as to what it actually means.  The LegacyHTMLDocumentParser
     // used to implement it.
     virtual bool processingData() const { return false; }
 
     // document() will return 0 after detach() is called.
     Document* document() const { ASSERT(m_document); return m_document; }
@@ skipping 49 matching lines @@
     // Every DocumentParser needs a pointer back to the document.
     // m_document will be 0 after the parser is stopped.
     RawPtrWillBeMember<Document> m_document;
 
     WillBeHeapHashSet<RawPtrWillBeWeakMember<DocumentParserClient> > m_clients;
 };
 
 } // namespace blink
 
 #endif // DocumentParser_h