Closed Bug 779535 Opened 8 years ago Closed 7 years ago

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

Categories

(Core :: Canvas: WebGL, defect)

ARM
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: romaxa, Assigned: romaxa)

Details

Attachments

(2 files, 2 obsolete files)

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
Assignee: nobody → romaxa
Status: NEW → ASSIGNED
Attachment #647983 - Flags: review?
Attachment #647983 - Flags: review? → review?(vladimir)
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)
Attachment #647983 - Flags: review?(vladimir) → review-
Attachment #647983 - Attachment is obsolete: true
Attachment #648010 - Flags: review?
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.)
Attachment #648010 - Flags: review? → review-
Oh, you're totally right... /me dummy head
Attachment #648010 - Attachment is obsolete: true
Attachment #648028 - Flags: review?(vladimir)
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
Attachment #648028 - Flags: review?(vladimir) → review+
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.
Attachment #648094 - Flags: review?(jgilbert)
Comment on attachment 648094 [details] [diff] [review]
Readability improvement

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

Excellent, thanks.
Attachment #648094 - Flags: review?(jgilbert) → review+
https://hg.mozilla.org/mozilla-central/rev/f0b216d9f512
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.