Set default document urls to 'about:blank'.

Set default document urls to 'about:blank'.

The patch ensures that the document urls are set to 'about:blank', as is
specified in [1], if a url has not been specified. It fixes the tests in
[2] and [3].

The functionality is implemented by binding the document urls to
'about:blank' if m_url is empty. To achieve this behavior the
Document::urlForBinding() function was added. The choice for a binding
was made rather than not using an intial value of 'about:blank' for the
document urls, as other parts of the code rely on the document urls
initially being empty.

More details regarding the binding can be found under:
https://codereview.chromium.org/2749803003/#msg37

[1] https://dom.spec.whatwg.org/#interface-document
[2] http://w3c-test.org/dom/nodes/DOMImplementation-createDocument.html
[3] http://w3c-test.org/dom/nodes/DOMImplementation-createHTMLDocument.html

BUG=563986
Review-Url: https://codereview.chromium.org/2754933004
Cr-Commit-Position: refs/heads/master@{#459019}
Committed: https://chromium.googlesource.com/chromium/src/+/45009c8984881526e99cdb26a752a3acd29a0b60

[MartinRogalla, 2017-03-18 03:27:54]

martin@martinrogalla.com changed reviewers:
+ tkent@chromium.org

[tkent, 2017-03-20 23:06:54]

https://codereview.chromium.org/2754933004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/2754933004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.h:1304: KURL urlForBinding() {
Let's declare urlForBinding() around url(), and add comments to both of url()
and urlForBinding() to explain their differences.

Also, move the function definition to Document.cpp.  We want to avoid inline
function definitions if it's not just one expression.

[MartinRogalla, 2017-03-22 04:26:02]

On 2017/03/20 23:06:54, tkent wrote:
>
https://codereview.chromium.org/2754933004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/Document.h (right):
> 
>
https://codereview.chromium.org/2754933004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/Document.h:1304: KURL urlForBinding() {
> Let's declare urlForBinding() around url(), and add comments to both of url()
> and urlForBinding() to explain their differences.
> 
> Also, move the function definition to Document.cpp.  We want to avoid inline
> function definitions if it's not just one expression.

I've made the modifications, rebased and ran the tests locally.

[tkent, 2017-03-22 08:36:27]

https://codereview.chromium.org/2754933004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/2754933004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.h:598: // Get the URL that was used
to retrieve this document.
We are not sure if we should remove it or not. So past tense isn't reasonable.
Also, this comment doesn't explain difference from urlForBinding().
How about something like:

// Return the document URL, or an empty URL if it's unavailable.
// This is not an implementation of web-exposed Document.prototype.URL.

[MartinRogalla, 2017-03-22 17:17:35]

On 2017/03/22 08:36:27, tkent wrote:
>
https://codereview.chromium.org/2754933004/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Document.h (right):
> 
>
https://codereview.chromium.org/2754933004/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Document.h:598: // Get the URL that was
used
> to retrieve this document.
> We are not sure if we should remove it or not. So past tense isn't reasonable.
> Also, this comment doesn't explain difference from urlForBinding().
> How about something like:
> 
> // Return the document URL, or an empty URL if it's unavailable.
> // This is not an implementation of web-exposed Document.prototype.URL.

I agree, I changed the comment to your suggestion.

[tkent, 2017-03-22 22:21:29]

The CQ bit was checked by tkent@chromium.org

[tkent, 2017-03-22 22:21:29]

lgtm

[commit-bot: I haz the power, 2017-03-22 22:22:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 00:07:20]

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

[commit-bot: I haz the power, 2017-03-23 00:07:21]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[MartinRogalla, 2017-03-23 01:21:05]

On 2017/03/23 00:07:21, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

Fixing the tests locally, recompiling, rerunning tests and then I will upload
the new patch.

[MartinRogalla, 2017-03-23 03:14:28]

The CQ bit was checked by martin@martinrogalla.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 03:14:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 03:30:25]

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

[commit-bot: I haz the power, 2017-03-23 03:30:26]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[tkent, 2017-03-23 05:49:59]

The CQ bit was checked by tkent@chromium.org

[tkent, 2017-03-23 05:50:00]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/2754933004/#ps60001
(title: "Fixed test expectations.")

[commit-bot: I haz the power, 2017-03-23 05:50:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 06:05:41]

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

[commit-bot: I haz the power, 2017-03-23 06:05:42]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[tkent, 2017-03-23 07:39:56]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2017-03-23 07:40:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 07:59:11]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490254796040920,
"parent_rev": "415f9bab260d050f3471daae4282a267aa954094", "commit_rev":
"45009c8984881526e99cdb26a752a3acd29a0b60"}

[commit-bot: I haz the power, 2017-03-23 08:00:18]

Description was changed from

==========
Set default document urls to 'about:blank'.

The patch ensures that the document urls are set to 'about:blank', as is
specified in [1], if a url has not been specified. It fixes the tests in
[2] and [3].

The functionality is implemented by binding the document urls to
'about:blank' if m_url is empty. To achieve this behavior the
Document::urlForBinding() function was added. The choice for a binding
was made rather than not using an intial value of 'about:blank' for the
document urls, as other parts of the code rely on the document urls
initially being empty.

