Convert HTMLFrameOwnerElement and FocusController to use Frame.

Convert HTMLFrameOwnerElement and FocusController to use Frame.

HTMLFrameOwnerElement and FocusController need to be able to hold
pointers to RemoteFrames as well as LocalFrames. The appropriate
members are converted from LocalFrame to the base class Frame in order
to support this.

BUG=346764
R=dcheng@chromium.org, eseidel@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170347

[kenrb, 2014-03-07 21:12:42]

Daniel, do you want to take first crack at reviewing this?

I tried to anticipate which places that access the Frame pointers from these
classes actually need to know about RemoteFrames and write handling code as
appropriate. We will have to see what assertions appear when we actually
instantiate RemoteFrame and run tests.

[eseidel, 2014-03-07 22:42:53]

https://codereview.chromium.org/189573002/diff/30001/Source/bindings/v8/V8Win...
File Source/bindings/v8/V8WindowShell.cpp (right):

https://codereview.chromium.org/189573002/diff/30001/Source/bindings/v8/V8Win...
Source/bindings/v8/V8WindowShell.cpp:435: if
(element->hasTagName(HTMLNames::iframeTag) && (frame =
toLocalFrame(toHTMLIFrameElement(element)->contentFrame())))
Confused.  This isn't always true, is it?  Shouldn't we check to see that the
iframe has a real local frame?  I presumed that would be the point of using the
compiler to do this, that it would force us to audit all callers?

[kenrb, 2014-03-07 23:56:44]

https://codereview.chromium.org/189573002/diff/30001/Source/bindings/v8/V8Win...
File Source/bindings/v8/V8WindowShell.cpp (right):

https://codereview.chromium.org/189573002/diff/30001/Source/bindings/v8/V8Win...
Source/bindings/v8/V8WindowShell.cpp:435: if
(element->hasTagName(HTMLNames::iframeTag) && (frame =
toLocalFrame(toHTMLIFrameElement(element)->contentFrame())))
On 2014/03/07 22:42:53, eseidel wrote:
> Confused.  This isn't always true, is it?  Shouldn't we check to see that the
> iframe has a real local frame?  I presumed that would be the point of using
the
> compiler to do this, that it would force us to audit all callers?

Sorry about that, I had added a bunch of checks but they were on the wrong
branch.

In general, my intent was to just use toLocalFrame() on code paths where we
should only get LocalFrames, and do a check on paths where we might have to
handle both. toLocalFrame() contains an implicit assertion that we are right.
Unfortunately we aren't far enough along to instantiate a RemoteFrame and test
to see which asserts fire.

Also, in the places where we check isLocalFrame() but have to handle both types,
the correct behavior for RemoteFrames is not always obvious.

[kenrb, 2014-03-12 23:15:24]

Eric: Daniel is at an offsite, are you able to have a look at this?

[dcheng, 2014-03-17 18:17:44]

Very neat. Mostly minor comments/questions to try to understand the followup
work.

https://codereview.chromium.org/189573002/diff/30002/Source/bindings/v8/Bindi...
File Source/bindings/v8/BindingSecurity.h (right):

https://codereview.chromium.org/189573002/diff/30002/Source/bindings/v8/Bindi...
Source/bindings/v8/BindingSecurity.h:34: #include "core/frame/LocalFrame.h"
Hm... can't this have remained a forward include?

https://codereview.chromium.org/189573002/diff/30002/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8HTMLFrameElementCustom.cpp (right):

https://codereview.chromium.org/189573002/diff/30002/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8HTMLFrameElementCustom.cpp:38: #include
"core/frame/LocalFrame.h"
Not clear what this new include is for.

https://codereview.chromium.org/189573002/diff/30002/Source/core/dom/TreeScop...
File Source/core/dom/TreeScope.cpp (right):

https://codereview.chromium.org/189573002/diff/30002/Source/core/dom/TreeScop...
Source/core/dom/TreeScope.cpp:354: // FIXME(kenrb): The toLocalFrame() cast
should be removed when RemoteFrames can have FrameTrees.
I don't really understand what this comment has to do with the changes here. It
doesn't look like we're doing anything with the frame tree here?

https://codereview.chromium.org/189573002/diff/30002/Source/core/editing/Edit...
File Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/189573002/diff/30002/Source/core/editing/Edit...
Source/core/editing/Editor.cpp:810:
toLocalFrame(page->focusController().focusedOrMainFrame())->selection().revealSelection(ScrollAlignment::alignCenterIfNeeded);
Is this always guaranteed to be a local frame? That doesn't seem like it would
always be the case.

https://codereview.chromium.org/189573002/diff/30002/Source/core/frame/Frame.cpp
File Source/core/frame/Frame.cpp (right):

https://codereview.chromium.org/189573002/diff/30002/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:192: doc->topDocument().clearAXObjectCache();
Do we need a FIXME here? What will topDocument() do in the presence of remote
frames?

https://codereview.chromium.org/189573002/diff/30002/Source/core/html/HTMLFra...
File Source/core/html/HTMLFrameElementBase.cpp (right):

https://codereview.chromium.org/189573002/diff/30002/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameElementBase.cpp:95: if (!contentFrame() ||
scriptURL.isEmpty() || !contentFrame()->isLocalFrame())
Do we need a FIXME here? Isn't location settable on a remote frame
(http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.html#sec...

https://codereview.chromium.org/189573002/diff/30002/Source/core/html/HTMLFra...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/189573002/diff/30002/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameOwnerElement.cpp:86:
toLocalFrame(frame)->loader().frameDetached();
Do we need a FIXME here? How are we planning on triggering the detach of remote
frames?

https://codereview.chromium.org/189573002/diff/30002/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameOwnerElement.cpp:128: if (contentFrame() &&
contentFrame()->isLocalFrame()) {
Similar comment to my earlier comment in HTMLFrameElementBase::openURL.

https://codereview.chromium.org/189573002/diff/30002/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/189573002/diff/30002/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:1753: // Cross-process drag and drop is not
yet supported.
Tag this with FIXME? Maybe we should track these instances in a bug somewhere as
well to make them easier to chase down later...

https://codereview.chromium.org/189573002/diff/30002/Source/core/page/FocusCo...
File Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/189573002/diff/30002/Source/core/page/FocusCo...
Source/core/page/FocusController.cpp:242: RefPtr<LocalFrame> oldFrame =
(m_focusedFrame && m_focusedFrame->isLocalFrame()) ?
toLocalFrame(m_focusedFrame.get()) : 0;
This logic is incomplete atm and depends on proper inter-frame focus handling
being moved out of Blink as well right?

https://codereview.chromium.org/189573002/diff/30002/Source/core/page/PageSer...
File Source/core/page/PageSerializer.cpp (right):

https://codereview.chromium.org/189573002/diff/30002/Source/core/page/PageSer...
Source/core/page/PageSerializer.cpp:158: // RemoteFrames not currently supported
here.
But eventually, right? Maybe tag with a FIXME?

Another question is how this interacts with WebPageSerializer. It looks like
that doesn't even add a frame to the list of frames to visit (and presumably
serialize). Is this code used by a different codepath in Source/core as well?
I'm assuming so (it looks like it might be called by StyledMarkupAccumulator?)
but it'd be good to have confirmation.

https://codereview.chromium.org/189573002/diff/30002/Source/web/ContextMenuCl...
File Source/web/ContextMenuClientImpl.cpp (right):

https://codereview.chromium.org/189573002/diff/30002/Source/web/ContextMenuCl...
Source/web/ContextMenuClientImpl.cpp:198: if
(toLocalFrame(m_webView->focusedWebCoreFrame())->editor().canUndo())
Hm... what guarantees that these are always focused? Is it because the only ways
to invoke the context menu require that the frame have focus (right-clicking,
context menu keyboard button)? If so, should we add an assert?

Or is this currently true because of the way WebViewImpl filters out events for
RemoteFrame?

https://codereview.chromium.org/189573002/diff/30002/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/189573002/diff/30002/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1290: LocalFrame* frame =
toLocalFrame(focusedWebCoreFrame());
Hmm... is focusedWebCoreFrame() eventually going to return LocalFrame once
'process focus tracking' is in Chrome?

https://codereview.chromium.org/189573002/diff/30002/Source/web/WebViewImpl.h
File Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/189573002/diff/30002/Source/web/WebViewImpl.h...
Source/web/WebViewImpl.h:72: class LocalFrame;
Why the forward declare for Color and LocalFrame?

[kenrb, 2014-03-18 20:01:18]

https://codereview.chromium.org/189573002/diff/30002/Source/bindings/v8/Bindi...
File Source/bindings/v8/BindingSecurity.h (right):

https://codereview.chromium.org/189573002/diff/30002/Source/bindings/v8/Bindi...
Source/bindings/v8/BindingSecurity.h:34: #include "core/frame/LocalFrame.h"
On 2014/03/17 18:17:44, dcheng wrote:
> Hm... can't this have remained a forward include?

No because a lot of clients call shouldAllowAccessToFrame() using LocalFrames,
and they don't include LocalFrame.h themselves, so if we forward declare then
they wouldn't be able to do the implicit conversion.

https://codereview.chromium.org/189573002/diff/30002/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8HTMLFrameElementCustom.cpp (right):

https://codereview.chromium.org/189573002/diff/30002/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8HTMLFrameElementCustom.cpp:38: #include
"core/frame/LocalFrame.h"
On 2014/03/17 18:17:44, dcheng wrote:
> Not clear what this new include is for.

Oops, not needed. I had originally added it when looking at the forward
declaration problem mentioned in the other comment.

https://codereview.chromium.org/189573002/diff/30002/Source/core/dom/TreeScop...
File Source/core/dom/TreeScope.cpp (right):

https://codereview.chromium.org/189573002/diff/30002/Source/core/dom/TreeScop...
Source/core/dom/TreeScope.cpp:354: // FIXME(kenrb): The toLocalFrame() cast
should be removed when RemoteFrames can have FrameTrees.
On 2014/03/17 18:17:44, dcheng wrote:
> I don't really understand what this comment has to do with the changes here.
It
> doesn't look like we're doing anything with the frame tree here?

I've expanded the comment. focusedFrameOwnerElement can't be changed to take a
Frame instead of a LocalFrame until FrameTree works with Frames.

https://codereview.chromium.org/189573002/diff/30002/Source/core/editing/Edit...
File Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/189573002/diff/30002/Source/core/editing/Edit...
Source/core/editing/Editor.cpp:810:
toLocalFrame(page->focusController().focusedOrMainFrame())->selection().revealSelection(ScrollAlignment::alignCenterIfNeeded);
On 2014/03/17 18:17:44, dcheng wrote:
> Is this always guaranteed to be a local frame? That doesn't seem like it would
> always be the case.

I was thinking a selected frame would have to be local in order to be editing
it, but I'm not sure about that so I've added the check for the sake of caution.

https://codereview.chromium.org/189573002/diff/30002/Source/core/frame/Frame.cpp
File Source/core/frame/Frame.cpp (right):

https://codereview.chromium.org/189573002/diff/30002/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:192: doc->topDocument().clearAXObjectCache();
On 2014/03/17 18:17:44, dcheng wrote:
> Do we need a FIXME here? What will topDocument() do in the presence of remote
> frames?

Done.

https://codereview.chromium.org/189573002/diff/30002/Source/core/html/HTMLFra...
File Source/core/html/HTMLFrameElementBase.cpp (right):

