Bug 231279 - ASSERTion failure in WebCore::deviceClass() on macCatalyst
Summary: ASSERTion failure in WebCore::deviceClass() on macCatalyst
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-10-05 22:43 PDT by Tim Horton
Modified: 2021-10-06 13:11 PDT (History)
8 users (show)

See Also:


Attachments
Patch (1.79 KB, patch)
2021-10-05 22:43 PDT, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2021-10-05 22:43:22 PDT
ASSERTion failure in WebCore::deviceClass() on macCatalyst
Comment 1 Tim Horton 2021-10-05 22:43:58 PDT
Created attachment 440331 [details]
Patch
Comment 2 EWS 2021-10-06 11:17:49 PDT
Committed r283638 (242589@main): <https://commits.webkit.org/242589@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440331 [details].
Comment 3 Radar WebKit Bug Importer 2021-10-06 11:19:52 PDT
<rdar://problem/83943064>
Comment 4 Darin Adler 2021-10-06 11:55:03 PDT
Comment on attachment 440331 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        * platform/ios/Device.cpp:
> +        (WebCore::deviceClass):

Separately, should we stop exporting this unclear-how-to-use correctly function and instead expose more-semantic functions that match our uses?

3 uses are checking against iPad, so it can support a different media playback style than others in the iOS family
2 uses are checking for iPhone + iPod since we are omitting defaultMediaSource there (not sure why)
1 use is checking for iPhone + iPod + Apple Watch, to try to approximate -[UIDevice userInterfaceIdiom] when we can’t call it.

That’s it, just those 3 types of callers. Maybe we just expose three functions instead of using MGDeviceClass?
Comment 5 Tim Horton 2021-10-06 11:59:57 PDT
(In reply to Darin Adler from comment #4)
> Comment on attachment 440331 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=440331&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        * platform/ios/Device.cpp:
> > +        (WebCore::deviceClass):
> 
> Separately, should we stop exporting this unclear-how-to-use correctly
> function and instead expose more-semantic functions that match our uses?
> 
> 3 uses are checking against iPad, so it can support a different media
> playback style than others in the iOS family

I expect these want to be iPhone+iPod checks instead, so we can probably expose just 2. And I agree!
Comment 6 Darin Adler 2021-10-06 12:34:19 PDT
Comment on attachment 440331 [details]
Patch

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

>>> Source/WebCore/ChangeLog:9
>>> +        (WebCore::deviceClass):
>> 
>> Separately, should we stop exporting this unclear-how-to-use correctly function and instead expose more-semantic functions that match our uses?
>> 
>> 3 uses are checking against iPad, so it can support a different media playback style than others in the iOS family
>> 2 uses are checking for iPhone + iPod since we are omitting defaultMediaSource there (not sure why)
>> 1 use is checking for iPhone + iPod + Apple Watch, to try to approximate -[UIDevice userInterfaceIdiom] when we can’t call it.
>> 
>> That’s it, just those 3 types of callers. Maybe we just expose three functions instead of using MGDeviceClass?
> 
> I expect these want to be iPhone+iPod checks instead, so we can probably expose just 2. And I agree!

Then, I think we can expose just one function: something that returns true for iPhone+iPod+Watch, since in the hypothetical where we had media on watch, it would be iPhone-style, not iPad-style. We have a phone-style family (iPhone+iPod+Watch) and a larger-screen-style family (iPad+TV). And of course many features that should adapt to the current size/shape rather than the platform (so narrow apps on iPad act enough like iPhone). I would be so tempted to name this deviceClassIsSmallScreen.

But don’t ask me how this function differs from deviceHasIPadCapability.
Comment 7 Tim Horton 2021-10-06 13:11:02 PDT
(In reply to Darin Adler from comment #6)
> Comment on attachment 440331 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=440331&action=review
> 
> >>> Source/WebCore/ChangeLog:9
> >>> +        (WebCore::deviceClass):
> >> 
> >> Separately, should we stop exporting this unclear-how-to-use correctly function and instead expose more-semantic functions that match our uses?
> >> 
> >> 3 uses are checking against iPad, so it can support a different media playback style than others in the iOS family
> >> 2 uses are checking for iPhone + iPod since we are omitting defaultMediaSource there (not sure why)
> >> 1 use is checking for iPhone + iPod + Apple Watch, to try to approximate -[UIDevice userInterfaceIdiom] when we can’t call it.
> >> 
> >> That’s it, just those 3 types of callers. Maybe we just expose three functions instead of using MGDeviceClass?
> > 
> > I expect these want to be iPhone+iPod checks instead, so we can probably expose just 2. And I agree!
> 
> Then, I think we can expose just one function: something that returns true
> for iPhone+iPod+Watch, since in the hypothetical where we had media on
> watch, it would be iPhone-style, not iPad-style. We have a phone-style
> family (iPhone+iPod+Watch) and a larger-screen-style family (iPad+TV). And
> of course many features that should adapt to the current size/shape rather
> than the platform (so narrow apps on iPad act enough like iPhone). I would
> be so tempted to name this deviceClassIsSmallScreen.

Good point! And then it 100% mimics the API we expose on top of UIUserInterfaceIdiom, but is non-adaptive. Seems like a great solution to reduce confusion. (except for the name; which we currently say "isPhoneOrWatch" but "isSmallScreen" is probably better since it does include iPod Touch as well)

> But don’t ask me how this function differs from deviceHasIPadCapability.

I don't understand this one either :) Need to do some digging.