Last Comment Bug 779535 - defaultNoAlpha preference is not used in case if nsIPropertyBag argument is 0
: defaultNoAlpha preference is not used in case if nsIPropertyBag argument is 0
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: Trunk
: ARM Linux
: -- normal (vote)
: mozilla17
Assigned To: Oleg Romashin (:romaxa)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-01 09:19 PDT by Oleg Romashin (:romaxa)
Modified: 2012-08-02 06:23 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
New alpha set pref value on earlier return (1.32 KB, patch)
2012-08-01 09:21 PDT, Oleg Romashin (:romaxa)
vladimir: review-
Details | Diff | Splinter Review
Ctor default alpha value setup from preferences (2.51 KB, patch)
2012-08-01 10:52 PDT, Oleg Romashin (:romaxa)
vladimir: review-
Details | Diff | Splinter Review
Ctor default alpha value setup from preferences (3.35 KB, patch)
2012-08-01 11:48 PDT, Oleg Romashin (:romaxa)
vladimir: review+
Details | Diff | Splinter Review
Readability improvement (1000 bytes, patch)
2012-08-01 14:39 PDT, Oleg Romashin (:romaxa)
jgilbert: review+
Details | Diff | Splinter Review

Description Oleg Romashin (:romaxa) 2012-08-01 09:19:50 PDT
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) 2012-08-01 09:21:01 PDT
Created attachment 647983 [details] [diff] [review]
New alpha set pref value on earlier return
Comment 2 Vladimir Vukicevic [:vlad] [:vladv] 2012-08-01 10:03:41 PDT
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) 2012-08-01 10:52:51 PDT
Created attachment 648010 [details] [diff] [review]
Ctor default alpha value setup from preferences
Comment 4 Vladimir Vukicevic [:vlad] [:vladv] 2012-08-01 11:37:50 PDT
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) 2012-08-01 11:48:22 PDT
Created attachment 648028 [details] [diff] [review]
Ctor default alpha value setup from preferences

Oh, you're totally right... /me dummy head
Comment 6 Vladimir Vukicevic [:vlad] [:vladv] 2012-08-01 11:50:22 PDT
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 Jeff Gilbert [:jgilbert] 2012-08-01 13:58:59 PDT
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) 2012-08-01 14:39:57 PDT
Created attachment 648094 [details] [diff] [review]
Readability improvement
Comment 9 Jeff Gilbert [:jgilbert] 2012-08-01 15:14:44 PDT
Comment on attachment 648094 [details] [diff] [review]
Readability improvement

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

Excellent, thanks.
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-08-01 15:41:41 PDT
And a bustage fix for 0333ef695e71.
https://hg.mozilla.org/integration/mozilla-inbound/rev/975aa5357853
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-08-01 19:39:27 PDT
https://hg.mozilla.org/mozilla-central/rev/f0b216d9f512

Note You need to log in before you can comment on or make changes to this bug.