Remove RenderViewImpl::GetFocusedElement.

Remove RenderViewImpl::GetFocusedElement.

R=esprehn,enne
BUG=612570
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/4969a3e260e13d9d91ac192cb675ab77f0831f35
Cr-Commit-Position: refs/heads/master@{#395166}

[dglazkov, 2016-05-17 20:00:59]

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

[commit-bot: I haz the power, 2016-05-17 20:02:45]

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

[dglazkov, 2016-05-17 22:00:15]

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

[dglazkov, 2016-05-17 22:00:16]

PTAL

https://codereview.chromium.org/1990553002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1990553002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewImpl.cpp:2945: if
(element->isContentEditable())
Not loving this duplication. I can try to fold removing WebElement::isEditable
into this patch?

[dglazkov, 2016-05-17 22:00:31]

Description was changed from

==========
Remove RenderViewImpl::GetFocusedElement.

BUG=612570
==========

to

==========
Remove RenderViewImpl::GetFocusedElement.

R=esprehn
BUG=612570
==========

[esprehn, 2016-05-17 22:16:43]

https://codereview.chromium.org/1990553002/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1990553002/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:1412: if
(!webview()->scrollFocusedEditableElementIntoRect(rect, will_animate))
This out param is pretty gross, maybe we can know if something will animate by
instead doing compositor()->hasPendingPageScaleAnimation() and exposing that
data? The call to startPageScaleAnimation inside WebViewImpl is actually just
ending up inside RenderWidgetCompositor anyway.

https://codereview.chromium.org/1990553002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1990553002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewImpl.cpp:2945: if
(element->isContentEditable())
On 2016/05/17 at 22:00:15, dglazkov wrote:
> Not loving this duplication. I can try to fold removing WebElement::isEditable
into this patch?

Nah, lets do that next. Feels more likely to break stuff.

[commit-bot: I haz the power, 2016-05-17 22:26:09]

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

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

Dry run: This issue passed the CQ dry run.

[dglazkov, 2016-05-18 05:11:57]

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

[dglazkov, 2016-05-18 05:12:01]

Now with HasPendingPageScaleAnimation!

[dglazkov, 2016-05-18 05:12:05]

Description was changed from

==========
Remove RenderViewImpl::GetFocusedElement.

R=esprehn
BUG=612570
==========

to

==========
Remove RenderViewImpl::GetFocusedElement.

R=esprehn
BUG=612570
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-05-18 05:12:20]

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

[commit-bot: I haz the power, 2016-05-18 05:46:01]

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

[commit-bot: I haz the power, 2016-05-18 05:46:02]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dglazkov, 2016-05-18 18:44:57]

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

[dglazkov, 2016-05-18 18:45:01]

Really works now.

[commit-bot: I haz the power, 2016-05-18 18:45:28]

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

[esprehn, 2016-05-18 18:49:17]

lgtm

https://codereview.chromium.org/1990553002/diff/40001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/1990553002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:278: bool HasPendingPageScaleAnimation() const;
you could probably do

bool has_pending_page_scale_animation() const { return ...; }

in the header too

[esprehn, 2016-05-18 18:49:28]

You need a cc/ OWNER btw.

[dglazkov, 2016-05-18 18:52:23]

dglazkov@chromium.org changed reviewers:
+ enne@chromium.org

[dglazkov, 2016-05-18 18:52:34]

On 2016/05/18 at 18:49:28, esprehn wrote:
> You need a cc/ OWNER btw.

+enne, PTAL? :)

[dglazkov, 2016-05-18 18:57:28]

Description was changed from

==========
Remove RenderViewImpl::GetFocusedElement.

R=esprehn
BUG=612570
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Remove RenderViewImpl::GetFocusedElement.

R=esprehn,enne
BUG=612570
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-05-18 19:23:41]

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

[commit-bot: I haz the power, 2016-05-18 19:23:42]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[enne (OOO), 2016-05-18 19:30:29]

https://codereview.chromium.org/1990553002/diff/40001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1990553002/diff/40001/content/renderer/render...
content/renderer/render_view_impl.cc:1416: if
(!compositor()->hasPendingPageScaleAnimation())
I don't know this code at all, but what's the goal of this check? It looks like
if you have a pending page scale animation, then eventually you'll get a
RenderViewImpl::DidCompletePageScaleAnimation which will call
FocusChangeComplete.  So, I'm guessing you're avoiding the call here because
it'll get done later?

I feel like there's a race here, potentially.  There could be an active page
scale animation, but one that's already been committed and so it not pending. 
Then you'd get FocusChangeComplete here and then another one later when the page
scale animation completes.

Is that what you want? What are you trying to do here?

[dglazkov, 2016-05-18 19:50:34]

https://codereview.chromium.org/1990553002/diff/40001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1990553002/diff/40001/content/renderer/render...
content/renderer/render_view_impl.cc:1416: if
(!compositor()->hasPendingPageScaleAnimation())
On 2016/05/18 at 19:30:29, enne wrote:
> I don't know this code at all, but what's the goal of this check? It looks
like if you have a pending page scale animation, then eventually you'll get a
RenderViewImpl::DidCompletePageScaleAnimation which will call
FocusChangeComplete.  So, I'm guessing you're avoiding the call here because
it'll get done later?
> 
> I feel like there's a race here, potentially.  There could be an active page
scale animation, but one that's already been committed and so it not pending. 
Then you'd get FocusChangeComplete here and then another one later when the page
scale animation completes.
> 
> Is that what you want? What are you trying to do here?

I am hoping not to make any functional changes -- this change here is just avoid
using passing bools instead of just querying asking the source. 

In the function above, the WebViewImpl::scrollFocusedEditableElementIntoRect may
or may (indirectly) invoke LayerTreeHost::StartPageScaleAnimation. The check in
question is just to see if that happened. There shouldn't be a race in this
case.

However, the new code could be behaving differently is
LayerTreeHost::pending_page_scale_animation_ is already set, and
WebViewImpl::scrollFocusedEditableElementIntoRect ends up _not_ calling
LayerTreeHost::StartPageScaleAnimation. I need to better understand if that's
possible and if the new behavior would be a good idea.

[dglazkov, 2016-05-19 04:12:31]

https://codereview.chromium.org/1990553002/diff/40001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1990553002/diff/40001/content/renderer/render...
content/renderer/render_view_impl.cc:1416: if
(!compositor()->hasPendingPageScaleAnimation())
Oh, I think you meant the code that was there before. Yeah, I can see that this
is really skeezy. I should probably look into it.

For this CL, would you like me to leave this alone and revert to using the bool?
Let me know.

[enne (OOO), 2016-05-19 18:40:42]

I looked at this a little bit more, and lgtm.

I think there's no race here, because this all happens on the same frame.  It
looks a bit awkward to have these two paths that need to know about each other's
behavior so indirectly, but this seems better than what was there before.

[dglazkov, 2016-05-20 04:19:08]

On 2016/05/19 at 18:40:42, enne wrote:
> I looked at this a little bit more, and lgtm.
> 
> I think there's no race here, because this all happens on the same frame.  It
looks a bit awkward to have these two paths that need to know about each other's
behavior so indirectly, but this seems better than what was there before.

Yay! Thanks!

[dglazkov, 2016-05-20 04:19:13]

The CQ bit was checked by dglazkov@chromium.org

[dglazkov, 2016-05-20 04:19:24]

The CQ bit was unchecked by dglazkov@chromium.org

[dglazkov, 2016-05-20 19:36:08]

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

[dglazkov, 2016-05-20 19:36:19]

Made Windows happy and brought back bool return because fake tests.

[commit-bot: I haz the power, 2016-05-20 19:36:23]

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

[commit-bot: I haz the power, 2016-05-20 21:03:50]

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

[commit-bot: I haz the power, 2016-05-20 21:03:51]

Dry run: This issue passed the CQ dry run.

[dglazkov, 2016-05-20 21:07:46]

The CQ bit was checked by dglazkov@chromium.org

[dglazkov, 2016-05-20 21:07:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
esprehn@chromium.org, enne@chromium.org
Link to the patchset: https://codereview.chromium.org/1990553002/#ps60001
(title: "Made Windows happy and brought back bool return because fake tests.")

[commit-bot: I haz the power, 2016-05-20 21:08:13]

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

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

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-20 21:15:42]

Description was changed from

==========
Remove RenderViewImpl::GetFocusedElement.

R=esprehn,enne
BUG=612570
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Remove RenderViewImpl::GetFocusedElement.

R=esprehn,enne
BUG=612570
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/4969a3e260e13d9d91ac192cb675ab77f0831f35
Cr-Commit-Position: refs/heads/master@{#395166}
==========

[commit-bot: I haz the power, 2016-05-20 21:15:44]

Patchset 4 (id:??) landed as
https://crrev.com/4969a3e260e13d9d91ac192cb675ab77f0831f35
Cr-Commit-Position: refs/heads/master@{#395166}