Summary: | [iPad] Videos on nhl.com can't be scrubbed when loaded with desktop UA. | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||||||||||||
Component: | New Bugs | Assignee: | Jer Noble <jer.noble> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | bdakin, bfulgham, commit-queue, darin, eric.carlson, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Jer Noble
2020-01-31 10:53:38 PST
Created attachment 389387 [details]
Patch
Created attachment 389395 [details]
Patch
Created attachment 389396 [details]
Patch
Created attachment 389401 [details]
Patch
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>. |