Popup loosing focus through keyboard events should update HTMLSelectElement display

Popup loosing focus through keyboard events should update HTMLSelectElement display

Ctrl-N while a popup menu is open, causes the window to loose focus, and ViewMsg_Close is sent to the popup widget.
This causes WebPagePopupImpl::close to be called.
Right now this causes the popup to close without updating the display from the provisional selection (ie HTMLSelectElement::indexToSelectOnCancel) to the actual selection.
WebPagePopupImpl::close should use WebPagePopupImpl::cancel to handle that case properly.

Loosing focus through mouse clicks were working because we manually call cancel from the mouse event handler.

Regression caused by r193034

BUG=597206
Committed: https://crrev.com/831776ec4a98f781dc8ce20bb3346538fd59b8da
Cr-Commit-Position: refs/heads/master@{#383271}

Committed: https://crrev.com/ae1e3ad7ec33e9e4f0d47cd37c170ef51d478752
Cr-Commit-Position: refs/heads/master@{#383691}

[keishi, 2016-03-25 07:05:17]

keishi@chromium.org changed reviewers:
+ tkent@chromium.org

[keishi, 2016-03-25 07:05:22]

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

[commit-bot: I haz the power, 2016-03-25 07:05:26]

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

[tkent, 2016-03-25 07:30:45]

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

https://codereview.chromium.org/1833843002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebPagePopupImpl.cpp:492: cancel();
Is it safe to call cancel() unconditionally?
In the PopupMenuImpl::setValueAndClosePopup path, we call
WebPagePopupImpl::closePopup, it calls then Chromium calls
WebPagePopupImpl::close().
With this CL, we call WebPagePopupImpl::closePopup again, and some steps are
executed again.

if (m_page)
    cancel();

is more sane?

[tkent, 2016-03-25 07:32:42]

On 2016/03/25 at 07:30:45, tkent wrote:
> In the PopupMenuImpl::setValueAndClosePopup path, we call
WebPagePopupImpl::closePopup, it calls then Chromium calls
WebPagePopupImpl::close().

it calls then -> it calls WebWidgetClient::closeWidgetSoon(), then

[keishi, 2016-03-25 07:57:26]

On 2016/03/25 07:32:42, tkent wrote:
> On 2016/03/25 at 07:30:45, tkent wrote:
> > In the PopupMenuImpl::setValueAndClosePopup path, we call
> WebPagePopupImpl::closePopup, it calls then Chromium calls
> WebPagePopupImpl::close().
> 
> it calls then -> it calls WebWidgetClient::closeWidgetSoon(), then

You're right. So WebPagePopupImpl::close() can be invoked from the Blink side
through WebPagePopupImpl::closePopup, or called directly from a RenderWidget.

[keishi, 2016-03-25 07:57:46]

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

https://codereview.chromium.org/1833843002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebPagePopupImpl.cpp:492: cancel();
On 2016/03/25 07:30:44, tkent wrote:
> Is it safe to call cancel() unconditionally?
> In the PopupMenuImpl::setValueAndClosePopup path, we call
> WebPagePopupImpl::closePopup, it calls then Chromium calls
> WebPagePopupImpl::close().
> With this CL, we call WebPagePopupImpl::closePopup again, and some steps are
> executed again.
> 
> if (m_page)
>     cancel();
> 
> is more sane?
> 

Done.

[tkent, 2016-03-25 08:06:51]

lgtm

https://codereview.chromium.org/1833843002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebPagePopupImpl.cpp (right):

https://codereview.chromium.org/1833843002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebPagePopupImpl.cpp:494: cancel();
I'm not sure if calling closeWidgetSoon() inside close() is safe.

m_widgetClient = nullptr;
if (m_page)
    cancel();

might be safer.

[keishi, 2016-03-25 10:34:36]

https://codereview.chromium.org/1833843002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebPagePopupImpl.cpp (right):

https://codereview.chromium.org/1833843002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebPagePopupImpl.cpp:494: cancel();
On 2016/03/25 08:06:51, tkent wrote:
> I'm not sure if calling closeWidgetSoon() inside close() is safe.
> 
> m_widgetClient = nullptr;
> if (m_page)
>     cancel();
> 
> might be safer.

Done.

[keishi, 2016-03-25 10:34:41]

The CQ bit was checked by keishi@chromium.org

[keishi, 2016-03-25 10:34:42]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/1833843002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-03-25 10:34:52]

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

[commit-bot: I haz the power, 2016-03-25 11:39:13]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-25 11:40:21]

