Initial work on a new <view> element backed by a mojo::View.

Initial work on a new <view> element backed by a mojo::View.

This CL introduces an HTMLViewElement which, when inserted into a document,
causes a mojo::View to be created and navigated to the provided URL. No
compositing is done, but the view manager handles the rendering (as I
understand it).

R=abarth@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/84fd0f44ed37b6acdf8e6369e2ea740819568e27

[Matt Perry, 2014-11-06 22:28:26]

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

[Matt Perry, 2014-11-06 22:28:27]

This works well enough in that it actually draws the subframe's content. Let me
know if I'm on the right track.

[esprehn, 2014-11-06 22:40:27]

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

[esprehn, 2014-11-06 22:40:29]

https://codereview.chromium.org/708903002/diff/1/sky/engine/core/html/HTMLVie...
File sky/engine/core/html/HTMLViewElement.cpp (right):

https://codereview.chromium.org/708903002/diff/1/sky/engine/core/html/HTMLVie...
sky/engine/core/html/HTMLViewElement.cpp:11: * version 2 of the License, or (at
your option) any later version.
ditto

https://codereview.chromium.org/708903002/diff/1/sky/engine/core/html/HTMLVie...
sky/engine/core/html/HTMLViewElement.cpp:35: #include "core/frame/UseCounter.h"
Lots of these aren't needed.

https://codereview.chromium.org/708903002/diff/1/sky/engine/core/html/HTMLVie...
sky/engine/core/html/HTMLViewElement.cpp:86: openURL();
You can just put this in ::insertedInto, we don't call synchronous script
anymore so we don't need this callback.

https://codereview.chromium.org/708903002/diff/1/sky/engine/core/html/HTMLVie...
sky/engine/core/html/HTMLViewElement.cpp:92: if (insertionPoint->inDocument() &&
document().isHTMLDocument() && !insertionPoint->isInShadowTree()) {
Remove isHTMLDocument() and isInShadowTree checks

https://codereview.chromium.org/708903002/diff/1/sky/engine/core/html/HTMLVie...
File sky/engine/core/html/HTMLViewElement.h (right):

https://codereview.chromium.org/708903002/diff/1/sky/engine/core/html/HTMLVie...
sky/engine/core/html/HTMLViewElement.h:22: */
Use the 3 line google copyright.

https://codereview.chromium.org/708903002/diff/1/sky/engine/core/rendering/Re...
File sky/engine/core/rendering/RenderRemote.h (right):

https://codereview.chromium.org/708903002/diff/1/sky/engine/core/rendering/Re...
sky/engine/core/rendering/RenderRemote.h:4: * Redistribution and use in source
and binary forms, with or without
All the files should use the google 3 line copyright.

https://codereview.chromium.org/708903002/diff/1/sky/engine/web/FrameLoaderCl...
File sky/engine/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/708903002/diff/1/sky/engine/web/FrameLoaderCl...
sky/engine/web/FrameLoaderClientImpl.cpp:260: ownerElement->offsetHeight());
You want clientLeft etc. not offset, your size doesn't respect padding.

[esprehn, 2014-11-06 22:40:46]

Also lets call this element <iframe>, not <view>.

[abarth-chromium, 2014-11-06 22:47:08]

On 2014/11/06 at 22:28:27, mpcomplete wrote:
> This works well enough in that it actually draws the subframe's content. Let
me know if I'm on the right track.

Neat!

[abarth-chromium, 2014-11-06 22:55:07]

This looks like you're on a good track, but we should separate creating from
sizing.  The size information isn't available when the element is inserted in
the element tree and the size will change whenever there is a layout.

In this CL, maybe we should just start with creation and then add a notification
pathway for size information in the next CL?

https://codereview.chromium.org/708903002/diff/1/sky/engine/core/html/HTMLVie...
File sky/engine/core/html/HTMLViewElement.cpp (right):

https://codereview.chromium.org/708903002/diff/1/sky/engine/core/html/HTMLVie...
sky/engine/core/html/HTMLViewElement.cpp:62: ScriptWrappable::init(this);
This isn't needed either, but we haven't bulk deleted it from the codebase yet. 
:)

https://codereview.chromium.org/708903002/diff/1/sky/engine/core/html/HTMLVie...
sky/engine/core/html/HTMLViewElement.cpp:106: if (inDocument())
Why do we check isDocument here but !document().frame() elsewhere?  I'd guess
that !document().frame() is the check you want.

https://codereview.chromium.org/708903002/diff/1/sky/engine/web/FrameLoaderCl...
File sky/engine/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/708903002/diff/1/sky/engine/web/FrameLoaderCl...
sky/engine/web/FrameLoaderClientImpl.cpp:260: ownerElement->offsetHeight());
On 2014/11/06 at 22:40:28, esprehn wrote:
> You want clientLeft etc. not offset, your size doesn't respect padding.

Don't we need to wait until layout happens before this information is available?
 As written, this will cause a layout in the middle of constructing the DOM.

https://codereview.chromium.org/708903002/diff/1/sky/viewer/document_view.cc
File sky/viewer/document_view.cc (right):

https://codereview.chromium.org/708903002/diff/1/sky/viewer/document_view.cc#...
sky/viewer/document_view.cc:160: root_->AddChild(child);
Do we need to observe child so we can destroy it later?

[abarth-chromium, 2014-11-06 22:55:09]

This looks like you're on a good track, but we should separate creating from
sizing.  The size information isn't available when the element is inserted in
the element tree and the size will change whenever there is a layout.

In this CL, maybe we should just start with creation and then add a notification
pathway for size information in the next CL?

[Matt Perry, 2014-11-06 23:38:51]

https://codereview.chromium.org/708903002/diff/1/sky/engine/core/html/HTMLVie...
File sky/engine/core/html/HTMLViewElement.cpp (right):

https://codereview.chromium.org/708903002/diff/1/sky/engine/core/html/HTMLVie...
sky/engine/core/html/HTMLViewElement.cpp:11: * version 2 of the License, or (at
your option) any later version.
On 2014/11/06 22:40:28, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/708903002/diff/1/sky/engine/core/html/HTMLVie...
sky/engine/core/html/HTMLViewElement.cpp:62: ScriptWrappable::init(this);
On 2014/11/06 22:55:07, abarth wrote:
> This isn't needed either, but we haven't bulk deleted it from the codebase
yet. 
> :)

Done.

https://codereview.chromium.org/708903002/diff/1/sky/engine/core/html/HTMLVie...
sky/engine/core/html/HTMLViewElement.cpp:86: openURL();
On 2014/11/06 22:40:28, esprehn wrote:
> You can just put this in ::insertedInto, we don't call synchronous script
> anymore so we don't need this callback.

Done.

https://codereview.chromium.org/708903002/diff/1/sky/engine/core/html/HTMLVie...
sky/engine/core/html/HTMLViewElement.cpp:92: if (insertionPoint->inDocument() &&
document().isHTMLDocument() && !insertionPoint->isInShadowTree()) {
On 2014/11/06 22:40:28, esprehn wrote:
> Remove isHTMLDocument() and isInShadowTree checks

Done.

https://codereview.chromium.org/708903002/diff/1/sky/engine/core/html/HTMLVie...
sky/engine/core/html/HTMLViewElement.cpp:106: if (inDocument())
On 2014/11/06 22:55:07, abarth wrote:
> Why do we check isDocument here but !document().frame() elsewhere?  I'd guess
> that !document().frame() is the check you want.

I'm not sure. I copied a lot of this from blink's HTMLFrameElementBase, so this
is left over from that. But when I change it to 

    if (document().frame())
        openURL();

the view is no longer loaded. (Or did you mean I should call openURL only if the
frame is NULL?)

https://codereview.chromium.org/708903002/diff/1/sky/engine/core/html/HTMLVie...
File sky/engine/core/html/HTMLViewElement.h (right):

https://codereview.chromium.org/708903002/diff/1/sky/engine/core/html/HTMLVie...
sky/engine/core/html/HTMLViewElement.h:22: */
On 2014/11/06 22:40:28, esprehn wrote:
> Use the 3 line google copyright.

Done.

https://codereview.chromium.org/708903002/diff/1/sky/engine/core/rendering/Re...
File sky/engine/core/rendering/RenderRemote.h (right):

https://codereview.chromium.org/708903002/diff/1/sky/engine/core/rendering/Re...
sky/engine/core/rendering/RenderRemote.h:4: * Redistribution and use in source
and binary forms, with or without
On 2014/11/06 22:40:28, esprehn wrote:
> All the files should use the google 3 line copyright.

Done.

https://codereview.chromium.org/708903002/diff/1/sky/engine/web/FrameLoaderCl...
File sky/engine/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/708903002/diff/1/sky/engine/web/FrameLoaderCl...
sky/engine/web/FrameLoaderClientImpl.cpp:260: ownerElement->offsetHeight());
On 2014/11/06 22:55:07, abarth wrote:
> On 2014/11/06 at 22:40:28, esprehn wrote:
> > You want clientLeft etc. not offset, your size doesn't respect padding.
> 
> Don't we need to wait until layout happens before this information is
available?
>  As written, this will cause a layout in the middle of constructing the DOM.

