(blink) Propagate scrolling/marginwidth/marginheight property values to child frame.

(blink) Propagate scrolling/marginwidth/marginheight property values to child frame.

We send these values in two cases:
1) Initally when we're creating a frame (by sending FrameHostMsg_CreateChildFrame), we send all of these properties in a WebFrameOwnerProperties struct.
2) Once the parent frame dynamically changes any of these properties. For the tree properties we have:
didChangeSrollingMode()
didChangeMarginWidth()
didChangeMarginHeight()

BUG=524725

[lazyboy, 2015-09-01 18:52:45]

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

[lazyboy, 2015-09-01 18:58:54]

Alex, can you check the overall structure of the CL? The chromium side CL is
https://chromiumcodereview.appspot.com/1307013004/.

[dcheng, 2015-09-02 06:38:25]

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

[dcheng, 2015-09-02 06:38:26]

Some initial thoughts.

https://codereview.chromium.org/1319863006/diff/40001/Source/core/html/HTMLIF...
File Source/core/html/HTMLIFrameElement.cpp (right):

https://codereview.chromium.org/1319863006/diff/40001/Source/core/html/HTMLIF...
Source/core/html/HTMLIFrameElement.cpp:123: } else if (name == marginwidthAttr)
{
I believe these attributes are supported on <frame>, not just <iframe>.

https://codereview.chromium.org/1319863006/diff/40001/public/BUILD.gn
File public/BUILD.gn (right):

https://codereview.chromium.org/1319863006/diff/40001/public/BUILD.gn#newcode443
public/BUILD.gn:443: "web/WebFrameOwnerElement.h",
I don't see this file in this patch.

https://codereview.chromium.org/1319863006/diff/40001/public/web/WebFrameOwne...
File public/web/WebFrameOwnerProperties.h (right):

https://codereview.chromium.org/1319863006/diff/40001/public/web/WebFrameOwne...
public/web/WebFrameOwnerProperties.h:37: enum ScrollingMode {
enum class.

[lazyboy, 2015-09-02 16:56:30]

https://chromiumcodereview.appspot.com/1319863006/diff/40001/Source/core/html...
File Source/core/html/HTMLIFrameElement.cpp (right):

https://chromiumcodereview.appspot.com/1319863006/diff/40001/Source/core/html...
Source/core/html/HTMLIFrameElement.cpp:123: } else if (name == marginwidthAttr)
{
On 2015/09/02 06:38:26, dcheng wrote:
> I believe these attributes are supported on <frame>, not just <iframe>.

Right, I forgot to move these back to FrameElementBase after experimenting.
Done.

https://chromiumcodereview.appspot.com/1319863006/diff/40001/public/BUILD.gn
File public/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1319863006/diff/40001/public/BUILD.gn#...
public/BUILD.gn:443: "web/WebFrameOwnerElement.h",
On 2015/09/02 06:38:26, dcheng wrote:
> I don't see this file in this patch.

It should be WebFrameOwnerProperties.h

https://chromiumcodereview.appspot.com/1319863006/diff/40001/public/web/WebFr...
File public/web/WebFrameOwnerProperties.h (right):

https://chromiumcodereview.appspot.com/1319863006/diff/40001/public/web/WebFr...
public/web/WebFrameOwnerProperties.h:37: enum ScrollingMode {
On 2015/09/02 06:38:26, dcheng wrote:
> enum class.

Done.

[alexmos, 2015-09-02 21:37:06]

+site-isolation-reviews

Thanks for taking this on, some more comments below.  The overall structure
looks good.  One question I had (more for your Chromium CL) is whether same-site
subframe navigations work.  I.e., A embeds B, A changes one of these properties
for the subframe, B navigates to B2.  How would B2 see the latest properties?

https://codereview.chromium.org/1319863006/diff/80001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1319863006/diff/80001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:590: const HTMLFrameOwnerElement* ownerElement =
m_frame->deprecatedLocalOwner();
Wouldn't just doing this via m_frame->owner() work for both local and remote
cases?  That would allow us to get rid of the deprecatedLocalOwner call and the
duplicate block.

https://codereview.chromium.org/1319863006/diff/80001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:596: if (!ownerElement && m_frame->owner() &&
(m_frame->owner()->scrollingMode() == ScrollbarAlwaysOff)) {
Does LocalFrame::createView need a similar fix? 
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/1319863006/diff/80001/Source/core/html/HTMLBo...
File Source/core/html/HTMLBodyElement.cpp (right):

https://codereview.chromium.org/1319863006/diff/80001/Source/core/html/HTMLBo...
Source/core/html/HTMLBodyElement.cpp:170: } else if (!ownerElement &&
document().frame()->owner()) {
Same here, is it possible to use document().frame()->owner() for both local and
remote cases?  Document::ownerElement also goes to the owner element via
frame().  

Also, document.ownerElement() has a null check for frame() - do we need one
here?

https://codereview.chromium.org/1319863006/diff/80001/Source/core/html/HTMLFr...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/1319863006/diff/80001/Source/core/html/HTMLFr...
Source/core/html/HTMLFrameOwnerElement.cpp:202:
document().frame()->loader().client()->didChangeScrollingMode(contentFrame(),
scrollbarMode);
HTMLPlugInElement also inherits from HTMLFrameOwnerElement.  Do we need to
propagate these properties for that too, or just HTMLFrameElementBase?  If yes,
then createChildFrame needs to be fixed as well, since it currently checks
isHTMLFrameElementBase to propagate margins.

https://codereview.chromium.org/1319863006/diff/80001/Source/core/html/HTMLFr...
File Source/core/html/HTMLFrameOwnerElement.h (right):

https://codereview.chromium.org/1319863006/diff/80001/Source/core/html/HTMLFr...
Source/core/html/HTMLFrameOwnerElement.h:83: int marginWidth() const { return
m_marginWidth; }
override?  (same for marginHeight)

https://codereview.chromium.org/1319863006/diff/80001/Source/core/loader/Fram...
File Source/core/loader/FrameLoaderClient.h (right):

https://codereview.chromium.org/1319863006/diff/80001/Source/core/loader/Fram...
Source/core/loader/FrameLoaderClient.h:137: virtual void
didChangeMarginHeight(Frame*, int) {}
Nit: how about grouping these with didChangeName and didChangeSandboxFlags
below?

https://codereview.chromium.org/1319863006/diff/80001/Source/web/FrameLoaderC...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/1319863006/diff/80001/Source/web/FrameLoaderC...
Source/web/FrameLoaderClientImpl.cpp:190: void
FrameLoaderClientImpl::didChangeScrollingMode(Frame* childFrame, ScrollbarMode
scrollingMode)
Nit: move these down to didChangeName and DidChangeSandboxFlags

https://codereview.chromium.org/1319863006/diff/80001/Source/web/RemoteBridge...
File Source/web/RemoteBridgeFrameOwner.cpp (right):

https://codereview.chromium.org/1319863006/diff/80001/Source/web/RemoteBridge...
Source/web/RemoteBridgeFrameOwner.cpp:15: , m_scrolling(ScrollbarAuto)
Any reason for not plumbing the initial values here directly, like we do for
sandbox flags?

https://codereview.chromium.org/1319863006/diff/80001/Source/web/WebFrame.cpp
File Source/web/WebFrame.cpp (right):

https://codereview.chromium.org/1319863006/diff/80001/Source/web/WebFrame.cpp...
Source/web/WebFrame.cpp:143: if (!owner)
When would owner be nullptr?  Do we need this check, or can we ASSERT(owner)?

https://codereview.chromium.org/1319863006/diff/80001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1319863006/diff/80001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:1761: marginWidth =
toHTMLFrameElementBase(ownerElement)->marginWidth();
Is the cast necessary here?  marginWidth() and marginHeight() exist on
HTMLFrameOwnerElement.  Also see my earlier question about whether this needs to
be guarded by isHTMLFrameElementBase.

https://codereview.chromium.org/1319863006/diff/80001/public/web/WebFrameClie...
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1319863006/diff/80001/public/web/WebFrameClie...
public/web/WebFrameClient.h:153: virtual WebFrame*
createChildFrame(WebLocalFrame* parent, WebTreeScopeType, const WebString&
frameName, WebSandboxFlags sandboxFlags, const WebFrameOwnerProperties&) {
return nullptr; }
Changing this signature was "fun" the last time I did it...  Don't forget to
update the createChildFrame in 
android_webview/renderer/print_web_view_helper.cc and
mojo/services/html_viewer/html_document.cc on the chromium side to avoid
breaking Android and mojo.  And presumably you'll add a temp version of this
with the old signature so you can land this independently?

https://codereview.chromium.org/1319863006/diff/80001/public/web/WebFrameOwne...
File public/web/WebFrameOwnerProperties.h (right):

https://codereview.chromium.org/1319863006/diff/80001/public/web/WebFrameOwne...
public/web/WebFrameOwnerProperties.h:54: WebFrameOwnerProperties(ScrollingMode
scrollingMode, int marginWidth, int marginHeight)
nit: empty line before this?

[lazyboy, 2015-09-15 01:40:33]

Hi Alex, Sorry for the delay here. I've updated the CL making same-site subframe
navigations work, please take a look. I've also updated the chromium CL.
I still need to write tests for the CL and I will start working on them after
next round of review. Ty.

https://chromiumcodereview.appspot.com/1319863006/diff/80001/Source/core/fram...
File Source/core/frame/FrameView.cpp (right):

https://chromiumcodereview.appspot.com/1319863006/diff/80001/Source/core/fram...
Source/core/frame/FrameView.cpp:590: const HTMLFrameOwnerElement* ownerElement =
m_frame->deprecatedLocalOwner();
On 2015/09/02 21:37:05, alexmos wrote:
> Wouldn't just doing this via m_frame->owner() work for both local and remote
> cases?  That would allow us to get rid of the deprecatedLocalOwner call and
the
> duplicate block.

Yes, Done.

https://chromiumcodereview.appspot.com/1319863006/diff/80001/Source/core/fram...
Source/core/frame/FrameView.cpp:596: if (!ownerElement && m_frame->owner() &&
(m_frame->owner()->scrollingMode() == ScrollbarAlwaysOff)) {
On 2015/09/02 21:37:05, alexmos wrote:
> Does LocalFrame::createView need a similar fix? 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I've added the fix there too similarly.

https://chromiumcodereview.appspot.com/1319863006/diff/80001/Source/core/html...
File Source/core/html/HTMLBodyElement.cpp (right):

https://chromiumcodereview.appspot.com/1319863006/diff/80001/Source/core/html...
Source/core/html/HTMLBodyElement.cpp:170: } else if (!ownerElement &&
document().frame()->owner()) {
On 2015/09/02 21:37:05, alexmos wrote:
> Same here, is it possible to use document().frame()->owner() for both local
and
> remote cases?  Document::ownerElement also goes to the owner element via
> frame().  
Done.
> 
> Also, document.ownerElement() has a null check for frame() - do we need one
> here?
Done.

https://chromiumcodereview.appspot.com/1319863006/diff/80001/Source/core/html...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://chromiumcodereview.appspot.com/1319863006/diff/80001/Source/core/html...
Source/core/html/HTMLFrameOwnerElement.cpp:202:
document().frame()->loader().client()->didChangeScrollingMode(contentFrame(),
scrollbarMode);
On 2015/09/02 21:37:05, alexmos wrote:
> HTMLPlugInElement also inherits from HTMLFrameOwnerElement. Do we need to
> propagate these properties for that too, or just HTMLFrameElementBase?
Since parseAttribute() in HTMLFrameElementBase was looking for these properties,
I'm guessing not. I don't see them here either:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/object
If yes,
> then createChildFrame needs to be fixed as well, since it currently checks
> isHTMLFrameElementBase to propagate margins.
I've made it not propagate updates if this is a plugin element.

https://chromiumcodereview.appspot.com/1319863006/diff/80001/Source/core/html...
File Source/core/html/HTMLFrameOwnerElement.h (right):

https://chromiumcodereview.appspot.com/1319863006/diff/80001/Source/core/html...
Source/core/html/HTMLFrameOwnerElement.h:83: int marginWidth() const { return
m_marginWidth; }
On 2015/09/02 21:37:05, alexmos wrote:
> override?  (same for marginHeight)

Done.

https://chromiumcodereview.appspot.com/1319863006/diff/80001/Source/core/load...
File Source/core/loader/FrameLoaderClient.h (right):

https://chromiumcodereview.appspot.com/1319863006/diff/80001/Source/core/load...
Source/core/loader/FrameLoaderClient.h:137: virtual void
didChangeMarginHeight(Frame*, int) {}
On 2015/09/02 21:37:05, alexmos wrote:
> Nit: how about grouping these with didChangeName and didChangeSandboxFlags
> below?

Done.

https://chromiumcodereview.appspot.com/1319863006/diff/80001/Source/web/Frame...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://chromiumcodereview.appspot.com/1319863006/diff/80001/Source/web/Frame...
Source/web/FrameLoaderClientImpl.cpp:190: void
FrameLoaderClientImpl::didChangeScrollingMode(Frame* childFrame, ScrollbarMode
scrollingMode)
On 2015/09/02 21:37:05, alexmos wrote:
> Nit: move these down to didChangeName and DidChangeSandboxFlags

Done.

https://chromiumcodereview.appspot.com/1319863006/diff/80001/Source/web/Remot...
File Source/web/RemoteBridgeFrameOwner.cpp (right):

https://chromiumcodereview.appspot.com/1319863006/diff/80001/Source/web/Remot...
Source/web/RemoteBridgeFrameOwner.cpp:15: , m_scrolling(ScrollbarAuto)
On 2015/09/02 21:37:06, alexmos wrote:
> Any reason for not plumbing the initial values here directly, like we do for
> sandbox flags?

I've moved the initial values to constructor.

https://chromiumcodereview.appspot.com/1319863006/diff/80001/Source/web/WebFr...
File Source/web/WebFrame.cpp (right):

https://chromiumcodereview.appspot.com/1319863006/diff/80001/Source/web/WebFr...
Source/web/WebFrame.cpp:143: if (!owner)
On 2015/09/02 21:37:06, alexmos wrote:
> When would owner be nullptr?  Do we need this check, or can we ASSERT(owner)?

Was not intentionally checking against nullptr, changed to ASSERT.

https://chromiumcodereview.appspot.com/1319863006/diff/80001/Source/web/WebLo...
File Source/web/WebLocalFrameImpl.cpp (right):

https://chromiumcodereview.appspot.com/1319863006/diff/80001/Source/web/WebLo...
Source/web/WebLocalFrameImpl.cpp:1761: marginWidth =
toHTMLFrameElementBase(ownerElement)->marginWidth();
On 2015/09/02 21:37:06, alexmos wrote:
> Is the cast necessary here?  marginWidth() and marginHeight() exist on
> HTMLFrameOwnerElement.  
Removed cast.
Also see my earlier question about whether this needs to
> be guarded by isHTMLFrameElementBase.

The caller of this function is FrameLoaderClientImpl::createFrame(), which
shouldn't be called for a plugin, right? I was inspired by looking at the
comment in WebLocalFrameImpl::fromFrameOwnerElement().

https://chromiumcodereview.appspot.com/1319863006/diff/80001/public/web/WebFr...
File public/web/WebFrameClient.h (right):

https://chromiumcodereview.appspot.com/1319863006/diff/80001/public/web/WebFr...
public/web/WebFrameClient.h:153: virtual WebFrame*
createChildFrame(WebLocalFrame* parent, WebTreeScopeType, const WebString&
frameName, WebSandboxFlags sandboxFlags, const WebFrameOwnerProperties&) {
return nullptr; }
On 2015/09/02 21:37:06, alexmos wrote:
> Changing this signature was "fun" the last time I did it...  Don't forget to
> update the createChildFrame in 
> android_webview/renderer/print_web_view_helper.cc
This probably moved to components/printing/renderer/print_web_view_helper.cc
now, right?
and
> mojo/services/html_viewer/html_document.cc on the chromium side to avoid
> breaking Android and mojo. 
Done.
And presumably you'll add a temp version of this
> with the old signature so you can land this independently?
Yes, I'll do that once the CL gets close to be completed and I have tests.

https://chromiumcodereview.appspot.com/1319863006/diff/80001/public/web/WebFr...
File public/web/WebFrameOwnerProperties.h (right):

https://chromiumcodereview.appspot.com/1319863006/diff/80001/public/web/WebFr...
public/web/WebFrameOwnerProperties.h:54: WebFrameOwnerProperties(ScrollingMode
scrollingMode, int marginWidth, int marginHeight)
On 2015/09/02 21:37:06, alexmos wrote:
> nit: empty line before this?

Done.

[dcheng, 2015-09-15 21:52:47]

https://codereview.chromium.org/1319863006/diff/120001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/1319863006/diff/120001/public/web/WebFrame.h#...
public/web/WebFrame.h:181: BLINK_EXPORT void setFrameOwnerProperties(const
WebFrameOwnerProperties&);
Like sandbox, these are properties of the frame element, right? Would it make
sense to consolidate sandbox flags here too?

[lazyboy, 2015-09-15 23:50:31]

https://codereview.chromium.org/1319863006/diff/120001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/1319863006/diff/120001/public/web/WebFrame.h#...
public/web/WebFrame.h:181: BLINK_EXPORT void setFrameOwnerProperties(const
WebFrameOwnerProperties&);
On 2015/09/15 21:52:47, dcheng wrote:
> Like sandbox, these are properties of the frame element, right?
yes.
Would it make
> sense to consolidate sandbox flags here too?
I started with that approach and dropped it for some reason. I agree these
should be packaged /w sandbox flags, Alex?

[alexmos, 2015-09-16 00:36:47]

https://codereview.chromium.org/1319863006/diff/80001/Source/core/html/HTMLFr...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/1319863006/diff/80001/Source/core/html/HTMLFr...
Source/core/html/HTMLFrameOwnerElement.cpp:202:
document().frame()->loader().client()->didChangeScrollingMode(contentFrame(),
scrollbarMode);
On 2015/09/15 01:40:32, lazyboy wrote:
> On 2015/09/02 21:37:05, alexmos wrote:
> > HTMLPlugInElement also inherits from HTMLFrameOwnerElement. Do we need to
> > propagate these properties for that too, or just HTMLFrameElementBase?
> Since parseAttribute() in HTMLFrameElementBase was looking for these
properties,
> I'm guessing not. I don't see them here either:
> https://developer.mozilla.org/en-US/docs/Web/HTML/Element/object
> If yes,
> > then createChildFrame needs to be fixed as well, since it currently checks
> > isHTMLFrameElementBase to propagate margins.
> I've made it not propagate updates if this is a plugin element.

Acknowledged.

https://codereview.chromium.org/1319863006/diff/80001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1319863006/diff/80001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:1761: marginWidth =
toHTMLFrameElementBase(ownerElement)->marginWidth();
On 2015/09/15 01:40:33, lazyboy wrote:
> On 2015/09/02 21:37:06, alexmos wrote:
> > Is the cast necessary here?  marginWidth() and marginHeight() exist on
> > HTMLFrameOwnerElement.  
> Removed cast.
> Also see my earlier question about whether this needs to
> > be guarded by isHTMLFrameElementBase.
> 
> The caller of this function is FrameLoaderClientImpl::createFrame(), which
> shouldn't be called for a plugin, right? I was inspired by looking at the
> comment in WebLocalFrameImpl::fromFrameOwnerElement().

I think you're right, we shouldn't get here for plugins (assuming plugins can't
create subframes:) ).  So then we don't need the isHTMLFrameElementBase check,
right?

https://codereview.chromium.org/1319863006/diff/80001/public/web/WebFrameClie...
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1319863006/diff/80001/public/web/WebFrameClie...
public/web/WebFrameClient.h:153: virtual WebFrame*
createChildFrame(WebLocalFrame* parent, WebTreeScopeType, const WebString&
frameName, WebSandboxFlags sandboxFlags, const WebFrameOwnerProperties&) {
return nullptr; }
On 2015/09/15 01:40:33, lazyboy wrote:
> On 2015/09/02 21:37:06, alexmos wrote:
> > Changing this signature was "fun" the last time I did it...  Don't forget to
> > update the createChildFrame in 
> > android_webview/renderer/print_web_view_helper.cc
> This probably moved to components/printing/renderer/print_web_view_helper.cc
> now, right?

Yes, looks like it.

https://codereview.chromium.org/1319863006/diff/120001/Source/core/frame/Fram...
File Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/1319863006/diff/120001/Source/core/frame/Fram...
Source/core/frame/FrameView.cpp:596: return;
Was losing the return intentional?

https://codereview.chromium.org/1319863006/diff/120001/Source/core/html/HTMLB...
File Source/core/html/HTMLBodyElement.cpp (left):

https://codereview.chromium.org/1319863006/diff/120001/Source/core/html/HTMLB...
Source/core/html/HTMLBodyElement.cpp:163: if
(!isHTMLFrameElementBase(ownerElement))
Is removing this ok?  Do we want to do the below for plugins or is this not
called for them?

https://codereview.chromium.org/1319863006/diff/120001/Source/core/html/HTMLF...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/1319863006/diff/120001/Source/core/html/HTMLF...
Source/core/html/HTMLFrameOwnerElement.cpp:205:
document().frame()->loader().client()->didChangeScrollingMode(contentFrame(),
scrollbarMode);
You're using three separate IPCs to propagate these updates to the browser
process, but then they get squished into the same
FrameMsg_SetFrameOwnerProperties on the way back to the renderer.  Should we be
consistent and either use one IPC here for all three things, or use different
IPCs on browser->renderer path too?

https://codereview.chromium.org/1319863006/diff/120001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/1319863006/diff/120001/public/web/WebFrame.h#...
public/web/WebFrame.h:181: BLINK_EXPORT void setFrameOwnerProperties(const
WebFrameOwnerProperties&);
On 2015/09/15 23:50:30, lazyboy wrote:
> On 2015/09/15 21:52:47, dcheng wrote:
> > Like sandbox, these are properties of the frame element, right?
> yes.
> Would it make
> > sense to consolidate sandbox flags here too?
> I started with that approach and dropped it for some reason. I agree these
> should be packaged /w sandbox flags, Alex?

I've been thinking about this too.  It'd be great to package them all together,
but there are also a couple of differences that worry me.  One is that sandbox
flags are replicated for remote frames as well (see comment on
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
for the reason), whereas these things don't matter for remote frames and aren't
sent to them.  Also, there's the whole sandbox-flags-take-effect-on-navigation
bit (which causes us to delay sandbox flags update IPCs for proxies), which
doesn't seem like it applies here.  And sandbox flags get copied into the
frame's SecurityContext when taking effect (this matters when inheriting sandbox
flags), so they are a bit more than frame owner properties.

[lazyboy, 2015-09-17 20:48:39]

https://chromiumcodereview.appspot.com/1319863006/diff/120001/Source/core/fra...
File Source/core/frame/FrameView.cpp (left):

https://chromiumcodereview.appspot.com/1319863006/diff/120001/Source/core/fra...
Source/core/frame/FrameView.cpp:596: return;
On 2015/09/16 00:36:47, alexmos wrote:
> Was losing the return intentional?

No, thanks for catching.

https://chromiumcodereview.appspot.com/1319863006/diff/120001/Source/core/htm...
File Source/core/html/HTMLBodyElement.cpp (left):

https://chromiumcodereview.appspot.com/1319863006/diff/120001/Source/core/htm...
Source/core/html/HTMLBodyElement.cpp:163: if
(!isHTMLFrameElementBase(ownerElement))
On 2015/09/16 00:36:47, alexmos wrote:
> Is removing this ok?  Do we want to do the below for plugins or is this not
> called for them?

This function is called for plugins, but the document's frame does not
have any owner:

WebViewPlugin::WebViewPlugin()
calls WebViewImpl::setMainFrame() which always sets owner = 0
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://chromiumcodereview.appspot.com/1319863006/diff/120001/Source/core/htm...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://chromiumcodereview.appspot.com/1319863006/diff/120001/Source/core/htm...
Source/core/html/HTMLFrameOwnerElement.cpp:205:
document().frame()->loader().client()->didChangeScrollingMode(contentFrame(),
scrollbarMode);
On 2015/09/16 00:36:47, alexmos wrote:
> You're using three separate IPCs to propagate these updates to the browser
> process, but then they get squished into the same
> FrameMsg_SetFrameOwnerProperties on the way back to the renderer.  Should we
be
> consistent and either use one IPC here for all three things, or use different
> IPCs on browser->renderer path too?

I've made renderer->browser one IPC too, with the cost of saving the
current frame owner properties state in RenderFrameImpl.

[alexmos, 2015-09-21 16:58:08]

LGTM - a couple of questions below, but I think this is ready for a closer look
from Daniel.

https://codereview.chromium.org/1319863006/diff/80001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1319863006/diff/80001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:1761: marginWidth =
toHTMLFrameElementBase(ownerElement)->marginWidth();
On 2015/09/16 00:36:47, alexmos wrote:
> On 2015/09/15 01:40:33, lazyboy wrote:
> > On 2015/09/02 21:37:06, alexmos wrote:
> > > Is the cast necessary here?  marginWidth() and marginHeight() exist on
> > > HTMLFrameOwnerElement.  
> > Removed cast.
> > Also see my earlier question about whether this needs to
> > > be guarded by isHTMLFrameElementBase.
> > 
> > The caller of this function is FrameLoaderClientImpl::createFrame(), which
> > shouldn't be called for a plugin, right? I was inspired by looking at the
> > comment in WebLocalFrameImpl::fromFrameOwnerElement().
> 
> I think you're right, we shouldn't get here for plugins (assuming plugins
can't
> create subframes:) ).  So then we don't need the isHTMLFrameElementBase check,
> right?

Latest PS still has isHTMLFrameElementBase check - did you want to remove it,
since we shouldn't get here for plugins?

https://codereview.chromium.org/1319863006/diff/120001/Source/core/html/HTMLB...
File Source/core/html/HTMLBodyElement.cpp (left):

https://codereview.chromium.org/1319863006/diff/120001/Source/core/html/HTMLB...
Source/core/html/HTMLBodyElement.cpp:163: if
(!isHTMLFrameElementBase(ownerElement))
On 2015/09/17 20:48:39, lazyboy wrote:
> On 2015/09/16 00:36:47, alexmos wrote:
> > Is removing this ok?  Do we want to do the below for plugins or is this not
> > called for them?
> 
> This function is called for plugins, but the document's frame does not
> have any owner:
> 
> WebViewPlugin::WebViewPlugin()
> calls WebViewImpl::setMainFrame() which always sets owner = 0
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Acknowledged.

https://codereview.chromium.org/1319863006/diff/120001/Source/core/html/HTMLF...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/1319863006/diff/120001/Source/core/html/HTMLF...
Source/core/html/HTMLFrameOwnerElement.cpp:205:
document().frame()->loader().client()->didChangeScrollingMode(contentFrame(),
scrollbarMode);
On 2015/09/17 20:48:39, lazyboy wrote:
> On 2015/09/16 00:36:47, alexmos wrote:
> > You're using three separate IPCs to propagate these updates to the browser
> > process, but then they get squished into the same
> > FrameMsg_SetFrameOwnerProperties on the way back to the renderer.  Should we
> be
> > consistent and either use one IPC here for all three things, or use
different
> > IPCs on browser->renderer path too?
> 
> I've made renderer->browser one IPC too, with the cost of saving the
> current frame owner properties state in RenderFrameImpl.

Is there a way to avoid storing them on RenderFrameImpl?  I'm worried about
whether they'd be initialized/reset there at proper times.  What about passing
them all through WebFrameClient any time one of them changes?

[lazyboy, 2015-09-21 18:04:51]

Addressed comments in patch set #9.
Any test ideas in blink? Or is browsertest in the companion chromium CL (yet to
write) enough?

https://codereview.chromium.org/1319863006/diff/80001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1319863006/diff/80001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:1761: marginWidth =
toHTMLFrameElementBase(ownerElement)->marginWidth();
On 2015/09/21 16:58:07, alexmos wrote:
> On 2015/09/16 00:36:47, alexmos wrote:
> > On 2015/09/15 01:40:33, lazyboy wrote:
> > > On 2015/09/02 21:37:06, alexmos wrote:
> > > > Is the cast necessary here?  marginWidth() and marginHeight() exist on
> > > > HTMLFrameOwnerElement.  
> > > Removed cast.
> > > Also see my earlier question about whether this needs to
> > > > be guarded by isHTMLFrameElementBase.
> > > 
> > > The caller of this function is FrameLoaderClientImpl::createFrame(), which
> > > shouldn't be called for a plugin, right? I was inspired by looking at the
> > > comment in WebLocalFrameImpl::fromFrameOwnerElement().
> > 
> > I think you're right, we shouldn't get here for plugins (assuming plugins
> can't
> > create subframes:) ).  So then we don't need the isHTMLFrameElementBase
check,
> > right?
> 
> Latest PS still has isHTMLFrameElementBase check - did you want to remove it,
> since we shouldn't get here for plugins?

Removed.

https://codereview.chromium.org/1319863006/diff/120001/Source/core/html/HTMLF...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/1319863006/diff/120001/Source/core/html/HTMLF...
Source/core/html/HTMLFrameOwnerElement.cpp:205:
document().frame()->loader().client()->didChangeScrollingMode(contentFrame(),
scrollbarMode);
On 2015/09/21 16:58:07, alexmos wrote:
> On 2015/09/17 20:48:39, lazyboy wrote:
> > On 2015/09/16 00:36:47, alexmos wrote:
> > > You're using three separate IPCs to propagate these updates to the browser
> > > process, but then they get squished into the same
> > > FrameMsg_SetFrameOwnerProperties on the way back to the renderer.  Should
we
> > be
> > > consistent and either use one IPC here for all three things, or use
> different
> > > IPCs on browser->renderer path too?
> > 
> > I've made renderer->browser one IPC too, with the cost of saving the
> > current frame owner properties state in RenderFrameImpl.
> 
> Is there a way to avoid storing them on RenderFrameImpl?  I'm worried about
> whether they'd be initialized/reset there at proper times.  What about passing
> them all through WebFrameClient any time one of them changes?

changed all functions to one: didChangeFrameOwnerProperties() and calling that
whenever one of the properties change.

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

On 2015/09/21 18:04:51, lazyboy wrote:
> Addressed comments in patch set #9.
Thanks!

