Added UseCounter for clearing browsing context name on cross-origin name

Added UseCounter for clearing browsing context name on cross-origin name

Added a UseCounter that tracks when a browsing context-name that would
have been cleared because of a main-level cross-origin navigation is
accessed.
Spec: https://html.spec.whatwg.org/multipage/browsers.html#resetBCName
ITI: https://groups.google.com/a/chromium.org/d/msg/blink-dev/8uZDknA2Ua0/Sm33B4MPCwAJ

I have tests ready for this functionality but since this is just a UseCounter
the tests would not really work.

BUG=706350
Review-Url: https://codereview.chromium.org/2795673002
Cr-Commit-Position: refs/heads/master@{#461997}
Committed: https://chromium.googlesource.com/chromium/src/+/f936f423177349c1840be6e9b83114a844e74fa8

[andypaicu, 2017-04-03 08:43:44]

Description was changed from

==========
Added UseCounter for clearing browsing context name on cross-origin name

Added a UseCounter that tracks when a browsing context-name that would
have been cleared because of a main-level cross-origin navigation is
accessed.

BUG=706350
==========

to

==========
Added UseCounter for clearing browsing context name on cross-origin name

Added a UseCounter that tracks when a browsing context-name that would
have been cleared because of a main-level cross-origin navigation is
accessed.
Spec: https://html.spec.whatwg.org/multipage/browsers.html#resetBCName
ITI:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/8uZDknA2Ua0/Sm33B4MP...

BUG=706350
==========

[andypaicu, 2017-04-03 08:43:44]

andypaicu@chromium.org changed reviewers:
+ jochen@chromium.org, mkwst@chromium.org

[andypaicu, 2017-04-03 08:44:44]

Description was changed from

==========
Added UseCounter for clearing browsing context name on cross-origin name

Added a UseCounter that tracks when a browsing context-name that would
have been cleared because of a main-level cross-origin navigation is
accessed.
Spec: https://html.spec.whatwg.org/multipage/browsers.html#resetBCName
ITI:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/8uZDknA2Ua0/Sm33B4MP...

BUG=706350
==========

to

==========
Added UseCounter for clearing browsing context name on cross-origin name

Added a UseCounter that tracks when a browsing context-name that would
have been cleared because of a main-level cross-origin navigation is
accessed.
Spec: https://html.spec.whatwg.org/multipage/browsers.html#resetBCName
ITI:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/8uZDknA2Ua0/Sm33B4MP...

I have tests ready for this functionality but since this is just a UseCounter
the tests would not really work.

BUG=706350
==========

[andypaicu, 2017-04-03 08:45:11]

The CQ bit was checked by andypaicu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 08:45:26]

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-04-03 09:34:31]

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

[commit-bot: I haz the power, 2017-04-03 09:34:32]

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

[andypaicu, 2017-04-03 11:56:15]

The CQ bit was checked by andypaicu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 11:56:21]

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-04-03 11:58:32]

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

[commit-bot: I haz the power, 2017-04-03 11:58:33]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[andypaicu, 2017-04-03 15:07:49]

The CQ bit was checked by andypaicu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 15:08:20]

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

[jochen (gone - plz use gerrit), 2017-04-03 15:24:26]

lgtm with nit

https://codereview.chromium.org/2795673002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2795673002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:1037: static inline
bool shouldClearWindowName(
nit. don't add inline

[commit-bot: I haz the power, 2017-04-03 15:38:49]

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

[commit-bot: I haz the power, 2017-04-03 15:38:50]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[andypaicu, 2017-04-04 07:21:07]

The CQ bit was checked by andypaicu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 07:21:19]

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

[andypaicu, 2017-04-04 07:23:40]

https://codereview.chromium.org/2795673002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2795673002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:1037: static inline
bool shouldClearWindowName(
On 2017/04/03 at 15:24:26, jochen (slow until Apr 4) wrote:
> nit. don't add inline

Done. Also moved function into the DocumentLoader class.

[commit-bot: I haz the power, 2017-04-04 07:52:49]

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

[commit-bot: I haz the power, 2017-04-04 07:52:50]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dcheng, 2017-04-04 07:53:21]

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

[dcheng, 2017-04-04 07:53:21]

