Move attributes and methods from HTMLDocument to Document

Move attributes and methods from HTMLDocument to Document

Move open, close, write, writeln from HTMLDocument to 
Document. 

Open method is defined as custom and was previously
implemented for HTMLDocument, it is moved to V8DocumentCustom.

Dynamic insertion, open/write/writeln/close is not supported 
for non-HTML document: 
https://html.spec.whatwg.org/#dynamic-markup-insertion. 
Code has been updated in Document::open, Document::write and 
Document::close to throw DOMException InvalidStateError. The 
LayoutTests which was previosuly throwing has been updated 
accordingly and it matches with Firefox functionality

parseFromString uses Document::setContent and this internally 
calls Document::open and Document::close. Since open and close,
now throw exception, parseString for the non-HTML document needs
two internal function Document::open and Document::close. Old 
open method will be initiated via bindings and new functions are
to be used for internal purpose. The new method does not pass
any argument and that's how two functions are differentiated.

The existing open and close function includes two DOM exceptions
as listed here:
http://w3c.github.io/webcomponents/spec/imports/#interface-document

In write it throws an exception if it is not HTML document. It
is inconsistent with specification but matches behavior of open
and close. A bug has been raised to correct the spec:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=28864

BUG=496376
R=philipj

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

[Habib Virji, 2015-06-10 13:56:25]

The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-10 13:56:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1176013002/1

[Habib Virji, 2015-06-10 14:37:26]

The CQ bit was unchecked by habib.virji@samsung.com

[Habib Virji, 2015-06-10 15:48:12]

The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-10 15:48:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1176013002/20001

[commit-bot: I haz the power, 2015-06-10 17:21:29]

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

[commit-bot: I haz the power, 2015-06-10 17:21:30]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

[Habib Virji, 2015-06-16 16:05:56]

The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-16 16:06:07]

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

[commit-bot: I haz the power, 2015-06-16 17:53:31]

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

[commit-bot: I haz the power, 2015-06-16 17:53:32]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

[Habib Virji, 2015-06-17 16:52:13]

@philip: 

This is not yet complete but I am kind of stuck and 
wanted your opinion. 

This CL tries to move Open, Close and Write/Writeln 
from HTMLDocument to Document. As per 
https://html.spec.whatwg.org/#dynamic-markup-insertion,
for XML document.write/open/close it should throw 
"InvalidStateError exception if the Document is an 
XML document."

As you can see in between patch 2 and patch 3, in 
patch 2 some test crashed and failed in 
imported/web-platform-tests/html/dom/dynamic-markup-insertion. 
To fix the issue, I introduced a check in open/write/close
 to throw an exception if document type is xhtml as done 
in firefox:
https://github.com/mozilla/gecko-dev/blob/56a152e92c2cff1c07f573c4102c12ebe55....

It fixed the issue of patch 2 but introduces issue for
DOMParser.parseFromString which has XML string. Since 
this calls Document::setContent and since it is XHTML
it throws an exception. 

The confusion here is changes I did in open/write/close 
to throw an exception is it right way forward? It makes it
compatible with firefox but breaks parseFromString for XML.

[philipj_slow, 2015-06-17 19:58:04]

On 2015/06/17 16:52:13, Habib Virji wrote:
> @philip: 
> 
> This is not yet complete but I am kind of stuck and 
> wanted your opinion. 

Yay, this looks like fun!

> This CL tries to move Open, Close and Write/Writeln 
> from HTMLDocument to Document. As per 
> https://html.spec.whatwg.org/#dynamic-markup-insertion,
> for XML document.write/open/close it should throw 
> "InvalidStateError exception if the Document is an 
> XML document."
> 
> As you can see in between patch 2 and patch 3, in 
> patch 2 some test crashed and failed in 
> imported/web-platform-tests/html/dom/dynamic-markup-insertion. 
> To fix the issue, I introduced a check in open/write/close
>  to throw an exception if document type is xhtml as done 
> in firefox:
>
https://github.com/mozilla/gecko-dev/blob/56a152e92c2cff1c07f573c4102c12ebe55....
> 
> It fixed the issue of patch 2 but introduces issue for
> DOMParser.parseFromString which has XML string. Since 
> this calls Document::setContent and since it is XHTML
> it throws an exception. 
> 
> The confusion here is changes I did in open/write/close 
> to throw an exception is it right way forward? It makes it
> compatible with firefox but breaks parseFromString for XML.

