Bug 207058 – [iPad] Videos on nhl.com can't be scrubbed when loaded with desktop UA.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

[iPad] Videos on nhl.com can't be scrubbed when loaded with desktop UA.

[Jer Noble]

Created attachment 389387 [details]
Patch

[Jer Noble]

Created attachment 389395 [details]
Patch

[Jer Noble]

Created attachment 389396 [details]
Patch

[Jer Noble]

Created attachment 389401 [details]
Patch

[Jer Noble]

<rdar://problem/58778950>

[Jer Noble]

Created attachment 389402 [details]
Patch

[Darin Adler]

Comment on attachment 389402 [details]
Patch

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

> Source/WebCore/ChangeLog:1
> +        [iPad] Videos on nhl.com can't be scrubbed when loaded with desktop UA.

Isn’t this missing the name and date?

> Source/WebCore/ChangeLog:12
> +        Drive-by fix: There's no need to re-parse the entire URL and walk through all the cases
> +        every time shouldDispatchSimulatedMouseEvents() is called. Save the results of the initial
> +        pass so that subsequent checks are just a simple bool check.

This is best done by factoring into a function that returns a bool and a separate function that memoizes the result rather than changing every return statement.

[Jer Noble]

Comment on attachment 389402 [details]
Patch

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

>> Source/WebCore/ChangeLog:1
>> +        [iPad] Videos on nhl.com can't be scrubbed when loaded with desktop UA.
> 
> Isn’t this missing the name and date?

Huh, that's weird. It does explain why webkit-patch threw an exception every time I tried to upload this patch.

>> Source/WebCore/ChangeLog:12
>> +        pass so that subsequent checks are just a simple bool check.
> 
> This is best done by factoring into a function that returns a bool and a separate function that memoizes the result rather than changing every return statement.

Will do.

[Jer Noble]

Created attachment 389518 [details]
Patch

[Darin Adler]

Comment on attachment 389518 [details]
Patch

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

> Source/WebCore/page/Quirks.cpp:295
> +    if (m_shouldDispatchSimulatedMouseEventsQuirk)
> +        return m_shouldDispatchSimulatedMouseEventsQuirk.value();

I suggest merging this with the code below.

> Source/WebCore/page/Quirks.cpp:353
> +    m_shouldDispatchSimulatedMouseEventsQuirk = doShouldDispatchChecks();
> +    return m_shouldDispatchSimulatedMouseEventsQuirk.value();

I suggest writing it like this:

    if (!m_shouldDispatchSimulatedMouseEventsQuirk)
        m_shouldDispatchSimulatedMouseEventsQuirk = doShouldDispatchChecks();
    return *m_shouldDispatchSimulatedMouseEventsQuirk;

I don’t think we need the second return statement above.

[Jer Noble]

(In reply to Darin Adler from comment #10)
> Comment on attachment 389518 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=389518&action=review
> 
> > Source/WebCore/page/Quirks.cpp:295
> > +    if (m_shouldDispatchSimulatedMouseEventsQuirk)
> > +        return m_shouldDispatchSimulatedMouseEventsQuirk.value();
> 
> I suggest merging this with the code below.
> 
> > Source/WebCore/page/Quirks.cpp:353
> > +    m_shouldDispatchSimulatedMouseEventsQuirk = doShouldDispatchChecks();
> > +    return m_shouldDispatchSimulatedMouseEventsQuirk.value();
> 
> I suggest writing it like this:
> 
>     if (!m_shouldDispatchSimulatedMouseEventsQuirk)
>         m_shouldDispatchSimulatedMouseEventsQuirk = doShouldDispatchChecks();
>     return *m_shouldDispatchSimulatedMouseEventsQuirk;
> 
> I don’t think we need the second return statement above.

Will do. (Didn't realize that Optional had an `operator *()`.)

[Jer Noble]

Created attachment 389559 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 389559 [details]
Patch for landing

Clearing flags on attachment: 389559

Committed r255592: <https://trac.webkit.org/changeset/255592>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(In reply to Jer Noble from comment #11)
> Didn't realize that Optional had an `operator *()`.

For a long time I strongly supported using has_value() and value() on optionals, but I now think that we should almost always use "!" and "*". Stylistic choice, so maybe needs to be debated.

[Jer Noble]

Committed follow-up fix in r255609 <https://trac.webkit.org/changeset/255609>.