Description was changed from

==========
Popup loosing focus through keyboard events should update HTMLSelectElement
display

Ctrl-N while a popup menu is open, causes the window to loose focus, and
ViewMsg_Close is sent to the popup widget.
This causes WebPagePopupImpl::close to be called.
Right now this causes the popup to close without updating the display from the
provisional selection (ie HTMLSelectElement::indexToSelectOnCancel) to the
actual selection.
WebPagePopupImpl::close should use WebPagePopupImpl::cancel to handle that case
properly.

Loosing focus through mouse clicks were working because we manually call cancel
from the mouse event handler.

Regression caused by r193034

BUG=597206
==========

to

==========
Popup loosing focus through keyboard events should update HTMLSelectElement
display

Ctrl-N while a popup menu is open, causes the window to loose focus, and
ViewMsg_Close is sent to the popup widget.
This causes WebPagePopupImpl::close to be called.
Right now this causes the popup to close without updating the display from the
provisional selection (ie HTMLSelectElement::indexToSelectOnCancel) to the
actual selection.
WebPagePopupImpl::close should use WebPagePopupImpl::cancel to handle that case
properly.

Loosing focus through mouse clicks were working because we manually call cancel
from the mouse event handler.

Regression caused by r193034

BUG=597206

Committed: https://crrev.com/831776ec4a98f781dc8ce20bb3346538fd59b8da
Cr-Commit-Position: refs/heads/master@{#383271}
==========

[commit-bot: I haz the power, 2016-03-25 11:40:22]

Patchset 3 (id:??) landed as
https://crrev.com/831776ec4a98f781dc8ce20bb3346538fd59b8da
Cr-Commit-Position: refs/heads/master@{#383271}

[tkent, 2016-03-28 23:46:27]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1841623002/ by tkent@chromium.org.

The reason for reverting is: Caused crashes; crbug.com/598291
.

[keishi, 2016-03-29 04:25:16]

Changed back the position to clear m_widgetClient

[tkent, 2016-03-29 05:10:09]

On 2016/03/29 at 04:25:16, keishi wrote:
> Changed back the position to clear m_widgetClient

Did you confirm that calling closeWidgetSoon() in close() is ok?

[keishi, 2016-03-29 06:09:04]

On 2016/03/29 05:10:09, tkent wrote:
> On 2016/03/29 at 04:25:16, keishi wrote:
> > Changed back the position to clear m_widgetClient
> 
> Did you confirm that calling closeWidgetSoon() in close() is ok?

There are checks like RenderWidget::closing_ so I didn't see anything bad
happen, but you're right.
Added a check before calling closeWidgetSoon.

[tkent, 2016-03-29 06:14:32]

lgtm

[keishi, 2016-03-29 07:08:31]

Description was changed from

==========
Popup loosing focus through keyboard events should update HTMLSelectElement
display

Ctrl-N while a popup menu is open, causes the window to loose focus, and
ViewMsg_Close is sent to the popup widget.
This causes WebPagePopupImpl::close to be called.
Right now this causes the popup to close without updating the display from the
provisional selection (ie HTMLSelectElement::indexToSelectOnCancel) to the
actual selection.
WebPagePopupImpl::close should use WebPagePopupImpl::cancel to handle that case
properly.

Loosing focus through mouse clicks were working because we manually call cancel
from the mouse event handler.

Regression caused by r193034

BUG=597206

Committed: https://crrev.com/831776ec4a98f781dc8ce20bb3346538fd59b8da
Cr-Commit-Position: refs/heads/master@{#383271}
==========

to

==========
Popup loosing focus through keyboard events should update HTMLSelectElement
display

Ctrl-N while a popup menu is open, causes the window to loose focus, and
ViewMsg_Close is sent to the popup widget.
This causes WebPagePopupImpl::close to be called.
Right now this causes the popup to close without updating the display from the
provisional selection (ie HTMLSelectElement::indexToSelectOnCancel) to the
actual selection.
WebPagePopupImpl::close should use WebPagePopupImpl::cancel to handle that case
properly.

Loosing focus through mouse clicks were working because we manually call cancel
from the mouse event handler.

Regression caused by r193034

BUG=597206

Committed: https://crrev.com/831776ec4a98f781dc8ce20bb3346538fd59b8da
Cr-Commit-Position: refs/heads/master@{#383271}
==========

[keishi, 2016-03-29 07:08:35]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2016-03-29 07:08:49]

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

[commit-bot: I haz the power, 2016-03-29 09:17:46]

Description was changed from

==========
Popup loosing focus through keyboard events should update HTMLSelectElement
display

Ctrl-N while a popup menu is open, causes the window to loose focus, and
ViewMsg_Close is sent to the popup widget.
This causes WebPagePopupImpl::close to be called.
Right now this causes the popup to close without updating the display from the
provisional selection (ie HTMLSelectElement::indexToSelectOnCancel) to the
actual selection.
WebPagePopupImpl::close should use WebPagePopupImpl::cancel to handle that case
properly.

Loosing focus through mouse clicks were working because we manually call cancel
from the mouse event handler.

Regression caused by r193034

BUG=597206

Committed: https://crrev.com/831776ec4a98f781dc8ce20bb3346538fd59b8da
Cr-Commit-Position: refs/heads/master@{#383271}
==========

to

==========
Popup loosing focus through keyboard events should update HTMLSelectElement
display

Ctrl-N while a popup menu is open, causes the window to loose focus, and
ViewMsg_Close is sent to the popup widget.
This causes WebPagePopupImpl::close to be called.
Right now this causes the popup to close without updating the display from the
provisional selection (ie HTMLSelectElement::indexToSelectOnCancel) to the
actual selection.
WebPagePopupImpl::close should use WebPagePopupImpl::cancel to handle that case
properly.

Loosing focus through mouse clicks were working because we manually call cancel
from the mouse event handler.

Regression caused by r193034

BUG=597206

Committed: https://crrev.com/831776ec4a98f781dc8ce20bb3346538fd59b8da
Cr-Commit-Position: refs/heads/master@{#383271}
==========

[commit-bot: I haz the power, 2016-03-29 09:17:48]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-29 09:19:34]

Description was changed from

==========
Popup loosing focus through keyboard events should update HTMLSelectElement
display

Ctrl-N while a popup menu is open, causes the window to loose focus, and
ViewMsg_Close is sent to the popup widget.
This causes WebPagePopupImpl::close to be called.
Right now this causes the popup to close without updating the display from the
provisional selection (ie HTMLSelectElement::indexToSelectOnCancel) to the
actual selection.
WebPagePopupImpl::close should use WebPagePopupImpl::cancel to handle that case
properly.

Loosing focus through mouse clicks were working because we manually call cancel
from the mouse event handler.

Regression caused by r193034

BUG=597206

Committed: https://crrev.com/831776ec4a98f781dc8ce20bb3346538fd59b8da
Cr-Commit-Position: refs/heads/master@{#383271}
==========

to

==========
Popup loosing focus through keyboard events should update HTMLSelectElement
display

Ctrl-N while a popup menu is open, causes the window to loose focus, and
ViewMsg_Close is sent to the popup widget.
This causes WebPagePopupImpl::close to be called.
Right now this causes the popup to close without updating the display from the
provisional selection (ie HTMLSelectElement::indexToSelectOnCancel) to the
actual selection.
WebPagePopupImpl::close should use WebPagePopupImpl::cancel to handle that case
properly.

Loosing focus through mouse clicks were working because we manually call cancel
from the mouse event handler.

Regression caused by r193034

BUG=597206

Committed: https://crrev.com/831776ec4a98f781dc8ce20bb3346538fd59b8da
Cr-Commit-Position: refs/heads/master@{#383271}

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

[commit-bot: I haz the power, 2016-03-29 09:19:35]

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