Rename Document::ownerElement to localOwner and fix main frame checks.

Rename Document::ownerElement to localOwner and fix main frame checks.

Document::ownerElement returns nullptr if the owner is in a remote frame so the
new name is more clear of that.

I've updated call sites that were using !ownerElement as a signal that the
document was in the main frame to check the frame's isMainFrame method instead
since that should work in OOPIF scenarios.

BUG=607001
Committed: https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c
Cr-Commit-Position: refs/heads/master@{#392637}

[bokan, 2016-04-30 16:59:30]

Description was changed from

==========
Rename Document::ownerElement to localOwnerElement and fix main frame checks.

Document::ownerElement returns nullptr if the owner is in a remote frame so the
new name is more clear of that. Additionally, some code has traditionally used
the absence of an ownerElement as a sign that the frame was the main frame. This
assumption breaks under OOPIF. This patch fixes checks in RootScroller and
EventHandler.

BUG=607001
==========

to

==========
Rename Document::ownerElement to localOwnerElement and fix main frame checks.

Document::ownerElement returns nullptr if the owner is in a remote frame so the
new name is more clear of that. Additionally, some code has traditionally used
the absence of an ownerElement as a sign that the frame was the main frame. This
assumption breaks under OOPIF. This patch fixes checks in RootScroller and
EventHandler.

BUG=607001
==========

[bokan, 2016-04-30 16:59:31]

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

[bokan, 2016-04-30 17:01:37]

Hi Daniel,

I used isMainFrame rather than Frame::owner, they seem equivalent but the former
is more clear as to intent. Is that fine? Turned out this wasn't used much in
EventHandler any more after all but I fixed that and RootScroller.

I also renamed ownerElement rather than adding a comment.

[bokan, 2016-04-30 17:01:44]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-30 17:01:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1942623002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1942623002/1

[commit-bot: I haz the power, 2016-04-30 19:58:19]

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

[commit-bot: I haz the power, 2016-04-30 19:58:19]

Dry run: This issue passed the CQ dry run.

[bokan, 2016-05-03 17:55:48]

ping

[dcheng, 2016-05-03 18:47:40]

I think if we're changing this, we should fix all the main frame checks (like
the ones in the various Document subclasses, in CompositingReasonFinder which
has a FIXME for this, etc).

If it's a lot of boilerplate, I could envision a Document::inMainFrame() kind of
thing.

https://codereview.chromium.org/1942623002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1942623002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/EventHandler.cpp:4184: return
&m_frame->page()->frameHost();
We don't really go for const-correctness elsewhere. I'm honestly ok with just
marking EventHandler::frameHost() const and just returning a non-const pointer.

[bokan, 2016-05-03 21:40:10]

Description was changed from

==========
Rename Document::ownerElement to localOwnerElement and fix main frame checks.

Document::ownerElement returns nullptr if the owner is in a remote frame so the
new name is more clear of that. Additionally, some code has traditionally used
the absence of an ownerElement as a sign that the frame was the main frame. This
assumption breaks under OOPIF. This patch fixes checks in RootScroller and
EventHandler.

BUG=607001
==========

to

==========
Rename Document::ownerElement to localOwnerElement and fix main frame checks.

Document::ownerElement returns nullptr if the owner is in a remote frame so the
new name is more clear of that.

I've updated call sites that were using !ownerElement as a signal that the
document was in the main frame to check the frame's isMainFrame method instead
since that should work in OOPIF scenarios.

BUG=607001
==========

[bokan, 2016-05-03 21:41:03]

> I think if we're changing this, we should fix all the main
> frame checks (like the ones in the various Document
> subclasses, in CompositingReasonFinder which has a FIXME
> for this, etc).
>
> If it's a lot of boilerplate, I could envision a
> Document::inMainFrame() kind of thing.

Done.

https://codereview.chromium.org/1942623002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1942623002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/EventHandler.cpp:4184: return
&m_frame->page()->frameHost();
On 2016/05/03 18:47:40, dcheng wrote:
> We don't really go for const-correctness elsewhere.

IMO, that's something we should be working towards fixing. We'll likely never
get the big classes to total const-correctness but this makes reading and
writing new code easier. Unless you're strongly opposed I'd like to keep this. I
added a little casting magic to remove duplication in the latest patch.

[bokan, 2016-05-03 21:41:04]

> I think if we're changing this, we should fix all the main
> frame checks (like the ones in the various Document
> subclasses, in CompositingReasonFinder which has a FIXME
> for this, etc).
>
> If it's a lot of boilerplate, I could envision a
> Document::inMainFrame() kind of thing.

Done.

https://codereview.chromium.org/1942623002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1942623002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/EventHandler.cpp:4184: return
&m_frame->page()->frameHost();
On 2016/05/03 18:47:40, dcheng wrote:
> We don't really go for const-correctness elsewhere.

IMO, that's something we should be working towards fixing. We'll likely never
get the big classes to total const-correctness but this makes reading and
writing new code easier. Unless you're strongly opposed I'd like to keep this. I
added a little casting magic to remove duplication in the latest patch.

[bokan, 2016-05-05 12:38:13]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-05 12:38:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1942623002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1942623002/40001

[commit-bot: I haz the power, 2016-05-05 14:02:00]

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

[commit-bot: I haz the power, 2016-05-05 14:02:01]

Dry run: This issue passed the CQ dry run.

[bokan, 2016-05-05 14:59:11]

ping

[dcheng, 2016-05-07 06:09:08]

Sorry for the delay on the review, I was looking at how topDocument() and
parentDocument() get used throughout the codebase today.

https://codereview.chromium.org/1942623002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1942623002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/EventHandler.cpp:4184: return
&m_frame->page()->frameHost();
On 2016/05/03 at 21:41:04, bokan wrote:
> On 2016/05/03 18:47:40, dcheng wrote:
> > We don't really go for const-correctness elsewhere.
> 
> IMO, that's something we should be working towards fixing. We'll likely never
get the big classes to total const-correctness but this makes reading and
writing new code easier. Unless you're strongly opposed I'd like to keep this. I
added a little casting magic to remove duplication in the latest patch.

Do we have any instances of this pattern anywhere else in Blink? If there's some
classes trying to be const-correct, then I agree it makes sense. Otherwise, this
code will just look out of place and be inconsistent with everywhere else.

https://codereview.chromium.org/1942623002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1942623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:601: if (!frame()) {
Having to add an extra case here feels awkward to me. Maybe we can just change
the error message below to "root scroll can only be set on the main frame's
document"?

https://codereview.chromium.org/1942623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:638: if (!frame())
Is it necessary to distinguish this case? We previously didn't (e.g. see above)

https://codereview.chromium.org/1942623002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/1942623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.h:699: HTMLFrameOwnerElement*
localOwnerElement() const;
Thinking about this more, 'Element' ought to imply (and be redundant with)
local. Would calling this localOwner() makes things clearer? It'd also be more
consistent with how Frame naming currently works.

[bokan, 2016-05-09 20:43:42]

Description was changed from

==========
Rename Document::ownerElement to localOwnerElement and fix main frame checks.

Document::ownerElement returns nullptr if the owner is in a remote frame so the
new name is more clear of that.

I've updated call sites that were using !ownerElement as a signal that the
document was in the main frame to check the frame's isMainFrame method instead
since that should work in OOPIF scenarios.

BUG=607001
==========

to

==========
Rename Document::ownerElement to localOwner and fix main frame checks.

Document::ownerElement returns nullptr if the owner is in a remote frame so the
new name is more clear of that.

I've updated call sites that were using !ownerElement as a signal that the
document was in the main frame to check the frame's isMainFrame method instead
since that should work in OOPIF scenarios.

BUG=607001
==========

[bokan, 2016-05-09 20:43:58]

ptal

https://codereview.chromium.org/1942623002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1942623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:601: if (!frame()) {
On 2016/05/07 06:09:07, dcheng wrote:
> Having to add an extra case here feels awkward to me. Maybe we can just change
> the error message below to "root scroll can only be set on the main frame's
> document"?

Done.

https://codereview.chromium.org/1942623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:638: if (!frame())
On 2016/05/07 06:09:07, dcheng wrote:
> Is it necessary to distinguish this case? We previously didn't (e.g. see
above)

Done.

https://codereview.chromium.org/1942623002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/1942623002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.h:699: HTMLFrameOwnerElement*
localOwnerElement() const;
On 2016/05/07 06:09:08, dcheng wrote:
> Thinking about this more, 'Element' ought to imply (and be redundant with)
> local. Would calling this localOwner() makes things clearer? It'd also be more
> consistent with how Frame naming currently works.

sgtm, done.

[esprehn, 2016-05-09 21:22:33]

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

[esprehn, 2016-05-09 21:22:34]

We usually just mark the getters as const, like Node* firstChild() const. no
need for the actual const correctness unless you really want it but it's pretty
gross honestly.

https://codereview.chromium.org/1942623002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1942623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:2436: return
frameHost()->rootScroller()->get() == toElement(&node);
you don't need to cast a node to an Element to compare it. Also this whole
function is:

if (!frameHost())
  return false;
return frameHost()->rootScroller() == node;

https://codereview.chromium.org/1942623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:3712: static_cast<const
EventHandler&>(*this).frameHost());
woah, just mark the getter as const

https://codereview.chromium.org/1942623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:3715: const FrameHost*
EventHandler::frameHost() const
no need for this, the getter can just be const

[dcheng, 2016-05-09 21:26:14]

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

[dcheng, 2016-05-09 21:26:16]

lgtm with nits addressed

