Implement custom request view

VERIFIED FIXED in Firefox 54

Status

P1
normal
VERIFIED FIXED
2 years ago
2 months ago

People

(Reporter: rickychien, Assigned: gasolin)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
Firefox 54
Dependency tree / graph

Firefox Tracking Flags

(firefox52 affected, firefox54 verified)

Details

(Whiteboard: [netmonitor])

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

Render content of the form using HTML/React component

- Decide where the UI should be displayed (sidebar, floating dialog)
- UX feedback needed.

Updated

2 years ago
Whiteboard: [devtools-html]

Updated

2 years ago
Whiteboard: [netmonitor]
(Reporter)

Updated

2 years ago
Depends on: 1309479

Updated

2 years ago
Flags: qe-verify+
QA Contact: ciprian.georgiu
(Reporter)

Comment 1

2 years ago
UX spec should discuss in bug 1309479

Updated

2 years ago
Priority: -- → P2
(Assignee)

Updated

2 years ago
Depends on: 1311614

Updated

2 years ago
Priority: P2 → P1
(Assignee)

Comment 2

2 years ago
We'd migrate Edit&Resend panel with current UX spec
Blocks: 1317645
(Assignee)

Updated

2 years ago
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Summary: Migrate Edit & Resend → Migrate Edit & Resend panel

Updated

2 years ago
Iteration: --- → 53.3 - Dec 26

Updated

2 years ago
Iteration: 53.3 - Dec 26 → 53.4 - Jan 9
(Reporter)

Comment 3

2 years ago
I'd like to change bug title to make this task become clarified. This task can be done parallel with DetailsView (bug 1328197).

CustomRequestView component is part of SidebarView (it contains DetailsView and CustomRequestView), so here is a breakdown task of SidebarView for migrating custom-request-view.js into CustomRequestView react component.

- Create new netmonitor/shared/components/custom-request-view.js react component.
- Remove netmonitor/custom-request-view.js.js.
- Remove all static XUL <<vbox id="custom-pane"> tags in netmonitor.xul
- CustomRequestView component will be imported directly in netmonitor-view.js
Summary: Migrate Edit & Resend panel → Implement custom request view
Comment hidden (mozreview-request)
(Assignee)

Comment 5

2 years ago
WIP, now it can act normally as current custom panel.

TODO:
* move send/cancel button action from request-menu-view to custom-request-panel.js
* consider rename components/custom-request-panel.js to components/custom-request-view.js though it just looks like a panel
* polish css
* remove custom-request-view.js and fix tests
Comment hidden (mozreview-request)

Updated

2 years ago
Iteration: 53.4 - Jan 9 → 53.5 - Jan 23
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

2 years ago
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

The patch is almost ready except fixing tests, I thinks its good time to ask for some feedback.

The patch did:

* add custom-request-panel component to render the actual Edit&Resend view
* move send/cancel button handler into component
* handle form actions via redux connect
* handle sendHTTPRequest with thunk
* use flex layout for all side panels and statistics view (instead of -moz-box)
Attachment #8824346 - Flags: feedback?(odvarko)
Attachment #8824346 - Flags: feedback?(jsnajdr)
(Reporter)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review105136

I'm ok with name changing from CustomRequestView to CustomRequestPanel. One thing you should know is that the current component structure of sidebar:

<Sidebar>
  <DetailsView></DetailsView>
  <CustomRequestView></CustomRequestView>
</Sidebar>

So if you change view to panel, we would change DetailsView to DetailsPanel as well.

::: devtools/client/netmonitor/actions/requests.js:67
(Diff revision 6)
>  
>  /**
> + * Send a new HTTP request using the data in the custom request form.
> + */
> +function sendCustomRequest() {
> +  /* eslint-disable consistent-return */

Why you disable consistent-return rule rather get rid of else { return ... }?

::: devtools/client/netmonitor/custom-request-view.js:55
(Diff revision 6)
> -    if (data.requestPostData) {
> -      let postData = data.requestPostData.postData.text;
> -      $("#custom-postdata-value").value = yield gNetwork.getString(postData);
> -    }
> -
>      window.emit(EVENTS.CUSTOMREQUESTVIEW_POPULATED);

All EVENTS constants should be removed and we are no longer to use window.emit() anymore. 

I'm also think that this file is pretty simple and do nothing except append react component. So perhaps we can go further and remove entire custom-request-view.js and then import CustomRequestPanel component in netmonitor-view.js

::: devtools/client/netmonitor/requests-menu-view.js:373
(Diff revision 6)
>    cloneSelectedRequest() {
>      this.store.dispatch(Actions.cloneSelectedRequest());
>    },

Is it possible to remove this wraping mehtod? Action is supposed to be dispatched directly without calling this wrapper function.

::: devtools/client/netmonitor/requests-menu-view.js:380
(Diff revision 6)
>    },
>  
>    /**
>     * Send a new HTTP request using the data in the custom request form.
>     */
> -  sendCustomRequest: function () {
> +  sendCustomRequest() {

likewise above

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:18
(Diff revision 6)
> +  getUrlQuery,
> +  parseQueryString,
> +  writeHeaderText,
> +} = require("../../request-utils");
> +
> +// Components

nit: remove this comment

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:20
(Diff revision 6)
> +  writeHeaderText,
> +} = require("../../request-utils");
> +
> +// Components
> +const {
> +  div,

nit: In alphabetical order

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:149
(Diff revision 6)
> +  updateQuery: PropTypes.func,
> +  updateUrl: PropTypes.func,
> +  url: PropTypes.string.isRequired,
> +};
> +
> +module.exports = connect(

nit: module.export = connect(... should be put at the end of the file.
(Assignee)

Comment 13

2 years ago
mozreview-review-reply
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review105136

> All EVENTS constants should be removed and we are no longer to use window.emit() anymore. 
> 
> I'm also think that this file is pretty simple and do nothing except append react component. So perhaps we can go further and remove entire custom-request-view.js and then import CustomRequestPanel component in netmonitor-view.js

It's been kept to mitigate unit tests bustage, will remove it once tests are fixed.

> Is it possible to remove this wraping mehtod? Action is supposed to be dispatched directly without calling this wrapper function.

It's also a workaround before fixed all unit tests, will remove them once tests are fixed.

Comment 14

2 years ago
mozreview-review
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review105608

::: devtools/client/netmonitor/actions/requests.js:69
(Diff revision 6)
> + * Send a new HTTP request using the data in the custom request form.
> + */
> +function sendCustomRequest() {
> +  /* eslint-disable consistent-return */
> +  return (dispatch, getState) => {
> +    if (NetMonitorController.supportsCustomRequest) {

nit: return early when the condition is not satisfied, and decrease the indent depth of the remaining code.

::: devtools/client/netmonitor/reducers/requests.js:152
(Diff revision 6)
> +
> +  if (!selectedId) {
> +    return state;
> +  }
> +
> +  let removedRequest = requests.find(r => r.id === selectedId);

Please use requests.get(selectedId) here. The 'requests' collection was converted to Immutable.Map recently.

::: devtools/client/netmonitor/reducers/requests.js:160
(Diff revision 6)
> +  if (!removedRequest || !removedRequest.isCustom) {
> +    return state;
> +  }
> +
> +  return state.withMutations(st => {
> +    st.requests = requests.filter(r => r !== removedRequest);

requests.delete(removedRequest)

::: devtools/client/netmonitor/reducers/requests.js:224
(Diff revision 6)
> +      let request = requests.get(selectedId);
> +      let queryString = writeQueryString(action.data.params);
> +      let link = request.url.replace(getUrlQuery(request.url), queryString);
> +      delete action.data.params;
> +      action.id = selectedId;
> +      action.data.url = link;
> +      return updateRequest(state, action);

The code would be more readable if a new "action" object was created instead of modifying the incoming one. It would also make the UPDATE_SELECTED_REQUEST_QUERY API better defined - in its current form, I can abuse it to perform any update on the selected request, not only the query string.

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:85
(Diff revision 6)
> +        id: "custom-url-value",
> +        onChange: updateUrl,
> +        value: url,
> +      }),
> +    ),
> +    // hide query filed when there is no params

typo: filed/field
Attachment #8824346 - Flags: feedback?(jsnajdr) → feedback+

Comment 15

2 years ago
mozreview-review
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review105952

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:130
(Diff revision 6)
> +        value: postData,
> +        wrap: "off",
> +      })
> +    ),
> +  );
> +}

The rendering code should use style consistent with existing render() methods. Mainly, curly brackets should not be put on separate lines.
Attachment #8824346 - Flags: feedback?(odvarko) → feedback+
Comment hidden (mozreview-request)
(Assignee)

Comment 17

2 years ago
rebase PR and address nits. 

Found an issue about react form display with 2 way binding between url and query-values. Will fix that and other tests before request a review.

Updated

2 years ago
Iteration: 53.5 - Jan 23 → 54.1 - Feb 6
I also ran into the same custom request view test failures in browser_net_resend.js in bug 1328197 (details panel). The root cause is due to data binding behavior in custom request panel, which caused keyboard simulating in mochitest doesn't work.

Fred's patch is dealing with this problem, so I'm going to disable the browser_net_resend.js test in bug 1328197.

Fred, kindly be reminded to re-enable the browser_net_resend.js in browser.ini after you fix the test failures.
Comment hidden (mozreview-request)
(Assignee)

Comment 20

2 years ago
The WIP rebased PR so its easier to pick up for sidebar work.

It also fixed 2 way sync between URL & query field

TODO:
* need fix header and body field
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 25

2 years ago
mozreview-review
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review108200

::: devtools/client/netmonitor/actions/requests.js:4
(Diff revision 11)
>  /* 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/. */
> -
> +/* globals NetMonitorController */

nit: add a new line between license and eslint config

::: devtools/client/netmonitor/actions/requests.js:7
(Diff revision 11)
>   * 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/. */
> -
> +/* globals NetMonitorController */
>  "use strict";
>  
> +const { getSelectedRequest } = require("../selectors/index");

nit: require('xxx') by alphabetical order

xx = require('../constans');
xx = require('../selectors/index');

::: devtools/client/netmonitor/actions/requests.js:80
(Diff revision 11)
> +function sendCustomRequest() {
> +  if (!NetMonitorController.supportsCustomRequest) {
> +    return cloneSelectedRequest();
> +  }
> +
> +  /* eslint-disable consistent-return */

Is there any reason to add eslint-disable? If it's possbile to be fixed properly, we should remove it.

::: devtools/client/netmonitor/actions/requests.js:110
(Diff revision 11)
> +        type: SEND_CUSTOM_REQUEST,
> +        id: id,
> +      };
> +    });
> +  };
> +  /* eslint-enable consistent-return */

likewise above

::: devtools/client/netmonitor/reducers/requests.js:54
(Diff revision 11)
>    transferredSize: undefined,
>    totalTime: undefined,
>    eventTimings: undefined,
>    headersSize: undefined,
> +  // temp state for showing custom requst form
> +  paramsText: undefined,

I guess this temp state only use for testing? Please remove it after problem is addressed.

