Add a separate call to initialize the bounds for a sky <iframe> element.

Add a separate call to initialize the bounds for a sky <iframe> element.

Also rename the methods to refer to "frames" not "views".

R=abarth@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/7375f7ab5c6cffe257cbad9e3f45fe006a9fb78c

[Matt Perry, 2014-11-07 21:15:14]

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

[esprehn, 2014-11-07 21:22:14]

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

[esprehn, 2014-11-07 21:22:15]

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

https://codereview.chromium.org/682413006/diff/1/sky/engine/core/rendering/Re...
sky/engine/core/rendering/RenderRemote.cpp:31: HTMLIFrameElement* frame =
static_cast<HTMLIFrameElement*>(node());
toHTMLIframeElement(), use the macros to define the correct cast.

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

https://codereview.chromium.org/682413006/diff/1/sky/viewer/document_view.cc#...
sky/viewer/document_view.cc:156: return static_cast<uint32_t>(-1);
It seems wrong to continuously cast -1 to a uint for the invalid id. Should
there be a constant for this?

[abarth-chromium, 2014-11-07 21:25:48]

abarth@chromium.org changed reviewers:
- esprehn@chromium.org

[abarth-chromium, 2014-11-07 21:25:50]

https://codereview.chromium.org/682413006/diff/1/sky/engine/core/loader/Empty...
File sky/engine/core/loader/EmptyClients.h (right):

https://codereview.chromium.org/682413006/diff/1/sky/engine/core/loader/Empty...
sky/engine/core/loader/EmptyClients.h:133: virtual uint32_t
createChildFrame(const KURL&) override { return static_cast<uint32_t>(-1); }
-1 is a strange uint32_t.  Maybe std::numeric_limits<uint32_t>::max() ?

Actually, I think 0 is a reserved view ID for this purpose.

https://codereview.chromium.org/682413006/diff/1/sky/engine/core/loader/Frame...
File sky/engine/core/loader/FrameLoaderClient.h (right):

https://codereview.chromium.org/682413006/diff/1/sky/engine/core/loader/Frame...
sky/engine/core/loader/FrameLoaderClient.h:93: virtual void
initializeChildFrame(uint32_t, const IntRect&) = 0;
Seems like createChildFrame should return an object that has the rest of these
functions hung off it.  Otherwise we're going to spam FrameLoaderClient with a
bunch of functions that take a uint32_t as a fake |this| parameter.

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

https://codereview.chromium.org/682413006/diff/1/sky/engine/core/rendering/Re...
sky/engine/core/rendering/RenderRemote.cpp:33: frame->frame_id(),
pixelSnappedIntRect(frameRect()));
"initialize" isn't quite the right term here.  It's likely that layout will be
called many times over the lifetime of this object.  Maybe "setBounds"  is a
better name?

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

https://codereview.chromium.org/682413006/diff/1/sky/engine/web/FrameLoaderCl...
sky/engine/web/FrameLoaderClientImpl.cpp:259: return -1;
We shouldn't ever get into this situation.  Maybe ASSERT_NOT_REACHED?

https://codereview.chromium.org/682413006/diff/1/sky/engine/web/FrameLoaderCl...
sky/engine/web/FrameLoaderClientImpl.cpp:259: return -1;
We shouldn't ever get into this situation.  Maybe ASSERT_NOT_REACHED?

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

https://codereview.chromium.org/682413006/diff/1/sky/viewer/document_view.cc#...
sky/viewer/document_view.cc:162: return child->id();
Why not just return |child| from this function?  Then RenderRemote can call
mojo::View::SetBounds directly.  We'll need to be careful with lifetime to make
sure we cut the connection between HTMLIFrameElement and this mojo::View when
the Document detaches from the Frame, but that should be doable.

[abarth-chromium, 2014-11-07 21:37:25]

I chatted with esprehn and eseidel, and it turns out calling SetBounds inside
layout isn't quite right because we'll generate extra SetBounds calls in some
cases because some layout modules involve multiple passes (e.g., flexbox). 
Instead, we'll need to defer the work to later in the pipeline.

My advice is to continue with this current approach in this CL (to build the
notification pathway) and then we can implement the work deferral in the next
CL.

[Matt Perry, 2014-11-10 23:00:57]

PTAL

https://codereview.chromium.org/682413006/diff/1/sky/engine/core/loader/Empty...
File sky/engine/core/loader/EmptyClients.h (right):

https://codereview.chromium.org/682413006/diff/1/sky/engine/core/loader/Empty...
sky/engine/core/loader/EmptyClients.h:133: virtual uint32_t
createChildFrame(const KURL&) override { return static_cast<uint32_t>(-1); }
On 2014/11/07 21:25:50, abarth wrote:
> -1 is a strange uint32_t.  Maybe std::numeric_limits<uint32_t>::max() ?
> 
> Actually, I think 0 is a reserved view ID for this purpose.

view.cc initializes id_ to -1 when it's not defined. But I'm using objects now
instead.

