Created attachment 472301 [details] [diff] [review] Quiet. Shh.
Attachment #472301 - Flags: review?(bjacob)
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.
Attachment #472301 - Flags: review?(bjacob) → review+
Created attachment 472419 [details] [diff] [review] 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: "
Attachment #472419 - Flags: review?(bjacob)
Comment on attachment 472419 [details] [diff] [review] Follow-up errm... just asked myself for review :-P
Attachment #472419 - Flags: review?(bjacob) → review?(vladimir)
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.
Attachment #472419 - Flags: review?(vladimir) → review-
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)
Created attachment 473115 [details] [diff] [review] 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.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
I ended up changing a few things in the commit -- take a look and see if the followup is still needed?
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?
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.
I see, thanks. Still one thing: the "WebGL: " prefix?
You need to log in before you can comment on or make changes to this bug.