Move persistent gesture state to Document, add DocumentUserGestureToken

Move persistent gesture state to Document, add DocumentUserGestureToken

Non-test gesture tokens are now created as a DocumentUserGestureToken, which take a Document* (which can be null, but such uses are limited). When it is created, it sets the m_hasReceivedUserGesture flag on Document.

Remove UserGestureIndicator::s_processedUserGestureSinceLoad, as Document::m_hasReceivedUserGesture is very similar and not process-wide.

BUG=624061
Committed: https://crrev.com/26ce312cc8b3f53dc76df85e45c17bba93685fc3
Cr-Commit-Position: refs/heads/master@{#427481}

[Nate Chapin, 2016-10-12 21:14:43]

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

[commit-bot: I haz the power, 2016-10-12 21:15:13]

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, 2016-10-12 22:34:21]

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

[commit-bot: I haz the power, 2016-10-12 22:34:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nate Chapin, 2016-10-12 23:10:13]

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

[commit-bot: I haz the power, 2016-10-12 23:10:33]

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, 2016-10-12 23:24:05]

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

[commit-bot: I haz the power, 2016-10-12 23:24:06]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nate Chapin, 2016-10-12 23:46:47]

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

[commit-bot: I haz the power, 2016-10-12 23:47:10]

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, 2016-10-12 23:54:02]

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

[commit-bot: I haz the power, 2016-10-12 23:54:03]

Dry run: Exceeded global retry quota

[Nate Chapin, 2016-10-13 17:49:42]

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

[commit-bot: I haz the power, 2016-10-13 17:50:12]

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, 2016-10-13 19:36:47]

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

[commit-bot: I haz the power, 2016-10-13 19:36:48]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2016-10-13 19:51:18]

Description was changed from

==========
Move persistent gesture state to Document, add DocumentUserGestureToken

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

to

==========
Move persistent gesture state to Document, add DocumentUserGestureToken

Non-test gesture tokens are now created as a DocumentUserGestureToken, which
take a Document* (which can be null, but such uses are limited). When it is
created, it sets the m_hasReceivedUserGesture flag on Document.

Remove UserGestureIndicator::s_processedUserGestureSinceLoad, as
Document::m_hasReceivedUserGesture is very similar and not process-wide.
==========

[Nate Chapin, 2016-10-13 20:03:18]

japhet@chromium.org changed reviewers:
+ jochen@chromium.org, rbyers@chromium.org

[Nate Chapin, 2016-10-13 20:03:19]

This is another lead in to https://codereview.chromium.org/2392773002. I'm not
super confident I got the right Document for every callsite, I'd appreciate some
extra attention to that.

With this patch, https://codereview.chromium.org/2392773002 should be reducible
to the Frame.cpp change and test changes.

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/UserGestureIndicatorTest.cpp (left):

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicatorTest.cpp:14:
EXPECT_FALSE(UserGestureIndicator::processedUserGestureSinceLoad());
I don't love the loss of test coverage for this, but this flag is now a lot
simpler, so idk.

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (left):

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:3878: UserGestureIndicator
gesture(
Moved to DragController.

[Rick Byers, 2016-10-17 15:35:53]

Still reviewing the details, but a couple high-level comments:

There's some risk of breaking changes here - eg. focus() calls that used to
bring up the OSK due to an interaction with another document on the page.  I
_think_ the risk is relatively low, but I'm not familiar with the other
'hasProcessedGestureSinceLoad' use cases.  Have you looked at the other use
cases?  Of course if we think the risk is low then it's probably best to just
give it a try and see if we get any reports of regressions rather than invest a
ton in trying to quantify the risk.

When a user interacts with an iframe, should the ancestor documents perhaps also
be marked as having had a user gesture?  Conceptually I think parent documents
are always strictly more privileged than their children (eg. they can "steal"
events for child documents via a transparent overlay).  Maybe you could strictly
improve the user/developer tradeoff of this approach by propagating up?

[Rick Byers, 2016-10-17 15:43:25]

Description was changed from

==========
Move persistent gesture state to Document, add DocumentUserGestureToken

Non-test gesture tokens are now created as a DocumentUserGestureToken, which
take a Document* (which can be null, but such uses are limited). When it is
created, it sets the m_hasReceivedUserGesture flag on Document.

Remove UserGestureIndicator::s_processedUserGestureSinceLoad, as
Document::m_hasReceivedUserGesture is very similar and not process-wide.
==========

to

==========
Move persistent gesture state to Document, add DocumentUserGestureToken

Non-test gesture tokens are now created as a DocumentUserGestureToken, which
take a Document* (which can be null, but such uses are limited). When it is
created, it sets the m_hasReceivedUserGesture flag on Document.

Remove UserGestureIndicator::s_processedUserGestureSinceLoad, as
Document::m_hasReceivedUserGesture is very similar and not process-wide.
==========

[Rick Byers, 2016-10-17 17:02:47]

The overall approach here seems really good (much better than the previous
attempt at a hack), thank you for being thorough!  Just a few mostly minor
comments.

Reference BUG=624061 in the CL description?

Since this is a pretty big CL, if you can keep rebase patchsets separate from
additional changes that'll make follow-up reviews a lot faster.

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

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.h:1287: void
setHasReceivedUserGesture() { m_hasReceivedUserGesture = true; }
Nit: add a short comment describing why this is meaningful (eg. indicates the
user has done something to interact with this document in particular).

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.h:1288: void
clearHasReceivedUserGesture() { m_hasReceivedUserGesture = false; }
I'm surprised you ever need to clear (I'm sure I'll see where it's being used
soon).  Maybe worth a comment explaining when it makes sense to invoke this?

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h (right):

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h:2: * Copyright (C)
2010 Apple Inc. All rights reserved.
New files should get the new license:
https://www.chromium.org/blink/coding-style#TOC-License

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h:34: class
DocumentUserGestureToken final : public UserGestureToken {
Add a short comment describing the purpose of the class.

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

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:942:
std::unique_ptr<UserGestureIndicator> gestureIndicator;
So the effect of the mouse changes is that it's the document where the mousedown
occurred that gets associated with the user gesture.  If you mousedown in one
document, drag to another and mouseup there, the mouseup still has a gesture but
that document doesn't get "hasReceivedUserGesture".  Right?

I can't think of any reason why that's likely to be a problem, but it's probably
worth calling out in a comment since it's something that could cause confusion,
or something we want to think more deeply about in the future (eg. as part of
standardizing this behavior).

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

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:176:
DocumentUserGestureToken::create(m_frame->document())));
I'm not positive what m_frame refers to here in all cases. Navid would it be
better to use target's document instead of m_frame to ensure we're getting the
inner-most document?

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/UserGestureIndicatorTest.cpp (left):

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicatorTest.cpp:14:
EXPECT_FALSE(UserGestureIndicator::processedUserGestureSinceLoad());
On 2016/10/13 20:03:19, Nate Chapin wrote:
> I don't love the loss of test coverage for this, but this flag is now a lot
> simpler, so idk.

Can you replace it with some test coverage for DocumentUserGestureToken?  It's
pretty trivial so shouldn't need much I don't think, still probably worth having
something...

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:285:
m_frame->autofillClient()->firstUserGestureObserved();
Is there a notifier per frame?  You should probably get someone familiar with
the autofill use cases to confirm that this change in semantics (from page to
main frame) is OK and won't break iframe form-filling scenarios.

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebUserGestureIndicator.h (right):

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebUserGestureIndicator.h:51: // time the page was
loaded.
Update the comment to describe the new semantics (no longer per-page).

[Navid Zolghadr, 2016-10-17 17:18:04]

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

[Navid Zolghadr, 2016-10-17 17:18:06]

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

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:176:
DocumentUserGestureToken::create(m_frame->document())));
On 2016/10/17 17:02:46, Rick Byers wrote:
> I'm not positive what m_frame refers to here in all cases. Navid would it be
> better to use target's document instead of m_frame to ensure we're getting the
> inner-most document?

Unfortunately that is the case for touch events. m_frame is the frame that this
PointerEventManager belongs to. Every iframe has its own EventHandler and
PointerEventManager. However all the touch events and touch pointer events are
being sent directly to the target node inside the innermost iframe right from
the top level frame and PointerEventManager. So if the pointerType is touch
m_frame refers to the main page frame. So if we would like to create the gesture
for the inner most iframe it is better to use iframe that the target is in. I
guess we need to cast to to Node and get the document.

[ojan, 2016-10-18 16:39:48]

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

[ojan, 2016-10-18 16:39:50]

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:787:
!m_frame->document()->hasReceivedUserGesture() && initiatingDocument)
What happens in this case?
1. Load a top-level page.
2. Click in an iframe.
3. The iframe gets navigated to a new URL at some point.
4. Navigate the top-level to a new URL.

I would expect to get at least one history entry (after #3), and maybe two (also
after #4)?

I think the safest thing would be having two entries as that matches what we
would do today, right?

I'm picturing a use case where someone is building a web browser type thing
where you click on links in the outer page to navigate the sub-frame, or you
click on links in one sub-frame to navigate another sub-frame. I don't know if
that case matters, but my gut feeling is that it makes sense to be a bit
conservative here.

In either case, we might want a test that covers this case.

[Nate Chapin, 2016-10-18 20:52:13]

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

[commit-bot: I haz the power, 2016-10-18 20:52:45]

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, 2016-10-18 23:14:46]

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

[commit-bot: I haz the power, 2016-10-18 23:14:47]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Nate Chapin, 2016-10-18 23:26:31]

japhet@chromium.org changed reviewers:
+ mathp@chromium.org

[Nate Chapin, 2016-10-18 23:26:33]

+mathp for the autofill question. See the comments in WebViewImpl.cpp

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

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.h:1287: void
setHasReceivedUserGesture() { m_hasReceivedUserGesture = true; }
On 2016/10/17 17:02:46, Rick Byers wrote:
> Nit: add a short comment describing why this is meaningful (eg. indicates the
> user has done something to interact with this document in particular).

Done.

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.h:1288: void
clearHasReceivedUserGesture() { m_hasReceivedUserGesture = false; }
On 2016/10/17 17:02:46, Rick Byers wrote:
> I'm surprised you ever need to clear (I'm sure I'll see where it's being used
> soon).  Maybe worth a comment explaining when it makes sense to invoke this?

Done.

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h (right):

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h:2: * Copyright (C)
2010 Apple Inc. All rights reserved.
On 2016/10/17 17:02:46, Rick Byers wrote:
> New files should get the new license:
> https://www.chromium.org/blink/coding-style#TOC-License

Done.

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h:34: class
DocumentUserGestureToken final : public UserGestureToken {
On 2016/10/17 17:02:46, Rick Byers wrote:
> Add a short comment describing the purpose of the class.

Done.

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

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:942:
std::unique_ptr<UserGestureIndicator> gestureIndicator;
On 2016/10/17 17:02:46, Rick Byers wrote:
> So the effect of the mouse changes is that it's the document where the
mousedown
> occurred that gets associated with the user gesture.  If you mousedown in one
> document, drag to another and mouseup there, the mouseup still has a gesture
but
> that document doesn't get "hasReceivedUserGesture".  Right?
> 
> I can't think of any reason why that's likely to be a problem, but it's
probably
> worth calling out in a comment since it's something that could cause
confusion,
> or something we want to think more deeply about in the future (eg. as part of
> standardizing this behavior).

Done, and explicitly noted that this behavior may be wrong :)

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

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:176:
DocumentUserGestureToken::create(m_frame->document())));
On 2016/10/17 17:18:05, Navid Zolghadr wrote:
> On 2016/10/17 17:02:46, Rick Byers wrote:
> > I'm not positive what m_frame refers to here in all cases. Navid would it be
> > better to use target's document instead of m_frame to ensure we're getting
the
> > inner-most document?
> 
> Unfortunately that is the case for touch events. m_frame is the frame that
this
> PointerEventManager belongs to. Every iframe has its own EventHandler and
> PointerEventManager. However all the touch events and touch pointer events are
> being sent directly to the target node inside the innermost iframe right from
> the top level frame and PointerEventManager. So if the pointerType is touch
> m_frame refers to the main page frame. So if we would like to create the
gesture
> for the inner most iframe it is better to use iframe that the target is in. I
> guess we need to cast to to Node and get the document.

Done. It's safe to assume that the EventTarget is a Node here, right?

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:787:
!m_frame->document()->hasReceivedUserGesture() && initiatingDocument)
On 2016/10/18 16:39:49, ojan wrote:
> What happens in this case?
> 1. Load a top-level page.
> 2. Click in an iframe.
> 3. The iframe gets navigated to a new URL at some point.
> 4. Navigate the top-level to a new URL.
> 
> I would expect to get at least one history entry (after #3), and maybe two
(also
> after #4)?
> 
> I think the safest thing would be having two entries as that matches what we
> would do today, right?
> 
> I'm picturing a use case where someone is building a web browser type thing
> where you click on links in the outer page to navigate the sub-frame, or you
> click on links in one sub-frame to navigate another sub-frame. I don't know if
> that case matters, but my gut feeling is that it makes sense to be a bit
> conservative here.
> 
> In either case, we might want a test that covers this case.

Hrm. This should probably be using initiatingDocument for hasReceivedUserGesture
checks. That should solve the problem of gestures in one frame driving
navigations in another. Updated.

In general, on the question of 1 or 2 history entries: (3) definitely gets a
user gesture. (4) depends on how the top-level navigation triggers: it gets a
history entry if there was or is a user gesture since last commit and/or history
entry addition. Also, if there is no Document* specified as the initiator, that
means that the navigation is being triggered from a trusted source (e.g., the
address bar) and a history entry may be added.

Does that make sense?

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/UserGestureIndicatorTest.cpp (left):

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicatorTest.cpp:14:
EXPECT_FALSE(UserGestureIndicator::processedUserGestureSinceLoad());
On 2016/10/17 17:02:47, Rick Byers wrote:
> On 2016/10/13 20:03:19, Nate Chapin wrote:
> > I don't love the loss of test coverage for this, but this flag is now a lot
> > simpler, so idk.
> 
> Can you replace it with some test coverage for DocumentUserGestureToken?  It's
> pretty trivial so shouldn't need much I don't think, still probably worth
having
> something...

Done.

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:285:
m_frame->autofillClient()->firstUserGestureObserved();
On 2016/10/17 17:02:47, Rick Byers wrote:
> Is there a notifier per frame?  You should probably get someone familiar with
> the autofill use cases to confirm that this change in semantics (from page to
> main frame) is OK and won't break iframe form-filling scenarios.

Yes, the notifier is per-frame, and UserGestureNotifier is only ever initialized
for the main frame. So the same autofill client will still be getting this
callback. The substantive change here is that only main frame user gestures will
trigger the firstUserGestureObserved() callback, where before any gesture in the
page (well, really the whole process) would trigger it.

@mathp, any idea whether that is OK?

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebUserGestureIndicator.h (right):

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebUserGestureIndicator.h:51: // time the page was
loaded.
On 2016/10/17 17:02:47, Rick Byers wrote:
> Update the comment to describe the new semantics (no longer per-page).

Done.

[ojan, 2016-10-18 23:40:25]

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:787:
!m_frame->document()->hasReceivedUserGesture() && initiatingDocument)
On 2016/10/18 at 23:26:33, Nate Chapin wrote:
> On 2016/10/18 16:39:49, ojan wrote:
> > What happens in this case?
> > 1. Load a top-level page.
> > 2. Click in an iframe.
> > 3. The iframe gets navigated to a new URL at some point.
> > 4. Navigate the top-level to a new URL.
> > 
> > I would expect to get at least one history entry (after #3), and maybe two
(also
> > after #4)?
> > 
> > I think the safest thing would be having two entries as that matches what we
> > would do today, right?
> > 
> > I'm picturing a use case where someone is building a web browser type thing
> > where you click on links in the outer page to navigate the sub-frame, or you
> > click on links in one sub-frame to navigate another sub-frame. I don't know
if
> > that case matters, but my gut feeling is that it makes sense to be a bit
> > conservative here.
> > 
> > In either case, we might want a test that covers this case.
> 
> Hrm. This should probably be using initiatingDocument for
hasReceivedUserGesture checks. That should solve the problem of gestures in one
frame driving navigations in another. Updated.
> 
> In general, on the question of 1 or 2 history entries: (3) definitely gets a
user gesture.

Right. And that means it gets a history entry?

> (4) depends on how the top-level navigation triggers: it gets a history entry
if there was or is a user gesture since last commit and/or history entry
addition.

Right. Assuming no user gesture between 3 and 4, then it won't get a history
entry, right? I think we take on some risk here for the use case I was
describing. I think we might want to store a bit on Page as to whether that Page
has ever had a gesture and use that bit. Or, we can try this and see what
breaks. :)

> Also, if there is no Document* specified as the initiator, that means that the
navigation is being triggered from a trusted source (e.g., the address bar) and
a history entry may be added.

I guess that seems reasonable.

> Does that make sense?

Yup. If it's not a lot of work, I think a test is worth it, but if it's a lot of
work, don't worry about it.

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:285:
m_frame->autofillClient()->firstUserGestureObserved();
On 2016/10/18 at 23:26:33, Nate Chapin wrote:
> On 2016/10/17 17:02:47, Rick Byers wrote:
> > Is there a notifier per frame?  You should probably get someone familiar
with
> > the autofill use cases to confirm that this change in semantics (from page
to
> > main frame) is OK and won't break iframe form-filling scenarios.
> 
> Yes, the notifier is per-frame, and UserGestureNotifier is only ever
initialized for the main frame. So the same autofill client will still be
getting this callback. The substantive change here is that only main frame user
gestures will trigger the firstUserGestureObserved() callback, where before any
gesture in the page (well, really the whole process) would trigger it.
> 
> @mathp, any idea whether that is OK?

I suspect it's not OK since a lot of times password fields, etc. are inside
login iframes.

You could just crawl all frames here, right? Since this is guarded with a bool,
we'd only ever crawl all frames once per page load, I think? Alternately, could
store a bit on Page as well, but that seems like overkill to me unless we have
common use cases.

[Peter Beverloo, 2016-10-19 16:36:46]

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

[Peter Beverloo, 2016-10-19 16:36:48]

Minor drive-by

https://codereview.chromium.org/2408333004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/2408333004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/Notification.cpp:225:
context->isDocument() ? toDocument(context) : nullptr,
Prefer DCHECK(context->isDocument()) for this code.

We don't use this function in non-document contexts, so best to be explicit
about it.

[Mathieu, 2016-10-19 17:20:31]

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:285:
m_frame->autofillClient()->firstUserGestureObserved();
On 2016/10/18 23:40:25, ojan wrote:
> On 2016/10/18 at 23:26:33, Nate Chapin wrote:
> > On 2016/10/17 17:02:47, Rick Byers wrote:
> > > Is there a notifier per frame?  You should probably get someone familiar
> with
> > > the autofill use cases to confirm that this change in semantics (from page
> to
> > > main frame) is OK and won't break iframe form-filling scenarios.
> > 
> > Yes, the notifier is per-frame, and UserGestureNotifier is only ever
> initialized for the main frame. So the same autofill client will still be
> getting this callback. The substantive change here is that only main frame
user
> gestures will trigger the firstUserGestureObserved() callback, where before
any
> gesture in the page (well, really the whole process) would trigger it.
> > 
> > @mathp, any idea whether that is OK?
> 
> I suspect it's not OK since a lot of times password fields, etc. are inside
> login iframes.
> 
> You could just crawl all frames here, right? Since this is guarded with a
bool,
> we'd only ever crawl all frames once per page load, I think? Alternately,
could
> store a bit on Page as well, but that seems like overkill to me unless we have
> common use cases.

Ojan is correct. More and more payments and password information is in iframes,
I'm pretty sure it would break some functionality.

[Rick Byers, 2016-10-19 18:01:31]

On 2016/10/19 17:20:31, Mathieu Perreault wrote:
>
https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/web/WebViewImpl.cpp:285:
> m_frame->autofillClient()->firstUserGestureObserved();
> On 2016/10/18 23:40:25, ojan wrote:
> > On 2016/10/18 at 23:26:33, Nate Chapin wrote:
> > > On 2016/10/17 17:02:47, Rick Byers wrote:
> > > > Is there a notifier per frame?  You should probably get someone familiar
> > with
> > > > the autofill use cases to confirm that this change in semantics (from
page
> > to
> > > > main frame) is OK and won't break iframe form-filling scenarios.
> > > 
> > > Yes, the notifier is per-frame, and UserGestureNotifier is only ever
> > initialized for the main frame. So the same autofill client will still be
> > getting this callback. The substantive change here is that only main frame
> user
> > gestures will trigger the firstUserGestureObserved() callback, where before
> any
> > gesture in the page (well, really the whole process) would trigger it.
> > > 
> > > @mathp, any idea whether that is OK?
> > 
> > I suspect it's not OK since a lot of times password fields, etc. are inside
> > login iframes.
> > 
> > You could just crawl all frames here, right? Since this is guarded with a
> bool,
> > we'd only ever crawl all frames once per page load, I think? Alternately,
> could
> > store a bit on Page as well, but that seems like overkill to me unless we
have
> > common use cases.
> 
> Ojan is correct. More and more payments and password information is in
iframes,
> I'm pretty sure it would break some functionality.

Would my suggestion from https://codereview.chromium.org/2408333004/#msg20
address the risk here?  I.e. whenever a gesture occurs in a frame, we also mark
all ancestors of that frame as having received a gesture?  Off the top of my
head I can't think of a likely case where that wouldn't be sufficient, or where
it would allow something which really should be blocked.

[Nate Chapin, 2016-10-21 17:20:07]

On 2016/10/19 18:01:31, Rick Byers wrote:
> On 2016/10/19 17:20:31, Mathieu Perreault wrote:
> >
>
https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/web/WebViewImpl.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/web/WebViewImpl.cpp:285:
> > m_frame->autofillClient()->firstUserGestureObserved();
> > On 2016/10/18 23:40:25, ojan wrote:
> > > On 2016/10/18 at 23:26:33, Nate Chapin wrote:
> > > > On 2016/10/17 17:02:47, Rick Byers wrote:
> > > > > Is there a notifier per frame?  You should probably get someone
familiar
> > > with
> > > > > the autofill use cases to confirm that this change in semantics (from
> page
> > > to
> > > > > main frame) is OK and won't break iframe form-filling scenarios.
> > > > 
> > > > Yes, the notifier is per-frame, and UserGestureNotifier is only ever
> > > initialized for the main frame. So the same autofill client will still be
> > > getting this callback. The substantive change here is that only main frame
> > user
> > > gestures will trigger the firstUserGestureObserved() callback, where
before
> > any
> > > gesture in the page (well, really the whole process) would trigger it.
> > > > 
> > > > @mathp, any idea whether that is OK?
> > > 
> > > I suspect it's not OK since a lot of times password fields, etc. are
inside
> > > login iframes.
> > > 
> > > You could just crawl all frames here, right? Since this is guarded with a
> > bool,
> > > we'd only ever crawl all frames once per page load, I think? Alternately,
> > could
> > > store a bit on Page as well, but that seems like overkill to me unless we
> have
> > > common use cases.
> > 
> > Ojan is correct. More and more payments and password information is in
> iframes,
> > I'm pretty sure it would break some functionality.
> 
> Would my suggestion from https://codereview.chromium.org/2408333004/#msg20
> address the risk here?  I.e. whenever a gesture occurs in a frame, we also
mark
> all ancestors of that frame as having received a gesture?  Off the top of my
> head I can't think of a likely case where that wouldn't be sufficient, or
where
> it would allow something which really should be blocked.

I'm inclined to go this route. I agree that it shouldn't allow something we want
to block, and it's a clean way to solve the autofill concern as well.

[Nate Chapin, 2016-10-21 18:10:18]

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

[commit-bot: I haz the power, 2016-10-21 18:10:54]

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

[Nate Chapin, 2016-10-21 18:11:04]

https://codereview.chromium.org/2408333004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/2408333004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/Notification.cpp:225:
context->isDocument() ? toDocument(context) : nullptr,
On 2016/10/19 16:36:48, Peter Beverloo wrote:
> Prefer DCHECK(context->isDocument()) for this code.
> 
> We don't use this function in non-document contexts, so best to be explicit
> about it.

Done.

https://codereview.chromium.org/2408333004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2408333004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:398:
m_frame->document()->clearHasReceivedUserGesture();
I moved this to updateForSameDocumentNavigation(). It was here so it would catch
both same and different document navigations, but if there's a new Document, we
don't need to reset state.

[commit-bot: I haz the power, 2016-10-21 20:17:24]

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

[commit-bot: I haz the power, 2016-10-21 20:17:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nate Chapin, 2016-10-21 20:43:28]

https://codereview.chromium.org/2408333004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/2408333004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/Notification.cpp:225:
context->isDocument() ? toDocument(context) : nullptr,
On 2016/10/21 18:11:04, Nate Chapin wrote:
> On 2016/10/19 16:36:48, Peter Beverloo wrote:
> > Prefer DCHECK(context->isDocument()) for this code.
> > 
> > We don't use this function in non-document contexts, so best to be explicit
> > about it.
> 
> Done.

...actually, the layout tests disagree. Switched back to checking isDocument().

[Nate Chapin, 2016-10-21 20:43:47]

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

[commit-bot: I haz the power, 2016-10-21 20:44:31]

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

[ojan, 2016-10-21 21:33:07]

https://codereview.chromium.org/2408333004/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h (right):

https://codereview.chromium.org/2408333004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h:31: if (document)
This can early return if the document->hasReceivedUserGesture(), right? No need
to walk up the ancestor chain on every keypress. :)

[Nate Chapin, 2016-10-21 21:36:49]

https://codereview.chromium.org/2408333004/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h (right):

https://codereview.chromium.org/2408333004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h:31: if (document)
On 2016/10/21 21:33:06, ojan wrote:
> This can early return if the document->hasReceivedUserGesture(), right? No
need
> to walk up the ancestor chain on every keypress. :)

