Bug 226655 – [iOS] Meaningful click heuristic should account for media state changes

• Component: Platform
• Status: RESOLVED FIXED

[Wenson Hsieh]

rdar://78330664

[Wenson Hsieh]

Created attachment 430603 [details]
Patch

[Wenson Hsieh]

Created attachment 430607 [details]
Patch

[Tim Horton]

Comment on attachment 430607 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430607&action=review

> Source/WebKit/WebProcess/WebPage/WebPage.h:2195
>      bool m_didHandleOrPreventMouseDownOrMouseUpEventDuringSyntheticClick { false };
> +    bool m_didChangePlayingMediaStateDuringSyntheticClick { false };

How many of these are we going to end up with? Is it actually important that we keep track of /why/? 
Should we just "m_currentSyntheticClickMayBeMeaningful { true }" and then falsy it in all the callbacks? Then you don't need m_isCompletingSyntheticClick or separate bits for each thing.

[Wenson Hsieh]

(In reply to Tim Horton from comment #3)
> Comment on attachment 430607 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430607&action=review
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.h:2195
> >      bool m_didHandleOrPreventMouseDownOrMouseUpEventDuringSyntheticClick { false };
> > +    bool m_didChangePlayingMediaStateDuringSyntheticClick { false };
> 
> How many of these are we going to end up with? Is it actually important that
> we keep track of /why/? 
> Should we just "m_currentSyntheticClickMayBeMeaningful { true }" and then
> falsy it in all the callbacks? Then you don't need
> m_isCompletingSyntheticClick or separate bits for each thing.

That's a good point — I'll change it to `m_currentSyntheticClickMayBeMeaningful` instead, in the way you described.

(As an aside, if we accumulate one or two more of these call sites in the future, we'll also probably want this to also turn it into an OptionSet).

[Wenson Hsieh]

Created attachment 430611 [details]
Patch

[Devin Rousso]

Comment on attachment 430611 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430611&action=review

r=me as well :)

> Source/WebKit/WebProcess/WebPage/WebPage.h:2193
> +    bool m_currentSyntheticClickMayBeNonMeaningful { true };

NIT: How about `m_currentSyntheticClickMayNotBeMeaningful`? 😅

[Wenson Hsieh]

(In reply to Devin Rousso from comment #6)
> Comment on attachment 430611 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430611&action=review
> 
> r=me as well :)
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.h:2193
> > +    bool m_currentSyntheticClickMayBeNonMeaningful { true };
> 
> NIT: How about `m_currentSyntheticClickMayNotBeMeaningful`? 😅

Sure — I don't have a preference for either one, so I'll change it to `m_currentSyntheticClickMayNotBeMeaningful`.

[Wenson Hsieh]

Created attachment 430616 [details]
Renaming

[EWS]

Committed r278515 (238514@main): <https://commits.webkit.org/238514@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430616 [details].