Last Comment Bug 694206 - Add Gonk (B2G) widget backend
: Add Gonk (B2G) widget backend
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget (show other bugs)
: Trunk
: All Gonk (Firefox OS)
: -- normal (vote)
: mozilla11
Assigned To: Michael Wu [:mwu]
:
Mentors:
Depends on: 700376 710041
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-12 17:15 PDT by Michael Wu [:mwu]
Modified: 2011-12-12 16:07 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Gonk backend (137.48 KB, patch)
2011-11-08 12:43 PST, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
Gonk backend, v2 (135.41 KB, patch)
2011-11-09 17:26 PST, Michael Wu [:mwu]
cjones.bugs: review+
Details | Diff | Splinter Review
Interdiff between v1 and v2 (27.15 KB, patch)
2011-11-09 17:27 PST, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
Interdiff between v2 and v3 (6.79 KB, patch)
2011-11-10 14:17 PST, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review

Description Michael Wu [:mwu] 2011-10-12 17:15:05 PDT

    
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-12 17:39:04 PDT
What I'd like to do here, if possible, is make a bare-bones widget backend based around a single "screen" GL context.  That will keep our system API footprint low, and could be really useful for future "embedders" (though I don't care too about them initially).  We would need separate code to manage input events.  For B2G, we don't need to implement IME in the widget layer.

The best way to do this might be to make a subclass of nsBaseWidget that was constructed with a GLContext*.  For input events, we could have some event mix-in or subclass GLWidget further.  We would want the GL widget base in xpwidget/, but we should probably make a gonk/ subdirectory for the input-processing code.
Comment 2 Michael Wu [:mwu] 2011-10-12 17:45:44 PDT
Yeah that's basically the plan.
Comment 3 Michael Wu [:mwu] 2011-11-08 12:43:36 PST
Created attachment 572958 [details] [diff] [review]
Gonk backend
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-08 22:32:50 PST
Comment on attachment 572958 [details] [diff] [review]
Gonk backend

I much prefer SnugglyPointer* style but I don't have the heart to ask
you to change it in here.

>diff --git a/configure.in b/configure.in

>+    dnl to trick the nss wrapper into doing the right thing
>+    ANDROID_VERSION=8
>+    AC_DEFINE_UNQUOTED(ANDROID_VERSION, 8)
>+    AC_SUBST(ANDROID_VERSION)

File a followup on removing this?

>diff --git a/gfx/thebes/GLContextProviderEGL.cpp b/gfx/thebes/GLContextProviderEGL.cpp

>+#ifdef ANDROID
>+#define printf_stderr(args...)  __android_log_print(ANDROID_LOG_INFO, "Gonk EGL" , ## args)

This is supposed to be pulled in by nsDebug.h.  Is that not working here?