https://codereview.chromium.org/189573002/diff/30002/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameElementBase.cpp:95: if (!contentFrame() ||
scriptURL.isEmpty() || !contentFrame()->isLocalFrame())
On 2014/03/17 18:17:44, dcheng wrote:
> Do we need a FIXME here? Isn't location settable on a remote frame
>
(http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.html#sec...

This check only prevents JavaScript URLs from running, which I believe is
correct behavior on a cross-origin frame. AFAICT we are okay to just ignore it.

https://codereview.chromium.org/189573002/diff/30002/Source/core/html/HTMLFra...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/189573002/diff/30002/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameOwnerElement.cpp:86:
toLocalFrame(frame)->loader().frameDetached();
On 2014/03/17 18:17:44, dcheng wrote:
> Do we need a FIXME here? How are we planning on triggering the detach of
remote
> frames?

I don't know exactly what attachment and detachment will look like on
RemoteFrames; figuring out the details is one of our next tasks. I don't think
we'll need a FIXME though because we can plumb the right calls when we no what
methods RemoteFrame will have for the purpose.

https://codereview.chromium.org/189573002/diff/30002/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameOwnerElement.cpp:128: if (contentFrame() &&
contentFrame()->isLocalFrame()) {
On 2014/03/17 18:17:44, dcheng wrote:
> Similar comment to my earlier comment in HTMLFrameElementBase::openURL.

I've added a FIXME here. I don't know yet if RemoteFrames will have
NavigationSchedulers, or if they can just do straight navigations.

https://codereview.chromium.org/189573002/diff/30002/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/189573002/diff/30002/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:1753: // Cross-process drag and drop is not
yet supported.
On 2014/03/17 18:17:44, dcheng wrote:
> Tag this with FIXME? Maybe we should track these instances in a bug somewhere
as
> well to make them easier to chase down later...

I'm not worried about that in cases where we will necessarily revisit this code
when we add later features. I prefer to use FIXMEs when there are things that
could be missed later.

https://codereview.chromium.org/189573002/diff/30002/Source/core/page/FocusCo...
File Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/189573002/diff/30002/Source/core/page/FocusCo...
Source/core/page/FocusController.cpp:242: RefPtr<LocalFrame> oldFrame =
(m_focusedFrame && m_focusedFrame->isLocalFrame()) ?
toLocalFrame(m_focusedFrame.get()) : 0;
On 2014/03/17 18:17:44, dcheng wrote:
> This logic is incomplete atm and depends on proper inter-frame focus handling
> being moved out of Blink as well right?

Yes this entire class is going to get carved up into pieces.

https://codereview.chromium.org/189573002/diff/30002/Source/core/page/PageSer...
File Source/core/page/PageSerializer.cpp (right):

https://codereview.chromium.org/189573002/diff/30002/Source/core/page/PageSer...
Source/core/page/PageSerializer.cpp:158: // RemoteFrames not currently supported
here.
On 2014/03/17 18:17:44, dcheng wrote:
> But eventually, right? Maybe tag with a FIXME?
> 
> Another question is how this interacts with WebPageSerializer. It looks like
> that doesn't even add a frame to the list of frames to visit (and presumably
> serialize). Is this code used by a different codepath in Source/core as well?
> I'm assuming so (it looks like it might be called by StyledMarkupAccumulator?)
> but it'd be good to have confirmation.

I've added a FIXME, but I don't know this area of the code well enough to
comment on your question.

https://codereview.chromium.org/189573002/diff/30002/Source/web/ContextMenuCl...
File Source/web/ContextMenuClientImpl.cpp (right):

https://codereview.chromium.org/189573002/diff/30002/Source/web/ContextMenuCl...
Source/web/ContextMenuClientImpl.cpp:198: if
(toLocalFrame(m_webView->focusedWebCoreFrame())->editor().canUndo())
On 2014/03/17 18:17:44, dcheng wrote:
> Hm... what guarantees that these are always focused? Is it because the only
ways
> to invoke the context menu require that the frame have focus (right-clicking,
> context menu keyboard button)? If so, should we add an assert?
> 
> Or is this currently true because of the way WebViewImpl filters out events
for
> RemoteFrame?

The downcast macro already contains an ASSERT. I think context menu creation
should already be in the correct process.

https://codereview.chromium.org/189573002/diff/30002/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/189573002/diff/30002/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1290: LocalFrame* frame =
toLocalFrame(focusedWebCoreFrame());
On 2014/03/17 18:17:44, dcheng wrote:
> Hmm... is focusedWebCoreFrame() eventually going to return LocalFrame once
> 'process focus tracking' is in Chrome?

