DevTools Network Agent implementation by petekanev · Pull Request #725 · NativeScript/android

petekanev · 2017-03-17T11:32:11Z

The Runtime will now expose __inspector object that contains methods for communicating directly with the DevTools frontend. JavaScript objects are constructed in compliance with the v8 inspector protocol and sent to the DevTools frontend when:

a request is about to be made to a url
a response is received from the server
a request is finished (normally right after a response is received)

Features:

making a request marks it on the Network tab Timeline, this allows you to select only those made in a certain period of the application's lifetime
Status codes and status text are displayed correctly for requests made from and to the device
requests show the headers they are sent with, as well as any data that is encoded in the request; requests also display headers from the response when they are completed
Image responses can be previewed; Text/documents can be expanded (when in a tree-like - xml, json format).

Current limitations:

for some reason the frontend doesn't always update the "timeline" with the time it took to complete the request. This directly affects the "Time" tab for the request, as it will show "Pending", though the response was received and the request was completed.

Associated issue: #715
Associated PRs:

Part of the #563 effort

Demo images (Click to enlarge):

Overview	Request	Response

add generated Network backend/frontend dispatchers

…ion on debugger connection

…the app is built in debug or release expose __inspector object to the global JS object that has callbacks for co mmunicating with the devtools inspector frontend rename loaderId in Page agent implementation to be matching with the one used in the network agent - NSLoaderIndentifier

update project template code to pass an additional parameter to StaticConfig

ns-bot · 2017-03-17T11:39:38Z

💚

petekanev · 2017-03-22T14:12:51Z

PR can be tested internally with the Groceries project:

git clone https://github.com/Pip3r4o/network-agent
cd network-agent
git checkout master
tns plugin add tns-core-modules@internal-preview
tns platform add android --frameworkPath http://nsbuild01.telerik.com/build/job/tns-android-PR/1038/artifact/dist/tns-android-3.0.0-2017.3.17.1.tgz
tns debug android

yyosifov · 2017-03-23T13:05:58Z


 public class Runtime {
-    private native void initNativeScript(int runtimeId, String filesPath, String nativeLibDir, boolean verboseLoggingEnabled, String packageName, Object[] v8Options, String callingDir);
+    private native void initNativeScript(int runtimeId, String filesPath, String nativeLibDir, boolean verboseLoggingEnabled, boolean isDebuggable, String packageName, Object[] v8Options, String callingDir);


8 Parameters sound like refactoring and class

Passing raw objects as parameters through JNI is much less cumbersome to deal with on the native (C++) side as it won't require precaching fieldIds for each (8 in total) of the properties.

yyosifov · 2017-03-23T13:08:10Z

+
+        v8::Local<v8::Object> argsObj = args[0]->ToObject();
+
+        if ((!argsObj->Has(context, ArgConverter::ConvertToV8String(isolate, "requestId")).FromMaybe(false) ||


do you think it's ok these to be constants? we repeat them twice here

Do you think all of the strings that we check for should be constants, or just some of them?

yyosifov · 2017-03-23T13:08:41Z

+
+        v8::Local<v8::Object> argsObj = args[0]->ToObject();
+
+        if ((!argsObj->Has(context, ArgConverter::ConvertToV8String(isolate, "requestId")).FromMaybe(false) ||


why not use a for statement - we check for 4 different parameters in the same pattern

The v8::Object has no iterator, and while we can go over all of its properties (https://github.com/NativeScript/android-runtime/blob/master/runtime/src/main/jni/include/v8.h#L3086 GetOwnPropertyNames), we will still have to validate each one.

yyosifov · 2017-03-23T13:09:32Z

+                "Not all parameters are present in the object argument in the call to ResponseReceived! Required params: 'requestId', `timestamp`, `type`, `response`");
+        }
+
+        auto requestId = argsObj->Get(context, ArgConverter::ConvertToV8String(isolate, "requestId")).ToLocalChecked()->ToString();


can we extract the validation + extraction logic to a function which returns a struct/class?

We can certainly move the validation and extraction in separate methods, but it won't be one universal method that can validate each and every callback, so I don't think there will be use of separate validation.

yyosifov · 2017-03-23T13:13:26Z

+
+static void LoadingFinishedCallback(const v8::FunctionCallbackInfo<v8::Value>& args) {
+    try {
+        auto networkAgentInstance = V8NetworkAgentImpl::Instance;


This is seen at least 3 times. Can we make a Guard method and extract this functionality there?

I am not sure making a Guard method will make the code more readable than it is right now. We will have to check whether the guard has passed, and code will look almost the same.

yyosifov · 2017-03-23T13:16:00Z

+NetworkRequestData::NetworkRequestData()
+    : m_data(""),
+      m_hasTextContent(true) { }
+NetworkRequestData::NetworkRequestData(std::string data, bool hasTextContent)


strange identation

yyosifov · 2017-03-23T13:16:12Z

+        nsEx.ReThrowToV8();
+    }
+}
+}


strange identation for the 3 }}}

Code is formatted according to the tool used - astyle. https://github.com/NativeScript/android-runtime/blob/master/tools/astyle.js#L11

yyosifov · 2017-03-23T13:17:19Z

