Demistify DocumentParser::append

Demistify DocumentParser::append

Today it takes a PassRefptr<StringImpl> as argument. This supposed to express
that the input string is passed in a destructive manner. However this is no
longer the case. It was needed by the background parser but this code has been
refactored and now append is only used on the synchronous parsing path.

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

R=kouhei@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187561

[kouhei (in TOK), 2014-12-18 04:34:36]

https://codereview.chromium.org/799803004/diff/1/Source/core/dom/DocumentPars...
File Source/core/dom/DocumentParser.h (left):

https://codereview.chromium.org/799803004/diff/1/Source/core/dom/DocumentPars...
Source/core/dom/DocumentParser.h:59: // FIXME: append() should be private, but
DocumentLoader::replaceDocumentWhileExecutingJavaScriptURL uses it for now.
Why should this FIXME be removed?

https://codereview.chromium.org/799803004/diff/1/Source/core/dom/DocumentPars...
Source/core/dom/DocumentParser.h:61: // ownership of the buffer. The parser
expects the PassRefPtr to hold the only ref of the StringImpl.
I'm ok with removing this FIXME.

https://codereview.chromium.org/799803004/diff/1/Source/core/html/parser/HTML...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/799803004/diff/1/Source/core/html/parser/HTML...
Source/core/html/parser/HTMLDocumentParser.cpp:771: SegmentedString
source(inputSource);
Is it safe to reuse the SegmentedString for both preloadScanner and input
stream? This is a stateful class.

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

https://codereview.chromium.org/799803004/diff/1/Source/core/dom/DocumentPars...
File Source/core/dom/DocumentParser.h (left):

https://codereview.chromium.org/799803004/diff/1/Source/core/dom/DocumentPars...
Source/core/dom/DocumentParser.h:59: // FIXME: append() should be private, but
DocumentLoader::replaceDocumentWhileExecutingJavaScriptURL uses it for now.
On 2014/12/18 04:34:35, kouhei wrote:
> Why should this FIXME be removed?

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.

https://codereview.chromium.org/799803004/diff/1/Source/core/html/parser/HTML...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/799803004/diff/1/Source/core/html/parser/HTML...
Source/core/html/parser/HTMLDocumentParser.cpp:771: SegmentedString
source(inputSource);
On 2014/12/18 04:34:36, kouhei wrote:
> Is it safe to reuse the SegmentedString for both preloadScanner and input
> stream? This is a stateful class.

Both get's const reference argument so it should be. Maybe makes sense to make
the variable explicitly const.

[kbalazs, 2014-12-19 01:07:14]

fyi chromium trybot fail is unrelated

[kouhei (in TOK), 2014-12-19 04:44:28]

lgtm % removing the fixme.

https://codereview.chromium.org/799803004/diff/1/Source/core/dom/DocumentPars...
File Source/core/dom/DocumentParser.h (left):

https://codereview.chromium.org/799803004/diff/1/Source/core/dom/DocumentPars...
Source/core/dom/DocumentParser.h:59: // FIXME: append() should be private, but
DocumentLoader::replaceDocumentWhileExecutingJavaScriptURL uses it for now.
On 2014/12/18 23:28:28, kbalazs wrote:
> On 2014/12/18 04:34:35, kouhei wrote:
> > Why should this FIXME be removed?
> 
> 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.

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.

https://codereview.chromium.org/799803004/diff/1/Source/core/html/parser/HTML...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/799803004/diff/1/Source/core/html/parser/HTML...
Source/core/html/parser/HTMLDocumentParser.cpp:771: SegmentedString
source(inputSource);
On 2014/12/18 23:28:28, kbalazs wrote:
> On 2014/12/18 04:34:36, kouhei wrote:
> > Is it safe to reuse the SegmentedString for both preloadScanner and input
> > stream? This is a stateful class.
> 
> Both get's const reference argument so it should be. Maybe makes sense to make
> the variable explicitly const.

Thanks for clarification. Makes sense.

[kbalazs, 2014-12-19 18:10:40]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-12-19 18:11:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/799803004/40001

[commit-bot: I haz the power, 2014-12-19 20:47:32]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187561