Ciprian Georgiu, Desktop QA Reporter
	Description • 8 years ago

[Affected versions]:
- latest Nightly 55.0a1
- latest Aurora 54.0a2

[Affected platforms]:
- Windows 10 x64
- Ubuntu 16.04 x64 LTS
- macOS 10.12

[Steps to reproduce]:
1. Start Firefox. 
2. Go to a website that has many requests e.g.: http://www.bbc.com/, https://www.youtube.com/ or http://edition.cnn.com/.
3. Open netmonitor. (Ctrl + Shift + Q)
4. Change rapidly the height of netmonitor panel. (drag it up and down)

[Expected result]:
- There is no lag while resizing the netmonitor height.

[Actual result]:
- A noticeable lag can be seen while resizing.

[Regression range]:
Last good revision: 2cb7e2608b1d0697364694ca8faf99692727c2ec
First bad revision: 030a883d87a71bda616a7991d7cdc044d16eed56
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2cb7e2608b1d0697364694ca8faf99692727c2ec&tochange=030a883d87a71bda616a7991d7cdc044d16eed56

Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1336383

[Additional notes]:
- this issue appears to not be reproduced on sites with less requests e.g. https://www.wikipedia.org/
- also, there is a clear delay when changing the Detail panel width (left to side)

	gasolin@mozilla.com
	Comment 2 • 8 years ago

As discussion with Honza & Ricky, this is most likely caused by using flex in CSS (it needs to be removed)
We should use fast layout CSS (table-layout: fixed)

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 5 • 8 years ago
mozreview-review

Comment on attachment 8858854 [details]
Bug 1349173 - Use div table layout to reduce reflow

https://reviewboard.mozilla.org/r/130850/#review133622

Great patch, I am seeing significant performance improvement here!

See my inline comments.

Honza

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:212
(Diff revision 2)
>    --timings-scale: 1;
>    --timings-rev-scale: 1;
>  }
>  
> -.requests-list-subitem {
> -  display: flex;
> +.requests-list-column {
> +  display: table-cell;

Thanks for renaming this (subitem -> column)

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:282
(Diff revision 2)
> -  flex: none;
> +  display: inline-block;
> +  width: 7px;
>    height: 4px;
>    margin-inline-start: 3px;
>    margin-inline-end: 6px;
> -  width: 7px;
> +  vertical-align: middle;

The sort icons is now displayed next to the label instead of at the end of the header button.

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:424
(Diff revision 2)
> -.requests-list-icon-and-file {
> -  width: 22vw;
> +/* File column */
> +
> +.requests-list-file {
> +  width: 260px;
> +  max-width: 260px;
> +  min-width: 260px;

This pattern is here several times (i.e. width + max-width + min-width). Why all these props are needed? Could we have a comment explaining it?

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:1117
(Diff revision 2)
>    border-color: transparent;
>    padding-inline-end: 0;
> -  cursor: pointer;
> +  margin-top: 3px;
> +  margin-bottom: 3px;
>    margin-inline-end: 1em;
>  }

There will be probably collision with patch for the status bar (bug 1353057). Let's see which one lands first.

	gasolin@mozilla.com
	Comment 8 • 8 years ago
mozreview-review

Comment on attachment 8858854 [details]
Bug 1349173 - Use div table layout to reduce reflow

https://reviewboard.mozilla.org/r/130850/#review133654

Great progress, it will be nice if we can use table element to replace div, the code will be more readable

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:192
(Diff revision 3)
> -  display: flex;
> -  flex-wrap: nowrap;
>  }
>  
> -.theme-firebug .requests-list-toolbar {
> -  height: 19px !important;
> +.requests-list-table {
> +  display: table;

can we use real table in request list? usd `table` instead of `div` in `request-list-table` element

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:199
(Diff revision 3)
> +  width: 100%;
> +  height: 100%;
>  }
>  
>  .requests-list-contents {
> -  height: 100%;
> +  display: table-row-group;

usd `tbody` instead of `div` in `request-list-contents` element

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:212
(Diff revision 3)
>    --timings-scale: 1;
>    --timings-rev-scale: 1;
>  }
>  
> -.requests-list-subitem {
> -  display: flex;
> +.requests-list-column {
> +  display: table-cell;

can we use real table in request list? usd `td` instead of `div` in each column

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:678
(Diff revision 3)
>  }
>  
> +/* Request list item */
> +
>  .request-list-item {
> -  display: flex;
> +  display: table-row;

can we use real table in request list? usd `tr` instead of `div` in each row, then we can reduce some styles

::: devtools/client/netmonitor/src/components/request-list-column-cause.js:13
(Diff revision 3)
>    createClass,
>    DOM,
>    PropTypes,
>  } = require("devtools/client/shared/vendor/react");
>  
> -const { div, span } = DOM;
> +const { div } = DOM;

const { td } = DOM;

::: devtools/client/netmonitor/src/components/request-list-column-cause.js:43
(Diff revision 3)
> -      causeUri = cause.loadingDocumentUri;
>        causeHasStack = cause.stacktrace && cause.stacktrace.length > 0;
>      }
>  
>      return (
> -      div({
> +      div({ className: "requests-list-column requests-list-cause", title: causeType },

td instead of `div`

::: devtools/client/netmonitor/src/components/request-list-column-content-size.js:14
(Diff revision 3)
>    DOM,
>    PropTypes,
>  } = require("devtools/client/shared/vendor/react");
>  const { getFormattedSize } = require("../utils/format-utils");
>  
> -const { div, span } = DOM;
> +const { div } = DOM;

const { td } = DOM;

::: devtools/client/netmonitor/src/components/request-list-column-domain.js:15
(Diff revision 3)
>    PropTypes,
>  } = require("devtools/client/shared/vendor/react");
>  const { L10N } = require("../utils/l10n");
>  const { propertiesEqual } = require("../utils/request-utils");
>  
> -const { div, span } = DOM;
> +const { div } = DOM;

const { div, td } = DOM;

::: devtools/client/netmonitor/src/components/request-list-column-domain.js:37
(Diff revision 3)
>      return !propertiesEqual(UPDATED_DOMAIN_PROPS, this.props.item, nextProps.item);
>    },
>  
>    render() {
> -    const { item, onSecurityIconClick } = this.props;
> -    const { urlDetails, remoteAddress, securityState } = item;
> +    let { item, onSecurityIconClick } = this.props;
> +    let { securityState, urlDetails: { host, isLocal } } = item;

why the `remoteAddress` is removed? It's not that useful to show the same info in the tooltip

	gasolin@mozilla.com
	Comment 10 • 8 years ago
mozreview-review

Comment on attachment 8858854 [details]
Bug 1349173 - Use div table layout to reduce reflow

https://reviewboard.mozilla.org/r/130850/#review133676

::: devtools/client/netmonitor/src/components/request-list-column-status.js:60
(Diff revision 4)
>        }
>      }
>  
>      return (
> -        div({ className: "requests-list-subitem requests-list-status", title },
> +        div({ className: "requests-list-column requests-list-status", title },
>          div({ className: "requests-list-status-icon", "data-code": code }),

missing intent here

::: devtools/client/netmonitor/src/components/request-list-content.js:237
(Diff revision 4)
> +      waterfallWidth,
>      } = this.props;
>  
>      return (
> +      div({ className: "requests-list-wrapper"},
> +        div({ className: "requests-list-table"},

use `table` instead of `div`

::: devtools/client/netmonitor/src/components/request-list-content.js:238
(Diff revision 4)
>      } = this.props;
>  
>      return (
> +      div({ className: "requests-list-wrapper"},
> +        div({ className: "requests-list-table"},
> -      div({
> +          div({

use `tbody` instead of `div`

::: devtools/client/netmonitor/src/components/request-list-item.js:126
(Diff revision 4)
> -    }
> -
> -    classList.push(index % 2 ? "odd" : "even");
>  
>      return (
>        div({

use `tr` instead of `div`

::: devtools/client/netmonitor/src/constants.js:185
(Diff revision 4)
>  const general = {
>    ACTIVITY_TYPE,
>    EVENTS,
>    FILTER_SEARCH_DELAY: 200,
>    HEADERS,
> -  // 100 KB in bytes
> +  SOURCE_SYNTAX_HIGHLIGHT_MAX_FILE_SIZE: 102400, // 100 KB in bytes

It's not used elsewhre, we could reuse this constant for our editor or remove it

::: devtools/client/netmonitor/src/selectors/ui.js:32
(Diff revision 4)
>    const lastEventMillis = Math.max(requests.lastEndedMillis,
>                                     timingMarkers.firstDocumentDOMContentLoadedTimestamp,
>                                     timingMarkers.firstDocumentLoadTimestamp);
>    const longestWidth = lastEventMillis - requests.firstStartedMillis;
> -  return Math.min(Math.max(ui.waterfallWidth / longestWidth, EPSILON), 1);
> +  return Math.min(Math.max(
> +    (ui.waterfallWidth - REQUESTS_WATERFALL.LABEL_WIDTH) / longestWidth, EPSILON), 1);

seems like those waterfall related change can be provided as a separate patch

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 11 • 8 years ago
mozreview-review-reply

Comment on attachment 8858854 [details]
Bug 1349173 - Use div table layout to reduce reflow

https://reviewboard.mozilla.org/r/130850/#review133654

Fred, I saw almost of your comments are about using actual table instead of div with display: table approach. I'm going to drop these issues but we can discuss this on bugzilla comment.

There are tons of articles comparing using actual table and div with display: table. Both of them are good. Table could provide more readable but div is more flexibility. The main reason why I'd prefer to use div is because we can gain the same performance but it's more flexibility and easier to adopt react-virtualized (Chromium's network monitor is using a similiar react-virtualized mechanism to reduce rendering too many requests in page.)

> why the `remoteAddress` is removed? It's not that useful to show the same info in the tooltip

Good question. The reason is that we've added a new Remote IP column recently, so I think the IP tooltip can be removed safely. But I'm ok to move it back if we think it's still useful.

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 15 • 8 years ago

I think is time to ask second round review. Patch will separate into smaller including (1. introducing empty file icon. 2. waterfall change. 3. using div table. 4. fix mochitest) soon.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 16 • 8 years ago
mozreview-review

Comment on attachment 8858854 [details]
Bug 1349173 - Use div table layout to reduce reflow

https://reviewboard.mozilla.org/r/130850/#review134244

I am seeing performance regression in revision 5-6

I suspect px -> vw change.

Honza

	gasolin@mozilla.com
	Comment 17 • 8 years ago
mozreview-review

Comment on attachment 8858854 [details]
Bug 1349173 - Use div table layout to reduce reflow

https://reviewboard.mozilla.org/r/130850/#review134222

::: devtools/client/netmonitor/src/components/request-list-column-domain.js:21
(Diff revision 8)
>  
>  const UPDATED_DOMAIN_PROPS = [
>    "urlDetails",
>    "remoteAddress",
>    "securityState",
>  ];

we can also  reorder the list properties in alphabetic as well

::: devtools/client/netmonitor/src/components/request-list-column-domain.js
(Diff revision 8)
>      } else if (securityState) {
>        iconClassList.push(`security-state-${securityState}`);
>        iconTitle = L10N.getStr(`netmonitor.security.state.${securityState}`);
>      }
>  
> -    let title = urlDetails.host + (remoteAddress ? ` (${remoteAddress})` : "");

remoteIP is a hidden column, user may not choose to display it. Therefore it still useful to show the remote IP in tooltip

::: devtools/client/netmonitor/src/components/request-list-column-remote-ip.js:32
(Diff revision 8)
> -    const { remoteAddress, remotePort } = this.props.item;
> -    let remoteSummary = remoteAddress ? `${remoteAddress}:${remotePort}` : "";
> +    let { remoteAddress, remotePort } = this.props.item;
> +    let remoteIP = remoteAddress ? `${remoteAddress}:${remotePort}` : "unknown";
>  
>      return (
> -      div({ className: "requests-list-subitem requests-list-remoteip" },
> -        span({ className: "subitem-label", title: remoteSummary }, remoteSummary),
> +      div({ className: "requests-list-column requests-list-remoteip", title: remoteIP },
> +        remoteIP

we can do it in single line if no lint error

::: devtools/client/netmonitor/src/components/request-list-column-transferred-size.js:51
(Diff revision 8)
>        text = L10N.getStr("networkMenu.sizeUnavailable");
>      }
>  
>      return (
> -      div({
> -        className: "requests-list-subitem requests-list-transferred",
> +      div({ className: "requests-list-column requests-list-transferred", title: text },
> +        text

can do in one line if no lint error

::: devtools/client/netmonitor/src/components/request-list-column-waterfall.js:66
(Diff revision 8)
>  
> -// List of properties of the timing info we want to create boxes for
> -const TIMING_KEYS = ["blocked", "dns", "connect", "send", "wait", "receive"];
> -
>  function timingBoxes(item) {
> -  const { eventTimings, totalTime, fromCache, fromServiceWorker } = item;
> +  let { eventTimings, totalTime, fromCache, fromServiceWorker } = item;

let { eventTimings, fromCache, fromServiceWorker, totalTime } = item;

::: devtools/client/netmonitor/src/components/request-list-header.js:82
(Diff revision 8)
> +    if (waterfallHeader) {
> -    // Measure its width and update the 'waterfallWidth' property in the store.
> +      // Measure its width and update the 'waterfallWidth' property in the store.
> -    // The 'waterfallWidth' will be further updated on every window resize.
> +      // The 'waterfallWidth' will be further updated on every window resize.
> -    setTimeout(() => {
> +      setTimeout(() => {
> -      let { width } = this.refs.header.getBoundingClientRect();
> -      this.props.resizeWaterfall(width);
> +        this.props.resizeWaterfall(waterfallHeader.getBoundingClientRect().width);
> +      }, 500);

maybe we can put something like `RESIZE_WATERFALL_TICKS = 500` into constant.js for future tuning.

Could move 

```
const REQUESTS_WATERFALL_HEADER_TICKS_MULTIPLE = 5; // ms
const REQUESTS_WATERFALL_HEADER_TICKS_SPACING_MIN = 60; // px
```

as well

::: devtools/client/netmonitor/src/constants.js:185
(Diff revision 8)
>  const general = {
>    ACTIVITY_TYPE,
>    EVENTS,
>    FILTER_SEARCH_DELAY: 200,
>    HEADERS,
> -  // 100 KB in bytes
> +  SYNTAX_HIGHLIGHT_MAX_SIZE: 51200, // 100 KB in bytes

to make it more clear, I suggest use `SOURCE_EDITOR_SYNTAX_HIGHLIGHT_MAX_SIZE`

& 51200 is `50 KB in bytes`

The constant could be `SOURCE_EDITOR_SYNTAX_HIGHLIGHT_MAX_SIZE = 51200, // 50 KB in bytes`

::: devtools/client/netmonitor/test/browser_net_security-state.js:28
(Diff revision 8)
>    gStore.dispatch(Actions.batchEnable(false));
>  
>    yield performRequests();
>  
>    for (let subitemNode of Array.from(document.querySelectorAll(
> -    "requests-list-subitem.requests-list-security-and-domain"))) {
> +    "requests-list-cell.requests-list-security-and-domain"))) {

does the replacement of "requests-list-subitem" is the "requests-list-column" ?

::: devtools/client/netmonitor/test/head.js:90
(Diff revision 8)
>  
>  // Always reset some prefs to their original values after the test finishes.
>  const gDefaultFilters = Services.prefs.getCharPref("devtools.netmonitor.filters");
>  
> +// Reveal all hidden columns for test
> +Services.prefs.setCharPref("devtools.netmonitor.hiddenColumns", "[]");

I think we should only pref on all headers when needed, or we may get pass some edge case with default column set.

	gasolin@mozilla.com
	Comment 18 • 8 years ago

Comment on attachment 8858854 [details]
Bug 1349173 - Use div table layout to reduce reflow

set f? ntim for style feedback

	gasolin@mozilla.com
	Comment 19 • 8 years ago
mozreview-review

Comment on attachment 8858854 [details]
Bug 1349173 - Use div table layout to reduce reflow

https://reviewboard.mozilla.org/r/130850/#review134264

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:348
(Diff revision 8)
> + */
>  
> -/* Network requests table: specific column dimensions */
> +/* Status column */
>  
>  .requests-list-status {
> -  max-width: 6em;
> +  width: 7vw;

if we need to specify each column's  width, we can define it with CSS variable, ex`--column-status-fixed-width`

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:569
(Diff revision 8)
> -  border-radius: 0; /* squares */
> +  padding-inline-start: 0;
> +  padding-inline-end: 4px;
> +  background-repeat: repeat-y;
> +  background-position: left center;
> +  /* Background created on a <canvas> in js. */
> +  /* @see devtools/client/netmonitor/netmonitor-view.js */

@see devtools/client/netmonitor/waterfall-background.js

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 20 • 8 years ago
mozreview-review-reply

Comment on attachment 8858854 [details]
Bug 1349173 - Use div table layout to reduce reflow

https://reviewboard.mozilla.org/r/130850/#review134222

> remoteIP is a hidden column, user may not choose to display it. Therefore it still useful to show the remote IP in tooltip

Hmm.. make sense. Let's put it back.

> we can do it in single line if no lint error

It will exceed 90 chars and throw linter error, so we can't :/

> can do in one line if no lint error

It will exceed 90 chars and throw linter error, so we can't :/

> I think we should only pref on all headers when needed, or we may get pass some edge case with default column set.

Yep! That's what I'm doing. Empty array for `hiddenColumns` means pref on all headers.

	Tim Nguyen :ntim
	Comment 22 • 8 years ago

Has anyone experimented with CSS grid? Might be better solution than table layouts.

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 23 • 8 years ago

We're converting flex to table (div table) since performance issue. CSS grid is also a good option. However, I don't know performance is our first target I cannot make sure Grid can perform as faster as table. In fact, our use case is not that complicated and needed to choose Grid (table can perform very well in such large tabular data IMO).

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 25 • 8 years ago

As I mentioned in meeting yesterday, waterfall is also an another noticeable performance impact. One case is that window resize is pretty slow and it causes waterfall re-calculate width from waterfall header element and then dispatch the latest width down to redux store. Every single resize will impact a one way data flow (action -> reducer -> store -> react components), so it's a terrible scheme we should avoid.

Note that Chromium is using a fixed canvas with `width: 210px` to render its waterfall timeline.

The latest patch has updated and uses fix `width: 220px` for waterfall column. Now I see the window resize is very responsive and smooth. Furthermore, column width has set as a percentage and remove these width, max-width, min-width hacks. Please check my patch again.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 26 • 8 years ago

(In reply to Ricky Chien [:rickychien] from comment #25)
> The latest patch has updated and uses fix `width: 220px` for waterfall
> column. Now I see the window resize is very responsive and smooth.
I am not seeing significant performance improvement. The thing is that the Timeline column benefits a lot from more horizontal space (width) especially when there is more requests. Could we improve the Waterfall rather in different bug or at least not set fixed width.

The patch is great, but two things related to hidden columns:

1) When hiding columns there is plenty of space not used by any data while some columns-data are hardly readable. See the attached screenshot.

2) When hiding the Timeline column all columns disappear and there is no way two get them back (without changing hiddenColumns pref manually)

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 27 • 8 years ago
mozreview-review

Comment on attachment 8858854 [details]
Bug 1349173 - Use div table layout to reduce reflow

https://reviewboard.mozilla.org/r/130850/#review134270

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:342
(Diff revision 9)
> -/* Network requests table: specific column dimensions */
> +/*
> + * In order to fix the width of header columns and body columns correspondingly
> + * and prevent width to be adjusted by waterfall header, so we give fixed value
> + * for width, max-width, min-width in columns
> + */

Is this comment still valid?
Perhaps we should move it closer to `.requests-list-waterfall` rule?

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 29 • 8 years ago
mozreview-review

Comment on attachment 8858854 [details]
Bug 1349173 - Use div table layout to reduce reflow

https://reviewboard.mozilla.org/r/130850/#review134328

Yup fixed, but please see my comment #26 yet.

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 30 • 8 years ago

I am seeing collisions when applying the patch:

patching file devtools/client/netmonitor/src/assets/styles/netmonitor.css
Hunk #10 FAILED at 1089
1 out of 12 hunks FAILED -- saving rejects to file devtools/client/netmonitor/src/assets/styles/netmonitor.css.rej
patching file devtools/client/netmonitor/src/components/request-list-column-status.js
Hunk #1 FAILED at 5
Hunk #3 FAILED at 50
2 out of 3 hunks FAILED -- saving rejects to file devtools/client/netmonitor/src/components/request-list-column-status.js.rej
patching file devtools/client/netmonitor/src/components/status-bar.js
Hunk #1 FAILED at 17
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/netmonitor/src/components/status-bar.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory

Honza

	Tim Nguyen :ntim
	Comment 31 • 8 years ago
mozreview-review

Comment on attachment 8858854 [details]
Bug 1349173 - Use div table layout to reduce reflow

https://reviewboard.mozilla.org/r/130850/#review134818

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:193
(Diff revision 11)
> +  height: 100%;
>  }
>  
> -.theme-firebug .requests-list-toolbar {
> -  height: 19px !important;
> +.requests-list-table {
> +  display: table;
> +  flex: auto;

The parent (.requests-list-wrapper) doesn't have display: flex; set, so this isn't useful.

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:214
(Diff revision 11)
> -  flex: none;
> -  box-sizing: border-box;
> -  align-items: center;
> -  padding: 3px;
>    cursor: default;
> -}
> +  text-align: center;

I don't think we want this on all columns.

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:234
(Diff revision 11)
> +  height: 24px;
> +  max-height: 24px;
> +  min-height: 24px;
> +  padding: 0;
> +}
> +
> +.theme-firebug .requests-list-headers {
> +  height: 19px;
> +}

This height property is not going to work (since there's min/max-height:24px set above.

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:277
(Diff revision 11)
>  
>  .requests-list-header-button > .button-text {
> -  flex: auto;
> -  white-space: nowrap;
> +  display: inline-block;
> +  text-align: center;
> +  vertical-align: middle;
> +  width: calc(100% - 8px);

Why this magic number ?

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:310
(Diff revision 11)
>  .requests-list-header-button[data-sorted],
> -.requests-list-header[data-active] + .requests-list-header .requests-list-header-button {
> +.requests-list-column[data-active] + .requests-list-column .requests-list-header-button {
>    border-image: linear-gradient(var(--theme-splitter-color), var(--theme-splitter-color)) 1 1;
>  }
>  
> -/* Firebug theme support for Network panel header */
> +.theme-firebug .requests-list-column {

Should be .theme-firebug .requests-list-headers

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:335
(Diff revision 11)
>  :root[platform="linux"].theme-firebug .requests-list-header-button[data-sorted] {
>    background-color: #FAC8AF !important;
>    color: inherit !important;
>  }
>  
> -.theme-firebug .requests-list-header:hover:active {
> +.theme-firebug .requests-list-column:hover:active {

.theme-firebug .requests-list-header-button:hover:active

::: devtools/client/netmonitor/src/components/request-list-header.js:71
(Diff revision 11)
> -          const name = header.name;
> -          const boxName = header.boxName || name;
> -          const label = L10N.getStr(`netmonitor.toolbar.${header.label || name}`);
>  
> +    return (
> +      div({ className: "devtools-toolbar requests-list-headers" },

nit: requests-list-header

::: devtools/client/netmonitor/test/browser_net_timing-division.js:13
(Diff revision 11)
>   */
>  
>  add_task(function* () {
> +  // Hide file and domain columns to make sure timing division can render properly
> +  Services.prefs.setCharPref(
> +    "devtools.netmonitor.hiddenColumns", "[\"file\",\"domain\"]");

Note that this will unhide Protocol and Remote IP which are hidden by default

	Tim Nguyen :ntim
	Comment 33 • 8 years ago
mozreview-review

Comment on attachment 8858854 [details]
Bug 1349173 - Use div table layout to reduce reflow

https://reviewboard.mozilla.org/r/130850/#review134976

::: devtools/client/netmonitor/src/components/status-bar.js:43
(Diff revision 12)
>      getFormattedSize(contentSize), getFormattedSize(transferredSize));
>    let finishText = L10N.getFormatStrWithNumbers("networkMenu.summary.finish",
>      getFormattedTime(millis));
>  
>    return (
> -    div({ className: "devtools-toolbar devtools-toolbar-bottom" },
> +    div({ className: "devtools-toolbar devtools-status-bar" },

If you change this class name, please update:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/toolbars.css#77

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 34 • 8 years ago
mozreview-review-reply

Comment on attachment 8858854 [details]
Bug 1349173 - Use div table layout to reduce reflow

https://reviewboard.mozilla.org/r/130850/#review134818

> I don't think we want this on all columns.

It would be easy to set text-align: center for all columns and then override them if it has different requirement (file and domain columns).

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 36 • 8 years ago

Current patch has put flexible waterfall column back, so shrinking the devtools width will affect waterfall column as well.

But my implementation still can see the (1) issue in comment 26. it's not that easy to find out a solution no matter using width: % or vw, because once hidding too many columns, the remaining columns cannot fill the 100% or 100vw width, so it causes unused space.

@ntim, do you have any good idea of that?


P.S. I saw Chromium devtools is using resizer to control column width. It's a straightforward solution to set a fixed column width through Javascript. I don't think we should introduce a resizer in this bug, it could be a nice to have feature though.

	Tim Nguyen :ntim
	Comment 38 • 8 years ago
mozreview-review

Comment on attachment 8858854 [details]
Bug 1349173 - Use div table layout to reduce reflow

https://reviewboard.mozilla.org/r/130850/#review135200

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:306
(Diff revisions 12 - 14)
>  .requests-list-header-button[data-sorted],
>  .requests-list-column[data-active] + .requests-list-column .requests-list-header-button {
>    border-image: linear-gradient(var(--theme-splitter-color), var(--theme-splitter-color)) 1 1;
>  }
>  
> -.theme-firebug .requests-list-column {
> +.theme-firebug .requests-list-header-button:hover:active {

Sorry about the misunderstanding, this needs to be:

.theme-firebug .requests-list-headers

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:331
(Diff revisions 12 - 14)
>  :root[platform="linux"].theme-firebug .requests-list-header-button[data-sorted] {
>    background-color: #FAC8AF !important;
>    color: inherit !important;
>  }
>  
>  .theme-firebug .requests-list-column:hover:active {

... and this one needs to be:

.theme-firebug .requests-list-header-button:hover:active

	Tim Nguyen :ntim
	Comment 39 • 8 years ago

(In reply to Ricky Chien [:rickychien] from comment #36)
> Current patch has put flexible waterfall column back, so shrinking the
> devtools width will affect waterfall column as well.
> 
> But my implementation still can see the (1) issue in comment 26. it's not
> that easy to find out a solution no matter using width: % or vw, because
> once hidding too many columns, the remaining columns cannot fill the 100% or
> 100vw width, so it causes unused space.
> 
> @ntim, do you have any good idea of that?

At first thought, one thing that might be possible is to make the table cover all columns except the waterfall, and make the waterfall flex side by side with the table, but that reintroduces flex is a less elegant manner.

I'll take a look if there's a more elegant solution with tables.

This is where flexbox/grid win btw :)

> P.S. I saw Chromium devtools is using resizer to control column width. It's
> a straightforward solution to set a fixed column width through Javascript. I
> don't think we should introduce a resizer in this bug, it could be a nice to
> have feature though.

Agreed, is there a bug filed?

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 40 • 8 years ago

Once we decide to use column resizer, we don't have to worry about CSS flex / grid solution for filling available space. Column Resizer will implement through Javascript to calculate the distributed width.

Bug has filed for implementing new column resizer, please see bug 1358414.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 46 • 8 years ago
mozreview-review

Comment on attachment 8858854 [details]
Bug 1349173 - Use div table layout to reduce reflow

https://reviewboard.mozilla.org/r/130850/#review135624

I beleive that this patch is ready to land.

Some comments:
- There is still room for performance (especially the Waterfall column)
- We might want to experiment with Grid
- We might also want to experiment with 'Column' layout (used by Storage panel), which could be more suitable for future Resizeable columns support.


Thanks Ricky!

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 47 • 8 years ago
mozreview-review

Comment on attachment 8860787 [details]
Bug 1349173 - Fix mochitest failures

https://reviewboard.mozilla.org/r/132740/#review135626

Works for me

Honza

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 48 • 8 years ago

Try is green. Thanks!

	Pulsebot
	Comment 49 • 8 years ago

Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a62f82eb4ff9
Use div table layout to reduce reflow r=Honza
https://hg.mozilla.org/integration/autoland/rev/e94b7bcae282
Fix mochitest failures r=Honza

	Carsten Book [:Tomcat]
	Comment 50 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a62f82eb4ff9
https://hg.mozilla.org/mozilla-central/rev/e94b7bcae282

	Ryan VanderMeulen [:RyanVM]
	Comment 51 • 8 years ago

Did we want this to ride the 55 train or was it something we should consider for uplift to Beta?

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 52 • 8 years ago

Per our discussion yesterday, we decided to focus on the 55 train and fix as many as perf regressions in 55 as we can. Let's ride the 55 train. Thanks!

	Johann Hofmann [:johannh]
	Comment 53 • 7 years ago

In addition to bug 1360458, this caused a slight visual regression in the header row text alignment. Can be seen here:

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=e17cbb839dd225a2da7e5d5bec43cf94e11749d8&newProject=mozilla-central&newRev=62b649c6b314f756f21cb95f2b0d491e2664e944

I could be the same problem as bug 1360457. Do you want me to open a new bug or is that covered by bug 1360457?

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 54 • 7 years ago

Yep, it's the same problem as bug 1360457 so it's not necessary to file a new bug. thanks

	Ciprian Georgiu, Desktop QA Reporter
	Comment 55 • 7 years ago

Verified fixed on 55.0b11 across platforms: Windows 10 x64, Mac OS X 10.11.5 and Ubuntu 16.04 x64.