Closed Bug 1341278 Opened 7 years ago Closed 7 years ago

Cannot hold checked filter button when go back from performance analysis

Categories

(DevTools :: Netmonitor, defect, P3)

defect

Tracking

(firefox53 unaffected, firefox54 fixed, firefox56 verified)

VERIFIED FIXED
Firefox 54
Tracking Status
firefox53 --- unaffected
firefox54 --- fixed
firefox56 --- verified

People

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

Details

Attachments

(1 file, 4 obsolete files)

[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.
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
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)
Attached patch netmonNotSaved.patch (obsolete) — Splinter Review
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)
Attached patch netmonFiltersNotSaved.patch (obsolete) — Splinter Review
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
Attached patch netmonFiltersNotSaved2.patch (obsolete) — Splinter Review
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
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)
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8cbc9a26039
Backed out changeset cd395be35318 for eslint failures. r=backout
Attached patch netmonFiltersNotSaved3.patch (obsolete) — Splinter Review
fixed lint.
br,
Adrien
Attachment #8840485 - Attachment is obsolete: true
Flags: needinfo?(schwartzmorn+bugzilla)
...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+
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
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
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)
https://hg.mozilla.org/mozilla-central/rev/30c1e64495a9
https://hg.mozilla.org/mozilla-central/rev/c1bec4529e08
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
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
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: