Add an UI to manage response header columns

NEW
Assigned to

Status

()

Firefox
Developer Tools: Netmonitor
P3
enhancement
8 months ago
4 months ago

People

(Reporter: ntim, Assigned: Ruturaj Vartak)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 5 obsolete attachments)

(Reporter)

Description

8 months ago
Created attachment 8882048 [details]
Screenshot of management feature from Chrome

Bug 1360495 added the ability to show response header columns. It would be nice if you could add/delete response header columns manually in a custom UI à la Chrome.
Priority: -- → P3
Duplicate of this bug: 1390830
(Assignee)

Updated

6 months ago
Assignee: nobody → ruturaj
(Assignee)

Comment 2

6 months ago
Created attachment 8899869 [details] [diff] [review]
wip-1377013-1.patch

Hey Tim,
This is what I'm trying...

- This patch tries to set a variable `customHeaderColumnsUIAvailable`
- Based on this var, render the CustomHeader component from request-list-content.js
- GET / SET prefs
- test cases ...


The current patch is just doing the first part, that too I'm unable to get the console messages, could you please check ...
- what is wrong
- and if its the right direction I'm going..
Attachment #8899869 - Flags: review?(ntim.bugs)
(Assignee)

Comment 3

6 months ago
Comment on attachment 8899869 [details] [diff] [review]
wip-1377013-1.patch

Hey,
Ignore the patch fixed the issue. Please let me know if the flow is right.
Flags: needinfo?(ntim.bugs)
Attachment #8899869 - Flags: review?(ntim.bugs) → review-
(Assignee)

Comment 4

6 months ago
Created attachment 8901552 [details] [diff] [review]
WIP-1377013-2.patch

A Dirty working patch

- the state of clicked 'Manage custom header columns' has been hardcoded, to reduce too many clicks in development
- haven't specifically liked how I've handled `columns` Record and `customHeaderColumns` in request-list-header.js
- the UI is just functional
- cols auto alignment is pending
- no CSS polish is present

Please check the flow.
Attachment #8899869 - Attachment is obsolete: true
Attachment #8901552 - Flags: review?(ntim.bugs)
(Reporter)

Comment 5

6 months ago
Comment on attachment 8901552 [details] [diff] [review]
WIP-1377013-2.patch

Review of attachment 8901552 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! Sorry it took me a bit long to look at this.

The flow seems about right, I didn't go into much detail with this review, but here is a first round of comments.

A little general comment about naming (applies to all actions/reducers/constants/etc):

- CustomHeaderColumnsUI -> HeaderColumnsModal
- CustomHeaderColumns -> HeaderColumns

Also, can we get rid of the RESPONSE_HEADERS constant and make all response headers removable/editable ? Chrome's behaviour of making the default response header columns un-editable and un-removable doesn't make sense to me.
Those defaults should just be inside the new pref you've introduced. It would simplify some of your code.

::: devtools/client/netmonitor/src/actions/ui.js
@@ +71,5 @@
>  
>  /**
> + * Enable flag to show custom Header Columns UI
> + */
> +function toggleCustomHeaderColumnsUI() {

How about toggleCustomHeaderModal() ?

@@ +73,5 @@
> + * Enable flag to show custom Header Columns UI
> + */
> +function toggleCustomHeaderColumnsUI() {
> +  return {
> +    type: SHOW_CUSTOM_HEADER_COLUMNS_UI,

So the action name uses the verb "SHOW" but the reducer uses "toggle", can you use toggle here ?

::: devtools/client/netmonitor/src/components/request-list-content.js
@@ +43,5 @@
>      dispatch: PropTypes.func.isRequired,
>      displayedRequests: PropTypes.object.isRequired,
>      firstRequestStartedMillis: PropTypes.number.isRequired,
>      fromCache: PropTypes.bool,
> +    isCustomHeaderColumnsUIAvailable: PropTypes.bool.isRequired,

Can you simply name this headerColumnsModalShown ?

@@ +242,5 @@
> +      isCustomHeaderColumnsUIAvailable,
> +      customHeaderColumns,
> +      addCustomHeaderColumn,
> +      deleteCustomHeaderColumn,
> +      renameCustomHeaderColumn,

The last 4 props add a lot of noise IMO. Please don't include them here, and pass them directly in your custom-headers-ui.js file (using module.exports = connect(...)).

@@ +276,4 @@
>              }))
>            )
> +        ),
> +        isCustomHeaderColumnsUIAvailable && CustomHeadersUI({

You shouldn't render this inside request-list-content.js, otherwise the modal will not show when the netmonitor is empty (which doesn't make much sense).

::: devtools/client/netmonitor/src/components/request-list-header.js
@@ +94,5 @@
> +      customHeaderColumns
> +    } = this.props;
> +    console.log("in request-list-header", HEADERS, columns);
> +
> +    // DIRTY CODE STARTS !! I DON'T LIKE THIS BIT