Technically it's not guaranteed that, if a Document has its user gesture flag
set, that all ancestors will have it set, because the flag can be cleared by the
history-entry-requires-user-gesture experiment. If the experiment is removed,
then I agree, that optimization makes sense.

[commit-bot: I haz the power, 2016-10-22 00:55:06]

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

[commit-bot: I haz the power, 2016-10-22 00:55:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Rick Byers, 2016-10-24 16:55:23]

Description was changed from

==========
Move persistent gesture state to Document, add DocumentUserGestureToken

Non-test gesture tokens are now created as a DocumentUserGestureToken, which
take a Document* (which can be null, but such uses are limited). When it is
created, it sets the m_hasReceivedUserGesture flag on Document.

Remove UserGestureIndicator::s_processedUserGestureSinceLoad, as
Document::m_hasReceivedUserGesture is very similar and not process-wide.
==========

to

==========
Move persistent gesture state to Document, add DocumentUserGestureToken

Non-test gesture tokens are now created as a DocumentUserGestureToken, which
take a Document* (which can be null, but such uses are limited). When it is
created, it sets the m_hasReceivedUserGesture flag on Document.

Remove UserGestureIndicator::s_processedUserGestureSinceLoad, as
Document::m_hasReceivedUserGesture is very similar and not process-wide.

