Closed Bug 971942 Opened 6 years ago Closed 6 years ago

Move some preferences from gfxPlatform to gfxPrefs (see comment 7 for the list)

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Version:
28 Branch
Platform:
x86
All
Type:
defect
Priority:
Not set

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla30

People

(Reporter: milan, Assigned: milan)

References

(Depends on 1 open bug)

Details

Attachments

(7 files, 15 obsolete files)

2. layout.frame_rate, layers.offmainthreadcomposition.frame-rate, layers.orientation.sync.timeout moved to gfxPrefs. Carry r=bgirard
16.79 KB, patch
milan
: review+
Details | Diff | Splinter Review
Part 3. layers.bufferrotation.enabled, layers.componentalpha.enabled, layers.scroll-graph, layers.acceleration.disabled, layers.acceleration.force-enabled moved to gfxPrefs. Carry r=clord
27.44 KB, patch
milan
: review+
Details | Diff | Splinter Review
Part 4. layers.frame-counter, layers.draw-borders, layers.draw-tile-borders, layers.draw-bigimage-borders, layers.dump, layers.frame-counter, ... moved to gfxPrefs. Carry r=nsilva
30.46 KB, patch
milan
: review+
Details | Diff | Splinter Review
Part 5. nglayout.debug.widget_update_flashing, canvas.azure.accelerated, layers.offmainthreadcomposition.enabled, ... moved to gfxPrefs. Carry r=bschouten
11.78 KB, patch
milan
: review+
Details | Diff | Splinter 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. Carry r=clord
17.87 KB, patch
milan
: review+
Details | Diff | Splinter Review
Part 7. gfx.canvas.skiagl.dynamic-cache, gfx.canvas.skiagl.cache-items, gfx.canvas.skiagl.cache-size moved to gfxPrefs. Carry r=jwillcox
2.63 KB, patch
milan
: review+
Details | Diff | Splinter Review
1. layers.async-video.enabled moved to gfxPrefs and only evaluated at startup. r=nsilva
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: nobody → milan
Depends on: 912794
Attachment #8376304 - Attachment description: layers.async-video.enabled moved to gfxPrefs. → Part 1. layers.async-video.enabled moved to gfxPrefs.
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
Summary: More preferences from gfxPlatform moving to gfxPrefs → Move some preferences from gfxPlatform to gfxPrefs (see comment 6 for the list)
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
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)
Attachment #8376304 - Attachment is obsolete: true
Attachment #8376503 - Flags: review?(nical.bugzilla)
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+
Attachment #8376507 - Flags: review?(bas) → review+
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.
Attachment #8376506 - Flags: review?(nical.bugzilla) → review+
(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.
Addressing issues raised in comment 16.
Attachment #8376503 - Attachment is obsolete: true
Attachment #8376503 - Flags: review?(nical.bugzilla)
Attachment #8377259 - Flags: review?(nical.bugzilla)
Attachment #8377259 - Flags: review?(nical.bugzilla) → review+
Attachment #8376509 - Flags: review?(snorp) → review+
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 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+
(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.
Sigh. This also needs bug 972099 which is blocked on a random orange.
Depends on: 972099
Leave out the part that sneaked in from another patch.
Attachment #8382478 - Attachment is obsolete: true
Attachment #8382579 - Flags: review+
You need to log in before you can comment on or make changes to this bug.