Summary: | WebKit2: Need a WebKit2 equivalent of the WebKit1 WebDatabaseManager | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jessie Berlin <jberlin> | ||||
Component: | WebKit2 | Assignee: | Jessie Berlin <jberlin> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | abarth, aroben, beidson, eric, jberlin, sam, webkit.review.bot | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Jessie Berlin
2010-12-06 17:38:52 PST
Created attachment 76147 [details]
Patch
Comment on attachment 76147 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76147&action=review Is it possible to add some API tests for this? You should probably have Sam give the new API a once-over. > WebKit2/DerivedSources.make:28 > + $(WebKit2)/WebProcess/WebCoreSupport \ Is this really needed? We don't have any .messages.in files in WebCoreSupport, do we? > WebKit2/UIProcess/WebDatabaseManagerProxy.cpp:64 > + m_databaseOriginsCallbacks.set(callbackID, callback.get()); You can use callback.release() here to save a ref/deref. > WebKit2/UIProcess/WebDatabaseManagerProxy.cpp:81 > + securityOrigins->append(WebSecurityOrigin::create(originIdentifiers[i]).leakRef()); Who is in charge of dereffing these objects? (In reply to comment #2) > (From update of attachment 76147 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76147&action=review > > Is it possible to add some API tests for this? Probably, is it ok to do that in a subsequent patch? > > You should probably have Sam give the new API a once-over. We talked about it earlier, but I think he is giving it another look now. > > > WebKit2/DerivedSources.make:28 > > + $(WebKit2)/WebProcess/WebCoreSupport \ > > Is this really needed? We don't have any .messages.in files in WebCoreSupport, do we? The WebDatabaseManager.messages.in is in WebCoreSupport. Do you think I should make a separate directory / filter for Databases and move all of the WebDatabaseManager stuff in there? > > > WebKit2/UIProcess/WebDatabaseManagerProxy.cpp:64 > > + m_databaseOriginsCallbacks.set(callbackID, callback.get()); > > You can use callback.release() here to save a ref/deref. Done. > > > WebKit2/UIProcess/WebDatabaseManagerProxy.cpp:81 > > + securityOrigins->append(WebSecurityOrigin::create(originIdentifiers[i]).leakRef()); > > Who is in charge of dereffing these objects? Changed to: size_t originIdentifiersCount = originIdentifiers.size(); Vector<RefPtr<APIObject> > securityOrigins(originIdentifiersCount); for (size_t i = 0; i < originIdentifiersCount; ++i) securityOrigins[i] = WebSecurityOrigin::create(originIdentifiers[i]); callback->performCallbackWithReturnValue(ImmutableArray::adopt(securityOrigins).get()); Comment on attachment 76147 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76147&action=review >>> WebKit2/DerivedSources.make:28 >>> + $(WebKit2)/WebProcess/WebCoreSupport \ >> >> Is this really needed? We don't have any .messages.in files in WebCoreSupport, do we? > > The WebDatabaseManager.messages.in is in WebCoreSupport. Do you think I should make a separate directory / filter for Databases and move all of the WebDatabaseManager stuff in there? It probably doesn't matter very much. I don't feel bothered enough by it to have you change it. >>> WebKit2/UIProcess/WebDatabaseManagerProxy.cpp:81 >>> + securityOrigins->append(WebSecurityOrigin::create(originIdentifiers[i]).leakRef()); >> >> Who is in charge of dereffing these objects? > > Changed to: > > size_t originIdentifiersCount = originIdentifiers.size(); > Vector<RefPtr<APIObject> > securityOrigins(originIdentifiersCount); > > for (size_t i = 0; i < originIdentifiersCount; ++i) > securityOrigins[i] = WebSecurityOrigin::create(originIdentifiers[i]); > > callback->performCallbackWithReturnValue(ImmutableArray::adopt(securityOrigins).get()); Beautiful! Comment on attachment 76147 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76147&action=review > WebKit2/UIProcess/WebDatabaseManagerProxy.h:47 > +typedef GenericCallback<WKArrayRef, ImmutableArray*> DatabaseOriginsCallback; I don't think the ImmutableArray* type is needed here. It should be inferred. > WebKit2/UIProcess/WebDatabaseManagerProxy.h:76 > + HashMap<uint64_t, RefPtr<DatabaseOriginsCallback> > m_databaseOriginsCallbacks; > + > +}; > + Extra newline. > WebKit2/UIProcess/WebPageProxy.h:467 > +template<typename T> > +void invalidateCallbackMap(HashMap<uint64_t, T>& map) > +{ > + Vector<T> callbacksVector; > + copyValuesToVector(map, callbacksVector); > + for (size_t i = 0, size = callbacksVector.size(); i < size; ++i) > + callbacksVector[i]->invalidate(); > + map.clear(); > +} > + > } // namespace WebKit > Let's move this to GenericCallback.h (In reply to comment #5) > (From update of attachment 76147 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76147&action=review > > > WebKit2/UIProcess/WebDatabaseManagerProxy.h:47 > > +typedef GenericCallback<WKArrayRef, ImmutableArray*> DatabaseOriginsCallback; > > I don't think the ImmutableArray* type is needed here. It should be inferred. Removed. > > > WebKit2/UIProcess/WebDatabaseManagerProxy.h:76 > > + HashMap<uint64_t, RefPtr<DatabaseOriginsCallback> > m_databaseOriginsCallbacks; > > + > > +}; > > + > > Extra newline. Removed. > > > WebKit2/UIProcess/WebPageProxy.h:467 > > +template<typename T> > > +void invalidateCallbackMap(HashMap<uint64_t, T>& map) > > +{ > > + Vector<T> callbacksVector; > > + copyValuesToVector(map, callbacksVector); > > + for (size_t i = 0, size = callbacksVector.size(); i < size; ++i) > > + callbacksVector[i]->invalidate(); > > + map.clear(); > > +} > > + > > } // namespace WebKit > > > > Let's move this to GenericCallback.h Done. http://trac.webkit.org/changeset/73808 might have broken Qt Linux Release (In reply to comment #7) > http://trac.webkit.org/changeset/73808 might have broken Qt Linux Release Attempting to fix now. Comment on attachment 76147 [details] Patch Committed in r73808 http://trac.webkit.org/changeset/73808 Qt build fix landed in r73814 http://trac.webkit.org/changeset/73814 I will add tests in a subsequent patch. There needs to be some work done so that deleteAllDatabases in the tests does not delete all the databases for the person running the tests. |