Closed Bug 1411855 Opened 2 years ago Closed 2 years ago

remove unused and no inline function in toolbar

Categories

(DevTools :: Netmonitor, enhancement, P3)

enhancement

Tracking

(firefox57 fix-optional, firefox58 fixed)

RESOLVED FIXED
Firefox 58
Tracking Status
firefox57 --- fix-optional
firefox58 --- fixed

People

(Reporter: gasolin, Assigned: gasolin)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

* getDisplayedRequestsSummary is claimed but not called http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/Toolbar.js#18

* we have inline function for autocompleteProvider, should be predefined instead
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Comment on attachment 8922213 [details]
Bug 1411855 - remove unused and no inline function in toolbar;

https://reviewboard.mozilla.org/r/193230/#review198454

Thanks for the clean up. LGTM!

::: devtools/client/netmonitor/src/components/Toolbar.js:159
(Diff revision 1)
>        "devtools-button",
>        "requests-list-pause-button",
>        recording ? "devtools-pause-icon" : "devtools-play-icon",
> +    ].join(" ");
> +
> +    // detail toggle button

nit: Detail (use capital)
Attachment #8922213 - Flags: review?(rchien) → review+
Can't see obvious perf gain in DAMP but at least in same level

Before

{
  "framework": {
    "name": "talos"
  },
  "suites": [
        {
          "name": "simple.netmonitor.open.DAMP",
          "value": 370.6224999999977
        },
        {
          "name": "simple.netmonitor.reload.DAMP",
          "value": 63.722499999999854
        },
        {
          "name": "simple.netmonitor.requestsFinished.DAMP",
          "value": 147.97499999999945
        },
        {
          "name": "simple.netmonitor.close.DAMP",
          "value": 31.0625
        },
        {
          "name": "complicated.netmonitor.open.DAMP",
          "value": 500.8174999999999
        },
        {
          "name": "complicated.netmonitor.reload.DAMP",
          "value": 1122.4400000000005
        },
        {
          "name": "complicated.netmonitor.requestsFinished.DAMP",
          "value": 5633.3825000000015
        },
        {
          "name": "complicated.netmonitor.close.DAMP",
          "value": 90.90000000000146
        }
      ],
      "value": 274.8612489145775
    }
  ]
}


After

{
        {
          "name": "simple.netmonitor.open.DAMP",
          "value": 360.6075000000001
        },
        {
          "name": "simple.netmonitor.reload.DAMP",
          "value": 61.61499999999978
        },
        {
          "name": "simple.netmonitor.requestsFinished.DAMP",
          "value": 138.48999999999978
        },
        {
          "name": "simple.netmonitor.close.DAMP",
          "value": 30.317500000000564
        },
        {
          "name": "complicated.netmonitor.open.DAMP",
          "value": 496.16749999999956
        },
        {
          "name": "complicated.netmonitor.reload.DAMP",
          "value": 1105.4500000000007
        },
        {
          "name": "complicated.netmonitor.requestsFinished.DAMP",
          "value": 5402.622500000001
        },
        {
          "name": "complicated.netmonitor.close.DAMP",
          "value": 93.95000000000255
        }
      ],
      "value": 268.60786585086134
    }
  ]
}
rebase and got `resource://devtools/client/shared/components/AutoCompletePopup.js, line 45: TypeError: list is undefined` while test the filter, will investigate before landing
Comment on attachment 8922213 [details]
Bug 1411855 - remove unused and no inline function in toolbar;

https://reviewboard.mozilla.org/r/193230/#review199130

::: devtools/client/netmonitor/src/components/Toolbar.js:229
(Diff revision 3)
>              delay: FILTER_SEARCH_DELAY,
>              keyShortcut: SEARCH_KEY_SHORTCUT,
>              placeholder: SEARCH_PLACE_HOLDER,
>              type: "filter",
>              onChange: setRequestFilterText,
> -            autocompleteProvider: filter =>
> +            autocompleteProvider: this.autocompleteFilter

The name autocompleteFilter seems confusing in this context. Can we simply use this.autocompleteProvider ?
Comment on attachment 8922213 [details]
Bug 1411855 - remove unused and no inline function in toolbar;

https://reviewboard.mozilla.org/r/193230/#review199134

::: devtools/client/netmonitor/src/components/Toolbar.js:119
(Diff revision 3)
> +  autocompleteFilter(filter) {
> +    autocompleteProvider(filter, this.props.filteredRequests);

You should return autocompleteProvider(...) to have the same behaviour as before.

The fact that you don't return causes comment 6.
Comment on attachment 8922213 [details]
Bug 1411855 - remove unused and no inline function in toolbar;

https://reviewboard.mozilla.org/r/193230/#review199130

> The name autocompleteFilter seems confusing in this context. Can we simply use this.autocompleteProvider ?

updated
Comment on attachment 8922213 [details]
Bug 1411855 - remove unused and no inline function in toolbar;

https://reviewboard.mozilla.org/r/193230/#review199134

> You should return autocompleteProvider(...) to have the same behaviour as before.
> 
> The fact that you don't return causes comment 6.

yes that's the reason, thanks for catch that
Pushed by flin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a74a0791fbb8
remove unused and no inline function in toolbar;r=rickychien
Backout by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/655bfb78042f
Backed out changeset a74a0791fbb8 for failing ESlint in devtools/client/netmonitor/src/components/Toolbar.js:27:1 r=backout on CLOSED TREE
Backed out changeset a74a0791fbb8 (bug 1411855) for failing ESlint in devtools/client/netmonitor/src/components/Toolbar.js:27:1 r=backout on CLOSED TREE
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a74a0791fbb85c1374c3267652b51b74219519b1&selectedJob=141000860
https://hg.mozilla.org/integration/autoland/rev/655bfb78042f705b8b3d838c8216b72926baa1e5
Flags: needinfo?(gasolin)
thanks for notice. lint fixed, eslint checked locally and try green https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3bac3d72928

let's land again
Flags: needinfo?(gasolin)
Pushed by flin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aadc92d4e79b
remove unused and no inline function in toolbar;r=rickychien
https://hg.mozilla.org/mozilla-central/rev/aadc92d4e79b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.