593720 - make WebGL quieter, less angry

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

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

Attachment: Quiet.

Shh.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 472301 [details] [diff] [review]
Quiet.

It's a start, so why not. But 2 things to keep in mind:

1. When verbose is disabled, if there are messages that would have been printed in verbose mode, it would be good to at least display one message saying "There are WebGL error or warning messages in this page, but they have been hidden. To unhide WebGL messages, go to about:config and set webgl.verbose to true."

2. there also are potentially very verbose messages not going through this. I'm thinking about the LogMessage I put in WebGLTexture::NeedFakeBlack, and in the future the messages we could put when vertex attrib 0 is disabled, etc. So perhaps this is best handled once and for all in WebGLContext::LogMessage.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Follow-up

This follow-up patch needs to be applied on top of yours.
 - prints one message saying there are messages and explaining how to see them.
 - put that in LogMessage() itself so everything goes through it
 - prefix messsages by "WebGL: "

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 472419 [details] [diff] [review]
Follow-up

errm... just asked myself for review :-P

Comment 4

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

Comment on attachment 472419 [details] [diff] [review]
Follow-up

No need; let's not complicate the code.  I don't care about these error messages, either you get all of them or you get none -- a developer will know about the flag, a user won't care.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

ok so I understand you mean "no need" for the change printing the message saying that there are messages.

do you agree with the other changes? (Putting it in LogMessage and adding the WebGL prefix)

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Follow-up, updated

Hope this one is OK. Keeping mVerbose as a plain bool and not printing anything when verbose=false. Another change is that when console==null we still print to stderr.

Comment 7

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

http://hg.mozilla.org/mozilla-central/rev/3a70f1b0e894

Comment 8

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

I ended up changing a few things in the commit -- take a look and see if the followup is still needed?

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

First I have a couple of questions about this changeset:
 - Since you're adding LogMessageIfVerbose(), why also add LogMessage(bool display, ...) ? Do you have a use case where you would pass to it something else than mVerbose ?
 - Why not at least let these two functions share the same code? (Implement LogMessageIfVerbose as calling LogMessage(mVerbose,...) ?
 - If you don't want LogMessage itself to check for mVerbose, why not just let the user do:
     if (mVerbose) LogMessage(...);
I think that's just as good as
     LogMessage(mVerbose, ...);

Also, my followup patch was prepending all WebGL messages with "WebGL: ". How about doing this?

Comment 10

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

Problem is that LogMessage is static, and mVerbose is a member variable.  There are places where we use LogMessage in a static context -- such as from any of the other WebGL objects that don't have a direct pointer back to the WebGLContext object.  LogIfVerbose is a non-static helper, whereas LogMessage(bool, ...) is static.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

I see, thanks.

Still one thing: the "WebGL: " prefix?