779535 - defaultNoAlpha preference is not used in case if nsIPropertyBag argument is 0

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Oleg Romashin (:romaxa)]

Noticed that no-alpha preference not applied correctly for simple webgl demos like 
http://learningwebgl.com/lessons/lesson05/index.html

Reason is that nsIPropertyBag *aOptions == 0 and we return earlier without setting alpha = 0 when webgl.default-no-alpha = true

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: New alpha set pref value on earlier return

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 647983 [details] [diff] [review]
New alpha set pref value on earlier return

pretty sure this needs to check mOptionsFrozen as well and make sure that no change to mOptions actually happens.  (e.g. if I call getContext() with { alpha: true; } and then call it again with null options -- we don't want to set mOptions.alpha = false unilaterally)

Comment 3

[Oleg Romashin (:romaxa)]

Attachment: Ctor default alpha value setup from preferences

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 648010 [details] [diff] [review]
Ctor default alpha value setup from preferences

Close -- except that when we declare newOpts, it'll get alpha = true no matter what, so we could have an incorrect mismatch still.

The Pref::GetBool should go in the WebGLContextOptions constructor and nowhere else; that should get the right behaviour across the board without any other code.  (You may need to move that constructor into the cpp file from the header.)

Comment 5

[Oleg Romashin (:romaxa)]

Attachment: Ctor default alpha value setup from preferences

Oh, you're totally right... /me dummy head

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 648028 [details] [diff] [review]
Ctor default alpha value setup from preferences

Sold, but:

>+{
>+    // Set default alpha state based on preference which could be enabled for mobile builds

Get rid of "which could be enabled for mobile builds" -- the pref is valid everywhere


>+    // newOpts.alpha would remain untouched if Property bug does not have value for that

Get rid of this comment

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 648028 [details] [diff] [review]
Ctor default alpha value setup from preferences

Review of attachment 648028 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WebGLContext.cpp
@@ +78,5 @@
> +      premultipliedAlpha(true), antialias(true),
> +      preserveDrawingBuffer(false)
> +{
> +    // Set default alpha state based on preference which could be enabled for mobile builds
> +    alpha = Preferences::GetBool("webgl.default-no-alpha", false) ? 0 : 1;

|alpha| is a bool, so don't use 0 and 1.

This is also more confusing than it needs to be. We should initialize alpha(true) above, then down here:
if (Preferences::GetBool("webgl.default-no-alpha", false))
  alpha = false;

This would better match the logic of:
Default is true.
If <our custom pref>, default to false.

Comment 8

[Oleg Romashin (:romaxa)]

Attachment: Readability improvement

Comment 9

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 648094 [details] [diff] [review]
Readability improvement

Review of attachment 648094 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent, thanks.

Comment 10

[Oleg Romashin (:romaxa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0b216d9f512
https://hg.mozilla.org/integration/mozilla-inbound/rev/0333ef695e71

Comment 11

[Ryan VanderMeulen [:RyanVM]]

And a bustage fix for 0333ef695e71.
https://hg.mozilla.org/integration/mozilla-inbound/rev/975aa5357853

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f0b216d9f512

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0333ef695e71
https://hg.mozilla.org/mozilla-central/rev/975aa5357853