What I would suggest here would be changing the "HEADERS" constant to exclude the waterfall, then creating a getAllHeaders() function that adds in the custom headers and the waterfall.
Attachment #8901552 - Flags: review?(ntim.bugs) → feedback+
(Reporter)

Updated

6 months ago
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 6

6 months ago
Created attachment 8903449 [details] [diff] [review]
wip-1377013-3.patch

- renamed variables
	CustomHeaderColumnsUI -> HeaderColumnsModal
	CustomHeaderColumns -> HeaderColumns
	toggleCustomHeaderColumnsUI -> toggleCustomHeaderModal
	isCustomHeaderColumnsUIAvailable -> headerColumnsModalShown
- moved Actions from request-list-content.js directly to custom-headers-ui.js
- ensured HeadersModal is available even if request-content isn't rendered
- dirty looking (request-list-header.js) code, I've created a local method. I tried moving RESPONSE_HEADERS and HEADERS, but with the current setup (use), it seemed contra-intuitive. Would've got to write more code for smaller things. But I can try again if we're pushing for it.
Attachment #8901552 - Attachment is obsolete: true
Attachment #8903449 - Flags: review?(ntim.bugs)
(Reporter)

Comment 7

6 months ago
Comment on attachment 8903449 [details] [diff] [review]
wip-1377013-3.patch

Review of attachment 8903449 [details] [diff] [review]:
-----------------------------------------------------------------

> I tried moving RESPONSE_HEADERS and HEADERS, but with the current setup (use), it seemed contra-intuitive. Would've got to write more code for smaller things. But I can try again if we're pushing for it.

Sorry I wasn't clear here.

What I wanted to say is to remove the RESPONSE_HEADERS constant and the code associated to it, and put the contents directly inside the `devtools.netmonitor.headerColumns` pref. Does that make sense ?

::: devtools/client/netmonitor/src/actions/ui.js
@@ +73,5 @@
> + * Enable flag to show custom Header Columns UI
> + */
> +function toggleCustomHeaderModal() {
> +  return {
> +    type: SHOW_CUSTOM_HEADER_COLUMNS_UI,

As mentioned in the previous review, please keep this action name in sync with the reducer function.

TOGGLE_CUSTOM_HEADER_MODAL

::: devtools/client/netmonitor/src/components/request-list-header.js
@@ +83,5 @@
>        }, 500);
>      }
>    },
>  
> +  getListHeaders() {

I still don't like what's inside the method, though I don't want to block review on this.

Once the rest of the patch looks OK, I'll figure out a proper solution (I have a few ideas in mind that I'll discuss with Honza or Ricky).

::: devtools/client/netmonitor/src/reducers/ui.js
@@ +63,5 @@
> +
> +  // forced for development
> +  headerColumnsModalShown: true,
> +  headerColumns:
> +    JSON.parse(Services.prefs.getCharPref("devtools.netmonitor.headerColumns"))

This should be set here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/src/utils/create-store.js#50
Attachment #8903449 - Flags: review?(ntim.bugs)
(Assignee)

Comment 8

6 months ago
Hey Tim,

I was using `devtools.netmonitor.headerColumns` to save a list of the custom added headers only. If we plan to have this pref to include the existing RESPONSE_HEADERS list, then we'll have to have an attribute assigned to the item as well. The item in the list would be something like `{name: "Server", isCustom: false}` for typical headers and `{name: "X-Header-Other", isCustom: true}`. Only the isCustom=true header would be removable / editable.

Is my understand in sync with your idea?

Thanks.
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 9

