Extract a DOMWindow interface from LocalDOMWindow and use it in the idl.

Extract a DOMWindow interface from LocalDOMWindow and use it in the idl.

This patch switches the bindings code from using LocalDOMWindow to using
the DOMWindow interface. This will allow OOPIF to add a RemoteDOMWindow
class that remote frames can expose to web content.

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

[dcheng, 2014-10-31 01:03:56]

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

[dcheng, 2014-10-31 03:20:42]

I've annotated the review with the reasoning for particular design choices I
made.

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/custom/V8EventTargetCustom.cpp (right):

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8EventTargetCustom.cpp:45: if
(impl->interfaceName() == EventTargetNames::LocalDOMWindow)
I'm not really sure what this is used for. It seems like this should probably be
renamed to EventTargetNames::DOMWindow.

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp (right):

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8HTMLDocumentCustom.cpp:41: #include
"core/frame/LocalDOMWindow.h"
This include (and others) are because we need to convert a LocalDOMWindow to a
DOMWindow, so we need the full definition of LocalDOMWindow.

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp (right):

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp:121: LocalFrame*
frame = toLocalDOMWindow(V8Window::toImpl(holder))->frame();
For things like this, I assume that the browser will always route these requests
to the renderer hosting the frame. However, much of this browser-side routing
does not exist today, so I expect that we'll hit a bunch of asserts in
--site-per-process mode.

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/custom/V8WindowCustom.cpp (right):

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8WindowCustom.cpp:81: LocalDOMWindow* impl =
toLocalDOMWindow(V8Window::toImpl(info.Holder()));
Things like this are not available cross-origin (and should be enforced by the
bindings security checks), so we should be able to assume a LocalDOMWindow here.

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8WindowCustom.cpp:246: // FIXME: This is likely
to be problematic for the use counters below.
I don't have a good solution for the use counters yet. It looks like it uses the
document to get to the FrameHost, so theoretically, we could use
DOMWindow::frame() for that instead. But I think this can wait for a separate
CL.

https://codereview.chromium.org/683013002/diff/60001/Source/core/events/Mouse...
File Source/core/events/MouseRelatedEvent.cpp (right):

https://codereview.chromium.org/683013002/diff/60001/Source/core/events/Mouse...
Source/core/events/MouseRelatedEvent.cpp:67: LocalFrame* frame = view() &&
view()->isLocalDOMWindow() ? toLocalDOMWindow(view())->frame() : 0;
It was quite a surprise to me that you can pass arbitrary Window objects to
initMouseEvent, etc. Right now, this means web pages could potentially inspect
the page zoom factor cross origin. With this change (and actual
RemoteDOMWindows), this would no longer be possible. I don't think this will be
a web compat problem, but I'm not 100% sure.

https://codereview.chromium.org/683013002/diff/60001/Source/core/frame/DOMWin...
File Source/core/frame/DOMWindow.h (right):

https://codereview.chromium.org/683013002/diff/60001/Source/core/frame/DOMWin...
Source/core/frame/DOMWindow.h:175: virtual void
postMessage(PassRefPtr<SerializedScriptValue> message, const MessagePortArray*,
const String& targetOrigin, LocalDOMWindow* source, ExceptionState&) = 0;
I can split this into a separate CL if it will make this patch easier to review.

https://codereview.chromium.org/683013002/diff/60001/Source/core/frame/DOMWin...
Source/core/frame/DOMWindow.h:193: DEFINE_ATTRIBUTE_EVENT_LISTENER(touchcancel);
I feel like we could do a little better here--RemoteDOMWindow shouldn't need
these event listener attributes, since they won't be gettable/settable anyway.
This is the easiest way for now though.

https://codereview.chromium.org/683013002/diff/60001/Source/core/frame/DOMWin...
File Source/core/frame/DOMWindowProperty.cpp (right):

https://codereview.chromium.org/683013002/diff/60001/Source/core/frame/DOMWin...
Source/core/frame/DOMWindowProperty.cpp:46: // remote DOM windows.
The only DOMWindowProperty this really affects is Location. I'd prefer not to
have to make Location an interface class and split into
LocalLocation/RemoteLocation. The WebFrame/Frame/DOMWindow split is complicated
enough as it is.

https://codereview.chromium.org/683013002/diff/60001/Source/core/frame/LocalF...
File Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/683013002/diff/60001/Source/core/frame/LocalF...
Source/core/frame/LocalFrame.h:83: virtual DOMWindow* domWindow() const
override;
I wanted to make keep this as a covariant return, but this leads to a circular
dependency between LocalFrame and LocalDOMWindow, since covariant return types
are only legal if a type is a subclass of the base class method's return type.
Since you can't forward declare an inheritance relationship, that means either
LocalFrame or LocalDOMWindow can't use a covariant return. Since there are a lot
less dependencies on LocalFrame::domWindow() returning a LocalDOMWindow, I chose
to change this one instead.

https://codereview.chromium.org/683013002/diff/60001/Source/core/frame/Locati...
File Source/core/frame/Location.cpp (right):

https://codereview.chromium.org/683013002/diff/60001/Source/core/frame/Locati...
Source/core/frame/Location.cpp:254: // FIXME: This is probably wrong.
I'm going to remove this FIXME in my next patchset--I looked at this more
closely, and it seems fine.

https://codereview.chromium.org/683013002/diff/60001/Source/core/page/DOMWind...
File Source/core/page/DOMWindowPagePopup.h (right):

https://codereview.chromium.org/683013002/diff/60001/Source/core/page/DOMWind...
Source/core/page/DOMWindowPagePopup.h:46: static PagePopupController*
pagePopupController(DOMWindow&);
These changes (and other similar changes in Source/modules) are due to the way
the generated bindings call these methods. They extract the C++ object from the
V8 wrapper object, but since we changed ImplementedAs to DOMWindow, we end up
with something like this:
  DOMWindow& window = *V8Window::toImpl(...);
  ... DOMWindowPagePopup::pagePopupController(window) ...
but this is a compiler error, since we need to downcast from to a
LocalDOMWindow.

While it would be nice for these methods to take a LocalDOMWindow& (since they
will logically never work on a remote DOM window), I don't see any way to do
this without adding special logic in the bindings just to handle this special
DOMWindow/LocalDOMWindow case. That seems very hacky and fragile.

[haraken, 2014-10-31 06:07:04]

not LGTM unfortunately. This CL is making too many "uncertain" changes in one
go. Let's land the changes more incrementally.

Also I'm getting confused when we should use DOMWindow and when we should use
LocalDOMWindow. For example, it's very confusing that LocalFrame::domWindow()
and LocalFrame::localDOMWindow() return the same object. Can we remove
LocalFrame::domWindow()?

Either way, I think we need to have a more consistent rule to distinguish
DOMWindow from LocalDOMWindow. Developers won't be able to understand which
DOMWindow they should use :)

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptState.cpp (right):

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptState.cpp:86: LocalDOMWindow*
ScriptState::domWindow() const

Rename this to localDOMWindow() in a follow-up CL.

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8Binding.cpp (right):

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8Binding.cpp:717: LocalDOMWindow*
enteredDOMWindow(v8::Isolate* isolate)

Rename this to enteredLocalDOMWindow in a follow-up CL.

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8Binding.cpp:730: LocalDOMWindow*
currentDOMWindow(v8::Isolate* isolate)

currentLocalDOMWindow

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8Binding.cpp:735: LocalDOMWindow*
callingDOMWindow(v8::Isolate* isolate)

callingLocalDOMWindow

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/custom/V8EventTargetCustom.cpp (right):

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8EventTargetCustom.cpp:45: if
(impl->interfaceName() == EventTargetNames::LocalDOMWindow)
On 2014/10/31 03:20:40, dcheng wrote:
> I'm not really sure what this is used for. It seems like this should probably
be
> renamed to EventTargetNames::DOMWindow.

Yeah, let's fix this in a follow-up CL.

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp (right):

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp:121: LocalFrame*
frame = toLocalDOMWindow(V8Window::toImpl(holder))->frame();
On 2014/10/31 03:20:40, dcheng wrote:
> For things like this, I assume that the browser will always route these
requests
> to the renderer hosting the frame. However, much of this browser-side routing
> does not exist today, so I expect that we'll hit a bunch of asserts in
> --site-per-process mode.

Don't we need to have:

  LocalDOMWindow* window = toLocalDOMWindow(V8Window::toImpl(holder));
  if (!window)
    return false;
  LocalFrame* frame = window->frame();

?

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/custom/V8InspectorFrontendHostCustom.cpp (right):

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8InspectorFrontendHostCustom.cpp:162: document =
window ? toLocalDOMWindow(window)->document() : nullptr;

Shouldn't this be:

  document = window->isLocalDOMWindow() ? toLocalDOMWindow(window)->document() :
nullptr;

?

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/custom/V8WindowCustom.cpp (right):

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8WindowCustom.cpp:246: // FIXME: This is likely
to be problematic for the use counters below.
On 2014/10/31 03:20:41, dcheng wrote:
> I don't have a good solution for the use counters yet. It looks like it uses
the
> document to get to the FrameHost, so theoretically, we could use
> DOMWindow::frame() for that instead. But I think this can wait for a separate
> CL.

The same problem should be happening in auto-generated code, since the
auto-generated code has UseCounters as well. How are we handling the
auto-generated code?

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

https://codereview.chromium.org/683013002/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5208: PassRefPtrWillBeRawPtr<Touch>
Document::createTouch(DOMWindow* window, EventTarget* target, int identifier,
double pageX, double pageY, double screenX, double screenY, double radiusX,
double radiusY, float rotationAngle, float force) const

Why doesn't this method take a LocalDOMWindow* parameter while we assume that
|window| is a LocalDOMWindow?

https://codereview.chromium.org/683013002/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5233: LocalFrame* frame = window ?
toLocalDOMWindow(window)->frame() : this->frame();

Here we're assuming that |window| is a LocalDOMWindow.

https://codereview.chromium.org/683013002/diff/60001/Source/core/events/UIEve...
File Source/core/events/UIEvent.h (right):

https://codereview.chromium.org/683013002/diff/60001/Source/core/events/UIEve...
Source/core/events/UIEvent.h:33: typedef DOMWindow AbstractView;

Actually I want to remove this typedef.

Now we have DOMWindow, LocalDOMWindow, RemoteDOMWindow and AbstractView, which
is confusing. Let's reduce the number from 4 to 3, at least :)

https://codereview.chromium.org/683013002/diff/60001/Source/core/frame/DOMWin...
File Source/core/frame/DOMWindowProperty.cpp (right):

https://codereview.chromium.org/683013002/diff/60001/Source/core/frame/DOMWin...
Source/core/frame/DOMWindowProperty.cpp:50: m_associatedDOMWindow =
m_frame->localDOMWindow();

Rename m_associatedDOMWindow to m_associatedLocalDOMWindow.

https://codereview.chromium.org/683013002/diff/60001/Source/core/frame/LocalF...
File Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/683013002/diff/60001/Source/core/frame/LocalF...
Source/core/frame/LocalFrame.h:224: return m_domWindow.get();

m_domWindow => m_localDOMWindow

[dcheng, 2014-10-31 06:31:01]

