1341278 - Cannot hold checked filter button when go back from performance analysis

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[magicp]

[Steps to reproduce]
1. Start Nightly
2. Open Netmonitor
3. Go to any sites (e.g. https://www.mozilla.org/en-US/contribute/)
4. Click "CSS" filter button
5. Start performance analysis
6. Click back button
7. Check filter button

[Actual Results]
Reset checked filter button from "CSS" to "All".

[Expected Results]
"CSS" filter button is holding the checked status, even if go back from performance analysis.

Comment 1

[magicp]

Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=620f06960400e4d95da0ed5c9682edda3aa136ec&tochange=53698c71b19066259c1b033569ddf1cf64710421

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the report!

@Adrien: any tips how we could solve this report?

My feeling is that the filter shouldn't be changed. It should change only if the user explicitly change it by clicking on a chart or legend in the performance view.

Honza

Comment 3

[Adrien Enault]

Hello,

I agree, I don't remember this behaviour when I changed when we save the filters. (but to be fair, I don't remember having clicked on Back from the performance screen, only clicking on the pie chart).

After investigation, the reason you go back to the all filter after clicking on back is that every single filter saved is clicked again because the controller is initialized, but, because the state is already initialized, it just disables them all, going back to no filter, which strikes me as a bit odd.

The easiest way to fix it is to ensure you start from no filters, as I did in the patch I'm attaching, but this means that each time you come back from the perf analysis, you save the filters. I don't have a strong opinion on whether it's a bug or not, but it seems a bit strange and unintended. The other way to fix would be to avoid re initializing the state.

From what I understand from your comment, you also want to disable saving the filters if the user clicks on one of the category in the pie chart of the perf analysis ?

Best regards,

Adrien.

Comment 4

[Adrien Enault]

Attachment: netmonNotSaved.patch

Comment 5

[Adrien Enault]

I took another look and to fix this issue, we could probably give a proper initial state when creating the store since Redux supports it. We would then stop having to "click" on the filters when mounting the Toolbar component (remove the componentDidMount function).

If you think this is the way to go, I could give it a go.

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Adrien Enault from comment #5)
> I took another look and to fix this issue, we could probably give a proper
> initial state when creating the store since Redux supports it. We would then
> stop having to "click" on the filters when mounting the Toolbar component
> (remove the componentDidMount function).
> 
> If you think this is the way to go, I could give it a go.
Yes, this sounds good to me!

Honza

Comment 7

[Adrien Enault]

Attachment: netmonFiltersNotSaved.patch

Hello,

The state is now initialized from the preferences at the store creation, which fixes the issue. I have updated the test to make it pass, as it had some side effects on when the Prefs is saved, but the bug itself is still not tested.

Best regards,

Adrien.

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8840048 [details] [diff] [review]
netmonFiltersNotSaved.patch

Review of attachment 8840048 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

R+ assuming try is green, and please resolve one nit inline comment.

Thanks for the patch!

Honza

::: devtools/client/netmonitor/store.js
@@ +23,5 @@
> +  });
> +  const initialState = {
> +    filters: new Filters({
> +        requestFilterTypes: new FilterTypes(activeFilters)
> +      }),

nit: fix indentation

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Just clearing NI flag.

Honza

Comment 10

[Adrien Enault]

Attachment: netmonFiltersNotSaved2.patch

Hello,
nit fixed, if you want to land it
best regards,
Adrien

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks Adrien!

Honza

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd395be35318
Fix loss of filters in netmon when clicking on back from the perf tabs. r=Honza

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for eslint failures:

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=212db4739778f1e58dfba06c660e7b1acd399750&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=79740615&repo=mozilla-inbound

TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/netmonitor/components/toolbar.js:17:9 | 'Prefs' is assigned a value but never used. (no-unused-vars)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/netmonitor/reducers/requests.js:251:2 | Missing semicolon. (semi)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/netmonitor/reducers/sort.js:36:2 | Missing semicolon. (semi)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/netmonitor/reducers/timing-markers.js:57:2 | Missing semicolon. (semi)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/netmonitor/reducers/ui.js:70:2 | Missing semicolon. (semi)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/netmonitor/store.js:32:4 | Missing semicolon. (semi)

Please fix the issues and submit an updated patch. You can run the lint command by typing |mach lint| in the console. It supports files as arguments (likely also paths).

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8cbc9a26039
Backed out changeset cd395be35318 for eslint failures. r=backout

Comment 15

[Adrien Enault]

Attachment: netmonFiltersNotSaved3.patch

fixed lint.
br,
Adrien

Comment 16

[Adrien Enault]

Attachment: netmonFiltersNotSaved4.patch

...and cleaned some useless lines in the modified test

Comment 17

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8840605 [details] [diff] [review]
netmonFiltersNotSaved4.patch

Review of attachment 8840605 [details] [diff] [review]:
-----------------------------------------------------------------

R+ assuming Try is green.

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f512b7246b4298a9e71c96f32d9499699c69ce89

(let's wait for the results and set checkin-needed if it's green)

Honza

Comment 18

[Adrien Enault]

Comment on attachment 8840605 [details] [diff] [review]
netmonFiltersNotSaved4.patch

Hello,
there is one failure on the try, but it does not seem to be related to the fix.
Best regards,
Adrien

Comment 19

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Adrien: 'checking-needed' keyword need to be used if you want to get a patch landed in the tree.

Honza

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30c1e64495a9
Fix loss of filters in netmon when clicking on back from the perf tabs. r=Honza

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1bec4529e08
Followup to hopefully make eslint happy a=me

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Eslint was failing because it was enabled for devtools/client/netmonitor/test between when this had its try push and when it landed (bug 1326408). Hopefully that followup fixes it.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/30c1e64495a9
https://hg.mozilla.org/mozilla-central/rev/c1bec4529e08

Comment 24

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 54.0a1 (2017-02-21) (64-bit) on WIndows 7, 64 Bit!

This bug's fix is verified with latest Beta!

Build ID   : 20170420011827
User Agent : Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0

[bugday-20170419]

Comment 25

[Md.Tarikul Islam Oashi]

I have reproduced this bug with nightly 54.0a1 (2017-02-21) on "Linux Mint 18.1 Serena"(64 Bit).

The bug's fix is now verified on Release 54.0.

Build ID 	20170608175746
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0

[Bugday-20170614]

Comment 26

[Mihai Boldan, Desktop QA [:mboldan]]

Thanks  Mohammad Maruf Rahman and Md.Tarikul Islam Oashifor testing this.
I also confirm that the issue is no longer reproducible on Firefox 56.0a1 (2017-06-20), or on Firefox 55.0b3. 
Tests were performed under Mac OS X 10.11.6.