I don't know. There might be reasons to return focused RemoteFrames sometimes,
but this code should not be reachable when a RemoteFrame has focus.

https://codereview.chromium.org/189573002/diff/30002/Source/web/WebViewImpl.h
File Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/189573002/diff/30002/Source/web/WebViewImpl.h...
Source/web/WebViewImpl.h:72: class LocalFrame;
On 2014/03/17 18:17:44, dcheng wrote:
> Why the forward declare for Color and LocalFrame?

Removed.

[dcheng, 2014-03-19 20:50:02]

LGTM. Let's revert the Editor.cpp check for isLocalFrame() though =)

https://codereview.chromium.org/189573002/diff/30002/Source/bindings/v8/Bindi...
File Source/bindings/v8/BindingSecurity.h (right):

https://codereview.chromium.org/189573002/diff/30002/Source/bindings/v8/Bindi...
Source/bindings/v8/BindingSecurity.h:34: #include "core/frame/LocalFrame.h"
On 2014/03/18 20:01:19, kenrb wrote:
> On 2014/03/17 18:17:44, dcheng wrote:
> > Hm... can't this have remained a forward include?
> 
> No because a lot of clients call shouldAllowAccessToFrame() using LocalFrames,
> and they don't include LocalFrame.h themselves, so if we forward declare then
> they wouldn't be able to do the implicit conversion.

Ugh. I think this is OK temporarily but we should clean this up in a follow
patch, so they correctly #include LocalFrame.h themselves.

https://codereview.chromium.org/189573002/diff/30002/Source/core/editing/Edit...
File Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/189573002/diff/30002/Source/core/editing/Edit...
Source/core/editing/Editor.cpp:810:
toLocalFrame(page->focusController().focusedOrMainFrame())->selection().revealSelection(ScrollAlignment::alignCenterIfNeeded);
On 2014/03/18 20:01:19, kenrb wrote:
> On 2014/03/17 18:17:44, dcheng wrote:
> > Is this always guaranteed to be a local frame? That doesn't seem like it
would
> > always be the case.
> 
> I was thinking a selected frame would have to be local in order to be editing
> it, but I'm not sure about that so I've added the check for the sake of
caution.

Thinking about this more, I'm not 100% sure what the behavior should be here,
since I'm not familiar with this part of the editing code. I don't think the
editing action itself can take place in a remote frame. And it looks like the
intent of this code is to scroll the changed selection into view... so it's not
clear to me why it falls back to focusedOrMainFrame() to do this (since
presumably the selection you're editing should be local to this frame...)

Maybe let's leave off the if() and see what explodes =)

[kenrb, 2014-03-20 15:56:57]

Eric: Daniel has reviewed but I also need a Blink OWNER. PTAL?

https://codereview.chromium.org/189573002/diff/30002/Source/core/editing/Edit...
File Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/189573002/diff/30002/Source/core/editing/Edit...
Source/core/editing/Editor.cpp:810:
toLocalFrame(page->focusController().focusedOrMainFrame())->selection().revealSelection(ScrollAlignment::alignCenterIfNeeded);
On 2014/03/19 20:50:03, dcheng wrote:
> On 2014/03/18 20:01:19, kenrb wrote:
> > On 2014/03/17 18:17:44, dcheng wrote:
> > > Is this always guaranteed to be a local frame? That doesn't seem like it
> would
> > > always be the case.
> > 
> > I was thinking a selected frame would have to be local in order to be
editing
> > it, but I'm not sure about that so I've added the check for the sake of
> caution.
> 
> Thinking about this more, I'm not 100% sure what the behavior should be here,
> since I'm not familiar with this part of the editing code. I don't think the
> editing action itself can take place in a remote frame. And it looks like the
> intent of this code is to scroll the changed selection into view... so it's
not
> clear to me why it falls back to focusedOrMainFrame() to do this (since
> presumably the selection you're editing should be local to this frame...)
> 
> Maybe let's leave off the if() and see what explodes =)

Ok. If this is wrong, we should see ASSERTs after we start testing editing with
RemoteFrames present.

[eseidel, 2014-03-21 19:01:35]

My primary concern is that we take advantage of the compiler to help us audit
callers who need to distinguish between remote and local frames.  Perhaps we
should use a toLocalFrameWrong() method instead or something to make these
FIXMEs easier to find.  It will be difficult to find incorrect toLocalFrame()
calls after we make them w/o having them crash in debug mode on obscure tests.

https://codereview.chromium.org/189573002/diff/80001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/189573002/diff/80001/Source/core/dom/Document...
Source/core/dom/Document.cpp:999: if (frame() &&
frame()->tree().isDescendantOf(toLocalFrame(frameOwnerElement->contentFrame())))
{
Why not just add the check now?  What would this code do when it's a
RemoteFrame?

https://codereview.chromium.org/189573002/diff/80001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5454: if (LocalFrame* focusedFrame =
toLocalFrame(page->focusController().focusedFrame())) {
Does this make sense?  Will the Document be able to talk to the FocusController
like this?

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

https://codereview.chromium.org/189573002/diff/80001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:83: if (this->ownerElement()) {
Why this->  Do we have an ownerElement local?

[eseidel, 2014-03-21 19:07:11]

I'm very supportive of baby-steps.  I just want to make sure we're taking steps
in directions which don't harm us later.  loosly converting between Frame and
LocalFrame may be difficult to undo.

[kenrb, 2014-03-21 19:59:30]

Thanks Eric, that's a good idea. I would rather use a name like
toLocalFrameTransitional() and remove the alias when we are ready, then fix them
all up.

https://codereview.chromium.org/189573002/diff/80001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/189573002/diff/80001/Source/core/dom/Document...
Source/core/dom/Document.cpp:999: if (frame() &&
frame()->tree().isDescendantOf(toLocalFrame(frameOwnerElement->contentFrame())))
{
On 2014/03/21 19:01:35, eseidel wrote:
> Why not just add the check now?  What would this code do when it's a
> RemoteFrame?

It will pass a Frame to the FrameTree rather than a LocalFrame, so no conversion
necessary.

https://codereview.chromium.org/189573002/diff/80001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5454: if (LocalFrame* focusedFrame =
toLocalFrame(page->focusController().focusedFrame())) {
On 2014/03/21 19:01:35, eseidel wrote:
> Does this make sense?  Will the Document be able to talk to the
FocusController
> like this?

I think so. We haven't started writing any code for the FocusController
refactoring (afaik), so we might need to look at this again later, but right now
I don't see why interactions like this would change.

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

https://codereview.chromium.org/189573002/diff/80001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:83: if (this->ownerElement()) {
On 2014/03/21 19:01:35, eseidel wrote:
> Why this->  Do we have an ownerElement local?

I asked the exact same question on dcheng's patch where he removed FrameInit...
the answer is that the method name currently conflicts with an argument name in
this function.

[kenrb, 2014-03-24 15:57:29]

Eric can you have another look at this? I added the downcast alias, but for now
there are only a couple of places where it is used (there might be more later).
Most of the FIXMEs relate to features that will need significant changes to work
properly with RemoteFrames (e.g. drag and drop), and we should remove the
temporary downcast long before then.

[eseidel, 2014-03-24 16:12:48]

Made it to FocusController.cpp, will look again in 20 mins or so.

https://codereview.chromium.org/189573002/diff/100001/Source/bindings/v8/Bind...
File Source/bindings/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/189573002/diff/100001/Source/bindings/v8/Bind...
Source/bindings/v8/BindingSecurity.cpp:85: if (!target->isLocalFrame())
This makes much more sense, thank you.

https://codereview.chromium.org/189573002/diff/100001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/189573002/diff/100001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:999: if (frame() &&
frame()->tree().isDescendantOf(toLocalFrameTemporary(frameOwnerElement->contentFrame())))
{
I'm much more comfortable having a temporary method like this, thank you.

https://codereview.chromium.org/189573002/diff/100001/Source/core/editing/Edi...
File Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/189573002/diff/100001/Source/core/editing/Edi...
Source/core/editing/Editor.cpp:810:
toLocalFrame(page->focusController().focusedOrMainFrame())->selection().revealSelection(ScrollAlignment::alignCenterIfNeeded);
This is always a LocalFrame?

https://codereview.chromium.org/189573002/diff/100001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/189573002/diff/100001/Source/core/frame/Local...
Source/core/frame/LocalFrame.h:242: #define toLocalFrameTemporary toLocalFrame
I would have just defined the function:

LocalFrame* toLocalFrameTemporary(Frame*)
{
   return toLocalFrame(Frame*);
}

[kenrb, 2014-03-24 16:27:40]

https://codereview.chromium.org/189573002/diff/100001/Source/core/editing/Edi...
File Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/189573002/diff/100001/Source/core/editing/Edi...
Source/core/editing/Editor.cpp:810:
toLocalFrame(page->focusController().focusedOrMainFrame())->selection().revealSelection(ScrollAlignment::alignCenterIfNeeded);
On 2014/03/24 16:12:48, eseidel wrote:
> This is always a LocalFrame?

Uncertain, one of my earlier patchsets had a check but discussing it with
Daniel, we thought it probably shouldn't ever be a RemoteFrame and he suggested
we should look for an ASSERT later to see if we are wrong.

https://codereview.chromium.org/189573002/diff/100001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/189573002/diff/100001/Source/core/frame/Local...
Source/core/frame/LocalFrame.h:242: #define toLocalFrameTemporary toLocalFrame
On 2014/03/24 16:12:48, eseidel wrote:
> I would have just defined the function:
> 
> LocalFrame* toLocalFrameTemporary(Frame*)
> {
>    return toLocalFrame(Frame*);
> }

The DEFINE_TYPE_CASTS macro ends up creating 4 functions: (const | non-const) x
(pointer argument | reference argument).

A #define seems a lot simpler.

[kenrb, 2014-03-25 18:52:28]

Eric: ping?

[eseidel, 2014-03-27 19:56:41]

lgtm

Sorry, my delays are not helping you be successful.

My primary concern is reducing the long-term technical debt on core/.  Obviously
we're going to have intermediate states, but we need those to be short.  We need
to figure out what the local/remote split looks like and get there as soon as
possible to reduce confusion.

I suspect that the *vast* majority of core/ only cares about LocalFrame*.

lgtm.  I wish you luck.

https://codereview.chromium.org/189573002/diff/100001/Source/core/page/FocusC...
File Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/189573002/diff/100001/Source/core/page/FocusC...
Source/core/page/FocusController.cpp:242: RefPtr<LocalFrame> oldFrame =
(m_focusedFrame && m_focusedFrame->isLocalFrame()) ?
toLocalFrame(m_focusedFrame.get()) : 0;
Sometimes we really want a version of these "to" functions which returns null if
the is-check is false.

[commit-bot: I haz the power, 2014-03-27 19:56:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kenrb@chromium.org/189573002/130001

[commit-bot: I haz the power, 2014-03-27 20:34:43]

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

[commit-bot: I haz the power, 2014-03-27 20:34:43]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[kenrb, 2014-03-27 21:07:53]

The CQ bit was checked by kenrb@chromium.org

[commit-bot: I haz the power, 2014-03-27 21:07:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kenrb@chromium.org/189573002/130001

[commit-bot: I haz the power, 2014-03-27 22:32:48]

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

[commit-bot: I haz the power, 2014-03-27 22:32:48]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[kenrb, 2014-03-28 16:02:23]

The CQ bit was checked by kenrb@chromium.org

[commit-bot: I haz the power, 2014-03-28 16:02:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kenrb@chromium.org/189573002/150001

[commit-bot: I haz the power, 2014-03-28 16:34:26]

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

[commit-bot: I haz the power, 2014-03-28 16:34:27]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[kenrb, 2014-03-28 17:40:45]

The CQ bit was checked by kenrb@chromium.org

[commit-bot: I haz the power, 2014-03-28 17:40:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kenrb@chromium.org/189573002/170001

[commit-bot: I haz the power, 2014-03-28 18:58:01]

Change committed as 170347