Open Bug 617028 Opened 9 years ago Updated 6 years ago

Log gfx problems in about:support (Report Failure)

Categories

(Core :: Graphics, defect)

x86
All
defect
Not set

Tracking

()

People

(Reporter: jrmuizel, Assigned: bjacob)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

We should be able to see when things go wrong without needing to crash.
Assignee: nobody → bjacob
Blocks: 549784
There's a front end to this interface in LayerManagerD3D9::ReportFailure(const nsACString &aMsg, HRESULT aCode).
Attached patch Initial version of the C++ side (obsolete) — Splinter Review
Summary: Log gfx problems in about:support → Log gfx problems in about:support (Report Failure)
Attached patch The about:support side (obsolete) — Splinter Review
Attached patch C++ side v2 (obsolete) — Splinter Review
Attachment #508553 - Attachment is obsolete: true
Blocks: 624801
Attached patch about:support v2Splinter Review
Attachment #509209 - Attachment is obsolete: true
Attached patch C++ side v3Splinter Review
Attachment #509527 - Attachment is obsolete: true
Attachment #509526 - Flags: review?(matt.woodrow+bugzilla)
Attachment #509568 - Flags: review?(ehsan)
Comment on attachment 509569 [details] [diff] [review]
C++ side v3

Perhaps logFailure should be [noscript]...
Attachment #509569 - Flags: review?(joe)
Comment on attachment 509526 [details] [diff] [review]
Add example call to LogFailure when binding a buffer fails

"Mapping pixel buffer failed\n" maybe? It will be the fMapBuffer call that is failing.

It might also be useful to log the value of mPixelBufferSize since that seems like the most likely cause of failure for this.

Looks good though, who do we have to bribe to get approval for this bug?
Attachment #509526 - Flags: review?(matt.woodrow+bugzilla) → review+
Comment on attachment 509569 [details] [diff] [review]
C++ side v3


>diff --git a/gfx/thebes/gfxFailure.h b/gfx/thebes/gfxFailure.h

>@@ -0,0 +1,11 @@

add copyright header

>+#include "nsString.h"
>+#include "nsIGfxInfo.h"

add whitespace

>diff --git a/widget/public/nsIGfxInfo.idl b/widget/public/nsIGfxInfo.idl

>+  void getFailures(
>+               [optional] out unsigned long failureCount,
>+               [retval, array, size_is(failureCount)] out string failures);

>+  void logFailure(in ACString failure);

[noscript, noxpcom]

or prove that noxpcom ⇒ noscript

>diff --git a/widget/src/xpwidgets/GfxInfoBase.cpp b/widget/src/xpwidgets/GfxInfoBase.cpp

>-GfxInfoBase::GfxInfoBase()
>+GfxInfoBase::GfxInfoBase() : mFailureCount(0)

newline before :

>+/* void getFailures ([optional] out unsigned long failureCount, [array, size_is (failureCount), retval] out string failures); */
>+/* XPConnect method of returning arrays is very ugly. Would not recommend. Failable nsMemory::Alloc makes things worse */

s/Failable/Fallible/

>+NS_IMETHODIMP GfxInfoBase::GetFailures(PRUint32 *failureCount NS_OUTPARAM, char ***failures NS_OUTPARAM)
>+{
>+

remove newline

>diff --git a/widget/src/xpwidgets/GfxInfoBase.h b/widget/src/xpwidgets/GfxInfoBase.h

>@@ -67,16 +68,20 @@ public:
>   // using GfxInfoBase::GetFeatureStatus;
>   // using GfxInfoBase::GetFeatureSuggestedDriverVersion;
>   // using GfxInfoBase::GetWebGLParameter;
>   // to import the relevant methods into their namespace.
>   NS_SCRIPTABLE NS_IMETHOD GetFeatureStatus(PRInt32 aFeature, PRInt32 *_retval NS_OUTPARAM);
>   NS_SCRIPTABLE NS_IMETHOD GetFeatureSuggestedDriverVersion(PRInt32 aFeature, nsAString & _retval NS_OUTPARAM);
>   NS_SCRIPTABLE NS_IMETHOD GetWebGLParameter(const nsAString & aParam, nsAString & _retval NS_OUTPARAM);
> 
>+  NS_SCRIPTABLE NS_IMETHOD GetFailures(PRUint32 *failureCount NS_OUTPARAM, char ***failures NS_OUTPARAM);
>+
>+  NS_SCRIPTABLE NS_IMETHOD LogFailure(const nsACString &failure);
>+

No need for newlines between GetWebGLParameter and others.

Do we need to add "using GfxInfoBase::GetFailures" and "using GfxInfoBase::LogFailure" to all the subclasses?

>+  PRUint32 mFailureCount;
>+  nsCString mFailures[9]; // The choice of 9 is Ehsan's

Put the nsCString first for optimal packing

Blame Ehsan more by putting his email address in
Attachment #509569 - Flags: review?(joe) → review+
Comment on attachment 509568 [details] [diff] [review]
about:support v2

>+    let trGraphicsFailures = [];
>+    gfxInfo.getFailures().forEach(function (value) {
>+        trGraphicsFailures.push(createParentElement("tr", [
>+            createElement("td", value)
>+        ]));
>+    });

FWIW, this could be written as:

      let trGraphicsFailures = gfxInfo.getFailures().map(
        function(value)
          createParentElement("tr", [
            createElement("td", value)
          ])
      );

r=me on either your js-ey version or my pythonic version.
Attachment #509568 - Flags: review?(ehsan) → review+
> >+  PRUint32 mFailureCount;
> >+  nsCString mFailures[9]; // The choice of 9 is Ehsan's
> 
> Put the nsCString first for optimal packing

Why do you think the alignment will be > 32 bits?
This landed in the latest nightly as:
http://hg.mozilla.org/mozilla-central/rev/165b354e3ec9

One slightly minor cosmetic issue: 
If there are no failures shown, the additional table (<tbody id="graphics-failures-tbody">) shows empty, adding a double border to the bottom of the table above it (<tbody id="graphics-tbody">).
What's left to fix here?
You need to log in before you can comment on or make changes to this bug.