In principle there should be no problem sorting this out, as per spec
DOMParser.parseFromString() isn't defined in terms of open() and close() like
the implementation is:
https://dvcs.w3.org/hg/innerhtml/raw-file/tip/index.html#widl-DOMParser-parse...

In cs.chromium.org you can find some other calls to Document::setContent which
potentially have the same problem, in particular XSLTProcessor and
XMLHttpRequest ought to be able to trigger the same assert you're seeing.

A few ways around this come to mind:
1. Rewrite Document::setContent to not use open() or close() at all.
2. Have two different open() methods, one that's called by bindings and throws
an exception per spec, and one that's called from the first and for any internal
uses.
3. Look at all the places that use Document::setContent() and see how the specs
express these cases. I would guess that normally it's expressed as creating a
parser, feeding it bytes and getting a Document back. This is different from
what we have, where the Document itself has a parser and you feed it with data
to modify the Document.

Option 2 seems simplest.

[Habib Virji, 2015-06-17 23:05:42]

Thanks Philip for the explanation it is quite clear, will work out the solution
and update soon.

[Habib Virji, 2015-06-18 11:50:38]

The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-18 11:51:02]

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

[commit-bot: I haz the power, 2015-06-18 13:10:11]

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

[commit-bot: I haz the power, 2015-06-18 13:10:13]

Dry run: This issue passed the CQ dry run.

[Habib Virji, 2015-06-18 14:15:36]

@philip: Thanks updated with your proposal # 2. It also involved updating close.

[Habib Virji, 2015-06-23 08:21:59]

habib.virji@samsung.com changed reviewers:
+ rbyers@chromium.org

[Rick Byers, 2015-06-23 21:09:44]

rbyers@chromium.org changed reviewers:
+ tkent@chromium.org
- rbyers@chromium.org

[Rick Byers, 2015-06-23 21:10:28]

rbyers@chromium.org changed reviewers:
+ rbyers@chromium.org

[Rick Byers, 2015-06-23 21:10:29]

Replacing myself with tkent who is specifically interested in DOM API
conformance.

