Closed Bug 1344158 Opened 7 years ago Closed 7 years ago

Move webConsoleClient to utils/client.js

Categories

(DevTools :: Netmonitor, enhancement, P1)

Product:
DevTools
Component:
Netmonitor
Type:
enhancement

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Iteration:
55.2 - Apr 3
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: rickychien, Assigned: gasolin)

References

Details

(Whiteboard: [netmonitor])

Attachments

(2 files, 1 obsolete file)

Bug 1344158 - PART 1: wrap webconsole;
59 bytes, text/x-review-board-request
Honza
: review+
Details
Bug 1344158 - PART 2: remove gNetwork and move getString to utils/client;
59 bytes, text/x-review-board-request
Honza
: review+
Details 
Move webConsoleClient from controller to utils/client.js and export some useful public APIs.

* Let utils/client.js takes over webConsoleClient getString, sendHTTPRequest
* Move controller public APIs to client.js
  * triggerActivity
  * inspectRequest (also use in webconsole panel)
* Export viewSourceInDebugger as a public API
* Export supportsCustomRequest as a public API
* Use these exported APIs in utils/client.js such as getString, supportCustomRequest in our codebase.
Blocks: 1344160
Flags: qe-verify?
Iteration: --- → 55.1 - Mar 20
Flags: qe-verify? → qe-verify-
Priority: -- → P1
Whiteboard: [netmonitor][triage] → [netmonitor-reserve]
Comment on attachment 8845808 [details]
Bug 1344158 - PART 2: remove gNetwork and move getString to utils/client;

https://reviewboard.mozilla.org/r/118976/#review121260

::: devtools/client/netmonitor/components/monitor-panel.js:73
(Diff revision 2)
>          requestHeadersFromUploadStream && requestPostData) {
>        getFormDataSections(
>          requestHeaders,
>          requestHeadersFromUploadStream,
>          requestPostData,
> -        window.gNetwork.getString.bind(window.gNetwork),
> +        getLongString,

I think we don't need to pass getString as a paramenter into getFormDataSections(). Instead, we can use getString in request-utils.js directly and then we don't have to pass it as a parameter anymore.

::: devtools/client/netmonitor/request-list-context-menu.js:226
(Diff revision 2)
>      // Try to extract any form data parameters.
>      let formDataSections = await getFormDataSections(
>        selected.requestHeaders,
>        selected.requestHeadersFromUploadStream,
>        selected.requestPostData,
> -      window.gNetwork.getString.bind(window.gNetwork));
> +      getLongString);

I think we don't need to pass getString as a paramenter into getFormDataSections(). Instead, we can use getString in request-utils.js directly and then we don't have to pass it as a parameter anymore.
Comment on attachment 8845742 [details]
Bug 1344158 - PART 1: wrap webconsole;

https://reviewboard.mozilla.org/r/118876/#review121258

::: devtools/client/netmonitor/netmonitor-controller.js:18
(Diff revision 1)
>    formDataURI,
>  } = require("./utils/request-utils");
>  const {
>    onFirefoxConnect,
>    onFirefoxDisconnect,
> +  getWebConsoleClient,

nit: alphabetical order

::: devtools/client/netmonitor/utils/client.js:60
(Diff revision 1)
>  /**
>   * Process disconnect events.
>   *
>   * @param {Object} tabTarget
>   */
>  function onFirefoxDisconnect(tabTarget) {

nit: activeConsole = null;

::: devtools/client/netmonitor/utils/client.js:77
(Diff revision 1)
> +}
> +
>  module.exports = {
>    onFirefoxConnect,
>    onFirefoxDisconnect,
> +  getWebConsoleClient,

nit: in alphabetical order
(In reply to Ricky Chien [:rickychien] from comment #4)
> Comment on attachment 8845808 [details]
> Bug 1344158 - PART 2: remove gNetwork and move getString to utils/client;
> 
> https://reviewboard.mozilla.org/r/118976/#review121260
> 
> ::: devtools/client/netmonitor/components/monitor-panel.js:73
> (Diff revision 2)
> >          requestHeadersFromUploadStream && requestPostData) {
> >        getFormDataSections(
> >          requestHeaders,
> >          requestHeadersFromUploadStream,
> >          requestPostData,
> > -        window.gNetwork.getString.bind(window.gNetwork),
> > +        getLongString,
> 
> I think we don't need to pass getString as a paramenter into
> getFormDataSections(). Instead, we can use getString in request-utils.js
> directly and then we don't have to pass it as a parameter anymore.
> 

When I import getLongString in request-utils, the mysterious side-affect happens
https://treeherder.mozilla.org/#/jobs?repo=try&revision=63f12120496d
(ex: actions/selection.js:48 - TypeError: getDisplayedRequests is not a function)

I think its related to our init process is not in the right order yet. (We render and hook the UI then start the controller, function such as `getFormDataSections` might be called in UI before we assign the webconsoleClient) 
And to prevent the cyclic dependency, it's more clear to pass the getLongString call instead of require utils/client in request-utils. So I tend to leave `getFormDataSections` as it is now.
Comment on attachment 8845808 [details]
Bug 1344158 - PART 2: remove gNetwork and move getString to utils/client;

https://reviewboard.mozilla.org/r/118976/#review121438

::: devtools/client/netmonitor/request-list-context-menu.js:151
(Diff revision 4)
>      }));
>  
>      menu.append(new MenuItem({
>        type: "separator",
> -      visible: !!selectedRequest,
> +      visible: !!(window.NetMonitorController.supportsCustomRequest &&
> +               selectedRequest && !selectedRequest.isCustom),

This is a bug that when the build doesn't support custom request, there will be an extra separator shown on context menu
Iteration: 55.1 - Mar 20 → ---
Attachment #8846499 - Attachment is obsolete: true
Attachment #8846499 - Flags: review?(odvarko)
Comment on attachment 8845742 [details]
Bug 1344158 - PART 1: wrap webconsole;

https://reviewboard.mozilla.org/r/118876/#review124746
Attachment #8845742 - Flags: review?(odvarko) → review+
Comment on attachment 8845808 [details]
Bug 1344158 - PART 2: remove gNetwork and move getString to utils/client;

https://reviewboard.mozilla.org/r/118976/#review124748

I like removing globals!

R+ assuming try is green.

Thanks,
Honza
Attachment #8845808 - Flags: review?(odvarko) → review+
I spot the leak in treeherfer(which didn't happen in previous try with these patch). 
It seems happened in all run, so I'd rebase and add a new try run again to confirm if its a not related issue
The leak on OSX-debug build is known in Bug 1329836, so I think its ok to land this, thanks!
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/76897119071c
PART 1: wrap webconsole;r=Honza
https://hg.mozilla.org/integration/autoland/rev/1e4c005a9563
PART 2: remove gNetwork and move getString to utils/client;r=Honza
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/76897119071c
https://hg.mozilla.org/mozilla-central/rev/1e4c005a9563
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Iteration: --- → 55.2 - Apr 3
Blocks: netmonitor-phaseII
No longer blocks: netmonitor-html
Whiteboard: [netmonitor-reserve] → [netmonitor]
Blocks: 1362054
No longer blocks: 1362054
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: