The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla17

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: romaxa, Assigned: romaxa)

Tracking

Trunk
mozilla17
ARM
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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)

Comment 1

5 years ago
Created attachment 647983 [details] [diff] [review]
New alpha set pref value on earlier return
Assignee: nobody → romaxa
Status: NEW → ASSIGNED
Attachment #647983 - Flags: review?
(Assignee)

Updated

5 years ago
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-
(Assignee)

Comment 3

5 years ago
Created attachment 648010 [details] [diff] [review]
Ctor default alpha value setup from preferences
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-
(Assignee)

Comment 5

5 years ago
Created attachment 648028 [details] [diff] [review]
Ctor default alpha value setup from preferences

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.
(Assignee)

Comment 8

5 years ago
Created attachment 648094 [details] [diff] [review]
Readability improvement
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+
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0b216d9f512
https://hg.mozilla.org/integration/mozilla-inbound/rev/0333ef695e71
And a bustage fix for 0333ef695e71.
https://hg.mozilla.org/integration/mozilla-inbound/rev/975aa5357853
https://hg.mozilla.org/mozilla-central/rev/f0b216d9f512
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/0333ef695e71
https://hg.mozilla.org/mozilla-central/rev/975aa5357853
You need to log in before you can comment on or make changes to this bug.