Propagate scrolling/marginwidth/marginheight property values to child frame.

Propagate scrolling/marginwidth/marginheight property values to child frame.

Initially when a frame is created with FrameHostMsg_CreateChildFrame, we
send all of these properties in WebFrameOwnerProperties struct. Later on,
if any of these properties change, upon receiving a
didChangeFrameOwnerProperties message from blink, we update our FTN data
structure. Once the browser process sees such a message, it notifies the
child renderer process with FrameMsg_SetFrameOwnerProperties. The renderer
then would store them as the frame's FrameOwner properties.

The blink part of this CL started here:
https://chromiumcodereview.appspot.com/1319863006/
and it was later merged here. Keeping the reference for initial
codereview comments.
BUG=524725
Committed: https://crrev.com/70605c32bd6a79317df00114ec556f87a7bd8afd
Cr-Commit-Position: refs/heads/master@{#357503}

[alexmos, 2015-09-02 21:42:08]

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

[alexmos, 2015-09-02 21:42:09]

Here are some initial comments for this as well, since I looked through it as I
was looking at your Blink-side CL.  Also, it'd be great to have browser tests
for this.

https://chromiumcodereview.appspot.com/1307013004/diff/80001/content/browser/...
File content/browser/frame_host/frame_tree_node.h (right):

https://chromiumcodereview.appspot.com/1307013004/diff/80001/content/browser/...
content/browser/frame_host/frame_tree_node.h:293: // navigation.
It sounds like taking effect on next navigation is not actually desired, but it
keeps us consistent with Blink's behavior today.  Is that correct?  Should we
add a note or TODO about that here?

https://chromiumcodereview.appspot.com/1307013004/diff/80001/content/browser/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://chromiumcodereview.appspot.com/1307013004/diff/80001/content/browser/...
content/browser/frame_host/render_frame_host_impl.cc:1332: 
How about adding a security check here that it's the child's parent that sent
this message, and if not, kill the renderer?  See OnDidChangeSandboxFlags above.
 Also, maybe the code to get and verify the child frame can be grouped into a
common helper function for your three new functions and OnDidChangeSandboxFlags.

https://chromiumcodereview.appspot.com/1307013004/diff/80001/content/browser/...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://chromiumcodereview.appspot.com/1307013004/diff/80001/content/browser/...
content/browser/frame_host/render_frame_host_impl.h:218: const
blink::WebFrameOwnerProperties& frame_owner_properties);
I think you need to update some unit tests as well.  E.g.,
frame_tree_unittest.cc uses OnCreateChildFrame.

https://chromiumcodereview.appspot.com/1307013004/diff/80001/content/renderer...
File content/renderer/render_frame_impl.cc (right):

https://chromiumcodereview.appspot.com/1307013004/diff/80001/content/renderer...
content/renderer/render_frame_impl.cc:620:
web_frame->setFrameOwnerProperties(frame_owner_properties);
This will transfer updated properties for cross-site frame navigations, but what
about same-site frame navigations?  I.e., how will this work: A embeds B, A
updates margins, B navigates to B2.  B2 presumably needs to use the updated
margins, but I don't see how that would work, since you don't forward the
updates right away in your new OnDidChange* functions.

https://chromiumcodereview.appspot.com/1307013004/diff/80001/content/renderer...
content/renderer/render_frame_impl.cc:629:
web_frame->initializeToReplaceRemoteFrame(
This remote-to-local path might need to call setFrameOwnerProperties as well. 
For example, if you have A-embed-B-embed-C, B updates the properties, and C
navigates to A, it will need the latest properties here, right?  Would be good
to have a test for this eventually.

https://chromiumcodereview.appspot.com/1307013004/diff/80001/content/renderer...
File content/renderer/render_thread_impl.cc (right):

https://chromiumcodereview.appspot.com/1307013004/diff/80001/content/renderer...
content/renderer/render_thread_impl.cc:1632: params.frame_owner_properties);
Heads-up: this will conflict with https://codereview.chromium.org/1312643002/
which I'll land soon.

[lazyboy, 2015-09-15 19:30:56]

Forgot to publish this one.

https://chromiumcodereview.appspot.com/1307013004/diff/80001/content/browser/...
File content/browser/frame_host/frame_tree_node.h (right):

https://chromiumcodereview.appspot.com/1307013004/diff/80001/content/browser/...
content/browser/frame_host/frame_tree_node.h:293: // navigation.
On 2015/09/02 21:42:09, alexmos wrote:
> It sounds like taking effect on next navigation is not actually desired, but
it
> keeps us consistent with Blink's behavior today.  Is that correct?  Should we
> add a note or TODO about that here?

Added TODO.

https://chromiumcodereview.appspot.com/1307013004/diff/80001/content/browser/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://chromiumcodereview.appspot.com/1307013004/diff/80001/content/browser/...
content/browser/frame_host/render_frame_host_impl.cc:1332: 
On 2015/09/02 21:42:09, alexmos wrote:
> How about adding a security check here that it's the child's parent that sent
> this message, and if not, kill the renderer?  See OnDidChangeSandboxFlags
above.
>  Also, maybe the code to get and verify the child frame can be grouped into a
> common helper function for your three new functions and
OnDidChangeSandboxFlags.

Done.

https://chromiumcodereview.appspot.com/1307013004/diff/80001/content/browser/...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://chromiumcodereview.appspot.com/1307013004/diff/80001/content/browser/...
content/browser/frame_host/render_frame_host_impl.h:218: const
blink::WebFrameOwnerProperties& frame_owner_properties);
On 2015/09/02 21:42:09, alexmos wrote:
> I think you need to update some unit tests as well.  E.g.,
> frame_tree_unittest.cc uses OnCreateChildFrame.

Done.

https://chromiumcodereview.appspot.com/1307013004/diff/80001/content/renderer...
File content/renderer/render_frame_impl.cc (right):

https://chromiumcodereview.appspot.com/1307013004/diff/80001/content/renderer...
content/renderer/render_frame_impl.cc:620:
web_frame->setFrameOwnerProperties(frame_owner_properties);
On 2015/09/02 21:42:09, alexmos wrote:
> This will transfer updated properties for cross-site frame navigations, but
what
> about same-site frame navigations?  I.e., how will this work: A embeds B, A
> updates margins, B navigates to B2.  B2 presumably needs to use the updated
> margins, but I don't see how that would work,
You're right, this case didn't use to work.
since you don't forward the
> updates right away in your new OnDidChange* functions.

I've started to send the updates right away in OnDidChange* functions.

https://chromiumcodereview.appspot.com/1307013004/diff/80001/content/renderer...
content/renderer/render_frame_impl.cc:629:
web_frame->initializeToReplaceRemoteFrame(
On 2015/09/02 21:42:09, alexmos wrote:
> This remote-to-local path might need to call setFrameOwnerProperties as well. 
> For example, if you have A-embed-B-embed-C, B updates the properties, and C
> navigates to A, it will need the latest properties here, right?  Would be good
> to have a test for this eventually.

Done.

https://chromiumcodereview.appspot.com/1307013004/diff/80001/content/renderer...
File content/renderer/render_thread_impl.cc (right):

https://chromiumcodereview.appspot.com/1307013004/diff/80001/content/renderer...
content/renderer/render_thread_impl.cc:1632: params.frame_owner_properties);
On 2015/09/02 21:42:09, alexmos wrote:
> Heads-up: this will conflict with https://codereview.chromium.org/1312643002/
> which I'll land soon.

Acknowledged.

[alexmos, 2015-09-21 18:53:22]

Great, just a few nits below.  Looking forward to having tests for this.

https://codereview.chromium.org/1307013004/diff/140001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1307013004/diff/140001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:227: frame_owner_properties_ =
frame_owner_properties;
nit: if there's no more work beyond the assignment, this can probably become
set_frame_owner_properties() and go into the header.

https://codereview.chromium.org/1307013004/diff/140001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1307013004/diff/140001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.h:289: // properties affect the child
renderer but are available on its parent's
nit: replace "available" with "maintained" or "stored"?

https://codereview.chromium.org/1307013004/diff/140001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.h:291: // we store it here.
nit: When this frame's parent dynamically updates these properties, we store
them here.