More details regarding the binding can be found under:
https://codereview.chromium.org/2749803003/#msg37

[1] https://dom.spec.whatwg.org/#interface-document
[2] http://w3c-test.org/dom/nodes/DOMImplementation-createDocument.html
[3] http://w3c-test.org/dom/nodes/DOMImplementation-createHTMLDocument.html

BUG=563986
==========

to

==========
Set default document urls to 'about:blank'.

The patch ensures that the document urls are set to 'about:blank', as is
specified in [1], if a url has not been specified. It fixes the tests in
[2] and [3].

The functionality is implemented by binding the document urls to
'about:blank' if m_url is empty. To achieve this behavior the
Document::urlForBinding() function was added. The choice for a binding
was made rather than not using an intial value of 'about:blank' for the
document urls, as other parts of the code rely on the document urls
initially being empty.

More details regarding the binding can be found under:
https://codereview.chromium.org/2749803003/#msg37

[1] https://dom.spec.whatwg.org/#interface-document
[2] http://w3c-test.org/dom/nodes/DOMImplementation-createDocument.html
[3] http://w3c-test.org/dom/nodes/DOMImplementation-createHTMLDocument.html

BUG=563986

Review-Url: https://codereview.chromium.org/2754933004
Cr-Commit-Position: refs/heads/master@{#459019}
Committed:
https://chromium.googlesource.com/chromium/src/+/45009c8984881526e99cdb26a752...
==========

[commit-bot: I haz the power, 2017-03-23 08:00:19]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/45009c8984881526e99cdb26a752...

[Mike Wittman, 2017-03-23 18:28:27]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2775693002/ by wittman@chromium.org.

The reason for reverting is: Appears to be causing failures on WebKit Mac10.12:
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Mac1...

01:50:58.061 5302 worker/7 external/wpt/dom/interfaces.html output stderr lines:
01:50:58.061 5302   CONSOLE MESSAGE: line 251: callback not yet supported
01:50:58.082 5261 [18796/51893] external/wpt/dom/interfaces.html failed
unexpectedly (text diff)
01:50:58.081 5302 worker/7 external/wpt/dom/interfaces.html failed:
.

[MartinRogalla, 2017-03-23 18:53:02]

On 2017/03/23 18:28:27, Mike Wittman wrote:
> A revert of this CL (patchset #4 id:60001) has been created in
> https://codereview.chromium.org/2775693002/ by mailto:wittman@chromium.org.
> 
> The reason for reverting is: Appears to be causing failures on WebKit
Mac10.12:
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Mac1...
> 
> 01:50:58.061 5302 worker/7 external/wpt/dom/interfaces.html output stderr
lines:
> 01:50:58.061 5302   CONSOLE MESSAGE: line 251: callback not yet supported
> 01:50:58.082 5261 [18796/51893] external/wpt/dom/interfaces.html failed
> unexpectedly (text diff)
> 01:50:58.081 5302 worker/7 external/wpt/dom/interfaces.html failed:
> .

tkent, do you have an idea what's going on? I'm quite confused as to why it
passes with trybot.

[tkent, 2017-03-23 22:32:23]

On 2017/03/23 at 18:53:02, martin wrote:
> tkent, do you have an idea what's going on? I'm quite confused as to why it
passes with trybot.

Unfortunately, the default set of trybots doesn't have mac10.12 coverage, and we
have three interfaces-expected.txt
* LayoutTests/external/wpt/dom/interfaces-expected.txt
* LayoutTests/platform/mac-mac10.11/external/wpt/dom/interfaces-expected.txt
* LayoutTests/platform/win/external/wpt/dom/interfaces-expected.txt

With the current fallback rule, only mac10.12 uses the first one.
These files are redundant.  We should remove
LayoutTests/platform/*/external/dom/interfaces-expected.txt, and this CL should
update LayoutTests/external/dom/inerfaces-expected.txt.

[MartinRogalla, 2017-03-24 22:30:03]

On 2017/03/23 22:32:23, tkent wrote:
> On 2017/03/23 at 18:53:02, martin wrote:
> > tkent, do you have an idea what's going on? I'm quite confused as to why it
> passes with trybot.
> 
> Unfortunately, the default set of trybots doesn't have mac10.12 coverage, and
we
> have three interfaces-expected.txt
> * LayoutTests/external/wpt/dom/interfaces-expected.txt
> * LayoutTests/platform/mac-mac10.11/external/wpt/dom/interfaces-expected.txt
> * LayoutTests/platform/win/external/wpt/dom/interfaces-expected.txt
> 
> With the current fallback rule, only mac10.12 uses the first one.
> These files are redundant.  We should remove
> LayoutTests/platform/*/external/dom/interfaces-expected.txt, and this CL
should
> update LayoutTests/external/dom/inerfaces-expected.txt.

Okay, I removed the platform specific tests and kept the main
third_party/WebKit/LayoutTests/external/wpt/dom/interfaces-expected.txt in the
following issue: https://codereview.chromium.org/2768373005