Cannot hold checked filter button when go back from performance analysis

VERIFIED FIXED in Firefox 54

Status

P3
normal
VERIFIED FIXED
2 years ago
8 months ago

People

(Reporter: magicp.jp, Assigned: schwartzmorn+bugzilla, NeedInfo)

Tracking

Trunk
Firefox 54

Firefox Tracking Flags

(firefox53 unaffected, firefox54 fixed, firefox55 verified, firefox56 verified)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

2 years ago
[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.
(Reporter)

Comment 1

2 years ago
Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=620f06960400e4d95da0ed5c9682edda3aa136ec&tochange=53698c71b19066259c1b033569ddf1cf64710421
Has Regression Range: --- → yes
status-firefox53: --- → unaffected
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
Has STR: --- → yes
Flags: needinfo?(schwartzmorn+bugzilla)
Priority: -- → P3
(Assignee)

Comment 3

2 years ago
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.
Flags: needinfo?(schwartzmorn+bugzilla) → needinfo?(odvarko)
(Assignee)

Comment 4

2 years ago
Created attachment 8839576 [details] [diff] [review]
netmonNotSaved.patch
(Assignee)

Comment 5

2 years ago
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.
(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
Flags: needinfo?(odvarko)
(Assignee)

Comment 7

2 years ago
Created attachment 8840048 [details] [diff] [review]
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.
Attachment #8839576 - Attachment is obsolete: true
Flags: needinfo?(odvarko)
Attachment #8840048 - Flags: review?(odvarko)
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
Attachment #8840048 - Flags: review?(odvarko) → review+
Just clearing NI flag.

Honza
Flags: needinfo?(odvarko)
Assignee: nobody → schwartzmorn+bugzilla
Status: NEW → ASSIGNED
(Assignee)

Comment 10

2 years ago
Created attachment 8840485 [details] [diff] [review]
netmonFiltersNotSaved2.patch

Hello,
nit fixed, if you want to land it
best regards,
Adrien
Attachment #8840048 - Attachment is obsolete: true
Flags: needinfo?(odvarko)
Attachment #8840485 - Flags: review+
Thanks Adrien!

Honza
Flags: needinfo?(odvarko)
Keywords: checkin-needed

Comment 12

2 years ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
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).
Flags: needinfo?(schwartzmorn+bugzilla)

Comment 14

2 years ago
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8cbc9a26039
Backed out changeset cd395be35318 for eslint failures. r=backout
(Assignee)

Comment 15

2 years ago
Created attachment 8840598 [details] [diff] [review]
netmonFiltersNotSaved3.patch

fixed lint.
br,
Adrien
Attachment #8840485 - Attachment is obsolete: true
Flags: needinfo?(schwartzmorn+bugzilla)
(Assignee)

Comment 16

2 years ago
Created attachment 8840605 [details] [diff] [review]
netmonFiltersNotSaved4.patch

...and cleaned some useless lines in the modified test
Attachment #8840598 - Attachment is obsolete: true
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
Attachment #8840605 - Flags: review+
(Assignee)

Comment 18

2 years ago
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
Attachment #8840605 - Flags: checkin+
@Adrien: 'checking-needed' keyword need to be used if you want to get a patch landed in the tree.

Honza
Keywords: checkin-needed

Comment 20

2 years ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 21

2 years ago
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1bec4529e08
Followup to hopefully make eslint happy a=me
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.
Flags: needinfo?(schwartzmorn+bugzilla)

Comment 23

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/30c1e64495a9
https://hg.mozilla.org/mozilla-central/rev/c1bec4529e08
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
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]
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]
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.
Status: RESOLVED → VERIFIED
status-firefox55: --- → verified
status-firefox56: --- → verified

Updated

8 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.