In WK2, we access localstorage databases in UIProcess. We should take the assertion to keep UIProcess active when database is being accessed.
<rdar://problem/26685576>
Created attachment 296304 [details] Patch.
Comment on attachment 296304 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=296304&action=review > Source/WebKit2/UIProcess/ios/WebSQLiteDatabaseTrackerClient.h:32 > +class WebSQLiteDatabaseTrackerClient : public WebCore::SQLiteDatabaseTrackerClient { What about the pre-existing Source/WebKit/ios/Storage/WebSQLiteDatabaseTrackerClient.h ?
Comment on attachment 296304 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=296304&action=review >> Source/WebKit2/UIProcess/ios/WebSQLiteDatabaseTrackerClient.h:32 >> +class WebSQLiteDatabaseTrackerClient : public WebCore::SQLiteDatabaseTrackerClient { > > What about the pre-existing Source/WebKit/ios/Storage/WebSQLiteDatabaseTrackerClient.h ? What I mean is: Is there a way to merge the 2 in WebCore under platform/?
(In reply to comment #3) > Comment on attachment 296304 [details] > Patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=296304&action=review > > > Source/WebKit2/UIProcess/ios/WebSQLiteDatabaseTrackerClient.h:32 > > +class WebSQLiteDatabaseTrackerClient : public WebCore::SQLiteDatabaseTrackerClient { > > What about the pre-existing > Source/WebKit/ios/Storage/WebSQLiteDatabaseTrackerClient.h ? Under the hood, the original WebSQLiteDatabaseTrackerClient uses WK1 WebDatabaseManager, which is not available in WK2. Also, I think it would be more efficient to use the same background assertion in ProcessAssertion class, instead of creating another task assertion.
Comment on attachment 296304 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=296304&action=review >>>> Source/WebKit2/UIProcess/ios/WebSQLiteDatabaseTrackerClient.h:32 >>>> +class WebSQLiteDatabaseTrackerClient : public WebCore::SQLiteDatabaseTrackerClient { >>> >>> What about the pre-existing Source/WebKit/ios/Storage/WebSQLiteDatabaseTrackerClient.h ? >> >> What I mean is: Is there a way to merge the 2 in WebCore under platform/? > > Under the hood, the original WebSQLiteDatabaseTrackerClient uses WK1 WebDatabaseManager, which is not available in WK2. Also, I think it would be more efficient to use the same background assertion in ProcessAssertion class, instead of creating another task assertion. What about the WebSQLiteDatabaseTracker that is already in WebKit2?
(In reply to comment #6) > Comment on attachment 296304 [details] > Patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=296304&action=review > > >>>> Source/WebKit2/UIProcess/ios/WebSQLiteDatabaseTrackerClient.h:32 > >>>> +class WebSQLiteDatabaseTrackerClient : public WebCore::SQLiteDatabaseTrackerClient { > >>> > >>> What about the pre-existing Source/WebKit/ios/Storage/WebSQLiteDatabaseTrackerClient.h ? > >> > >> What I mean is: Is there a way to merge the 2 in WebCore under platform/? > > > > Under the hood, the original WebSQLiteDatabaseTrackerClient uses WK1 WebDatabaseManager, which is not available in WK2. Also, I think it would be more efficient to use the same background assertion in ProcessAssertion class, instead of creating another task assertion. > > What about the WebSQLiteDatabaseTracker that is already in WebKit2? This is only used in child processes like Network process and WebProcess, but local storage databases are handled in UIProcess.
Seems like Gavin or Brady should review, if no one else is planning to.
Comment on attachment 296304 [details] Patch. I think I'm with Chris on this; We should find a way to move both the WK1 and WK2 implementations of this to WebCore. Then SQLiteTransaction can take the assertion itself, which would always solve this problem everywhere once and for all.
(In reply to comment #9) > Comment on attachment 296304 [details] > Patch. > > I think I'm with Chris on this; We should find a way to move both the WK1 > and WK2 implementations of this to WebCore. > > Then SQLiteTransaction can take the assertion itself, which would always > solve this problem everywhere once and for all. I agree, this would be ideal. However, moving the background task assertion into WebCore means we need to indirectly add UIKit dependencies for WebCore since the assertion is in UIApplication. I will look at possible ways to achieve this without introducing UIKit dependencies.
Created attachment 298881 [details] Move database transaction background task handling to WebCore so that both WK1 and WK2 can use it.
Created attachment 298931 [details] Fix iOS build break
Created attachment 298932 [details] Move database transaction background task handling to WebCore so that both WK1 and WK2 can use it.
Comment on attachment 298932 [details] Move database transaction background task handling to WebCore so that both WK1 and WK2 can use it. This isn't quite what I had in mind. The concepts of "an in-progress SQLite transaction" and "an open SQLite database" are both WebCore concepts, and yet this patch still seems to leave tons of logic in the WebKits. I expected to see much more code down in WebCore as well as much less difference between WK1 and WK2. I'm not sure this is better.
(In reply to comment #14) > Comment on attachment 298932 [details] > Move database transaction background task handling to WebCore so that both > WK1 and WK2 can use it. > > This isn't quite what I had in mind. > > The concepts of "an in-progress SQLite transaction" and "an open SQLite > database" are both WebCore concepts, and yet this patch still seems to leave > tons of logic in the WebKits. > > I expected to see much more code down in WebCore as well as much less > difference between WK1 and WK2. > > I'm not sure this is better. As discussed in person, this patch moves the background tasks handling into WebCore, which is one step closer to our ultimate goal - keep "SQLite transaction" all inside WebCore. Since WebCore can't have dependency on UIKit, we still need to do the minimum setup in WK1 and WK2 to pass the background task handlers to WebCore, which is essentially what this patch does.
Comment on attachment 298932 [details] Move database transaction background task handling to WebCore so that both WK1 and WK2 can use it. View in context: https://bugs.webkit.org/attachment.cgi?id=298932&action=review > Source/WebCore/platform/ios/WebBackgroundTaskController.hSource/WebKit/ios/Storage/WebSQLiteDatabaseTrackerClient.mm:2 > - * Copyright (C) 2010 Apple Inc. All Rights Reserved. > + * Copyright (C) 2017 Apple Inc. All rights reserved. 2010, 2017 > Source/WebCore/platform/ios/WebSQLiteDatabaseTrackerClient.mm:2 > + * Copyright (C) 2010 Apple Inc. All Rights Reserved. 2017
Created attachment 300388 [details] Address review comments before landing.
Comment on attachment 300388 [details] Address review comments before landing. Clearing flags on attachment: 300388 Committed r211551: <http://trac.webkit.org/changeset/211551>
All reviewed patches have been landed. Closing bug.
(In reply to comment #18) > Comment on attachment 300388 [details] > Address review comments before landing. > > Clearing flags on attachment: 300388 > > Committed r211551: <http://trac.webkit.org/changeset/211551> It broke the Apple Mac cmake build, see this for details: https://build.webkit.org/builders/Apple%20El%20Capitan%20CMake%20Debug%20%28Build%29/builds/11745
fixes landed in https://trac.webkit.org/changeset/211626 and https://trac.webkit.org/changeset/211628
(In reply to comment #21) > fixes landed in https://trac.webkit.org/changeset/211626 > and https://trac.webkit.org/changeset/211628 Thank you for fixing this!