::: devtools/client/netmonitor/requests-menu-view.js:421
(Diff revision 11)
>    },
>  
>    /**
>     * Send a new HTTP request using the data in the custom request form.
>     */
> -  sendCustomRequest: function () {
> +  sendCustomRequest() {

For reducing the difficulty of cleaning request-menu-view.js, we should not add or modify APIs in request-menu-view.js anymore.

Please remove sendCustomReques() and call action directly.

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:23
(Diff revision 11)
> +const {
> +  button,
> +  div,
> +  input,
> +  label,
> +  textarea

nit: textarea,

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:26
(Diff revision 11)
> +const CUSTOM_NEW_REQUEST = L10N.getStr("netmonitor.custom.newRequest");
> +const CUSTOM_SEND = L10N.getStr("netmonitor.custom.send");
> +const CUSTOM_CANCEL = L10N.getStr("netmonitor.custom.cancel");
> +const CUSTOM_QUERY = L10N.getStr("netmonitor.custom.query");
> +const CUSTOM_HEADERS = L10N.getStr("netmonitor.custom.headers");
> +const CUSTOM_POSTDATA = L10N.getStr("netmonitor.custom.postData");

nit: please follow the alphabeticall order

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:34
(Diff revision 11)
> +const CUSTOM_QUERY = L10N.getStr("netmonitor.custom.query");
> +const CUSTOM_HEADERS = L10N.getStr("netmonitor.custom.headers");
> +const CUSTOM_POSTDATA = L10N.getStr("netmonitor.custom.postData");
> +
> +function CustomRequestPanel({
> +  headers,

Component interface has to align with details panel, so we can remove some request relevant props and just define a request prop and deal with the internel object properties within CustomRequestPanel() function.

https://hg.mozilla.org/mozilla-central/file/12b1474b8065/devtools/client/netmonitor/shared/components/details-panel.js#l40

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:48
(Diff revision 11)
> +  updateMethod,
> +  updateQuery,
> +  updateUrl,
> +  url,
> +}) {
> +  return div({ className: "custom-request-panel" },

Please improve the code styling and some of className definitions are wrong (no ,)

label({ className: "tabpanel-summary-label custom-header" },
  CUSTOM_NEW_REQUEST
),
  button({
    className: "devtools-toolbarbutton tool-button",
    id: "custom-request-send-button",
    onClick: sendCustomRequest,
  },
    CUSTOM_SEND,
  ),
  button({
    className: "devtools-toolbarbutton tool-button",
    id: "custom-request-close-button",
    onClick: removeSelectedCustomRequest,
  },
    CUSTOM_CANCEL,
  ),

...

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:97
(Diff revision 11)
> +      textarea({
> +        className: "tabpanel-summary-input",
> +        id: "custom-query-value",
> +        onChange: updateQuery,
> +        rows: 4,
> +        value: paramsText ? paramsText : writeQueryText(paramsArray),

nit: paramsText || writeQueryText(paramsArray),

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:153
(Diff revision 11)
> +};
> +
> +/**
> + * Write out a list of query params into a chunk of text
> + *
> + * @param array params

nit: {array} params

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:155
(Diff revision 11)
> +/**
> + * Write out a list of query params into a chunk of text
> + *
> + * @param array params
> + *        Array of query params {name, value}
> + * @return string

nit: {string}

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:200
(Diff revision 11)
> +    updateBody: (evt) => {
> +      let text = evt.target.value;
> +      dispatch(Actions.updateSelectedRequest({
> +        requestPostData: {
> +          postData: { text }
> +        }
> +      }));
> +    },
> +    updateHeaders: (evt) => {
> +      let headersText = evt.target.value;
> +      dispatch(Actions.updateSelectedRequestHeader({
> +        requestHeaders: { headersText }
> +      }));
> +    },
> +    updateMethod: (evt) => {
> +      let method = evt.target.value.trim();
> +      dispatch(Actions.updateSelectedRequest({ method }));
> +    },
> +    // Update the url based on the query string field
> +    updateQuery: (evt) => {
> +      let paramsText = evt.target.value;
> +      dispatch(Actions.updateSelectedRequestQuery({ paramsText }));
> +    },
> +    updateUrl: (evt) => {
> +      let url = evt.target.value;
> +      dispatch(Actions.updateSelectedRequest({ url }));
> +    },

This part could be the bigest issue should be addressed first in this patch.

It seems like it's unnecessary to keep the state in redux store. Instead, what we can do is to implement this part by using a stateful component. Create state within stateful component to memorize the data we care such as postData, headersText, method, paramsText and url, then we can remove these actions and reducers.
(Assignee)

Comment 26

2 years ago
mozreview-review-reply
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review108200

> This part could be the bigest issue should be addressed first in this patch.
> 
> It seems like it's unnecessary to keep the state in redux store. Instead, what we can do is to implement this part by using a stateful component. Create state within stateful component to memorize the data we care such as postData, headersText, method, paramsText and url, then we can remove these actions and reducers.

Make sense, I'll convert this view as a stateful component and not propagate the states to reducer.
(Assignee)

Comment 27

2 years ago
updated PR
 
* fixed header and body field
* removed CustomRequestView in netmonitor/
* remove unused preselect action
* remove actions wrapper from request-menu-view

Known issue:

* The thunk seems not work immediately, which cause the resend_cor test fail
* convert to stateful componet and not propagate the states to reducer
Comment hidden (mozreview-request)
(Assignee)

Comment 29

2 years ago
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

postpone review till the view is converted to stateful component
Attachment #8824346 - Flags: review?(odvarko)
(Assignee)

Comment 30

2 years ago
With the second thought, I think we still need to store current request state into redux.

When user select back and forth to these custom requests, we can show exact user's current edit state. If we just store current editing in component state, it will cause several unpredictable issues when user edit more than one custom request.

Ricky, how do you think?
Flags: needinfo?(rchien)
Yes, you're right. It's necessary to keep the state in each request so that user may want to switch to different clone requests while modifying the custom fields and then previous modified state can restore as expected.

After offline discussion, we decided to add an additional state for storing query, postData, headers for each custom request fields. I'd prefer to define the var name prefix with custom such as customQueryValue, customPostDataValue and customHeadersValue.
Flags: needinfo?(rchien)
(Reporter)

Comment 32

2 years ago
mozreview-review-reply
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review108200

> Make sense, I'll convert this view as a stateful component and not propagate the states to reducer.

Drop this issue since store the state in redux seems necessary.
Comment hidden (mozreview-request)
(Reporter)

Comment 34

2 years ago
mozreview-review
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review108494

::: devtools/client/netmonitor/actions/requests.js:46
(Diff revision 13)
> +function updateSelectedRequestHeader(data, batch) {
> +  return {
> +    type: UPDATE_SELECTED_REQUEST_HEADER,
> +    data,
> +    meta: { batch },
> +  };
> +}
> +
> +function updateSelectedRequestQuery(data, batch) {
> +  return {
> +    type: UPDATE_SELECTED_REQUEST_QUERY,
> +    data,
> +    meta: { batch },
> +  };

Please updateSelectedRequestHeader() and updateSelectedRequestQuery(), updateSelectedRequest() is enough to deal with all our needs.

We can parse these headers and query values before dispatch updateSelectedRequest() function , and then pass them as data payload into updateSelectedRequest(...).

::: devtools/client/netmonitor/reducers/requests.js:176
(Diff revision 13)
> + * @param array params
> + *        Array of query  params {name, value}
> + * @return string
> + *         Query string that can be appended to a url.
> + */
> +function writeQueryString(params) {

nit: This one line expression only use in one place, so I think it's not that useful to define as a separate function.

::: devtools/client/netmonitor/reducers/requests.js:180
(Diff revision 13)
> +/**
> + * Parse a text representation of a name[divider]value list with
> + * the given name regex and divider character.
> + *
> + * @param string text
> + *        Text of list
> + * @return array
> + *         Array of headers info {name, value}
> + */

/**
 * Parse a text representation of a name[divider]value list with
 * the given name regex and divider character.
 *
 * @param {string} text - text of list
 * @return {array} an array of headers info {name, value}
 */

::: devtools/client/netmonitor/reducers/requests.js:190
(Diff revision 13)
> + *        Text of list
> + * @return array
> + *         Array of headers info {name, value}
> + */
> +function parseRequestText(text, namereg, divider) {
> +  let regex = new RegExp("(" + namereg + ")\\" + divider + "\\s*(.+)");

nit: Let's use template string instead.

::: devtools/client/netmonitor/reducers/requests.js:195
(Diff revision 13)
> +  let regex = new RegExp("(" + namereg + ")\\" + divider + "\\s*(.+)");
> +  let pairs = [];
> +
> +  for (let line of text.split("\n")) {
> +    let matches;
> +    if (matches = regex.exec(line)) { // eslint-disable-line

nit: do not disable this rule, try to fix the eslint error.

let matches = regex.exec(line);
if (matches) ...

::: devtools/client/netmonitor/reducers/requests.js:197
(Diff revision 13)
> +
> +  for (let line of text.split("\n")) {
> +    let matches;
> +    if (matches = regex.exec(line)) { // eslint-disable-line
> +      let [, name, value] = matches;
> +      pairs.push({name: name, value: value});

nit: pairs.push({ name, value });

::: devtools/client/netmonitor/reducers/requests.js:211
(Diff revision 13)
> + * @param string text
> + *        Text of query string representation
> + * @return array
> + *         Array of query params {name, value}
> + */
> +function parseQueryText(text) {

nit: I think the one line expression function is simple enough and only call in one place. Please remove it and invoke it directly.

::: devtools/client/netmonitor/reducers/requests.js:223
(Diff revision 13)
> + * @param string text
> + *        Text of headers
> + * @return array
> + *         Array of headers info {name, value}
> + */
> +function parseHeadersText(text) {

nit: I think the one line expression function is simple enough and only call in one place. Please remove it and invoke it directly.
Attachment #8824346 - Flags: review?(rchien)
(Assignee)

Comment 35

2 years ago
mozreview-review-reply
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review108494

> Please updateSelectedRequestHeader() and updateSelectedRequestQuery(), updateSelectedRequest() is enough to deal with all our needs.
> 
> We can parse these headers and query values before dispatch updateSelectedRequest() function , and then pass them as data payload into updateSelectedRequest(...).

It does not work if user clean out all queries or headers. Since the passed in string is blank, the url/headers wont be updated.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 38

2 years ago
mozreview-review
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review108514

::: devtools/client/netmonitor/actions/requests.js:5
(Diff revision 16)
>  /* 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/. */
>  
> +/* globals NetMonitorController */

nit: same issue here. add new line between license and eslint config and "use strict".

::: devtools/client/netmonitor/actions/requests.js:8
(Diff revision 16)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/* globals NetMonitorController */
>  "use strict";
>  
> +const { getSelectedRequest } = require("../selectors/index");

nit: move const { getSelectedRequest } = require("../selectors/index"); after require("../constants");

::: devtools/client/netmonitor/actions/requests.js:47
(Diff revision 16)
> +function updateSelectedRequestHeader(data, batch) {
> +  return {
> +    type: UPDATE_SELECTED_REQUEST_HEADER,
> +    data,
> +    meta: { batch },
> +  };
> +}
> +
> +function updateSelectedRequestQuery(data, batch) {
> +  return {
> +    type: UPDATE_SELECTED_REQUEST_QUERY,
> +    data,
> +    meta: { batch },
> +  };
> +}

After offline discussion, I'm still thinking both of these actions are unnecessary. Please remove them and move parsing code prior to dispatching action.

::: devtools/client/netmonitor/actions/requests.js:83
(Diff revision 16)
> +    return cloneSelectedRequest();
> +  }
> +
> +  return (dispatch, getState) => {
> +    const state = getState();
> +    const selected = getSelectedRequest(state);

nit: state is not used in other places

const selected = getSelectedRequest(getState());

::: devtools/client/netmonitor/actions/requests.js:87
(Diff revision 16)
> +    const state = getState();
> +    const selected = getSelectedRequest(state);
> +
> +    if (!selected) {
> +      // avoid consistent-return eslint error by async return the action
> +      () => cloneSelectedRequest();

Is it correct? It seems like to create a callback function but not invoke it?

::: devtools/client/netmonitor/reducers/requests.js:183
(Diff revision 16)
> +
> +  for (let line of text.split("\n")) {
> +    let matches = regex.exec(line);
> +    if (matches) {
> +      let [, name, value] = matches;
> +      pairs.push({

nit: The var name is "paris" so it should be predictable and won't add other props. it's ok to write it as one line IMO.

pairs.push({ name, value });

::: devtools/client/netmonitor/reducers/requests.js:293
(Diff revision 16)
> +        return updateRequest(state, queryAction);
> +      }
> +
> +      let request = requests.get(selectedId);
> +      // Write out a list of query params into a query string
> +      let queryString = queryArray.map(({name, value}) => name + "=" + value).join("&");

nit: add spaces

{ name, value }

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:46
(Diff revision 16)
> +  let method = null;
> +  let customQueryValue = null;
> +  let requestHeaders = null;
> +  let requestPostData = null;
> +  let url = null;
> +
> +  if (request) {
> +    method = request.method || request.method;
> +    customQueryValue = request.customQueryValue || customQueryValue;
> +    requestHeaders = request.requestHeaders || requestHeaders;
> +    requestPostData = request.requestPostData || requestPostData;
> +    url = request.url || url;
> +  }

nit: embrace new es6 syntax

set default value for request

function CustomRequestPanel({
  removeSelectedCustomRequest,
  request = {},
  ...
})

let {
  method,
  customQueryValue,
  requestHeaders,
  requestPostData,
  url,
} = request;

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:66
(Diff revision 16)
> +  if (requestHeaders) {
> +    headers = requestHeaders.customHeadersValue ?
> +      requestHeaders.customHeadersValue : writeHeaderText(requestHeaders.headers);
> +  }
> +  let queryArray = url ? parseQueryString(getUrlQuery(url)) : [];
> +  let params = customQueryValue ? customQueryValue : writeQueryText(queryArray);

params =  customQueryValue || writeQueryText(queryArray);

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:157
(Diff revision 16)
> +
> +CustomRequestPanel.displayName = "CustomRequestPanel";
> +
> +CustomRequestPanel.propTypes = {
> +  request: PropTypes.object,
> +  removeSelectedCustomRequest: PropTypes.func,

nit: alphabetical order

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:174
(Diff revision 16)
> + * @param {array} params
> + *        Array of query params {name, value}
> + * @return {string}
> + *         List of query params in text format
> + */
> +function writeQueryText(params) {

nit: remove this function and invoke it directly.

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:179
(Diff revision 16)
> +function writeQueryText(params) {
> +  return params ?
> +    params.map(({name, value}) => name + "=" + value).join("\n") : "";
> +}
> +
> +module.exports = connect(

nit: surround arrow function parameters with () even if there is only one parameter.

(state) => ...
(dispatch) => ...

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:180
(Diff revision 16)
> +  state => {
> +    const request = getSelectedRequest(state);
> +    return { request };
> +  },

nit: one line expression

(state) => {( request: getSelectedRequest(state) )}
Attachment #8824346 - Flags: review?(rchien)
(Reporter)

Comment 39

2 years ago
mozreview-review-reply
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review108514

> Is it correct? It seems like to create a callback function but not invoke it?

It seems that cloneSelectedRequest() should be invoked immediately, it doesn't make sense to execute in the callback. Is it possible to put a return at the end of the function to fix the eslint error?

Comment 40

2 years ago
mozreview-review
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review108574

I am seeing the following issue:

1) Open the sidebar and press 'Edit and Resend'
2) Click 'Cancel' button in the custom request panel
3) Check out the request list, the cloned request is still there and it should not -> BUG

Also, there are some UX differences from the current behavior.

1. Pressing 'Send' in the custom request dialog doesn't close it (it does in the current impl). I actually like this UX since it allows to quickly send the same request multiple times. But we should make sure it isn't a bug but a feature - if we agree on this behavior.
2. Pressing 'Cancel' in the custom request dialog does close the entire side panel. The current impl opens the Details View. I vote for keeping the behavior the same in this case.

Honza
Attachment #8824346 - Flags: review?(odvarko)
(Reporter)

Comment 41

2 years ago
mozreview-review
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review110130

I noticed there are some issue and UI broken after applying and playing with the patch for a while.

1. Comparing with old UI, all textarea elements should set `resize: none;` to remove the adjustable controller at the bottom right of textarea.
2. margin and padding around input and textarea are slightly different with old UI, we should keep it as is.
3. font in textarea looks different, it should apply `font: message-box;`
4. Please make sure new UI display properly when docking to bottom and right side. (note that there is a responsive sidebar `@media (max-width: 700px)` which is using for docking to side of browser window)
5. Vertical splitter is broken when docking to side of browser, we are unable to apply height: 100vh anymore. Please see my comment below.

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:70
(Diff revision 16)
> +  let queryArray = url ? parseQueryString(getUrlQuery(url)) : [];
> +  let params = customQueryValue ? customQueryValue : writeQueryText(queryArray);
> +  let postData = requestPostData && requestPostData.postData.text ?
> +    requestPostData.postData.text : "";
> +
> +  return div({ className: "custom-request-panel" },

nit: wrap react factory function with parentheses () if it exceeds one line

For example:


```js
return (
  div({ className: "custom-request-panel" },
    ...
  )
);

```

::: devtools/client/themes/netmonitor.css:48
(Diff revision 16)
>  
>  #details-pane-toggle[disabled] {
>    display: none;
>  }
>  
> -#custom-pane {
> +#react-custom-request-panel-hook {

Is is possible to set overflow: auto for top react element .custom-request-panel and do not set it in #react-xxx-hook?

IMO, the way we set css rules in #react-xxx-hook is a temporary workaround and it's going to be deprecated soon.

And also, please move all #react-xxx-hook to the bottom.

::: devtools/client/themes/netmonitor.css:793
(Diff revision 16)
>    }
>  }
>  
> -/* Custom request form */
> +/* Custom request view */
> +
> +#react-custom-request-panel-hook {

likewise above

::: devtools/client/themes/netmonitor.css:797
(Diff revision 16)
> +
> +#react-custom-request-panel-hook {
> +  padding: 0.6em 1em;
> +}
> +
> +.custom-request-panel {

I'm seeing the vertical splitter is broken after click "dock to the side of browser window".

Note that we can apply height: 100vh; because it only works for bottom mode. You can see custom request panel fills all vertical space and splitter is unmovable after docking the devtools panel to right side.

Fortunately, solution is already existed in DetailsPanel. Please take a look and apply the same css rules to fix the problem.

::: devtools/client/themes/netmonitor.css:864
(Diff revision 16)
>  }
>  
>  /* Performance analysis view */
>  
>  #network-statistics-view {
> -  display: -moz-box;
> +  display: flex;

Are there any purpose to change #network-statistics-view?

I'm seeing laout broken in statistics view
1. The splitter between two charts is disappeared.
2. Only one chart shows up (should be two) when docking devtools panel to the side.

::: devtools/client/themes/netmonitor.css:890
(Diff revision 16)
>    border-color: rgba(0,0,0,0.2);
>    cursor: default;
>    pointer-events: none;
>  }
>  
>  #network-statistics-charts {

likewise above
Attachment #8824346 - Flags: review-
(In reply to Jan Honza Odvarko [:Honza] from comment #40)
> Comment on attachment 8824346 [details]
> Bug 1308449 - Implement custom request view;
> 
> https://reviewboard.mozilla.org/r/102874/#review108574
> 
> I am seeing the following issue:
> 
> 1) Open the sidebar and press 'Edit and Resend'
> 2) Click 'Cancel' button in the custom request panel
> 3) Check out the request list, the cloned request is still there and it
> should not -> BUG
> 
> Also, there are some UX differences from the current behavior.
> 
> 1. Pressing 'Send' in the custom request dialog doesn't close it (it does in
> the current impl). I actually like this UX since it allows to quickly send
> the same request multiple times. But we should make sure it isn't a bug but
> a feature - if we agree on this behavior.

I'd prefer to keep UI as is because the behavior of current patch looks weird to me. Old implementation will focus on new custom request after pressing the 'Send' button, the way is more intuitive by notifying a successful custom request has been sent. Furthermore, it's hard to notice the new change because the successful custom requests are invisible and they always append at the end of request list, you have to scroll to bottom to see it.

> 2. Pressing 'Cancel' in the custom request dialog does close the entire side
> panel. The current impl opens the Details View. I vote for keeping the
> behavior the same in this case.

Agree.
(Assignee)

Comment 43

2 years ago
mozreview-review-reply
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review108514

> After offline discussion, I'm still thinking both of these actions are unnecessary. Please remove them and move parsing code prior to dispatching action.

For updateBody/updateHeaders/updateQuery we need get the select request and its id from `state` to manipulate the new dataset. The mapDispatchToProps function does not pass the `state` function. We can do so in mergeProps[1] but I think its more elagent to do so in the reducer.

[1] https://github.com/reactjs/react-redux/blob/master/docs/api.md#connectmapstatetoprops-mapdispatchtoprops-mergeprops-options
I think you might misunderstood my comment. We can remain updateSelectedRequest() for addressing selected request within reducer without using mergeProps(), but we should remove updateSelectedRequestHeader/updateSelectedRequestQuery since they are unnecessary.
(Assignee)

Comment 45

2 years ago
mozreview-review-reply
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review108514

> nit: embrace new es6 syntax
> 
> set default value for request
> 
> function CustomRequestPanel({
>   removeSelectedCustomRequest,
>   request = {},
>   ...
> })
> 
> let {
>   method,
>   customQueryValue,
>   requestHeaders,
>   requestPostData,
>   url,
> } = request;

We cant just use `request = {}` here because the default value assignment[1] only works when request is `undefined`. (we will get `null` here)
Though change returned request to `undefined` does not work either. At the end I found the deconstruct works well with `request || {}` syntax;

let {
  method,
  customQueryValue,
  requestHeaders,
  requestPostData,
  url,
} = request || {};

[1] https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment#Default_values
(In reply to Fred Lin [:gasolin] from comment #45)
> We cant just use `request = {}` here because the default value assignment[1]
> only works when request is `undefined`. (we will get `null` here)
> Though change returned request to `undefined` does not work either. At the
> end I found the deconstruct works well with `request || {}` syntax;

Good catch! It sounds weird because null belongs to a false family as well. Anyway, it think the issue can also be fixed by replacing null with undefined in selector at http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/selectors/requests.js#97, which can prevent someone (maybe external contributors) from writing the default values in restructuring assignments again.
(Assignee)

Comment 47

2 years ago
> Though change returned request to `undefined` does not work either.

I tried that again and set to `undefined` works @@
Comment hidden (mozreview-request)
(Assignee)

Comment 49

2 years ago
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

WIP moved all updateSelectedRequest reducer back to dispatch and fix several css issues

Some known bugs are in progress and this PR is provided for early test.
Attachment #8824346 - Flags: review?(rchien)
Attachment #8824346 - Flags: review?(odvarko)
(Reporter)

Comment 50

2 years ago
mozreview-review
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review110532

::: devtools/client/netmonitor/actions/requests.js:81
(Diff revision 17)
> +      type: SEND_CUSTOM_REQUEST,
> +      id: response.eventActor.actor,
> +    }));
> +
> +    // avoid consistent-return eslint error
> +    return {};

nit: Is it possible to return null or undefined instead of {}?

Please remove the comment because it sounds a workaround to fix eslint error, but return a null is normal trick in JS.

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:96
(Diff revision 17)
> +        id: "custom-method-and-url",
> +      },
> +        input({
> +          className: "custom-method-value",
> +          id: "custom-method-value",
> +          onChange: updateMethod.bind(this, request),

nit: 

existing code prefers to create a new arrow function instead of bind.
```
onChange: () => updateMethod(request),
```

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:102
(Diff revision 17)
> +          value: method || "GET",
> +        }),
> +        input({
> +          className: "custom-url-value",
> +          id: "custom-url-value",
> +          onChange: updateUrl.bind(this, request),

likewise

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:115
(Diff revision 17)
> +      },
> +        label({ className: "tabpanel-summary-label" }, CUSTOM_QUERY),
> +        textarea({
> +          className: "tabpanel-summary-input",
> +          id: "custom-query-value",
> +          onChange: updateQuery.bind(this, request),

likewise

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:129
(Diff revision 17)
> +      },
> +        label({ className: "tabpanel-summary-label" }, CUSTOM_HEADERS),
> +        textarea({
> +          className: "tabpanel-summary-input",
> +          id: "custom-headers-value",
> +          onChange: updateHeaders.bind(this, request),

likewise

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:143
(Diff revision 17)
> +      },
> +        label({ className: "tabpanel-summary-label" }, CUSTOM_POSTDATA),
> +        textarea({
> +          className: "tabpanel-summary-input",
> +          id: "custom-postdata-value",
> +          onChange: updateBody.bind(this, request),

likewise

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:159
(Diff revision 17)
> +  updateBody: PropTypes.func,
> +  updateHeaders: PropTypes.func,
> +  updateMethod: PropTypes.func,
> +  updateQuery: PropTypes.func,
> +  updateUrl: PropTypes.func,

Good job for getting rid of a bunch of updateSelectedRequestXXX here. The next thing is we should reduce the number of props if possible, in order to simplify the component interface.

Think of a scenario that someone wants to import this component to do something, it's not that easy to figure out how to implement these callback props (updateBody, updateHeaders...etc)

The best way is that we need just one generic update method which calls `updateRequest` here to tell people to pass this `updateRequest` props and then we will update state for you.

So my suggestion is to remove 

updateBody: PropTypes.func,
updateHeaders: PropTypes.func,
updateMethod: PropTypes.func,
updateQuery: PropTypes.func,
updateUrl: PropTypes.func,

but construct one updateRequest props along with a implementation like this

updateRequest: (id, data) => dispatch(Actions.updateRequest(id, data, true)

and then we can extract these updateRequestPostData, updateRequestHeaders...etc (with more explicit name) into function appeding them after parseRequestText()

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:234
(Diff revision 17)
> +          false));
> +      }
> +    },
> +    updateMethod: (request, evt) => {
> +      let method = evt.target.value.trim();
> +      dispatch(Actions.updateRequest(request.id, { method }, false));

Note that we should enable batch mode if possible.

Please set the third argument to true.

```
Actions.updateRequest(request.id, { method }, true)
```

::: devtools/client/themes/netmonitor.css:31
(Diff revision 17)
>    flex: 0 0 auto;
>    flex-wrap: nowrap;
>    align-items: center;
>  }
>  
> -#details-pane {
> +.custom-request-panel,

nit: there is another rule at line 1025, please merge them.

::: devtools/client/themes/netmonitor.css:792
(Diff revision 17)
>  
> -/* Custom request form */
> +/* Custom request view */
> +
> +.custom-request-panel {
> +  overflow: auto;
> +  height: 100vh;

This will break UI when docking to right side. We should figure out a better solution instead.
Attachment #8824346 - Flags: review-
(Reporter)

Comment 51

2 years ago
mozreview-review
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review110840

::: devtools/client/netmonitor/test/browser_net_resend.js:20
(Diff revision 17)
>  add_task(function* () {
>    let { tab, monitor } = yield initNetMonitor(POST_DATA_URL);
>    info("Starting test... ");
>  
>    let { panelWin } = monitor;
> -  let { document, EVENTS, NetMonitorView } = panelWin;
> +  let { document, gStore, EVENTS, NetMonitorView, windowRequire } = panelWin;

nit: remove unused EVENTS.

::: devtools/client/netmonitor/test/browser_net_resend_cors.js:15
(Diff revision 17)
>  
>  add_task(function* () {
>    let { tab, monitor } = yield initNetMonitor(CORS_URL);
>    info("Starting test... ");
>  
> -  let { EVENTS, NetMonitorView } = monitor.panelWin;
> +  let { EVENTS, gStore, NetMonitorView, windowRequire } = monitor.panelWin;

nit: remove unused EVENTS.
(Assignee)

Comment 52

2 years ago
mozreview-review-reply
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review110532

> Note that we should enable batch mode if possible.
> 
> Please set the third argument to true.
> 
> ```
> Actions.updateRequest(request.id, { method }, true)
> ```

The edit UI need synced state to reflect the change, or you can see UI glitch when you removed a portion of params or headers
Comment hidden (mozreview-request)
(Assignee)

Comment 54

2 years ago
The lastes PR changes

* rebased
* fix custom view button style
* fix custom request edit & send
* cloned request should not stay after cancel
* use single updateRequest method to update all form values

I think its ready for next round review
(Reporter)

Comment 55

2 years ago
mozreview-review
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review110990

I leaved some comments and code quality is better now!

The patch has conflicts so please fix it and then I'll test again.

::: devtools/client/netmonitor/reducers/requests.js:47
(Diff revision 18)
>    transferredSize: undefined,
>    totalTime: undefined,
>    eventTimings: undefined,
>    headersSize: undefined,
> +  // this state only appears when user edit the custom requst form
> +  customQueryValue: undefined,

nit:

// Text value is used for storing custom request forms

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:92
(Diff revision 18)
> +        id: "custom-method-and-url",
> +      },
> +        input({
> +          className: "custom-method-value",
> +          id: "custom-method-value",
> +          onChange: (evt) => updateRequest(request, evt),

nit: put bound argument (request) after evt.

(evt) => updateRequest(evt, request),

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:98
(Diff revision 18)
> +          value: method || "GET",
> +        }),
> +        input({
> +          className: "custom-url-value",
> +          id: "custom-url-value",
> +          onChange: (evt) => updateRequest(request, evt),

nit: like wise

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:111
(Diff revision 18)
> +      },
> +        label({ className: "tabpanel-summary-label" }, CUSTOM_QUERY),
> +        textarea({
> +          className: "tabpanel-summary-input",
> +          id: "custom-query-value",
> +          onChange: (evt) => updateRequest(request, evt),

nit: like wise

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:125
(Diff revision 18)
> +      },
> +        label({ className: "tabpanel-summary-label" }, CUSTOM_HEADERS),
> +        textarea({
> +          className: "tabpanel-summary-input",
> +          id: "custom-headers-value",
> +          onChange: (evt) => updateRequest(request, evt),

nit: like wise

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:139
(Diff revision 18)
> +      },
> +        label({ className: "tabpanel-summary-label" }, CUSTOM_POSTDATA),
> +        textarea({
> +          className: "tabpanel-summary-input",
> +          id: "custom-postdata-value",
> +          onChange: (evt) => updateRequest(request, evt),

nit: like wise

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:152
(Diff revision 18)
> +}
> +
> +CustomRequestPanel.displayName = "CustomRequestPanel";
> +
> +CustomRequestPanel.propTypes = {
> +  removeSelectedCustomRequest: PropTypes.func,

nit: isRequired

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:154
(Diff revision 18)
> +CustomRequestPanel.displayName = "CustomRequestPanel";
> +
> +CustomRequestPanel.propTypes = {
> +  removeSelectedCustomRequest: PropTypes.func,
> +  request: PropTypes.object,
> +  sendCustomRequest: PropTypes.func,

nit: isRequired

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:155
(Diff revision 18)
> +
> +CustomRequestPanel.propTypes = {
> +  removeSelectedCustomRequest: PropTypes.func,
> +  request: PropTypes.object,
> +  sendCustomRequest: PropTypes.func,
> +  updateRequest: PropTypes.func,

nit: isRequired

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:185
(Diff revision 18)
> +module.exports = connect(
> +  (state) => ({ request: getSelectedRequest(state) }),
> +  (dispatch) => ({
> +    removeSelectedCustomRequest: () => dispatch(Actions.removeSelectedCustomRequest()),
> +    sendCustomRequest: () => dispatch(Actions.sendCustomRequest()),
> +    updateRequest: (request, evt) => {

let's move updateRequest function out of connect and pass simple updateRequest action instead, so that make this.props.updateRequest as simple / generic as possbile.

```
// All updateRequest batch mode should be disabled to make UI editing in sync
updateRequest: (id, data) => dispatch(Actions.updateRequest(id, data, false)),
```

As above example, we can move relevant comment on top of updateRequest(). Remember all comment should start with a capital. I've seen there are a bunch of comments written with lower case, so please fix them as well.


I prefer to give an another name for outside function, let's call it updateCustomRequestFields

```
function parseRequestText(text, namereg, divider) {
 ...
}

function updateCustomRequestFields(evt, request, updateRequest) {

}
```

You will have to add an additional updateRequest argument for calling it in file scope.

How do you think?

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:188
(Diff revision 18)
> +    removeSelectedCustomRequest: () => dispatch(Actions.removeSelectedCustomRequest()),
> +    sendCustomRequest: () => dispatch(Actions.sendCustomRequest()),
> +    updateRequest: (request, evt) => {
> +      let val = evt.target.value;
> +      switch (evt.target.id) {
> +        case "custom-headers-value":

nit: 

I like the way we use switch here. Let's remain one dispatch(Actions.updateRequest()) at the button and use a data variables for keeping derived data payload.

ex:

let data;

case "custom-headers-value":
  ...
  data = { ... };
  break;
case "custom-method-value":
  ...
  data = { ... };
  break;

default:
  break;

dispatch(Actions.updateRequest(request.id, data, false));

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:193
(Diff revision 18)
> +        case "custom-headers-value":
> +          let customHeadersValue = val || "";
> +          // Parse text representation of multiple HTTP headers
> +          let headersArray = parseRequestText(customHeadersValue, "\\S+?", ":");
> +          if (customHeadersValue !== "" &&
> +              headersArray.length != customHeadersValue.split("\n").length) {

nit: !==

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:194
(Diff revision 18)
> +          let customHeadersValue = val || "";
> +          // Parse text representation of multiple HTTP headers
> +          let headersArray = parseRequestText(customHeadersValue, "\\S+?", ":");
> +          if (customHeadersValue !== "" &&
> +              headersArray.length != customHeadersValue.split("\n").length) {
> +            // update request.requestHeaders.customHeadersValue to show

nit: comment should start with a capital

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:205
(Diff revision 18)
> +                  customHeadersValue,
> +                },
> +              },
> +              false));
> +          } else {
> +            // only update header while headers string is parsable

nit: comment should start with a capital

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:243
(Diff revision 18)
> +          let queryString = queryArray.map(
> +            ({ name, value }) => name + "=" + value).join("&");
> +          let url = request.url.replace(getUrlQuery(request.url), queryString);
> +          if (customQueryValue !== "" &&
> +              queryArray.length != customQueryValue.split("\n").length) {
> +            // update request.customQueryValue to show current user inputs

nit: comment should start with a capital

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:251
(Diff revision 18)
> +              {
> +                customQueryValue,
> +              },
> +              false));
> +          } else {
> +            // update url while query string is parsable

nit: comment should start with a capital

::: devtools/client/netmonitor/shared/components/headers-panel.js:166
(Diff revision 18)
>              className: "tabpanel-summary-value textbox-input devtools-monospace",
>              readOnly: true,
>              value: `${status} ${statusText}`,
>            }),
>            NetMonitorController.supportsCustomRequest && input({
> -            className: "tool-button",
> +            className: "headers-tool-button tool-button",

nit: do not introduce headers-tool-button, but we can use .headers-summary .tool-button in netmonitor.css instead.

::: devtools/client/netmonitor/shared/components/headers-panel.js:172
(Diff revision 18)
>              onClick: cloneSelectedRequest,
>              type: "button",
>              value: EDIT_AND_RESEND,
>            }),
>            input({
> -            className: "tool-button",
> +            className: "headers-tool-button tool-button",

nit: do not introduce headers-tool-button, but we can use .headers-summary .tool-button in netmonitor.css instead.
Attachment #8824346 - Flags: review?(rchien) → review-
(Reporter)

Comment 56

2 years ago
mozreview-review
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review111020

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:67
(Diff revision 18)
> +    requestPostData.postData.text : "";
> +
> +  return (
> +    div({ className: "custom-request-panel" },
> +      div({ className: "tabpanel-summary-container custom-request" },
> +        label({ className: "tabpanel-summary-label custom-header" },

Is there any reason to define label here? I thought <label for="..."> is useful for combining with input, but I didn't see this combination here.

If there is no any purpose to use label, we can use div instead.

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:71
(Diff revision 18)
> +      div({ className: "tabpanel-summary-container custom-request" },
> +        label({ className: "tabpanel-summary-label custom-header" },
> +          CUSTOM_NEW_REQUEST
> +        ),
> +        button({
> +          className: "custom-tool-button tool-button",

nit: remove custom-tool-button and use .custom-request-panel .tool-button in netmonitor.css instead.

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:78
(Diff revision 18)
> +          onClick: sendCustomRequest,
> +        },
> +          CUSTOM_SEND
> +        ),
> +        button({
> +          className: "custom-tool-button tool-button",

nit: remove custom-tool-button and use .custom-request-panel .tool-button in netmonitor.css instead.
(Assignee)

Comment 57

2 years ago
mozreview-review-reply
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review110990

> let's move updateRequest function out of connect and pass simple updateRequest action instead, so that make this.props.updateRequest as simple / generic as possbile.
> 
> ```
> // All updateRequest batch mode should be disabled to make UI editing in sync
> updateRequest: (id, data) => dispatch(Actions.updateRequest(id, data, false)),
> ```
> 
> As above example, we can move relevant comment on top of updateRequest(). Remember all comment should start with a capital. I've seen there are a bunch of comments written with lower case, so please fix them as well.
> 
> 
> I prefer to give an another name for outside function, let's call it updateCustomRequestFields
> 
> ```
> function parseRequestText(text, namereg, divider) {
>  ...
> }
> 
> function updateCustomRequestFields(evt, request, updateRequest) {
> 
> }
> ```
> 
> You will have to add an additional updateRequest argument for calling it in file scope.
> 
> How do you think?

I think this abstraction is not necessary if we call `dispatch(Actions.updateRequest(request.id, data, false));` at bottom only once. We can put the explaination above that cal

> nit: 
> 
> I like the way we use switch here. Let's remain one dispatch(Actions.updateRequest()) at the button and use a data variables for keeping derived data payload.
> 
> ex:
> 
> let data;
> 
> case "custom-headers-value":
>   ...
>   data = { ... };
>   break;
> case "custom-method-value":
>   ...
>   data = { ... };
>   break;
> 
> default:
>   break;
> 
> dispatch(Actions.updateRequest(request.id, data, false));

great idea! let's do it
Comment hidden (mozreview-request)
Created attachment 8834274 [details]
UI comparisons .png

Great job! It is very close to the end. And there are some UI issues I noticed. (see attachment)

* Titles (New request, Query String, Request Headers) in new implementation is not align with input / textarea, and padding between titles are slightly different (but it was in XUL version) I think we can improve this part to make it look better.
* UI improvement - padding in headers panel is completely different with custom request panel, I'd prefer to align the padding of custom request panel with headers panel (which means thinner padding like we see in headers panel).
* Scrollbar will exceed view height so that bottom portion is invisible. We should apply height: calc(100vh - 48px); with a comment to indicate that minus 48px for devtools toolbar height - netmonitor toolbar height
* After docking panel to the side, vertical splitter is unable to shrink as short as possible. (XUL impl is ok in this case)
(Reporter)

Comment 60

2 years ago
mozreview-review
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review111386

Please comment 59 and address UI issues. Thanks!
Attachment #8824346 - Flags: review?(rchien) → review-

Updated

2 years ago
Iteration: 54.1 - Feb 6 → 54.2 - Feb 20
Comment hidden (mozreview-request)
(Assignee)

Comment 62

2 years ago
Last PR fixed related tests and make UI looks more like part of the Header panel(by applying same padding)


> * After docking panel to the side, vertical splitter is unable to shrink as short as possible. (XUL impl is ok in this case)

As offline discussion this issues is related to XUL splitter, The issue will be auto fixed when we apply HTML splitter (which is expected land very soon), so I suggest just let it as it is.
(Reporter)

Comment 63

2 years ago
mozreview-review
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review111444

::: devtools/client/netmonitor/reducers/requests.js:47
(Diff revision 20)
>    transferredSize: undefined,
>    totalTime: undefined,
>    eventTimings: undefined,
>    headersSize: undefined,
> +  // Text value is used for storing custom request query
> +  // Which only appears when user edit the custom requst form

nit: Just write capitcal for first charecter in a sentence. So "Which only ..." should be "which only..."

::: devtools/client/netmonitor/shared/components/custom-request-panel.js:260
(Diff revision 20)
> +  (state) => ({ request: getSelectedRequest(state) }),
> +  (dispatch) => ({
> +    removeSelectedCustomRequest: () => dispatch(Actions.removeSelectedCustomRequest()),
> +    sendCustomRequest: () => dispatch(Actions.sendCustomRequest()),
> +    // All updateRequest batch mode should be disabled to make UI editing in sync
> +    updateRequest: (id, data) => dispatch(Actions.updateRequest(id, data, false)),

nit: 

updateRequest() looks more like a generic action method now! It would be better and useful for any other purposes if we pass batch as third argument

```
updateRequest: (id, data, batch) => dispatch(Actions.updateRequest(id, data, batch)),
```

invoke updateRequest(request.id, data, false); and move comment to line 250.

::: devtools/client/themes/netmonitor.css:777
(Diff revision 20)
>  
> -/* Custom request form */
> +/* Custom request view */
> +
> +.custom-request-panel {
> +  overflow: auto;
> +  /* full view hight - toolbar height */

nit: // Full
Attachment #8824346 - Flags: review?(rchien) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 65

2 years ago
new PR addressed nits and tests should all green now
(Reporter)

Comment 66

2 years ago
mozreview-review
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review111478

It looks pretty good now. I like this patch :)

Only one UI issue remains: the vertical space between the first title (New Request) and input field is taller than the other titles (Request Headers and Request Body). We should make sure these height of vertical spaces identical.

r+ and please make sure try is green. Thanks a lot!
Attachment #8824346 - Flags: review?(rchien) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 68

2 years ago
Created attachment 8834342 [details]
customRequest

The first line of Custom Request view looks like <h1> for me, so I keep the same Header size and add more space between button and the next line, here's how it looks now.
(In reply to Fred Lin [:gasolin] from comment #68)
> Created attachment 8834342 [details]
> customRequest
> 
> The first line of Custom Request view looks like <h1> for me, so I keep the
> same Header size and add more space between button and the next line, here's
> how it looks now.

New UI LGTM. Thanks!

Comment 70

2 years ago
mozreview-review
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review111514

::: devtools/client/themes/netmonitor.css:1245
(Diff revision 22)
>  
>  .headers-summary .tool-button {
> -  border: 1px solid transparent;
> +  margin-inline-end: 6px;
> +}
> +
> +.tool-button {

You can get rid of the .tool-button class by using the .devtools-button class.

.devtools-button will automatically get a background when the button contains some text content.

Comment 71

2 years ago
mozreview-review
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review111576

I am seeing one UI issue

1) Select request with URL params
2) Open the custom form
3) Select some text in the 'Query String' field and press delete -> the text is removed
4) Press Delete one more time -> the text is back

The entire URL was read only previously.
Although, I would like a way to remove some arguments before sending (can be done in separate bugs).

Honza
Attachment #8824346 - Flags: review?(odvarko) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 73

2 years ago
Issued addressed, please kindly review it again
Comment hidden (mozreview-request)

Comment 75

2 years ago
mozreview-review
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review111944

Two comments:

1) I am able to properly edit the `Query String` in Custom Request form but pressing the Send button generates request with additional arguments from the original requests.

2) When the Custom request form is opened and I am editing the `Query String` URL arguments in the temporary request are not updated.

Again, I very much like this feature but, we can keep the `Query String` read only for now and implement this feature as part of another report.

Honza
Attachment #8824346 - Flags: review?(odvarko) → review-
Blocks: 1337620
(Assignee)

Comment 76

2 years ago
I tested in Fx51 and found the `Query String` field is editable and the result will update to the URL field (and vice versa).

In this patch Query String wont updated URL until it matches the correct params pair.
Comment hidden (mozreview-request)
(Assignee)

Comment 78

2 years ago
The PR fixed a bug for customize headers.

Haven't meet the first issue mentioned in comment 75 , could you provide your test URL for me?

Current implement neglect the invalid syntax entries, so if the syntax is not right(contain '=' in line), it won't be append to URL.

We should help user in custom request view with some form validation, but it could be done in another bug with UX involvement
Duplicate of this bug: 1337620
Comment hidden (mozreview-request)
(Assignee)

Comment 81

2 years ago
The PR fixed form validation bugs that Honza demo-ed in Vidyo.
Please kindly review it again, thanks!

Comment 82

2 years ago
> Duplicate of this bug: 1337620

Hi Honza, Thanks! Existing raw headers custom pane does not have vertical scroll bar when overflowing. Is this issue will be fixed by this new one?

Comment 83

2 years ago
> Hi Honza, Thanks! Existing raw headers custom pane does not have vertical
> scroll bar when overflowing. Is this issue will be fixed by this new one?

Oops sorry, Raw headers is not in custom-pane.

Comment 84

2 years ago
mozreview-review
Comment on attachment 8824346 [details]
Bug 1308449 - Implement custom request view;

https://reviewboard.mozilla.org/r/102874/#review112360

Thanks Fred, I don't see any other problems.

R+ assuming try is green.


Great job here!

Honza
Attachment #8824346 - Flags: review?(odvarko) → review+
(Assignee)

Comment 85

2 years ago
treeherder green, thanks!
Keywords: checkin-needed

Comment 86

2 years ago
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bdc79139892e
Implement custom request view;r=Honza,rickychien
Keywords: checkin-needed

Updated

2 years ago
Blocks: 1255116

Comment 87

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bdc79139892e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
(Assignee)

Comment 88

2 years ago
Hi, ciprian:

Please help us check if the duplicate Bug 1337620 (Blank custom-pane is displayed in the netmonitor when switching theme) is solved as well. The reproducible steps and videos are described in Bug 1337620. 

Thanks.
Hello Fred,

I've managed to reproduce the issue mentioned in bug 1337620. It's no longer reproducible with latest Nightly 54.0a1 (2017-02-20). 

Also, I performed tests around Custom request view and I can confirm that it's working as expected. Tests were done on: Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
Flags: qe-verify+
(Assignee)

Comment 90

2 years ago
Thanks Ciprian!

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.