651981 - Tracking bug for GfxInfo API problems

Status: REOPENED

(Core :: Graphics, defect)

Description

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

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.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Yes, the getters should never throw.

Comment 2

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

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.

Comment 3

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

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.

Comment 4

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

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.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Attachment: Here's a sketch of what this could look like

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Attachment: Here's a more shippable version

Comment 7

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

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.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Attachment: Again, this time with error handling

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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++)

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Attachment: Again, this time with betterness

Fixes Ms2ger's comments.

Comment 11

[Jason Orendorff [: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?

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 571463 [details] [diff] [review]
Again, this time with betterness

Carrying forward r+.

Comment 13

[Joe Drew (not getting mail)]

(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 14

[Matt Woodrow (:mattwoodrow)]

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?

Comment 15

[Ed Morley [:emorley]]

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 :-)

Comment 16

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

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.

Comment 17

[Joe Drew (not getting mail)]

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?

Comment 18

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

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.