Turn DocumentParser::pinToMainThread into a cleaner api

Turn DocumentParser::pinToMainThread into a cleaner api

pinToMainThread makes the false impression that it can happen any time. In reality this
has to be decided before the actual parsing is started. Better to express it with a cleaner api.

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

[kbalazs, 2014-12-05 23:24:58]

b.kelemen@samsung.com changed reviewers:
+ japhet@chromium.org, kouhei@chromium.org

[kbalazs, 2014-12-05 23:37:58]

https://codereview.chromium.org/779393002/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (left):

https://codereview.chromium.org/779393002/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:1240: m_parser->pinToMainThread();
Note that this called XMLDocumentParser::pinToMainThread which is noop.

About the yeilding mentioned in the comment that does not happen for synchronous
parsing which is enforced by using open() instead of implicitOpen(). And this
should not use insert for sure because insert is only for document.write.

https://codereview.chromium.org/779393002/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/779393002/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:2334: forceSynchronousParsing();
This is in open() (not obvious from the context).

https://codereview.chromium.org/779393002/diff/1/Source/core/loader/DocumentW...
File Source/core/loader/DocumentWriter.cpp (right):

https://codereview.chromium.org/779393002/diff/1/Source/core/loader/DocumentW...
Source/core/loader/DocumentWriter.cpp:63: m_parser = m_document->implicitOpen();
moved out from initializer list because forceSyncParse has to be called on the
document before.

[kbalazs, 2014-12-05 23:42:04]

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

https://codereview.chromium.org/779393002/diff/1/Source/core/html/parser/HTML...
Source/core/html/parser/HTMLDocumentParser.cpp:804: // from insert() so that
only document.write() uses insert.
This comment was pretty confusing also. No one else should use insert other than
document.write. I don't think there is any problem with append being synchronous
(i.e. pumping immediately) because that is what callers expect and that's what
other DocumentParser subclasses do also.

[kouhei (in TOK), 2014-12-08 01:35:28]

Please address LayoutTests failures.

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

https://codereview.chromium.org/779393002/diff/1/Source/core/html/parser/HTML...
Source/core/html/parser/HTMLDocumentParser.cpp:991: parser->insert(source); //
Use insert() so that the parser will not yield.
should we change this to append?

[kbalazs, 2014-12-08 20:37:37]

On 2014/12/08 01:35:28, kouhei wrote:
> Please address LayoutTests failures.

Done. In case a parser was already created previously my assert failed. Now I
pass the mode flag to implicitOpen which begins with cancelParsing hence makes
sure we start from scratch.

> 
>
https://codereview.chromium.org/779393002/diff/1/Source/core/html/parser/HTML...
> File Source/core/html/parser/HTMLDocumentParser.cpp (right):
> 
>
https://codereview.chromium.org/779393002/diff/1/Source/core/html/parser/HTML...
> Source/core/html/parser/HTMLDocumentParser.cpp:991: parser->insert(source); //
> Use insert() so that the parser will not yield.
> should we change this to append?

Yes, absolutely.

[kouhei (in TOK), 2014-12-09 01:34:05]

On 2014/12/08 20:37:37, kbalazs wrote:
> On 2014/12/08 01:35:28, kouhei wrote:
> > Please address LayoutTests failures.
> 
> Done. In case a parser was already created previously my assert failed. Now I
> pass the mode flag to implicitOpen which begins with cancelParsing hence makes
> sure we start from scratch.

Would you take a look at
http/tests/inspector/network/network-iframe-load-and-delete.html ?

[kbalazs, 2014-12-09 01:50:16]

On 2014/12/09 01:34:05, kouhei wrote:
> On 2014/12/08 20:37:37, kbalazs wrote:
> > On 2014/12/08 01:35:28, kouhei wrote:
> > > Please address LayoutTests failures.
> > 
> > Done. In case a parser was already created previously my assert failed. Now
I
> > pass the mode flag to implicitOpen which begins with cancelParsing hence
makes
> > sure we start from scratch.
> 
> Would you take a look at
> http/tests/inspector/network/network-iframe-load-and-delete.html ?

Sure. Tomorrow. :)

[kbalazs, 2014-12-09 21:41:25]

Apparently a TextDocument can also be created in sync mode, which I didn't
consider before. Fixed.

[kbalazs, 2014-12-11 02:23:09]

On 2014/12/09 21:41:25, kbalazs wrote:
> Apparently a TextDocument can also be created in sync mode, which I didn't
> consider before. Fixed.

Greenness, ptal.

[kouhei (in TOK), 2014-12-11 02:24:56]

lgtm. Thanks for the clean up.

[kbalazs, 2014-12-11 16:27:31]

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

[commit-bot: I haz the power, 2014-12-11 16:28:18]

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

[commit-bot: I haz the power, 2014-12-11 16:41:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-11 16:41:45]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)

[kbalazs, 2014-12-13 00:32:27]

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

[kbalazs, 2014-12-13 00:32:48]

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

[kbalazs, 2014-12-13 00:38:42]

On 2014/12/11 16:41:45, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_gn_rel on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)

Removed DocWriter::m_forceSyncParse because it was only used in an assert -
making release build fail because unused - and we have logically equivalent
assert in HTMLDocumentParser anyways.

[kbalazs, 2014-12-13 00:39:04]

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

[commit-bot: I haz the power, 2014-12-13 00:39:49]

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

[commit-bot: I haz the power, 2014-12-13 03:30:36]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187078