Closed Bug 1309194 Opened 8 years ago Closed 8 years ago

Implement summary info in Net Panel Toolbar

Categories

(DevTools :: Netmonitor, defect, P1)

Product:
DevTools
Component:
Netmonitor
Type:
defect

Tracking

(firefox53 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 53
Iteration:
53.1 - Nov 28
Tracking Flags:
Tracking Status
firefox53 --- verified

People

(Reporter: steveck, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(2 files, 1 obsolete file)

Bug 1309194 - Implement summary info in Net Panel Toolbar
58 bytes, text/x-review-board-request
jsnajdr
: review+
Honza
: review+
ntim
: feedback+
Details
summary.png
95.59 KB, image/png
Details 
A subitem for Bug #1308426 that need to implement summary info(including the button for opening Analysis perspective) in Net Panel Toolbar
Whiteboard: [netmonitor]
Flags: qe-verify+
QA Contact: ciprian.georgiu
Priority: -- → P2
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Iteration: --- → 52.3 - Nov 7
Priority: P2 → P1
This is the WIP for summary text component. Since I'll OOO next week, feel free to continue the rest work on it. Or I can pick it up a week later.
Flags: needinfo?(rchien)
Don't worry! I'll continue working on this and finish entire toolbar refactoring:)
Assignee: gasolin → rchien
Flags: needinfo?(rchien)
There are something changed in this patch apart from converting summary button into react. 

- Moved all UI related actions and reducers into actions/ui.js and reducers/ui.js
- Remove sidebar.js since all UI related logics should be put in ui.js.
- Put summary into a separate Record for management all summary button state, I think we could file a follow up bug to do the same thing for the toggle button state.
- Replace list-style-image: url(images/profiler-stopwatch.svg); with background-image since it causes css broken.
- Fixed rtl issue for background-image.
Comment on attachment 8808488 [details]
Bug 1309194 - Implement summary info in Net Panel Toolbar

https://reviewboard.mozilla.org/r/91328/#review91172

::: devtools/client/netmonitor/requests-menu-view.js:132
(Diff revision 3)
>      this.contextMenu = new RequestListContextMenu();
>  
>      let widgetParentEl = $("#requests-menu-contents");
>      this.widget = new SideMenuWidget(widgetParentEl);
>      this._splitter = $("#network-inspector-view-splitter");
> -    this._summary = $("#requests-menu-network-summary-button");
> +    store.dispatch(Actions.updateSummary(L10N.getStr("networkMenu.empty")));

This "set-initial-value" operation is better handled by setting an initial state in the reducer. Firing an "init" action like this is not necessary.

::: devtools/client/netmonitor/requests-menu-view.js:561
(Diff revision 3)
> +    const summaryText = this.store.getState().ui.summary.text;
>      let visibleItems = this.visibleItems;
>      let visibleRequestsCount = visibleItems.length;
>      if (!visibleRequestsCount) {
> -      this._summary.setAttribute("label", L10N.getStr("networkMenu.empty"));
> +      if (summaryText !== L10N.getStr("networkMenu.empty")) {
> +        this.store.dispatch(Actions.updateSummary(
> +        L10N.getStr("networkMenu.empty")));
> +      }
>        return;
>      }

This kind of logic - check the current state and update it if necessary - should be in the reducer. Here, you should just dispatch action that says "list is empty" and not worry about anything else.

::: devtools/client/netmonitor/requests-menu-view.js:578
(Diff revision 3)
> -    let str = PluralForm.get(visibleRequestsCount,
> -      L10N.getStr("networkMenu.summary"));
> +    const summary = PluralForm.get(visibleRequestsCount,
> +      L10N.getStr("networkMenu.summary"))
> -
> -    this._summary.setAttribute("label", str
>        .replace("#1", visibleRequestsCount)
>        .replace("#2", L10N.numberWithDecimals((totalBytes || 0) / 1024,
>          CONTENT_SIZE_DECIMALS))
>        .replace("#3", L10N.numberWithDecimals((totalMillis || 0) / 1000,
> -        REQUEST_TIME_DECIMALS))
> -    );
> +        REQUEST_TIME_DECIMALS));
> +
> +    if (summaryText !== summary) {
> +      this.store.dispatch(Actions.updateSummary(summary));
> +    }

Things like formatting a displayed text and doing l10n should be in the view, i.e., in the React component.

The Redux store should contain data. I.e., object like this:

summary: {
  count,
  bytes,
  millis
}

::: devtools/client/themes/netmonitor.css:757
(Diff revision 3)
> +#requests-menu-network-summary-button::before {
> +  background-image: url(images/profiler-stopwatch.svg);
> +  filter: var(--icon-filter);
> +}
> +
> +#requests-menu-network-summary-button:-moz-locale-dir(ltr)::before {
> +  left: -1em;
> +}
> +
> +#requests-menu-network-summary-button:-moz-locale-dir(rtl)::before {
> +  right: -1.6em;
>  }

Maybe you want to just use "devtools-button" class for this button? It should define all the styling you need.

The "left" and "right" properties look like a pile of random numbers. Why is background image offset specified in "em" units, which depend on font size? Why is it 1.0 and 1.6? Why not 0.95 and 1.7?
Comment on attachment 8808488 [details]
Bug 1309194 - Implement summary info in Net Panel Toolbar

https://reviewboard.mozilla.org/r/91328/#review91184

::: devtools/client/netmonitor/components/summary-button.js:23
(Diff revision 3)
> +    onClick: () => {
> +      NetMonitorView.toggleFrontendMode();
> +    }

nit: Move the handler implementation to mapDispatchToProps. It will be soon dispatching a real action, instead of calling something on NetMonitorView directly.
Comment on attachment 8808488 [details]
Bug 1309194 - Implement summary info in Net Panel Toolbar

https://reviewboard.mozilla.org/r/91328/#review91172

> This kind of logic - check the current state and update it if necessary - should be in the reducer. Here, you should just dispatch action that says "list is empty" and not worry about anything else.

Ideally, we don't need to check current value and previous value in reducer every time and it also doesn't work in this case. Here is a kind of workaround to avoid recursive calls because dispatch action causes refreshSummary() invoke again. And also onChange() calling in storeWatcher never end so that we are not able to update currentValue = newValue. That case is complicated to describe, but you can comment my code to see what happens exactly.

> Maybe you want to just use "devtools-button" class for this button? It should define all the styling you need.
> 
> The "left" and "right" properties look like a pile of random numbers. Why is background image offset specified in "em" units, which depend on font size? Why is it 1.0 and 1.6? Why not 0.95 and 1.7?

I've already set "devtools-button" class in summary-button.js, and indeed I don't have better solution for setting background image offset. Just replace em with px and eyes check it renders properly.
(In reply to Ricky Chien [:rickychien] from comment #10)
> Ideally, we don't need to check current value and previous value in reducer
> every time and it also doesn't work in this case. Here is a kind of
> workaround to avoid recursive calls because dispatch action causes
> refreshSummary() invoke again. And also onChange() calling in storeWatcher
> never end so that we are not able to update currentValue = newValue. That
> case is complicated to describe, but you can comment my code to see what
> happens exactly.

Yes, I've seen this infinite recursion, too. It's a bug in the storeWatcher function. If you want to fix it, look at my RequestList patch:

https://reviewboard.mozilla.org/r/90444/diff/2#26

And see how I changed the storeWatcher function. First, it updates the currentValue, and only then calls the onChange callback.
(In reply to Ricky Chien [:rickychien] from comment #10)
> I've already set "devtools-button" class in summary-button.js, and indeed I
> don't have better solution for setting background image offset. Just replace
> em with px and eyes check it renders properly.

There are few problems with this solution, where the performance icon is placed outside the button element:
- when clicking the icon, nothing happens. It should switch to the performance view
- after resizing the window so that the width is small enough, the perf icon overlaps with the UI that's to the left side, i.e., the filter buttons

Can we convert this into a proper markup, where the perf icon is a real element with width, height, and background and that's placed inline inside the button, together with the text? That would feel much less hacky.
Comment on attachment 8808488 [details]
Bug 1309194 - Implement summary info in Net Panel Toolbar

https://reviewboard.mozilla.org/r/91328/#review91470

::: devtools/client/netmonitor/components/summary-button.js:16
(Diff revision 3)
> +  const title = (text === L10N.getStr("networkMenu.empty")) ?
> +      L10N.getStr("netmonitor.toolbar.perf") : text;

Style nit: align both lines

::: devtools/client/themes/netmonitor.css:759
(Diff revision 3)
> +  font-weight: 600;
> +}
> +
> +#requests-menu-network-summary-button::before {
> +  background-image: url(images/profiler-stopwatch.svg);
> +  filter: var(--icon-filter);

You don't need to apply the filter, it's already here:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/toolbars.css#192

::: devtools/client/themes/netmonitor.css:763
(Diff revision 3)
> +  background-image: url(images/profiler-stopwatch.svg);
> +  filter: var(--icon-filter);
> +}
> +
> +#requests-menu-network-summary-button:-moz-locale-dir(ltr)::before {
> +  left: -1em;

You should be able to change https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/common.css#274 to stop using absolute positioning.
Comment on attachment 8808488 [details]
Bug 1309194 - Implement summary info in Net Panel Toolbar

https://reviewboard.mozilla.org/r/91328/#review91512

There is already good review comments from Jarda and Ntim so here are main:

- There should always be an empty line between "use strict"; and the Mozilla license header
- The perf icon doesn't have proper cursor. I guess that  this should be fixed as soon as the icon is moved into the button.
- The file name for actions/reducer: ui.js is too generic. React is all about ui. I would rather keep sidebar.js and introduce summary.js (or summary-button.js, etc.) Both these UI parts can be nicely separated and independent.
- Could you also replace the "requests-menu-spacer" by html:div?

Honza
Attachment #8808488 - Flags: review?(odvarko)
(In reply to Jan Honza Odvarko [:Honza] from comment #14)
> There is already good review comments from Jarda and Ntim so here are main:
> 
> - There should always be an empty line between "use strict"; and the Mozilla
> license header

Good catch! Let's file a separate bug for doing this.

> - The perf icon doesn't have proper cursor. I guess that  this should be
> fixed as soon as the icon is moved into the button.

Yes, Jarda mentioned this as well. Put icon and text into button to keep the behavior as usual.

> - The file name for actions/reducer: ui.js is too generic. React is all
> about ui. I would rather keep sidebar.js and introduce summary.js (or
> summary-button.js, etc.) Both these UI parts can be nicely separated and
> independent.

I still prefer to have an UI state because in my experience UI relevant states are pretty simple. Most of the cases are to set show/hide state for the UI component. If we are doing something like update the UI number, text, icon and so on, those states should not be UI related and is supposed to be the data state which will trigger follow-up UI updates once data state is changed. 

(see how my patch update summary text. I removed the state for storing summary but introduced requests to handle all request items. Every view components who interests in the state of request items should access and calculate the data by themselves through mapStateToProps)

> - Could you also replace the "requests-menu-spacer" by html:div?

There is another layout issue needs to be addressed for converting spacer to html:div and that would introduce additional irrelevant code in this bug. I'd like to fix it in bug 1308426 :)
Comment on attachment 8808488 [details]
Bug 1309194 - Implement summary info in Net Panel Toolbar

https://reviewboard.mozilla.org/r/91328/#review91534

(In reply to Ricky Chien [:rickychien] from comment #16)
> > - The file name for actions/reducer: ui.js is too generic. React is all
> > about ui. I would rather keep sidebar.js and introduce summary.js (or
> > summary-button.js, etc.) Both these UI parts can be nicely separated and
> > independent.
> 
> I still prefer to have an UI state because in my experience UI relevant
> states are pretty simple.

Yes, they are simple, but I am still not convinced that we want to have all
the ui/presentation states (even non related) in one reducer as a pile of 
things.

I quite liked how the sidebar.js reducer was implemented.
We had all sidebar related state fields nicely located at one place,
separated from other state fields.
(see also another inline comment related to the 'visible' field).

Anyway, I might be wrong. Jarda, what do you think?

> Most of the cases are to set show/hide state for
> the UI component. If we are doing something like update the UI number, text,
> icon and so on, those states should not be UI related and is supposed to be
> the data state which will trigger follow-up UI updates once data state is
> changed.

Yes, e.g. if the list of requests changes, the summary text changes as well
automatically.


> There is another layout issue needs to be addressed for converting spacer to
> html:div and that would introduce additional irrelevant code in this bug.
> I'd like to fix it in bug 1308426 :)

Cool, let's do in as part of that bug.

::: devtools/client/netmonitor/components/summary-button.js:25
(Diff revision 4)
> +  const text = (itemCount === 0) ? L10N.getStr("networkMenu.empty") :
> +    // https://developer.mozilla.org/en-US/docs/Localization_and_Plurals
> +    PluralForm.get(itemCount, L10N.getStr("networkMenu.summary"))
> +    .replace("#1", itemCount)
> +    .replace("#2", L10N.numberWithDecimals((
> +      SummaryButton.getTotalBytesOfRequests(items) || 0) / 1024,
> +      CONTENT_SIZE_DECIMALS))
> +    .replace("#3", L10N.numberWithDecimals((
> +      SummaryButton.getTotalMillisOfRequests(items) || 0) / 1000,
> +      REQUEST_TIME_DECIMALS));

Could we implement this calculation as a memoized selector? Such selector can be nicely utilized in a test too.

::: devtools/client/netmonitor/reducers/requests.js:13
(Diff revision 4)
> +const {
> +  UPDATE_REQUESTS,
> +} = require("../constants");
> +
> +const Requests = I.Record({
> +  items: [],

If I understand correctly, the list of requests will be also utilized in bug 1309866, correct?

::: devtools/client/netmonitor/reducers/ui.js:16
(Diff revision 4)
> +  TOGGLE_SIDEBAR,
> +} = require("../constants");
> +const { L10N } = require("../l10n");
> +
> +const Summary = I.Record({
> +  text: L10N.getStr("networkMenu.empty"),

I am not sure if I understand this state field. Isn't the entire text for the summary label just calculated from the list of request?

::: devtools/client/netmonitor/reducers/ui.js:21
(Diff revision 4)
> +  text: L10N.getStr("networkMenu.empty"),
> +});
> +
> +const UI = I.Record({
> +  toggleButtonDisabled: true,
> +  visible: false,

Now, when the reducer is called 'ui' it isn't clear what the 'visible' flag means.

If you want to keep all presentation data in this reducer we should change the names too. 

So, this could be e.g.: `sidebarVisible`

::: devtools/client/themes/netmonitor.css:767
(Diff revision 4)
> +  height: 16px;
> +}
> +
> +.summary-info-text {
> +  margin-inline-start: 0.5em;
> +  font-weight: 600;

Why the text is bold? (it wasn't before)
Attachment #8808488 - Flags: review?(odvarko)
Jarda, please see my question in comment #18
Flags: needinfo?(jsnajdr)
(In reply to Jan Honza Odvarko [:Honza] from comment #18)
> > I still prefer to have an UI state because in my experience UI relevant
> > states are pretty simple.
> 
> Yes, they are simple, but I am still not convinced that we want to have all
> the ui/presentation states (even non related) in one reducer as a pile of 
> things.
> 
> I quite liked how the sidebar.js reducer was implemented.
> We had all sidebar related state fields nicely located at one place,
> separated from other state fields.
> (see also another inline comment related to the 'visible' field).
> 
> Anyway, I might be wrong. Jarda, what do you think?

It's a common Redux pattern to make a distinction between "data" and "ui" in the state. "data" contain the model data for the app, and "ui" contains ephemeral information like which part of the UI is visible/hidden/expanded/collapsed etc.

So, the "ui" part of the state makes sense. Also, most of the actions and reducers are trivial, just setting some boolean field boolean field. Having X separate files for each of them sounds like an overkill.

For our situation, I think most of the UI state is a temporary scaffolding that will disappear soon. After we land the Redux request list, most of the UI information can be computed from the "data" part of the state.

For example, I already removed the "disabledToggleButton" from the code in bug 1309866, because its value is equivalent to "state.requests.size == 0".

Similarly, there won't be a need for a "summary" field in the state, because that information will be computed from the "requests" data by a memoized selector.

So, my inclination is to use a "ui", and not to worry about it much now - most of it is temporary anyway. I can't predict what the React version of the sidebar will bring us, so let's make a final decision after we have the sidebar.

> Could we implement this calculation as a memoized selector? Such selector
> can be nicely utilized in a test too.

I agree - the SummaryButton should only render the HTML markup, and consume input in this shape:

summary: {
  requestCount,
  totalBytes,
  totalMillis
}

A separate selector-like function should consume "items" as input and produce "summary" as output. This would be forward-compatible with bug 1309866.

> > +const Requests = I.Record({
> > +  items: [],
> 
> If I understand correctly, the list of requests will be also utilized in bug
> 1309866, correct?

Yes, but please don't worry about getting it 100% correct now - as long as the code has reasonable structure (state holds the data, selector does the computation, React component does the rendering), it can be easily changed in bug 1309866.

> > +.summary-info-text {
> > +  margin-inline-start: 0.5em;
> > +  font-weight: 600;
> 
> Why the text is bold? (it wasn't before)

It was, the font-weight property was in another rule.

Anyway, I think it would look better if it wasn't bold, but that's another story. Eventually, Helen should help us modernize the Netmonitor UI, so that it looks nice and clean like debugger.html, for example.
Flags: needinfo?(jsnajdr)
Comment on attachment 8808488 [details]
Bug 1309194 - Implement summary info in Net Panel Toolbar

https://reviewboard.mozilla.org/r/91328/#review91534

I'd like to have our application state like this:

{
  ui: {
    sidebar: {
      open: false,
    },
    statistics: {
      open: false,
    },
    ...
  },
  requests: [{}, {}, {}...]
  filters: {
    types: {...}
    url: ""  
  },
  
}

What do you think?

> If I understand correctly, the list of requests will be also utilized in bug 1309866, correct?

Correct! Having this primary array of request items in the store, view components (RequestMenuView, SummaryButton, Statistics and Waterfall) are able to compute derived data from the list of requests and render themselves.

I'm ok with landing approach for rebasing in bug 1309866 once this bug is landed or filing a separate bug to utilize the list of requests in RequefstMenuView.

> I am not sure if I understand this state field. Isn't the entire text for the summary label just calculated from the list of request?

Ah! It was my mistake that I forgot to remove legacy summary code.

> Why the text is bold? (it wasn't before)

It was before. I copied these css rules (both text and icon) from xul elements and keep it as is.

https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/common.css#404-407
Comment on attachment 8808488 [details]
Bug 1309194 - Implement summary info in Net Panel Toolbar

https://reviewboard.mozilla.org/r/91328/#review91568

This looks good and works very well for me when I'm testing the code.

Please restructure the SummaryButton code so that it looks like this:

// React component that does just the rendering, no computation
function SummaryButton(props) {
  let { count, totalBytes, totalMillis } = props.summary;
  return div({}, `${count} requests, ${bytes} bytes, ${millis} ms`);
}

// Selector to compute the summary from items
function getSummary(items) {
  return {
    count: items.length,
    totalBytes: getTotalBytes...,
    totalMillis: getTotalMillis...,
  }
}

exports.getSummary = getSummary; // tests will need this

// Connect it all together
module.exports = connect(
  state => ({
    summary: getSummary(state.requests.items)
  })
)(SummaryButton)

Also, please clean up the unused "summary" fields in UI state etc.
Attachment #8808488 - Flags: review?(jsnajdr)
Attached image summary.png — 
(In reply to Jarda Snajdr [:jsnajdr] from comment #20)
> (In reply to Jan Honza Odvarko [:Honza] from comment #18)
> So, my inclination is to use a "ui", and not to worry about it much now -
> most of it is temporary anyway. I can't predict what the React version of
> the sidebar will bring us, so let's make a final decision after we have the
> sidebar.
Okay, agreed.

> > Why the text is bold? (it wasn't before)
> 
> It was, the font-weight property was in another rule.
Weird, it wasn't bold on my machine. 

I am attaching a screenshot showing how it looks like when the patch is applied (bold), is not applied (not bold) and how it looks like in the Nightly build (not bold).

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #23)
> Weird, it wasn't bold on my machine. 
> 
> I am attaching a screenshot showing how it looks like when the patch is
> applied (bold), is not applied (not bold) and how it looks like in the
> Nightly build (not bold).

This looks like the font-size is wrong - the font is significantly bigger with the patch.

Also, there is probably a difference between platforms. Devtools use the system font, so the font on Mac is different from the font on Windows. It seems like the behavior of font-weight: 600 is different across platform. Somewhere the 600 variant is available, somewhere it isn't.
(In reply to Jan Honza Odvarko [:Honza] from comment #23)
> Created attachment 8809014 [details]
> summary.png

HTML buttons and selects don't use the system font on Windows. To get the system font, you should add: 

button, select {
  font: inherit;
}

after :root[platform="linux"] .devtools-monospace in devtools/client/themes/common.css
Comment on attachment 8808488 [details]
Bug 1309194 - Implement summary info in Net Panel Toolbar

https://reviewboard.mozilla.org/r/91328/#review91622

::: devtools/client/netmonitor/components/toggle-button.js:65
(Diff revisions 5 - 6)
> +  (stateProps, dispatchProps, ownProps) => {
> +    return Object.assign({}, ownProps, stateProps, {
> +      triggerSidebar: () => dispatchProps.triggerSidebar(!stateProps.open),
> +    });
> +  }

Why does the toggling of the sidebar need to be so complicated? Why not just dispatch a "toggle" action and let the reducer do the rest?

It's so natural and straightforward to do this in the reducer: it's been designed to do the job of taking the current state and the action as an input, do the work, and produce a new state as the output.

Why is this job outsourced to such an unlikely place as the mergeProps function of a connect call?
I just noticed that we used onMouseDown instead of onClick in ToggleButton but after replacing it with onClick that caused so many test timeout failures. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=14c63eaab34bdb8c534b062bb51f6c44ff30a745

I don't have idea why it leads to intermittent becoming permanent. Reverted the changes to solve that problem.
It is still weird to see these timeout test failures on try. I rebased to main trunk and push to try again.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c6afbfc4f5743424bf7ba8645dfe77651bc3ed9
Comment on attachment 8808488 [details]
Bug 1309194 - Implement summary info in Net Panel Toolbar

https://reviewboard.mozilla.org/r/91328/#review91860

::: devtools/client/themes/common.css:33
(Diff revision 10)
>  .devtools-monospace {
>    font-family: var(--monospace-font-family);
>  }
>  
>  :root[platform="linux"] .devtools-monospace {
> -  font-size: 80%;
> +  font-size: inherit;

That's not right, the monospace font size on Linux is intentionally set to 80%.

If you want to fix the Windows issue, add an extra rule after this one:

  :root[platform="linux"] .devtools-monospace {
    font-size: 80%;
  }
+ button, select {
+   font: inherit;
+ }
button, select {
  font: inherit;
}

That will impact on macOS button and it also looks weird.
Honza, I still saw the bold font (font-weight: 600) on Windows Nightly 52.0a1 (2016-11-09). (It's bold on Linux as well)

bold font is defined at [1] which is generic for all platforms, so I don't think that will different on Windows.

https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/common.css#406
(In reply to Ricky Chien [:rickychien] from comment #35)
> button, select {
>   font: inherit;
> }
> 
> That will impact on macOS button and it also looks weird.

how about:

button, select {
  font: message-box;
}
Flags: needinfo?(rchien)
Tim, thanks for your suggestion!

According discussion on IRC with Honza, it seems that the default system fonts are different in XUL and HTML. We have agreed with using HTML default font after migration and removing font-weight: 600 as well.
Flags: needinfo?(rchien)
Comment 37 is solution for font issue, please ignore comment 38 which was wrong. Patch has been updated.
Comment on attachment 8808488 [details]
Bug 1309194 - Implement summary info in Net Panel Toolbar

https://reviewboard.mozilla.org/r/91328/#review91916

::: devtools/client/themes/common.css:36
(Diff revisions 9 - 11)
>  
>  :root[platform="linux"] .devtools-monospace {
> -  font-size: inherit;
> +  font-size: 80%;
>  }
>  
> +button, select {

This fixes the problem for me but, shouldn't we use less general CSS selector for this?

In any case, there should be a comment explainig why it's here.
Attachment #8808488 - Flags: review?(odvarko)
I think it should apply to all devtools and override user agent rule at forms.css [1], so I'm in favor of using general selector.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/style/res/forms.css#651
Please see question/comment #41

Honza
Flags: needinfo?(ntim.bugs)
(In reply to Jan Honza Odvarko [:Honza] from comment #43)
> Please see question/comment #41
> 
> Honza

The global selector is fine. A comment would be nice though:
/* Override wrong system font from forms.css on Windows */
Flags: needinfo?(ntim.bugs)
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #44)
> (In reply to Jan Honza Odvarko [:Honza] from comment #43)
> > Please see question/comment #41
> > 
> > Honza
> 
> The global selector is fine. A comment would be nice though:
> /* Override wrong system font from forms.css on Windows */

Shouldn't we at least specify the platform in the selector?

:root[platform="win"] button, select { ... }


Honza
Flags: needinfo?(ntim.bugs)
(In reply to Jan Honza Odvarko [:Honza] from comment #45)
> (In reply to Tim Nguyen :ntim (use needinfo?) from comment #44)
> > (In reply to Jan Honza Odvarko [:Honza] from comment #43)
> > > Please see question/comment #41
> > > 
> > > Honza
> > 
> > The global selector is fine. A comment would be nice though:
> > /* Override wrong system font from forms.css on Windows */
> 
> Shouldn't we at least specify the platform in the selector?
> 
> :root[platform="win"] button, select { ... }
> 
> 
> Honza

I guess you meant :root[platform="win"] button, :root[platform="win"] select { ... }

To be honest, I'm fine either way.
Flags: needinfo?(ntim.bugs)
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #46)
> I guess you meant :root[platform="win"] button, :root[platform="win"] select
> { ... }
Yes

> To be honest, I'm fine either way.
Alright, if Tim says ok then we are safe :-)

Ricky, I'll let you decide!

Honza
I prefer to use global button, select. Font has been overridden on all platforms due to forms.css but it's lucky we don't see any impact on Mac and Linux. It could avoid the font being changed in forms.css.
(In reply to Ricky Chien [:rickychien] from comment #48)
> I prefer to use global button, select. Font has been overridden on all
> platforms due to forms.css but it's lucky we don't see any impact on Mac and
> Linux. It could avoid the font being changed in forms.css.

Ok, for me. I can't r+ atm, I thin you need to set the review flag again for me.

Honza
Comment on attachment 8808488 [details]
Bug 1309194 - Implement summary info in Net Panel Toolbar

https://reviewboard.mozilla.org/r/91328/#review92576

r+ from me with several reservations:

** CSS **
I don't like the global "button { font: message-box }" CSS rule. On Windows, it doesn't affect only the summary view, but also all places where the is a "large" button with text:
- Netmonitor: "Reload" button in empty view
- Memory: "Take Snapshot" button
- Performance: "Start Recording Performance" button

These buttons' text is now in smaller font:
* before: "-moz-button", i.e., MS Shell Dlg 13.3px
* after: "message-box", i.e., Segoe UI 12px

I didn't check the situation on Linux. All these rules for system fonts don't make any sense to me. I think they are ripe for a complete cleanup. My limited typographic education tells me that the spectrum of used font faces and font sizes should be kept to minimum.

Other CSS nits:
- the "performance" icon in the summary info should change opacity on mouseover. There are rules for ".devtools-button::before" that do this, but they don't apply in this case.
- the colors of the performance icon and the summary text should be the same. They aren't in the dark theme - white vs lightblue

I'm OK with solving these CSS problems as followups. I expect to do a lot of other CSS improvements as part of the netmonitor.html project.

** Test failures on try **
I ran my own try run here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=89fbfd4ec0cbe869fb6bfe38c51331c004554228&selectedJob=30951510

and there is only one failure that looks dangerous:
TEST-UNEXPECTED-FAIL | devtools/client/inspector/test/browser_inspector_search-05.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort. -
...
TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_footer-summary.js | leaked 2 window(s) until shutdown [url = about:devtools-toolbox]

What I think is happening:
The browser_net_footer-summary.js test reports these leaks on every debug run (I saw them happen on my local machine), but they are treated as "warnings" and don't make the test suite fail.

It's the browser_inspector_search-05.js that really fails (and is not related to this patch), and the netmonitor leak is only reported here because the Treeherder greps the test logs for TEST-UNEXPECTED-FAIL, which makes it rise to the surface.
Attachment #8808488 - Flags: review?(jsnajdr) → review+
(In reply to Jarda Snajdr [:jsnajdr] from comment #50)
> Comment on attachment 8808488 [details]
> Bug 1309194 - Implement summary info in Net Panel Toolbar
> 
> https://reviewboard.mozilla.org/r/91328/#review92576
> 
> r+ from me with several reservations:
> 
> ** CSS **
> I don't like the global "button { font: message-box }" CSS rule. On Windows,
> it doesn't affect only the summary view, but also all places where the is a
> "large" button with text:
> - Netmonitor: "Reload" button in empty view
> - Memory: "Take Snapshot" button
> - Performance: "Start Recording Performance" button
> 
> These buttons' text is now in smaller font:
> * before: "-moz-button", i.e., MS Shell Dlg 13.3px
> * after: "message-box", i.e., Segoe UI 12px
On my Windows VM, it's Segoe UI 13px (at least that's the expected result).

> I didn't check the situation on Linux. All these rules for system fonts
> don't make any sense to me. I think they are ripe for a complete cleanup. My
> limited typographic education tells me that the spectrum of used font faces
> and font sizes should be kept to minimum.
I intentionally targeted all buttons when I suggested the rule. The use of MS Shell Dlg is very wrong on windows, especially when all the surrounding UI uses Segoe UI.

I agree they should be cleaned up, but this is the cleanest solution I could find, as we can't touch forms.css as it is.
I found out html namespace selector could be a nice temporary solution for addressing css issue for html:button during the transition. I've been updated my patch and it got rid of inconsistent button font in devtools.
Blocks: 1317205
Comment on attachment 8808488 [details]
Bug 1309194 - Implement summary info in Net Panel Toolbar

It's really lucky that test failures has gone away after rebasing to the latest trunk.

See following two try results:

try: -b o -p linux,linux64,macosx64,win32,win64 -u mochitests -t none
https://treeherder.mozilla.org/#/jobs?repo=try&revision=90af582b2e13b0685bf248a9ec7d127cb2a8b44b

Stole the try syntax from Jarda's comment 50 and try again.
try: -b do -p linux64,macosx64,win32 -u mochitest-dt,mochitest-e10s-devtools-chrome -t none
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d7bb71c79512462bf672906042d5ee4e49c526a

Both results seems to look good to me :) I'm glad to land it after r+ from Honza.

ntim, could you take a look at the last piece of CSS changes? I hope the namespace selector is able to satisfy our requirements.
Attachment #8808488 - Flags: feedback?(ntim.bugs)
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #51)
> > These buttons' text is now in smaller font:
> > * before: "-moz-button", i.e., MS Shell Dlg 13.3px
> > * after: "message-box", i.e., Segoe UI 12px
> On my Windows VM, it's Segoe UI 13px (at least that's the expected result).

I tested on a Windows 7 VMware image I have. There seem to be differences between Windows versions.

I also see bigger font sized on the filter buttons that are already converted to HTML. We should make sure that the CSS fixes make it to the Aurora, even if this patch is landed after the merge day.
Comment on attachment 8808488 [details]
Bug 1309194 - Implement summary info in Net Panel Toolbar

https://reviewboard.mozilla.org/r/91328/#review92672
Attachment #8808488 - Flags: review?(odvarko) → review+
Iteration: 52.3 - Nov 14 → 53.1 - Nov 28
Attachment #8808488 - Flags: feedback?(ntim.bugs) → feedback+
The CSS changes look fine to me.
Note that I haven't tested the patch for visual regressions, but I don't want to block this from landing, and I'm fine with dealing with those in a follow up (assuming we land the patch in 53)
Blocks: 1316484
Pushed by jsnajdr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/014fd4d5443d
Implement summary info in Net Panel Toolbar r=Honza,jsnajdr
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/014fd4d5443d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
This issue is verified fixed on latest Nightly 53.0a1 (2016-11-21) across platforms:
- Windows 10 x64
- Mac OS X 10.11.5
- Ubuntu 16.04 x64 LTS

I will mark this accordingly, due the fact that I didn't encountered any issues in Net Panel Toolbar, after this refactoring task.
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
Flags: qe-verify+
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: