Closed Bug 1309191 Opened 3 years ago Closed 3 years ago

Implement filter buttons for Net Panel Toolbar

Categories

(DevTools :: Netmonitor, defect, P1)

defect

Tracking

(firefox52 verified)

VERIFIED FIXED
Firefox 52
Iteration:
52.3 - Nov 14
Tracking Status
firefox52 --- verified

People

(Reporter: steveck, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(2 files, 2 obsolete files)

A subitem for Bug #1308426 that need to implement clear/filter buttons in Net Panel Toolbar
Whiteboard: [netmonitor]
Flags: qe-verify+
QA Contact: ciprian.georgiu
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Depends on: 1308500
Priority: -- → P1
Comment on attachment 8801055 [details]
Bug 1309191 - Implement filter buttons for Net Panel Toolbar

I uploaded my WIP patch and need your feedback.

React component for FilterButtons is completed as well as a part of Redux (filter action, filter reducer and state)

As I mentioned previously in meeting, we are not able to connect Redux completely without touching the requests-menu-view.js. requests-menu-view.js used $() selector to access the filter XUL element and use addEventLister to control the RequestMenuView itself.
Attachment #8801055 - Flags: feedback?(odvarko)
Attachment #8801055 - Flags: feedback?(jsnajdr)
Comment on attachment 8801055 [details]
Bug 1309191 - Implement filter buttons for Net Panel Toolbar

https://reviewboard.mozilla.org/r/85872/#review84482

::: devtools/client/netmonitor/netmonitor-controller.js:84
(Diff revision 3)
>      this._startup = promise.defer();
>      {
> +      try {
> -      NetMonitorView.initialize();
> +        NetMonitorView.initialize();
> +      } catch (err) {
> +        throw err;

Why this try/catch block?

::: devtools/client/netmonitor/netmonitor.xul
(Diff revision 3)
>        <hbox id="netmonitor-toolbar" class="devtools-toolbar">
>          <toolbarbutton id="requests-menu-clear-button"
>                         class="devtools-toolbarbutton devtools-clear-icon"
>                         tooltiptext="&netmonitorUI.footer.clear;"/>
> -        <hbox id="requests-menu-filter-buttons">
> +        <hbox id="react-filter-buttons-hook"></hbox>
> -          <button id="requests-menu-filter-all-button"

This will need to be a HTML element (<html:div>) if you want to mount a HTML React component here. Otherwise, <div>s will be XUL divs, which will cause layout problems.
Comment on attachment 8801055 [details]
Bug 1309191 - Implement filter buttons for Net Panel Toolbar

https://reviewboard.mozilla.org/r/85872/#review84484

Please have a look at the existing code in https://github.com/jsnajdr/gecko-dev/blob/master/devtools/client/netmonitor/
The Redux code for filtering is already implemented there. It's better to port it instead of rewriting from scratch. Saves time and avoids merge conflicts.

::: devtools/client/netmonitor/reducers/filters.js:33
(Diff revision 3)
> +
> +      if (filterType === "all") {
> +        return initialState;
> +      }
> +      return Object.assign({}, state, { [filterType]: !state[filterType] });
> +  }

When enabling a filter, "all" must me reset to false.
When disabling last enabled filter, "all" must be set to true.
Attachment #8801055 - Flags: feedback?(jsnajdr) → feedback+
Comment on attachment 8801055 [details]
Bug 1309191 - Implement filter buttons for Net Panel Toolbar

https://reviewboard.mozilla.org/r/85872/#review84498

Nice, I like where the is going!

Please see all my inline comments.

Honza

::: devtools/client/netmonitor/components/FilterButtons.js:39
(Diff revision 3)
> +module.exports = connect(
> +  (state) => ({
> +    filters: state.filters,
> +    onButtonClick: () => {},
> +  })
> +)(FilterButtons);

What if we used ES6 for all our React components?

::: devtools/client/netmonitor/constants.js:7
(Diff revision 3)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";
> +
> +const actionTypes = {
> +  FILTER_TOGGLE: "FILTER_TOGGLE",

Could we rather define two actions
* ENABLE_FILTER_KEY
* DISABLE_FILTER_KEY

With implementation something like:

/**
 *  Enable filter. Key can be set to 'all', 'html', etc.
 */
const enableFilterKey = (key) => ({ 
    type: types.ENABLE_FILTER_KEY,
    key: key, 
}); 

const disableFilterKey = (key) => ({ 
    type: types.DISABLE_FILTER_KEY,
    key: key, 
}); 

--

See also this discussion [1].


[1] https://groups.google.com/forum/#!searchin/mozilla.dev.developer-tools/toggle$20action%7Csort:relevance/mozilla.dev.developer-tools/XPx2pgz6EQs/LAEd4P5JIAAJ

::: devtools/client/netmonitor/netmonitor-controller.js:82
(Diff revision 3)
>        return this._startup.promise;
>      }
>      this._startup = promise.defer();
>      {
> +      try {
> -      NetMonitorView.initialize();
> +        NetMonitorView.initialize();

Please use spaces not tabs.

::: devtools/client/netmonitor/netmonitor.xul:98
(Diff revision 3)
>      <vbox id="network-inspector-view" flex="1">
>        <hbox id="netmonitor-toolbar" class="devtools-toolbar">
>          <toolbarbutton id="requests-menu-clear-button"
>                         class="devtools-toolbarbutton devtools-clear-icon"
>                         tooltiptext="&netmonitorUI.footer.clear;"/>
> -        <hbox id="requests-menu-filter-buttons">
> +        <hbox id="react-filter-buttons-hook"></hbox>

Why did you change the ID here? The original ID is used by FilterButtons component, no?

::: devtools/client/netmonitor/store.js:24
(Diff revision 3)
> +      media: false,
> +      flash: false,
> +      ws: false,
> +      other: false,
> +    },
>    };

Note that the original _activeFilters state was an array (like: ["all", "html"]). The transition might be easier if we keep the data structures the same.
(I would vote for keeping the original structure)
Attachment #8801055 - Flags: feedback?(odvarko) → feedback+
Comment on attachment 8801055 [details]
Bug 1309191 - Implement filter buttons for Net Panel Toolbar

https://reviewboard.mozilla.org/r/85872/#review85020

::: devtools/client/netmonitor/reducers/filters.js:60
(Diff revision 4)
> +    default:
> +      return state;
> +  }
> +}
> +
> +module.exports = filters;

its not clear what this filters applied for. When we visit code 3 month later, we might wonder if filters means search filter or something else. 

I suggest use toolbar_filter or request_menu_filter to better present its intention
Patch is ready and mochitest failures have been fixed! I'll rebase bug 1308500 to support l10n string once the netmonitor.properties is landed in m-c.

Note that this an important initial patch to refactor Toolbar module into react + redux. All following refactoring works will follow this paradigm (best practice), so we can review that carefully and also set up rules or conventions for anything (the design of actions, reducers or even store structure).

Although the code beyond react & redux is really ugly right now especially in netmonitor-view.js, request-menu-view.js and toolbar-view.js, I think i's okay and acceptable during the transition.

FilterButtons component is a best practice for react stateless functional component [1], so I prefer to not use ES2015 classes here.

And I also replaced filterOn and filterOnlyOn APIs with actions respectively and remove relative tests.

I prefer to define filter types as:

    filters: {
      types: {
        all: true,
        html: false,
        css: false,
        js: false,
        xhr: false,
        fonts: false,
        images: false,
        media: false,
        flash: false,
        ws: false,
        other: false,
      },
    },

It's really simple to align with FilterButtons component and easier to maintain by adding / deleting a type in the store and affect UI on the fly.

[1] https://medium.com/@housecor/react-stateless-functional-components-nine-wins-you-might-have-overlooked-997b0d933dbc#.p4ylxrscq
Attachment #8801055 - Flags: feedback?(gasolin)
Blocks: 1310922
Iteration: --- → 52.3 - Nov 7
Duplicate of this bug: 1310922
After discussing with Jarda, I'll have a separate patch here to use Immutable.js within this bug. Second patch will be coming soon.
Comment on attachment 8801055 [details]
Bug 1309191 - Implement filter buttons for Net Panel Toolbar

https://reviewboard.mozilla.org/r/85872/#review85452

::: devtools/client/netmonitor/components/FilterButtons.js:18
(Diff revision 6)
> +function FilterButtons({
> +  filterTypes,
> +  triggerFilterType,
> +}) {
> +  const buttons = Object.entries(filterTypes).map(([type, checked]) => {
> +    let className = ["requests-menu-filter-button"];

nit: I usually call this "classList", to distinguish array from the final string.

::: devtools/client/netmonitor/components/FilterButtons.js:24
(Diff revision 6)
> +    checked && className.push("checked");
> +
> +    return dom.button({
> +      id: `requests-menu-filter-${type}-button`,
> +      className: className.join(" "),
> +      checked,

Does the HTML button really have a "checked" attribute? Isn't this covered by the CSS class?

::: devtools/client/netmonitor/components/FilterButtons.js:40
(Diff revision 6)
> +  (state) => ({
> +    filterTypes: state.filters.types,
> +  }),

To avoid changing this code again in near future, the state should map to props like this:

(state) => ({ state })

I.e., map the state properties 1:1, no need to rename filters.types to filterTypes. That only adds confusion.

Also, with Immutable.js, it will be very convenient to pass the whole state as one property. The resulting props will be:

{
  state: { the immutable object from store }
  triggerFilterType: handler from mapDispatchToProps
  ...: all other handlers from mapDispatchToProps
}

::: devtools/client/netmonitor/netmonitor-view.js:83
(Diff revision 6)
>  };
>  // px
>  const NETWORK_ANALYSIS_PIE_CHART_DIAMETER = 200;
>  
> +// Inilialize the global store
> +const store = configureStore();

The store should be a "var", because then it's accessible as a window property (window.store). This is very useful for tests. See what memory tool does:

http://searchfox.org/mozilla-central/source/devtools/client/memory/initializer.js#30,34

::: devtools/client/netmonitor/netmonitor-view.js:1449
(Diff revision 6)
> +      store.dispatch(actions.updateFilterType("all", true));
> +      store.dispatch(actions.updateFilterType(item.label, true));

This trick to disable all filters and enabling only one is really hard to understand.

There should be an atomic action FILTER_TYPE_ONLY_ON (or similar name) that does all the state changes in the reducer.

I think it's wrong to expose only primitive actions on the store and force the view author to do the complicated composing themselves in the view code. The complexity should live in the reducer.

Also, dispatching two separate actions will cause the filtering and sorting being recomputed twice.

::: devtools/client/netmonitor/requests-menu-view.js:30
(Diff revision 6)
>         getAbbreviatedMimeType,
>         getUriNameWithQuery,
>         getUriHostPort,
>         getUriHost,
>         loadCauseString} = require("./request-utils");
> +const actions = require("./actions/index");

nit: modules that expose static functions as properties usually have names starting with uppercase: Actions.

This is consistent with things like Math.max or React.createClass.

::: devtools/client/netmonitor/test/browser_net_icon-preview.js
(Diff revision 6)
> -  RequestsMenu.filterOn("images");
> -  info("Checking the image thumbnail when only images are shown.");
> -  checkImageThumbnail();
> -

Shouldn't this test be fixed instead of deleting it?

::: devtools/client/netmonitor/test/browser_net_prefs-reload.js
(Diff revision 6)
> -    filters: {
> -      // A custom new value to be used for the verified preference.
> -      newValue: ["html", "css"],
> -      // Getter used to retrieve the current value from the frontend, in order
> -      // to verify that the pref was applied properly.
> -      validateValue: ($) => getView().RequestsMenu._activeFilters,
> -      // Predicate used to modify the frontend when setting the new pref value,
> -      // before trying to validate the changes.
> -      modifyFrontend: ($, value) => value.forEach(e => getView().RequestsMenu.filterOn(e))
> -    },

Again, fix instead of deleting.

::: devtools/client/netmonitor/toolbar-view.js:29
(Diff revision 6)
> +    dumpn("Initializing the ToolbarView");
> +
> +    ReactDOM.render(createElement(
> +      Provider,
> +      { store },
> +      createElement(FilterButtons)

nit: The convention in devtools code is to use createFactory:

const FilterButtons = createFactory(require(...));

and then use FilterButtons() instead of createElement(FilterButtons).

::: devtools/client/netmonitor/toolbar-view.js:41
(Diff revision 6)
> +
> +  /**
> +   * Destruction function, called when the debugger is closed.
> +   */
> +  destroy: function () {
> +    dumpn("Destroying the ToolbarView");

Unmount the rendered React component here. It will leak otherwise.
Attachment #8801055 - Flags: review?(jsnajdr)
Comment on attachment 8801055 [details]
Bug 1309191 - Implement filter buttons for Net Panel Toolbar

https://reviewboard.mozilla.org/r/85872/#review85464

::: devtools/client/themes/netmonitor.css:764
(Diff revision 6)
>  
>  .requests-menu-filter-button:hover:active {
>    background-color: var(--theme-selection-background-semitransparent);
>  }
>  
> -.requests-menu-filter-button:not(:active)[checked] {
> +.requests-menu-filter-button:not(:active).checked {

Can you remove all the code related to .requests-menu-filter-button and use the menu-filter-button class instead?
Comment on attachment 8801055 [details]
Bug 1309191 - Implement filter buttons for Net Panel Toolbar

https://reviewboard.mozilla.org/r/85872/#review85492

::: devtools/client/netmonitor/components/FilterButtons.js:13
(Diff revision 6)
> +  PropTypes,
> +} = require("devtools/client/shared/vendor/react");
> +const { connect } = require("devtools/client/shared/vendor/react-redux");
> +const actions = require("../actions/index");
> +
> +function FilterButtons({

current all files in gecko and react components in `devtools/shared/components` use `dash-case.js` (xxx-xx) instead of `DoubleCamelCase.js`, should we follow the same converntion?

::: devtools/client/netmonitor/components/FilterButtons.js:15
(Diff revision 6)
> +const { connect } = require("devtools/client/shared/vendor/react-redux");
> +const actions = require("../actions/index");
> +
> +function FilterButtons({
> +  filterTypes,
> +  triggerFilterType,

`onClick`|`clickHandler` would better explain its intention

::: devtools/client/netmonitor/components/FilterButtons.js:36
(Diff revision 6)
> +  return dom.div({ id: "requests-menu-filter-buttons" }, buttons);
> +}
> +
> +FilterButtons.PropTypes = {
> +  filterTypes: PropTypes.object.isRequired,
> +  onButtonClick: PropTypes.func.iRequired,

should be `triggerFilterType`,  or `onClick`|`clickHandler` as previous suggestion
Attachment #8801055 - Flags: feedback?(gasolin) → feedback+
Comment on attachment 8801055 [details]
Bug 1309191 - Implement filter buttons for Net Panel Toolbar

https://reviewboard.mozilla.org/r/85872/#review85492

> `onClick`|`clickHandler` would better explain its intention

No, I don't prefer that since triggerFilterType is used by both onClick and onKeyDown, so we should use a general name for it.
Comment on attachment 8801055 [details]
Bug 1309191 - Implement filter buttons for Net Panel Toolbar

https://reviewboard.mozilla.org/r/85872/#review85492

> current all files in gecko and react components in `devtools/shared/components` use `dash-case.js` (xxx-xx) instead of `DoubleCamelCase.js`, should we follow the same converntion?

Good question! 
Most of the outside react repos are using CamelCase.js and debugger.html as well (https://github.com/devtools-html/debugger.html/tree/master/public/js/components).
(In reply to Ricky Chien [:rickychien] from comment #19)
> Good question! 
> Most of the outside react repos are using CamelCase.js and debugger.html as
> well
> (https://github.com/devtools-html/debugger.html/tree/master/public/js/
> components).
I vote for being consistent with the devtools/ code base, i.e. using dash-case.js style. Btw. I haven't seen any discussion about changing the style to CamelCase.js. Debugger.html sounds like an exception in this case (like they are already living on github etc).

But of course, we might want to post a question to developer-tools group and ask.

Honza
Addressed all issues and rebase for supporting l10n with netmonitor.properties. Please take a look again :)
Comment on attachment 8801055 [details]
Bug 1309191 - Implement filter buttons for Net Panel Toolbar

https://reviewboard.mozilla.org/r/85872/#review85528

Nice work!

Please see my inline comments.

Honza

::: devtools/client/netmonitor/actions/filters.js:11
(Diff revision 8)
> +const {
> +  UPDATE_FILTER_TYPE,
> +  UPDATE_FILTER_TYPE_EXCLUSIVE,
> +} = require("../constants");
> +
> +function updateFilterType(filterType, checked) {

It would be great to have a comment descriging the action.

::: devtools/client/netmonitor/actions/filters.js:19
(Diff revision 8)
> +    filterType,
> +    checked,
> +  };
> +}
> +
> +function updateFilterTypeExclusive(filterType, checked) {

The same, please create a comment descriging the action.

::: devtools/client/netmonitor/components/moz.build:8
(Diff revision 8)
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DevToolsModules(
> +    'FilterButtons.js',
> +)

I mentioned this already, just for the record, I think we should use dash-style.js naming style to be consistent.

::: devtools/client/netmonitor/netmonitor-view.js:34
(Diff revision 8)
>  const {L10N} = require("./l10n");
>  const {RequestsMenuView} = require("./requests-menu-view");
> +const {ToolbarView} = require("./toolbar-view");
> +const {configureStore} = require("./store");
> +
> +// Inilialize the global redux variables

Fix typo: Initialize
Attachment #8801055 - Flags: review?(odvarko)
Comment on attachment 8801055 [details]
Bug 1309191 - Implement filter buttons for Net Panel Toolbar

https://reviewboard.mozilla.org/r/85872/#review85522

Starts to look really good! I only have a few minor issues remaining.

::: devtools/client/locales/en-US/netmonitor.properties:481
(Diff revision 8)
>  netmonitor.footer.filterFreetext.label=Filter URLs
>  netmonitor.footer.filterFreetext.key=F

It would be good to convert the remaining "netmonitor.footer" labels, too. Otherwise, the naming scheme will remain inconsistent and chaotic forever - nobody will have the motivation to finish the task.

::: devtools/client/netmonitor/constants.js:11
(Diff revision 8)
> +const actionTypes = {
> +  UPDATE_FILTER_TYPE: "UPDATE_FILTER_TYPE",
> +  UPDATE_FILTER_TYPE_EXCLUSIVE: "UPDATE_FILTER_TYPE_EXCLUSIVE",
> +};
> +
> +module.exports = Object.assign({}, actionTypes);

nit: Object.assign is not needed here.

::: devtools/client/netmonitor/netmonitor-view.js:35
(Diff revision 8)
>  const {RequestsMenuView} = require("./requests-menu-view");
> +const {ToolbarView} = require("./toolbar-view");
> +const {configureStore} = require("./store");
> +
> +// Inilialize the global redux variables
> +var gActions = require("./actions/index");

The gActions variable doesn't contain any data or state - just code. It's better to require() the module in every file where it's needed.

It makes sense for gStore to be a globa, as it is a singleton with app data.

::: devtools/client/netmonitor/reducers/filters.js:12
(Diff revision 8)
> +  types: {
> +    all: true,
> +    html: false,
> +    css: false,
> +    js: false,
> +    xhr: false,
> +    fonts: false,
> +    images: false,
> +    media: false,
> +    flash: false,
> +    ws: false,
> +    other: false,
> +  },

The initial state is duplicated at two places - not nice.

Immediately after Redux creates a store, it dispatches an "INIT" action on it. The expected outcome is that every reducer returns its initial state:

https://github.com/reactjs/redux/blob/55f1d08000b1b064eaa933bbbd132230e53bcccb/src/createStore.js#L242-L245

You can use this to hydrate the initial state without specifying everything in the "initialState" parameter to createStore.

::: devtools/client/netmonitor/reducers/filters.js:51
(Diff revision 8)
> +  }
> +
> +  return newState;
> +}
> +
> +function filterTypesExclusive(state, action) {

filterTypesExclusive doesn't need the "checked" parameter - it's always true. When setting it to "false", the action can put the store into a prohibited state - with every filter disabled, including the "all" one.

Consider giving these functions better names. "filterTypes" sounds like it's filtering some list of types, not setting flags.

I'm starting to be in favor of having actions FILTER_TYPE_TOGGLE and FILTER_TYPE_ONLY_ON. That looks much more comprehensible and well-named to me.

::: devtools/client/netmonitor/requests-menu-view.js:191
(Diff revision 8)
> +    this.unsubscribeStore = store.subscribe(() => {
> +      const state = store.getState();
> +      this._activeFilters = Object
> +          .entries(state.filters.types)
> +          .filter(([type, checked]) => checked)
> +          .map(([type, checked]) => type);
> +      this.reFilterRequests();
> +    });

This subscribe handler should be smarter and only call reFilterRequests when the active filters really changed.

My storeWatcher function here:

https://github.com/jsnajdr/gecko-dev/blob/master/devtools/client/netmonitor/requests-menu-view.js#L160-L170

does a simple memoization - stores the previous value of some computed quantity, and calls an update handler only if the next value is really different.

After we add more data to the store, it will start firing hundreds of updates, and only very few of them will change the active filters.

::: devtools/client/netmonitor/test/browser_net_icon-preview.js:14
(Diff revision 8)
> -  let { $, $all, EVENTS, ACTIVITY_TYPE, NetMonitorView, NetMonitorController } =
> -    monitor.panelWin;
> +  let { $, $all, EVENTS, ACTIVITY_TYPE, NetMonitorView, NetMonitorController,
> +        gStore, gActions } = monitor.panelWin;

Import the actions like any other module - lot of Netmonitor mochitests import the "l10n" module.
Attachment #8801055 - Flags: review?(jsnajdr)
> ::: devtools/client/netmonitor/reducers/filters.js:51
> (Diff revision 8)
> > +  }
> > +
> > +  return newState;
> > +}
> > +
> > +function filterTypesExclusive(state, action) {
> 
> filterTypesExclusive doesn't need the "checked" parameter - it's always
> true. When setting it to "false", the action can put the store into a
> prohibited state - with every filter disabled, including the "all" one.
> 
> Consider giving these functions better names. "filterTypes" sounds like it's
> filtering some list of types, not setting flags.
> 
> I'm starting to be in favor of having actions FILTER_TYPE_TOGGLE and
> FILTER_TYPE_ONLY_ON. That looks much more comprehensible and well-named to
> me.

Honza, do you have any opinion of this? I can't come out with better names for it.
Flags: needinfo?(odvarko)
Comment on attachment 8801055 [details]
Bug 1309191 - Implement filter buttons for Net Panel Toolbar

https://reviewboard.mozilla.org/r/85872/#review85618

::: devtools/client/themes/netmonitor.css:745
(Diff revision 9)
>  #custom-method-value {
>    width: 4.5em;
>  }
>  
>  /* Main toolbar */
> -.requests-menu-filter-button {
> +.menu-filter-button {

The CSS for .menu-filter-button is already defined in common.css, so you can just remove the one in netmonitor.css (including the hover/active/checked CSS)
(In reply to Ricky Chien [:rickychien] from comment #27)
> > I'm starting to be in favor of having actions FILTER_TYPE_TOGGLE and
> > FILTER_TYPE_ONLY_ON. That looks much more comprehensible and well-named to
> > me.
> 
> Honza, do you have any opinion of this? I can't come out with better names
> for it.
Naming is hard.

I like if verb comes first (like setFlag, enableSomething, executeCommand, etc.). Also, in our case, we are dealing with boolean values and I think we should use verbs as 'toggle' (to flip boolean value) and 'enable' (to set boolean value to true).

// Toggle an existing filter type state (true/false)
TOGGLE_FILTER_TYPE

// Set an existing filter type to true. If type 'all' is
// specified - all the other filter types are set to false.
ENABLE_FILTER_TYPE

--

This is optional, but... I don't like the 'TYPE' part since it sounds like it indicates an action type (the constant is an action type constant). So for me the following would work a bit better:

TOGGLE_FILTER -> flip the value.
ENABLE_FILTER -> set the value to true.

Honza
Flags: needinfo?(odvarko)
Issues of first are addressed and I'm going to submit second patch for introducing Immutable.js
Update patch for Immutable.js at https://reviewboard.mozilla.org/r/85872/diff/10-11/

I'm not so familiar with Immutable.js and it takes time to figure out to use appropriate APIs.

Please take a look for the part of Immutable.js and I'd like to land it once in this bug.
Comment on attachment 8801055 [details]
Bug 1309191 - Implement filter buttons for Net Panel Toolbar

https://reviewboard.mozilla.org/r/85872/#review85816

::: devtools/client/netmonitor/requests-menu-view.js:31
(Diff revision 11)
>         getAbbreviatedMimeType,
>         getUriNameWithQuery,
>         getUriHostPort,
>         getUriHost,
>         loadCauseString} = require("./request-utils");
> +const Actions = require("./actions/index");

nit:missing indent
Comment on attachment 8801055 [details]
Bug 1309191 - Implement filter buttons for Net Panel Toolbar

https://reviewboard.mozilla.org/r/85872/#review85812

::: devtools/client/netmonitor/netmonitor-controller.js:6
(Diff revisions 9 - 11)
>  /* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
>  /* vim: set ft=javascript ts=2 et sw=2 tw=80: */
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> -/* globals window, document, NetMonitorView, gStore, gActions */
> +/* globals window, document, NetMonitorView, gStore, Actions */

The "Actions" global is the "const Actions" imported in netmonitor-view.js? And it's visible even here in netmonitor-controller.js?

::: devtools/client/netmonitor/netmonitor-view.js:111
(Diff revisions 9 - 11)
>     */
>    initialize: function () {
>      this._initializePanes();
>  
>      this.Toolbar.initialize(gStore);
> -    this.RequestsMenu.initialize(gStore);
> +    this.RequestsMenu.initialize(gStore, storeWatcher);

Move the storeWatcher definition to requests-menu-view.js. No need to pass it as a parameter.

Can be optionally factored out to its own module, but only after it's used by multiple modules. I think it won't happen - as we convert the whole UI to React, subscribing to the store won't be necessary.

::: devtools/client/netmonitor/reducers/filters.js:6
(Diff revisions 9 - 11)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  "use strict";
>  
> +const Immutable = require("devtools/client/shared/vendor/immutable");

debugger.html uses:

const I = require(".../immutable");

I like it a lot, because it avoids the Java-style verbosity where you write "Immutable.Something" everywhere, replacing it with nice and concise "I.Something". Opinions?

::: devtools/client/netmonitor/reducers/filters.js:14
(Diff revisions 9 - 11)
> -  UPDATE_FILTER_TYPE_EXCLUSIVE,
> +  ENABLE_FILTER_ONLY,
>  } = require("../constants");
>  
> -const initialState = {
> -  types: {
> +
> +const FiltersState = Immutable.Record({
> +  types: Immutable.OrderedMap({

This should also be an I.Record - there are always the same properties, nothing is ever added or removed.

Also, you can use a neat trick where record.remove("prop") doesn't remove the property, but resets it to the default value. Works only for Records - the default value is specified in the Record definition, and properties cannot be really removed or added.

::: devtools/client/netmonitor/reducers/filters.js:38
(Diff revisions 9 - 11)
> -  // Ignore unknown filtertype
> -  if (!Object.keys(state).includes(filterType)) {
> +  // Ignore unknown filter type
> +  if (!state.has(filter)) {
>      return state;
>    }
> -  if (filterType === "all") {
> -    return initialState.types;
> +  if (filter === "all") {
> +    return new FiltersState().types;

Define a new Record just for the types (named FilterTypes or something like that), and write just:

return new FilterTypes();

::: devtools/client/netmonitor/reducers/filters.js:41
(Diff revisions 9 - 11)
> -  newState = Object.assign({}, state, {
> -    all: false,
> -    [filterType]: checked,
> +  newState = state.withMutations(type => {
> +    type.set("all", false);
> +    type.set(filter, !state.get(filter));

nit: The variable should be named "types" - it's the record with all the types, not a single type field.

::: devtools/client/netmonitor/reducers/filters.js:65
(Diff revisions 9 - 11)
>    }
> -  if (filterType === "all") {
> -    return initialState.types;
> +  if (filter === "all") {
> +    return new FiltersState().types;
>    }
>  
> -  newState = Object.assign({}, initialState.types, {
> +  newState = state.map((checked, type) => type === filter);

Nit: Another way to write this is:

newState = new FilterTypes({
  all: false,
  [filter]: true
});

Not sure if it's better than the "map" call, up to you to choose.

::: devtools/client/netmonitor/reducers/filters.js:75
(Diff revisions 9 - 11)
> -function filters(state = initialState, action) {
> +function filters(state = new FiltersState(), action) {
>    let types;
>    switch (action.type) {
> -    case UPDATE_FILTER_TYPE:
> -      types = filterTypes(state.types, action);
> -      return Object.assign({}, state, { types });
> +    case TOGGLE_FILTER:
> +      types = toggleFilter(state.types, action);
> +      return state.merge({ types });

If there's only one property to set, you can also use:

state.setIn("types", types);
Attachment #8801055 - Flags: review?(jsnajdr)
(In reply to Jarda Snajdr [:jsnajdr] from comment #36)
> I like it a lot, because it avoids the Java-style verbosity where you write
> "Immutable.Something" everywhere, replacing it with nice and concise
> "I.Something". Opinions?

It's ok for me if this is unique and specific case preferably shared across DevTools code base. In general, I rather don't like shortcuts and I think it would be bad if we end up with: I.something, S.somethingElse, AB.something, etc. in the code.

One nice way how to avoid verbosity is also destructing.

Instead of repeating "Immutable.Something" everywhere in a module we might want to do:

let { Something } = Immutable;

... at the beginning of the module. This also works pretty well for react elements e.g.

let { span, dom } = React.DOM;

... especially when you use span and div many times in a module

Honza
Comment on attachment 8801055 [details]
Bug 1309191 - Implement filter buttons for Net Panel Toolbar

https://reviewboard.mozilla.org/r/85872/#review85842

::: devtools/client/netmonitor/components/filter-buttons.js:6
(Diff revision 11)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";
> +
> +const {

I'd prefer extract explicit DOM elements at the top instead of calling alias as  `dom.button`

ex:
```
const { DOM, PropTypes } = require("devtools/client/shared/vendor/react");
...

// Shortcuts
const { button } = DOM;
```
(In reply to Fred Lin [:gasolin] from comment #38)
> I'd prefer extract explicit DOM elements at the top instead of calling alias
> as  `dom.button`
> 
> ex:
> ```
> const { DOM, PropTypes } = require("devtools/client/shared/vendor/react");
> ...
> 
> // Shortcuts
> const { button } = DOM;
> ```

Yes, I really like this too.

Honza
Comment on attachment 8801055 [details]
Bug 1309191 - Implement filter buttons for Net Panel Toolbar

https://reviewboard.mozilla.org/r/85872/#review85842

> I'd prefer extract explicit DOM elements at the top instead of calling alias as  `dom.button`
> 
> ex:
> ```
> const { DOM, PropTypes } = require("devtools/client/shared/vendor/react");
> ...
> 
> // Shortcuts
> const { button } = DOM;
> ```

I reference the code from webconsole, so let's keep the consistency with webconsole.

Also, It would be convenient when trying to create some different DOM element, because you don't have to modify two lines.
Comment on attachment 8801055 [details]
Bug 1309191 - Implement filter buttons for Net Panel Toolbar

https://reviewboard.mozilla.org/r/85872/#review85812

> The "Actions" global is the "const Actions" imported in netmonitor-view.js? And it's visible even here in netmonitor-controller.js?

Yes, I know that is ugly but every global will populate in here

> debugger.html uses:
> 
> const I = require(".../immutable");
> 
> I like it a lot, because it avoids the Java-style verbosity where you write "Immutable.Something" everywhere, replacing it with nice and concise "I.Something". Opinions?

I'm in favor of using Immutable because It is too short to understand what it means and webconsole also use the full name Immutable;
Comment on attachment 8801055 [details]
Bug 1309191 - Implement filter buttons for Net Panel Toolbar

https://reviewboard.mozilla.org/r/85872/#review85850

::: devtools/client/netmonitor/toolbar-view.js:25
(Diff revision 11)
> +   * Initialization function, called when the debugger is started.
> +   */
> +  initialize: function (store) {
> +    dumpn("Initializing the ToolbarView");
> +
> +    this._containerNode = $("#react-filter-buttons-hook");

since its just a filterbutton container of toolbox, we can name it as `_filterContainerNode` and use `_containerNode` when we migrate all xuls to react components
Comment on attachment 8801055 [details]
Bug 1309191 - Implement filter buttons for Net Panel Toolbar

https://reviewboard.mozilla.org/r/85872/#review85812

> I'm in favor of using Immutable because It is too short to understand what it means and webconsole also use the full name Immutable;

Let's use destructing const { Record } = require(".../immutable");. I like this!
(In reply to Jan Honza Odvarko [:Honza] from comment #37)
> It's ok for me if this is unique and specific case preferably shared across
> DevTools code base. In general, I rather don't like shortcuts and I think it
> would be bad if we end up with: I.something, S.somethingElse, AB.something,
> etc. in the code.

Immutable is unique in a way - it's not just another library with some API, but it replaces the language's core data structures with their own version. Making these identifiers as short as possible makes sense. Other libraries that are used with supershort names are lodash (_) and jQuery ($). They are also the "extension of the language" kind of library.

> One nice way how to avoid verbosity is also destructing.
> 
> Instead of repeating "Immutable.Something" everywhere in a module we might
> want to do:
> 
> let { Something } = Immutable;

Will be confusing for Map or Set - overrides the native mutable versions.

> ... at the beginning of the module. This also works pretty well for react
> elements e.g.
> 
> let { span, dom } = React.DOM;

I like this, provided it doesn't create any serious name conflicts.
I did some research on how to do "map" and "filter" operations on a Immutable.Record:

Record itself has methods "map" and "filter", but they behave strangely - instead of returning a new, transformed object and leaving the original one intact, as one would expect from an immutable structure, they mutate the Record in-place and return nothing. So, never use these methods on a Redux state, as it will do unwanted mutations!

They have a two years old issue reported: https://github.com/facebook/immutable-js/issues/268

The right way seems to be to convert the Record to a Seq, transform it and then convert to a final output structure. For example, this code will convert Record({ html: true, js: false }) to ["html"]:

rec.toSeq() // Convert to a keyed seq
   .filter(value => value) // Keep only true keys
   .keySeq() // Convert to a seq that contains only keys, discard values
   .toArray() // Convert to JS array
Comment on attachment 8801055 [details]
Bug 1309191 - Implement filter buttons for Net Panel Toolbar

https://reviewboard.mozilla.org/r/85872/#review86116

::: devtools/client/netmonitor/netmonitor.xul:77
(Diff revision 12)
>  
>    <deck id="body" class="theme-sidebar" flex="1">
>  
>      <vbox id="network-inspector-view" flex="1">
>        <hbox id="netmonitor-toolbar" class="devtools-toolbar">
>          <toolbarbutton id="requests-menu-clear-button"

As bug title, the clear button is not handled in this patch, would you like to create another bug to handle it?
Comment on attachment 8801055 [details]
Bug 1309191 - Implement filter buttons for Net Panel Toolbar

https://reviewboard.mozilla.org/r/85872/#review86116

> As bug title, the clear button is not handled in this patch, would you like to create another bug to handle it?

Sure!
Blocks: 1311591
Renamed bug title since clear button is another different react component comparing to filter buttons. I'd like to deal with clear button in separate bug 1311591.
Summary: Implement clear/filter buttons for Net Panel Toolbar → Implement filter buttons for Net Panel Toolbar
Comment on attachment 8801055 [details]
Bug 1309191 - Implement filter buttons for Net Panel Toolbar

https://reviewboard.mozilla.org/r/85872/#review86128

Looks good to me!

I only have some suggestions on how to make Immutable code cleaner.

Also, I noticed that the HTML-ized buttons look differently from the original XUL ones. The XUL buttons were a few pixel higher, the new HTML ones look "squeezed".

The cause is that the XUL button contains a inner xul:label element with class .button-text, which has a 1px margin at the top and bottom. There is no such spacing in the HTML version. This also affects the Console (the React version). I don't mind if it's fixed in a separate followup bug.

::: devtools/client/netmonitor/components/filter-buttons.js:18
(Diff revision 15)
> +function FilterButtons({
> +  filterTypes,
> +  triggerFilterType,
> +}) {
> +  const buttons = filterTypes
> +    .entrySeq().toArray().map(([type, checked]) => {

Similar cleanup can be done here: move toArray to the end.

::: devtools/client/netmonitor/requests-menu-view.js:208
(Diff revision 15)
> +        this._activeFilters = newTypes
> +          .entrySeq()
> +          .filter(([type, checked]) => checked)
> +          .toArray()
> +          .map(([type]) => type);

Do all the map and filter operations on the Seq and convert it to the resulting structure (JS array or anything else) only at the end. This should be more efficient.

This code can be rewritten like this:

this._activeFilters = newTypes
  .toSeq()
  .filter((checked, key) => checked)
  .keySeq()
  .toArray()
  
Things worth noticing:
- toSeq() for Map and Record returns a KeyedSeq type
- filter/map on KeyedSeq doesn't use have array arg, but two separate args
- keySeq() converts KeyedSeq to normal Seq, returning the keys and forgetting values
- toArray() is at the very end
Attachment #8801055 - Flags: review?(jsnajdr) → review+
Comment on attachment 8801055 [details]
Bug 1309191 - Implement filter buttons for Net Panel Toolbar

https://reviewboard.mozilla.org/r/85872/#review86138

R+ assuming Try is green.

Thanks!
Honza
Attachment #8801055 - Flags: review?(odvarko) → review+
Note Bug 1268444 is checked-in and will change freetextfilter shortcut key, you may need a rebase to avoid the conflict.
Blocks: 1309193
Since my patch is conflict with Bug 1268444 as Fred mentioned on comment 58. I rebased to latest m-c and fix all conflicts. Everything looks great now!
Pushed by wpan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ac36d8c6cb45
Implement filter buttons for Net Panel Toolbar r=Honza,jsnajdr
Since I am PTO during this time, so I can't fix that failure ASAP. Fred, could you help me take a look what's going on in Windows 8 failure? 

I haven't seen this error in last try (but it seems that mochitest only cover for Window 7)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75695735a732

Thanks!
Flags: needinfo?(rchien) → needinfo?(gasolin)
OK, rebased and push a new try to see what happened
Flags: needinfo?(gasolin)
The browser_net_autoscroll looks like a random intermittent not related to the filter button changes. The test looks rather fragile and will need to be rewritten completely for the React/Redux version. If it continues to cause trouble with landing this patch, I'd prefer to disable it.

Fred, the rebased patch you submitted has a different MozReview-Commit-ID, so there are now two patches attached. Could you reconcile them into one? The current situation is very confusing, as it's not clear what you've changed, or if you changed anything at all.
Flags: needinfo?(gasolin)
The current PR is used to trigger the try run,
I would change the patch to just disable browser_net_autoscroll test
Flags: needinfo?(gasolin)
Attachment #8803288 - Attachment is obsolete: true
Attachment #8803288 - Flags: review?(odvarko)
Attachment #8803288 - Flags: review?(jsnajdr)
Comment on attachment 8803293 [details]
Bug 1309191 - Disable the browser_net_autoscroll mochitest

https://reviewboard.mozilla.org/r/87608/#review86554
Attachment #8803293 - Flags: review?(odvarko) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7a464e03462b
Disable the browser_net_autoscroll mochitest r=Honza
Keywords: checkin-needed
(In reply to Fred Lin [:gasolin] from comment #67)
> The current PR is used to trigger the try run,
> I would change the patch to just disable browser_net_autoscroll test

You don't need to push patches to MozReview if you only want to start a try run. Please discard the patch - it causes confusion when determining what should land and what shouldn't.
Flags: needinfo?(gasolin)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/41978ce40aaa
Implement filter buttons for Net Panel Toolbar r=Honza,jsnajdr
https://hg.mozilla.org/mozilla-central/rev/7a464e03462b
https://hg.mozilla.org/mozilla-central/rev/41978ce40aaa
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Depends on: 1312333
Flags: needinfo?(gasolin)
I can confirm the filter buttons work accordingly. I verified using Fx 52.0a1 (20161108030212).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.