Replying to your more high-level comments. In general, the philosophy should be
to assume and use LocalDOMWindow where possible. That is why LocalFrame/Document
provide LocalDOMWindow accessors rather than just a DOMWindow accessor. As a
practical example of this, in the bindings code, we rely on the fact that the
security checks will prevent many methods from ever being called on
non-LocalDOMWindows. So for anything that doesn't need to work cross-origin,
it's safe to assume LocalDOMWindow. Certain attributes that do work cross-origin
(postMessage, window.location) need special handling.

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp (right):

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp:121: LocalFrame*
frame = toLocalDOMWindow(V8Window::toImpl(holder))->frame();
On 2014/10/31 06:07:04, haraken wrote:
> On 2014/10/31 03:20:40, dcheng wrote:
> > For things like this, I assume that the browser will always route these
> requests
> > to the renderer hosting the frame. However, much of this browser-side
routing
> > does not exist today, so I expect that we'll hit a bunch of asserts in
> > --site-per-process mode.
> 
> Don't we need to have:
> 
>   LocalDOMWindow* window = toLocalDOMWindow(V8Window::toImpl(holder));
>   if (!window)
>     return false;
>   LocalFrame* frame = window->frame();
> 
> ?

The browser should be routing any calls of this nature correctly so we never end
up in the "wrong renderer": in other words, this should only be called in a
renderer where the frame is a LocalFrame. toLocalDOMWindow() will actually
RELEASE_ASSERT if the DOMWindow is of the wrong type. It's almost guaranteed
that this will assert right now. However, it only affects --site-per-process so
I'd prefer to make this as aggressive as possible so we can find problematic
areas. In the final state, there should be no checks for
isLocalDOMWindow()--since the browser will always route it to the renderer
hosting the frame, the DOMWindow will always be local.

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/custom/V8InspectorFrontendHostCustom.cpp (right):

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8InspectorFrontendHostCustom.cpp:162: document =
window ? toLocalDOMWindow(window)->document() : nullptr;
On 2014/10/31 06:07:04, haraken wrote:
> 
> Shouldn't this be:
> 
>   document = window->isLocalDOMWindow() ? toLocalDOMWindow(window)->document()
:
> nullptr;
> 
> ?

See my previous comment. This code should only ever run in the "right" renderer,
so we should never have to handle a non-LocalDOMWindow here. That is probably
not the case today, but that's why I prefer an explicit crash in
--site-per-process mode: this helps us keep track of problematic areas.

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/custom/V8WindowCustom.cpp (right):

https://codereview.chromium.org/683013002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8WindowCustom.cpp:246: // FIXME: This is likely
to be problematic for the use counters below.
On 2014/10/31 06:07:04, haraken wrote:
> On 2014/10/31 03:20:41, dcheng wrote:
> > I don't have a good solution for the use counters yet. It looks like it uses
> the
> > document to get to the FrameHost, so theoretically, we could use
> > DOMWindow::frame() for that instead. But I think this can wait for a
separate
> > CL.
> 
> The same problem should be happening in auto-generated code, since the
> auto-generated code has UseCounters as well. How are we handling the
> auto-generated code?

Let me look into this. Hopefully we can just update it to get to FrameHost via
DOMWindow instead of via Document.

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

https://codereview.chromium.org/683013002/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5208: PassRefPtrWillBeRawPtr<Touch>
Document::createTouch(DOMWindow* window, EventTarget* target, int identifier,
double pageX, double pageY, double screenX, double screenY, double radiusX,
double radiusY, float rotationAngle, float force) const
On 2014/10/31 06:07:04, haraken wrote:
> 
> Why doesn't this method take a LocalDOMWindow* parameter while we assume that
> |window| is a LocalDOMWindow?

This is tricky. This is required because of several things:
1. This method is defined in the idl as createTouch(Window window, ...)
2. Window's ImplementedAs changed to DOMWindow.
3. It's not possible to downcast from DOMWindow to LocalDOMWindow without an
explicit static_cast.

However, as I mention below, this change is actually incorrect--we shouldn't be
assuming LocalDOMWindow here, because this is web exposed.

https://codereview.chromium.org/683013002/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5233: LocalFrame* frame = window ?
toLocalDOMWindow(window)->frame() : this->frame();
On 2014/10/31 06:07:04, haraken wrote:
> 
> Here we're assuming that |window| is a LocalDOMWindow.

This change is actually incorrect--I forgot to update this one to pass DOMWindow
through to Touch() and have that check isLocalDOMWindow(). I'll be reverting
this in the next patchset.

[haraken, 2014-10-31 06:42:10]

> Replying to your more high-level comments. In general, the philosophy should
be
> to assume and use LocalDOMWindow where possible. That is why
LocalFrame/Document
> provide LocalDOMWindow accessors rather than just a DOMWindow accessor. As a
> practical example of this, in the bindings code, we rely on the fact that the
> security checks will prevent many methods from ever being called on
> non-LocalDOMWindows. So for anything that doesn't need to work cross-origin,
> it's safe to assume LocalDOMWindow. Certain attributes that do work
cross-origin
> (postMessage, window.location) need special handling.

OK, makes sense.

That being said, the distinction of LocalDOMWindow/DOMWindow and
LocalFrame/Frame are getting very confusing. I think we need to rethink about a
programming rule before landing more changes. The rule must be easy-to-follow
and less error-prone for normal Blink developers.

If you are planning to use LocalDOMWindow and RemoteDOMWindow as much as
possible and use DOMWindow in only a limited places, how about renaming:

- LocalDOMWindow => DOMWindow
- DOMWindow => CrossOriginDOMWindow
- LocalFrame => Frame
- Frame => CrossOriginFrame

?

"CrossOriginDOMWindow" makes it clear that you're intentionally doing something
that allows cross-origin access. Also developers can keep using "DOMWindow" if
they don't know what they are doing.

BTW, how often will we need to use CrossOriginDOMWindow and CrossOriginFrame
once we finish cleaning up the code base? If the places are limited (e.g.,
postMessage), I'm happy, but otherwise I'm not really happy :)

[dcheng, 2014-10-31 08:08:18]

