I think this is the best resolution to the problem of this experiment breaking a ...
3 years, 8 months ago
(2017-03-28 23:40:35 UTC)
#4
I think this is the best resolution to the problem of this experiment breaking a
couple hundred layout tests: have a test that explicitly enables and tests its
behavior, but leave the old behavior on by default for all the other tests.
As you mentioned on intervention-dev, I added a rule that if a Document has been
displayed for 5 seconds, it can ignore the user gesture requirement. I picked
that time limit arbitrarily. I didn't add a test case for the time limit,
because tests with 5+ second delays are evil and I don't think there's a way for
layout tests to fast-forward currentTime().
Note that there's an existing policy that meta refreshes are only eligible for
history entries if they are at least 1 second long. We should probably consider
that obsolete if we ship this intervention.
Nate Chapin
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
3 years, 8 months ago
(2017-03-28 23:59:25 UTC)
#5
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_chromeos_rel_ng/builds/393448)
3 years, 8 months ago
(2017-03-29 01:53:27 UTC)
#8
Might make sense to split this into two patches since the change and the tests ...
3 years, 8 months ago
(2017-04-03 21:03:54 UTC)
#9
Might make sense to split this into two patches since the change and the tests
do different things. At a minimum though, the change in behavior is probably the
more significant part of this patch than the tests, so should be the first line
of the description.
https://codereview.chromium.org/2780953002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/history/history-entry-requires-user-gesture.html
(right):
https://codereview.chromium.org/2780953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/history/history-entry-requires-user-gesture.html:1:
<body onload="loaded();">
Should we also have a version of this test that has a user gesture?
https://codereview.chromium.org/2780953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/history/history-entry-requires-user-gesture.html:7:
internals.settings.setHistoryEntryRequiresUserGesture(true);
A downside of this solution is that we have to keep the setting around forever.
Is this just a problem of taking the time to update the other tests? Is it hard
to give them all a user gesture?
I'd be fine with a TODO to update the tests when we remove this setting.
https://codereview.chromium.org/2780953002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):
https://codereview.chromium.org/2780953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:3197: return elapsedTime() >=
5000 || m_frame->hasReceivedUserGesture();
I think you could test this as a SimTest that fast forwards the clock?
Also, can you move this time into a static constant so it can be more
self-documenting?
Nate Chapin
Description was changed from ========== Add a test for historyEntryRequiresUserGesture flag. Also, exempt documents from ...
3 years, 8 months ago
(2017-04-04 20:40:34 UTC)
#10
Description was changed from
==========
Add a test for historyEntryRequiresUserGesture flag.
Also, exempt documents from its restriction if they have been committed for
at least 5 seconds.
BUG=638198
TEST=http/tests/history/history-entry-requires-user-gesture.html
==========
to
==========
When historyEntryRequiresUserGesture is enabled, exempt docs that have been
committed for 5 seconds
Also, add a test for historyEntryRequiresUserGesture flag.
BUG=638198
TEST=http/tests/history/history-entry-requires-user-gesture.html
==========
Nate Chapin
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
3 years, 8 months ago
(2017-04-04 20:41:04 UTC)
#11
Agreed on the description-ordering. I'm somewhat inclined to leave this as one CL, since they're ...
3 years, 8 months ago
(2017-04-04 20:42:20 UTC)
#13
Agreed on the description-ordering. I'm somewhat inclined to leave this as one
CL, since they're both clearly related to the same feature and both part of
getting it hardened and ready for live experiments. If you feel strongly, I'll
split it.
https://codereview.chromium.org/2780953002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/history/history-entry-requires-user-gesture.html
(right):
https://codereview.chromium.org/2780953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/history/history-entry-requires-user-gesture.html:1:
<body onload="loaded();">
On 2017/04/03 21:03:54, ojan wrote:
> Should we also have a version of this test that has a user gesture?
Added a 2nd step to this test, so there's one navigation with a user gesture and
one without.
https://codereview.chromium.org/2780953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/history/history-entry-requires-user-gesture.html:7:
internals.settings.setHistoryEntryRequiresUserGesture(true);
On 2017/04/03 21:03:54, ojan wrote:
> A downside of this solution is that we have to keep the setting around
forever.
>
> Is this just a problem of taking the time to update the other tests? Is it
hard
> to give them all a user gesture?
>
> I'd be fine with a TODO to update the tests when we remove this setting.
Time + churn. Giving them all a user gesture is a fairly tedious but mechanical
change. More importantly, quite a few of the tests that depend on the existing
behavior are web-platform-tests, and I don't want to update those until this
behavior is in the spec. I just don't think it makes sense to do it until this
feature is proven, at which point we can spec it, update the tests, and remove
the flag.
Added TODO in Document::canCreateHistoryEntry().
https://codereview.chromium.org/2780953002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):
https://codereview.chromium.org/2780953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:3197: return elapsedTime() >=
5000 || m_frame->hasReceivedUserGesture();
On 2017/04/03 21:03:54, ojan wrote:
> I think you could test this as a SimTest that fast forwards the clock?
I could find ways to mess with last time of a compositing frame, but didn't
immediately see a way to fast forward currentTime()
>
> Also, can you move this time into a static constant so it can be more
> self-documenting?
Done.
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
3 years, 8 months ago
(2017-04-04 22:41:47 UTC)
#14
lgtm https://codereview.chromium.org/2780953002/diff/20001/third_party/WebKit/LayoutTests/http/tests/history/history-entry-requires-user-gesture.html File third_party/WebKit/LayoutTests/http/tests/history/history-entry-requires-user-gesture.html (right): https://codereview.chromium.org/2780953002/diff/20001/third_party/WebKit/LayoutTests/http/tests/history/history-entry-requires-user-gesture.html#newcode7 third_party/WebKit/LayoutTests/http/tests/history/history-entry-requires-user-gesture.html:7: internals.settings.setHistoryEntryRequiresUserGesture(true); On 2017/04/04 at 20:42:19, Nate Chapin wrote: ...
3 years, 8 months ago
(2017-04-05 17:39:13 UTC)
#17
lgtm
https://codereview.chromium.org/2780953002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/history/history-entry-requires-user-gesture.html
(right):
https://codereview.chromium.org/2780953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/history/history-entry-requires-user-gesture.html:7:
internals.settings.setHistoryEntryRequiresUserGesture(true);
On 2017/04/04 at 20:42:19, Nate Chapin wrote:
> On 2017/04/03 21:03:54, ojan wrote:
> > A downside of this solution is that we have to keep the setting around
forever.
> >
> > Is this just a problem of taking the time to update the other tests? Is it
hard
> > to give them all a user gesture?
> >
> > I'd be fine with a TODO to update the tests when we remove this setting.
>
> Time + churn. Giving them all a user gesture is a fairly tedious but
mechanical change. More importantly, quite a few of the tests that depend on the
existing behavior are web-platform-tests, and I don't want to update those until
this behavior is in the spec. I just don't think it makes sense to do it until
this feature is proven, at which point we can spec it, update the tests, and
remove the flag.
>
> Added TODO in Document::canCreateHistoryEntry().
Ah. That makes total sense. I wasn't thinking about the web-platform-tests/spec
side of this. A TODO SGTM.
ojan
lgtm
3 years, 8 months ago
(2017-04-05 17:40:55 UTC)
#18
lgtm
Nate Chapin
The CQ bit was checked by japhet@chromium.org
3 years, 8 months ago
(2017-04-05 19:03:15 UTC)
#19
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491418995475680, "parent_rev": "4e77e02d524fcd145491333934273562baba08e0", "commit_rev": "5a9d9d9333295c2b73567e4256cd68abe5481380"}
3 years, 8 months ago
(2017-04-05 19:09:27 UTC)
#21
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491418995475680,
"parent_rev": "4e77e02d524fcd145491333934273562baba08e0", "commit_rev":
"5a9d9d9333295c2b73567e4256cd68abe5481380"}
commit-bot: I haz the power
Description was changed from ========== When historyEntryRequiresUserGesture is enabled, exempt docs that have been committed ...
3 years, 8 months ago
(2017-04-05 19:10:16 UTC)
#22
Message was sent while issue was closed.
Description was changed from
==========
When historyEntryRequiresUserGesture is enabled, exempt docs that have been
committed for 5 seconds
Also, add a test for historyEntryRequiresUserGesture flag.
BUG=638198
TEST=http/tests/history/history-entry-requires-user-gesture.html
==========
to
==========
When historyEntryRequiresUserGesture is enabled, exempt docs that have been
committed for 5 seconds
Also, add a test for historyEntryRequiresUserGesture flag.
BUG=638198
TEST=http/tests/history/history-entry-requires-user-gesture.html
Review-Url: https://codereview.chromium.org/2780953002
Cr-Commit-Position: refs/heads/master@{#462166}
Committed:
https://chromium.googlesource.com/chromium/src/+/5a9d9d9333295c2b73567e4256cd...
==========
commit-bot: I haz the power
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/5a9d9d9333295c2b73567e4256cd68abe5481380
3 years, 8 months ago
(2017-04-05 19:10:17 UTC)
#23
Issue 2780953002: When historyEntryRequiresUserGesture is enabled, exempt docs that have been committed for 5 seconds
(Closed)
Created 3 years, 8 months ago by Nate Chapin
Modified 3 years, 8 months ago
Reviewers: ojan, Z_DONOTUSE
Base URL:
Comments: 7
Chromium Code Reviews
Issue
2780953002:
When historyEntryRequiresUserGesture is enabled, exempt docs that have been committed for 5 seconds (Closed)