[tkent, 2015-06-23 23:42:17]

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2320: if (isXHTMLDocument()) {
Why do you check isXHTMLDocument(), not isXMLDocument()?
What about SVGDocument, ImageDocument, PluginDocument, and MediaDocument?

[tkent, 2015-06-23 23:44:20]

> https://html.spec.whatwg.org/#dynamic-markup-insertion. 

Please avoid to link to the all-in-one version of the specification.  It should
be
https://html.spec.whatwg.org/multipage/webappapis.html#dynamic-markup-insertion

[philipj_slow, 2015-06-24 11:26:38]

On 2015/06/23 23:44:20, tkent wrote:
> > https://html.spec.whatwg.org/#dynamic-markup-insertion. 
> 
> Please avoid to link to the all-in-one version of the specification.  It
should
> be
>
https://html.spec.whatwg.org/multipage/webappapis.html#dynamic-markup-insertion

I actually use the single-page spec whenever I get to pick as those links tend
to change less frequently, as the way in which the spec is split seems to change
more often than the IDs themselves change. The split spec also isn't as good at
backreferences, for example if you click "ignore-opens-during-unload counter" in
these two specs the multipage spec is missing the link to a reference from
another page.

[philipj_slow, 2015-06-24 11:52:19]

I think this will work out, just some questions.

https://codereview.chromium.org/1176013002/diff/60001/LayoutTests/fast/dom/do...
File LayoutTests/fast/dom/domparser-parsefromstring-mimetype-support.html
(right):

https://codereview.chromium.org/1176013002/diff/60001/LayoutTests/fast/dom/do...
LayoutTests/fast/dom/domparser-parsefromstring-mimetype-support.html:158:
</html>
This looks like an accidental change unrelated to the HTMLDocument->Document
move.

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2316:
exceptionState.throwDOMException(InvalidStateError, "Imported document doesn't
support open().");
Is an import document also an HTML document? If not it could be included in the
error handling below and afterwards you could ASSERT(!importLoader()) if it's
important.

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2320: if (isXHTMLDocument()) {
On 2015/06/23 23:42:17, tkent wrote:
> Why do you check isXHTMLDocument(), not isXMLDocument()?
> What about SVGDocument, ImageDocument, PluginDocument, and MediaDocument?

The spec says "If the Document object is not flagged as an HTML document, throw
an InvalidStateError exception and abort these steps." so this ought to be |if
(!isHTMLDocument())| with a message like "Only HTML documents support open()."

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2475: if (isXHTMLDocument()) {
Spec says "If the Document object is not flagged as an HTML document, throw an
InvalidStateError exception and abort these steps."

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2723: if (isXHTMLDocument()) {
Here the spec actually says "If the method was invoked on an XML document, throw
an InvalidStateError exception and abort these steps." which would be |if
(isXMLDocument())|. If that's absolutely equivalent to !isHTMLDocument() I would
use that instead for consistency, and if there is some difference we should
probably have the spec changed.

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2744: open(ownerDocument, exceptionState);
It looks like it's impossible for this to throw an exception, so
ASSERT_NO_EXCEPTION? If it were possible, it would also be necessary to handle
it.

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.h:495: // This is used for internal purpose and does
not handle exceptions.
s/for internal purpose/internally/

I think also move this up here and in the C++ so that the order matches the flow
order: script-exposed open(), internal open(), implicitOpen().

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
File Source/core/dom/Document.idl (right):

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.idl:110: // FIXME: There are two open() methods in the
spec.
Make this TODO(habib.virji) since it's a new problem you've found.

[Habib Virji, 2015-06-25 13:08:38]

The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-25 13:08:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1176013002/80001

[Habib Virji, 2015-06-25 13:09:47]

Thanks philip and tkent, please find my comments inline.

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2316:
exceptionState.throwDOMException(InvalidStateError, "Imported document doesn't
support open().");
Changed as suggested and it is do important added assert.

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2320: if (isXHTMLDocument()) {
I find spec in-consistent. 

What you mentioned is do specified in the steps but a note before this step
says:
"Throws an InvalidStateError exception if the Document is an XML document." 

Since !isHTMLDocument() != isXMLDocument() (as explained below), note and steps
are inconsistent.

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2475: if (isXHTMLDocument()) {
Since !isHTMLDocument() != isXMLDocument() (as explained below), note and steps
are inconsistent.

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2723: if (isXHTMLDocument()) {
isXMLDocument() relies on value of DocumentClass, which is initialized as
XMLDocument when creating the XML document, XMLDocument | XHTMLDocument when
creating a XHTMLDocument and so forth. Same applies for isHTMLDocument(). 

This implies !isHTMLDocument sets DocumentClass only for HTMLDocument and will
imply not be set for XML, SVG, XHTML but also for Image, Plugin and Media. Which
is not the spec says, so have kept isXMLDocument(). 

Spec for write() is consistent with steps and note. So either note for open and
close should be updated or step for write should be updated.

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2744: open(ownerDocument, exceptionState);
The reason for adding was it relied on ASSERT_NO_EXCEPTION value as this was
defaultValue for the open(document, exceptionState). With two method one without
parameter, needed to remove default values and this was the only call that was
without exception but with the Document parameter. I have back exceptionState
default in the open with parameter to avoid indicating this will generate an
error.

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.h:495: // This is used for internal purpose and does
not handle exceptions.
On 2015/06/24 11:52:19, philipj wrote:
> s/for internal purpose/internally/
> 
> I think also move this up here and in the C++ so that the order matches the
flow
> order: script-exposed open(), internal open(), implicitOpen().

Done.

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
File Source/core/dom/Document.idl (right):

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.idl:110: // FIXME: There are two open() methods in the
spec.
This FIXME is from the HTMLDocument.idl. 

There are two open() methods in the spec::
https://html.spec.whatwg.org/#document...
1. Document open(optional DOMString type = "text/html", optional DOMString
replace = "");
2. WindowProxy open(DOMString url, DOMString name, DOMString features, optional
boolean replace = false);

Hence keeping statement as it is, since open() I have defined is more for an
internally purpose, while spec open function have different functionality.

[Habib Virji, 2015-06-25 13:10:04]

The CQ bit was unchecked by habib.virji@samsung.com

[philipj_slow, 2015-06-27 21:42:56]

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2316:
exceptionState.throwDOMException(InvalidStateError, "Imported document doesn't
support open().");
On 2015/06/25 13:09:46, Habib Virji wrote:
> Changed as suggested and it is do important added assert.

Can you link to
http://w3c.github.io/webcomponents/spec/imports/#interface-document for these to
make it clear that there is actually a spec for this bit, which I assumed there
wasn't and was happily surprised to find.

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2320: if (isXHTMLDocument()) {
On 2015/06/25 13:09:46, Habib Virji wrote:
> I find spec in-consistent. 
> 
> What you mentioned is do specified in the steps but a note before this step
> says:
> "Throws an InvalidStateError exception if the Document is an XML document." 
> 
> Since !isHTMLDocument() != isXMLDocument() (as explained below), note and
steps
> are inconsistent. 

The note is non-normative, so !isHTMLDocument() is what's actually required by
the spec for open().

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2723: if (isXHTMLDocument()) {
On 2015/06/25 13:09:46, Habib Virji wrote:
> isXMLDocument() relies on value of DocumentClass, which is initialized as
> XMLDocument when creating the XML document, XMLDocument | XHTMLDocument when
> creating a XHTMLDocument and so forth. Same applies for isHTMLDocument(). 
> 
> This implies !isHTMLDocument sets DocumentClass only for HTMLDocument and will
> imply not be set for XML, SVG, XHTML but also for Image, Plugin and Media.
Which
> is not the spec says, so have kept isXMLDocument(). 
> 
> Spec for write() is consistent with steps and note. So either note for open
and
> close should be updated or step for write should be updated. 

Does write() need to be supported for Image, Plugin or Media documents?

I agree that the spec is inconsistent. What do you think it should say? To me it
seems most sensible to consistently throw for !isHTMLDocument.

[philipj_slow, 2015-06-27 21:49:24]

https://codereview.chromium.org/1176013002/diff/100001/Source/core/dom/Docume...
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/1176013002/diff/100001/Source/core/dom/Docume...
Source/core/dom/Document.h:495: // open() is the DOM API document.open()
Maybe "This is ..." and the same change for close() below, as this otherwise
kind of looks like it's talking about the open() above the comment. I also still
think matching the order to the calling order would make sense, i.e. moving this
and the web-exposed close() up before the internal ones here and in the .cpp

https://codereview.chromium.org/1176013002/diff/100001/Source/core/dom/Docume...
Source/core/dom/Document.h:496: void open(Document* ownerDocument,
ExceptionState& = ASSERT_NO_EXCEPTION);
It doesn't look like this is ever called with just one argument. If so, there
doesn't need to be a default second argument.

[Habib Virji, 2015-06-29 12:17:42]

PTAL.

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2316:
exceptionState.throwDOMException(InvalidStateError, "Imported document doesn't
support open().");
On 2015/06/27 21:42:56, philipj wrote:
> On 2015/06/25 13:09:46, Habib Virji wrote:
> > Changed as suggested and it is do important added assert.
> 
> Can you link to
> http://w3c.github.io/webcomponents/spec/imports/#interface-document for these
to
> make it clear that there is actually a spec for this bit, which I assumed
there
> wasn't and was happily surprised to find.

Done.

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2320: if (isXHTMLDocument()) {
On 2015/06/27 21:42:56, philipj wrote:
> On 2015/06/25 13:09:46, Habib Virji wrote:
> > I find spec in-consistent. 
> > 
> > What you mentioned is do specified in the steps but a note before this step
> > says:
> > "Throws an InvalidStateError exception if the Document is an XML document." 
> > 
> > Since !isHTMLDocument() != isXMLDocument() (as explained below), note and
> steps
> > are inconsistent. 
> 
> The note is non-normative, so !isHTMLDocument() is what's actually required by
> the spec for open().

Done.

https://codereview.chromium.org/1176013002/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2723: if (isXHTMLDocument()) {
It should not. I agree it should throw for !isHTMLDocument.

https://codereview.chromium.org/1176013002/diff/100001/Source/core/dom/Docume...
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/1176013002/diff/100001/Source/core/dom/Docume...
Source/core/dom/Document.h:495: // open() is the DOM API document.open()
On 2015/06/27 21:49:24, philipj wrote:
> Maybe "This is ..." and the same change for close() below, as this otherwise
> kind of looks like it's talking about the open() above the comment. I also
still
> think matching the order to the calling order would make sense, i.e. moving
this
> and the web-exposed close() up before the internal ones here and in the .cpp

Done.

https://codereview.chromium.org/1176013002/diff/100001/Source/core/dom/Docume...
Source/core/dom/Document.h:496: void open(Document* ownerDocument,
ExceptionState& = ASSERT_NO_EXCEPTION);
It was actually but I have removed from here and add it only one place it was
calling.

[philipj_slow, 2015-06-29 15:02:39]

https://codereview.chromium.org/1176013002/diff/120001/Source/core/dom/Docume...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1176013002/diff/120001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:2727:
exceptionState.throwDOMException(InvalidStateError, "XML document doesn't
support write().");
I think this message should match those for open() and close(). I've filed a
spec bug, please add it in a comment before this check:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=28864

https://codereview.chromium.org/1176013002/diff/120001/Source/core/dom/Docume...
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/1176013002/diff/120001/Source/core/dom/Docume...
Source/core/dom/Document.h:493: // The open and close are used internally for
dynamic insertion and does not handle exceptions.
Sorry for the churn, but I meant this order of things, here and in
implementation:

// This is the DOM API document.open()
void open(Document* ownerDocument, ExceptionState&);
// This is used internally and does not handle exceptions.
void open();
PassRefPtrWillBeRawPtr<DocumentParser>
implicitOpen(ParserSynchronizationPolicy);

// This is the DOM API document.close()
void close(ExceptionState&);
// This is used internally and does not handle exceptions.
void close();
// In some situations ...

https://codereview.chromium.org/1176013002/diff/120001/Source/core/dom/Docume...
Source/core/dom/Document.h:526: void write(LocalDOMWindow*, const
Vector<String>& text, ExceptionState& = ASSERT_NO_EXCEPTION);
I don't think these are ever called without the ExceptionState& argument, so can
the default be removed?

[Habib Virji, 2015-06-29 15:25:21]

PTAL.

https://codereview.chromium.org/1176013002/diff/120001/Source/core/dom/Docume...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1176013002/diff/120001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:2727:
exceptionState.throwDOMException(InvalidStateError, "XML document doesn't
support write().");
Added in the description and correct the message..

https://codereview.chromium.org/1176013002/diff/120001/Source/core/dom/Docume...
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/1176013002/diff/120001/Source/core/dom/Docume...
Source/core/dom/Document.h:493: // The open and close are used internally for
dynamic insertion and does not handle exceptions.
Sorry for not getting the order.. Updates now as suggested.

https://codereview.chromium.org/1176013002/diff/120001/Source/core/dom/Docume...
Source/core/dom/Document.h:526: void write(LocalDOMWindow*, const
Vector<String>& text, ExceptionState& = ASSERT_NO_EXCEPTION);
On 2015/06/29 15:02:38, philipj wrote:
> I don't think these are ever called without the ExceptionState& argument, so
can
> the default be removed?

Done.

[philipj_slow, 2015-06-29 19:39:31]

philipj@opera.com changed reviewers:
+ esprehn@chromium.org

[philipj_slow, 2015-06-29 19:39:32]

This now LGTM, small and conservative change and the description is very
helpful.

It is unfortunate that we have so many flavors of open() and close() now, but
that was to be expected with this approach. If it can be simplified it would be
best to do so in separate CLs. I think the ideal end state would be if either
open/close/write aren't used at all internally, or at least if they're only used
for HTML documents.

I would like a second opinion on this, though. esprehn@, maybe you could take a
look? You've been around far longer than me and have at least occasionally poked
around this code.

https://codereview.chromium.org/1176013002/diff/140001/Source/core/dom/Docume...
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/1176013002/diff/140001/Source/core/dom/Docume...
Source/core/dom/Document.h:499: // This close() is based on the DOM API
document.close()
"This is the DOM API document.close()" for consistency with the above.

[Rick Byers, 2015-07-02 13:44:54]

rbyers@chromium.org changed reviewers:
- rbyers@chromium.org

[tkent, 2015-07-02 23:52:35]

lgtm

[philipj_slow, 2015-07-03 21:09:18]

Thanks for taking a look, tkent!

In my mind this CL is OK to land now, just tweak the documentation of close().

[Habib Virji, 2015-07-08 08:49:21]

The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-08 08:49:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1176013002/160001

[commit-bot: I haz the power, 2015-07-08 08:53:34]

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

[commit-bot: I haz the power, 2015-07-08 08:53:35]

Dry run: Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/62117)

[Habib Virji, 2015-07-08 09:46:16]

The CQ bit was checked by habib.virji@samsung.com

[Habib Virji, 2015-07-08 09:46:16]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
philipj@opera.com
Link to the patchset: https://codereview.chromium.org/1176013002/#ps180001
(title: "Updated to latest master")

[commit-bot: I haz the power, 2015-07-08 09:46:53]

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

[Habib Virji, 2015-07-08 09:47:11]

Thanks tkent and philipj. 

philipj updated close API comments before commit.

[commit-bot: I haz the power, 2015-07-08 10:53:06]

Committed patchset #10 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198491