https://codereview.chromium.org/2795673002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2795673002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:1070: if
(shouldClearWindowName(*m_frame, frameSecurityOrigin, *document)) {
Can we just check this in LocalDOMWindow::installNewDocument()? We can capture
both the previous SecurityOrigin and the new SecurityOrigin there, to avoid this
plumbing.

https://codereview.chromium.org/2795673002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.h (right):

https://codereview.chromium.org/2795673002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.h:100: SecurityOrigin*
frameSecurityOrigin);
Please clang-format the cl

(the presubmit should have warned about formatting, but it won't actually
prevent you from uploading it/committing it)

[dcheng, 2017-04-04 07:57:59]

https://codereview.chromium.org/2795673002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2795673002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:1070: if
(shouldClearWindowName(*m_frame, frameSecurityOrigin, *document)) {
On 2017/04/04 07:53:21, dcheng wrote:
> Can we just check this in LocalDOMWindow::installNewDocument()? We can capture
> both the previous SecurityOrigin and the new SecurityOrigin there, to avoid
this
> plumbing.

Ah never mind, that doesn't work because we may have changed the DOMWindow.

But I would still try to name frameSecurityOrigin a bit more descriptively:
maybe "previousSecurityOrigin"

[dcheng, 2017-04-04 07:59:39]

https://codereview.chromium.org/2795673002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2795673002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:1070: if
(shouldClearWindowName(*m_frame, frameSecurityOrigin, *document)) {
On 2017/04/04 07:57:59, dcheng wrote:
> On 2017/04/04 07:53:21, dcheng wrote:
> > Can we just check this in LocalDOMWindow::installNewDocument()? We can
capture
> > both the previous SecurityOrigin and the new SecurityOrigin there, to avoid
> this
> > plumbing.
> 
> Ah never mind, that doesn't work because we may have changed the DOMWindow.
> 
> But I would still try to name frameSecurityOrigin a bit more descriptively:
> maybe "previousSecurityOrigin"

Actually we can avoid plumbing around frameSecurityOrigin: just capture it at
the beginning of this function, before we install a new DOM window on line 1066.

[Mike West, 2017-04-04 12:12:01]

Pedantic grammar nits aside, LGTM once you deal with dcheng@'s feedback. :)

https://codereview.chromium.org/2795673002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2795673002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:1071: //
TODO(andypaicu): decide  if we can do this without breaking functionality
Nit: double-space after "decide"
Nit: Capital "D" for "decide".
Nit: "Decide _whether_" sounds better than "if".
Nit: Actually, I'd just remove the whole first sentence, and change the last
sentence to "If we decide to move forward with this change, we'd actually clean
the name here."

[andypaicu, 2017-04-04 14:03:17]

On 2017/04/04 at 07:59:39, dcheng wrote:
>
https://codereview.chromium.org/2795673002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):
> 
>
https://codereview.chromium.org/2795673002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/loader/DocumentLoader.cpp:1070: if
(shouldClearWindowName(*m_frame, frameSecurityOrigin, *document)) {
> On 2017/04/04 07:57:59, dcheng wrote:
> > On 2017/04/04 07:53:21, dcheng wrote:
> > > Can we just check this in LocalDOMWindow::installNewDocument()? We can
capture
> > > both the previous SecurityOrigin and the new SecurityOrigin there, to
avoid
> > this
> > > plumbing.
> > 
> > Ah never mind, that doesn't work because we may have changed the DOMWindow.
> > 
> > But I would still try to name frameSecurityOrigin a bit more descriptively:
> > maybe "previousSecurityOrigin"
> 
> Actually we can avoid plumbing around frameSecurityOrigin: just capture it at
the beginning of this function, before we install a new DOM window on line 1066.

Of course... I for some reason was under the impression that it happens a bit
before calling installNewDocument on "m_frame.loader()->clear()" but it's not
the case.

This definitely makes the code better.

[andypaicu, 2017-04-04 14:03:38]

On 2017/04/04 at 14:03:17, andypaicu wrote:
> On 2017/04/04 at 07:59:39, dcheng wrote:
> >
https://codereview.chromium.org/2795673002/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):
> > 
> >
https://codereview.chromium.org/2795673002/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/loader/DocumentLoader.cpp:1070: if
(shouldClearWindowName(*m_frame, frameSecurityOrigin, *document)) {
> > On 2017/04/04 07:57:59, dcheng wrote:
> > > On 2017/04/04 07:53:21, dcheng wrote:
> > > > Can we just check this in LocalDOMWindow::installNewDocument()? We can
capture
> > > > both the previous SecurityOrigin and the new SecurityOrigin there, to
avoid
> > > this
> > > > plumbing.
> > > 
> > > Ah never mind, that doesn't work because we may have changed the
DOMWindow.
> > > 
> > > But I would still try to name frameSecurityOrigin a bit more
descriptively:
> > > maybe "previousSecurityOrigin"
> > 
> > Actually we can avoid plumbing around frameSecurityOrigin: just capture it
at the beginning of this function, before we install a new DOM window on line
1066.
> 
> Of course... I for some reason was under the impression that it happens a bit
before calling installNewDocument on "m_frame.loader()->clear()" but it's not
the case.
> 
> This definitely makes the code better.

Done

[andypaicu, 2017-04-04 14:04:03]

On 2017/04/04 at 12:12:01, mkwst wrote:
> Pedantic grammar nits aside, LGTM once you deal with dcheng@'s feedback. :)
> 
>
https://codereview.chromium.org/2795673002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):
> 
>
https://codereview.chromium.org/2795673002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/loader/DocumentLoader.cpp:1071: //
TODO(andypaicu): decide  if we can do this without breaking functionality
> Nit: double-space after "decide"
> Nit: Capital "D" for "decide".
> Nit: "Decide _whether_" sounds better than "if".
> Nit: Actually, I'd just remove the whole first sentence, and change the last
sentence to "If we decide to move forward with this change, we'd actually clean
the name here."

Done

[andypaicu, 2017-04-04 14:04:16]

The CQ bit was checked by andypaicu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 14:04:29]

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-04-04 14:06:12]

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

[commit-bot: I haz the power, 2017-04-04 14:06:13]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[andypaicu, 2017-04-04 14:15:19]

The CQ bit was checked by andypaicu@chromium.org to run a CQ dry run

[andypaicu, 2017-04-04 14:15:28]

https://codereview.chromium.org/2795673002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.h (right):

https://codereview.chromium.org/2795673002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.h:100: SecurityOrigin*
frameSecurityOrigin);
On 2017/04/04 at 07:53:21, dcheng wrote:
> Please clang-format the cl
> 
> (the presubmit should have warned about formatting, but it won't actually
prevent you from uploading it/committing it)

Good catch. My presubmit did not raise any warnings about this and it turns out
git cl format was throwing a permission exception which was silently hidden
during presubmit checks.

[commit-bot: I haz the power, 2017-04-04 14:15:31]

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-04-04 15:52:49]

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