> Any test ideas in blink? Or is browsertest in the companion chromium CL (yet
to
> write) enough?
Browsertests may be good enough.  On the Blink side you could add a test to
WebFrameTest.cpp, e.g., set the replicated properties on a remote FrameOwner and
see if they take effect.  There are various examples there of how to set up a
local frame with a remote parent (e.g., Nick just did it in
SendBeaconFromChildWithRemoteMainFrame).

[lazyboy, 2015-09-21 20:58:30]

I've added two blink tests in WebFrameTest in patch set #10

[alexmos, 2015-09-22 16:48:23]

Tests LGTM with nits, thanks for adding!

https://codereview.chromium.org/1319863006/diff/200001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1319863006/diff/200001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:1629: FrameView* frameView =
static_cast<WebLocalFrameImpl*>(localFrame)->frameView();
nit: maybe add a comment about what this is checking? (that scrollbars are
enabled by default)

https://codereview.chromium.org/1319863006/diff/200001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:1647: // Turn of scrolling in the subframe.
nit: s/of/off/

https://codereview.chromium.org/1319863006/diff/200001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/1319863006/diff/200001/public/web/WebFrame.h#...
public/web/WebFrame.h:175: // FIXME: Currently, the update only takes effect on
next frame navigation.
We don't need this FIXME on sandbox flags, as this is intentional for them (this
is what the spec says).  Did you mean to put this on setFrameOwnerProperties
below?

Also, Blink now uses TODO(someone) instead of FIXME.

[lazyboy, 2015-09-22 22:02:43]

https://codereview.chromium.org/1319863006/diff/200001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1319863006/diff/200001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:1629: FrameView* frameView =
static_cast<WebLocalFrameImpl*>(localFrame)->frameView();
On 2015/09/22 16:48:23, alexmos wrote:
> nit: maybe add a comment about what this is checking? (that scrollbars are
> enabled by default)

Done.

https://codereview.chromium.org/1319863006/diff/200001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:1647: // Turn of scrolling in the subframe.
On 2015/09/22 16:48:23, alexmos wrote:
> nit: s/of/off/

Done.

https://codereview.chromium.org/1319863006/diff/200001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/1319863006/diff/200001/public/web/WebFrame.h#...
public/web/WebFrame.h:175: // FIXME: Currently, the update only takes effect on
next frame navigation.
On 2015/09/22 16:48:23, alexmos wrote:
> We don't need this FIXME on sandbox flags, as this is intentional for them
(this
> is what the spec says).  Did you mean to put this on setFrameOwnerProperties
> below?
Yes.
> 
> Also, Blink now uses TODO(someone) instead of FIXME.

Done.

[lazyboy, 2015-11-11 11:27:15]

lazyboy@chromium.org changed reviewers:
- alexmos@chromium.org, dcheng@chromium.org