Remove focus/blur webview handlers

Remove focus/blur webview handlers

These are not apparently needed in order to make BrowserPlugin based webview work.

Committed: https://crrev.com/53ba951bb6ed4df0998240520fed9d00a7ffff67
Cr-Commit-Position: refs/heads/master@{#404165}

[avallee, 2016-07-06 14:51:51]

avallee@chromium.org changed reviewers:
+ lazyboy@chromium.org

[avallee, 2016-07-06 14:51:52]

lazyboy: This seems to pass tests. Do you know why this is here? Tests seem fine
without these lines.

Fady: FYI/care to comment?

[Fady Samuel, 2016-07-06 14:55:06]

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

[Fady Samuel, 2016-07-06 14:55:06]

Are you sure these tests pass?
https://cs.chromium.org/chromium/src/chrome/browser/apps/guest_view/web_view_...

https://codereview.chromium.org/2123033002/diff/1/extensions/renderer/resourc...
File extensions/renderer/resources/guest_view/guest_view_container.js (left):

https://codereview.chromium.org/2123033002/diff/1/extensions/renderer/resourc...
extensions/renderer/resources/guest_view/guest_view_container.js:104:
this.element.addEventListener('focus', this.weakWrapper(function(e) {
Not LGTM. This will break focus, although I think we don't have direct test
coverage for this unfortuantely. Basically focus propagation happens through
BrowserPlugin (unless that's changed now?) and so if we don't pass along focus
state to BrowserPlugin then the guest content's focus state will not be updated.

[avallee, 2016-07-06 15:46:04]

On 2016/07/06 14:55:06, Fady Samuel wrote:
> Are you sure these tests pass?
>
https://cs.chromium.org/chromium/src/chrome/browser/apps/guest_view/web_view_...
> 
>
https://codereview.chromium.org/2123033002/diff/1/extensions/renderer/resourc...
> File extensions/renderer/resources/guest_view/guest_view_container.js (left):
> 
>
https://codereview.chromium.org/2123033002/diff/1/extensions/renderer/resourc...
> extensions/renderer/resources/guest_view/guest_view_container.js:104:
> this.element.addEventListener('focus', this.weakWrapper(function(e) {
> Not LGTM. This will break focus, although I think we don't have direct test
> coverage for this unfortuantely. Basically focus propagation happens through
> BrowserPlugin (unless that's changed now?) and so if we don't pass along focus
> state to BrowserPlugin then the guest content's focus state will not be
updated.

I've just tried this in Browser Sample with the browser plugin and focus works
just fine. It also fixes the bug that when you alt-tab away from the window, the
<webview> does not regain focus when you alt-tab back.

I ran the whole suite of WebView* tests. 

Note: Google Test filter = WebViewFocusInteractiveTest.Focus_FocusEvent
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from WebViewFocusInteractiveTest, where TypeParam =
[ RUN      ] WebViewFocusInteractiveTest.Focus_FocusEvent
[91253:91253:0705/163230:WARNING:password_store_factory.cc(250)] Using basic
(unencrypted) store for password storage. See
https://chromium.googlesource.com/chromium/src/+/master/docs/linux_password_s...
for more information about password storage options.
[91253:91253:0705/163232:INFO:CONSOLE(134)] "loadstop", source:
chrome-extension://pbpknpajjjbgepejnocaakcnnpgahpoo/embedder.js (134)
[91253:91253:0705/163232:INFO:CONSOLE(138)] "Injected script into webview.",
source: chrome-extension://pbpknpajjjbgepejnocaakcnnpgahpoo/embedder.js (138)
[       OK ] WebViewFocusInteractiveTest.Focus_FocusEvent (4192 ms)
[----------] 1 test from WebViewFocusInteractiveTest (4192 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (4192 ms total)
[  PASSED  ] 1 test.
[3/46] WebViewFocusInteractiveTest.Focus_FocusEvent (4981 ms)

[Fady Samuel, 2016-07-06 17:31:57]

https://codereview.chromium.org/2123033002/diff/1/extensions/renderer/resourc...
File extensions/renderer/resources/guest_view/guest_view_container.js (left):

https://codereview.chromium.org/2123033002/diff/1/extensions/renderer/resourc...
extensions/renderer/resources/guest_view/guest_view_container.js:104:
this.element.addEventListener('focus', this.weakWrapper(function(e) {
On 2016/07/06 14:55:06, Fady Samuel wrote:
> Not LGTM. This will break focus, although I think we don't have direct test
> coverage for this unfortuantely. Basically focus propagation happens through
> BrowserPlugin (unless that's changed now?) and so if we don't pass along focus
> state to BrowserPlugin then the guest content's focus state will not be
updated.

Ah I think I see why/how this is not necessary anymore:
GuestViewContainer.prototype.focus is overridden.

I wonder if we need to override blur too?

[avallee, 2016-07-06 17:34:23]

https://codereview.chromium.org/2123033002/diff/1/extensions/renderer/resourc...
File extensions/renderer/resources/guest_view/guest_view_container.js (left):

https://codereview.chromium.org/2123033002/diff/1/extensions/renderer/resourc...
extensions/renderer/resources/guest_view/guest_view_container.js:104:
this.element.addEventListener('focus', this.weakWrapper(function(e) {
On 2016/07/06 17:31:57, Fady Samuel wrote:
> On 2016/07/06 14:55:06, Fady Samuel wrote:
> > Not LGTM. This will break focus, although I think we don't have direct test
> > coverage for this unfortuantely. Basically focus propagation happens through
> > BrowserPlugin (unless that's changed now?) and so if we don't pass along
focus
> > state to BrowserPlugin then the guest content's focus state will not be
> updated.
> 
> Ah I think I see why/how this is not necessary anymore:
> GuestViewContainer.prototype.focus is overridden.
> 
> I wonder if we need to override blur too?

I could see how propagating the event might make sense. It might want to know
that something blurred the window. However, calling blur() will make it lose
focus completely.

[Fady Samuel, 2016-07-06 17:37:24]

On 2016/07/06 17:34:27, avallee wrote:

The current expectation is if you call blur, the guest should know it lost
focus. Does that happen with your change?

[avallee, 2016-07-06 19:12:30]

On 2016/07/06 17:37:24, Fady Samuel wrote:
> On 2016/07/06 17:34:27, avallee wrote:
> 
> The current expectation is if you call blur, the guest should know it lost
> focus. Does that happen with your change?

Tried running this snippet in the inspector.

Clicking the C button that shows up will blur the webview after 3 seconds and
then focus it again 3 seconds later. During this time, it will log to the
console document.activeElement every second. I click the button then click on
the content. The input box gets highlighted and a blinking caret. Then at some
point it stops for 3 seconds and then the caret comes back.

var cycle = document.createElement('button');cycle.title = 'Cycle
focus';cycle.innerText='C';cycle.addEventListener('click', function(){var
wv=document.getElementsByTagName('webview')[0];var
i=setInterval(function(){console.log(document.activeElement);},1000);setTimeout(function(){wv.blur();setTimeout(function(){wv.focus();setTimeout(clearInterval,1000,i);},3000)},3000)});document.getElementById('controls').appendChild(cycle);

[Fady Samuel, 2016-07-06 19:19:53]

OK lgtm

[avallee, 2016-07-06 20:32:23]

Description was changed from

==========
Remove focus/blur webview handlers

BUG=
==========

to

==========
Remove focus/blur webview handlers

These are not apparently needed in order to make BrowserPlugin based webview
work.
==========

[avallee, 2016-07-06 20:32:23]

avallee@chromium.org changed reviewers:
- lazyboy@chromium.org

[avallee, 2016-07-07 15:07:00]

The CQ bit was checked by avallee@chromium.org

[commit-bot: I haz the power, 2016-07-07 15:07:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 17:27:46]

Description was changed from

==========
Remove focus/blur webview handlers

These are not apparently needed in order to make BrowserPlugin based webview
work.
==========

to

==========
Remove focus/blur webview handlers

These are not apparently needed in order to make BrowserPlugin based webview
work.
==========

[commit-bot: I haz the power, 2016-07-07 17:27:47]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-07-07 17:28:11]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-07 17:30:25]

Description was changed from

==========
Remove focus/blur webview handlers

These are not apparently needed in order to make BrowserPlugin based webview
work.
==========

to

==========
Remove focus/blur webview handlers

These are not apparently needed in order to make BrowserPlugin based webview
work.

Committed: https://crrev.com/53ba951bb6ed4df0998240520fed9d00a7ffff67
Cr-Commit-Position: refs/heads/master@{#404165}
==========

[commit-bot: I haz the power, 2016-07-07 17:30:26]

Patchset 1 (id:??) landed as
https://crrev.com/53ba951bb6ed4df0998240520fed9d00a7ffff67
Cr-Commit-Position: refs/heads/master@{#404165}