Closed
Bug 971942
Opened 11 years ago
Closed 11 years ago
Move some preferences from gfxPlatform to gfxPrefs (see comment 7 for the list)
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: milan, Assigned: milan)
References
(Depends on 1 open bug)
Details
Attachments
(7 files, 15 obsolete files)
16.79 KB,
patch
|
milan
:
review+
|
Details | Diff | Splinter Review |
27.44 KB,
patch
|
milan
:
review+
|
Details | Diff | Splinter Review |
30.46 KB,
patch
|
milan
:
review+
|
Details | Diff | Splinter Review |
11.78 KB,
patch
|
milan
:
review+
|
Details | Diff | Splinter Review |
17.87 KB,
patch
|
milan
:
review+
|
Details | Diff | Splinter Review |
2.63 KB,
patch
|
milan
:
review+
|
Details | Diff | Splinter Review |
4.92 KB,
patch
|
milan
:
review+
|
Details | Diff | Splinter Review |
Low hanging fruit for the preferences in gfxPlatform to be moved to gfxPrefs, and not worry about main thread issues.
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8376304 -
Attachment description: layers.async-video.enabled moved to gfxPrefs. → Part 1. layers.async-video.enabled moved to gfxPrefs.
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8376305 -
Attachment description: 2. layout.frame_rate, layers.offmainthreadcomposition.frame-rate, layers.orientation.sync.timeout moved to gfxPrefs → Part 2. layout.frame_rate, layers.offmainthreadcomposition.frame-rate, layers.orientation.sync.timeout moved to gfxPrefs
Assignee | ||
Comment 3•11 years ago
|
||
Assignee | ||
Comment 4•11 years ago
|
||
Assignee | ||
Comment 5•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Summary: More preferences from gfxPlatform moving to gfxPrefs → Move some preferences from gfxPlatform to gfxPrefs (see comment 6 for the list)
Assignee | ||
Comment 7•11 years ago
|
||
Here are all the preferences that will be handled in this bug:
canvas.azure.accelerated
gfx.canvas.skiagl.dynamic-cache
gfx.canvas.skiagl.cache-items
gfx.canvas.skiagl.cache-size
gfx.direct2d.disabled
gfx.direct2d.force-enabled
layers.acceleration.disabled
layers.acceleration.draw-fps
layers.acceleration.force-enabled
layers.async-video.enabled
layers.bufferrotation.enabled
layers.componentalpha.enabled
layers.draw-bigimage-borders
layers.draw-borders
layers.draw-tile-borders
layers.dump
layers.enable-tiles
layers.frame-counter
layout.frame_rate
layers.low-precision-buffer
layers.low-precision-resolution
layers.offmainthreadcomposition.enabled
layers.offmainthreadcomposition.force
layers.offmainthreadcomposition.frame-rate
layers.offmainthreadcomposition.testing.enabled
layers.orientation.sync.timeout
layers.prefer-d3d9
layers.prefer-opengl
layers.progressive-paint
layers.scroll-graph
nglayout.debug.widget_update_flashing
Assignee | ||
Updated•11 years ago
|
Summary: Move some preferences from gfxPlatform to gfxPrefs (see comment 6 for the list) → Move some preferences from gfxPlatform to gfxPrefs (see comment 7 for the list)
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #8376304 -
Attachment is obsolete: true
Attachment #8376503 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #8376305 -
Attachment is obsolete: true
Attachment #8376504 -
Flags: review?(bgirard)
Assignee | ||
Comment 10•11 years ago
|
||
Attachment #8376307 -
Attachment is obsolete: true
Attachment #8376505 -
Flags: review?(chrislord.net)
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #8376308 -
Attachment is obsolete: true
Attachment #8376506 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #8376309 -
Attachment is obsolete: true
Attachment #8376507 -
Flags: review?(bas)
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #8376508 -
Flags: review?(bas)
Assignee | ||
Comment 14•11 years ago
|
||
Attachment #8376509 -
Flags: review?(snorp)
Comment 15•11 years ago
|
||
Comment on attachment 8376504 [details] [diff] [review]
Part 2. layout.frame_rate, layers.offmainthreadcomposition.frame-rate, layers.orientation.sync.timeout moved to gfxPrefs.
Review of attachment 8376504 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/thebes/gfxPrefs.h
@@ +117,5 @@
> DECL_GFX_PREFS(Skip, "layers.async-video.enabled", AsyncVideoEnabled, bool, false);
> #else
> DECL_GFX_PREFS(Live, "layers.async-video.enabled", AsyncVideoEnabled, bool, false);
> #endif
> + DECL_GFX_PREFS(Once, "layers.offmainthreadcomposition.frame-rate", LayersCompositionFrameRate, int32_t,- 1);
nit: , -1
This preference should be fine to be Live. The if condition might race but otherwise should be fine. It's handy to turn on a FPS without restarting.
Attachment #8376504 -
Flags: review?(bgirard) → review+
Updated•11 years ago
|
Attachment #8376507 -
Flags: review?(bas) → review+
Comment 16•11 years ago
|
||
Comment on attachment 8376503 [details] [diff] [review]
Part 1. layers.async-video.enabled moved to gfxPrefs.
Review of attachment 8376503 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/thebes/gfxPrefs.h
@@ +114,5 @@
> +#ifdef XP_WIN
> + // On windows, ignore the preference value, forcing async video to false.
> + DECL_GFX_PREFS(Skip, "layers.async-video.enabled", AsyncVideoEnabled, bool, false);
> +#else
> + DECL_GFX_PREFS(Live, "layers.async-video.enabled", AsyncVideoEnabled, bool, false);
I think this should be "Once" rather than "Live" since we decide whether to use async-video or not once at start up.
Updated•11 years ago
|
Attachment #8376506 -
Flags: review?(nical.bugzilla) → review+
Assignee | ||
Comment 17•11 years ago
|
||
(In reply to Nicolas Silva [:nical] from comment #16)
> Comment on attachment 8376503 [details] [diff] [review]
> Part 1. layers.async-video.enabled moved to gfxPrefs.
>
> Review of attachment 8376503 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: gfx/thebes/gfxPrefs.h
> @@ +114,5 @@
> > +#ifdef XP_WIN
> > + // On windows, ignore the preference value, forcing async video to false.
> > + DECL_GFX_PREFS(Skip, "layers.async-video.enabled", AsyncVideoEnabled, bool, false);
> > +#else
> > + DECL_GFX_PREFS(Live, "layers.async-video.enabled", AsyncVideoEnabled, bool, false);
>
> I think this should be "Once" rather than "Live" since we decide whether to
> use async-video or not once at start up.
OK, that's true, once you examine all the callers of the original gfxPlatform::AsyncVideoEnabled() - both calls were only made during the initialization, since the result wasn't cached. I'll change this to Once - I wouldn't be surprised if having this preference changed from the default in the reference tests, for instance, would have caused the settings in two places to be out of sync.
Assignee | ||
Comment 18•11 years ago
|
||
Addressing issues raised in comment 16.
Attachment #8376503 -
Attachment is obsolete: true
Attachment #8376503 -
Flags: review?(nical.bugzilla)
Attachment #8377259 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 19•11 years ago
|
||
Addressing issues in comment 15.
Attachment #8376504 -
Attachment is obsolete: true
Attachment #8377262 -
Flags: review+
Updated•11 years ago
|
Attachment #8377259 -
Flags: review?(nical.bugzilla) → review+
Updated•11 years ago
|
Attachment #8376509 -
Flags: review?(snorp) → review+
Comment 20•11 years ago
|
||
Comment on attachment 8376505 [details] [diff] [review]
Part 3. layers.bufferrotation.enabled, layers.componentalpha.enabled, layers.scroll-graph, layers.acceleration.disabled, layers.acceleration.force-enabled moved to gfxPrefs.
Review of attachment 8376505 [details] [diff] [review]:
-----------------------------------------------------------------
Fine, with one nit below, but I have no strong opinion on it.
::: gfx/thebes/gfxPrefs.h
@@ +133,3 @@
> DECL_GFX_PREFS(Once, "layers.offmainthreadcomposition.frame-rate", LayersCompositionFrameRate, int32_t,- 1);
> DECL_GFX_PREFS(Live, "layers.orientation.sync.timeout", OrientationSyncMillis, uint32_t, (uint32_t)0);
> + DECL_GFX_PREFS(Once, "layers.scroll-graph", LayersScrollGraph, bool, false);
Should this be called LayersScrollGraphEnabled? The function name is quite odd without any suffix to denote what it actually means.
Attachment #8376505 -
Flags: review?(chrislord.net) → review+
Comment 21•11 years ago
|
||
Comment on attachment 8376508 [details] [diff] [review]
Part 6. layers.low-precision-resolution, layers.prefer-opengl, layers.prefer-d3d9, layers.enable-tiles, gfx.direct2d.disabled, gfx.direct2d.force-enabled, moved to gfxPrefs.
Review of attachment 8376508 [details] [diff] [review]:
-----------------------------------------------------------------
Adding my own review because of tiles bits. Looks good, with the one suggestion below.
::: gfx/thebes/gfxPrefs.h
@@ +137,5 @@
> DECL_GFX_PREFS(Live, "layers.draw-bigimage-borders", DrawBigImageBorders, bool, false);
> DECL_GFX_PREFS(Live, "layers.draw-borders", DrawLayerBorders, bool, false);
> DECL_GFX_PREFS(Live, "layers.draw-tile-borders", DrawTileBorders, bool, false);
> DECL_GFX_PREFS(Once, "layers.dump", LayersDump, bool, false);
> + DECL_GFX_PREFS(Once, "layers.enable-tiles", LayersEnableTiles, bool, false);
I think we should call this LayersTilesEnabled (and maybe rename the pref if we don't want the dissonance between pref name and function name) - otherwise it sounds like it's an action rather than a request.
Attachment #8376508 -
Flags: review+
Assignee | ||
Comment 22•11 years ago
|
||
Assignee | ||
Comment 23•11 years ago
|
||
(In reply to Chris Lord [:cwiiis] from comment #20)
> Comment on attachment 8376505 [details] [diff] [review]
> Part 3. layers.bufferrotation.enabled, layers.componentalpha.enabled,
> ...
> > + DECL_GFX_PREFS(Once, "layers.scroll-graph", LayersScrollGraph, bool, false);
>
> Should this be called LayersScrollGraphEnabled? The function name is quite
> odd without any suffix to denote what it actually means.
Yeah, I just matched what it was in the pref; pref probably could have been layers.scroll-graph.enabled in the first place. I may leave it as is now and get back to it.
While waiting for 910860 to land (no dependencies, but I built patches on top), things got bitrotted. I'll upload new patches and carry the reviews.
Assignee | ||
Comment 24•11 years ago
|
||
Attachment #8377259 -
Attachment is obsolete: true
Attachment #8382478 -
Flags: review+
Assignee | ||
Comment 25•11 years ago
|
||
Attachment #8377262 -
Attachment is obsolete: true
Attachment #8382479 -
Flags: review+
Assignee | ||
Comment 26•11 years ago
|
||
Attachment #8376505 -
Attachment is obsolete: true
Attachment #8382482 -
Flags: review+
Assignee | ||
Comment 27•11 years ago
|
||
Attachment #8376506 -
Attachment is obsolete: true
Attachment #8382484 -
Flags: review+
Assignee | ||
Comment 28•11 years ago
|
||
Attachment #8376507 -
Attachment is obsolete: true
Attachment #8382486 -
Flags: review+
Assignee | ||
Comment 29•11 years ago
|
||
Attachment #8376508 -
Attachment is obsolete: true
Attachment #8376508 -
Flags: review?(bas)
Attachment #8382489 -
Flags: review+
Assignee | ||
Comment 30•11 years ago
|
||
Attachment #8376509 -
Attachment is obsolete: true
Attachment #8382491 -
Flags: review+
Assignee | ||
Comment 31•11 years ago
|
||
Sigh. This also needs bug 972099 which is blocked on a random orange.
Depends on: 972099
Assignee | ||
Comment 32•11 years ago
|
||
Leave out the part that sneaked in from another patch.
Attachment #8382478 -
Attachment is obsolete: true
Attachment #8382579 -
Flags: review+
Assignee | ||
Comment 33•11 years ago
|
||
Comment 34•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0cc15020119e
https://hg.mozilla.org/mozilla-central/rev/0426e3a7ec19
https://hg.mozilla.org/mozilla-central/rev/bc21c9660789
https://hg.mozilla.org/mozilla-central/rev/569eced7a98c
https://hg.mozilla.org/mozilla-central/rev/ccdcf979cf4f
https://hg.mozilla.org/mozilla-central/rev/16fcd522a6be
https://hg.mozilla.org/mozilla-central/rev/042b223a1092
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Assignee | ||
Comment 35•11 years ago
|
||
Sorry, my bad, didn't put in the inbound links. So, backwards given the trunk links in comment 34, but just for completeness:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cc15020119e
https://hg.mozilla.org/integration/mozilla-inbound/rev/0426e3a7ec19
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc21c9660789
https://hg.mozilla.org/integration/mozilla-inbound/rev/569eced7a98c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccdcf979cf4f
https://hg.mozilla.org/integration/mozilla-inbound/rev/16fcd522a6be
https://hg.mozilla.org/integration/mozilla-inbound/rev/042b223a1092
You need to log in
before you can comment on or make changes to this bug.
Description
•