Closed Bug 1128165 Opened 7 years ago Closed 7 years ago

Expose Silk Preferences in about:config

Categories

(Firefox :: General, defect, P1)

x86_64
All
defect

Tracking

()

RESOLVED FIXED
Firefox 38
Tracking Status
firefox38 --- fixed

People

(Reporter: mchang, Assigned: mchang)

References

()

Details

Attachments

(1 file)

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
(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.
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)
(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 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+
(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)
Will land after we get vsync on windows to prevent someone from enabling on windows and crashing.
Depends on: 1127151
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)
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?
(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.
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...
(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.
https://hg.mozilla.org/mozilla-central/rev/b0867af35eb0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.