Implement Console tab support with Rhino #188

potyl · 2015-05-10T21:48:26Z

This is an implementation of issue #81 using Mozilla's Rhino JavaScript library.

jasta · 2015-05-16T01:01:42Z

stetho-sample/src/debug/java/com/facebook/stetho/sample/SampleDebugApplication.java

@@ -28,10 +36,33 @@ public void onCreate() {

    long startTime = SystemClock.elapsedRealtime();
    final Context context = this;
+
+    // Setup a javascript runtime


Err, accidentally commented on the diff previously. Let's try to simplify this boilerplate (possibly with an Android-specific helper?)

Only 2 lines are mandatory:

JavaScriptRuntimeBuilder jsRuntimeBuilder = new JavaScriptRuntimeBuilder(context); Stetho.newInitializerBuilder(context).enableWebKitInspector(jsRuntimeBuilder.javaScriptInspectorModulesProvider());

The other lines where adding to show everything that can be done:

import a class

import a package

add a variable

add a custom function

What I should do instead is to add these code snippets to the documentation. Let me rework the patch with documentation & screenshots. I'll also comment on the dex increase, apk size, proguard settings.

jasta · 2015-05-16T01:02:15Z

Can you post a screenshot or a sample session that shows what you can do with this? I'm also interested in how large the dependencies are (how big is the sample APK with this change)?

potyl · 2015-05-28T20:19:49Z

Ok, I have updated my repository with a new branch. See the branch https://github.com/potyl/stetho/commits/stetho/repl-js.

The integration in the sample app is kept to a bare minimum. I have added a dedicated stetho-repl-js readme for the plugin. In thee i cover examples on how to enhance the runtime, documented the APK file sizes with tips on how to proguard the app.

Mandatory screenshot:

I also have a branch that adds this screenshot to the gh-pages, see https://github.com/potyl/stetho/tree/gh-pages.

jasta · 2015-05-28T22:32:13Z

Wow, amazing. A whopping 1.2MB jar file though, but as it is optional I think we can definitely work with that.

This PR is based off your repl branch, not repl-js. Was that on purpose? I'd like to put comments on an actual PR instead of the diffs since it's easier to manage through the review.

potyl · 2015-05-29T07:58:36Z

I've changed the branch so that the old one would still be present just in case. For now i wanted to have your opinion. I see that i forgot to add the APK sizes, I'l add that to my documentation.

I will do a reset --hard to the original branch of the PR.

As you mentioned the APK does take a load with the javascript REPL, i would suggest that we keep it optional. For instance in our main app at work I can't enable the module since it busts the dex count even with proguard :/

potyl · 2015-05-29T09:51:24Z

I have updated the read me to include apk sizes and tweaked a bit the documentation. The branch stetho/repl is now final.

I'm waiting for your comments.

jasta · 2015-05-29T17:57:17Z

Can you take a look at the Travis CI failures? Looks like simple lint errors which you can see locally by checking out the XML report generated by gradle lint

potyl · 2015-05-29T18:40:58Z

I pushed a fix to lint. I don't understand why my branch still fails. The error i get is for:

OldTargetApi: Target SDK attribute is not targeting latest version

Which is triggered by:

defaultConfig {
    minSdkVersion 9
    targetSdkVersion 21
    versionCode 1
    versionName "1.0"

But all the other projects have targetSdkVersion 21 so i don't understand why stetho-repl-js fails.
Any ideas?

potyl · 2015-06-08T20:16:02Z

Any news on this pull request?

jasta · 2015-06-09T06:31:01Z

stetho-repl-js/README.md

+public class MyApplication extends Application {
+  public void onCreate() {
+    super.onCreate();
+    JavaScriptRuntimeBuilder jsRuntimeBuilder = new JavaScriptRuntimeBuilder(this);


nit: would be nice if you bolded the lines that are unique to the repl-js integration. Not 100% sure what that syntax is tho when inside a preformatted code block...

From what I've read there's not that much that we can do in code blocks besides selecting the language. I'll try to see if I can use a table with the original block on the left and the modified block on the right.

If it would work i could mimic the output of sdiff and it will help to understand what was added.

Also, we should do this construction lazily inside the inspector modules provider. The intention of this awkward interface is to make it so that Stetho.initialize does no real work at all until a client actually connects.

Don't bother re the formatting. It's not that important :)

jasta · 2015-06-09T07:06:15Z

...ho-repl-js/src/main/java/com/facebook/stetho/inspector/console/js/JavaScriptRuntimeRepl.java

+      Object result;
+      final Context jsContext = enterJsContext();
+      try {
+        result = jsContext.evaluateString(mJsScope, expression, "chrome", 1, null);


What does "chrome" mean here?

"chrome" is the "file name" and 1 is the line number. Remember this is a JS interpreter, in case where it would find an error parsing the JS string the interpreter wants to show a nice error message such as: "compilation error in chrome (this would be afile name) at line 1 + $lineno"

So we need to tell the interpreter the name of the "file" and at which line number to start counting lines.

Ack'd, sounds good. It does appear in multiple places though so I'd prefer if it were centralized in a named constant somewhere.

jasta · 2015-06-09T07:13:37Z

Sorry for the review delay, I've been focusing on a big thorn in my side: #197 :)

jasta · 2015-06-09T07:22:11Z

Do you mind changing the PR title to something more descriptive like "Implement Console tab support with Rhino"? Makes things nicer when it gets merged :)

jasta · 2015-07-14T03:13:35Z

Any more updates? I think with the latest round of feedback implemented this should be merge-able.

potyl · 2015-07-14T07:14:43Z

I didn't finish going through all comments/remarks. Let me review the branch this evening and merge the commits. I now have a bit more free time and I can work on this.

potyl · 2015-07-15T07:35:02Z

Ok, I've rebased all my changes and removed the commit that modifies stetho-sample so that it doesn't use the javascript repl (I believe that this was requested at some point).

Code can still be improved at some point:

use a custom writeToConsole
find a way to get a handle to JsonRpcPeer for the console output

I still believe that as a preview the current basis is solid enough to be used, so we could merge this.

jasta · 2015-07-17T09:01:10Z

Oh I see, you actually changed the branch. It looks like you're really expecting to merge https://github.com/potyl/stetho/tree/stetho/js-rhino, is that right? If so, can you just push to the original repl branch from js-rhino so I can merge with github's UI?

potyl · 2015-07-17T13:37:49Z

My bad! Once I get home i'll update the proper branch

On Friday, July 17, 2015, Josh Guilfoyle notifications@github.com wrote:

Oh I see, you actually changed the branch. It looks like you're really
expecting to merge https://github.com/potyl/stetho/tree/stetho/js-rhino,
is that right? If so, can you just push to the original repl branch from
js-rhino so I can merge with github's UI?

—
Reply to this email directly or view it on GitHub
#188 (comment).

Emmanuel Rodriguez

potyl · 2015-07-17T14:32:28Z

I've pushed to the proper branch. Turns out that I also confused the commits and I previously left the commit to add the javascript to the stetho-sample app and removed the documentation on how to integrate it. This is also now corrected in the stetho/repl branch.

jasta · 2015-07-17T22:03:28Z

stetho-js-rhino/README.md

+```xml
+<dependency>
+  <groupId>com.facebook.stetho</groupId>
+  <artifactId>stetho-repl-js</artifactId>


should be stetho-js-rhino

jasta · 2015-07-17T22:05:10Z

Very minor documentation comments; this is otherwise ready to merge. Great work, I'm so excited for this feature!

This is a first draft at adding a REPL. What works: - basic JS expressions - evaluatied statements preserve their states (can create a variable and it will persist between successive calls) - return types are properly expanded by chrome's inspector - binding of android's Context (passed as `context`) - binding for classes: `System.gc()` is `Packages.java.lang.System.gc()` - console redirection with `console.log()` - `importPackage()` to import all classes under a java package - `importClass()` to import a single class - binding variables - binding functions For more details see: - https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Rhino/Scripting_Java - https://developer.mozilla.org/en-US/docs/Rhino_documentation

potyl · 2015-07-18T07:26:05Z

Thanks for spotting the 2 documentation errors. I've squashed the fix and ran a grep. No more occurrences of the old repo name.

jasta · 2015-07-18T16:13:18Z

Excellent, I'm AFK this weekend but will merge when I get back. Awesome
stuff :)

El sáb., 18 de julio de 2015 0:26, Emmanuel Rodriguez <
notifications@github.com> escribió:

Thanks for spotting the 2 documentation errors. I've squashed the fix and
ran a grep. No more occurrences of the old repo name.

—
Reply to this email directly or view it on GitHub
#188 (comment).

potyl · 2015-07-18T16:55:35Z

Thanks a lot!
Cheers.

Implement Console tab support with Rhino

ubaierbhat · 2017-07-26T14:22:28Z

EvaluatorException for sql query is this caused by Rhino?

facebook-github-bot added the CLA Signed label May 10, 2015

potyl mentioned this pull request May 12, 2015

Implement Console tab #81

Closed

jasta reviewed May 16, 2015
View reviewed changes

potyl force-pushed the stetho/repl branch from f2e0eea to 16ba254 Compare May 29, 2015 09:49

potyl force-pushed the stetho/repl branch from 16ba254 to f6d1d99 Compare May 29, 2015 18:27

potyl force-pushed the stetho/repl branch from f6d1d99 to ea24303 Compare May 29, 2015 19:33

jasta reviewed Jun 9, 2015
View reviewed changes

potyl changed the title ~~Stetho/repl~~ Implement Console tab support with Rhino Jun 9, 2015

jasta added the enhancement label Jul 17, 2015

potyl force-pushed the stetho/repl branch from ea24303 to 9b6b822 Compare July 17, 2015 14:26

jasta reviewed Jul 17, 2015
View reviewed changes

potyl added 2 commits July 18, 2015 09:18

Document briefly the JavaScript module in the main README

4566605

potyl force-pushed the stetho/repl branch from 9b6b822 to 4566605 Compare July 18, 2015 07:25

jasta added a commit that referenced this pull request Jul 20, 2015

Merge pull request #188 from potyl/stetho/repl

082ee9c

Implement Console tab support with Rhino

jasta merged commit 082ee9c into facebookarchive:master Jul 20, 2015

potyl deleted the stetho/repl branch August 6, 2015 20:11

jasta mentioned this pull request Aug 10, 2015

Display the DumperPlugin in the Developer Tools #68

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Console tab support with Rhino #188

Implement Console tab support with Rhino #188

potyl commented May 10, 2015

jasta May 16, 2015

potyl May 16, 2015

jasta commented May 16, 2015

potyl commented May 28, 2015

jasta commented May 28, 2015

potyl commented May 29, 2015

potyl commented May 29, 2015

jasta commented May 29, 2015

potyl commented May 29, 2015

potyl commented Jun 8, 2015

jasta Jun 9, 2015

potyl Jun 9, 2015

jasta Jun 9, 2015

jasta Jun 9, 2015

jasta Jun 9, 2015

potyl Jun 9, 2015

jasta Jun 9, 2015

jasta commented Jun 9, 2015

jasta commented Jun 9, 2015

jasta commented Jul 14, 2015

potyl commented Jul 14, 2015

potyl commented Jul 15, 2015

jasta commented Jul 17, 2015

potyl commented Jul 17, 2015

potyl commented Jul 17, 2015

jasta Jul 17, 2015

jasta commented Jul 17, 2015

potyl commented Jul 18, 2015

jasta commented Jul 18, 2015

potyl commented Jul 18, 2015

ubaierbhat commented Jul 26, 2017

Implement Console tab support with Rhino #188

Implement Console tab support with Rhino #188

Conversation

potyl commented May 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasta commented May 16, 2015

potyl commented May 28, 2015

jasta commented May 28, 2015

potyl commented May 29, 2015

potyl commented May 29, 2015

jasta commented May 29, 2015

potyl commented May 29, 2015

potyl commented Jun 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasta commented Jun 9, 2015

jasta commented Jun 9, 2015

jasta commented Jul 14, 2015

potyl commented Jul 14, 2015

potyl commented Jul 15, 2015

jasta commented Jul 17, 2015

potyl commented Jul 17, 2015

potyl commented Jul 17, 2015

Choose a reason for hiding this comment

jasta commented Jul 17, 2015

potyl commented Jul 18, 2015

jasta commented Jul 18, 2015

potyl commented Jul 18, 2015

ubaierbhat commented Jul 26, 2017