On 2014/10/31 at 06:42:10, haraken wrote:
> > Replying to your more high-level comments. In general, the philosophy should
be
> > to assume and use LocalDOMWindow where possible. That is why
LocalFrame/Document
> > provide LocalDOMWindow accessors rather than just a DOMWindow accessor. As a
> > practical example of this, in the bindings code, we rely on the fact that
the
> > security checks will prevent many methods from ever being called on
> > non-LocalDOMWindows. So for anything that doesn't need to work cross-origin,
> > it's safe to assume LocalDOMWindow. Certain attributes that do work
cross-origin
> > (postMessage, window.location) need special handling.
> 
> OK, makes sense.
> 
> That being said, the distinction of LocalDOMWindow/DOMWindow and
LocalFrame/Frame are getting very confusing. I think we need to rethink about a
programming rule before landing more changes. The rule must be easy-to-follow
and less error-prone for normal Blink developers.

Unfortunately, it's a bit more difficult to come up with a one-size fits all
rule at the moment. There are many places in the code that have not been
rewritten to take out-of-process frames into account. However, this is where we
want to eventually end up:
- Most things are not accessible cross-origin. These should use LocalDOMWindow
whenever possible (except where it might be in conflict with auto-generated
bindings code). It might be possible to make the bindings code smarter to handle
this as well, if we can somehow extend it to understand that performing security
checks should imply DOMWindow is actually a LocalDOMWindow.
- If you're writing code that checks window->isLocalDOMWindow() or
frame->isLocalFrame(), it is probably wrong. There are only a few places that
will ever need to check this (navigations, postMessage, maybe rendering), and
most people are not working on those places.

> 
> If you are planning to use LocalDOMWindow and RemoteDOMWindow as much as
possible and use DOMWindow in only a limited places, how about renaming:
> 
> - LocalDOMWindow => DOMWindow
> - DOMWindow => CrossOriginDOMWindow
> - LocalFrame => Frame
> - Frame => CrossOriginFrame
> 
> ?
> 

Remote implies cross-origin, but local does not imply same-origin. Because many
pages have a lot of iframes, we expect it to be fairly common that we will have
local frames in the same frame tree which are cross-origin to each other.
Because of that, we chose Local/Remote instead of <no prefix>/CrossOrigin.

> "CrossOriginDOMWindow" makes it clear that you're intentionally doing
something that allows cross-origin access. Also developers can keep using
"DOMWindow" if they don't know what they are doing.
> 
> BTW, how often will we need to use CrossOriginDOMWindow and CrossOriginFrame
once we finish cleaning up the code base? If the places are limited (e.g.,
postMessage), I'm happy, but otherwise I'm not really happy :)

Use of these classes should be fairly limited. Code that has to deal with them
should be pretty localized.

[haraken, 2014-11-08 09:17:26]

Here is the second round of comments!

BTW, not related to your CL, what prevents us from renaming DOMWindow to Window?

https://codereview.chromium.org/683013002/diff/140001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/683013002/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Initializer.cpp:306: return;

If the target window is a RemoteDOMWindow, this code returns silently. We might
want to throw a SecurityError, just like the line 311 does.

https://codereview.chromium.org/683013002/diff/140001/Source/bindings/core/v8...
File Source/bindings/core/v8/custom/V8WindowCustom.cpp (right):

https://codereview.chromium.org/683013002/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8WindowCustom.cpp:246: // FIXME: This is likely
to be problematic for the use counters below.

Just help me understand: Why is it problematic?

I mentioned earlier that the same problem (although I don't understand the
problem) should be happening in auto-generated code, but it shouldn't.
Auto-generated code doesn't use window to record use counters.

https://codereview.chromium.org/683013002/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8WindowCustom.cpp:466: LocalDOMWindow*
targetWindow = toLocalDOMWindow(V8Window::toImpl(window));

I think V8Window::toImpl(window) can be a RemoteDOMWindow. Can we return false
when isLocalDOMWindow() fails?

https://codereview.chromium.org/683013002/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8WindowCustom.cpp:510: LocalDOMWindow*
targetWindow = toLocalDOMWindow(V8Window::toImpl(window));

Ditto.

https://codereview.chromium.org/683013002/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8WindowCustom.cpp:541: // FIXME: How is this
used? Does this need to be able to handle remote DOM windows?

I don't fully understand this comment.

The !window->isLocalDOMWindow() check below is very important in order not to
expose a wrapper of a RemoteDOMWindow to user's script, isn't it?

Hmm, I got confused. In what case is it possible that toV8 is called with a
RemoteDOMWindow? If it happens, I guess it is just wrong. Can we probably assert
that window->isLocalDOMWindow is true?

https://codereview.chromium.org/683013002/diff/140001/Source/core/events/Touc...
File Source/core/events/TouchEvent.cpp (right):

