Last Comment Bug 766251 - Reimplement GfxInfo on Android to get GL strings in a Java thread
: Reimplement GfxInfo on Android to get GL strings in a Java thread
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla16
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
Mentors:
Depends on: 771774 845877
Blocks: 766816 763805
  Show dependency treegraph
 
Reported: 2012-06-19 12:20 PDT by Benoit Jacob [:bjacob] (mostly away)
Modified: 2013-02-27 09:10 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
almost-ready WIP patch (20.66 KB, patch)
2012-06-22 14:45 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
1: The Java and AndroidBridge parts (15.11 KB, patch)
2012-06-25 12:45 PDT, Benoit Jacob [:bjacob] (mostly away)
bugmail: review+
Details | Diff | Splinter Review
spreadsheet showing that there is no significant impact on startup times, and supporting the choice of where we spawn the GfxInfoThread (14.84 KB, application/vnd.oasis.opendocument.spreadsheet)
2012-06-25 13:02 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details
2: The GfxInfo part (14.33 KB, patch)
2012-06-25 13:04 PDT, Benoit Jacob [:bjacob] (mostly away)
jmuizelaar: review+
Details | Diff | Splinter Review
3: fatal-assert GL layers support on android in nsBaseWidget (857 bytes, patch)
2012-06-25 13:08 PDT, Benoit Jacob [:bjacob] (mostly away)
jmuizelaar: review+
Details | Diff | Splinter Review
4: clean up GetGfxDriverInfo on Android (1.56 KB, patch)
2012-06-25 13:09 PDT, Benoit Jacob [:bjacob] (mostly away)
jmuizelaar: review+
Details | Diff | Splinter Review
5: update Adreno WebGL blacklisting (2.20 KB, patch)
2012-06-25 13:15 PDT, Benoit Jacob [:bjacob] (mostly away)
jmuizelaar: review+
Details | Diff | Splinter Review
1: The Java and AndroidBridge parts (updated) (14.97 KB, patch)
2012-06-26 09:29 PDT, Benoit Jacob [:bjacob] (mostly away)
jmuizelaar: review+
Details | Diff | Splinter Review

Description Benoit Jacob [:bjacob] (mostly away) 2012-06-19 12:20:02 PDT
GfxInfo on Android has a problem: it only exposes device names, like "Samsung Nexus S", it doesn't expose GPU strings like "PowerVR" which are what we need for most Gfx purposes.

The only way to get that is to call glGetString on a GL context.

We will likely need this info before creating a GL context, for example to decide whether we prefer to use EGLImage or a share group. I'm not sure if bug 728524 comment 31 is still current, but even if it isn't, it's sort of reasonable to think that we may need to have GPU info before we create a context, and in particular the EGLImage vs. share group story is reasonable regardless of whether that comment is still current.

Anyway, the only reasonable alternative to creating a GL context in a java thread to get these strings, would be to get these strings from the GL context of the LayerManager, but that leads to a convoluted design.

Actually, doing this GL work in a Java thread on startup even seems to be an optimization as it seems to preload the GL libraries so that subsequent eglInitialize calls run faster. Here's a log of two consecutive runs (each doing EGL initialization, querying GL strings, and cleanup) showing the second run to avoid re-loading GL libraries and running faster (from start to when the GL context is ready, the first run takes 109 ms, the second one takes 55ms, so it's 2x faster):


06-19 15:15:48.904 I/GfxInfoThread( 5017): run #1
06-19 15:15:48.904 I/GfxInfoThread( 5017): after getEGL
06-19 15:15:48.904 D/libEGL  ( 5017): loaded /system/lib/egl/libGLES_android.so
06-19 15:15:48.904 I/GeckoViewsFactory( 5017): Creating custom Gecko view: TabsPanel
06-19 15:15:48.912 D/libEGL  ( 5017): loaded /vendor/lib/egl/libEGL_POWERVR_SGX540_120.so
06-19 15:15:48.916 D/libEGL  ( 5017): loaded /vendor/lib/egl/libGLESv1_CM_POWERVR_SGX540_120.so
06-19 15:15:48.916 D/libEGL  ( 5017): loaded /vendor/lib/egl/libGLESv2_POWERVR_SGX540_120.so
06-19 15:15:48.928 I/GfxInfoThread( 5017): after eglGetDisplay
06-19 15:15:48.931 I/GeckoViewsFactory( 5017): Creating custom Gecko view: TabsPanel$TabsListContainer
06-19 15:15:48.982 I/GfxInfoThread( 5017): after eglInitialize
06-19 15:15:48.990 I/GfxInfoThread( 5017): after eglChooseConfig #1 (queried number of configs)
06-19 15:15:48.990 I/GfxInfoThread( 5017): after eglChooseConfig #2 (queried configs)
06-19 15:15:49.002 I/GfxInfoThread( 5017): after eglCreateContext
06-19 15:15:49.013 I/GfxInfoThread( 5017): after eglCreatePbufferSurface
06-19 15:15:49.013 I/GfxInfoThread( 5017): after eglMakeCurrent
06-19 15:15:49.013 I/GfxInfoThread( 5017): after getGL
06-19 15:15:49.013 I/GfxInfoThread( 5017): after glGetString
06-19 15:15:49.013 I/GfxInfoThread( 5017): renderer: PowerVR SGX 540
06-19 15:15:49.025 I/GeckoViewsFactory( 5017): Creating custom Gecko view: FormAssistPopup
06-19 15:15:49.029 I/GeckoViewsFactory( 5017): Creating custom Gecko view: AboutHomeContent
06-19 15:15:49.068 D/dalvikvm( 5017): GC_EXTERNAL_ALLOC freed 297K, 48% free 3037K/5767K, external 1904K/2137K, paused 36ms
06-19 15:15:49.068 I/GfxInfoThread( 5017): after cleanup
06-19 15:15:49.068 I/GfxInfoThread( 5017): run #2
06-19 15:15:49.068 I/GfxInfoThread( 5017): after getEGL
06-19 15:15:49.068 I/GfxInfoThread( 5017): after eglGetDisplay
06-19 15:15:49.103 I/GfxInfoThread( 5017): after eglInitialize
06-19 15:15:49.107 I/GfxInfoThread( 5017): after eglChooseConfig #1 (queried number of configs)
06-19 15:15:49.107 I/GfxInfoThread( 5017): after eglChooseConfig #2 (queried configs)
06-19 15:15:49.115 I/GfxInfoThread( 5017): after eglCreateContext
06-19 15:15:49.115 I/GeckoViewsFactory( 5017): Creating custom Gecko view: FindInPageBar
06-19 15:15:49.119 I/GfxInfoThread( 5017): after eglCreatePbufferSurface
06-19 15:15:49.123 I/GfxInfoThread( 5017): after eglMakeCurrent
06-19 15:15:49.123 I/GfxInfoThread( 5017): after getGL
06-19 15:15:49.123 I/GfxInfoThread( 5017): after glGetString
06-19 15:15:49.123 I/GfxInfoThread( 5017): renderer: PowerVR SGX 540
06-19 15:15:49.123 I/dalvikvm( 5017): Could not find method android.view.ViewConfiguration.hasPermanentMenuKey, referenced from method org.mozilla.gecko.BrowserToolbar.from
06-19 15:15:49.123 W/dalvikvm( 5017): VFY: unable to resolve virtual method 779: Landroid/view/ViewConfiguration;.hasPermanentMenuKey ()Z
06-19 15:15:49.123 D/dalvikvm( 5017): VFY: replacing opcode 0x6e at 0x01a9
06-19 15:15:49.123 D/dalvikvm( 5017): VFY: dead code 0x01ac-01b0 in Lorg/mozilla/gecko/BrowserToolbar;.from (Landroid/widget/LinearLayout;)V
06-19 15:15:49.123 D/dalvikvm( 5017): VFY: dead code 0x01f1-01f3 in Lorg/mozilla/gecko/BrowserToolbar;.from (Landroid/widget/LinearLayout;)V
06-19 15:15:49.146 I/GfxInfoThread( 5017): after cleanup
Comment 1 Benoit Jacob [:bjacob] (mostly away) 2012-06-20 12:18:53 PDT
Got a better proof-of-concept now, using GLES 2.0 and leaving the EGL display connection initialized so that subsequent EGL initialization is fast. Here is a log of two consecutive runs, showing that the first one takes 180 ms and the second one takes only 45 ms. Also (not shown here) further down the log, when Gecko actually initializes EGL for Layers, it actually goes fast and doesn't load again the libraries.

06-20 14:54:45.166 I/GfxInfoThread( 7870): run #1
06-20 14:54:45.166 D/dalvikvm( 7870): No JNI_OnLoad found in /data/data/org.mozilla.fennec_bjacob/lib/libmozglue.so 0x40515c98, skipping init
06-20 14:54:45.166 W/GeckoApp( 7870): zerdatime 131577568 - onCreate
06-20 14:54:45.193 D/libEGL  ( 7870): loaded /system/lib/egl/libGLES_android.so
06-20 14:54:45.201 I/GeckoViewsFactory( 7870): Creating custom Gecko view: TabsPanel
06-20 14:54:45.213 D/libEGL  ( 7870): loaded /vendor/lib/egl/libEGL_POWERVR_SGX540_120.so
06-20 14:54:45.236 D/libEGL  ( 7870): loaded /vendor/lib/egl/libGLESv1_CM_POWERVR_SGX540_120.so
06-20 14:54:45.236 D/libEGL  ( 7870): loaded /vendor/lib/egl/libGLESv2_POWERVR_SGX540_120.so
06-20 14:54:45.244 I/GfxInfoThread( 7870): after eglGetDisplay
06-20 14:54:45.263 I/GeckoViewsFactory( 7870): Creating custom Gecko view: TabsPanel$TabsListContainer
06-20 14:54:45.322 I/GfxInfoThread( 7870): after eglInitialize
06-20 14:54:45.322 I/GfxInfoThread( 7870): after eglChooseConfig
06-20 14:54:45.326 I/GfxInfoThread( 7870): after eglCreateContext
06-20 14:54:45.334 I/GfxInfoThread( 7870): after eglCreatePbufferSurface
06-20 14:54:45.334 E/GeckoEvent( 7870): created GFX_INFO_DATA_READY event
06-20 14:54:45.338 I/GeckoAppShell( 7870): Sending GFX_INFO_DATA_READY event to Gecko, characters: VENDOR
06-20 14:54:45.338 I/GeckoAppShell( 7870): Imagination Technologies
06-20 14:54:45.338 I/GeckoAppShell( 7870): RENDERER
06-20 14:54:45.338 I/GeckoAppShell( 7870): PowerVR SGX 540
06-20 14:54:45.338 I/GeckoAppShell( 7870): VERSION
06-20 14:54:45.338 I/GeckoAppShell( 7870): OpenGL ES 2.0
06-20 14:54:45.338 I/GfxInfoThread( 7870): after sendEventToGecko
06-20 14:54:45.345 I/GfxInfoThread( 7870): after cleanup
06-20 14:54:45.345 I/GfxInfoThread( 7870): run #2
06-20 14:54:45.345 I/GfxInfoThread( 7870): after eglGetDisplay
06-20 14:54:45.345 I/GfxInfoThread( 7870): after eglInitialize
06-20 14:54:45.345 I/GfxInfoThread( 7870): after eglChooseConfig
06-20 14:54:45.349 I/GfxInfoThread( 7870): after eglCreateContext
06-20 14:54:45.353 I/GeckoViewsFactory( 7870): Creating custom Gecko view: FormAssistPopup
06-20 14:54:45.357 I/GeckoViewsFactory( 7870): Creating custom Gecko view: AboutHomeContent
06-20 14:54:45.385 D/dalvikvm( 7870): GC_EXTERNAL_ALLOC freed 296K, 48% free 3038K/5767K, external 1904K/2137K, paused 23ms
06-20 14:54:45.385 I/GfxInfoThread( 7870): after eglCreatePbufferSurface
06-20 14:54:45.385 E/GeckoEvent( 7870): created GFX_INFO_DATA_READY event
06-20 14:54:45.385 I/GeckoAppShell( 7870): Sending GFX_INFO_DATA_READY event to Gecko, characters: VENDOR
06-20 14:54:45.385 I/GeckoAppShell( 7870): Imagination Technologies
06-20 14:54:45.385 I/GeckoAppShell( 7870): RENDERER
06-20 14:54:45.385 I/GeckoAppShell( 7870): PowerVR SGX 540
06-20 14:54:45.385 I/GeckoAppShell( 7870): VERSION
06-20 14:54:45.385 I/GeckoAppShell( 7870): OpenGL ES 2.0
06-20 14:54:45.385 I/GfxInfoThread( 7870): after sendEventToGecko
06-20 14:54:45.392 I/GfxInfoThread( 7870): after cleanup
Comment 2 Benoit Jacob [:bjacob] (mostly away) 2012-06-22 14:45:21 PDT
Created attachment 635925 [details] [diff] [review]
almost-ready WIP patch

Works; needs some more comments/polish
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2012-06-25 12:45:16 PDT
Created attachment 636447 [details] [diff] [review]
1: The Java and AndroidBridge parts

This is the Java-side, including the binding. R=kats for java/bindings and r=jrmuizel for the EGL/GLES2 bits.
Comment 4 Benoit Jacob [:bjacob] (mostly away) 2012-06-25 13:02:09 PDT
Created attachment 636455 [details]
spreadsheet showing that there is no significant impact on startup times, and supporting the choice of where we spawn the GfxInfoThread
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2012-06-25 13:04:18 PDT
Created attachment 636458 [details] [diff] [review]
2: The GfxInfo part
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2012-06-25 13:08:04 PDT
Created attachment 636462 [details] [diff] [review]
3: fatal-assert GL layers support on android in nsBaseWidget

Currently, when OpenGL Layers can't be initialized, we crash at some point when displaying our first actual page (as opposed to the home page).

This makes us crash earlier and more consistently.
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2012-06-25 13:09:13 PDT
Created attachment 636463 [details] [diff] [review]
4: clean up GetGfxDriverInfo on Android

This was just hurting my eyes.
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2012-06-25 13:15:20 PDT
Created attachment 636467 [details] [diff] [review]
5: update Adreno WebGL blacklisting

Adreno WebGL blacklisting was done in a hackish way as we didn't have the information we needed from GfxInfo. Now we do, so this can be implemented in the same way as we have implemented blacklisting rules on X11 for instance.

Theoretically, the really recommended way is to add rules in GetGfxDriverInfo. But this requires defining a device family in GfxDriverInfo::GetDeviceFamily which lives in another directory (../xpwidgets). This lack of code locality seems like The Worst Thing to me. It's also sad that this large function full of OS-specific cases has to be compiled fully on all OSes. Moreover, it expects numeric device IDs (like PCI IDs) and what we have here is GL renderer strings. As a hack, we could hash the strings, but I rather think that we should 1) go for the present patch here and 2) really for real this time spend the time it takes to implement proper blacklisting.
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2012-06-25 13:16:33 PDT
Ignore the part of comment 8 that's saying how it requires IDs to be numeric: that was wrong. It already supports strings. The rest stands.
Comment 10 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-25 13:28:37 PDT
Comment on attachment 636447 [details] [diff] [review]
1: The Java and AndroidBridge parts

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

r+ with comments addressed. You definitely need to fix the XUL one before landing.

::: mobile/android/base/GeckoApp.java
@@ +93,5 @@
>      public static GeckoApp mAppContext;
>      public static boolean mDOMFullScreen = false;
>      protected MenuPanel mMenuPanel;
>      protected Menu mMenu;
> +    public static GfxInfoThread sGfxInfoThread = null;

Move this field to GeckoAppShell. It doesn't really matter much either way where it is but at least the code will be less spread around.

::: mobile/android/base/gfx/GfxInfoThread.java
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*
> + * Copyright (C) 2008 The Android Open Source Project

Is this copyright needed?

@@ +40,5 @@
> +    public GfxInfoThread() {
> +        mDataQueue = new SynchronousQueue<String>();
> +    }
> +
> +    public void error(String msg) {

Make this function private. Same for the eglError function.

@@ +182,5 @@
> +        // clean up after ourselves. This is especially important as some Android devices
> +        // have a very low limit on the global number of GL contexts.
> +        egl.eglMakeCurrent(eglDisplay, EGL10.EGL_NO_SURFACE, EGL10.EGL_NO_SURFACE, EGL10.EGL_NO_CONTEXT);
> +        egl.eglDestroySurface(eglDisplay, eglSurface);
> +        egl.eglDestroyContext(eglDisplay, eglContext);

Does it make more sense to do this cleanup before calling mDataQueue.put(data) above? It seems like you don't need this GL context once you have created the data string, and it would be better to free it as soon as possible.

::: widget/android/AndroidBridge.cpp
@@ +184,5 @@
>          jSurfacePointerField = jEnv->GetFieldID(jSurfaceClass, "mNativeSurface", "I");
>  
>      jNotifyWakeLockChanged = (jmethodID) jEnv->GetStaticMethodID(jGeckoAppShellClass, "notifyWakeLockChanged", "(Ljava/lang/String;Ljava/lang/String;)V");
>  
> +    jGetGfxInfoData = (jmethodID) jEnv->GetStaticMethodID(jGeckoAppShellClass, "getGfxInfoData", "()Ljava/lang/String;");

In order to not break the XUL fennec build, you'll need to add a stub function with this signature to embedding/android/GeckoAppShell.java as well. (Look for the notifyScreenShot function in embedding/android/GeckoAppShell.java for an example.

@@ +2334,5 @@
> +    if (jniFrame.CheckForException())
> +        return;
> +
> +    nsJNIString jniStr(jstrRet, env);
> +    CopyUTF16toUTF8(jniStr, aRet);

All the other functions in this file that do things like use "aRet.Assign(nsJNIString(jstrRet, env));" instead of this CopyUTF16ToUTF8 thing. Is there a reason you're doing it this way? I don't know if this is right or wrong but since it works I assume it's right.
Comment 11 Benoit Jacob [:bjacob] (mostly away) 2012-06-26 09:29:13 PDT
Created attachment 636737 [details] [diff] [review]
1: The Java and AndroidBridge parts (updated)

Thanks for the review. Updated patch coming. Replying inline below.

(In reply to Kartikaya Gupta (:kats) from comment #10)
> Comment on attachment 636447 [details] [diff] [review]
> 1: The Java and AndroidBridge parts
> 
> Review of attachment 636447 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with comments addressed. You definitely need to fix the XUL one before
> landing.
> 
> ::: mobile/android/base/GeckoApp.java
> @@ +93,5 @@
> >      public static GeckoApp mAppContext;
> >      public static boolean mDOMFullScreen = false;
> >      protected MenuPanel mMenuPanel;
> >      protected Menu mMenu;
> > +    public static GfxInfoThread sGfxInfoThread = null;
> 
> Move this field to GeckoAppShell. It doesn't really matter much either way
> where it is but at least the code will be less spread around.

Done.

> 
> ::: mobile/android/base/gfx/GfxInfoThread.java
> @@ +3,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +/*
> > + * Copyright (C) 2008 The Android Open Source Project
> 
> Is this copyright needed?

No, thanks for catching this. It was from the earlier version of my patch, that was based off the GLSurfaceView source code. I changed this to the point that the bits of original code that somewhat remain are trivial.

> 
> @@ +40,5 @@
> > +    public GfxInfoThread() {
> > +        mDataQueue = new SynchronousQueue<String>();
> > +    }
> > +
> > +    public void error(String msg) {
> 
> Make this function private. Same for the eglError function.

Done.

> 
> @@ +182,5 @@
> > +        // clean up after ourselves. This is especially important as some Android devices
> > +        // have a very low limit on the global number of GL contexts.
> > +        egl.eglMakeCurrent(eglDisplay, EGL10.EGL_NO_SURFACE, EGL10.EGL_NO_SURFACE, EGL10.EGL_NO_CONTEXT);
> > +        egl.eglDestroySurface(eglDisplay, eglSurface);
> > +        egl.eglDestroyContext(eglDisplay, eglContext);
> 
> Does it make more sense to do this cleanup before calling
> mDataQueue.put(data) above? It seems like you don't need this GL context
> once you have created the data string, and it would be better to free it as
> soon as possible.

Thanks, good catch, done. (This actually matters).

> 
> ::: widget/android/AndroidBridge.cpp
> @@ +184,5 @@
> >          jSurfacePointerField = jEnv->GetFieldID(jSurfaceClass, "mNativeSurface", "I");
> >  
> >      jNotifyWakeLockChanged = (jmethodID) jEnv->GetStaticMethodID(jGeckoAppShellClass, "notifyWakeLockChanged", "(Ljava/lang/String;Ljava/lang/String;)V");
> >  
> > +    jGetGfxInfoData = (jmethodID) jEnv->GetStaticMethodID(jGeckoAppShellClass, "getGfxInfoData", "()Ljava/lang/String;");
> 
> In order to not break the XUL fennec build, you'll need to add a stub
> function with this signature to embedding/android/GeckoAppShell.java as
> well. (Look for the notifyScreenShot function in
> embedding/android/GeckoAppShell.java for an example.

OK, thanks for the time-saving tip.

> 
> @@ +2334,5 @@
> > +    if (jniFrame.CheckForException())
> > +        return;
> > +
> > +    nsJNIString jniStr(jstrRet, env);
> > +    CopyUTF16toUTF8(jniStr, aRet);
> 
> All the other functions in this file that do things like use
> "aRet.Assign(nsJNIString(jstrRet, env));" instead of this CopyUTF16ToUTF8
> thing. Is there a reason you're doing it this way? I don't know if this is
> right or wrong but since it works I assume it's right.

Yes: these other methods have a aRet that is a nsAString which is UTF-16. My aRet is a nsACString which is UTF-8. So the model that I followed in this file was AndroidBridge::GetMimeTypeFromExtensions, which does:

    nsJNIString jniStr(jstrType, env);
    aMimeType.Assign(NS_ConvertUTF16toUTF8(jniStr.get()));

This however seems a more complicated way of doing the same thing as CopyUTF16toUTF8 does.
Comment 12 Benoit Jacob [:bjacob] (mostly away) 2012-06-26 09:31:56 PDT
https://tbpl.mozilla.org/?tree=Try&rev=082e098786cf
Comment 13 Benoit Jacob [:bjacob] (mostly away) 2012-06-26 14:14:01 PDT
Got the embedding/ part wrong. New try: https://tbpl.mozilla.org/?tree=Try&rev=c877e2cc59fc
Comment 14 Jeff Muizelaar [:jrmuizel] 2012-06-27 11:00:59 PDT
Comment on attachment 636737 [details] [diff] [review]
1: The Java and AndroidBridge parts (updated)

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

::: mobile/android/base/gfx/GfxInfoThread.java
@@ +48,5 @@
> +              "Please report a Mozilla bug.");
> +        try {
> +            data = mDataQueue.take();
> +        } catch (InterruptedException e) {
> +            Thread.currentThread().interrupt();

I think you can just have getData() throw InterruptedException then you don't need the try { } catch block.

@@ +168,5 @@
> +        // finally send the data. Notice that we've already freed the EGL resources, so that they don't
> +        // remain there until the data is read.
> +        try {
> +            mDataQueue.put(data);
> +        } catch (InterruptedException e) {

It's probably worth adding this comment:
// Restore the interrupted status because we can't throw through run()
Comment 15 Jeff Muizelaar [:jrmuizel] 2012-07-03 11:06:45 PDT
Comment on attachment 636458 [details] [diff] [review]
2: The GfxInfo part

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

::: widget/android/GfxInfo.cpp
@@ +24,5 @@
>  NS_IMPL_ISUPPORTS_INHERITED1(GfxInfo, GfxInfoBase, nsIGfxInfoDebug)
>  #endif
>  
> +GfxInfo::GfxInfo()
> +  : mInitializedFromGfxInfoData(false)

FromJavaData

@@ +93,5 @@
> +      } else if(!strcmp(line, "ERROR")) {
> +        stringToFill = &mError;
> +      }
> +    }
> +  }

I'd prefer that we parse each pair explicitly instead of implicitly because it
makes it more obvious what's going on. But it can make error handling more complicated so I understand why you did it implicitly. I can usually be appeased with comments.

@@ +273,5 @@
>  
>    /* Add an App Note for now so that we get the data immediately. These
>     * can go away after we store the above in the socorro db */
>    nsCAutoString note;
>    /* AppendPrintf only supports 32 character strings, mrghh. */

Please fix this comment.

::: widget/android/GfxInfo.h
@@ +10,5 @@
>  
>  #include "GfxInfoBase.h"
>  #include "GfxDriverInfo.h"
>  
> +#include "nsAutoPtr.h"

No thanks
Comment 16 Benoit Jacob [:bjacob] (mostly away) 2012-07-04 16:13:07 PDT
Part of this tryserver push: https://tbpl.mozilla.org/?tree=Try&rev=35d614a1c367

Jeff: I didn't apply the Java InterruptedException review comment because it occurred to me while doing it, that this was only propagating the problem to the caller. Maybe I'm missing something, but it seemed to me that the try{}catch would have to be somewhere up the call chain anyway.

Note You need to log in before you can comment on or make changes to this bug.