Done.

https://codereview.chromium.org/708903002/diff/1/sky/viewer/document_view.cc
File sky/viewer/document_view.cc (right):

https://codereview.chromium.org/708903002/diff/1/sky/viewer/document_view.cc#...
sky/viewer/document_view.cc:160: root_->AddChild(child);
On 2014/11/06 22:55:07, abarth wrote:
> Do we need to observe child so we can destroy it later?

I must admit I don't understand the ownership here. Does root_ own its children?
Who owns root_? When we call child->Embed, a new DocumentView is created, whose
root_ node is for some reason different from child.

[abarth-chromium, 2014-11-06 23:45:20]

On 2014/11/06 at 23:38:51, mpcomplete wrote:
> I'm not sure. I copied a lot of this from blink's HTMLFrameElementBase, so
this is left over from that. But when I change it to 
> 
>     if (document().frame())
>         openURL();
> 
> the view is no longer loaded. (Or did you mean I should call openURL only if
the frame is NULL?)

That's odd.  "Frame" refers to the visual part of the system.  We'll want to
openURL only if we're hooked up to the visual part of the system (and not just
in some random document created by script floating off in space).

It looks like openURL already bails out if there's no frame, so maybe we can
just skip this branch.

>
https://codereview.chromium.org/708903002/diff/1/sky/viewer/document_view.cc#...
> sky/viewer/document_view.cc:160: root_->AddChild(child);
> On 2014/11/06 22:55:07, abarth wrote:
> > Do we need to observe child so we can destroy it later?
> 
> I must admit I don't understand the ownership here. Does root_ own its
children?

I'm not sure.  We can ask sky@

> Who owns root_?

I'm pretty sure root_ is owned by someone else and we don't need to worry about
it.

> When we call child->Embed, a new DocumentView is created, whose root_ node is
for some reason different from child.

In the Mojo view system, there's a views server and many view clients.  The
child we create and the DocumentView's root_ represent the same view on the
server (the implementation type is ServerView), but they're two separate clients
so they get two separate client representations of the view.  This makes more
sense if you think about the multi-process case where the contents of the view
might be rendered in another process.  In that case, we'd need to use two
separate client representations of the view.  My guess is that they all have the
same view_id.

[Matt Perry, 2014-11-07 17:27:40]

Ok, I've addressed the last review comments and renamed the element to iframe.

https://codereview.chromium.org/708903002/diff/1/sky/engine/core/html/HTMLVie...
File sky/engine/core/html/HTMLViewElement.cpp (right):

https://codereview.chromium.org/708903002/diff/1/sky/engine/core/html/HTMLVie...
sky/engine/core/html/HTMLViewElement.cpp:106: if (inDocument())
On 2014/11/06 23:38:50, Matt Perry wrote:
> On 2014/11/06 22:55:07, abarth wrote:
> > Why do we check isDocument here but !document().frame() elsewhere?  I'd
guess
> > that !document().frame() is the check you want.
> 
> I'm not sure. I copied a lot of this from blink's HTMLFrameElementBase, so
this
> is left over from that. But when I change it to 
> 
>     if (document().frame())
>         openURL();
> 
> the view is no longer loaded. (Or did you mean I should call openURL only if
the
> frame is NULL?)

