Event Timing: implement interactionId logic #49304
Conversation
|
EWS run on previous version of this PR (hash 7eebc23) |
webkit-ews-buildbot
added
the
merging-blocked
francovs
force-pushed
the
event-timing-interaction
branch
from
7eebc23 to
dbf98be
Compare
|
EWS run on previous version of this PR (hash dbf98be) |
|
EWS run on previous version of this PR (hash a19279c) |
francovs
force-pushed
the
event-timing-interaction
branch
from
a19279c to
ff198d8
Compare
|
EWS run on previous version of this PR (hash ff198d8) |
francovs
force-pushed
the
event-timing-interaction
branch
from
ff198d8 to
53d7738
Compare
|
EWS run on previous version of this PR (hash 53d7738) |
francovs
force-pushed
the
event-timing-interaction
branch
from
53d7738 to
f69c260
Compare
|
EWS run on previous version of this PR (hash f69c260) |
francovs
force-pushed
the
event-timing-interaction
branch
from
f69c260 to
48bcd5c
Compare
|
EWS run on previous version of this PR (hash 48bcd5c) |
| class PerformanceEventTiming final : public PerformanceEntry { | ||
| public: | ||
| using InteractionID = uint64_t; | ||
| struct Candidate { |
There was a problem hiding this comment.
consider running dump-class-layout on this to make sure you've got an efficiently laid out struct.
francovs
force-pushed
the
event-timing-interaction
branch
from
48bcd5c to
8ec253d
Compare
|
EWS run on previous version of this PR (hash 8ec253d) |
webkit-ews-buildbot
added
the
merging-blocked
francovs
force-pushed
the
event-timing-interaction
branch
from
8ec253d to
8ef5b40
Compare
|
EWS run on previous version of this PR (hash 8ef5b40) |
webkit-ews-buildbot
added
the
merging-blocked
| #include "PushSubscriptionOwner.h" | ||
| #include "Supplementable.h" | ||
| #include "WindowOrWorkerGlobalScope.h" | ||
| #include <JavaScriptCore/HandleForward.h> |
There was a problem hiding this comment.
We shouldn't be including PerformanceEventTiming.h here.
We should move InteractionID out of PerformanceEventTiming and forward declare it here instead.
There was a problem hiding this comment.
PerformanceEventTiming.h is also required due PerformanceEventTiming::Candidate, which is used in m_performanceEventTimingCandidates (and others), so it cannot be forward-declared.
I believe the PerformanceEventTiming.h is lightweight now (it does not bring in Performance.h anymore) so it shouldn't be a disaster.
There was a problem hiding this comment.
We should move PerformanceEventTiming::Candidate out of PerformanceEventTiming as well so that we don't need to include the header file. Generally speaking, we shouldn't be including any header files unless absolutely necessary.
There was a problem hiding this comment.
Moved to a separate header (page/PerformanceEventTimingCandidate.h)
|
|
||
| increaseInteractionCount(); | ||
| auto it = m_pendingKeyDowns.find(code); | ||
| it->value.interactionID = ensureUserInteractionValue(); |
There was a problem hiding this comment.
I would have called this function generateUserInteractionValue after merging it with increaseInteractionCount. Having separate functions to increase the count and return the current value is not a common pattern we use in WebCore.
There was a problem hiding this comment.
It is hard to merge these functions cleanly because contextmenu and pointerup sometimes use the current interaction count without incrementing it.
I added a return to increaseInteractionCount(), simplifying some logic, but kept ensureUserInteractionValue(). I also created maintainInteractionCount() to document cases in which the spec uses the current interaction value without incrementing.
francovs
force-pushed
the
event-timing-interaction
branch
from
4cdc128 to
ec91133
Compare
|
EWS run on previous version of this PR (hash ec91133) |
🧪 wpe-wk2
| { | ||
| // User interaction value should be increased by a small integer to | ||
| // "discourage developers from considering it as a counter": | ||
| ensureUserInteractionValue().value += 7; |
There was a problem hiding this comment.
Hm... doesn't this mean the developer could just divide the number by 7 to get an effective counter?
Do other browsers hard-code a value like this?
There was a problem hiding this comment.
Yes, though this isn't really a problem since window.interactionCount is available. The spec explicitly allows a constant increment, it only warns agains exposing leaks:
A user agent may choose to increase it by a small random integer every time, or choose a constant. A user agent must not use a shared global user interaction values for all Windows, because this could introduce cross-origin leaks.
My understanding is that the point is to provide a soft warning to a user trying to match PerformanceEventTiming.interactionId to window.interactionCount. For that, maybe it would be better to pick something around 100, which would ensure these two are at least two orders of magnitude apart when considering the latest interaction.
I picked 7 on a whim because it is small and has no obvious divisibility test, but it turns out both chromium and Gecko picked the same number:
// Interaction ID increment. We increase this value by an integer greater than 1
// to discourage developers from using the value to 'count' the number of user
// interactions. This is consistent with the spec, which allows the increasing
// the user interaction value by a small number chosen by the user agent.
constexpr uint32_t kInteractionIdIncrement = 7;
francovs
force-pushed
the
event-timing-interaction
branch
from
ec91133 to
1fa4f7f
Compare
|
EWS run on previous version of this PR (hash 1fa4f7f) |
| struct PerformanceEventTimingCandidate { | ||
| EventType type { }; | ||
| bool cancelable { false }; | ||
| Seconds startTime { 0 }; |
There was a problem hiding this comment.
I don't think you need to explicitly initialize Seconds like this
There was a problem hiding this comment.
Now fixed, but removing the explicit initializer caused some bots to hit -Werror=missing-field-initializers. I had to add some unnecessary stuff to the designated constructor call to get them to build:
// LocalDOMWindow::initializeEventTimingEntry():
return PerformanceEventTimingCandidate {
.type = type,
.cancelable = event.cancelable(),
.startTime = startTime,
.processingStart = processingStart,
.processingEnd = { }, // here
.duration = { }, // here
.target = { }, // here
.interactionID = computeInteractionID(event, type)
};I could also add explicit constructors to PerformanceEventTimingCandidate to avoid all this, but it felt very boilerplate-y.
--
I think this is a bad warning since missing elements should be copy initialized from { }, which feels safe to me (see "Implicitly initialized elements"). This example in Godbolt reproduces the warning.
francovs
force-pushed
the
event-timing-interaction
branch
from
1fa4f7f to
6bb52d8
Compare
|
EWS run on previous version of this PR (hash 6bb52d8) |
francovs
force-pushed
the
event-timing-interaction
branch
from
6bb52d8 to
d66ae1f
Compare
|
EWS run on previous version of this PR (hash d66ae1f) |
francovs
force-pushed
the
event-timing-interaction
branch
from
d66ae1f to
6f74b34
Compare
|
EWS run on previous version of this PR (hash 6f74b34) |
francovs
force-pushed
the
event-timing-interaction
branch
from
6f74b34 to
787fe6a
Compare
|
EWS run on current version of this PR (hash 787fe6a) |
https://bugs.webkit.org/show_bug.cgi?id=297361 rdar://158264903 Reviewed by Ryosuke Niwa. Assigns interactionId to PerformanceEventTiming entries that qualify, according to https://www.w3.org/TR/event-timing/. Only certain deliberate user actions such as clicks or keypresses are considered; passive events such as pointerover or pointerexit do not qualify. Some events are grouped together, receiving the same interactionId. One such example is pointerdown, pointerup and click. We currently only support a single pointer, so the map variables related to pointer interactions (pointerMap[] and pendingPointerDown[]) mentioned in the spec (https://www.w3.org/TR/event-timing/#pointer-interaction-value-map) were implemented as a struct (PointerInteractionState) containing the necessary state for a single pointer. In that conversion, pointerMap was renamed to PointerInteractionState::interactionID. New files created to achieve more fine grained header inclusion: * Source/WebCore/page/EventTimingInteractionID.h * Source/WebCore/page/PerformanceEventTimingCandidate.h Canonical link: https://commits.webkit.org/299033@main
webkit-commit-queue
force-pushed
the
event-timing-interaction
branch
from
787fe6a to
5e3ae48
Compare
|
Committed 299033@main (5e3ae48): https://commits.webkit.org/299033@main Reviewed commits have been landed. Closing PR #49304 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
6 participants
5e3ae48
787fe6a
🛠 win🛠 ios-sim🧪 wpe-wk2🧪 win-tests🧪 ios-wk2🧪 api-wpe🧪 ios-wk2-wpt🧪 api-ios🧪 mac-wk2🧪 mac-AS-debug-wk2🛠 vision-sim🧪 mac-intel-wk2🛠 playstation🛠 tv🛠 mac-safer-cpp🛠 tv-sim🛠 watch🛠 watch-sim