Closed
Bug 1384962
Opened 4 years ago
Closed 4 years ago
Loading Preferences page resets user set process count value if the value happen to be the default
Categories
(Firefox :: Preferences, defect, P1)
Tracking
()
VERIFIED
FIXED
Firefox 56
People
(Reporter: timdream, Assigned: timdream)
References
Details
(Whiteboard: [photon-preference][e10s-multi:?])
Attachments
(2 files)
59 bytes,
text/x-review-board-request
|
jaws
:
review+
|
Details |
12.08 KB,
patch
|
timdream
:
review+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1382649 +++ STR originally filed in bug 1382649 comment 33. 1. Set dom.ipc.processCount.web to 2 in Nightly 2. Go to Preferences, uncheck "use default performance" 3. Set the process count to 4 with the UI (4 is the default dom.ipc.processCount value in Nightly) 4. Reload the Perferences page Expected: 1. process count set to 4 Actual: 1. process count flip to "2 (default)" [Tracking Requested - why for this release]: Follow up fix to bug 1382649, which was incomplete.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•4 years ago
|
||
Unfortunately, I cannot verify my tests at home because of bug 1252976. Try run is scheduled on MozReview. I did manually verify the patch with STR above.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Comment 3•4 years ago
|
||
I think you linked to the wrong bug in comment #2? That bug is fixed.
Comment 4•4 years ago
|
||
mozreview-review |
Comment on attachment 8890903 [details] Bug 1384962 - Don't set process count for user on Preferences page load, https://reviewboard.mozilla.org/r/162110/#review167590 ::: commit-message-658cb:3 (Diff revision 1) > +Bug 1384962 - Don't set process count for user on Preferences page load, r=jaws > + > +This is a follow-up to incompete fix in bug 1382649. In that bug, I attempted to s/incompete/incomplete/ ::: browser/components/preferences/in-content-new/main.js:1117 (Diff revision 1) > accelerationPref.value != accelerationPref.defaultValue) { > defaultPerformancePref.value = false; > } > }, > > - updatePerformanceSettingsBox() { > + updatePerformanceSettingsBox(changed) { Please use a object as the argument to the function so we can have "named parameters". updatePerformanceSettingsBox({duringChangeEvent}) { ... } Then the calling code would be: this.updatePerformanceSettingsBox({duringChangeEvent: true});
Attachment #8890903 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 5•4 years ago
|
||
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3) > I think you linked to the wrong bug in comment #2? That bug is fixed. I was meant to link to bug 1384153 in comment 2, thanks for spotting that.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•4 years ago
|
||
Approval Request Comment [Feature/Bug causing the regression]: Incomplete fix in bug 1382649 [User impact if declined]: On Beta, user will see their process count revert to value set by e10s rollout if they set the process count value to 1. [Is this code covered by automated tests?]: Yes. [Has the fix been verified in Nightly?]: Not yet. [Needs manual test from QE? If yes, steps to reproduce]: See STR in comment 0 but set process count to 1 on step 3. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: No. [Why is the change risky/not risky?]: Localized. Only as risky as bug 1382649. [String changes made/needed]: None. Patch for beta strips the test case change in in-content-new since the feature is disabled on 55.
Attachment #8891168 -
Flags: review+
Attachment #8891168 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 9•4 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #0) Noted to verify this bug on Beta branch, in step 3 of the STR in comment 0, you would need to set the process count to 1.
Comment 10•4 years ago
|
||
Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9bba40accbf6 Don't set process count for user on Preferences page load, r=jaws
Backed out for frequently exceeding maximum run time for browser/components/preferences/in-content/tests/browser_performance.js: https://hg.mozilla.org/integration/autoland/rev/09a412e3cc3b1a1fd9a1325e161d36b91018ef24 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9bba40accbf61384715d963df18e05227676591f&filter-searchStr=533f3b0d33f8841996a16249ba630067c1ca4719 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=118816229&repo=autoland [task 2017-07-28T05:07:15.506613Z] 05:07:15 INFO - Entering test bound [task 2017-07-28T05:07:15.511240Z] 05:07:15 INFO - Buffered messages logged at 05:07:12 [task 2017-07-28T05:07:15.513203Z] 05:07:15 INFO - TEST-PASS | browser/components/preferences/in-content-new/tests/browser_performance.js | General pane was selected - [task 2017-07-28T05:07:15.515213Z] 05:07:15 INFO - TEST-PASS | browser/components/preferences/in-content-new/tests/browser_performance.js | performance settings section should be shown - [task 2017-07-28T05:07:15.519100Z] 05:07:15 INFO - TEST-PASS | browser/components/preferences/in-content-new/tests/browser_performance.js | pref value is false - [task 2017-07-28T05:07:15.525566Z] 05:07:15 INFO - TEST-PASS | browser/components/preferences/in-content-new/tests/browser_performance.js | checkbox should not be checked - [task 2017-07-28T05:07:15.527812Z] 05:07:15 INFO - Buffered messages logged at 05:07:13 [task 2017-07-28T05:07:15.530738Z] 05:07:15 INFO - Leaving test bound [task 2017-07-28T05:07:15.532618Z] 05:07:15 INFO - Buffered messages finished [task 2017-07-28T05:07:15.534833Z] 05:07:15 INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content-new/tests/browser_performance.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort. -
Flags: needinfo?(timdream)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•4 years ago
|
||
New test items moved to another file -- will ask for checked-in when Try confirms added tests works. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f085b2a8e26f01a53ec72d063095138f37f27449
Flags: needinfo?(timdream)
Assignee | ||
Updated•4 years ago
|
Keywords: checkin-needed
Comment 14•4 years ago
|
||
Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/a4d0803e7690 Don't set process count for user on Preferences page load, r=jaws
Keywords: checkin-needed
Updated•4 years ago
|
status-firefox55:
--- → unaffected
status-firefox56:
--- → affected
Comment 15•4 years ago
|
||
This bug is not reproducible on latest Beta 55.0b13.
Updated•4 years ago
|
Comment 16•4 years ago
|
||
Sorry, didn't mean to flip that. Tim, should we let this ride 56 given comment 15?
Flags: needinfo?(timdream)
Comment 17•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a4d0803e7690
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Assignee | ||
Comment 19•4 years ago
|
||
I am pretty sure it's reproducible... Did you check comment 9? The STR is slightly different, and it just a reduced steps from bug 1382649 comment 33. which you have reproduced.
Flags: needinfo?(timdream) → needinfo?(brindusa.tot)
Comment 20•4 years ago
|
||
Yes, you are right! With scenario from comment 9, it is reproducible on Beta 55.0b13. Sorry for the misunderstanding.
Flags: needinfo?(brindusa.tot)
Comment 21•4 years ago
|
||
Comment on attachment 8891168 [details] [diff] [review] beta.patch followup fix for e10s-multi pref ui, beta55+ should be in 55.0 rc1
Attachment #8891168 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 22•4 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/f93965089c0a
Flags: in-testsuite+
Comment 23•4 years ago
|
||
Verified as fixed with latest Nightly 56.0a1, build ID 20170730100307 on Windows 10, Ubuntu 16.04 and Mac OS X 10.12.
Updated•4 years ago
|
Comment 24•4 years ago
|
||
Verified as fixed with the Release Candidate build 55.0, Build ID 20170731093223, on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12.
Status: RESOLVED → VERIFIED
Updated•4 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•