https://codereview.chromium.org/1307013004/diff/140001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.h:293: // due to the limitation of
current blink implementation. Fix.
nit: I'd say "to be consistent with the current Blink implementation" and maybe
mention that if the Blink behavior changes this will need to be fixed as well. 
Also, maybe a better place for this TODO is on
WebFrame::setFrameOwnerProperties?  Browser side already forwards the updates
right away, so it's the Blink side that would need to do something differently
to apply them.

https://codereview.chromium.org/1307013004/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1307013004/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1312: FrameTreeNode* child
= CheckAndGetIfImmediateChild(
I'm wondering if there's a better name for this.  It's not clear what the
"Check" refers to (though the comment helps), but I don't have any better
ideas...

https://codereview.chromium.org/1307013004/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1307013004/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:621: FrameTreeNode*
CheckAndGetIfImmediateChild(
Maybe this fits better in the anonymous namespace in render_frame_host_impl.cc?

[lazyboy, 2015-09-22 02:38:54]

I've started adding tests. So far I've covered marginwidth/marginheight with all
possible transitions between remote and local frame. I haven't settled on how to
verify scrollbars in the test yet, but hopefully will come up /w something
tomorrow.

Since blink/chromium repo merge is happening soon, I should probably aim for
that to happen and not worry about splitting these CLs to make multiple repo
builds happy (:, WDYT?

https://chromiumcodereview.appspot.com/1307013004/diff/140001/content/browser...
File content/browser/frame_host/frame_tree_node.cc (right):

https://chromiumcodereview.appspot.com/1307013004/diff/140001/content/browser...
content/browser/frame_host/frame_tree_node.cc:227: frame_owner_properties_ =
frame_owner_properties;
On 2015/09/21 18:53:21, alexmos wrote:
> nit: if there's no more work beyond the assignment, this can probably become
> set_frame_owner_properties() and go into the header.

Done.
Ah, I started /w more work beyond assignment... then this simplified to a
setter.

https://chromiumcodereview.appspot.com/1307013004/diff/140001/content/browser...
File content/browser/frame_host/frame_tree_node.h (right):

https://chromiumcodereview.appspot.com/1307013004/diff/140001/content/browser...
content/browser/frame_host/frame_tree_node.h:289: // properties affect the child
renderer but are available on its parent's
On 2015/09/21 18:53:21, alexmos wrote:
> nit: replace "available" with "maintained" or "stored"?

Done.

https://chromiumcodereview.appspot.com/1307013004/diff/140001/content/browser...
content/browser/frame_host/frame_tree_node.h:291: // we store it here.
On 2015/09/21 18:53:21, alexmos wrote:
> nit: When this frame's parent dynamically updates these properties, we store
> them here.

Done.

https://chromiumcodereview.appspot.com/1307013004/diff/140001/content/browser...
content/browser/frame_host/frame_tree_node.h:293: // due to the limitation of
current blink implementation. Fix.
On 2015/09/21 18:53:21, alexmos wrote:
> nit: I'd say "to be consistent with the current Blink implementation" and
maybe
> mention that if the Blink behavior changes this will need to be fixed as well.

> Also, maybe a better place for this TODO is on
> WebFrame::setFrameOwnerProperties?  Browser side already forwards the updates
> right away, so it's the Blink side that would need to do something differently
> to apply them.

Done.

https://chromiumcodereview.appspot.com/1307013004/diff/140001/content/browser...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://chromiumcodereview.appspot.com/1307013004/diff/140001/content/browser...
content/browser/frame_host/render_frame_host_impl.cc:1312: FrameTreeNode* child
= CheckAndGetIfImmediateChild(
On 2015/09/21 18:53:22, alexmos wrote:
> I'm wondering if there's a better name for this.  It's not clear what the
> "Check" refers to (though the comment helps), but I don't have any better
> ideas...

EnsureIfImmediateChildAndReturn? or maybe just drop the AndReturn part?
The return is obvious from return type...

https://chromiumcodereview.appspot.com/1307013004/diff/140001/content/browser...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://chromiumcodereview.appspot.com/1307013004/diff/140001/content/browser...
content/browser/frame_host/render_frame_host_impl.h:621: FrameTreeNode*
CheckAndGetIfImmediateChild(
On 2015/09/21 18:53:22, alexmos wrote:
> Maybe this fits better in the anonymous namespace in
render_frame_host_impl.cc?

I still need |this|, do you mean to also pass that RFH* or FrameTreeNode* param
to this function?

[alexmos, 2015-09-22 16:56:53]

On 2015/09/22 02:38:54, lazyboy wrote:
> I've started adding tests. So far I've covered marginwidth/marginheight with
all
> possible transitions between remote and local frame. I haven't settled on how
to
> verify scrollbars in the test yet, but hopefully will come up /w something
> tomorrow.
> 
> Since blink/chromium repo merge is happening soon, I should probably aim for
> that to happen and not worry about splitting these CLs to make multiple repo
> builds happy (:, WDYT?

Sounds good, we're not in a hurry to land these before tomorrow, so might as
well save the effort of splitting up the CLs.

Let me know when the content-side tests are ready to look at.

https://chromiumcodereview.appspot.com/1307013004/diff/140001/content/browser...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://chromiumcodereview.appspot.com/1307013004/diff/140001/content/browser...
content/browser/frame_host/render_frame_host_impl.cc:1312: FrameTreeNode* child
= CheckAndGetIfImmediateChild(
On 2015/09/22 02:38:54, lazyboy wrote:
> On 2015/09/21 18:53:22, alexmos wrote:
> > I'm wondering if there's a better name for this.  It's not clear what the
> > "Check" refers to (though the comment helps), but I don't have any better
> > ideas...
> 
> EnsureIfImmediateChildAndReturn? or maybe just drop the AndReturn part?
> The return is obvious from return type...

I actually like what you currently have a bit more. :)  Charlie may have some
better ideas about the name.

https://chromiumcodereview.appspot.com/1307013004/diff/140001/content/browser...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://chromiumcodereview.appspot.com/1307013004/diff/140001/content/browser...
content/browser/frame_host/render_frame_host_impl.h:621: FrameTreeNode*
CheckAndGetIfImmediateChild(
On 2015/09/22 02:38:54, lazyboy wrote:
> On 2015/09/21 18:53:22, alexmos wrote:
> > Maybe this fits better in the anonymous namespace in
> render_frame_host_impl.cc?
> 
> I still need |this|, do you mean to also pass that RFH* or FrameTreeNode*
param
> to this function?

Ah, right.  It's fine to keep it where it is then.

[lazyboy, 2015-09-22 19:58:38]

Hi Alex, I've finished adding the browsertests in Patch Set #11.

[alexmos, 2015-09-23 22:38:27]

Thanks for adding tests!  Some comments on them below.

https://codereview.chromium.org/1307013004/diff/220001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1307013004/diff/220001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1843: const int
FRAME_ELEMENT_WIDTH = 200;
I wonder if there's an easier way to check if the scrollbar is present, rather
than hardcoding the width and using a threshold.  E.g., looking at
StackOverflow, this JS snippet may work?  
  var hasScrollbar = document.body.scrollHeight > document.body.clientHeight;

https://codereview.chromium.org/1307013004/diff/220001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1852:
"window.domAutomationController.send("
I don't think ExecuteScript needs the domAutomationController.send (see comment
on
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...)

https://codereview.chromium.org/1307013004/diff/220001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1857: GURL urls[4] = {
nit: can omit array size, here and below.

https://codereview.chromium.org/1307013004/diff/220001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1863:
embedded_test_server()->GetURL("a.com", "/tall_page2.html"),
Do we really need to test the local-to-local case?

https://codereview.chromium.org/1307013004/diff/220001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1865:
embedded_test_server()->GetURL("b.com", "/tall_page2.html")
Can we get away with using just one tall page?  Does navigating to same URL
still update these properties? (I'm assuming so, since the very first navigation
is from b.com/tall_page1.html to b.com/tall_page1.html, and that works?)

https://codereview.chromium.org/1307013004/diff/220001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1879: EXPECT_TRUE(
nit: line break doesn't look necessary.

https://codereview.chromium.org/1307013004/diff/220001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1944:
"window.domAutomationController.send("
See earlier comment about domAutomationController.send not being necessary (here
and below)

https://codereview.chromium.org/1307013004/diff/220001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1954: // Navigate.
nit: no need for this comment.  Instead, maybe explain at a high level what's
going to happen in a comment on the for loop above.

https://codereview.chromium.org/1307013004/diff/220001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1958: EXPECT_TRUE(
nit: no line break

https://codereview.chromium.org/1307013004/diff/220001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1970:
EXPECT_TRUE(ExecuteScriptAndExtractString(
Does ExecuteScriptAndExtractInt not work for these?

[lazyboy, 2015-09-30 23:37:55]

(I've updated the CL to contain both blink and chromium changes, note that
updating CL description is pending).

https://chromiumcodereview.appspot.com/1307013004/diff/220001/content/browser...
File content/browser/site_per_process_browsertest.cc (right):

https://chromiumcodereview.appspot.com/1307013004/diff/220001/content/browser...
content/browser/site_per_process_browsertest.cc:1843: const int
FRAME_ELEMENT_WIDTH = 200;
On 2015/09/23 22:38:27, alexmos wrote:
> I wonder if there's an easier way to check if the scrollbar is present, rather
> than hardcoding the width and using a threshold.  E.g., looking at
> StackOverflow, this JS snippet may work?  
>   var hasScrollbar = document.body.scrollHeight > document.body.clientHeight;

That gives us whether there's any content to scroll, which would always be true
for the test. We want to check whether we are
actually showing a scrollbar or not.

https://chromiumcodereview.appspot.com/1307013004/diff/220001/content/browser...
content/browser/site_per_process_browsertest.cc:1852:
"window.domAutomationController.send("
On 2015/09/23 22:38:27, alexmos wrote:
> I don't think ExecuteScript needs the domAutomationController.send (see
comment
> on
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...)

Done.

https://chromiumcodereview.appspot.com/1307013004/diff/220001/content/browser...
content/browser/site_per_process_browsertest.cc:1857: GURL urls[4] = {
On 2015/09/23 22:38:27, alexmos wrote:
> nit: can omit array size, here and below.

Done.

https://chromiumcodereview.appspot.com/1307013004/diff/220001/content/browser...
content/browser/site_per_process_browsertest.cc:1863:
embedded_test_server()->GetURL("a.com", "/tall_page2.html"),
On 2015/09/23 22:38:27, alexmos wrote:
> Do we really need to test the local-to-local case?

Removed local to local case.

https://chromiumcodereview.appspot.com/1307013004/diff/220001/content/browser...
content/browser/site_per_process_browsertest.cc:1865:
embedded_test_server()->GetURL("b.com", "/tall_page2.html")
On 2015/09/23 22:38:27, alexmos wrote:
> Can we get away with using just one tall page?  Does navigating to same URL
Using one tall_page.html
> still update these properties? (I'm assuming so, since the very first
navigation
> is from b.com/tall_page1.html to b.com/tall_page1.html, and that works?)
That passes the test, but I meant to actually change the path. I've changed it
to go from b.com/title1.html to b.com/tall_page.html

https://chromiumcodereview.appspot.com/1307013004/diff/220001/content/browser...
content/browser/site_per_process_browsertest.cc:1879: EXPECT_TRUE(
On 2015/09/23 22:38:27, alexmos wrote:
> nit: line break doesn't look necessary.

Done.

https://chromiumcodereview.appspot.com/1307013004/diff/220001/content/browser...
content/browser/site_per_process_browsertest.cc:1944:
"window.domAutomationController.send("
On 2015/09/23 22:38:27, alexmos wrote:
> See earlier comment about domAutomationController.send not being necessary
(here
> and below)

Done.

https://chromiumcodereview.appspot.com/1307013004/diff/220001/content/browser...
content/browser/site_per_process_browsertest.cc:1954: // Navigate.
On 2015/09/23 22:38:27, alexmos wrote:
> nit: no need for this comment.  Instead, maybe explain at a high level what's
> going to happen in a comment on the for loop above.

Done.

https://chromiumcodereview.appspot.com/1307013004/diff/220001/content/browser...
content/browser/site_per_process_browsertest.cc:1958: EXPECT_TRUE(
On 2015/09/23 22:38:27, alexmos wrote:
> nit: no line break

Done.

https://chromiumcodereview.appspot.com/1307013004/diff/220001/content/browser...
content/browser/site_per_process_browsertest.cc:1970:
EXPECT_TRUE(ExecuteScriptAndExtractString(
On 2015/09/23 22:38:27, alexmos wrote:
> Does ExecuteScriptAndExtractInt not work for these?

No. getAttribute returns string.

[alexmos, 2015-10-02 21:24:20]

Great, thanks for making progress on this!  Just a few more nits and comments
below.

Please also update the CL description.  E.g., remove (cr) from title, change
didChangeX to actual name, and maybe also add something about how these
properties are forwarded to the renderer right away, and how they are processed
there.  Probably keep the Blink CL link, but mention that it has initial
comments for the Blink part and was later merged here.

I see also that you added html_viewer changes in the latest patchset.  I didn't
review them closely, as I'm not familiar with how they do conversions, etc --
it'd be best to find someone who is.  Is there a way to split most of that off
into a separate CL, as this one is getting very long?

https://codereview.chromium.org/1307013004/diff/220001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1307013004/diff/220001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1843: const int
FRAME_ELEMENT_WIDTH = 200;
On 2015/09/30 23:37:54, lazyboy wrote:
> On 2015/09/23 22:38:27, alexmos wrote:
> > I wonder if there's an easier way to check if the scrollbar is present,
rather
> > than hardcoding the width and using a threshold.  E.g., looking at
> > StackOverflow, this JS snippet may work?  
> >   var hasScrollbar = document.body.scrollHeight >
document.body.clientHeight;
> 
> That gives us whether there's any content to scroll, which would always be
true
> for the test. We want to check whether we are
> actually showing a scrollbar or not.

Acknowledged.  Perhaps explain the need for threshold in a comment?

https://codereview.chromium.org/1307013004/diff/220001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1970:
EXPECT_TRUE(ExecuteScriptAndExtractString(
On 2015/09/30 23:37:54, lazyboy wrote:
> On 2015/09/23 22:38:27, alexmos wrote:
> > Does ExecuteScriptAndExtractInt not work for these?
> 
> No. getAttribute returns string.

Acknowledged.

https://codereview.chromium.org/1307013004/diff/240001/components/html_viewer...
File components/html_viewer/html_frame_properties.cc (right):

https://codereview.chromium.org/1307013004/diff/240001/components/html_viewer...
components/html_viewer/html_frame_properties.cc:161: bool
FrameOwnerPropertiesFromClientProperty(
Should we have unit tests for this and FrameOwnerPropertiesToClientProperty?

https://codereview.chromium.org/1307013004/diff/240001/components/html_viewer...
File components/html_viewer/replicated_frame_state.h (right):

https://codereview.chromium.org/1307013004/diff/240001/components/html_viewer...
components/html_viewer/replicated_frame_state.h:29:
blink::WebFrameOwnerProperties owner_properties;
I'm not really familiar with html_viewer, but I'm wondering why these properties
are included here?  This looks similar to content's FrameReplicationState, and
we didn't include these properties there, as we didn't want them to be
replicated for all proxies, right?

https://codereview.chromium.org/1307013004/diff/240001/components/printing/re...
File components/printing/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/1307013004/diff/240001/components/printing/re...
components/printing/renderer/print_web_view_helper.cc:601: virtual void
frameDetached(blink::WebFrame* frame, DetachType type);
why the override->virtual change on frameDetached?

https://codereview.chromium.org/1307013004/diff/240001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1307013004/diff/240001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.h:293: // properties, we store them
here.
nit: last sentence makes it sound like the properties are only stored on dynamic
updates, but they're also stored when the child frame is first created.

https://codereview.chromium.org/1307013004/diff/240001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.h:295: // Note that the update only
takes effect on the next frame navigation,
nit: s/the update/dynamic updates/, s/takes/take/.  I'd also remove "to be
consistent...", since the code here doesn't do anything to be consistent with
Blink -- that happens on the Blink side in WebFrame::setFrameOwnerProperties.

https://codereview.chromium.org/1307013004/diff/240001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1307013004/diff/240001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1334: int32
child_frame_routing_id, bad_message::BadMessageReason reason) {
nit: second parameter on separate line?

https://codereview.chromium.org/1307013004/diff/240001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1359:
child_rfh->GetRoutingID(), child->frame_owner_properties()));
nit: why not just pass |frame_owner_properties|?

https://codereview.chromium.org/1307013004/diff/240001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1307013004/diff/240001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:255: void
DidChangeChildFrameOwnerProperties(FrameTreeNode* child);
What is this function for?  I don't see it used anywhere.

https://codereview.chromium.org/1307013004/diff/240001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1307013004/diff/240001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1839: // Verify that scrolling
property propagates to child frames correctly.
nit: s/scrolling property/the "scrolling" property on frame elements/

https://codereview.chromium.org/1307013004/diff/240001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1957: // Before each navigation,
we change the marginwidth and marginheight
nit: blank line before comment?

https://codereview.chromium.org/1307013004/diff/240001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1307013004/diff/240001/content/common/frame_m...
content/common/frame_messages.h:364:
IPC_STRUCT_MEMBER(blink::WebFrameOwnerProperties, frame_owner_properties)
Following the pattern for other members, let's add a comment explaining what
this is. (And maybe also why it's not part of replication_state.)

https://codereview.chromium.org/1307013004/diff/240001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1307013004/diff/240001/content/renderer/rende...
content/renderer/render_frame_impl.cc:254: int
GetRoutingIDForFrame(blink::WebFrame* child_frame) {
I don't think this has to be specific to child frames, and the function name is
already generic, so |child_frame| can just become |frame|, right?

https://codereview.chromium.org/1307013004/diff/240001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrame.h (right):

https://codereview.chromium.org/1307013004/diff/240001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrame.h:180: // the next navigation.
Nit, remove last sentence, as it's the same as TODO below.

https://codereview.chromium.org/1307013004/diff/240001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrame.h:182: // navigation.
nit: mention that this matches same-process behavior.

https://codereview.chromium.org/1307013004/diff/240001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1307013004/diff/240001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameClient.h:178: // Some frame owner
properties have changed for a child frame of this frame.  Frame owner properties
currently include: scrolling, marginwidth and marginheight.
nit: might look neater with a line break after first sentence.

[lazyboy, 2015-10-05 22:16:09]

Updated the CL description.
Removed html_viewer changes for now, added TODOs there.

https://codereview.chromium.org/1307013004/diff/220001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1307013004/diff/220001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1843: const int
FRAME_ELEMENT_WIDTH = 200;
On 2015/10/02 21:24:19, alexmos wrote:
> On 2015/09/30 23:37:54, lazyboy wrote:
> > On 2015/09/23 22:38:27, alexmos wrote:
> > > I wonder if there's an easier way to check if the scrollbar is present,
> rather
> > > than hardcoding the width and using a threshold.  E.g., looking at
> > > StackOverflow, this JS snippet may work?  
> > >   var hasScrollbar = document.body.scrollHeight >
> document.body.clientHeight;
> > 
> > That gives us whether there's any content to scroll, which would always be
> true
> > for the test. We want to check whether we are
> > actually showing a scrollbar or not.
> 
> Acknowledged.  Perhaps explain the need for threshold in a comment?

Done.

https://codereview.chromium.org/1307013004/diff/240001/components/html_viewer...
File components/html_viewer/html_frame_properties.cc (right):

https://codereview.chromium.org/1307013004/diff/240001/components/html_viewer...
components/html_viewer/html_frame_properties.cc:161: bool
FrameOwnerPropertiesFromClientProperty(
On 2015/10/02 21:24:19, alexmos wrote:
> Should we have unit tests for this and FrameOwnerPropertiesToClientProperty?

I've removed these on the latest patch.

https://codereview.chromium.org/1307013004/diff/240001/components/html_viewer...
File components/html_viewer/replicated_frame_state.h (right):

https://codereview.chromium.org/1307013004/diff/240001/components/html_viewer...
components/html_viewer/replicated_frame_state.h:29:
blink::WebFrameOwnerProperties owner_properties;
On 2015/10/02 21:24:19, alexmos wrote:
> I'm not really familiar with html_viewer, but I'm wondering why these
properties
> are included here?  This looks similar to content's FrameReplicationState, and
I've reverted this change and added a TODO.
> we didn't include these properties there, as we didn't want them to be
> replicated for all proxies, right?
Yes, we do not want to replicated them in remote frames.

https://codereview.chromium.org/1307013004/diff/240001/components/printing/re...
File components/printing/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/1307013004/diff/240001/components/printing/re...
components/printing/renderer/print_web_view_helper.cc:601: virtual void
frameDetached(blink::WebFrame* frame, DetachType type);
On 2015/10/02 21:24:19, alexmos wrote:
> why the override->virtual change on frameDetached?

Think this noise was introduced during merge, fixed.

https://codereview.chromium.org/1307013004/diff/240001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1307013004/diff/240001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.h:293: // properties, we store them
here.
On 2015/10/02 21:24:19, alexmos wrote:
> nit: last sentence makes it sound like the properties are only stored on
dynamic
> updates, but they're also stored when the child frame is first created.

Changed "store" to "update".

https://codereview.chromium.org/1307013004/diff/240001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.h:295: // Note that the update only
takes effect on the next frame navigation,
On 2015/10/02 21:24:19, alexmos wrote:
> nit: s/the update/dynamic updates/, s/takes/take/.  I'd also remove "to be
> consistent...", since the code here doesn't do anything to be consistent with
> Blink -- that happens on the Blink side in WebFrame::setFrameOwnerProperties.

Done.

https://codereview.chromium.org/1307013004/diff/240001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1307013004/diff/240001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1334: int32
child_frame_routing_id, bad_message::BadMessageReason reason) {
On 2015/10/02 21:24:20, alexmos wrote:
> nit: second parameter on separate line?

Done.

https://codereview.chromium.org/1307013004/diff/240001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1359:
child_rfh->GetRoutingID(), child->frame_owner_properties()));
On 2015/10/02 21:24:19, alexmos wrote:
> nit: why not just pass |frame_owner_properties|?

Done.

https://codereview.chromium.org/1307013004/diff/240001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1307013004/diff/240001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:255: void
DidChangeChildFrameOwnerProperties(FrameTreeNode* child);
On 2015/10/02 21:24:20, alexmos wrote:
> What is this function for?  I don't see it used anywhere.

Used to be used on previous iterations, removed.

https://codereview.chromium.org/1307013004/diff/240001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1307013004/diff/240001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1839: // Verify that scrolling
property propagates to child frames correctly.
On 2015/10/02 21:24:20, alexmos wrote:
> nit: s/scrolling property/the "scrolling" property on frame elements/

Done.

https://codereview.chromium.org/1307013004/diff/240001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1957: // Before each navigation,
we change the marginwidth and marginheight
On 2015/10/02 21:24:20, alexmos wrote:
> nit: blank line before comment?

Done.

https://codereview.chromium.org/1307013004/diff/240001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1307013004/diff/240001/content/common/frame_m...
content/common/frame_messages.h:364:
IPC_STRUCT_MEMBER(blink::WebFrameOwnerProperties, frame_owner_properties)
On 2015/10/02 21:24:20, alexmos wrote:
> Following the pattern for other members, let's add a comment explaining what
> this is. (And maybe also why it's not part of replication_state.)

Done.

https://codereview.chromium.org/1307013004/diff/240001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1307013004/diff/240001/content/renderer/rende...
content/renderer/render_frame_impl.cc:254: int
GetRoutingIDForFrame(blink::WebFrame* child_frame) {
On 2015/10/02 21:24:20, alexmos wrote:
> I don't think this has to be specific to child frames, and the function name
is
> already generic, so |child_frame| can just become |frame|, right?

Done.

https://codereview.chromium.org/1307013004/diff/240001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrame.h (right):

https://codereview.chromium.org/1307013004/diff/240001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrame.h:180: // the next navigation.
On 2015/10/02 21:24:20, alexmos wrote:
> Nit, remove last sentence, as it's the same as TODO below.

Done.

https://codereview.chromium.org/1307013004/diff/240001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrame.h:182: // navigation.
On 2015/10/02 21:24:20, alexmos wrote:
> nit: mention that this matches same-process behavior.

Done.

https://codereview.chromium.org/1307013004/diff/240001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1307013004/diff/240001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameClient.h:178: // Some frame owner
properties have changed for a child frame of this frame.  Frame owner properties
currently include: scrolling, marginwidth and marginheight.
On 2015/10/02 21:24:20, alexmos wrote:
> nit: might look neater with a line break after first sentence.

Done.

[alexmos, 2015-10-06 16:43:23]

Thanks, LGTM.

https://codereview.chromium.org/1307013004/diff/260001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1307013004/diff/260001/content/common/frame_m...
content/common/frame_messages.h:365: // properties of the FrameOwnerElement from
the parent process.
nit: there's no class named FrameOwnerElement.  Maybe HTMLFrameOwnerElement or
just "frame element"?

[lazyboy, 2015-10-06 16:49:56]

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

[lazyboy, 2015-10-06 16:49:57]

+Daniel for third_party/WebKit/* changes, note that these
changes are from blink CL that you started looking at:
https://chromiumcodereview.appspot.com/1319863006/

https://codereview.chromium.org/1307013004/diff/260001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1307013004/diff/260001/content/common/frame_m...
content/common/frame_messages.h:365: // properties of the FrameOwnerElement from
the parent process.
On 2015/10/06 16:43:23, alexmos wrote:
> nit: there's no class named FrameOwnerElement.  Maybe HTMLFrameOwnerElement or
> just "frame element"?

Changed to HTMLFrameOwnerElement.

[dcheng, 2015-10-08 06:43:15]

https://codereview.chromium.org/1307013004/diff/300001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1307013004/diff/300001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1338: FrameTreeNode*
RenderFrameHostImpl::CheckAndGetIfImmediateChild(
Nit: I think immediate is a bit superfluous.

https://codereview.chromium.org/1307013004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/1307013004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:197: void
HTMLFrameOwnerElement::setScrollingMode(ScrollbarMode scrollbarMode)
Can you help me understand why we moved this from HTMLFrameElementBase? It seems
to me that plugins didn't use to do this, but now they might.

https://codereview.chromium.org/1307013004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/1307013004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:862: void
FrameLoaderClientImpl::didChangeFrameOwnerProperties(Frame* childFrame,
ScrollbarMode scrollingMode, int marginWidth, int marginHeight)
Instead of passing all these values, maybe just pass a HTMLFrameOwnerElement
here?

[lazyboy, 2015-10-08 17:57:40]

https://codereview.chromium.org/1307013004/diff/300001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1307013004/diff/300001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1338: FrameTreeNode*
RenderFrameHostImpl::CheckAndGetIfImmediateChild(
On 2015/10/08 06:43:15, dcheng wrote:
> Nit: I think immediate is a bit superfluous.

Removed "Immediate".
Done.

https://codereview.chromium.org/1307013004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/1307013004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:197: void
HTMLFrameOwnerElement::setScrollingMode(ScrollbarMode scrollbarMode)
On 2015/10/08 06:43:15, dcheng wrote:
> Can you help me understand why we moved this from HTMLFrameElementBase? It
seems
> to me that plugins didn't use to do this, but now they might.

The scrollingMode() getter is in FrameOwner super class now, so I moved the
setter here (first concrete Implementation of FrameOwner).

The other option is to return default values from scrollingMode() in this class
and override it in HTMLFrameElementBase, I did that in latest patchset, see if
that makes more sense.

https://codereview.chromium.org/1307013004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/1307013004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:862: void
FrameLoaderClientImpl::didChangeFrameOwnerProperties(Frame* childFrame,
ScrollbarMode scrollingMode, int marginWidth, int marginHeight)
On 2015/10/08 06:43:15, dcheng wrote:
> Instead of passing all these values, maybe just pass a HTMLFrameOwnerElement
> here?

Passing HTMLFrameOwnerElementBase*
Done.

[alexmos, 2015-10-09 21:55:50]

https://codereview.chromium.org/1307013004/diff/320001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1307013004/diff/320001/content/renderer/rende...
content/renderer/render_frame_impl.cc:254: int
GetRoutingIDForFrame(blink::WebFrame* frame) {
Heads up: you don't need this anymore, as Nick has just landed a helper that you
can use: https://codereview.chromium.org/1398443004/

[dcheng, 2015-10-12 04:50:01]

In the long run, I wonder if it makes sense to change the abstraction so the
canonical values are stored in the embedder, rather than mirrored from Blink to
the embedder like this. But that's a task for another patch if we go down that
route.

https://codereview.chromium.org/1307013004/diff/320001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1307013004/diff/320001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1868: const int THRESHOLD = 4;
kThreshhold, kFrameElementWidth, etc.

https://codereview.chromium.org/1307013004/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/1307013004/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrame.cpp:146:
toRemoteBridgeFrameOwner(owner)->setScrollingMode(static_cast<ScrollbarMode>(frameOwnerProperties.scrollingMode));
Since all callsites seem to pass in a WebFrameOwnerProperties::ScrollbarMode,
I'd probably just change the argument type and static_cast<> inside the setter.

https://codereview.chromium.org/1307013004/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1307013004/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:1631: EXPECT_EQ("11/22",
content);
Using executeScriptAndReturnValue feels like it'd be a bit more straightforward,
though you'd have to go through some conversions..

https://codereview.chromium.org/1307013004/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:1633: FrameView* frameView
= static_cast<WebLocalFrameImpl*>(localFrame)->frameView();
toWebLocalFrameImpl

https://codereview.chromium.org/1307013004/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:1653:
properties.scrollingMode =
static_cast<WebFrameOwnerProperties::ScrollingMode>(ScrollbarAlwaysOff);
Why not just assign the enum value of the proper type?

https://codereview.chromium.org/1307013004/diff/320001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameOwnerProperties.h (right):

https://codereview.chromium.org/1307013004/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameOwnerProperties.h:1: /*
New files should use the new license block:
https://www.chromium.org/blink/coding-style#TOC-License

(I think using the old license block is causing git to mistakenly detect this as
a copy + edit)

https://codereview.chromium.org/1307013004/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameOwnerProperties.h:55:
WebFrameOwnerProperties(ScrollingMode scrollingMode, int marginWidth, int
marginHeight)
I'd add a #if INSIDE_BLINK constructor that delegates to this one, with the
first constructor taking a blink::ScrollbarMode. This makes it easier for people
calling into this inside Blink.

[lazyboy, 2015-10-13 21:59:08]

dcheng	
1 day, 17 hours ago (2015-10-12 04:50:01 UTC) #18
In the long run, I wonder if it makes sense to change the abstraction so the
canonical values are stored in the embedder, rather than mirrored from Blink to
the embedder like this. But that's a task for another patch if we go down that
route.

OK, do you want me to put a TODO in frame_tree_node.h?

https://codereview.chromium.org/1307013004/diff/320001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1307013004/diff/320001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1868: const int THRESHOLD = 4;
On 2015/10/12 04:50:01, dcheng wrote:
> kThreshhold, kFrameElementWidth, etc.

Done.

https://codereview.chromium.org/1307013004/diff/320001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1307013004/diff/320001/content/renderer/rende...
content/renderer/render_frame_impl.cc:254: int
GetRoutingIDForFrame(blink::WebFrame* frame) {
On 2015/10/09 21:55:49, alexmos wrote:
> Heads up: you don't need this anymore, as Nick has just landed a helper that
you
> can use: https://codereview.chromium.org/1398443004/

Done.

https://codereview.chromium.org/1307013004/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/1307013004/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrame.cpp:146:
toRemoteBridgeFrameOwner(owner)->setScrollingMode(static_cast<ScrollbarMode>(frameOwnerProperties.scrollingMode));
On 2015/10/12 04:50:01, dcheng wrote:
> Since all callsites seem to pass in a WebFrameOwnerProperties::ScrollbarMode,
> I'd probably just change the argument type and static_cast<> inside the
setter.

Done.

https://codereview.chromium.org/1307013004/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1307013004/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:1631: EXPECT_EQ("11/22",
content);
On 2015/10/12 04:50:01, dcheng wrote:
> Using executeScriptAndReturnValue feels like it'd be a bit more
straightforward,
> though you'd have to go through some conversions..

Done.
Can you also check if Utf8Value is the right way to extract string from the
returned v8::Value?

https://codereview.chromium.org/1307013004/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:1633: FrameView* frameView
= static_cast<WebLocalFrameImpl*>(localFrame)->frameView();
On 2015/10/12 04:50:01, dcheng wrote:
> toWebLocalFrameImpl

Done.

https://codereview.chromium.org/1307013004/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:1653:
properties.scrollingMode =
static_cast<WebFrameOwnerProperties::ScrollingMode>(ScrollbarAlwaysOff);
On 2015/10/12 04:50:01, dcheng wrote:
> Why not just assign the enum value of the proper type?

Done.

https://codereview.chromium.org/1307013004/diff/320001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameOwnerProperties.h (right):

https://codereview.chromium.org/1307013004/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameOwnerProperties.h:1: /*
On 2015/10/12 04:50:01, dcheng wrote:
> New files should use the new license block:
> https://www.chromium.org/blink/coding-style#TOC-License
> 
> (I think using the old license block is causing git to mistakenly detect this
as
> a copy + edit)

Done.

https://codereview.chromium.org/1307013004/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameOwnerProperties.h:55:
WebFrameOwnerProperties(ScrollingMode scrollingMode, int marginWidth, int
marginHeight)
On 2015/10/12 04:50:01, dcheng wrote:
> I'd add a #if INSIDE_BLINK constructor that delegates to this one, with the
> first constructor taking a blink::ScrollbarMode. This makes it easier for
people
> calling into this inside Blink.

Done.

[dcheng, 2015-10-14 07:20:43]

On 2015/10/13 at 21:59:08, lazyboy wrote:
> dcheng	
> 1 day, 17 hours ago (2015-10-12 04:50:01 UTC) #18
> In the long run, I wonder if it makes sense to change the abstraction so the
> canonical values are stored in the embedder, rather than mirrored from Blink
to
> the embedder like this. But that's a task for another patch if we go down that
> route.
> 
> OK, do you want me to put a TODO in frame_tree_node.h?
> 

I'm not sure if we want to do that either: you can have an iframe element that
doesn't have a corresponding FrameTreeNode. At the end of the day, I suspect
there's not much you can do: you can read back the value of the property, and it
needs to match what you expect. So I guess you should just consider this random
musing =)

(Also, I'd get a content OWNER to start looking to avoid serializing the reviews
too much)

https://codereview.chromium.org/1307013004/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1307013004/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2011:
RemoteBridgeFrameOwner* remoteOwner = toRemoteBridgeFrameOwner(frame->owner());
Maybe this should just call WebFrame::setReplicatedFrameOwnerProperties?

https://codereview.chromium.org/1307013004/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1307013004/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:1634: v8::Local<v8::Value>
marginWidthResult =
localFrame->executeScriptAndReturnValue(WebScriptSource("bodyMarginWidth"));
document.body.marginwidth directly here? Similar comments elsewhere in this
test.

https://codereview.chromium.org/1307013004/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:1637: EXPECT_EQ("11",
std::string(*widthUtf8, widthUtf8.length()));
I think it might be easier to verify the result with Int32Value?

https://codereview.chromium.org/1307013004/diff/340001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameOwnerProperties.h (right):

https://codereview.chromium.org/1307013004/diff/340001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameOwnerProperties.h:38: :
WebFrameOwnerProperties(static_cast<ScrollingMode>(scrollingMode), marginWidth,
marginHeight)
I don't think anyone else actually calls this constructor. So we should just
merge the initializer list of this overload directly inside the
BLINK_IMPLEMENTATION block.

[dcheng, 2015-10-14 07:57:09]

https://codereview.chromium.org/1307013004/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1307013004/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2011:
RemoteBridgeFrameOwner* remoteOwner = toRemoteBridgeFrameOwner(frame->owner());
On 2015/10/14 at 07:20:43, dcheng wrote:
> Maybe this should just call WebFrame::setReplicatedFrameOwnerProperties?

Also, can we add a comment as to why this is necessary? I assume that this is
because a remote frame doesn't get the notifications that would update these
flags/properties, so we need to sync it up?

If that's the case, then how do we ensure these are still in sync when the frame
is actually swapped in before committing the provisional load?

[lazyboy, 2015-10-14 20:05:28]

lazyboy@chromium.org changed reviewers:
+ creis@chromium.org

[lazyboy, 2015-10-14 20:05:30]

+creis@ for content/ review.

https://codereview.chromium.org/1307013004/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1307013004/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2011:
RemoteBridgeFrameOwner* remoteOwner = toRemoteBridgeFrameOwner(frame->owner());
On 2015/10/14 07:20:43, dcheng wrote:
> Maybe this should just call WebFrame::setReplicatedFrameOwnerProperties?

WebFrame::setFrameOwnerProperties needs core Frame for |frame| which is not set
until line 2021, causes a crash.

https://codereview.chromium.org/1307013004/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2011:
RemoteBridgeFrameOwner* remoteOwner = toRemoteBridgeFrameOwner(frame->owner());
On 2015/10/14 07:57:08, dcheng wrote:
> On 2015/10/14 at 07:20:43, dcheng wrote:
> > Maybe this should just call WebFrame::setReplicatedFrameOwnerProperties?
> 
> Also, can we add a comment as to why this is necessary? I assume that this is
> because a remote frame doesn't get the notifications that would update these
> flags/properties, so we need to sync it up?
Yes.
in A-embed B-embed C scenario, if we navigate C to A's site instance, then
in A's process during remote->local, it doesn't have any idea about the frame
owner properties for the frame. This will ensure we have them right.
Added comment.
> 
> If that's the case, then how do we ensure these are still in sync when the
frame
> is actually swapped in before committing the provisional load?
Can you elaborate this one?

https://codereview.chromium.org/1307013004/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1307013004/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:1634: v8::Local<v8::Value>
marginWidthResult =
localFrame->executeScriptAndReturnValue(WebScriptSource("bodyMarginWidth"));
On 2015/10/14 07:20:43, dcheng wrote:
> document.body.marginwidth directly here? Similar comments elsewhere in this
> test.

Done.
I need document.body.getAttribute('marginwidth') though.

https://codereview.chromium.org/1307013004/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:1637: EXPECT_EQ("11",
std::string(*widthUtf8, widthUtf8.length()));
On 2015/10/14 07:20:43, dcheng wrote:
> I think it might be easier to verify the result with Int32Value?

Done.
Using parseInt() to force int.

https://codereview.chromium.org/1307013004/diff/340001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameOwnerProperties.h (right):

https://codereview.chromium.org/1307013004/diff/340001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameOwnerProperties.h:38: :
WebFrameOwnerProperties(static_cast<ScrollingMode>(scrollingMode), marginWidth,
marginHeight)
On 2015/10/14 07:20:43, dcheng wrote:
> I don't think anyone else actually calls this constructor. So we should just
> merge the initializer list of this overload directly inside the
> BLINK_IMPLEMENTATION block.

Done.

[Charlie Reis, 2015-10-14 23:34:18]

Nice.  content/ LGTM with nits.

https://codereview.chromium.org/1307013004/diff/380001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1307013004/diff/380001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.h:152: const
blink::WebFrameOwnerProperties& frame_owner_properties() {
nit: Please put these after CommitPendingSandboxFlags, since all the other
occurrences are right below sandbox flags.

https://codereview.chromium.org/1307013004/diff/380001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1307013004/diff/380001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1338: FrameTreeNode*
RenderFrameHostImpl::CheckAndGetIfChild(
nit: If we're not going to list this in the order of the .h file, then let's
list it above the first call site (OnDidChangeSandboxFlags).

https://codereview.chromium.org/1307013004/diff/380001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1361: // parent.
Also mention why we don't need to tell the proxies.

https://codereview.chromium.org/1307013004/diff/380001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1307013004/diff/380001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:633: // immediate child of
this FrameTreeNode.  Kills this renderer otherwise
nit: Change second sentence to:
|child_frame_routing_id| is considered untrusted, so the renderer process is
killed if it refers to a FrameTreeNode that is not a child of this node.

(I want to be clear that it doesn't kill the process if the node isn't found,
since that can happen via races.)

https://codereview.chromium.org/1307013004/diff/380001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:635: FrameTreeNode*
CheckAndGetIfChild(
FindAndVerifyChild

https://codereview.chromium.org/1307013004/diff/380001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1307013004/diff/380001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1862: auto has_scrollbar =
[](RenderFrameHostImpl* rfh) {
Please add an explanatory comment, since the implementation of this lambda isn't
obviously connected to determining whether there's a scrollbar.

https://codereview.chromium.org/1307013004/diff/380001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1868: const int kThreshold = 4;
How wide is the scrollbar itself?  Is this lambda trying to detect the presence
of the scrollbar by the width, and hoping that the margin for error doesn't hide
the presence of the scrollbar itself?  Seems like we could be cutting it close.

https://codereview.chromium.org/1307013004/diff/380001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1882: GURL urls[] = {
// Run the test over a variety of parent/child cases.

https://codereview.chromium.org/1307013004/diff/380001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1884:
embedded_test_server()->GetURL("b.com", "/tall_page.html"),
Isn't this a no-op, since we start on b.com?  c.com might be a more interesting
case, since it requires a remote-to-remote process swap.

https://codereview.chromium.org/1307013004/diff/380001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1950:
embedded_test_server()->GetURL("b.com", "/title2.html"),
Same.

[lazyboy, 2015-10-15 02:31:39]

https://codereview.chromium.org/1307013004/diff/380001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1307013004/diff/380001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.h:152: const
blink::WebFrameOwnerProperties& frame_owner_properties() {
On 2015/10/14 23:34:17, Charlie Reis wrote:
> nit: Please put these after CommitPendingSandboxFlags, since all the other
> occurrences are right below sandbox flags.

Done.

https://codereview.chromium.org/1307013004/diff/380001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1307013004/diff/380001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1338: FrameTreeNode*
RenderFrameHostImpl::CheckAndGetIfChild(
On 2015/10/14 23:34:17, Charlie Reis wrote:
> nit: If we're not going to list this in the order of the .h file, then let's
> list it above the first call site (OnDidChangeSandboxFlags).

Done.

https://codereview.chromium.org/1307013004/diff/380001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1361: // parent.
On 2015/10/14 23:34:17, Charlie Reis wrote:
> Also mention why we don't need to tell the proxies.

Done.

https://codereview.chromium.org/1307013004/diff/380001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1307013004/diff/380001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:633: // immediate child of
this FrameTreeNode.  Kills this renderer otherwise
On 2015/10/14 23:34:17, Charlie Reis wrote:
> nit: Change second sentence to:
> |child_frame_routing_id| is considered untrusted, so the renderer process is
> killed if it refers to a FrameTreeNode that is not a child of this node.
> 
> (I want to be clear that it doesn't kill the process if the node isn't found,
> since that can happen via races.)

Done.

https://codereview.chromium.org/1307013004/diff/380001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:635: FrameTreeNode*
CheckAndGetIfChild(
On 2015/10/14 23:34:17, Charlie Reis wrote:
> FindAndVerifyChild

Done.

https://codereview.chromium.org/1307013004/diff/380001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1307013004/diff/380001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1862: auto has_scrollbar =
[](RenderFrameHostImpl* rfh) {
On 2015/10/14 23:34:17, Charlie Reis wrote:
> Please add an explanatory comment, since the implementation of this lambda
isn't
> obviously connected to determining whether there's a scrollbar.

Done.

https://codereview.chromium.org/1307013004/diff/380001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1868: const int kThreshold = 4;
On 2015/10/14 23:34:17, Charlie Reis wrote:
> How wide is the scrollbar itself?  Is this lambda trying to detect the
presence
> of the scrollbar by the width, and hoping that the margin for error doesn't
hide
> the presence of the scrollbar itself?  Seems like we could be cutting it
close.

This was supposed to prevent off by one -ish errors. Because there's default
border in iframes, we can be off by 4 (2 * 2px).
However, since both you and Alex seem to had comments around it, this
suggests this isn't a good idea. I've removed this and set the frame's
border to 0.

https://codereview.chromium.org/1307013004/diff/380001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1882: GURL urls[] = {
On 2015/10/14 23:34:17, Charlie Reis wrote:
> // Run the test over a variety of parent/child cases.

Done.

https://codereview.chromium.org/1307013004/diff/380001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1884:
embedded_test_server()->GetURL("b.com", "/tall_page.html"),
On 2015/10/14 23:34:17, Charlie Reis wrote:
> Isn't this a no-op, since we start on b.com?  c.com might be a more
interesting
> case, since it requires a remote-to-remote process swap.

Done.

https://codereview.chromium.org/1307013004/diff/380001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1950:
embedded_test_server()->GetURL("b.com", "/title2.html"),
On 2015/10/14 23:34:17, Charlie Reis wrote:
> Same.

Done.

[dcheng, 2015-10-15 06:06:53]

https://codereview.chromium.org/1307013004/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1307013004/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2011:
RemoteBridgeFrameOwner* remoteOwner = toRemoteBridgeFrameOwner(frame->owner());
> Can you elaborate this one?

We prepare a local frame when we have a provisional navigation. Between the time
the local frame is prepared and the time it's actually committed, a lot of other
stuff can run. If these properties are changed between these two times, then it
seems like we wouldn't get the correct properties.

> WebFrame::setFrameOwnerProperties needs core Frame for |frame| which is not
set until line 2021, causes a crash.

Can we move this block to line 2022 then?

https://codereview.chromium.org/1307013004/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/1307013004/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrame.cpp:150: void
WebFrame::setFrameOwnerProperties(const WebFrameOwnerProperties&
frameOwnerProperties)
Unlike sandbox flags, this is only called on WebLocalFrame. So we should move
this to WebLocalFrame/WebLocalFrameImpl.

https://codereview.chromium.org/1307013004/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1307013004/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:1633: v8::Local<v8::Value>
marginWidthResult =
localFrame->executeScriptAndReturnValue(WebScriptSource("parseInt(document.body.getAttribute('marginwidth')
|| '0');"));
Why || '0'?

https://codereview.chromium.org/1307013004/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:1672: v8::Local<v8::Value>
marginWidthResult =
localFrame->executeScriptAndReturnValue(WebScriptSource("parseInt(document.body.getAttribute('marginwidth')
|| '0');"));
Ditto, why || '0'? We can test if the return value is undefined if that's what
we're trying to check for.

[Charlie Reis, 2015-10-15 16:26:30]

Thanks.  content/ still LGTM with nit.

https://codereview.chromium.org/1307013004/diff/400001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1307013004/diff/400001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1361: // These properties
only affect the RenderFrame and lives in its parent
nit: s/lives/live/

[lazyboy, 2015-10-15 16:56:38]

https://codereview.chromium.org/1307013004/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1307013004/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2011:
RemoteBridgeFrameOwner* remoteOwner = toRemoteBridgeFrameOwner(frame->owner());
On 2015/10/15 06:06:53, dcheng wrote:
> > Can you elaborate this one?
> 
> We prepare a local frame when we have a provisional navigation. Between the
time
> the local frame is prepared and the time it's actually committed, a lot of
other
> stuff can run. If these properties are changed between these two times, then
it
> seems like we wouldn't get the correct properties.
OK.
If I understand correctly, if these properties are dynamically
set using JavaScript during that time, we'll be off, right?
Now since these properties currently do not take effect immediately
and we actually have to navigate the frame to see its effect, we
should be fine IMO. Note that this is true for non --site-per-process
too.
> 
> > WebFrame::setFrameOwnerProperties needs core Frame for |frame| which is not
> set until line 2021, causes a crash.
> 
> Can we move this block to line 2022 then?
Moved, I'll ask Alex if it's OK to move setSandboxFlags there too.

https://codereview.chromium.org/1307013004/diff/400001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1307013004/diff/400001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1361: // These properties
only affect the RenderFrame and lives in its parent
On 2015/10/15 16:26:30, Charlie Reis wrote:
> nit: s/lives/live/

Done.
Thanks.

https://codereview.chromium.org/1307013004/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/1307013004/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrame.cpp:150: void
WebFrame::setFrameOwnerProperties(const WebFrameOwnerProperties&
frameOwnerProperties)
On 2015/10/15 06:06:53, dcheng wrote:
> Unlike sandbox flags, this is only called on WebLocalFrame. So we should move
> this to WebLocalFrame/WebLocalFrameImpl.

Done.

https://codereview.chromium.org/1307013004/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1307013004/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:1633: v8::Local<v8::Value>
marginWidthResult =
localFrame->executeScriptAndReturnValue(WebScriptSource("parseInt(document.body.getAttribute('marginwidth')
|| '0');"));
On 2015/10/15 06:06:53, dcheng wrote:
> Why || '0'?

Removed this one since we expect non zero or not undefined here.

https://codereview.chromium.org/1307013004/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:1672: v8::Local<v8::Value>
marginWidthResult =
localFrame->executeScriptAndReturnValue(WebScriptSource("parseInt(document.body.getAttribute('marginwidth')
|| '0');"));
On 2015/10/15 06:06:53, dcheng wrote:
> Ditto, why || '0'? We can test if the return value is undefined if that's what
> we're trying to check for.

Technically margin of 0 can come from Number(0) and undefined.
This keeps things simple, otherwise I'd have to check for undefined first, then
if not undefined check for int 0.

[dcheng, 2015-10-15 22:07:46]

https://codereview.chromium.org/1307013004/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1307013004/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:1672: v8::Local<v8::Value>
marginWidthResult =
localFrame->executeScriptAndReturnValue(WebScriptSource("parseInt(document.body.getAttribute('marginwidth')
|| '0');"));
On 2015/10/15 at 16:56:38, lazyboy wrote:
> On 2015/10/15 06:06:53, dcheng wrote:
> > Ditto, why || '0'? We can test if the return value is undefined if that's
what
> > we're trying to check for.
> 
> Technically margin of 0 can come from Number(0) and undefined.
> This keeps things simple, otherwise I'd have to check for undefined first,
then if not undefined check for int 0.

I don't think I follow. In the test, it should be deterministic whether it's '0'
or undefined, right?

[dcheng, 2015-10-15 22:20:17]

https://codereview.chromium.org/1307013004/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1307013004/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:1672: v8::Local<v8::Value>
marginWidthResult =
localFrame->executeScriptAndReturnValue(WebScriptSource("parseInt(document.body.getAttribute('marginwidth')
|| '0');"));
On 2015/10/15 at 22:07:46, dcheng wrote:
> On 2015/10/15 at 16:56:38, lazyboy wrote:
> > On 2015/10/15 06:06:53, dcheng wrote:
> > > Ditto, why || '0'? We can test if the return value is undefined if that's
what
> > > we're trying to check for.
> > 
> > Technically margin of 0 can come from Number(0) and undefined.
> > This keeps things simple, otherwise I'd have to check for undefined first,
then if not undefined check for int 0.
> 
> I don't think I follow. In the test, it should be deterministic whether it's
'0' or undefined, right?

Also... since this is a unit test, maybe just use
HTMLBodyElement::getIntegralAttribute() directly rather than going through JS?

Sorry, I really should have noticed this earlier.

[lazyboy, 2015-10-15 23:00:52]

https://codereview.chromium.org/1307013004/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1307013004/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:1672: v8::Local<v8::Value>
marginWidthResult =
localFrame->executeScriptAndReturnValue(WebScriptSource("parseInt(document.body.getAttribute('marginwidth')
|| '0');"));
On 2015/10/15 22:07:46, dcheng wrote:
> On 2015/10/15 at 16:56:38, lazyboy wrote:
> > On 2015/10/15 06:06:53, dcheng wrote:
> > > Ditto, why || '0'? We can test if the return value is undefined if that's
> what
> > > we're trying to check for.
> > 
> > Technically margin of 0 can come from Number(0) and undefined.
> > This keeps things simple, otherwise I'd have to check for undefined first,
> then if not undefined check for int 0.
> 
> I don't think I follow. In the test, it should be deterministic whether it's
'0'
> or undefined, right?

Yes, it is deterministic in this test. What I wanted to do was not
depend on what was returned null/undefined/0.

https://codereview.chromium.org/1307013004/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:1672: v8::Local<v8::Value>
marginWidthResult =
localFrame->executeScriptAndReturnValue(WebScriptSource("parseInt(document.body.getAttribute('marginwidth')
|| '0');"));
On 2015/10/15 22:20:17, dcheng wrote:
> On 2015/10/15 at 22:07:46, dcheng wrote:
> > On 2015/10/15 at 16:56:38, lazyboy wrote:
> > > On 2015/10/15 06:06:53, dcheng wrote:
> > > > Ditto, why || '0'? We can test if the return value is undefined if
that's
> what
> > > > we're trying to check for.
> > > 
> > > Technically margin of 0 can come from Number(0) and undefined.
> > > This keeps things simple, otherwise I'd have to check for undefined first,
> then if not undefined check for int 0.
> > 
> > I don't think I follow. In the test, it should be deterministic whether it's
> '0' or undefined, right?
> 
> Also... since this is a unit test, maybe just use
> HTMLBodyElement::getIntegralAttribute() directly rather than going through JS?
> 
> Sorry, I really should have noticed this earlier.

That is just cool! Thanks.
Done.

[alexmos, 2015-10-16 17:17:01]

https://codereview.chromium.org/1307013004/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1307013004/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2011:
RemoteBridgeFrameOwner* remoteOwner = toRemoteBridgeFrameOwner(frame->owner());
> > Can we move this block to line 2022 then?
> Moved, I'll ask Alex if it's OK to move setSandboxFlags there too.

I think this should be ok.  This needs to happens before
Document::initSecurityContext gets called, which shouldn't happen before
frame->init().

[lazyboy, 2015-10-21 18:17:58]

@dcheng ping.

[dcheng, 2015-10-21 21:04:01]

https://codereview.chromium.org/1307013004/diff/520001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1307013004/diff/520001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1323: return child;
I assume we explicitly allow not finding a child, to prevent a race from killing
the renderer. Would be nice to comment if that's the case.

https://codereview.chromium.org/1307013004/diff/520001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/1307013004/diff/520001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:67: // TODO(dcheng): Currently,
the update only takes effect on next frame
What's this TODO for? =)

[lazyboy, 2015-10-23 21:19:35]

Oh, forgot to send my replies :(

https://chromiumcodereview.appspot.com/1307013004/diff/520001/content/browser...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://chromiumcodereview.appspot.com/1307013004/diff/520001/content/browser...
content/browser/frame_host/render_frame_host_impl.cc:1323: return child;
On 2015/10/21 21:04:01, dcheng wrote:
> I assume we explicitly allow not finding a child, to prevent a race from
killing
> the renderer.
Yes, I believe so (this pattern already existed in this file).
Would be nice to comment if that's the case.

Done.

https://chromiumcodereview.appspot.com/1307013004/diff/520001/third_party/Web...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://chromiumcodereview.appspot.com/1307013004/diff/520001/third_party/Web...
third_party/WebKit/public/web/WebLocalFrame.h:67: // TODO(dcheng): Currently,
the update only takes effect on next frame
On 2015/10/21 21:04:01, dcheng wrote:
> What's this TODO for? =)

This corresponds to the FIXMEs in 
https://codereview.chromium.org/1307013004/diff/520001/third_party/WebKit/Sou...

} else if (name == marginwidthAttr) {	 131     } else if (name ==
marginwidthAttr) {
 123         m_marginWidth = value.toInt();	 132        
setMarginWidth(value.toInt());
 124         // FIXME: If we are already attached, this has no effect.

, etc.
Maybe a TODO here is superfluous?

[dcheng, 2015-10-26 16:57:23]

lgtm

[lazyboy, 2015-10-27 13:50:19]

lazyboy@chromium.org changed reviewers:
+ asvitkine@chromium.org, sky@chromium.org, thestig@chromium.org

[lazyboy, 2015-10-27 13:50:21]

+sky for components/html_viewer
+thestig for components/printing
+asvitkine for histograms.xml

[Alexei Svitkine (slow), 2015-10-27 15:12:25]

histograms.xml lgtm

[Lei Zhang, 2015-10-27 16:16:52]

components/printing lgtm

[sky, 2015-10-27 21:04:13]

LGTM

[lazyboy, 2015-10-27 22:14:29]

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

[commit-bot: I haz the power, 2015-10-27 22:15:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1307013004/540001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1307013004/540001

[commit-bot: I haz the power, 2015-10-27 22:19:03]

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

[commit-bot: I haz the power, 2015-10-27 22:19:04]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lazyboy, 2015-11-03 01:20:16]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2015-11-03 01:20:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexmos@chromium.org, creis@chromium.org, sky@chromium.org,
thestig@chromium.org, dcheng@chromium.org, asvitkine@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1307013004/#ps580001 (title:
"content_unittests compile fix")

[commit-bot: I haz the power, 2015-11-03 01:21:15]

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

[commit-bot: I haz the power, 2015-11-03 01:27:41]

Committed patchset #30 (id:580001)

[commit-bot: I haz the power, 2015-11-03 01:29:05]

Patchset 30 (id:??) landed as
https://crrev.com/70605c32bd6a79317df00114ec556f87a7bd8afd
Cr-Commit-Position: refs/heads/master@{#357503}