[commit-bot: I haz the power, 2017-04-04 15:52:50]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-04-04 19:04:15]

LGTM

One concern I have is that this use counter will probably under report: it
doesn't handle cross-process navigations or the fact that there may be
cross-process main window references.

[andypaicu, 2017-04-05 07:11:04]

The CQ bit was checked by andypaicu@chromium.org

[andypaicu, 2017-04-05 07:11:04]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2795673002/#ps100001
(title: "RU")

[commit-bot: I haz the power, 2017-04-05 07:11:23]

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-04-05 07:15:47]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491376264444990,
"parent_rev": "ca04d1cb992560ad509e4497252371327c0cde83", "commit_rev":
"f936f423177349c1840be6e9b83114a844e74fa8"}

[commit-bot: I haz the power, 2017-04-05 07:16:38]

Description was changed from

==========
Added UseCounter for clearing browsing context name on cross-origin name

Added a UseCounter that tracks when a browsing context-name that would
have been cleared because of a main-level cross-origin navigation is
accessed.
Spec: https://html.spec.whatwg.org/multipage/browsers.html#resetBCName
ITI:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/8uZDknA2Ua0/Sm33B4MP...

I have tests ready for this functionality but since this is just a UseCounter
the tests would not really work.

BUG=706350
==========

to

==========
Added UseCounter for clearing browsing context name on cross-origin name

Added a UseCounter that tracks when a browsing context-name that would
have been cleared because of a main-level cross-origin navigation is
accessed.
Spec: https://html.spec.whatwg.org/multipage/browsers.html#resetBCName
ITI:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/8uZDknA2Ua0/Sm33B4MP...

I have tests ready for this functionality but since this is just a UseCounter
the tests would not really work.

BUG=706350

Review-Url: https://codereview.chromium.org/2795673002
Cr-Commit-Position: refs/heads/master@{#461997}
Committed:
https://chromium.googlesource.com/chromium/src/+/f936f423177349c1840be6e9b831...
==========

[commit-bot: I haz the power, 2017-04-05 07:16:39]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/f936f423177349c1840be6e9b831...