https://codereview.chromium.org/1942623002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1942623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:632: if (!frame() ||
!isInMainFrame())
Nit: !frame() is redundant here probably

https://codereview.chromium.org/1942623002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1942623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:3715: const FrameHost*
EventHandler::frameHost() const
I think we should just make this FrameHost* EventHandler::frameHost() const, for
consistency with the rest of Blink (e.g. the ownerElement method renamed in this
CL is a const-method returning a non-const pointer still).

I think it's unlikely we're going to want this pattern in Blink, unless someone
can figure out what this would look like and what the benefits would be.

[bokan, 2016-05-09 22:26:18]

https://codereview.chromium.org/1942623002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1942623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:632: if (!frame() ||
!isInMainFrame())
On 2016/05/09 21:26:16, dcheng wrote:
> Nit: !frame() is redundant here probably

Done.

https://codereview.chromium.org/1942623002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1942623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:2436: return
frameHost()->rootScroller()->get() == toElement(&node);
On 2016/05/09 21:22:34, esprehn wrote:
> you don't need to cast a node to an Element to compare it.

Done, but this will break silently if Element or ContainerNode ever multiply
inherit from an additional class before Node. Do we disallow MI with data
members?

> Also this whole function is:
> 
> if (!frameHost())
>   return false;
> return frameHost()->rootScroller() == node;

The rootScroller check is needed since it can be null in the OOPIF case right
now.

https://codereview.chromium.org/1942623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:3712: static_cast<const
EventHandler&>(*this).frameHost());
On 2016/05/09 21:22:34, esprehn wrote:
> woah, just mark the getter as const

Done.

https://codereview.chromium.org/1942623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:3715: const FrameHost*
EventHandler::frameHost() const
On 2016/05/09 21:22:34, esprehn wrote:
> no need for this, the getter can just be const

Done.

[dcheng, 2016-05-10 01:22:46]

(still )lgtm

[bokan, 2016-05-10 15:57:57]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2016-05-10 15:58:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1942623002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1942623002/80001

[commit-bot: I haz the power, 2016-05-10 17:29:22]

Description was changed from

==========
Rename Document::ownerElement to localOwner and fix main frame checks.

Document::ownerElement returns nullptr if the owner is in a remote frame so the
new name is more clear of that.

I've updated call sites that were using !ownerElement as a signal that the
document was in the main frame to check the frame's isMainFrame method instead
since that should work in OOPIF scenarios.

BUG=607001
==========

to

==========
Rename Document::ownerElement to localOwner and fix main frame checks.

Document::ownerElement returns nullptr if the owner is in a remote frame so the
new name is more clear of that.

I've updated call sites that were using !ownerElement as a signal that the
document was in the main frame to check the frame's isMainFrame method instead
since that should work in OOPIF scenarios.

BUG=607001
==========

[commit-bot: I haz the power, 2016-05-10 17:29:24]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-10 17:31:10]

Description was changed from

==========
Rename Document::ownerElement to localOwner and fix main frame checks.

Document::ownerElement returns nullptr if the owner is in a remote frame so the
new name is more clear of that.

I've updated call sites that were using !ownerElement as a signal that the
document was in the main frame to check the frame's isMainFrame method instead
since that should work in OOPIF scenarios.

BUG=607001
==========

to

==========
Rename Document::ownerElement to localOwner and fix main frame checks.

Document::ownerElement returns nullptr if the owner is in a remote frame so the
new name is more clear of that.

I've updated call sites that were using !ownerElement as a signal that the
document was in the main frame to check the frame's isMainFrame method instead
since that should work in OOPIF scenarios.

BUG=607001

Committed: https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c
Cr-Commit-Position: refs/heads/master@{#392637}
==========

[commit-bot: I haz the power, 2016-05-10 17:31:12]

Patchset 5 (id:??) landed as
https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c
Cr-Commit-Position: refs/heads/master@{#392637}

[Charlie Reis, 2016-05-10 19:40:25]

Description was changed from

==========
Rename Document::ownerElement to localOwner and fix main frame checks.

Document::ownerElement returns nullptr if the owner is in a remote frame so the
new name is more clear of that.

I've updated call sites that were using !ownerElement as a signal that the
document was in the main frame to check the frame's isMainFrame method instead
since that should work in OOPIF scenarios.

BUG=607001

Committed: https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c
Cr-Commit-Position: refs/heads/master@{#392637}
==========

to

==========
Rename Document::ownerElement to localOwner and fix main frame checks.

Document::ownerElement returns nullptr if the owner is in a remote frame so the
new name is more clear of that.

I've updated call sites that were using !ownerElement as a signal that the
document was in the main frame to check the frame's isMainFrame method instead
since that should work in OOPIF scenarios.

BUG=607001

Committed: https://crrev.com/f66e6b578fce9b4aed705f973acc547235e73e2c
Cr-Commit-Position: refs/heads/master@{#392637}
==========