https://codereview.chromium.org/683013002/diff/140001/Source/core/events/Touc...
Source/core/events/TouchEvent.cpp:99: if (!cancelable() && view() &&
view()->frame() && view()->frame()->isLocalFrame()) {

Nit: Can we check view()->isLocalWindow() instead of
view()->frame()->isLocalFrame() just for consistency?

https://codereview.chromium.org/683013002/diff/140001/Source/core/frame/DOMWi...
File Source/core/frame/DOMWindow.h (right):

https://codereview.chromium.org/683013002/diff/140001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindow.h:175: virtual void
postMessage(PassRefPtr<SerializedScriptValue> message, const MessagePortArray*,
const String& targetOrigin, LocalDOMWindow* source, ExceptionState&) = 0;

Split these changes to a separate CL.

https://codereview.chromium.org/683013002/diff/140001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/683013002/diff/140001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:244: DOMWindow* LocalFrame::domWindow() const

Can we remove this method?

DOMWindow* window = localFrame()->localDOMWindow();

will just work. It looks confusing to have both domWindow() and
localDOMWindow().

https://codereview.chromium.org/683013002/diff/140001/Source/core/frame/Remot...
File Source/core/frame/RemoteFrame.h (right):

https://codereview.chromium.org/683013002/diff/140001/Source/core/frame/Remot...
Source/core/frame/RemoteFrame.h:25: virtual DOMWindow* domWindow() const
override { return 0; }

I'm confused with this. Why does RemoteDOMWindow return 0 for domWindow()? Given
that RemoteDOMWindow is DOMWindow, I think it should return a DOMWindow object.

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

https://codereview.chromium.org/683013002/diff/140001/Source/core/html/HTMLFr...
Source/core/html/HTMLFrameOwnerElement.h:45: DOMWindow* contentWindow() const;

How is it possible that contentWindow is a RemoteDOMWindow? I guess it is always
a LocalDOMWindow.

https://codereview.chromium.org/683013002/diff/140001/Source/core/page/DOMWin...
File Source/core/page/DOMWindowPagePopup.cpp (right):

https://codereview.chromium.org/683013002/diff/140001/Source/core/page/DOMWin...
Source/core/page/DOMWindowPagePopup.cpp:52: PagePopupController*
DOMWindowPagePopup::pagePopupController(DOMWindow& window)

What's the point of changing LocalDOMWindow to DOMWindow, while we assume that
the window is a LocalDOMWindow below?

https://codereview.chromium.org/683013002/diff/140001/Source/modules/crypto/D...
File Source/modules/crypto/DOMWindowCrypto.cpp (right):

https://codereview.chromium.org/683013002/diff/140001/Source/modules/crypto/D...
Source/modules/crypto/DOMWindowCrypto.cpp:61: Crypto*
DOMWindowCrypto::crypto(DOMWindow& window)

Ditto. What's the point of changing LocalDOMWindow to DOMWindow, whereas we
assume that the window is a LocalDOMWindow below?

https://codereview.chromium.org/683013002/diff/140001/Source/modules/filesyst...
File Source/modules/filesystem/DOMWindowFileSystem.cpp (right):

https://codereview.chromium.org/683013002/diff/140001/Source/modules/filesyst...
Source/modules/filesystem/DOMWindowFileSystem.cpp:51: void
DOMWindowFileSystem::webkitRequestFileSystem(DOMWindow& windowArg, int type,
long long size, FileSystemCallback* successCallback, ErrorCallback*
errorCallback)

Ditto.

https://codereview.chromium.org/683013002/diff/140001/Source/modules/filesyst...
Source/modules/filesystem/DOMWindowFileSystem.cpp:75: void
DOMWindowFileSystem::webkitResolveLocalFileSystemURL(DOMWindow& windowArg, const
String& url, EntryCallback* successCallback, ErrorCallback* errorCallback)

Ditto.

https://codereview.chromium.org/683013002/diff/140001/Source/modules/indexedd...
File Source/modules/indexeddb/DOMWindowIndexedDatabase.cpp (right):

https://codereview.chromium.org/683013002/diff/140001/Source/modules/indexedd...
Source/modules/indexeddb/DOMWindowIndexedDatabase.cpp:78: IDBFactory*
DOMWindowIndexedDatabase::indexedDB(DOMWindow& window)

Ditto.

https://codereview.chromium.org/683013002/diff/140001/Source/modules/quota/DO...
File Source/modules/quota/DOMWindowQuota.cpp (right):

https://codereview.chromium.org/683013002/diff/140001/Source/modules/quota/DO...
Source/modules/quota/DOMWindowQuota.cpp:65: DeprecatedStorageInfo*
DOMWindowQuota::webkitStorageInfo(DOMWindow& window)

Ditto.

https://codereview.chromium.org/683013002/diff/140001/Source/modules/speech/D...
File Source/modules/speech/DOMWindowSpeechSynthesis.cpp (right):

https://codereview.chromium.org/683013002/diff/140001/Source/modules/speech/D...
Source/modules/speech/DOMWindowSpeechSynthesis.cpp:64: SpeechSynthesis*
DOMWindowSpeechSynthesis::speechSynthesis(DOMWindow& window)

Ditto.

https://codereview.chromium.org/683013002/diff/140001/Source/modules/webdatab...
File Source/modules/webdatabase/DOMWindowWebDatabase.cpp (right):

https://codereview.chromium.org/683013002/diff/140001/Source/modules/webdatab...
Source/modules/webdatabase/DOMWindowWebDatabase.cpp:43: Database*
DOMWindowWebDatabase::openDatabase(DOMWindow& windowArg, const String& name,
const String& version, const String& displayName, unsigned long estimatedSize,
DatabaseCallback* creationCallback, ExceptionState& exceptionState)

Ditto.

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

https://codereview.chromium.org/683013002/diff/140001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:965: return
frame()->localDOMWindow()->pendingUnloadEventListeners();

I'd propose to rename frame() to localFrame() to reduce confusion.

Methods that return Local things should be named localXXX(), members that store
Local things should be named localXXX(), etc.

You can do that in a follow-up CL.

[dcheng, 2014-11-08 23:59:56]

https://codereview.chromium.org/683013002/diff/140001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/683013002/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Initializer.cpp:306: return;
On 2014/11/08 at 09:17:24, haraken wrote:
> If the target window is a RemoteDOMWindow, this code returns silently. We
might want to throw a SecurityError, just like the line 311 does.

Done. For now, I had to make the functions to generate error messages virtual
(and we won't be able to generate a sensible error for remote frames until the
security origin replication work I mentioned at BlinkOn 3 is finished). With
security origin replication, I believe we won't need to make this virtual--it
will be sufficient to implement it on the base DOMWindow class.

https://codereview.chromium.org/683013002/diff/140001/Source/bindings/core/v8...
File Source/bindings/core/v8/custom/V8WindowCustom.cpp (right):

https://codereview.chromium.org/683013002/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8WindowCustom.cpp:246: // FIXME: This is likely
to be problematic for the use counters below.
On 2014/11/08 09:17:24, haraken wrote:
> 
> Just help me understand: Why is it problematic?
> 
> I mentioned earlier that the same problem (although I don't understand the
> problem) should be happening in auto-generated code, but it shouldn't.
> Auto-generated code doesn't use window to record use counters.

UseCounter::countIfNotPrivateScript has two overloads: one takes a Document&,
and the other takes a ExecutionContext*. Auto-generated code tends to use the
latter. Since the UseCounter code just uses Document to get to FrameHost*, we
can update this to just pass Frame* instead.

https://codereview.chromium.org/683013002/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8WindowCustom.cpp:466: LocalDOMWindow*
targetWindow = toLocalDOMWindow(V8Window::toImpl(window));
On 2014/11/08 09:17:25, haraken wrote:
> 
> I think V8Window::toImpl(window) can be a RemoteDOMWindow. Can we return false
> when isLocalDOMWindow() fails?

Done.

https://codereview.chromium.org/683013002/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8WindowCustom.cpp:510: LocalDOMWindow*
targetWindow = toLocalDOMWindow(V8Window::toImpl(window));
On 2014/11/08 09:17:25, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/683013002/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8WindowCustom.cpp:541: // FIXME: How is this
used? Does this need to be able to handle remote DOM windows?
On 2014/11/08 09:17:25, haraken wrote:
> 
> I don't fully understand this comment.
> 
> The !window->isLocalDOMWindow() check below is very important in order not to
> expose a wrapper of a RemoteDOMWindow to user's script, isn't it?
> 
> Hmm, I got confused. In what case is it possible that toV8 is called with a
> RemoteDOMWindow? If it happens, I guess it is just wrong. Can we probably
assert
> that window->isLocalDOMWindow is true?

I've updated this FIXME. We do need to handle this eventually--this is called on
the code paths for window.self, etc (see 
https://docs.google.com/a/google.com/document/d/1IIxqMG35-AA3F-EloiITJBEy9KMH...
has some high-level details of the code flow).

I'm still trying to think through how this should work for remote frames though.
RemoteDOMWindow shouldn't really need a V8Context, since it cannot execute
script. And you don't really need a 'global' object for remote frames, since
there is no context to execute in, so we really just need the WindowProxy
concept for remote frames to keep window references from breaking on navigation.

https://codereview.chromium.org/683013002/diff/140001/Source/core/events/Touc...
File Source/core/events/TouchEvent.cpp (right):

https://codereview.chromium.org/683013002/diff/140001/Source/core/events/Touc...
Source/core/events/TouchEvent.cpp:99: if (!cancelable() && view() &&
view()->frame() && view()->frame()->isLocalFrame()) {
On 2014/11/08 09:17:25, haraken wrote:
> 
> Nit: Can we check view()->isLocalWindow() instead of
> view()->frame()->isLocalFrame() just for consistency?

Done.

https://codereview.chromium.org/683013002/diff/140001/Source/core/events/UIEv...
File Source/core/events/UIEvent.h (right):

https://codereview.chromium.org/683013002/diff/140001/Source/core/events/UIEv...
Source/core/events/UIEvent.h:33: typedef DOMWindow AbstractView;
I added a FIXME to get rid of AbstractView.

https://codereview.chromium.org/683013002/diff/140001/Source/core/frame/DOMWi...
File Source/core/frame/DOMWindow.h (right):

https://codereview.chromium.org/683013002/diff/140001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindow.h:175: virtual void
postMessage(PassRefPtr<SerializedScriptValue> message, const MessagePortArray*,
const String& targetOrigin, LocalDOMWindow* source, ExceptionState&) = 0;
On 2014/11/08 at 09:17:25, haraken wrote:
> Split these changes to a separate CL.

Done.

https://codereview.chromium.org/683013002/diff/140001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/683013002/diff/140001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:244: DOMWindow* LocalFrame::domWindow() const
On 2014/11/08 09:17:25, haraken wrote:
> 
> Can we remove this method?
> 
> DOMWindow* window = localFrame()->localDOMWindow();
> 
> will just work. It looks confusing to have both domWindow() and
> localDOMWindow().

The main reason we have localDOMWindow() is because I can't use covariant
returns for both LocalFrame::domWindow() and LocalDOMWindow::frame(), since this
would be a circular dependency. So instead, we expose the local version through
a non-virtual getter.

Unfortunately, removing either leads to uglier code elsewhere:
1. The reason we have this exposed as an overridable method on Frame is to
simplify code like DOMWindow::top(). Without this virtual, DOMWindow::top()
would have to look like:
    Frame* f = frame()->tree()->top();
    return f->isLocalFrame() ?
static_cast<DOMWindow*>(toLocalFrame(f)->localDOMWindow())
        : toRemoteFrame(f)->remoteDOMWindow();

2. The reason we have LocalFrame::localDOMWindow() is to prevent useless casts.
For example, DOMWindowProperty is always associated with a LocalFrame atm.
Without localDOMWindow(), the code would change from:
#if ENABLE(OILPAN)
        m_frame->localDOMWindow()->registerProperty(this);
#else
        m_associatedDOMWindow = m_frame->localDOMWindow();
        m_associatedDOMWindow->registerProperty(this);
#endif

to:
#if ENABLE(OILPAN)
        toLocalDOMWindow(m_frame->DOMWindow())->registerProperty(this);
#else
        m_associatedDOMWindow = toLocalDOMWindow(m_frame->localDOMWindow());
        m_associatedDOMWindow->registerProperty(this);
#endif

This cast is pointless, because we always have LocalFrames here, and LocalFrames
must have LocalDOMWindows.

https://codereview.chromium.org/683013002/diff/140001/Source/core/frame/Remot...
File Source/core/frame/RemoteFrame.h (right):

https://codereview.chromium.org/683013002/diff/140001/Source/core/frame/Remot...
Source/core/frame/RemoteFrame.h:25: virtual DOMWindow* domWindow() const
override { return 0; }
On 2014/11/08 09:17:25, haraken wrote:
> 
> I'm confused with this. Why does RemoteDOMWindow return 0 for domWindow()?
Given
> that RemoteDOMWindow is DOMWindow, I think it should return a DOMWindow
object.

This isn't implemented yet, but will be in a future patch.

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

https://codereview.chromium.org/683013002/diff/140001/Source/core/html/HTMLFr...
Source/core/html/HTMLFrameOwnerElement.h:45: DOMWindow* contentWindow() const;
On 2014/11/08 09:17:25, haraken wrote:
> 
> How is it possible that contentWindow is a RemoteDOMWindow? I guess it is
always
> a LocalDOMWindow.

Let's say you have a top-level frame on https://a.com with:
<body>
<iframe id="childFrame" src="https://b.com"></iframe>
</body>

document.body.getElementById('childFrame').contentWindow should work, even
though b.com may be rendered elsewhere.

https://codereview.chromium.org/683013002/diff/140001/Source/core/page/DOMWin...
File Source/core/page/DOMWindowPagePopup.cpp (right):

https://codereview.chromium.org/683013002/diff/140001/Source/core/page/DOMWin...
Source/core/page/DOMWindowPagePopup.cpp:52: PagePopupController*
DOMWindowPagePopup::pagePopupController(DOMWindow& window)
On 2014/11/08 at 09:17:25, haraken wrote:
> What's the point of changing LocalDOMWindow to DOMWindow, while we assume that
the window is a LocalDOMWindow below?

This is required by the way the bindings code currently generates code for
partial Window interfaces. The generated code wants to get the object associated
object with Window. But since I changed Window's ImplementedAs to DOMWindow,
bindings code passes around DOMWindow internally now, so it passes a DOMWindow
to these functions now (for example, pagePopupControllerAttributeGetter in the
generated V8Window.cpp).

There are two potential solutions:
1) Take the approach this patch does. Since these attributes should not be
accessible cross-origin, we should only ever have to handle a LocalDOMWindow
here. So just unconditionally cast to LocalDOMWindow.
2) Teach bindings code that it can downcast to LocalDOMWindow before calling
these methods. It could only use this heuristic on DOMWindow, and only with the
default security checks. I rejected this because it felt like it was tying
bindings and the implementation too tightly... but they're already pretty
tightly bound as it is.

[haraken, 2014-11-10 08:58:20]

LGTM

For code health, we might want to:

- Rename DOMWindow to Window
- Name methods and variables consistently:
--- Things that return local things should have a "local" prefix.
--- Things that return remote things should have a "remote" prefix.

in follow-up CLs.

https://codereview.chromium.org/683013002/diff/140001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/683013002/diff/140001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:244: DOMWindow* LocalFrame::domWindow() const
On 2014/11/08 23:59:56, dcheng wrote:
> On 2014/11/08 09:17:25, haraken wrote:
> > 
> > Can we remove this method?
> > 
> > DOMWindow* window = localFrame()->localDOMWindow();
> > 
> > will just work. It looks confusing to have both domWindow() and
> > localDOMWindow().
> 
> The main reason we have localDOMWindow() is because I can't use covariant
> returns for both LocalFrame::domWindow() and LocalDOMWindow::frame(), since
this
> would be a circular dependency. So instead, we expose the local version
through
> a non-virtual getter.
> 
> Unfortunately, removing either leads to uglier code elsewhere:
> 1. The reason we have this exposed as an overridable method on Frame is to
> simplify code like DOMWindow::top(). Without this virtual, DOMWindow::top()
> would have to look like:
>     Frame* f = frame()->tree()->top();
>     return f->isLocalFrame() ?
> static_cast<DOMWindow*>(toLocalFrame(f)->localDOMWindow())
>         : toRemoteFrame(f)->remoteDOMWindow();
> 
> 2. The reason we have LocalFrame::localDOMWindow() is to prevent useless
casts.
> For example, DOMWindowProperty is always associated with a LocalFrame atm.
> Without localDOMWindow(), the code would change from:
> #if ENABLE(OILPAN)
>         m_frame->localDOMWindow()->registerProperty(this);
> #else
>         m_associatedDOMWindow = m_frame->localDOMWindow();
>         m_associatedDOMWindow->registerProperty(this);
> #endif
> 
> to:
> #if ENABLE(OILPAN)
>         toLocalDOMWindow(m_frame->DOMWindow())->registerProperty(this);
> #else
>         m_associatedDOMWindow = toLocalDOMWindow(m_frame->localDOMWindow());
>         m_associatedDOMWindow->registerProperty(this);
> #endif
> 
> This cast is pointless, because we always have LocalFrames here, and
LocalFrames
> must have LocalDOMWindows.

Makes sense.

https://codereview.chromium.org/683013002/diff/140001/Source/core/page/DOMWin...
File Source/core/page/DOMWindowPagePopup.cpp (right):

https://codereview.chromium.org/683013002/diff/140001/Source/core/page/DOMWin...
Source/core/page/DOMWindowPagePopup.cpp:52: PagePopupController*
DOMWindowPagePopup::pagePopupController(DOMWindow& window)
On 2014/11/08 23:59:56, dcheng wrote:
> On 2014/11/08 at 09:17:25, haraken wrote:
> > What's the point of changing LocalDOMWindow to DOMWindow, while we assume
that
> the window is a LocalDOMWindow below?
> 
> This is required by the way the bindings code currently generates code for
> partial Window interfaces. The generated code wants to get the object
associated
> object with Window. But since I changed Window's ImplementedAs to DOMWindow,
> bindings code passes around DOMWindow internally now, so it passes a DOMWindow
> to these functions now (for example, pagePopupControllerAttributeGetter in the
> generated V8Window.cpp).
> 
> There are two potential solutions:
> 1) Take the approach this patch does. Since these attributes should not be
> accessible cross-origin, we should only ever have to handle a LocalDOMWindow
> here. So just unconditionally cast to LocalDOMWindow.
> 2) Teach bindings code that it can downcast to LocalDOMWindow before calling
> these methods. It could only use this heuristic on DOMWindow, and only with
the
> default security checks. I rejected this because it felt like it was tying
> bindings and the implementation too tightly... but they're already pretty
> tightly bound as it is.

Ah, understood. I'm fine with your approach :)

https://codereview.chromium.org/683013002/diff/200001/Source/core/events/Mous...
File Source/core/events/MouseRelatedEvent.cpp (right):

https://codereview.chromium.org/683013002/diff/200001/Source/core/events/Mous...
Source/core/events/MouseRelatedEvent.cpp:43: if (!abstractView ||
!abstractView->isLocalDOMWindow())

Is it OK to return LayoutSize() for a RemoteDOMWindow?

https://codereview.chromium.org/683013002/diff/200001/Source/core/events/Mous...
Source/core/events/MouseRelatedEvent.cpp:113: if (!event->view() ||
!event->view()->isLocalDOMWindow())

Is it OK to return 1 for a RemoteDOMWindow?

https://codereview.chromium.org/683013002/diff/200001/Source/core/events/Touc...
File Source/core/events/TouchEvent.cpp (right):

https://codereview.chromium.org/683013002/diff/200001/Source/core/events/Touc...
Source/core/events/TouchEvent.cpp:99: if (!cancelable() && view() &&
view()->isLocalDOMWindow() && view()->frame()) {

Hmm, it looks like you're assuming that AbstractView is always a LocalDOMWindow.
Probably can we rename view() to localDOMWindow() and return 0 for a
RemoteDOMWindow?

You can address it in a follow-up CL.

[dcheng, 2014-11-10 09:29:44]

https://codereview.chromium.org/683013002/diff/200001/Source/core/events/Mous...
File Source/core/events/MouseRelatedEvent.cpp (right):

https://codereview.chromium.org/683013002/diff/200001/Source/core/events/Mous...
Source/core/events/MouseRelatedEvent.cpp:43: if (!abstractView ||
!abstractView->isLocalDOMWindow())
On 2014/11/10 08:58:20, haraken wrote:
> 
> Is it OK to return LayoutSize() for a RemoteDOMWindow?

This code is hit on the path for initMouseEvent(). Since JS can pass any window
to initMouseEvent(), it could pass a window for a frame that is remote. I think
you could argue that the current implementation is actually an info leak. I
think you could potentially use it to detect page zoom across origins--pass a
cross-origin Window object to initMouseEvent() and observe how the
clientX/clientY coordinates change from the supplied values. In that respect, I
think that returning LayoutSize() is reasonable here.

https://codereview.chromium.org/683013002/diff/200001/Source/core/events/Mous...
Source/core/events/MouseRelatedEvent.cpp:67: LocalFrame* frame = view() &&
view()->isLocalDOMWindow() ? toLocalDOMWindow(view())->frame() : 0;
Note: looking at more code, I'm not sure if we can actually get a
RemoteDOMWindow here. I will probably change this to just call
toLocalDOMWindow() directly for the time being, and see if we get any errors
with the --site-per-process flag.

https://codereview.chromium.org/683013002/diff/200001/Source/core/events/Mous...
Source/core/events/MouseRelatedEvent.cpp:113: if (!event->view() ||
!event->view()->isLocalDOMWindow())
On 2014/11/10 08:58:20, haraken wrote:
> 
> Is it OK to return 1 for a RemoteDOMWindow?
> 

This is similar to my previous comment.

https://codereview.chromium.org/683013002/diff/200001/Source/core/events/Touc...
File Source/core/events/TouchEvent.cpp (right):

https://codereview.chromium.org/683013002/diff/200001/Source/core/events/Touc...
Source/core/events/TouchEvent.cpp:99: if (!cancelable() && view() &&
view()->isLocalDOMWindow() && view()->frame()) {
On 2014/11/10 08:58:20, haraken wrote:
> 
> Hmm, it looks like you're assuming that AbstractView is always a
LocalDOMWindow.
> Probably can we rename view() to localDOMWindow() and return 0 for a
> RemoteDOMWindow?
> 
> You can address it in a follow-up CL.

I need to double check, but I think it is possible for view() to be remote.
view() can be set by JS by calling initTouchEvent().

[dcheng, 2014-11-10 20:41:55]

https://codereview.chromium.org/683013002/diff/200001/Source/core/events/Mous...
File Source/core/events/MouseRelatedEvent.cpp (right):

https://codereview.chromium.org/683013002/diff/200001/Source/core/events/Mous...
Source/core/events/MouseRelatedEvent.cpp:67: LocalFrame* frame = view() &&
view()->isLocalDOMWindow() ? toLocalDOMWindow(view())->frame() : 0;
On 2014/11/10 09:29:43, dcheng wrote:
> Note: looking at more code, I'm not sure if we can actually get a
> RemoteDOMWindow here. I will probably change this to just call
> toLocalDOMWindow() directly for the time being, and see if we get any errors
> with the --site-per-process flag.

Actually it turns out this is web-exposed:
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...

UIEventInit/MouseEventInit can pass a user-supplied DOMWindow back to us. So we
can't eliminate this check =(

[dcheng, 2014-11-10 20:41:58]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-11-10 20:42:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/683013002/200001

[commit-bot: I haz the power, 2014-11-10 21:57:19]

Committed patchset #11 (id:200001) as 185067