[iPad] Videos on nhl.com can't be scrubbed when loaded with desktop UA.
Created attachment 389387 [details] Patch
Created attachment 389395 [details] Patch
Created attachment 389396 [details] Patch
Created attachment 389401 [details] Patch
<rdar://problem/58778950>
Created attachment 389402 [details] Patch
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.
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.
Created attachment 389518 [details] Patch
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.
(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 *()`.)
Created attachment 389559 [details] Patch for landing
Comment on attachment 389559 [details] Patch for landing Clearing flags on attachment: 389559 Committed r255592: <https://trac.webkit.org/changeset/255592>
All reviewed patches have been landed. Closing bug.
(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.
Committed follow-up fix in r255609 <https://trac.webkit.org/changeset/255609>.