Closed Bug 1390037 Opened 7 years ago Closed 7 years ago

create a connector for connect to chrome remote debugging protocol on netmonitor.

Categories

(DevTools :: Netmonitor, enhancement, P2)

Product:
DevTools
Component:
Netmonitor
Type:
enhancement

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: locke12456, Assigned: locke12456)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

Bug 1390037 - Implement chrome connector to support Chrome RDP for netmonitor.
59 bytes, text/x-review-board-request
Honza
: review+
Details
Bug 1390037 - Code clean up;
59 bytes, text/x-review-board-request
rickychien
: review+
Details
Bug 1390037 - Create readme file;
59 bytes, text/x-review-board-request
rickychien
: review+
Details 
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.90 Safari/537.36




Expected results:

implement a Chrome connector for monitoring chrome remote debugging protocol.
Component: Untriaged → Developer Tools: Netmonitor
Assignee: nobody → locke12456
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P2
Attachment #8899725 - Flags: review?(rchien)
Attachment #8899725 - Flags: review?(odvarko)
Attachment #8899725 - Attachment is obsolete: true
Requirements
Google Chrome >= 59.0.3071.115

setup
1. Using DevTools (Chrome) as protocol client.
command:
google-chrome --remote-debugging-port=9222 -v

2. Open the netmonitor services
more details:
http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/README.md
Attachment #8900581 - Attachment is obsolete: true
Attachment #8900581 - Flags: review?(rchien)
Attachment #8900581 - Flags: review?(odvarko)
Comment on attachment 8899725 [details]
Bug 1390037 - Implement chrome connector to support Chrome RDP for netmonitor.

https://reviewboard.mozilla.org/r/171050/#review179520

::: devtools/client/netmonitor/src/connector/chrome-connector.js:5
(Diff revision 4)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";

nit: add a line in between

::: devtools/client/netmonitor/src/connector/chrome-connector.js:7
(Diff revision 4)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +const { ACTIVITY_TYPE } = require("../constants");
> +const { CDPConnector } = require("./chrome/events");

nit: add a line in between

::: devtools/client/netmonitor/src/connector/chrome-connector.js:28
(Diff revision 4)
> +    // currently all events are about "navigation" is not support on CDP
> +    this.connector.willNavigate(this.willNavigate);

Please extract the code of willNavigate() to here instead of calling it as a separate function. 

As a result, it unnecessary to comply with the pattern of firefox-connector and we don't have to create an useless willNavigate method.

::: devtools/client/netmonitor/src/connector/chrome-connector.js:31
(Diff revision 4)
> +    this.connector = new CDPConnector();
> +    this.connector.setup(tabConnection, this.actions);
> +    // currently all events are about "navigation" is not support on CDP
> +    this.connector.willNavigate(this.willNavigate);
> +  }
> +  async disconnect() {

nit: add a line in between.

Please check and make sure that all functions are separate by an empty line to make our code style more better.

::: devtools/client/netmonitor/src/connector/chrome-connector.js:40
(Diff revision 4)
> +  async fetchResponseCookies(responseCookies) {
> +    // currently Network.getCookie can't mapping to any response header via id or domain
> +  }
> +
> +  async fetchRequestCookies(requestCookies) {
> +    // currently Network.getCookie can't mapping to any request header via id or domain
> +  }

Remove these empty methods if chrome doesn't support them.

nit: add a line in between.

::: devtools/client/netmonitor/src/connector/chrome-connector.js:48
(Diff revision 4)
> +     * Triggers a specific "activity" to be performed by the frontend.
> +     * This can be, for example, triggering reloads or enabling/disabling cache.
> +     *
> +     * @param {number} type The activity type. See the ACTIVITY_TYPE const.
> +     * @return {object} A promise resolved once the activity finishes and the frontend
> +     *                  is back into "standby" mode.
> +     */

Please fix the code indent of this comment

::: devtools/client/netmonitor/src/connector/chrome-connector.js:57
(Diff revision 4)
> +    let standBy = () => {
> +      this.currentActivity = ACTIVITY_TYPE.NONE;
> +    };

Insead of creating an inner standBy() method without resuing elsewhere, we'd probably get rid of this and define it as an anonymous arrow function within then() directly.

::: devtools/client/netmonitor/src/connector/chrome-connector.js:70
(Diff revision 4)
> +          () => this.connector.Page.reload().then(standBy)
> +        );
> +    }
> +    this.currentActivity = ACTIVITY_TYPE.NONE;
> +    return Promise.reject(new Error("Invalid activity type"));
> +  }

nit: add a line in between.

::: devtools/client/netmonitor/src/connector/chrome-connector.js:77
(Diff revision 4)
> +     * Send a HTTP request data payload
> +     *
> +     * @param {object} data data payload would like to sent to backend
> +     * @param {function} callback callback will be invoked after the request finished
> +     */
> +  sendHTTPRequest(data, callback) {

nit: add a line in between.

::: devtools/client/netmonitor/src/connector/chrome-connector.js:80
(Diff revision 4)
> +     * @param {function} callback callback will be invoked after the request finished
> +     */
> +  sendHTTPRequest(data, callback) {
> +    // TODO : not support. currently didn't provide this feature in CDP API.
> +  }
> +  setPreferences() {

nit: add a line in between.

::: devtools/client/netmonitor/src/connector/chrome-connector.js:83
(Diff revision 4)
> +    // TODO : not support. currently didn't provide this feature in CDP API.
> +  }
> +  setPreferences() {
> +    // TODO : implement.
> +  }
> +  viewSourceInDebugger() {

nit: add a line in between.
Comment on attachment 8899725 [details]
Bug 1390037 - Implement chrome connector to support Chrome RDP for netmonitor.

https://reviewboard.mozilla.org/r/171050/#review181610

Locke, thanks for your contribution on netmonitor!

This looks like a big patch and I can see there are a few modules created under src/connector/chrome/ folder. I'm a bit confused of those modules structure and I also got some questions like why we need a bulk-loader and what it's supposed to achieve? Perhaps you can try to give an explanation on bugzilla comment to elaborate the architecture behind your scheme and what's problem you're trying to solve?

For example:

* connector/chrome/bulk-loader.js

In order to reduce the frequency of network request update, bulk-loader is going to introduce a task schedule mechanism to queue and delay the upcoming network request. This is helpful to speed up performance and mitigate UI frozen when loading large website.

This can help reviewer a lots when they want to understand a bunch of new code. Thanks:)
Attachment #8899725 - Flags: review?(rchien)
* connector/chrome/utils.js

Payload is like a factory it will mapping all payload data when network request update from CDP.  for example, chrome connector had a method called 'onNetworkUpdate' in event.js, chrome connector input request data then payload will output reqest, header and post data.

* connector/chrome/events.js

CDPConnector handle Network and Page events from Chrome remote debug protocol. it will used utils.js and bulk-loader.js when received request/response data from CDP.
for example,  'onNetworkUpdate' is handled request received from CDP. When data received it will create and mapping request data to payload, after data mapped it will add to bulk-loader then data will add/update to netmonitor request list.
This is great progress Locke!

Note that running Net monitor panel on top of the Launchpad requires patches from bug 1395834.
(webpack configuration needs update)

Btw. I've been successfully testing the connection to Chrome and I am looking at the code now.

Honza.
Severity: normal → enhancement
Attachment #8909897 - Attachment is obsolete: true
Attachment #8909897 - Flags: review?(rchien)
Attachment #8909897 - Flags: review?(odvarko)
Comment on attachment 8899725 [details]
Bug 1390037 - Implement chrome connector to support Chrome RDP for netmonitor.

https://reviewboard.mozilla.org/r/171050/#review186634

I think this patch is good start and it's landable in the current form.

R+, but some comments

* There are a lot of style issues. Instead of pointing out every missing line and code-formatting glitch I made a follow up patch fixing it.
* There are some TODO comment thorought the code. Locke, can you please do a summary (as a comment here in this report) and suggest what should be the next steps.
* We need a new README.md file (in connector dir) containing instructions (can be a follow up bug report)

Honza
Attachment #8899725 - Flags: review?(odvarko) → review+
Comment on attachment 8909898 [details]
Bug 1390037 - Code clean up;

https://reviewboard.mozilla.org/r/181388/#review186876

Thank you Honza, I like this code clean up. Although there is something we can improve, we are able to see netmonitor running on Chrome through CDP connector! Thanks
Attachment #8909898 - Flags: review?(rchien) → review+
Comment on attachment 8909897 [details]
Bug 1390037 - Implement chrome connector to support Chrome RDP for netmonitor.

https://reviewboard.mozilla.org/r/181386/#review186878
Attachment #8909897 - Flags: review+
(In reply to Ricky Chien [:rickychien] from comment #17)
> Comment on attachment 8909898 [details]
> Bug 1390037 - Code clean up;
> 
> https://reviewboard.mozilla.org/r/181388/#review186876
> 
> Thank you Honza, I like this code clean up. Although there is something we
> can improve, we are able to see netmonitor running on Chrome through CDP
> connector! Thanks

Thanks for quick review!

I attached one more patch introducing the readme file.

Honza
Comment on attachment 8910128 [details]
Bug 1390037 - Create readme file;

https://reviewboard.mozilla.org/r/181616/#review186952

LGTM. thanks
Attachment #8910128 - Flags: review?(rchien) → review+
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7bd14c0f6406
Implement chrome connector to support Chrome RDP for netmonitor. r=Honza
OK, landing this beast. Great work Locke!

@locke: Please read comment #16

Honza
Flags: needinfo?(locke12456)
TODO list:
* Cookie
Cookies information currently couldn't mapping to request list because getCookies method not contain id, if want to mapping request list, need a id to identify but this method only return a list. 

* Security detail
refer to prototype:
https://github.com/locke12456/ChromeConnector/blob/develop/chrome/response.js#L46

The security information haven't contain fingerprint from CDP, and Security API didn't on release version of CDP, so I didn't implement on this patch.

* Timing
refer to prototype:
https://github.com/locke12456/ChromeConnector/blob/develop/chrome/response.js#L124

CDP provide many timing information, currently only used connect, send, dns and receiveHeaders in Chrome connector, maybe have other information could show on netmonitor.

* ResponseContent
Here has a property called encodedDataLength map to transferredSize, I'm not sure that is correct, but looks no problem so far. need to verify this property.

* Request
I'm not sure how to identify XHR, so I do set it to false. need to verify this.
Flags: needinfo?(locke12456) → needinfo?(odvarko)
(In reply to Jan Honza Odvarko [:Honza] from comment #23)
> OK, landing this beast. Great work Locke!
> 
> @locke: Please read comment #16
> 
> Honza

Hi Honza
Thanks you review. :)
I have write some introduction on github.
https://github.com/locke12456/ChromeConnector

Could use this one?
(In reply to Locke Chen from comment #24)
> TODO list:
Thanks for the list Locke!

I created meta bug 1401467 for further work on Netmonitor's support for CDP.

(In reply to Locke Chen from comment #25)
> Thanks you review. :)
> I have write some introduction on github.
> https://github.com/locke12456/ChromeConnector
> Could use this one?
Yes, I made a comment the meta bug.

Honza
Flags: needinfo?(odvarko)
(In reply to Jan Honza Odvarko [:Honza] from comment #26)
> (In reply to Locke Chen from comment #24)
> > TODO list:
> Thanks for the list Locke!
> 
> I created meta bug 1401467 for further work on Netmonitor's support for CDP.
> 
> (In reply to Locke Chen from comment #25)
> > Thanks you review. :)
> > I have write some introduction on github.
> > https://github.com/locke12456/ChromeConnector
> > Could use this one?
> Yes, I made a comment the meta bug.
> 
> Honza

Awesome! That's looks more clearly.
you can also assign to me if the task is not urgent. I think I also could use my free time do that.
Thanks a lot!
(In reply to Locke Chen from comment #27)
> Awesome! That's looks more clearly.
> you can also assign to me if the task is not urgent. I think I also could
> use my free time do that.
Excellent!

The bug I created is 'meta', which is now used to collect all info and cover the entire 'Net monitor support for CDP' goal.
We can now report further bugs for specific tasks (the meta will depend on those).

It looks like there is already space for other 6 bugs (mostly coming from your TODO list).
I'll start filing those bug reports and I'll ping you as soon as they exist 

Consequently we can fix them step by step.

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #28)
> (In reply to Locke Chen from comment #27)
> > Awesome! That's looks more clearly.
> > you can also assign to me if the task is not urgent. I think I also could
> > use my free time do that.
> Excellent!
> 
> The bug I created is 'meta', which is now used to collect all info and cover
> the entire 'Net monitor support for CDP' goal.
> We can now report further bugs for specific tasks (the meta will depend on
> those).
> 
> It looks like there is already space for other 6 bugs (mostly coming from
> your TODO list).
> I'll start filing those bug reports and I'll ping you as soon as they exist 
> 
> Consequently we can fix them step by step.
> 
> Honza

No problem, or just assign to me. I'll take a look for that. :)
but maybe that will more slowly than now. XD
https://hg.mozilla.org/mozilla-central/rev/7bd14c0f6406
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: