Closed Bug 1349173 Opened 8 years ago Closed 8 years ago

[Performance] There is a noticeable lag while changing the netmonitor panel height

Categories

(DevTools :: Netmonitor, defect, P1)

Product:
DevTools
Component:
Netmonitor
Version:
54 Branch
Type:
defect

Tracking

(firefox52 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54 wontfix, firefox55 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Iteration:
55.4 - May 1
Tracking Flags:
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- wontfix
firefox55 --- verified

People

(Reporter: cgeorgiu, Assigned: rickychien)

References

(Depends on 1 open bug)

Details

(Keywords: regression, Whiteboard: [netmonitor])

Attachments

(4 files)

lag - net monitor height.gif
2.56 MB, image/gif
Details
Bug 1349173 - Use div table layout to reduce reflow
59 bytes, text/x-review-board-request
Honza
: review+
Details
hidden-columns.png
68.01 KB, image/png
Details
Bug 1349173 - Fix mochitest failures
59 bytes, text/x-review-board-request
Honza
: review+
Details
[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)
Has Regression Range: --- → yes
Has STR: --- → yes
Blocks: netmonitor-html
Flags: qe-verify+
Priority: -- → P3
Whiteboard: [netmonitor-reserve]
Version: Trunk → 54 Branch
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)
Priority: P3 → P1
No longer blocks: netmonitor-html
Priority: P1 → P2
Whiteboard: [netmonitor-reserve] → [netmonitor]
Summary: There is a noticeable lag while changing the netmonitor panel height → [Performance] There is a noticeable lag while changing the netmonitor panel height
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Iteration: --- → 55.3 - Apr 17
Priority: P2 → P1
Assignee: gasolin → rchien
Depends on: 1356126
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.
Attachment #8858854 - Flags: review?(odvarko) → review-
Blocks: 1350969
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
Attachment #8858854 - Flags: review?(gasolin)
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
Attachment #8858854 - Flags: review?(gasolin)
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.
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.
Iteration: 55.3 - Apr 17 → 55.4 - May 1
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
Attachment #8858854 - Flags: review?(odvarko) → 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.
Attachment #8858854 - Flags: review?(gasolin)
Comment on attachment 8858854 [details] Bug 1349173 - Use div table layout to reduce reflow set f? ntim for style feedback
Attachment #8858854 - Flags: feedback?(ntim.bugs)
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
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.
Has anyone experimented with CSS grid? Might be better solution than table layouts.
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).
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.
Attached image hidden-columns.png —
(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
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?
Attachment #8858854 - Flags: review?(odvarko) → 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
Attachment #8858854 - Flags: review?(odvarko) → review-
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
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
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
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).
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.
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
(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?
Blocks: 1358414
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.
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
Attachment #8858854 - Flags: review?(odvarko) → review+
Comment on attachment 8860787 [details] Bug 1349173 - Fix mochitest failures https://reviewboard.mozilla.org/r/132740/#review135626 Works for me Honza
Attachment #8860787 - Flags: review?(odvarko) → review+
Try is green. Thanks!
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
https://hg.mozilla.org/mozilla-central/rev/a62f82eb4ff9 https://hg.mozilla.org/mozilla-central/rev/e94b7bcae282
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Did we want this to ride the 55 train or was it something we should consider for uplift to Beta?
status-firefox-esr52: --- → unaffected
Flags: needinfo?(rchien)
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!
Flags: needinfo?(rchien)
Depends on: 1360457
Depends on: 1360458
Depends on: 1360459
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?
Flags: needinfo?(rchien)
Yep, it's the same problem as bug 1360457 so it's not necessary to file a new bug. thanks
Flags: needinfo?(rchien)
Depends on: 1376267
Verified fixed on 55.0b11 across platforms: Windows 10 x64, Mac OS X 10.11.5 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: qe-verify+
Depends on: 1413829
Depends on: 1413941
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: