Expose RenderFrameHost::GetLastCommittedOrigin().

Expose RenderFrameHost::GetLastCommittedOrigin().

It is an error to call this unless the RFH is current -- similar to
GetLastCommittedURL, but we actually enforce it.

There are two pending CLs that will want to use this: 1413853005 and 1465533008.

BUG=498580
TBR=abarth@chromium.org

Committed: https://crrev.com/1466c8459a40da0077d6e62174c2fec808b9f50f
Cr-Commit-Position: refs/heads/master@{#361722}

[ncarter (slow), 2015-11-23 23:33:07]

nick@chromium.org changed reviewers:
+ alexmos@chromium.org, creis@chromium.org

[alexmos, 2015-11-24 00:05:49]

https://codereview.chromium.org/1475433002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_tree_browsertest.cc (right):

https://codereview.chromium.org/1475433002/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_browsertest.cc:45:
"window.domAutomationController.send(document.domain);", &origin));
Any reason to use this instead of document.origin?  The latter seems more
thorough, as it includes the scheme and would resolve to "null" for unique
origin cases, rather than "".  

Speaking of which, we don't support document.domain changes yet.  I.e.,
url::Origin doesn't track it, and the browser process won't find out about
dynamic document.domain assignments.  Is this ok for the intended use cases?

https://codereview.chromium.org/1475433002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1475433002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:320: return
frame_tree_node_->current_replication_state().origin;
We can now just do frame_tree_node_->current_origin().  Same for tests (Charlie
cleaned that up on Friday).

[ncarter (slow), 2015-11-24 00:35:33]

https://codereview.chromium.org/1475433002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_tree_browsertest.cc (right):

https://codereview.chromium.org/1475433002/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_browsertest.cc:45:
"window.domAutomationController.send(document.domain);", &origin));
On 2015/11/24 00:05:49, alexmos wrote:
> Any reason to use this instead of document.origin?  The latter seems more
> thorough, as it includes the scheme and would resolve to "null" for unique
> origin cases, rather than "".  
> 
> Speaking of which, we don't support document.domain changes yet.  I.e.,
> url::Origin doesn't track it, and the browser process won't find out about
> dynamic document.domain assignments.  Is this ok for the intended use cases?

Good point; I'll switch to document.origin. I originally implemented this using
window.location.origin (which isn't right for about:blank) and then got it
working with document.domain because I forgot I could look at document.origin.

https://codereview.chromium.org/1475433002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1475433002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:320: return
frame_tree_node_->current_replication_state().origin;
On 2015/11/24 00:05:49, alexmos wrote:
> We can now just do frame_tree_node_->current_origin().  Same for tests
(Charlie
> cleaned that up on Friday).

Will do.

[ncarter (slow), 2015-11-24 06:04:12]

Updated; PTAL

[alexmos, 2015-11-24 18:23:48]

Thanks, LGTM with nits below.

https://codereview.chromium.org/1475433002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_browsertest.cc (right):

https://codereview.chromium.org/1475433002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:41: std::string
GetEffectiveOriginFromRenderer(FrameTreeNode* node) {
Nit: can this just be GetOriginFromRenderer?  "Effective origin" sounds like
something that reflects document.domain changes, and document.origin doesn't.

https://codereview.chromium.org/1475433002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:217: std::string
scraped_origin;
This looks unused.

https://codereview.chromium.org/1475433002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1475433002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:322: return
frame_tree_node_->current_replication_state().origin;
Nit: s/current_replication_state().origin/current_origin()/

https://codereview.chromium.org/1475433002/diff/40001/content/public/browser/...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/1475433002/diff/40001/content/public/browser/...
content/public/browser/render_frame_host.h:82: // Returns the last committed
origin of the frame.
Is it worth adding some sort of note here that it's incorrect to call this on a
non-current RFH?

[ncarter (slow), 2015-11-24 19:35:08]

https://codereview.chromium.org/1475433002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_browsertest.cc (right):

https://codereview.chromium.org/1475433002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:41: std::string
GetEffectiveOriginFromRenderer(FrameTreeNode* node) {
On 2015/11/24 18:23:48, alexmos wrote:
> Nit: can this just be GetOriginFromRenderer?  "Effective origin" sounds like
> something that reflects document.domain changes, and document.origin doesn't.

Done.

https://codereview.chromium.org/1475433002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:217: std::string
scraped_origin;
On 2015/11/24 18:23:48, alexmos wrote:
> This looks unused.

Done.

https://codereview.chromium.org/1475433002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1475433002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:322: return
frame_tree_node_->current_replication_state().origin;
On 2015/11/24 18:23:48, alexmos wrote:
> Nit: s/current_replication_state().origin/current_origin()/

Done.

https://codereview.chromium.org/1475433002/diff/40001/content/public/browser/...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/1475433002/diff/40001/content/public/browser/...
content/public/browser/render_frame_host.h:82: // Returns the last committed
origin of the frame.
On 2015/11/24 18:23:48, alexmos wrote:
> Is it worth adding some sort of note here that it's incorrect to call this on
a
> non-current RFH?

Done.

[Charlie Reis, 2015-11-24 21:00:43]

Thanks for linking to the CLs that need it.  LGTM with nits.

https://codereview.chromium.org/1475433002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_browsertest.cc (right):

https://codereview.chromium.org/1475433002/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:217: GURL
about_blank("about:blank");
nit: url::kAboutBlankURL

https://codereview.chromium.org/1475433002/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:239:
root->child_at(0)->current_frame_host()->GetLastCommittedOrigin()));
Can you mention something in the comment above about what's in this iframe (or
include an EXPECT_EQ url check for it)?  It's not obvious from reading the test
why we expect it to be same-origin, unless you go and open frame_tree/top.html.

