Comment on attachment 582267 [details] [diff] [review] Close connection in nsPermissionManager Review of attachment 582267 [details] [diff] [review]: ----------------------------------------------------------------- looks ok

Comment on attachment 582267 [details] [diff] [review] Close connection in nsPermissionManager Review of attachment 582267 [details] [diff] [review]: ----------------------------------------------------------------- ::: extensions/cookie/nsPermissionManager.cpp @@ +797,5 @@ > mStmtInsert = nsnull; > mStmtDelete = nsnull; > mStmtUpdate = nsnull; > + mozilla::DebugOnly<nsresult> rv = mDBConn->Close(); > + MOZ_ASSERT(NS_SUCCEEDED(rv)); You should add a helper method to this and make this and `RemoveAllInternal` call the helper. We were doing it right in one place, but not the other.

Comment on attachment 582835 [details] [diff] [review] Close connection in nsPermissionManager Review of attachment 582835 [details] [diff] [review]: ----------------------------------------------------------------- ::: extensions/cookie/nsPermissionManager.cpp @@ +620,5 @@ > return rv; > } > > +void > +nsPermissionManager::CloseDBInternal() I don't like the Internal suffix here, we usually use it when there's another exposed method with the same name, that's not this case, just CloseDB() will be fine afaict. @@ +646,1 @@ > mDBConn = nsnull; you can move this inside the helper "if (mDBConn)" scope ::: extensions/cookie/nsPermissionManager.h @@ +221,5 @@ > PRUint32 aExpireType, > PRInt64 aExpireTime, > const PRUnichar *aData); > void NotifyObservers(nsIPermission *aPermission, const PRUnichar *aData); > + void CloseDBInternal(); this may like a nice but simple javadoc explaining what it does.

This implement all the comments, except for the javadoc, since the file had no javadoc I just added a plain comment. https://tbpl.mozilla.org/?tree=Try&rev=8e4a8b9a5073

Comment on attachment 582972 [details] [diff] [review] Close connection in nsPermissionManager Review of attachment 582972 [details] [diff] [review]: ----------------------------------------------------------------- I don't have further comments, I'm forwarding final review to either swilsh or dwitte, on a first come first serve handling, since sdwilsh appears to be on vacation

Comment on attachment 582972 [details] [diff] [review] Close connection in nsPermissionManager Review of attachment 582972 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine to me too. I can't technically r+ this, so f+ at taras's request.

Comment on attachment 582972 [details] [diff] [review] Close connection in nsPermissionManager Looks good, thanks. r=dwitte