Nevermind, I was wrong. I must've had a bug somewhere else. I've removed the
redundant checks.

https://codereview.chromium.org/708903002/diff/1/sky/viewer/document_view.cc
File sky/viewer/document_view.cc (right):

https://codereview.chromium.org/708903002/diff/1/sky/viewer/document_view.cc#...
sky/viewer/document_view.cc:160: root_->AddChild(child);
On 2014/11/06 23:38:50, Matt Perry wrote:
> On 2014/11/06 22:55:07, abarth wrote:
> > Do we need to observe child so we can destroy it later?
> 
> I must admit I don't understand the ownership here. Does root_ own its
children?
> Who owns root_? When we call child->Embed, a new DocumentView is created,
whose
> root_ node is for some reason different from child.

Sky says we need to delete the children manually, because root_ here is a remote
View and will not delete children created locally here. I've added that below.

[abarth-chromium, 2014-11-07 19:08:30]

This is great.  LGTM!

https://codereview.chromium.org/708903002/diff/40001/sky/engine/core/html/HTM...
File sky/engine/core/html/HTMLIFrameElement.h (right):

https://codereview.chromium.org/708903002/diff/40001/sky/engine/core/html/HTM...
sky/engine/core/html/HTMLIFrameElement.h:35: AtomicString m_URL;
Is there a reason to store this value?

[Matt Perry, 2014-11-07 19:14:11]

https://codereview.chromium.org/708903002/diff/40001/sky/engine/core/html/HTM...
File sky/engine/core/html/HTMLIFrameElement.cpp (right):

https://codereview.chromium.org/708903002/diff/40001/sky/engine/core/html/HTM...
sky/engine/core/html/HTMLIFrameElement.cpp:40: openURL();
Re: storing m_URL. If insertedInto is called after parseAttribute (which I think
will be most /all of the time), we need the URL we parsed earlier.

[Matt Perry, 2014-11-07 19:14:18]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-11-07 19:18:24]

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

[commit-bot: I haz the power, 2014-11-07 19:18:26]

Commit queue rejected this change because it did not recognize the base URL.
Please commit your change manually.

[abarth-chromium, 2014-11-07 19:18:39]

On 2014/11/07 at 19:14:11, mpcomplete wrote:
>
https://codereview.chromium.org/708903002/diff/40001/sky/engine/core/html/HTM...
> File sky/engine/core/html/HTMLIFrameElement.cpp (right):
> 
>
https://codereview.chromium.org/708903002/diff/40001/sky/engine/core/html/HTM...
> sky/engine/core/html/HTMLIFrameElement.cpp:40: openURL();
> Re: storing m_URL. If insertedInto is called after parseAttribute (which I
think will be most /all of the time), we need the URL we parsed earlier.

Can't we just call getAttribute and do the parsing when we create the view?

[Matt Perry, 2014-11-07 19:28:57]

On 2014/11/07 19:18:39, abarth wrote:
> On 2014/11/07 at 19:14:11, mpcomplete wrote:
> >
>
https://codereview.chromium.org/708903002/diff/40001/sky/engine/core/html/HTM...
> > File sky/engine/core/html/HTMLIFrameElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/708903002/diff/40001/sky/engine/core/html/HTM...
> > sky/engine/core/html/HTMLIFrameElement.cpp:40: openURL();
> > Re: storing m_URL. If insertedInto is called after parseAttribute (which I
> think will be most /all of the time), we need the URL we parsed earlier.
> 
> Can't we just call getAttribute and do the parsing when we create the view?

Looks like we can. I've done that, and renamed openURL to createView.

[Matt Perry, 2014-11-07 19:36:31]

Committed patchset #4 (id:60001) manually as
84fd0f44ed37b6acdf8e6369e2ea740819568e27 (presubmit successful).