https://codereview.chromium.org/682413006/diff/1/sky/engine/core/loader/Frame...
File sky/engine/core/loader/FrameLoaderClient.h (right):

https://codereview.chromium.org/682413006/diff/1/sky/engine/core/loader/Frame...
sky/engine/core/loader/FrameLoaderClient.h:93: virtual void
initializeChildFrame(uint32_t, const IntRect&) = 0;
On 2014/11/07 21:25:50, abarth wrote:
> Seems like createChildFrame should return an object that has the rest of these
> functions hung off it.  Otherwise we're going to spam FrameLoaderClient with a
> bunch of functions that take a uint32_t as a fake |this| parameter.

Done.

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

https://codereview.chromium.org/682413006/diff/1/sky/engine/core/rendering/Re...
sky/engine/core/rendering/RenderRemote.cpp:31: HTMLIFrameElement* frame =
static_cast<HTMLIFrameElement*>(node());
On 2014/11/07 21:22:15, esprehn wrote:
> toHTMLIframeElement(), use the macros to define the correct cast.

Done.

https://codereview.chromium.org/682413006/diff/1/sky/engine/core/rendering/Re...
sky/engine/core/rendering/RenderRemote.cpp:33: frame->frame_id(),
pixelSnappedIntRect(frameRect()));
On 2014/11/07 21:25:50, abarth wrote:
> "initialize" isn't quite the right term here.  It's likely that layout will be
> called many times over the lifetime of this object.  Maybe "setBounds"  is a
> better name?

Done.

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

https://codereview.chromium.org/682413006/diff/1/sky/engine/web/FrameLoaderCl...
sky/engine/web/FrameLoaderClientImpl.cpp:259: return -1;
On 2014/11/07 21:25:50, abarth wrote:
> We shouldn't ever get into this situation.  Maybe ASSERT_NOT_REACHED?

Done.

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

https://codereview.chromium.org/682413006/diff/1/sky/viewer/document_view.cc#...
sky/viewer/document_view.cc:156: return static_cast<uint32_t>(-1);
On 2014/11/07 21:22:15, esprehn wrote:
> It seems wrong to continuously cast -1 to a uint for the invalid id. Should
> there be a constant for this?

Switched to using objects instead of IDs.

https://codereview.chromium.org/682413006/diff/1/sky/viewer/document_view.cc#...
sky/viewer/document_view.cc:162: return child->id();
On 2014/11/07 21:25:50, abarth wrote:
> Why not just return |child| from this function?  Then RenderRemote can call
> mojo::View::SetBounds directly.  We'll need to be careful with lifetime to
make
> sure we cut the connection between HTMLIFrameElement and this mojo::View when
> the Document detaches from the Frame, but that should be doable.

I wasn't sure about the layering rules, but that's definitely easier.

[abarth-chromium, 2014-11-10 23:03:51]

https://codereview.chromium.org/682413006/diff/40001/sky/engine/core/frame/Re...
File sky/engine/core/frame/RemoteFrame.h (right):

https://codereview.chromium.org/682413006/diff/40001/sky/engine/core/frame/Re...
sky/engine/core/frame/RemoteFrame.h:18: // This class encapsulates the mojo View
that backs an HTMLIFrameElement.
Why encapsulate?  Can't folks just use a mojo::View directly?  I think you're
missing RemoteFrame.cpp so maybe there's non-trivial code there. :)

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

https://codereview.chromium.org/682413006/diff/40001/sky/engine/core/html/HTM...
sky/engine/core/html/HTMLIFrameElement.cpp:24: m_contentFrame(nullptr)
No need for this.  OwnPtr does it automatically.

[Matt Perry, 2014-11-10 23:35:34]

https://codereview.chromium.org/682413006/diff/40001/sky/engine/core/frame/Re...
File sky/engine/core/frame/RemoteFrame.h (right):

https://codereview.chromium.org/682413006/diff/40001/sky/engine/core/frame/Re...
sky/engine/core/frame/RemoteFrame.h:18: // This class encapsulates the mojo View
that backs an HTMLIFrameElement.
On 2014/11/10 23:03:51, abarth wrote:
> Why encapsulate?  Can't folks just use a mojo::View directly?  I think you're
> missing RemoteFrame.cpp so maybe there's non-trivial code there. :)

I was expecting there to be more methods eventually, like detach(). But I guess
I can encapsulate later if needed.

[abarth-chromium, 2014-11-10 23:47:41]

lgtm

https://codereview.chromium.org/682413006/diff/80001/sky/engine/core/loader/F...
File sky/engine/core/loader/FrameLoaderClient.h (right):

https://codereview.chromium.org/682413006/diff/80001/sky/engine/core/loader/F...
sky/engine/core/loader/FrameLoaderClient.h:43: class View;
Don't indent pls.  (The rest of the file is wrong for being indented.)

[Matt Perry, 2014-11-11 16:12:02]

Committed patchset #6 (id:100001) manually as
7375f7ab5c6cffe257cbad9e3f45fe006a9fb78c (presubmit successful).