>@@ -596,7 +615,7 @@ public:
>         fGetConfigs(mEGLDisplay, ec, nc, &nc);
> 
>         for (int i = 0; i < nc; ++i) {
>-            printf_stderr ("========= EGL Config %d ========\n");
>+            printf_stderr ("========= EGL Config %d ========\n", i);

Whups.

>diff --git a/js/src/jscntxt.cpp b/js/src/jscntxt.cpp
>--- a/js/src/jscntxt.cpp
>+++ b/js/src/jscntxt.cpp
>@@ -1554,7 +1554,7 @@ JSContext::purge()
> static bool
> ComputeIsJITBroken()
> {
>-#ifndef ANDROID
>+#if !defined(ANDROID) || defined(GONK)

The first person to try to port b2g to 2.1 on a Galaxy S is going to
hate you ;).

>diff --git a/memory/mozalloc/Makefile.in b/memory/mozalloc/Makefile.in
>--- a/memory/mozalloc/Makefile.in
>+++ b/memory/mozalloc/Makefile.in
>@@ -51,8 +51,13 @@ endif
> 
> MODULE		= mozalloc
> LIBRARY_NAME	= mozalloc
>+DIST_INSTALL 	= 1
>+
>+ifeq (gonk,$(MOZ_WIDGET_TOOLKIT))
>+FORCE_STATIC_LIB= 1
>+else
> FORCE_SHARED_LIB= 1
>-DIST_INSTALL 	= 1
>+endif

We discussed this change a bit but I'm still not 100% which dsos are
going to use which heaps.  Let's file a followup and sort that out.

>diff --git a/widget/public/Makefile.in b/widget/public/Makefile.in
>--- a/widget/public/Makefile.in
>+++ b/widget/public/Makefile.in
>@@ -89,7 +89,7 @@ ifeq ($(MOZ_WIDGET_TOOLKIT),os2)
> EXPORTS		+= nsIDragSessionOS2.h
> endif
> 
>-ifeq ($(MOZ_WIDGET_TOOLKIT),android)
>+ifneq (,$(filter android gonk,$(MOZ_WIDGET_TOOLKIT)))

Shouldn't this be |ifeq|?

>diff --git a/widget/src/gonk/nsAppShell.cpp b/widget/src/gonk/nsAppShell.cpp

>+#define LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "Gonk" , ## args)
>+
>+using namespace mozilla;
>+
>+bool gDrawRequest = false;
>+static nsAppShell *gAppShell = NULL;
>+static int epollfd = 0;
>+static int signalfds[2] = {0};
>+
>+class fdHandler;

Nit: FdHandler.

>+static nsTArray<fdHandler> gHandlers;
>+

Nontrivial static ctor.  We still care about startup speed on gonk
(but not as much).

>+// XXX we wouldn't have to do this if we had epoll_pwait

So three things.  We don't really need to use epoll here, unless it
simplifies something for you.  Second, even though bionic doesn't
export epoll_pwait (or pselect), it's a native linux syscall so you
can manually define a syscall wrapper and save the pain here.  Third,
signalfd() is cleaner and simpler anyway, so just use that.

>+static void
>+multitouchHandler(int fd, fdHandler *data)
>+{
>+static void
>+keyHandler(int fd, fdHandler *data)
>+{

My eyes kind of glazed over reading this.  rubber-stamp on this from
me since it works so far.  If jlebar understands this code now and you
want review on this, please ask him.

>+    nsresult rv = nsBaseAppShell::Init();
>+    NS_ENSURE_SUCCESS(rv, rv);
>+

Please don't use macros that affect control flow.

>+    ret = sigaction(SIGUSR2, &sig, NULL);
>+    NS_ENSURE_FALSE(ret, NS_ERROR_UNEXPECTED);
>+

signalfd() would be better.

>+bool
>+nsAppShell::ProcessNextNativeEvent(bool mayWait)
>+{

>+    if (mNativeCallbackRequest) {
>+        mNativeCallbackRequest = false;

Shouldn't this be an (atomic) counter?  Or point at which comment says
that it doesn't need to be.  The gtk2 backend uses a pipe for
signaling, which acts as a counter.

>+    if (gDrawRequest) {
>+        gDrawRequest = false;

It's ok that this isn't a counter because using a bool will serve as
request compression.  Please document.

>+void
>+nsAppShell::NotifyNativeEvent()
>+{
>+    kill(getpid(), SIGUSR2);

Need to use tkill() here.

>diff --git a/widget/src/gonk/nsLookAndFeel.cpp b/widget/src/gonk/nsLookAndFeel.cpp

>+    // XXX we'll want to use context.obtainStyledAttributes on the java side to
>+    // get all of these; see TextView.java for a good exmaple.
>+

Don't think so.

>diff --git a/widget/src/gonk/nsScreenManagerGonk.cpp b/widget/src/gonk/nsScreenManagerGonk.cpp

>+NS_IMETHODIMP
>+nsScreenGonk::GetRect(PRInt32 *outLeft,  PRInt32 *outTop,
>+                      PRInt32 *outWidth, PRInt32 *outHeight)
>+{
>+    // XXX
>+    *outLeft = 0;
>+    *outTop = 0;
>+
>+    *outWidth = 480;
>+    *outHeight = 800;
>+

File a followup plz.

>+NS_IMETHODIMP
>+nsScreenGonk::GetPixelDepth(PRInt32 *aPixelDepth)
>+{
>+    // XXX do we need to lie here about 16bpp?  Or
>+    // should we actually check and return the right thing?
>+    *aPixelDepth = 24;

We definitely want to check and return the right thing.  We want to
look good on the sgs2.  Most of the places that will want to switch
16/24bpp because of perf constraints are far away from here, and don't
check this value (but maybe should).  Followup plz.

>diff --git a/widget/src/gonk/nsWidgetFactory.cpp b/widget/src/gonk/nsWidgetFactory.cpp

>+#include "nsHTMLFormatConverter.h"
>+//#include "nsTransferable.h"
>+#include "nsLookAndFeel.h"
>+#include "nsAppShellSingleton.h"
>+#include "nsScreenManagerGonk.h"
>+//#include "nsFilePicker.h"
>+//#include "nsClipboard.h"
>+//#include "nsClipboardHelper.h"
>+//#include "nsIdleServiceGonk.h"
>+//#include "nsDragService.h"
>+//#include "nsSound.h"
>+//#include "nsBidiKeyboard.h"
>+//#include "nsNativeThemeGonk.h"

Any reason to leave this stuff commented out?  I would sort of prefer
it all get deleted.  It looks like most of this stuff will be
implemented at a higher level, either in gaia or the b2g chrome
package.

>diff --git a/widget/src/gonk/nsWindow.cpp b/widget/src/gonk/nsWindow.cpp

>+static nsRefPtr<mozilla::gl::GLContext> sGLContext;

Nit:

 using namespace mozilla::gl;
 using namespace mozilla::layers;

 nsRefPtr<GLContext> ...

>+nsWindow::nsWindow()
>+{
>+    if (!sGLContext) {
>+        mNativeWindow = new android::FramebufferNativeWindow();

Do you understand all the dependencies of FramebufferNativeWindow?  It
looks like it's a reasonably simple non-vendor-overrideable wrapper
around the framebuffer provided by gralloc.  I wouldn't be opposed to
a direct impl here on top of gralloc.  But, there's no hurry.

>+        sGLContext = mozilla::gl::GLContextProvider::CreateForWindow(this);

GLContextProvider

>+    static_cast<mozilla::layers::LayerManagerOGL*>(gWindowToRedraw->GetLayerManager(nsnull))->

LayerManagerOGL.

>+mozilla::layers::LayerManager *

LayerManager

>+    nsRefPtr<mozilla::layers::LayerManagerOGL> layerManager =
>+        new mozilla::layers::LayerManagerOGL(this);
>+

LayerManagerOGL

>+    if (layerManager && layerManager->Initialize(sGLContext))

layerManager is infallibly allocated.

>+        mLayerManager = layerManager;
>+

We're going to need to handle the no-GPU case here with
BasicLayerManager.  If there's a CPU fallback impl provided by libGL,
then we're ok for now, but oh man, we don't want to use that in the
long run.  A really simple implementation would be to mmap /dev/fb0,
create a gfxImageSurface from that, and use that as the draw target of
a BasicLayerManager.  Let's file a followup.

Alright, pretty happy with this.  Nice work!  I just want to see a
version with cleaned up nsAppShell.cpp, and the rest we can do in
followups.
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-08 22:56:59 PST
I'm really excited about this patch.  This feels like we're writing a real OS.  Also, the fact that it feels like about 5x-10x less goop than we needed in the android backend makes me also feel like we're on the right track.
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-09 00:43:13 PST
It'd be interesting to try out the native gonk backend on the emulator to see what happens.  We'll need that path to work soon for testing.
Comment 7 Michael Wu [:mwu] 2011-11-09 16:07:53 PST
(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> >diff --git a/configure.in b/configure.in
> 
> >+    dnl to trick the nss wrapper into doing the right thing
> >+    ANDROID_VERSION=8
> >+    AC_DEFINE_UNQUOTED(ANDROID_VERSION, 8)
> >+    AC_SUBST(ANDROID_VERSION)
> 
> File a followup on removing this?
> 
> >diff --git a/gfx/thebes/GLContextProviderEGL.cpp b/gfx/thebes/GLContextProviderEGL.cpp
> 
> >+#ifdef ANDROID
> >+#define printf_stderr(args...)  __android_log_print(ANDROID_LOG_INFO, "Gonk EGL" , ## args)
> 
> This is supposed to be pulled in by nsDebug.h.  Is that not working here?
> 

Just some debugging junk I left by mistake.

> >diff --git a/js/src/jscntxt.cpp b/js/src/jscntxt.cpp
> >--- a/js/src/jscntxt.cpp
> >+++ b/js/src/jscntxt.cpp
> >@@ -1554,7 +1554,7 @@ JSContext::purge()
> > static bool
> > ComputeIsJITBroken()
> > {
> >-#ifndef ANDROID
> >+#if !defined(ANDROID) || defined(GONK)
> 
> The first person to try to port b2g to 2.1 on a Galaxy S is going to
> hate you ;).
> 

FWIW, this is here because the stl string things we do in here crash.

> >diff --git a/memory/mozalloc/Makefile.in b/memory/mozalloc/Makefile.in
> >--- a/memory/mozalloc/Makefile.in
> >+++ b/memory/mozalloc/Makefile.in
> >@@ -51,8 +51,13 @@ endif
> > 
> > MODULE		= mozalloc
> > LIBRARY_NAME	= mozalloc
> >+DIST_INSTALL 	= 1
> >+
> >+ifeq (gonk,$(MOZ_WIDGET_TOOLKIT))
> >+FORCE_STATIC_LIB= 1
> >+else
> > FORCE_SHARED_LIB= 1
> >-DIST_INSTALL 	= 1
> >+endif
> 
> We discussed this change a bit but I'm still not 100% which dsos are
> going to use which heaps.  Let's file a followup and sort that out.
> 

The theory here is that everything links against mozutils, and mozutil's symbols take precedence over the system allocator symbols. We'll need to check, but either way, I don't think there should be more than one allocator being used.

> >diff --git a/widget/public/Makefile.in b/widget/public/Makefile.in
> >--- a/widget/public/Makefile.in
> >+++ b/widget/public/Makefile.in
> >@@ -89,7 +89,7 @@ ifeq ($(MOZ_WIDGET_TOOLKIT),os2)
> > EXPORTS		+= nsIDragSessionOS2.h
> > endif
> > 
> >-ifeq ($(MOZ_WIDGET_TOOLKIT),android)
> >+ifneq (,$(filter android gonk,$(MOZ_WIDGET_TOOLKIT)))
> 
> Shouldn't this be |ifeq|?
> 

Nope.

> >diff --git a/widget/src/gonk/nsAppShell.cpp b/widget/src/gonk/nsAppShell.cpp
> 
> >+#define LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "Gonk" , ## args)
> >+
> >+using namespace mozilla;
> >+
> >+bool gDrawRequest = false;
> >+static nsAppShell *gAppShell = NULL;
> >+static int epollfd = 0;
> >+static int signalfds[2] = {0};
> >+
> >+class fdHandler;
> 
> Nit: FdHandler.
> 

Fixed.

> >+static nsTArray<fdHandler> gHandlers;
> >+
> 
> Nontrivial static ctor.  We still care about startup speed on gonk
> (but not as much).
> 
> >+// XXX we wouldn't have to do this if we had epoll_pwait
> 
> So three things.  We don't really need to use epoll here, unless it
> simplifies something for you.  Second, even though bionic doesn't
> export epoll_pwait (or pselect), it's a native linux syscall so you
> can manually define a syscall wrapper and save the pain here.  Third,
> signalfd() is cleaner and simpler anyway, so just use that.
> 

I tried signalfd, which is even newer than epoll_pwait, and it was a pain in the ass. I don't want to do this now.

> >+    nsresult rv = nsBaseAppShell::Init();
> >+    NS_ENSURE_SUCCESS(rv, rv);
> >+
> 
> Please don't use macros that affect control flow.
> 

I'd like an alternative which spews on debug builds and takes no more than two lines. The usual alternative of

if (NS_FAILED(rv))
    return rv;

wastes enormous amounts of time when I have to dig in with a debugger to figure out which one of these silently failed.

> >+bool
> >+nsAppShell::ProcessNextNativeEvent(bool mayWait)
> >+{
> 
> >+    if (mNativeCallbackRequest) {
> >+        mNativeCallbackRequest = false;
> 
> Shouldn't this be an (atomic) counter?  Or point at which comment says
> that it doesn't need to be.  The gtk2 backend uses a pipe for
> signaling, which acts as a counter.
> 

No need. It doesn't matter if someone sets mNativeCallbackRequest after we check it but before we clear it because by this point, we've already committed to running the native callback. As long we clear mNativeCallbackRequest before running the native callback, there should be no situations where a native callback isn't called after one is requested. Also, these requests can be coalesced.

Though I admit, a comment would prevent someone from inserting unnecessary atomic operations here in order to "fix" it.

> >+void
> >+nsAppShell::NotifyNativeEvent()
> >+{
> >+    kill(getpid(), SIGUSR2);
> 
> Need to use tkill() here.
> 

Well, it works currently and bionic doesn't provide tkill for reasons I don't understand. I suspected I needed tkill when I tried signalfd though.

> >diff --git a/widget/src/gonk/nsLookAndFeel.cpp b/widget/src/gonk/nsLookAndFeel.cpp
> 
> >+    // XXX we'll want to use context.obtainStyledAttributes on the java side to
> >+    // get all of these; see TextView.java for a good exmaple.
> >+
> 
> Don't think so.
> 

Removed.

> >diff --git a/widget/src/gonk/nsScreenManagerGonk.cpp b/widget/src/gonk/nsScreenManagerGonk.cpp
> 
> >+NS_IMETHODIMP
> >+nsScreenGonk::GetRect(PRInt32 *outLeft,  PRInt32 *outTop,
> >+                      PRInt32 *outWidth, PRInt32 *outHeight)
> >+{
> >+    // XXX
> >+    *outLeft = 0;
> >+    *outTop = 0;
> >+
> >+    *outWidth = 480;
> >+    *outHeight = 800;
> >+
> 
> File a followup plz.
> 

Fixed.

> >+NS_IMETHODIMP
> >+nsScreenGonk::GetPixelDepth(PRInt32 *aPixelDepth)
> >+{
> >+    // XXX do we need to lie here about 16bpp?  Or
> >+    // should we actually check and return the right thing?
> >+    *aPixelDepth = 24;
> 
> We definitely want to check and return the right thing.  We want to
> look good on the sgs2.  Most of the places that will want to switch
> 16/24bpp because of perf constraints are far away from here, and don't
> check this value (but maybe should).  Followup plz.
> 

Will file.

> >diff --git a/widget/src/gonk/nsWidgetFactory.cpp b/widget/src/gonk/nsWidgetFactory.cpp
> 
> >+#include "nsHTMLFormatConverter.h"
> >+//#include "nsTransferable.h"
> >+#include "nsLookAndFeel.h"
> >+#include "nsAppShellSingleton.h"
> >+#include "nsScreenManagerGonk.h"
> >+//#include "nsFilePicker.h"
> >+//#include "nsClipboard.h"
> >+//#include "nsClipboardHelper.h"
> >+//#include "nsIdleServiceGonk.h"
> >+//#include "nsDragService.h"
> >+//#include "nsSound.h"
> >+//#include "nsBidiKeyboard.h"
> >+//#include "nsNativeThemeGonk.h"
> 
> Any reason to leave this stuff commented out?  I would sort of prefer
> it all get deleted.  It looks like most of this stuff will be
> implemented at a higher level, either in gaia or the b2g chrome
> package.
> 

Cleaned up.

> >diff --git a/widget/src/gonk/nsWindow.cpp b/widget/src/gonk/nsWindow.cpp
> 
> >+static nsRefPtr<mozilla::gl::GLContext> sGLContext;
> 
> Nit:
> 
>  using namespace mozilla::gl;
>  using namespace mozilla::layers;
> 
>  nsRefPtr<GLContext> ...
> 

Done.

> >+    if (layerManager && layerManager->Initialize(sGLContext))
> 
> layerManager is infallibly allocated.
> 

Fixed.

> >+        mLayerManager = layerManager;
> >+
> 
> We're going to need to handle the no-GPU case here with
> BasicLayerManager.  If there's a CPU fallback impl provided by libGL,
> then we're ok for now, but oh man, we don't want to use that in the
> long run.  A really simple implementation would be to mmap /dev/fb0,
> create a gfxImageSurface from that, and use that as the draw target of
> a BasicLayerManager.  Let's file a followup.
> 

Will file.

> Alright, pretty happy with this.  Nice work!  I just want to see a
> version with cleaned up nsAppShell.cpp, and the rest we can do in
> followups.
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-09 16:22:57 PST
(In reply to Michael Wu [:mwu] from comment #7)
> > >diff --git a/js/src/jscntxt.cpp b/js/src/jscntxt.cpp
> > >--- a/js/src/jscntxt.cpp
> > >+++ b/js/src/jscntxt.cpp
> > >@@ -1554,7 +1554,7 @@ JSContext::purge()
> > > static bool
> > > ComputeIsJITBroken()
> > > {
> > >-#ifndef ANDROID
> > >+#if !defined(ANDROID) || defined(GONK)
> > 
> > The first person to try to port b2g to 2.1 on a Galaxy S is going to
> > hate you ;).
> > 
> 
> FWIW, this is here because the stl string things we do in here crash.
> 

That's, er, bad.  File a bug?  What's crashing?

> > >diff --git a/widget/public/Makefile.in b/widget/public/Makefile.in
> > >--- a/widget/public/Makefile.in
> > >+++ b/widget/public/Makefile.in
> > >@@ -89,7 +89,7 @@ ifeq ($(MOZ_WIDGET_TOOLKIT),os2)
> > > EXPORTS		+= nsIDragSessionOS2.h
> > > endif
> > > 
> > >-ifeq ($(MOZ_WIDGET_TOOLKIT),android)
> > >+ifneq (,$(filter android gonk,$(MOZ_WIDGET_TOOLKIT)))
> > 
> > Shouldn't this be |ifeq|?
> > 
> 
> Nope.
> 

Currently when MOZ_WIDGET_TOOLKIT=android, this ifeq evaluates to
true.  With your change, when MOZ_WIDGET_TOOLKIT=android, then
$(filter android gonk,$(MOZ_WIDGET_TOOLKIT))) results in "", and the
ifneq (,) evaluates to false.  So you've changed the branch here.
Unless this was intentional (why?) or I missed something...

> > >+static nsTArray<fdHandler> gHandlers;
> > >+
> > 
> > Nontrivial static ctor.  We still care about startup speed on gonk
> > (but not as much).
> > 
> > >+// XXX we wouldn't have to do this if we had epoll_pwait
> > 
> > So three things.  We don't really need to use epoll here, unless it
> > simplifies something for you.  Second, even though bionic doesn't
> > export epoll_pwait (or pselect), it's a native linux syscall so you
> > can manually define a syscall wrapper and save the pain here.  Third,
> > signalfd() is cleaner and simpler anyway, so just use that.
> > 
> 
> I tried signalfd, which is even newer than epoll_pwait, and it was a pain in
> the ass. I don't want to do this now.
> 

Why was using it a pain?  BTW, I realized after I reviewed this that
you can't use epoll_pwait anyway because it's vulnerable to a race
condition.

There's actually a problem with signalfd() here too: if something else
temporarily signal(SIGUSR2), then we'll miss wakeups.  I care less
about that, but how about we just use the normal setup of a wakeup
pipe here.  It's basically the same code, and I don't see that using
signals makes anything simpler.

> > >+    nsresult rv = nsBaseAppShell::Init();
> > >+    NS_ENSURE_SUCCESS(rv, rv);
> > >+
> > 
> > Please don't use macros that affect control flow.
> > 
> 
> I'd like an alternative which spews on debug builds and takes no more than
> two lines. The usual alternative of
> 
> if (NS_FAILED(rv))
>     return rv;
> 
> wastes enormous amounts of time when I have to dig in with a debugger to
> figure out which one of these silently failed.
> 

There's a helper like NS_FAILED() that prints error info.  Use that.

> > >+bool
> > >+nsAppShell::ProcessNextNativeEvent(bool mayWait)
> > >+{
> > 
> > >+    if (mNativeCallbackRequest) {
> > >+        mNativeCallbackRequest = false;
> > 
> > Shouldn't this be an (atomic) counter?  Or point at which comment says
> > that it doesn't need to be.  The gtk2 backend uses a pipe for
> > signaling, which acts as a counter.
> > 
> 
> No need. It doesn't matter if someone sets mNativeCallbackRequest after we
> check it but before we clear it because by this point, we've already
> committed to running the native callback. As long we clear
> mNativeCallbackRequest before running the native callback, there should be
> no situations where a native callback isn't called after one is requested.
> Also, these requests can be coalesced.
> 

The problem is, if something requests a native callback 3 times, then
with at least the gtk2 backend, it will be invoked 3 times.  So here,
if you use this strategy, you have to guarantee that the client
processes *all* native callbacks when it's invoked, or makes a
provision for enqueuing the ones it doesn't process.  Or that has to
be part of the contract.  Can you point me at where that's documented?

If you use a pipe for Notify(), you can just match what gtk2 does and
distinguish these requests when reading the pipe.  Then we don't have
to have the deeper discussion.

> > >+void
> > >+nsAppShell::NotifyNativeEvent()
> > >+{
> > >+    kill(getpid(), SIGUSR2);
> > 
> > Need to use tkill() here.
> > 
> 
> Well, it works currently and bionic doesn't provide tkill for reasons I
> don't understand. I suspected I needed tkill when I tried signalfd though.
> 

Let's just use a wakeup pipe and not worry about this.
Comment 9 Michael Wu [:mwu] 2011-11-09 16:39:24 PST
(In reply to Chris Jones [:cjones] [:warhammer] from comment #8)
> (In reply to Michael Wu [:mwu] from comment #7)
> > > >diff --git a/js/src/jscntxt.cpp b/js/src/jscntxt.cpp
> > > >--- a/js/src/jscntxt.cpp
> > > >+++ b/js/src/jscntxt.cpp
> > > >@@ -1554,7 +1554,7 @@ JSContext::purge()
> > > > static bool
> > > > ComputeIsJITBroken()
> > > > {
> > > >-#ifndef ANDROID
> > > >+#if !defined(ANDROID) || defined(GONK)
> > > 
> > > The first person to try to port b2g to 2.1 on a Galaxy S is going to
> > > hate you ;).
> > > 
> > 
> > FWIW, this is here because the stl string things we do in here crash.
> > 
> 
> That's, er, bad.  File a bug?  What's crashing?
> 

Don't remember. Crashed last I tried though.

> > > >diff --git a/widget/public/Makefile.in b/widget/public/Makefile.in
> > > >--- a/widget/public/Makefile.in
> > > >+++ b/widget/public/Makefile.in
> > > >@@ -89,7 +89,7 @@ ifeq ($(MOZ_WIDGET_TOOLKIT),os2)
> > > > EXPORTS		+= nsIDragSessionOS2.h
> > > > endif
> > > > 
> > > >-ifeq ($(MOZ_WIDGET_TOOLKIT),android)
> > > >+ifneq (,$(filter android gonk,$(MOZ_WIDGET_TOOLKIT)))
> > > 
> > > Shouldn't this be |ifeq|?
> > > 
> > 
> > Nope.
> > 
> 
> Currently when MOZ_WIDGET_TOOLKIT=android, this ifeq evaluates to
> true.  With your change, when MOZ_WIDGET_TOOLKIT=android, then
> $(filter android gonk,$(MOZ_WIDGET_TOOLKIT))) results in "", and the
> ifneq (,) evaluates to false.  So you've changed the branch here.
> Unless this was intentional (why?) or I missed something...
> 

filter should give "android" or "gonk", not "".

$(filter pattern...,text)
    Returns all whitespace-separated words in text that do match any of the pattern words, removing any words that do not match. The patterns are written using ‘%’, just like the patterns used in the patsubst function above. 

> > > >+static nsTArray<fdHandler> gHandlers;
> > > >+
> > > 
> > > Nontrivial static ctor.  We still care about startup speed on gonk
> > > (but not as much).
> > > 
> > > >+// XXX we wouldn't have to do this if we had epoll_pwait
> > > 
> > > So three things.  We don't really need to use epoll here, unless it
> > > simplifies something for you.  Second, even though bionic doesn't
> > > export epoll_pwait (or pselect), it's a native linux syscall so you
> > > can manually define a syscall wrapper and save the pain here.  Third,
> > > signalfd() is cleaner and simpler anyway, so just use that.
> > > 
> > 
> > I tried signalfd, which is even newer than epoll_pwait, and it was a pain in
> > the ass. I don't want to do this now.
> > 
> 
> Why was using it a pain?  BTW, I realized after I reviewed this that
> you can't use epoll_pwait anyway because it's vulnerable to a race
> condition.
> 
> There's actually a problem with signalfd() here too: if something else
> temporarily signal(SIGUSR2), then we'll miss wakeups.  I care less
> about that, but how about we just use the normal setup of a wakeup
> pipe here.  It's basically the same code, and I don't see that using
> signals makes anything simpler.
> 

Ahh, you're right. Signals aren't necessary at all here.

> > > >+    nsresult rv = nsBaseAppShell::Init();
> > > >+    NS_ENSURE_SUCCESS(rv, rv);
> > > >+
> > > 
> > > Please don't use macros that affect control flow.
> > > 
> > 
> > I'd like an alternative which spews on debug builds and takes no more than
> > two lines. The usual alternative of
> > 
> > if (NS_FAILED(rv))
> >     return rv;
> > 
> > wastes enormous amounts of time when I have to dig in with a debugger to
> > figure out which one of these silently failed.
> > 
> 
> There's a helper like NS_FAILED() that prints error info.  Use that.
> 

Will do.

> > > >+bool
> > > >+nsAppShell::ProcessNextNativeEvent(bool mayWait)
> > > >+{
> > > 
> > > >+    if (mNativeCallbackRequest) {
> > > >+        mNativeCallbackRequest = false;
> > > 
> > > Shouldn't this be an (atomic) counter?  Or point at which comment says
> > > that it doesn't need to be.  The gtk2 backend uses a pipe for
> > > signaling, which acts as a counter.
> > > 
> > 
> > No need. It doesn't matter if someone sets mNativeCallbackRequest after we
> > check it but before we clear it because by this point, we've already
> > committed to running the native callback. As long we clear
> > mNativeCallbackRequest before running the native callback, there should be
> > no situations where a native callback isn't called after one is requested.
> > Also, these requests can be coalesced.
> > 
> 
> The problem is, if something requests a native callback 3 times, then
> with at least the gtk2 backend, it will be invoked 3 times.  So here,
> if you use this strategy, you have to guarantee that the client
> processes *all* native callbacks when it's invoked, or makes a
> provision for enqueuing the ones it doesn't process.  Or that has to
> be part of the contract.  Can you point me at where that's documented?
> 

See https://mxr.mozilla.org/mozilla-central/source/widget/src/xpwidgets/nsBaseAppShell.cpp#96 . NativeEventCallback always schedules a new one itself if it wants to process more events.
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-09 16:43:44 PST
(In reply to Michael Wu [:mwu] from comment #9)
> > Currently when MOZ_WIDGET_TOOLKIT=android, this ifeq evaluates to
> > true.  With your change, when MOZ_WIDGET_TOOLKIT=android, then
> > $(filter android gonk,$(MOZ_WIDGET_TOOLKIT))) results in "", and the
> > ifneq (,) evaluates to false.  So you've changed the branch here.
> > Unless this was intentional (why?) or I missed something...
> > 
> 
> filter should give "android" or "gonk", not "".
> 
> $(filter pattern...,text)
>     Returns all whitespace-separated words in text that do match any of the
> pattern words, removing any words that do not match. The patterns are
> written using ‘%’, just like the patterns used in the patsubst function
> above. 
> 

Right-o!  My fault.

> > > > >+bool
> > > > >+nsAppShell::ProcessNextNativeEvent(bool mayWait)
> > > > >+{
> > > > 
> > > > >+    if (mNativeCallbackRequest) {
> > > > >+        mNativeCallbackRequest = false;
> > > > 
> > > > Shouldn't this be an (atomic) counter?  Or point at which comment says
> > > > that it doesn't need to be.  The gtk2 backend uses a pipe for
> > > > signaling, which acts as a counter.
> > > > 
> > > 
> > > No need. It doesn't matter if someone sets mNativeCallbackRequest after we
> > > check it but before we clear it because by this point, we've already
> > > committed to running the native callback. As long we clear
> > > mNativeCallbackRequest before running the native callback, there should be
> > > no situations where a native callback isn't called after one is requested.
> > > Also, these requests can be coalesced.
> > > 
> > 
> > The problem is, if something requests a native callback 3 times, then
> > with at least the gtk2 backend, it will be invoked 3 times.  So here,
> > if you use this strategy, you have to guarantee that the client
> > processes *all* native callbacks when it's invoked, or makes a
> > provision for enqueuing the ones it doesn't process.  Or that has to
> > be part of the contract.  Can you point me at where that's documented?
> > 
> 
> See
> https://mxr.mozilla.org/mozilla-central/source/widget/src/xpwidgets/
> nsBaseAppShell.cpp#96 . NativeEventCallback always schedules a new one
> itself if it wants to process more events.

Alright.  Please document that here, or cross-ref.
Comment 11 Michael Wu [:mwu] 2011-11-09 17:26:54 PST
Created attachment 573385 [details] [diff] [review]
Gonk backend, v2

I can drop in the NS_FAILED/NS_ENSURE_SUCCESS replacement once it's found.
Comment 12 Michael Wu [:mwu] 2011-11-09 17:27:27 PST
Created attachment 573387 [details] [diff] [review]
Interdiff between v1 and v2

For reference
Comment 13 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-09 18:17:00 PST
Comment on attachment 573385 [details] [diff] [review]
Gonk backend, v2

Thanks for the interdiff.

> >+static nsTArray<fdHandler> gHandlers;
> >+
> 
> Nontrivial static ctor.  We still care about startup speed on gonk
> (but not as much).
> 

This wasn't fixed.  This is additionally going to generate "XPCOM
ctor/dtor called from static context" warnings.

>diff --git a/widget/src/gonk/nsAppShell.cpp b/widget/src/gonk/nsAppShell.cpp

>+static void
>+keyHandler(int fd, FdHandler *data)
>+{
>+    // The Linux kernel's input documentation (Documentation/input/input.txt)
>+    // says that we'll always read a multiple of sizeof(input_event) bytes here.

Do you mind moving or adding this to multitouchHandler()? I had just
written a note asking about this when I ran into this comment.

>+    input_event events[16];
>+    ssize_t bytesRead = read(fd, events, sizeof(events));
>+    if (bytesRead < 0) {
>+        LOG("Error reading in keyHandler");
>+        return;
>+    }
>+    MOZ_ASSERT(bytesRead % sizeof(input_event) == 0);
>+

Thanks for adding this.  Plz add to multitouchHandler() as well.

>+nsresult
>+nsAppShell::Init()
>+
>+    chdir("/dev/input");
>+
>+        int fd = open(entry->d_name, O_RDONLY);

snprintf() plz per discussion.

>diff --git a/widget/src/gonk/nsAppShell.h b/widget/src/gonk/nsAppShell.h

>+    bool mNativeCallbackRequest;

Please note that there's a known read/write race on this, but that
it's basically OK.

r=me with fixes above.
Comment 14 Michael Wu [:mwu] 2011-11-10 14:17:49 PST
Created attachment 573640 [details] [diff] [review]
Interdiff between v2 and v3

Remaining issues addressed.
Comment 15 Michael Wu [:mwu] 2011-11-10 16:21:56 PST
Undid one small unintentional change to fix Linux, but otherwise the commit is v3 of the patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/27a6377f1e17
Comment 16 Marco Bonardo [::mak] 2011-11-11 02:24:12 PST
https://hg.mozilla.org/mozilla-central/rev/27a6377f1e17

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