+        }
+
+        void setHasTextContent(const bool hasTextContent) {
+            m_hasTextContent = hasTextContent;


Do we have a convention to use this-> for instance members?

yyosifov · 2017-03-23T13:18:26Z

+namespace v8_inspector {
+
+namespace NetworkAgentState {
+static const char networkEnabled[] = "networkEnabled";


identation is not ok here

Indentation is according to the formatting tool used - astyle. https://github.com/NativeScript/android-runtime/blob/master/tools/astyle.js#L11

yyosifov · 2017-03-23T13:18:53Z

+namespace v8_inspector {
+
+namespace NetworkAgentState {
+static const char networkEnabled[] = "networkEnabled";


aren't constants upperCase-named by convention? You can give me a convention for the C++ code we right, so that I don't write dummy comments :)

I've simply used the same convention that was in place by the default v8 inspector implementations that came from the chromium project -> https://github.com/NativeScript/android-runtime/blob/master/runtime/src/main/jni/v8_inspector/src/inspector/v8-profiler-agent-impl.cc#L24

https://chromium.googlesource.com/v8/v8.git/+/refs/heads/5.5.372/src/inspector/v8-runtime-agent-impl.cc#52

yyosifov · 2017-03-23T13:19:57Z

                try {
                    v8Inspector = new AndroidJsV8Inspector(app, logger);
                    v8Inspector.start();
-                    File debugBreakFile = new File("/data/local/tmp", app.getPackageName() + "-debugger-started");


what's the reasoning here?

Oops, must have committed this by mistake. Nice catch!

Plamen5kov

In modules implementation: https://github.com/NativeScript/NativeScript/blob/master/tns-core-modules/debugger/debugger.ts#L169
Please check that mimetype exists before doing operations on it.

petekanev · 2017-03-24T13:24:19Z

@Plamen5kov addressed in NativeScript/NativeScript#3867

ns-bot · 2017-03-24T13:46:19Z

💚

ns-bot · 2017-03-24T14:45:07Z

💚

Plamen5kov

After running builds and a fix for the commented issue.

Plamen5kov · 2017-03-24T12:53:26Z

+
+    inspectorJSObject->Set(ArgConverter::ConvertToV8String(isolate, "responseReceived"), FunctionTemplate::New(isolate, NetworkDomainCallbackHandlers::ResponseReceivedCallback));
+    inspectorJSObject->Set(ArgConverter::ConvertToV8String(isolate, "requestWillBeSent"), FunctionTemplate::New(isolate, NetworkDomainCallbackHandlers::RequestWillBeSentCallback));
+    inspectorJSObject->Set(ArgConverter::ConvertToV8String(isolate, "dataForRequestId"), FunctionTemplate::New(isolate, NetworkDomainCallbackHandlers::DataForRequestId));


DataForRequestId -> DataForRequestIdCallback

Plamen5kov · 2017-03-24T13:28:01Z

+static const char* FrameId = "NSFrameIdentifier";
+static const char* LoaderId = "NSLoaderIdentifier";
+
+static void ResponseReceivedCallback(const v8::FunctionCallbackInfo<v8::Value>& args) {


Extract all methods to a source file, and leave only the API in the header.
Please add comments on all methods, so their function is clear without reading the code. The name's just not verbose enough.

Callbacks have since been documented. I'll just extract them as static members of a new class now.

Plamen5kov · 2017-03-24T14:18:45Z

@@ -19,7 +20,8 @@ JsV8InspectorClient::JsV8InspectorClient(v8::Isolate* isolate)
      session_(nullptr),
      connection(nullptr),
      context_(),


initialize context with default value

It would be wrong to initialize the persistent container with value different than the default (0).

ns-bot · 2017-03-27T07:25:54Z

💚

petekanev added 4 commits March 16, 2017 09:00

update inspector protocol to support network domain

20ecd93

add generated Network backend/frontend dispatchers

add network domain implementation and enable it in the inspector sess…

2918a26

…ion on debugger connection

add exception handling in network agent callbacks

598d1e8

update project template code to pass an additional parameter to StaticConfig

petekanev requested a review from Plamen5kov March 17, 2017 11:32

petekanev self-assigned this Mar 17, 2017

yyosifov reviewed Mar 23, 2017

View reviewed changes

Plamen5kov reviewed Mar 24, 2017

View reviewed changes

revert changes to RuntimeHelper class

c485553

add comments documenting the new callbacks

2f0062b

Plamen5kov approved these changes Mar 27, 2017

View reviewed changes

extracted network agent callbacks from utility file to class

d2c5eb4

petekanev merged commit b753387 into master Mar 27, 2017

petekanev deleted the pete/network-agent branch March 27, 2017 08:29


		v8::Local<v8::Object> argsObj = args[0]->ToObject();

		if ((!argsObj->Has(context, ArgConverter::ConvertToV8String(isolate, "requestId")).FromMaybe(false) \|\|

Uh oh!

Conversation

petekanev commented Mar 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ns-bot commented Mar 17, 2017

Uh oh!

petekanev commented Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yyosifov Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Plamen5kov left a comment

Choose a reason for hiding this comment

Uh oh!

petekanev commented Mar 24, 2017

Uh oh!

ns-bot commented Mar 24, 2017

Uh oh!

ns-bot commented Mar 24, 2017

Uh oh!

Plamen5kov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ns-bot commented Mar 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

petekanev commented Mar 17, 2017 •

edited

Loading

petekanev commented Mar 22, 2017 •

edited

Loading

yyosifov Mar 23, 2017 •

edited

Loading