Added support of Database Domain extensions #264
Conversation
| mContext = context; | ||
| } | ||
|
|
||
| protected abstract void onRegistered(JsonRpcPeer peer); |
There was a problem hiding this comment.
I'm not sure it's required to make this abstraction work at the peer manager level. Probably good enough to just have an onFirstPeerConnected and onLastPeerDisconnected method, no?
There was a problem hiding this comment.
Please check the latest commit. As you suggested, implementor shouldn't implement this class anymore.
… api for registering and viewing providers
|
@jasta Thanks for your review! :). So for the round 2 of reviews these are the changes that I made based on your comments. I hope, it is the way you intended, if not, please correct me.
In the sample project, the |
| .build()) | ||
| .build(); | ||
|
|
||
| Database database = new Database(); |
There was a problem hiding this comment.
Sorry for the late review, I was working on getting stetho-1.2.0 out with a much improved API to support this kind of customization. I think we can now approach this with a new, much stronger API:
new DefaultInspectorModulesProvider()
.provideDatabaseDriver(new ContentProviderDatabaseDriver(...))
.finish();
This implies that the default provider is install installed. If the default is to be configured, they can use the existing (as of 1.2.0) method .databaseFiles(...) and just provide a no-op files lister.
|
This is looking really great. I think with a couple of refinements we'd be ready to go. Sorry again for the delay :( |
|
Thanks for your comment @jasta. Sorry for long response to your response :) Just couldn't leave this beach https://www.facebook.com/pages/Paralia-Stefanou-Seitan-limania-Chania/163349380464470 on my vacation. Now, back to work :) I will take 1.2.0 and make the necessary updates based on latest lib changes and your comments. Will update you soon. Thanks. |
Conflicts: stetho/src/main/java/com/facebook/stetho/Stetho.java
…ization. Refined DatabaseDriver and removed duplicated registration code.
|
@jasta Please check latest changes. The changes are based on your last comments.
|
|
@jasta do you want me to make any changes? |
| @@ -42,7 +49,56 @@ private void initializeStetho(final Context context) { | |||
| .finish(); | |||
| } | |||
| }) | |||
| .enableWebKitInspector(Stetho.defaultInspectorModulesProvider(context)) | |||
| .enableWebKitInspector(new ExtInspectorModulesProvider(context)) | |||
There was a problem hiding this comment.
Not sure why we need a separate class here. It should be possible to write:
.enableWebKitInspector(
new DefaultInspectorModulesBuilder()
.databaseDriver(createBlaBla())
.finish());This was the original intention of the new builder API at least.
| return tableName; | ||
| } | ||
| } | ||
| return ""; |
There was a problem hiding this comment.
Prefer null and to mark this method @Nullable. Using magic string constants like "" can end up very hard to track down when bugs occur.
|
This is very well done, thank you. I again apologize for such a long delay in review. After getting through the full diff series I really only have nit-pick comments and I'm happy to just take those as notes to follow-up myself to do some tidy work or if you want you can follow-up after the fact. I otherwise am comfortable merging as-is. |
Added support of Database Domain extensions
|
@jasta Thanks for your comments and review. Glad that this PR made it's way to the master :) We are using stetho in debug build types and this is one of the helpful and time saving tools! |
|
@sromku of course, totally my fault making you wait so long. We're looking at a 1.3.0 release this month so it will be nice to finally get this turned around for you in an official release :) |
|
Much thanks! :) |
Based on ticket #258