Remove ReadOnly flag when mouseleave with touch

Remove ReadOnly flag when mouseleave with touch

In this patch, we remove ReadOnly flag when mouseleave with touch in 
EventHandler::handleMouseMoveOrLeaveEvent since we want to 
updateHoverActiveState without hit test for this case.

BUG=678363
Review-Url: https://codereview.chromium.org/2734153003
Cr-Commit-Position: refs/heads/master@{#455647}
Committed: https://chromium.googlesource.com/chromium/src/+/7a06624223475946620e19af8322f7cf4ebd8dc1

[chaopeng, 2017-03-07 16:07:57]

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

[commit-bot: I haz the power, 2017-03-07 16:08:46]

Dry run: 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, 2017-03-07 17:59:14]

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

[commit-bot: I haz the power, 2017-03-07 17:59:15]

Dry run: This issue passed the CQ dry run.

[chaopeng, 2017-03-08 15:22:45]

Description was changed from

==========
Make Touch and MouseLeave go to Mouse Move code path

BUG=678363
==========

to

==========
Make Touch and MouseLeave go to Mouse Move code path

BUG=678363
==========

[chaopeng, 2017-03-08 15:22:46]

chaopeng@chromium.org changed reviewers:
+ mustaq@chromium.org

[chaopeng, 2017-03-08 17:02:43]

Description was changed from

==========
Make Touch and MouseLeave go to Mouse Move code path

BUG=678363
==========

to

==========
Remove DCHECK(!request.readOnly()) in updateHoverActiveState

We can drop DCHECK(!request.readOnly()) in Document::updateHoverActiveState
since we do not do any hit test in updateHoverActiveState.

BUG=678363
==========

[chaopeng, 2017-03-08 17:03:07]

PTAL. Thank you.

[mustaq, 2017-03-08 18:11:15]

lgtm

[dtapuska, 2017-03-08 18:16:04]

On 2017/03/08 18:11:15, mustaq wrote:
> lgtm

I don't think this is the correct approach. Without the DCHECK you might as well
just remove the ReadOnly flag.

[chaopeng, 2017-03-08 18:40:44]

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

[commit-bot: I haz the power, 2017-03-08 18:41:19]

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

[chaopeng, 2017-03-08 18:42:48]

On 2017/03/08 18:16:04, dtapuska wrote:
> On 2017/03/08 18:11:15, mustaq wrote:
> > lgtm
> 
> I don't think this is the correct approach. Without the DCHECK you might as
well
> just remove the ReadOnly flag.

Remove ReadOnly flag might be better for this issue, it only effects
mouseleave+touch code path. Updated, PTAL. Thank you.

[chaopeng, 2017-03-08 18:43:11]

chaopeng@chromium.org changed reviewers:
+ dtapuska@chromium.org

[mustaq, 2017-03-08 18:49:52]

On 2017/03/08 18:42:48, chaopeng wrote:
> On 2017/03/08 18:16:04, dtapuska wrote:
> > On 2017/03/08 18:11:15, mustaq wrote:
> > > lgtm
> > 
> > I don't think this is the correct approach. Without the DCHECK you might as
> well
> > just remove the ReadOnly flag.
> 
> Remove ReadOnly flag might be better for this issue, it only effects
> mouseleave+touch code path. Updated, PTAL. Thank you.

I think this is better since this would also fix the hover problem with
touch-drag-leave case you showed me. Thanks for the suggestion, Dave.

I still think the |request| parameter is a misuse of HitTestRequest in
updateHoverActiveState() since the function doesn't bother with hit-testing.
This
caused dummy hittest-like code as in EventHandler::activeIntervalTimerFired.

Please add a TODO in updateHoverActiveState to replace the param.

[mustaq, 2017-03-08 18:50:53]

On 2017/03/08 18:49:52, mustaq wrote:
> On 2017/03/08 18:42:48, chaopeng wrote:
> > On 2017/03/08 18:16:04, dtapuska wrote:
> > > On 2017/03/08 18:11:15, mustaq wrote:
> > > > lgtm
> > > 
> > > I don't think this is the correct approach. Without the DCHECK you might
as
> > well
> > > just remove the ReadOnly flag.
> > 
> > Remove ReadOnly flag might be better for this issue, it only effects
> > mouseleave+touch code path. Updated, PTAL. Thank you.
> 
> I think this is better since this would also fix the hover problem with
> touch-drag-leave case you showed me. Thanks for the suggestion, Dave.
> 
> I still think the |request| parameter is a misuse of HitTestRequest in
> updateHoverActiveState() since the function doesn't bother with hit-testing.
> This
> caused dummy hittest-like code as in EventHandler::activeIntervalTimerFired.
> 
> Please add a TODO in updateHoverActiveState to replace the param.

Also need to update the CL description.

[commit-bot: I haz the power, 2017-03-08 21:06:19]

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

[commit-bot: I haz the power, 2017-03-08 21:06:21]

Dry run: This issue passed the CQ dry run.

[dtapuska, 2017-03-08 21:10:34]