6 months ago
(In reply to Ruturaj Vartak from comment #8)
> Hey Tim,
> 
> I was using `devtools.netmonitor.headerColumns` to save a list of the custom
> added headers only. If we plan to have this pref to include the existing
> RESPONSE_HEADERS list, then we'll have to have an attribute assigned to the
> item as well. The item in the list would be something like `{name: "Server",
> isCustom: false}` for typical headers and `{name: "X-Header-Other",
> isCustom: true}`. Only the isCustom=true header would be removable /
> editable.
> 
> Is my understand in sync with your idea?

All headers should be editable/removable (including the default ones), so no need for the isCustom flag.
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 10

6 months ago
Created attachment 8904603 [details] [diff] [review]
wip-1377013-4.patch

- removed usage RESPONSE_HEADERS: have commented a lot, just for visual ref. of history.
- renamed TOGGLE_CUSTOM_HEADER_MODAL: I'd forgotten last time around.
Attachment #8903449 - Attachment is obsolete: true
Attachment #8904603 - Flags: review?(ntim.bugs)
(Assignee)

Comment 11

6 months ago
Created attachment 8904605 [details]
custom-headers-menu.png

Removed sub-menu list derived from RESPONSE_HEADERS, changed it to a simple `Custom Headers...` menu.
(Reporter)

Comment 12

6 months ago
Comment on attachment 8904603 [details] [diff] [review]
wip-1377013-4.patch

Review of attachment 8904603 [details] [diff] [review]:
-----------------------------------------------------------------

This is closer to what I was thinking of.

The devtools.netmonitor.headerColumns pref should be set to: [
  "Cache-Control",
  "Connection",
  "Content-Encoding",
  "Content-Length",
  "ETag",
  "Keep-Alive",
  "Last-Modified",
  "Server",
  "Vary"
];

The user should be able to add/remove items from that list.

Also, the context menu should be kept as it was in the previous patch.

::: devtools/client/netmonitor/configs/local.json
@@ +1,3 @@
> +{
> +  "title": "Netmonitor",
> +  "environment": "development",

nit: remove this file

::: devtools/client/netmonitor/index.js
@@ +24,4 @@
>       "[\"status\",\"method\",\"file\",\"domain\",\"cause\"," +
>       "\"type\",\"transferred\",\"contentSize\",\"waterfall\"]"
>  );
> +pref("devtools.netmonitor.headerColumns", "[]");

The value here should contain everything that was in the RESPONSE_HEADERS constant (this is what I meant by default headers).

::: devtools/client/netmonitor/src/constants.js
@@ +108,5 @@
> +//   "Keep-Alive",
> +//   "Last-Modified",
> +//   "Server",
> +//   "Vary"
> +// ];

nit: remove this, and move the contents into devtools.netmonitor.headerColumns pref.

::: devtools/client/netmonitor/src/reducers/ui.js
@@ +18,2 @@
>    RESET_COLUMNS,
> +  // RESPONSE_HEADERS,

nit: remove this.

@@ +50,5 @@
> +//   Object.assign(
> +//     cols,
> +//     // RESPONSE_HEADERS.reduce((acc, header) => Object.assign(acc, { [header]: false }), {})
> +//   )
> +// );

Now that you no longer need to take care of RESPONSE_HEADERS, combine this with the cols variable above:

const Columns = I.Record({
  ...
});

::: devtools/client/netmonitor/src/request-list-header-context-menu.js
@@ +66,5 @@
> +    // // Setup Custom Header Columns menu
> +    // subMenu.responseHeaders.push({ type: "separator" });
> +    // subMenu.responseHeaders.push({
> +    //   label: L10N.getStr("netmonitor.toolbar.headerColumnsMenu")
> +    // });

Please keep the submenu, it should contain all the user and default response headers + the menu item to customize it.

::: devtools/client/preferences/devtools.js
@@ +167,4 @@
>  pref("devtools.netmonitor.visibleColumns",
>    "[\"status\",\"method\",\"file\",\"domain\",\"cause\",\"type\",\"transferred\",\"contentSize\",\"waterfall\"]"
>  );
> +pref("devtools.netmonitor.headerColumns", "[]");

The value here should contain everything that was in the RESPONSE_HEADERS constant (this is what I meant by default headers).

::: devtools/client/shared/components/reps/reps.js
@@ +2134,4 @@
>    const numProperties = propertiesNames.length;
>  
>    // We want to have at most a hundred slices.
> +  const bucketSize = Math.pow(10, Math.max(2, Math.ceil(Math.log10(numProperties)) - 2));

nit: undo changes in reps.js
Attachment #8904603 - Flags: review?(ntim.bugs)
(Assignee)

Comment 13

6 months ago
Hey Tim,

So, The `devtools.netmonitor.headerColumns` should by default have those `RESPONSE_HEADERS` list in `netmonitor/index.js` and `client/preferences/devtools.js`. And things should flow from that pref. However...

Expectation #1
I assume the expectation would be - on the `Reset Columns` of the ContextMenu, this pref / list should get updated back, right? In such a case, I'll have to still keep `RESPONSE_HEADERS` in constants.js. If this expectation is incorrect, then I can assume its OK to not have that constant set.

Suggestions? Since we initially wanted to do away with RESPONSE_HEADERS.

Expectation #2
From the context menu, the user should be able to toggle the Response Headers as currently found - Right?

I guess above point too is expected right ?

thanks.
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 14

6 months ago
(In reply to Ruturaj Vartak from comment #13)
> Hey Tim,
> 
> So, The `devtools.netmonitor.headerColumns` should by default have those
> `RESPONSE_HEADERS` list in `netmonitor/index.js` and
> `client/preferences/devtools.js`. And things should flow from that pref.
> However...
> 
> Expectation #1
> I assume the expectation would be - on the `Reset Columns` of the
> ContextMenu, this pref / list should get updated back, right? In such a
> case, I'll have to still keep `RESPONSE_HEADERS` in constants.js. If this
> expectation is incorrect, then I can assume its OK to not have that constant
> set.
>
> Suggestions? Since we initially wanted to do away with RESPONSE_HEADERS.

There's Services.prefs.clearUserPref() which will restore the default pref value.

> Expectation #2
> From the context menu, the user should be able to toggle the Response
> Headers as currently found - Right?
Yes
Flags: needinfo?(ntim.bugs)
Severity: normal → enhancement
(Assignee)

Comment 15

5 months ago
Created attachment 8909018 [details] [diff] [review]
WIP-1377013-5.patch

Hey Tim, 

- Worked on having a common `headerColumns` pref.
- There are many unwanted hunks, which are from some suggestions by Honza on a quick fix in bug#1399390. I'll remove them in coming commits.
- I'm not sure how to code shouldComponentUpdate for request-list-item; Not sure how my idea will perform, I've left commented code there for an idea that I had.
Attachment #8904603 - Attachment is obsolete: true
Attachment #8909018 - Flags: review?(ntim.bugs)
(Reporter)

Comment 16

5 months ago
Comment on attachment 8909018 [details] [diff] [review]
WIP-1377013-5.patch

Review of attachment 8909018 [details] [diff] [review]:
-----------------------------------------------------------------

Redirecting to Ricky for review.
Attachment #8909018 - Flags: review?(ntim.bugs) → review?(rchien)
Hi, can you rebase your patch on top of the latest commit, I saw some conflict while applying your patch. Thanks
Flags: needinfo?(ruturaj)
Attachment #8909018 - Flags: review?(rchien)
(Assignee)

Comment 18

5 months ago
Created attachment 8912097 [details] [diff] [review]
WIP-1377013-6.patch

Hi,
I've merged the latest master, should apply cleanly now..
Attachment #8909018 - Attachment is obsolete: true
Flags: needinfo?(ruturaj)
Attachment #8912097 - Flags: review?(rchien)
Comment on attachment 8912097 [details] [diff] [review]
WIP-1377013-6.patch

Review of attachment 8912097 [details] [diff] [review]:
-----------------------------------------------------------------

Ruturaj Vartak,

At first, I'm really appreciative of implementing this big patch! It seems a bit complicated to me to spend for a while reviewing.

I haven't finished scanning all changes since there are some issues perhaps we can address them in advance.

1. I'm still seeing a few of unused comments scattered in your patch, please clean them.

2. Running ./mach build will throw failure like

The error occurred while processing the following file:
 0:17.57
 0:17.57     /Users/Ricky/Documents/mozilla-unified/devtools/templates.mozbuild
 0:17.57
 0:17.57 This file was included as part of processing:
 0:17.57
 0:17.57     /Users/Ricky/Documents/mozilla-unified/devtools/client/netmonitor/src/components/moz.build

you probably forgot to modify mozbuild according to the folder changes.

3. The header modal always shows up on the top of entire netmonitor panel, it looks weird. (see attachment).

::: devtools/client/netmonitor/src/actions/ui.js
@@ +90,5 @@
> +  return {
> +    type: TOGGLE_CUSTOM_HEADER_MODAL,
> +  };
> +}
> +

nit: adding a brief comment for each action.

::: devtools/client/netmonitor/src/components/request-list-header.js
@@ +126,5 @@
>  
>      return (
>        div({ className: "devtools-toolbar requests-list-headers" },
> +        this.getListHeaders().map((header) => {
> +          // console.log(header, header.name);

remove console.log
Attachment #8912097 - Flags: review?(rchien) → review-
Created attachment 8912271 [details]
header-modal.png

The patch is unable to run on toolbox. I just test on launchpad but the header modal looks like really weird.
(Assignee)

Comment 21

5 months ago
Hey Ricky,

Thanks for the review.

Yes I know this entire patch is not yet ready. I have implemented the whole thing with some idea / and directions from Tim (:ntim). Unfortunately he seems to be under the weather; And I know it would be difficult to go over this patch in one go :) Even when I cameback to the patch after your request, I felt disoriented! :D

- I haven't yet focused on look and feel, css, etc. just yet. Just got a bare bone minimum working. Once the flow is clear, I will polish it up with help from you guys.
- For the sake of development, I had left the Modal `open`
- Hence there might be many console messages, random commented out code blocks lying
- I'm worried about "I'm not sure how to code shouldComponentUpdate for request-list-item; Not sure how my idea will perform, I've left commented code there for an idea that I had." this part as of now
- Once the whole thing is done, I intend on adding some test-cases.

When you're done looking in more detail could you please look at comment#15 ?

I'll get a patch to work on `./mach build` env, currently was just working on launchpad for development.
(Assignee)

Comment 22

5 months ago
Hey,

I merged latest master@d486c00 - and the DevTools doesn't open up in the ./mach build env. Firefox dumps this error. So couldn't verify the moz.build order fix worked or not (even though I'm sure it would've worked).

=======================================================================================

$ /home/rutu/code/gecko-dev/obj-x86_64-pc-linux-gnu/dist/bin/firefox -P development
console.error: 
Object
    - message = Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]
    - fileName = undefined
    - lineNumber = 132
    - stack = @undefined:132:NaN|switchWebconsole@resource://devtools/client/definitions.js:132:7|@resource://devtools/client/definitions.js:138:1|requireHook@resource://devtools/shared/Loader.jsm:71:16|@resource://devtools/client/framework/devtools.js:25:3|requireHook@resource://devtools/shared/Loader.jsm:71:16|@resource://devtools/client/framework/devtools-browser.js:18:21|requireHook@resource://devtools/shared/Loader.jsm:71:16|require@resource://devtools/shared/Loader.jsm:131:12|initDevTools@file:///home/rutu/code/gecko-dev/obj-x86_64-pc-linux-gnu/dist/bin/browser/components/devtools-startup.js:409:5|_initDevTools@chrome://devtools-shim/content/DevToolsShim.jsm:200:5|get gDevTools@chrome://devtools-shim/content/DevToolsShim.jsm:58:7|restoreDevToolsSession@chrome://devtools-shim/content/DevToolsShim.jsm:162:5|ssi_restoreWindows@resource:///modules/sessionstore/SessionStore.jsm:3585:5|initializeWindow@resource:///modules/sessionstore/SessionStore.jsm:1172:11|onBeforeBrowserWindowShown/<@resource:///modules/sessionstore/SessionStore.jsm:1323:9|promise callback*onBeforeBrowserWindowShown@resource:///modules/sessionstore/SessionStore.jsm:1308:5|ssi_observe@resource:///modules/sessionstore/SessionStore.jsm:769:9|onLoad@chrome://browser/content/browser.js:1317:5|onload@chrome://browser/content/browser.xul:1:1|
    - toString = () => toString
JavaScript error: undefined, line 132: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]
*** UTM:SVC TimerManager:registerTimer called after profile-before-change notification. Ignoring timer registration for id: telemetry_modules_ping


=================================================

Should I wait for a day, try merging again?
Flags: needinfo?(rchien)
You can try to build latest Firefox without applying your patch, and see whether the error is still there.

Your error comes from nsIPrefBranch.getBoolPref which is pref() relevant API, so you might want to take a look and make sure there is no typos.
Flags: needinfo?(rchien)
You need to log in before you can comment on or make changes to this bug.