Closed
Bug 1128165
Opened 10 years ago
Closed 10 years ago
Expose Silk Preferences in about:config
Categories
(Firefox :: General, defect, P1)
Tracking
()
RESOLVED
FIXED
Firefox 38
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: mchang, Assigned: mchang)
References
()
Details
Attachments
(1 file)
1.01 KB,
patch
|
Dolske
:
review+
|
Details | Diff | Splinter Review |
Right now the only way to enable Silk is to manually set the pref with edit-prefs.js on b2g or build locally and change it in gfxPrefs.h. Expose the silk prefs in about:config so it's easier for others to test. The three prefs are:
gfx.vsync.hw-vsync.enabled
gfx.vsync.compositor
gfx.vsync.refreshdriver
Comment 1•10 years ago
|
||
Developer menu as well?
Assignee | ||
Comment 2•10 years ago
|
||
(In reply to Milan Sreckovic [:milan] from comment #1)
> Developer menu as well?
I'm less concerned about b2g / developer menu because I don't want to make the preferences "live", which will require a b2g restart. Using the edit-prefs.sh script with b2g ensures everything is restarted, which I prefer. Desktop would require a local build to enable, which is why I'm ok putting it in about:config.
Assignee | ||
Comment 3•10 years ago
|
||
Expose the silk prefs in about:config on OSX + Windows because we don't plan to support linux. I don't want someone to randomly enable it on linux and have it crash without the ability to restart firefox since it would crash on boot.
Attachment #8558063 -
Flags: review?(bugmail.mozilla)
Comment 4•10 years ago
|
||
(In reply to Mason Chang [:mchang] from comment #3)
> Expose the silk prefs in about:config on OSX + Windows because we don't plan
> to support linux. I don't want someone to randomly enable it on linux and
> have it crash without the ability to restart firefox since it would crash on
> boot.
Uhh. It's not really good to have prefs that cause crashes (especially crash on boot/start). If these prefs are so problematic on Linux, there should probably be some #ifdef code in the code using these prefs to ignore them on Linux.
Comment 5•10 years ago
|
||
Comment on attachment 8558063 [details] [diff] [review]
Expose Silk prefs in about:config
Fine to expose these existing hidden prefs, but please file a followup to avoid crashing.
Attachment #8558063 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Justin Dolske [:Dolske] from comment #5)
> Comment on attachment 8558063 [details] [diff] [review]
> Expose Silk prefs in about:config
>
> Fine to expose these existing hidden prefs, but please file a followup to
> avoid crashing.
The current patch is ifdef to only expose these prefs on Windows/Mac. I just tested on linux and the preferences are not exposed in about:config, so we shouldn't be able to enable these prefs without a local build on linux and someone manually setting the pref in the code, so this shouldn't be a problem right?
Flags: needinfo?(dolske)
Assignee | ||
Comment 7•10 years ago
|
||
Will land after we get vsync on windows to prevent someone from enabling on windows and crashing.
Depends on: 1127151
Comment 8•10 years ago
|
||
Nothing prevents a user from manually adding prefs via about:config (right click, New submenu). And of course addons can set prefs as part of their default install or through their own UI.
The point is that having prefs that cause a crash when set is simply bad form.
Flags: needinfo?(dolske)
Comment 9•10 years ago
|
||
I misunderstood the intent; if all we need is for people to be able to change the preference on desktop, using about:config, we don't need this patch - they can already do that just by doing New (as Justin mentioned in comment 8). If this is for internal testing, that should be good enough. So, do we actually want this patch at this point? Also, there are three different prefs - are all 8 permutations supported?
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Milan Sreckovic [:milan] from comment #9)
> I misunderstood the intent; if all we need is for people to be able to
> change the preference on desktop, using about:config, we don't need this
> patch - they can already do that just by doing New (as Justin mentioned in
> comment 8). If this is for internal testing, that should be good enough.
> So, do we actually want this patch at this point? Also, there are three
> different prefs - are all 8 permutations supported?
I guess technically this is true. I still want the patch so that it's easier for others to test it. Also, when we enable it by default, which I would like to do 1 platform at at time, it will be in this file as well. I don't see a good argument to not have it.
All permutations are not supported, Hardware vsync must always be enabled to have either the compositor or refresh driver work. But having just the compositor + vsync or refresh driver + vsync are supported for now.
Comment 11•10 years ago
|
||
Good stuff. Let's make sure that setting the values to the unsupported preferences doesn't "crash" - let's just block those code paths, the same way that we would block the Linux code path so we'd never go down the path where we could crash...
Assignee | ||
Comment 12•10 years ago
|
||
Assignee | ||
Comment 13•10 years ago
|
||
(In reply to Milan Sreckovic [:milan] from comment #11)
> Good stuff. Let's make sure that setting the values to the unsupported
> preferences doesn't "crash" - let's just block those code paths, the same
> way that we would block the Linux code path so we'd never go down the path
> where we could crash...
Filed as a follow up bug 1133526.
Comment 14•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
You need to log in
before you can comment on or make changes to this bug.
Description
•