On 2017/03/08 18:50:53, mustaq wrote:
> On 2017/03/08 18:49:52, mustaq wrote:
> > On 2017/03/08 18:42:48, chaopeng wrote:
> > > On 2017/03/08 18:16:04, dtapuska wrote:
> > > > On 2017/03/08 18:11:15, mustaq wrote:
> > > > > lgtm
> > > > 
> > > > I don't think this is the correct approach. Without the DCHECK you might
> as
> > > well
> > > > just remove the ReadOnly flag.
> > > 
> > > Remove ReadOnly flag might be better for this issue, it only effects
> > > mouseleave+touch code path. Updated, PTAL. Thank you.
> > 
> > I think this is better since this would also fix the hover problem with
> > touch-drag-leave case you showed me. Thanks for the suggestion, Dave.
> > 
> > I still think the |request| parameter is a misuse of HitTestRequest in
> > updateHoverActiveState() since the function doesn't bother with hit-testing.
> > This
> > caused dummy hittest-like code as in EventHandler::activeIntervalTimerFired.
> > 
> > Please add a TODO in updateHoverActiveState to replace the param.
> 
> Also need to update the CL description.

lgtm % CL description

[chaopeng, 2017-03-08 21:41:47]

Description was changed from

==========
Remove DCHECK(!request.readOnly()) in updateHoverActiveState

We can drop DCHECK(!request.readOnly()) in Document::updateHoverActiveState
since we do not do any hit test in updateHoverActiveState.

BUG=678363
==========

to

==========
Remove ReadOnly flag when mouseleave with touch

In this patch, we remove ReadOnly flag when mouseleave with touch in 
EventHandler::handleMouseMoveOrLeaveEvent since we want to
updateHoverActiveState without hit test for this case.

BUG=678363
==========

[chaopeng, 2017-03-08 21:44:48]

Description was changed from

==========
Remove ReadOnly flag when mouseleave with touch

In this patch, we remove ReadOnly flag when mouseleave with touch in 
EventHandler::handleMouseMoveOrLeaveEvent since we want to
updateHoverActiveState without hit test for this case.

BUG=678363
==========

to

==========
Remove ReadOnly flag when mouseleave with touch

In this patch, we remove ReadOnly flag when mouseleave with touch in 
EventHandler::handleMouseMoveOrLeaveEvent since we want to 
updateHoverActiveState without hit test for this case.

BUG=678363
==========

[chaopeng, 2017-03-08 21:53:38]

chaopeng@chromium.org changed reviewers:
+ rbyers@chromium.org

[chaopeng, 2017-03-08 21:54:42]

rbyers@ PTAL. Thank you.

[dtapuska, 2017-03-08 22:15:03]

On 2017/03/08 21:54:42, chaopeng wrote:
> rbyers@ PTAL. Thank you.

you should probably try bokan@

[chaopeng, 2017-03-08 22:46:31]

chaopeng@chromium.org changed reviewers:
+ bokan@chromium.org
- rbyers@chromium.org

[chaopeng, 2017-03-08 22:46:55]

bokan@ PTAL thank you

[Rick Byers, 2017-03-09 00:23:25]

On 2017/03/08 22:46:55, chaopeng wrote:
> bokan@ PTAL thank you

RS LGTM - mustaq and dtapuska are experts here.  (but thanks for the redirection
- I'm perfcrastinating by doing code reviews <grin>).

[chaopeng, 2017-03-09 00:36:30]

The CQ bit was checked by chaopeng@chromium.org

[chaopeng, 2017-03-09 00:36:31]

The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org,
dtapuska@chromium.org
Link to the patchset: https://codereview.chromium.org/2734153003/#ps60001
(title: "add todo")

[commit-bot: I haz the power, 2017-03-09 00:37:04]

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, 2017-03-09 02:42:59]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489019790920160,
"parent_rev": "f0f14b715fc62c3e8d9888970ebda21ea7a6e399", "commit_rev":
"7a06624223475946620e19af8322f7cf4ebd8dc1"}

[commit-bot: I haz the power, 2017-03-09 02:43:38]

Description was changed from

==========
Remove ReadOnly flag when mouseleave with touch

In this patch, we remove ReadOnly flag when mouseleave with touch in 
EventHandler::handleMouseMoveOrLeaveEvent since we want to 
updateHoverActiveState without hit test for this case.

BUG=678363
==========

to

==========
Remove ReadOnly flag when mouseleave with touch

In this patch, we remove ReadOnly flag when mouseleave with touch in 
EventHandler::handleMouseMoveOrLeaveEvent since we want to 
updateHoverActiveState without hit test for this case.

BUG=678363

Review-Url: https://codereview.chromium.org/2734153003
Cr-Commit-Position: refs/heads/master@{#455647}
Committed:
https://chromium.googlesource.com/chromium/src/+/7a06624223475946620e19af8322...
==========

[commit-bot: I haz the power, 2017-03-09 02:43:40]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/7a06624223475946620e19af8322...

[bokan, 2017-03-09 02:56:49]

Sorry for the delay - was busy perfing. FYI for the future, if you're just
looking for a rubber stamp on an already l-g-t-m'd patch then feel free to ping
me over IM.