https://codereview.chromium.org/1475433002/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:246:
EXPECT_EQ(root->child_at(0)->current_url(), frame_url);
nit: Swap order (expected, actual).

https://codereview.chromium.org/1475433002/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:259:
EXPECT_EQ(root->child_at(0)->current_url(), about_blank);
nit: Swap order.

https://codereview.chromium.org/1475433002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1475433002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:320: return url::Origin();
Sanity check: Since there aren't callers yet, do we want to go further and
enforce this with a CHECK?  Returning an empty Origin as a fallback seems kind
of safe (certainly better than returning some other frame's origin), but I'm not
sure what implications it will have in practice.

https://codereview.chromium.org/1475433002/diff/60001/url/origin.h
File url/origin.h (right):

https://codereview.chromium.org/1475433002/diff/60001/url/origin.h#newcode120
url/origin.h:120: // Allows Origin to used as a key in STL (for example, a
std::set or
nit: to be used

[ncarter (slow), 2015-11-24 22:06:21]

Description was changed from

==========
Expose RenderFrameHost::GetLastCommittedOrigin().

It is an error to call this unless the RFH is current (similar to
GetLastCommittedURL)

There are two pending CLs that will want to use this: 1413853005 and 1465533008.

BUG=498580
==========

to

==========
Expose RenderFrameHost::GetLastCommittedOrigin().

It is an error to call this unless the RFH is current -- similar to
GetLastCommittedURL, but we actually enforce it.

There are two pending CLs that will want to use this: 1413853005 and 1465533008.

BUG=498580
==========

[ncarter (slow), 2015-11-24 22:07:33]

nick@chromium.org changed reviewers:
+ abarth@chromium.org

[ncarter (slow), 2015-11-24 22:07:34]

abarth: please have a look at the url/ changes, which are comment-only

https://codereview.chromium.org/1475433002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_browsertest.cc (right):

https://codereview.chromium.org/1475433002/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:217: GURL
about_blank("about:blank");
On 2015/11/24 21:00:42, Charlie Reis wrote:
> nit: url::kAboutBlankURL

Done.

https://codereview.chromium.org/1475433002/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:239:
root->child_at(0)->current_frame_host()->GetLastCommittedOrigin()));
On 2015/11/24 21:00:42, Charlie Reis wrote:
> Can you mention something in the comment above about what's in this iframe (or
> include an EXPECT_EQ url check for it)?  It's not obvious from reading the
test
> why we expect it to be same-origin, unless you go and open
frame_tree/top.html.

Done.

https://codereview.chromium.org/1475433002/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:246:
EXPECT_EQ(root->child_at(0)->current_url(), frame_url);
On 2015/11/24 21:00:42, Charlie Reis wrote:
> nit: Swap order (expected, actual).

Done.

https://codereview.chromium.org/1475433002/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:259:
EXPECT_EQ(root->child_at(0)->current_url(), about_blank);
On 2015/11/24 21:00:42, Charlie Reis wrote:
> nit: Swap order.

Done.

https://codereview.chromium.org/1475433002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1475433002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:320: return url::Origin();
On 2015/11/24 21:00:42, Charlie Reis wrote:
> Sanity check: Since there aren't callers yet, do we want to go further and
> enforce this with a CHECK?  Returning an empty Origin as a fallback seems kind
> of safe (certainly better than returning some other frame's origin), but I'm
not
> sure what implications it will have in practice.

Switched to a CHECK based on our face-to-face discussion. This is the sort of
case that makes me wish we had an UMA-logged NOTREACHED().

https://codereview.chromium.org/1475433002/diff/60001/url/origin.h
File url/origin.h (right):

https://codereview.chromium.org/1475433002/diff/60001/url/origin.h#newcode120
url/origin.h:120: // Allows Origin to used as a key in STL (for example, a
std::set or
On 2015/11/24 21:00:42, Charlie Reis wrote:
> nit: to be used

Done.

[ncarter (slow), 2015-11-25 19:19:00]

TBR'ing abarth since the changes to url are trivial comment fixes.

[ncarter (slow), 2015-11-25 19:19:12]

Description was changed from

==========
Expose RenderFrameHost::GetLastCommittedOrigin().

It is an error to call this unless the RFH is current -- similar to
GetLastCommittedURL, but we actually enforce it.

There are two pending CLs that will want to use this: 1413853005 and 1465533008.

BUG=498580
==========

to

==========
Expose RenderFrameHost::GetLastCommittedOrigin().

It is an error to call this unless the RFH is current -- similar to
GetLastCommittedURL, but we actually enforce it.

There are two pending CLs that will want to use this: 1413853005 and 1465533008.

BUG=498580
TBR=abarth@chromium.org
==========

[ncarter (slow), 2015-11-25 19:19:23]

The CQ bit was checked by nick@chromium.org

[ncarter (slow), 2015-11-25 19:19:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexmos@chromium.org, creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1475433002/#ps80001
(title: "Charlie's fixes.")

[commit-bot: I haz the power, 2015-11-25 19:20:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1475433002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1475433002/80001

[commit-bot: I haz the power, 2015-11-25 20:08:12]

Description was changed from

==========
Expose RenderFrameHost::GetLastCommittedOrigin().

It is an error to call this unless the RFH is current -- similar to
GetLastCommittedURL, but we actually enforce it.

There are two pending CLs that will want to use this: 1413853005 and 1465533008.

BUG=498580
TBR=abarth@chromium.org
==========

to

==========
Expose RenderFrameHost::GetLastCommittedOrigin().

It is an error to call this unless the RFH is current -- similar to
GetLastCommittedURL, but we actually enforce it.

There are two pending CLs that will want to use this: 1413853005 and 1465533008.

BUG=498580
TBR=abarth@chromium.org
==========

[commit-bot: I haz the power, 2015-11-25 20:08:13]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-25 20:09:17]

Description was changed from

==========
Expose RenderFrameHost::GetLastCommittedOrigin().

It is an error to call this unless the RFH is current -- similar to
GetLastCommittedURL, but we actually enforce it.

There are two pending CLs that will want to use this: 1413853005 and 1465533008.

BUG=498580
TBR=abarth@chromium.org
==========

to

==========
Expose RenderFrameHost::GetLastCommittedOrigin().

It is an error to call this unless the RFH is current -- similar to
GetLastCommittedURL, but we actually enforce it.

There are two pending CLs that will want to use this: 1413853005 and 1465533008.

BUG=498580
TBR=abarth@chromium.org

Committed: https://crrev.com/1466c8459a40da0077d6e62174c2fec808b9f50f
Cr-Commit-Position: refs/heads/master@{#361722}
==========

[commit-bot: I haz the power, 2015-11-25 20:09:18]

Patchset 5 (id:??) landed as
https://crrev.com/1466c8459a40da0077d6e62174c2fec808b9f50f
Cr-Commit-Position: refs/heads/master@{#361722}