Bug 211896 - Regression(r254856) Family Health iOS app is broken due to lack for WebSQL support
Summary: Regression(r254856) Family Health iOS app is broken due to lack for WebSQL su...
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: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 204907
  Show dependency treegraph
 
Reported: 2020-05-14 08:07 PDT by Chris Dumez
Modified: 2020-05-14 10:52 PDT (History)
9 users (show)

See Also:


Attachments
Patch (5.50 KB, patch)
2020-05-14 08:34 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2020-05-14 08:07:42 PDT
Family Health iOS app is broken due to lack for WebSQL support.
Comment 1 Chris Dumez 2020-05-14 08:08:21 PDT
<rdar://problem/63025045>
Comment 2 Chris Dumez 2020-05-14 08:34:33 PDT
Created attachment 399361 [details]
Patch
Comment 3 Chris Dumez 2020-05-14 08:36:08 PDT
Comment on attachment 399361 [details]
Patch

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

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:399
> +    bool webSQLEnabled = (IOSApplication::isFamilyHealthApp() || IOSApplication::isJesusCalling()) && applicationSDKVersion() < DYLD_IOS_VERSION_14_0;

Note that personally, I think we should use:
bool webSQLEnabled = applicationSDKVersion() < DYLD_IOS_VERSION_14_0;

without any application checks. Please let me know if you agree / disagree.
Comment 4 Maciej Stachowiak 2020-05-14 08:42:08 PDT
Comment on attachment 399361 [details]
Patch

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

>> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:399
>> +    bool webSQLEnabled = (IOSApplication::isFamilyHealthApp() || IOSApplication::isJesusCalling()) && applicationSDKVersion() < DYLD_IOS_VERSION_14_0;
> 
> Note that personally, I think we should use:
> bool webSQLEnabled = applicationSDKVersion() < DYLD_IOS_VERSION_14_0;
> 
> without any application checks. Please let me know if you agree / disagree.

Lack of application check creates a risk we'll have to keep rolling the OS version forward into the future, which makes it harder to ever delete WebSQL. So I think limiting it to specific apps is probably better. Though perhaps if there's enough apps, we could factor it out into a function so we don't end up with an unwieldy or expression.
Comment 5 Maciej Stachowiak 2020-05-14 08:46:12 PDT
Comment on attachment 399361 [details]
Patch

Do we have a way to test this type of change, via API tests for instance?

Please add a test if possible.

Otherwise, r=me
Comment 6 Chris Dumez 2020-05-14 08:47:17 PDT
(In reply to Maciej Stachowiak from comment #5)
> Comment on attachment 399361 [details]
> Patch
> 
> Do we have a way to test this type of change, via API tests for instance?
> 
> Please add a test if possible.
> 
> Otherwise, r=me

As far as I know, we still have test coverage for WebSQL being enabled.
Comment 7 Chris Dumez 2020-05-14 08:52:53 PDT
(In reply to Maciej Stachowiak from comment #4)
> Comment on attachment 399361 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399361&action=review
> 
> >> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:399
> >> +    bool webSQLEnabled = (IOSApplication::isFamilyHealthApp() || IOSApplication::isJesusCalling()) && applicationSDKVersion() < DYLD_IOS_VERSION_14_0;
> > 
> > Note that personally, I think we should use:
> > bool webSQLEnabled = applicationSDKVersion() < DYLD_IOS_VERSION_14_0;
> > 
> > without any application checks. Please let me know if you agree / disagree.
> 
> Lack of application check creates a risk we'll have to keep rolling the OS
> version forward into the future, which makes it harder to ever delete
> WebSQL. So I think limiting it to specific apps is probably better. Though
> perhaps if there's enough apps, we could factor it out into a function so we
> don't end up with an unwieldy or expression.

Why would we keep rolling the IOS version forward? We drop support for WebSQL in a specific SDK, and then apps have to update if they want to build with the new SDK.

Checking only for specific apps is risky because we may not notice quickly all the apps that are broken without WebSQL or the breakage may be more subtle for certain apps. Also, it is quite a waste of engineering time to investigate app breakage and bisect it to this particular WebSQL change. I wasted a day on this and don't wish this on anybody else.
Comment 8 EWS 2020-05-14 09:11:30 PDT
Committed r261695: <https://trac.webkit.org/changeset/261695>

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