Summary: | Regression(r254856) Family Health iOS app is broken due to lack for WebSQL support | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||
Component: | New Bugs | Assignee: | Chris Dumez <cdumez> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | achristensen, beidson, benjamin, cmarcelo, ews-watchlist, ggaren, mjs, sihui_liu, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=210889 https://bugs.webkit.org/show_bug.cgi?id=211906 |
||||||
Bug Depends on: | |||||||
Bug Blocks: | 204907 | ||||||
Attachments: |
|
Description
Chris Dumez
2020-05-14 08:07:42 PDT
Created attachment 399361 [details]
Patch
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 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 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
(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. (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. Committed r261695: <https://trac.webkit.org/changeset/261695> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399361 [details]. |