BUG=624061
==========

[Rick Byers, 2016-10-24 16:56:16]

On 2016/10/17 17:02:47, Rick Byers wrote:
> The overall approach here seems really good (much better than the previous
> attempt at a hack), thank you for being thorough!  Just a few mostly minor
> comments.
> 
> Reference BUG=624061 in the CL description?

FYI, I updated your CL description with this.

> Since this is a pretty big CL, if you can keep rebase patchsets separate from
> additional changes that'll make follow-up reviews a lot faster.
> 
>
https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Document.h (right):
> 
>
https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Document.h:1287: void
> setHasReceivedUserGesture() { m_hasReceivedUserGesture = true; }
> Nit: add a short comment describing why this is meaningful (eg. indicates the
> user has done something to interact with this document in particular).
> 
>
https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Document.h:1288: void
> clearHasReceivedUserGesture() { m_hasReceivedUserGesture = false; }
> I'm surprised you ever need to clear (I'm sure I'll see where it's being used
> soon).  Maybe worth a comment explaining when it makes sense to invoke this?
> 
>
https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h (right):
> 
>
https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h:2: * Copyright
(C)
> 2010 Apple Inc. All rights reserved.
> New files should get the new license:
> https://www.chromium.org/blink/coding-style#TOC-License
> 
>
https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h:34: class
> DocumentUserGestureToken final : public UserGestureToken {
> Add a short comment describing the purpose of the class.
> 
>
https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/input/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/input/EventHandler.cpp:942:
> std::unique_ptr<UserGestureIndicator> gestureIndicator;
> So the effect of the mouse changes is that it's the document where the
mousedown
> occurred that gets associated with the user gesture.  If you mousedown in one
> document, drag to another and mouseup there, the mouseup still has a gesture
but
> that document doesn't get "hasReceivedUserGesture".  Right?
> 
> I can't think of any reason why that's likely to be a problem, but it's
probably
> worth calling out in a comment since it's something that could cause
confusion,
> or something we want to think more deeply about in the future (eg. as part of
> standardizing this behavior).
> 
>
https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):
> 
>
https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/input/PointerEventManager.cpp:176:
> DocumentUserGestureToken::create(m_frame->document())));
> I'm not positive what m_frame refers to here in all cases. Navid would it be
> better to use target's document instead of m_frame to ensure we're getting the
> inner-most document?
> 
>
https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/UserGestureIndicatorTest.cpp (left):
> 
>
https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/UserGestureIndicatorTest.cpp:14:
> EXPECT_FALSE(UserGestureIndicator::processedUserGestureSinceLoad());
> On 2016/10/13 20:03:19, Nate Chapin wrote:
> > I don't love the loss of test coverage for this, but this flag is now a lot
> > simpler, so idk.
> 
> Can you replace it with some test coverage for DocumentUserGestureToken?  It's
> pretty trivial so shouldn't need much I don't think, still probably worth
having
> something...
> 
>
https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/web/WebViewImpl.cpp:285:
> m_frame->autofillClient()->firstUserGestureObserved();
> Is there a notifier per frame?  You should probably get someone familiar with
> the autofill use cases to confirm that this change in semantics (from page to
> main frame) is OK and won't break iframe form-filling scenarios.
> 
>
https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/publ...
> File third_party/WebKit/public/web/WebUserGestureIndicator.h (right):
> 
>
https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/publ...
> third_party/WebKit/public/web/WebUserGestureIndicator.h:51: // time the page
was
> loaded.
> Update the comment to describe the new semantics (no longer per-page).

[Nate Chapin, 2016-10-24 18:25:46]

On 2016/10/24 16:56:16, Rick Byers wrote:
> On 2016/10/17 17:02:47, Rick Byers wrote:
> > The overall approach here seems really good (much better than the previous
> > attempt at a hack), thank you for being thorough!  Just a few mostly minor
> > comments.
> > 
> > Reference BUG=624061 in the CL description?
> 
> FYI, I updated your CL description with this.
> 

Thanks, sorry I missed that one.

[Rick Byers, 2016-10-24 18:30:47]

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

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:176:
DocumentUserGestureToken::create(m_frame->document())));
On 2016/10/18 23:26:33, Nate Chapin wrote:
> On 2016/10/17 17:18:05, Navid Zolghadr wrote:
> > On 2016/10/17 17:02:46, Rick Byers wrote:
> > > I'm not positive what m_frame refers to here in all cases. Navid would it
be
> > > better to use target's document instead of m_frame to ensure we're getting
> the
> > > inner-most document?
> > 
> > Unfortunately that is the case for touch events. m_frame is the frame that
> this
> > PointerEventManager belongs to. Every iframe has its own EventHandler and
> > PointerEventManager. However all the touch events and touch pointer events
are
> > being sent directly to the target node inside the innermost iframe right
from
> > the top level frame and PointerEventManager. So if the pointerType is touch
> > m_frame refers to the main page frame. So if we would like to create the
> gesture
> > for the inner most iframe it is better to use iframe that the target is in.
I
> > guess we need to cast to to Node and get the document.
> 
> Done. It's safe to assume that the EventTarget is a Node here, right?

I don't think so - I believe there are rare scenarios where a pointer event can
be dispatched to a document or the window.  It's probably OK not to set the
document gesture state in those cases if you want to keep the code trivial.

https://codereview.chromium.org/2408333004/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h (right):

https://codereview.chromium.org/2408333004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h:30: for (;
document; document = document->parentDocument()) {
Any thoughts about what should be done for OOPIF?  Perhaps just file a bug for
now (cc'ing some site-isolation-dev folks like kenrb) and reference it with a
TODO here?

https://codereview.chromium.org/2408333004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h:31: if (document)
On 2016/10/21 21:36:49, Nate Chapin wrote:
> On 2016/10/21 21:33:06, ojan wrote:
> > This can early return if the document->hasReceivedUserGesture(), right? No
> need
> > to walk up the ancestor chain on every keypress. :)
> 
> Technically it's not guaranteed that, if a Document has its user gesture flag
> set, that all ancestors will have it set, because the flag can be cleared by
the
> history-entry-requires-user-gesture experiment. If the experiment is removed,
> then I agree, that optimization makes sense.

If there's no good way to avoid this then I'm not too worried about it.  I don't
think we should ever be taking a token any more frequently than "response"
scenarios (eg. each key press, not each and every mouse/touchmove which is
"animation"), and walking the frame chain should always be quite cheap.

[Nate Chapin, 2016-10-24 18:45:05]

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

[Nate Chapin, 2016-10-24 18:45:19]

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

https://codereview.chromium.org/2408333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:176:
DocumentUserGestureToken::create(m_frame->document())));
On 2016/10/24 18:30:46, Rick Byers wrote:
> On 2016/10/18 23:26:33, Nate Chapin wrote:
> > On 2016/10/17 17:18:05, Navid Zolghadr wrote:
> > > On 2016/10/17 17:02:46, Rick Byers wrote:
> > > > I'm not positive what m_frame refers to here in all cases. Navid would
it
> be
> > > > better to use target's document instead of m_frame to ensure we're
getting
> > the
> > > > inner-most document?
> > > 
> > > Unfortunately that is the case for touch events. m_frame is the frame that
> > this
> > > PointerEventManager belongs to. Every iframe has its own EventHandler and
> > > PointerEventManager. However all the touch events and touch pointer events
> are
> > > being sent directly to the target node inside the innermost iframe right
> from
> > > the top level frame and PointerEventManager. So if the pointerType is
touch
> > > m_frame refers to the main page frame. So if we would like to create the
> > gesture
> > > for the inner most iframe it is better to use iframe that the target is
in.
> I
> > > guess we need to cast to to Node and get the document.
> > 
> > Done. It's safe to assume that the EventTarget is a Node here, right?
> 
> I don't think so - I believe there are rare scenarios where a pointer event
can
> be dispatched to a document or the window.  It's probably OK not to set the
> document gesture state in those cases if you want to keep the code trivial.

SG, added a null check.

https://codereview.chromium.org/2408333004/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h (right):

https://codereview.chromium.org/2408333004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h:30: for (;
document; document = document->parentDocument()) {
On 2016/10/24 18:30:47, Rick Byers wrote:
> Any thoughts about what should be done for OOPIF?  Perhaps just file a bug for
> now (cc'ing some site-isolation-dev folks like kenrb) and reference it with a
> TODO here?

Added TODO, filed https://bugs.chromium.org/p/chromium/issues/detail?id=658800.

[commit-bot: I haz the power, 2016-10-24 18:46:03]

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

[Rick Byers, 2016-10-24 19:04:15]

LGTM from my perspective.

Thanks for keeping the rebases separate - made reviewing updates really easy :-)

[commit-bot: I haz the power, 2016-10-24 19:41:40]

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

[commit-bot: I haz the power, 2016-10-24 19:41:41]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[ojan, 2016-10-24 20:06:53]

lgtm

[Nate Chapin, 2016-10-24 20:07:40]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2016-10-24 20:08:43]

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-10-24 20:45:31]

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

[commit-bot: I haz the power, 2016-10-24 20:45:33]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Nate Chapin, 2016-10-24 20:50:08]

japhet@chromium.org changed reviewers:
+ creis@chromium.org, reillyg@chromium.org

[Nate Chapin, 2016-10-24 20:50:10]

reillyg, would you mind reviewing these mechanical extensions/renderer changes?

creis: 4 lines in render_frame_impl.cc, please?

[Charlie Reis, 2016-10-24 21:38:16]

render_frame_impl.cc looks ok, but I'm concerned about the OOPIF TODO, since
OOPIFs are on by default on trunk already.

https://codereview.chromium.org/2408333004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h (right):

https://codereview.chromium.org/2408333004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h:31: // See
http://crbug.com/658800
What are the consequences of leaving this as a TODO?  OOPIFs are live on Chrome
Beta and we're very close to turning them on for Stable, so we can't take on
more blocking bugs or TODOs like this.  I'd like this to be resolved before
landing, unless it's clear that the current behavior isn't a problem.

[Nate Chapin, 2016-10-24 22:03:45]

On 2016/10/24 21:38:16, Charlie Reis wrote:
> render_frame_impl.cc looks ok, but I'm concerned about the OOPIF TODO, since
> OOPIFs are on by default on trunk already.
> 
>
https://codereview.chromium.org/2408333004/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h (right):
> 
>
https://codereview.chromium.org/2408333004/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h:31: // See
> http://crbug.com/658800
> What are the consequences of leaving this as a TODO?  OOPIFs are live on
Chrome
> Beta and we're very close to turning them on for Stable, so we can't take on
> more blocking bugs or TODOs like this.  I'd like this to be resolved before
> landing, unless it's clear that the current behavior isn't a problem.

Replied on the bug. tl;dr, what we have is pretty broken in a lot of cases,
including OOPIFs, but this might be slightly more broken.

[alexmos, 2016-10-25 00:08:03]

On 2016/10/24 22:03:45, Nate Chapin wrote:
> On 2016/10/24 21:38:16, Charlie Reis wrote:
> > render_frame_impl.cc looks ok, but I'm concerned about the OOPIF TODO, since
> > OOPIFs are on by default on trunk already.
> > 
> >
>
https://codereview.chromium.org/2408333004/diff/200001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h (right):
> > 
> >
>
https://codereview.chromium.org/2408333004/diff/200001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h:31: // See
> > http://crbug.com/658800
> > What are the consequences of leaving this as a TODO?  OOPIFs are live on
> Chrome
> > Beta and we're very close to turning them on for Stable, so we can't take on
> > more blocking bugs or TODOs like this.  I'd like this to be resolved before
> > landing, unless it's clear that the current behavior isn't a problem.
> 
> Replied on the bug. tl;dr, what we have is pretty broken in a lot of cases,
> including OOPIFs, but this might be slightly more broken.

Instead of stopping at a remote parentDocument(), would it be better to walk
through the full frame ancestor chain here and set the gesture on documents for
all LocalFrames along the way?  That way, I think the A-B-A cases you mentioned
on the bug won't break, and this won't make the OOPIF user gesture behavior any
more broken than it already is.

[Reilly Grant (use Gerrit), 2016-10-25 00:53:23]

The //extensions bit lgtm but that seems like just plumbing for the more
interesting changes in Blink.

[Nate Chapin, 2016-10-25 01:54:13]

On 2016/10/25 00:08:03, alexmos wrote:
> On 2016/10/24 22:03:45, Nate Chapin wrote:
> > On 2016/10/24 21:38:16, Charlie Reis wrote:
> > > render_frame_impl.cc looks ok, but I'm concerned about the OOPIF TODO,
since
> > > OOPIFs are on by default on trunk already.
> > > 
> > >
> >
>
https://codereview.chromium.org/2408333004/diff/200001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2408333004/diff/200001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h:31: // See
> > > http://crbug.com/658800
> > > What are the consequences of leaving this as a TODO?  OOPIFs are live on
> > Chrome
> > > Beta and we're very close to turning them on for Stable, so we can't take
on
> > > more blocking bugs or TODOs like this.  I'd like this to be resolved
before
> > > landing, unless it's clear that the current behavior isn't a problem.
> > 
> > Replied on the bug. tl;dr, what we have is pretty broken in a lot of cases,
> > including OOPIFs, but this might be slightly more broken.
> 
> Instead of stopping at a remote parentDocument(), would it be better to walk
> through the full frame ancestor chain here and set the gesture on documents
for
> all LocalFrames along the way?  That way, I think the A-B-A cases you
mentioned
> on the bug won't break, and this won't make the OOPIF user gesture behavior
any
> more broken than it already is.

I think that's reasonable. The main question is whether it's sufficient to leave
this no more broken, or whether, as nasko suggested in the bug, it should be
fixed properly in this CL.

[Nate Chapin, 2016-10-25 17:51:39]

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

[commit-bot: I haz the power, 2016-10-25 17:52:35]

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

[Charlie Reis, 2016-10-25 18:24:37]

On 2016/10/25 01:54:13, Nate Chapin wrote:
> On 2016/10/25 00:08:03, alexmos wrote:
> > On 2016/10/24 22:03:45, Nate Chapin wrote:
> > > On 2016/10/24 21:38:16, Charlie Reis wrote:
> > > > render_frame_impl.cc looks ok, but I'm concerned about the OOPIF TODO,
> since
> > > > OOPIFs are on by default on trunk already.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2408333004/diff/200001/third_party/WebKit/Sou...
> > > > File third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h
> (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2408333004/diff/200001/third_party/WebKit/Sou...
> > > > third_party/WebKit/Source/core/dom/DocumentUserGestureToken.h:31: // See
> > > > http://crbug.com/658800
> > > > What are the consequences of leaving this as a TODO?  OOPIFs are live on
> > > Chrome
> > > > Beta and we're very close to turning them on for Stable, so we can't
take
> on
> > > > more blocking bugs or TODOs like this.  I'd like this to be resolved
> before
> > > > landing, unless it's clear that the current behavior isn't a problem.
> > > 
> > > Replied on the bug. tl;dr, what we have is pretty broken in a lot of
cases,
> > > including OOPIFs, but this might be slightly more broken.
> > 
> > Instead of stopping at a remote parentDocument(), would it be better to walk
> > through the full frame ancestor chain here and set the gesture on documents
> for
> > all LocalFrames along the way?  That way, I think the A-B-A cases you
> mentioned
> > on the bug won't break, and this won't make the OOPIF user gesture behavior
> any
> > more broken than it already is.
> 
> I think that's reasonable. The main question is whether it's sufficient to
leave
> this no more broken, or whether, as nasko suggested in the bug, it should be
> fixed properly in this CL.

I'm ok with not making it worse.  Thanks!  

Also, content/ LGTM.

[commit-bot: I haz the power, 2016-10-25 19:14:39]

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

[commit-bot: I haz the power, 2016-10-25 19:14:40]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nate Chapin, 2016-10-25 19:24:25]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2016-10-25 19:24:26]

The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org,
reillyg@chromium.org, ojan@chromium.org, creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2408333004/#ps240001
(title: "Re-add dropped null check")

[commit-bot: I haz the power, 2016-10-25 19:25:09]

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-10-25 21:30:38]

Description was changed from

==========
Move persistent gesture state to Document, add DocumentUserGestureToken

Non-test gesture tokens are now created as a DocumentUserGestureToken, which
take a Document* (which can be null, but such uses are limited). When it is
created, it sets the m_hasReceivedUserGesture flag on Document.

Remove UserGestureIndicator::s_processedUserGestureSinceLoad, as
Document::m_hasReceivedUserGesture is very similar and not process-wide.

BUG=624061
==========

to

==========
Move persistent gesture state to Document, add DocumentUserGestureToken

Non-test gesture tokens are now created as a DocumentUserGestureToken, which
take a Document* (which can be null, but such uses are limited). When it is
created, it sets the m_hasReceivedUserGesture flag on Document.

Remove UserGestureIndicator::s_processedUserGestureSinceLoad, as
Document::m_hasReceivedUserGesture is very similar and not process-wide.

BUG=624061
==========

[commit-bot: I haz the power, 2016-10-25 21:30:41]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-10-25 21:33:30]

Description was changed from

==========
Move persistent gesture state to Document, add DocumentUserGestureToken

Non-test gesture tokens are now created as a DocumentUserGestureToken, which
take a Document* (which can be null, but such uses are limited). When it is
created, it sets the m_hasReceivedUserGesture flag on Document.

Remove UserGestureIndicator::s_processedUserGestureSinceLoad, as
Document::m_hasReceivedUserGesture is very similar and not process-wide.

BUG=624061
==========

to

==========
Move persistent gesture state to Document, add DocumentUserGestureToken

Non-test gesture tokens are now created as a DocumentUserGestureToken, which
take a Document* (which can be null, but such uses are limited). When it is
created, it sets the m_hasReceivedUserGesture flag on Document.

Remove UserGestureIndicator::s_processedUserGestureSinceLoad, as
Document::m_hasReceivedUserGesture is very similar and not process-wide.

BUG=624061

Committed: https://crrev.com/26ce312cc8b3f53dc76df85e45c17bba93685fc3
Cr-Commit-Position: refs/heads/master@{#427481}
==========

[commit-bot: I haz the power, 2016-10-25 21:33:32]

Patchset 13 (id:??) landed as
https://crrev.com/26ce312cc8b3f53dc76df85e45c17bba93685fc3
Cr-Commit-Position: refs/heads/master@{#427481}