Open Bug 651981 Opened 9 years ago Updated 7 years ago

Tracking bug for GfxInfo API problems

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

REOPENED
mozilla11

People

(Reporter: bjacob, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: Leave open!)

Attachments

(2 files, 2 obsolete files)

Eventually we may want to fix the GfxInfo API.

Here's one problem: getters can throw, which is a pain for the JS programmer, see bug 642502 comment 13.
Yes, the getters should never throw.
As discussed over lunch:
 1) GfxInfo should only provide, well, info about gfx. The blacklisting decision process should be taken out of it, into a new class.
 2) maybe GfxInfo should just provide a "GfxInfo objet" from which the JS programmer could access properties, instead of being an interface with getters. The benefit of such an OO approach becomes clearer in conjunction with 1) : one would first obtain a GfxInfo object, then pass it to the blacklisting decision function.
From bug 642502 comment 22:

> >+#ifdef XP_WIN
> >+    // If ANGLE is not available but OpenGL is, we want to report on the OpenGL feature, because that's what's going to get used.
> >+    // In all other cases we want to report on the ANGLE feature.
> >+    var webglfeature = gfxInfo.FEATURE_WEBGL_ANGLE;
> >+    if (gfxInfo.getFeatureStatus(gfxInfo.FEATURE_WEBGL_ANGLE)  != gfxInfo.FEATURE_NO_INFO &&
> >+        gfxInfo.getFeatureStatus(gfxInfo.FEATURE_WEBGL_OPENGL) == gfxInfo.FEATURE_NO_INFO)
> >+      webglfeature = gfxInfo.FEATURE_WEBGL_OPENGL;
> >+#else
> >+    var webglfeature = gfxInfo.FEATURE_WEBGL_OPENGL;
> >+#endif
> 
> A slightly more general comment: it seems fragile to have this logic here,
> because it needs to be manually kept in sync with the back-end logic that
> actually decides what gets used when. Having about:support do nothing but just
> unconditionally call GfxInfo APIs and print their return values directly seems
> like the ideal state that we want to strive for. Maybe that should be the
> overarching goal for bug 651981?

Yes, this is exactly where we want to go.
Also, bug 642502 comment 27: for features that are not applicable to the host OS, we need to decide on a standard behavior for getFeatureStatus(). I suggest adding a new kind of status code, like FEATURE_NOT_PRESENT_ON_THIS_OS. Currently it seems that we just return NO_INFO and that could vary between GfxInfo OS-specific implementations.
Blocks: 668008
Attached patch Here's a more shippable version (obsolete) — Splinter Review
Attachment #556030 - Attachment is obsolete: true
Attachment #567156 - Flags: review?(bjacob)
Comment on attachment 567156 [details] [diff] [review]
Here's a more shippable version

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

r=me except for the JSAPI stuff (InfoObject), need another reviewer.
Attachment #567156 - Flags: review?(jorendorff)
Attachment #567156 - Flags: review?(bjacob)
Attachment #567156 - Flags: review+
Attachment #567156 - Attachment is obsolete: true
Attachment #567156 - Flags: review?(jorendorff)
Attachment #571446 - Flags: review?(jorendorff)
Comment on attachment 571446 [details] [diff] [review]
Again, this time with error handling

Drive-by comments:

>--- a/widget/src/xpwidgets/GfxInfoBase.cpp
>+++ b/widget/src/xpwidgets/GfxInfoBase.cpp
>+nsresult GfxInfoBase::GetInfo(JSContext* aCx, jsval* aResult)
>+{
>+  if (obj.mOk) {
>+    *aResult = OBJECT_TO_JSVAL(obj.mObj);
>+    return NS_OK;
>+  } else {
>+    return NS_ERROR_FAILURE;
>+  }

Nit:

if (!obj.mOk) {
  return NS_ERROR_FAILURE;
}

*aResult = OBJECT_TO_JSVAL(obj.mObj);
return NS_OK;

Also, inconsistent indentation.

>+GfxInfoBase::RemoveCollector(GfxInfoCollectorBase* collector)
>+    for (unsigned int i = 0; i < sCollectors->Length(); i++) {

PRUint32?

>--- /dev/null
>+++ b/widget/src/xpwidgets/GfxInfoCollector.h
>@@ -0,0 +1,96 @@

License header!

>+#ifndef __mozilla_widget_GfxInfoCollector_h__
>+#define __mozilla_widget_GfxInfoCollector_h__

No leading or trailing underscores please.

>+class InfoObject
>+    InfoObject(JSContext *aCx) : mCx(aCx)
>+    {
>+        mObj = JS_NewObject(mCx, NULL, NULL, NULL);
>+        if (!mObj)
>+            mOk = JS_TRUE;

Not mOk = JS_FALSE?

>+    JSBool mOk;

Not a bool?

>+class GfxInfoCollectorBase
>+{
>+  public:
>+    GfxInfoCollectorBase();
>+    virtual void GetInfo(InfoObject &obj) = 0;
>+    ~GfxInfoCollectorBase();

virtual destructor? (Disclaimer: I don't know C++)
Fixes Ms2ger's comments.
Attachment #571446 - Attachment is obsolete: true
Attachment #571446 - Flags: review?(jorendorff)
Attachment #571463 - Flags: review?(jorendorff)
Comment on attachment 571446 [details] [diff] [review]
Again, this time with error handling

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

r=me for the JSAPI usage, but there are a few bugs that must be fixed for this code to work at all (see below).

I'm not a peer in widget/src, so I guess this might need another review.

::: widget/src/xpwidgets/GfxInfoBase.cpp
@@ +689,5 @@
> +InitCollectors()
> +{
> +    if (!sCollectors)
> +        sCollectors = new nsTArray<GfxInfoCollectorBase*>;
> +}

Nit: these indents are 4 spaces, but I think this code probably wants 2-space indents.

@@ +697,5 @@
> +  InitCollectors();
> +  InfoObject obj(aCx);
> +
> +  for (PRUint32 i = 0; i < sCollectors->Length(); i++) {
> +      (*sCollectors)[i]->GetInfo(obj);

Same nit on this line.

::: widget/src/xpwidgets/GfxInfoCollector.h
@@ +7,5 @@
> +namespace widget {
> +
> +
> +/* this is handy wrapper around JSAPI to make it more pleasant to use */
> +class InfoObject

Since this class keeps a pointer to a JSObject, which is subject to garbage collection, it is important that this class be instantiated only on the stack (which the GC scans, searching for such pointers). Making the constructor private, and declaring a private copy constructor that isn't defined anywhere, could help ensure that.

The error handling in this class is fine as long as mCx definitely won't be used for anything else for the lifetime of the InfoObject. That might be hard to guarantee, but it depends on what the GetInfo methods are like. If they're all as simple as your example, it's fine.

@@ +17,5 @@
> +    {
> +        mObj = JS_NewObject(mCx, NULL, NULL, NULL);
> +        if (!mObj)
> +            mOk = JS_TRUE;
> +    }

This constructor does not always initialize mOk. Also it seems like JS_TRUE is the wrong value here. Maybe you want to say:
  mOk = (mObj != NULL);

@@ +22,5 @@
> +
> +    void DefineProperty(const char *name, int value)
> +    {
> +        if (mOk)
> +            return;

if (!mOk), right?

You could write this instead:
    mOk = mOk && JS_DefineProperty( ... );

@@ +30,5 @@
> +
> +    void DefineProperty(const char *name, nsAString &value)
> +    {
> +        if (mOk)
> +            return;

if (!mOk), right?

@@ +35,5 @@
> +
> +        const nsPromiseFlatString &flat = PromiseFlatString(value);
> +        JSString *string = JS_NewUCStringCopyN(mCx, static_cast<const jschar*>(flat.get()), flat.Length());
> +        if (!string)
> +            mOk = JS_TRUE;

JS_FALSE.

@@ +38,5 @@
> +        if (!string)
> +            mOk = JS_TRUE;
> +
> +        if (!mOk)
> +            mOk = JS_DefineProperty(mCx, mObj, name, STRING_TO_JSVAL(string), NULL, NULL, JSPROP_ENUMERATE);

You could use && here too.

@@ +52,5 @@
> +
> +Here's an example usage:
> +
> +class Foo {
> +    Joe::Joe() : mInfoCollector(this, &Foo::GetAweseomeness) {}

Foo::Foo, not Joe::Joe, right?
Attachment #571446 - Attachment is obsolete: false
Comment on attachment 571463 [details] [diff] [review]
Again, this time with betterness

Carrying forward r+.
Attachment #571463 - Flags: review?(jorendorff) → review+
(In reply to Jason Orendorff [:jorendorff] from comment #11)

> > +class Foo {
> > +    Joe::Joe() : mInfoCollector(this, &Foo::GetAweseomeness) {}
> 
> Foo::Foo, not Joe::Joe, right?

Jeff has numerous copies of joe-sucks.cpp and joe-sucks.png on his various computers.
Comment on attachment 571463 [details] [diff] [review]
Again, this time with betterness

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

::: widget/src/xpwidgets/GfxInfoCollector.h
@@ +79,5 @@
> +  InfoObject(JSContext *aCx) : mCx(aCx)
> +  {
> +    mObj = JS_NewObject(mCx, NULL, NULL, NULL);
> +    if (!mObj)
> +      mOk = JS_TRUE;

mOk is uninitialized here if we don't hit this branch.

Also having mOk be true when there's an error seems a little backwards maybe?
Depends on: 702770
https://hg.mozilla.org/mozilla-central/rev/831f0b92c182

To save time/mistakes when merging, please could you set assignee + target milestone when landing on inbound (https://wiki.mozilla.org/Tree_Rules/Inbound#Please_do_the_following_after_pushing_to_inbound).

Thanks :-)
Assignee: nobody → jmuizelaar
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: Leave open!
Target Milestone: mozilla11 → ---
Assignee: jmuizelaar → nobody
This is a tracking bug for all present and future GfxInfo problems; I suppose that the first problem was that we shouldn't have attached any patches to it, instead create blocking bugs.
No longer blocks: 701948
Depends on: 701948
Target Milestone: --- → mozilla11
I'm not entirely convinced about the utility of indefinite-life metabugs whose only purpose is to draw dependent bugs. Should we just close this and track everything separately--for example, using the status whiteboard?
I think that meta bugs are useful as long as they have useful still open blockers. In this case, this allowed me to find back bug 701948 which is still worth doing if it's not done already.

status whiteboard? seems harder to agree on a whiteboard conventions etc. than the cost of keeping one bug around (though I understand that in the context of bugkill that cost seems higher).

maybe what we need is a refinement of bugkill query to omit tracking bugs? we can have a whiteboard/keyword to identify them.
You need to log in before you can comment on or make changes to this bug.