630007 - Back-end support for Flash on Android (minimal support NPAPI. Pre-Honeycomb inproccess support only)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement, P1)

Description

[Matt Sturgeon]

User-Agent:       Mozilla/5.0 (Windows NT 6.0; rv:2.0b11pre) Gecko/20110129 Firefox/4.0b11pre
Build Identifier: 

Web plugins (e.g. Flash) are not currently used by Firefox for Mobile (Fennec).
Flash is available on Android 2.2 and later (a large proportion of Android Smartphones). If it is available on the phone it is installed on, it should be made use of - enable/disable-able in the add-ons preferences like desktop plugins.

Reproducible: Always

Steps to Reproduce:
1. Go to a webpage that uses a plugin installed on the device
2. Attempt to view/use content
Actual Results:  
Plugin not used, and webpage not displayed as the designer intended as a relult

Expected Results:  
Plugin is used by Fennec to optimize User eXperience.

Comment 1

[Tony Chung [:tchung]]

making this a meta bug, unless there's already one tracking flash for android fennec support.

Comment 2

[Matt Sturgeon]

For The Record:
What is blocking flash(/other-plugins-in-future) support in Firefox-on-Android?
I know that currently only the default browser has made use of flash, is there no open API?

Comment 3

[Doug Turner (:dougt)]

2.0next per blassey.  I don't really care if we support or don't support flash.

Comment 4

[Thomas Arend [:tarend]]

flash = in top 3 user requests / complaints

Comment 5

[Matt Sturgeon]

only 3 users request flash support? really? have you seen the android reviews. I've only seen 3 that DON'T request/complain about flash. Many are giving as little as 1 star because of it.

I feel as soon as an API is discovered/released it should be made use of (of course with an end-user option to "disable plugins such as Flash")...

Comment 6

[Brian Crowder]

Mr. Sturgeon, you seem to have misread comment #4 as saying that we have only three requests for Flash.  It actually should be read to say that Flash is among our *Top 3* requests/complaints.  I for one take getting Flash in pretty seriously.  The big barrier here, as I understand it, is that we are having difficulty getting solid documentation from Google/Adobe regarding what this API is meant to look and behave like on Android.

Comment 7

[Matt Sturgeon]

Oh, thanks for the clarification Brian. I did suspect it was lack of API documentation... I certainly couldn't find any. One of the things Google boasts about Android is the ability for different apps to interact with one-and-other...

I would request someone important in Mozilla email Google (CC'ing Adobe), requesting API documentation, and post the reply here.

Comment 8

[Doug Turner (:dougt)]

to clarify my thoughts.  Comment #3 should have read:

I do not care if we support or don't support flash "in this release".  We are basically a few weeks from shipping and do not think we have the ability to provide a high quality browser experience w/ flash and ship on time.  Fennec 1.x did not provide flash support, and at this point, I would consider adding flash to be a feature request.

I am sorry, it was a bit bitchy and trolly.  I should do better.

Comment 9

[Matt Sturgeon]

I assume you mean sipping Beta 5? Or are you referring to Final?

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #9)
> I assume you mean sipping Beta 5? Or are you referring to Final?

If we magically had a patch today, it would be at risk for final. Barring that magic I don't see how we can get flash support in for this final release.

Comment 11

[Doug Turner (:dougt)]

what brad said

Comment 12

[Mikael Magnusson]

I have built and loaded a plugin which shows up in about:plugins, but it doesn't get initialized. What parts are missing in Fennec for this to work? The following changeset mentions some parts which still needs to be implemented:

http://hg.mozilla.org/projects/electrolysis/rev/c62406f6a060

But are those needed for the plugin to get initialized at all? NP_Initialize is not called in my case.

Comment 16

[hmdqyg]

(In reply to comment #2)
> For The Record:
> What is blocking flash(/other-plugins-in-future) support in Firefox-on-Android?
> I know that currently only the default browser has made use of flash, is there
> no open API?

I don't think so bacause Opera Mobile 11 for Android can use flash plugin

Comment 17

[Matt Sturgeon]

Maybe someone from Mozilla can contact Google's Android Team directly about this. Or even Adobe.

Comment 18

[marc_ache]

For the record the Dolphin mini and Dolphin HD browsers on Android also support flash just fine.

Comment 19

[Josh Matthews [:jdm]]

Scott pointed me towards this source links in the stock browser. Not sure if anyone has gone digging in the source before.

http://android.git.kernel.org/?p=platform/external/webkit.git;a=tree;f=WebKit/android/plugins;h=dbba8af7508f22b2a26109f23d76504a7816016c;hb=refs/heads/honeycomb-release
http://android.git.kernel.org/?p=platform/frameworks/base.git;a=blob;f=core/java/android/webkit/PluginManager.java;h=e5eeb8cf59c5039e022a402172125f86fa7f8716;hb=HEAD
http://android.git.kernel.org/?p=platform/frameworks/base.git;a=tree;f=core/java/android/webkit;h=a5b9c890cd867e9114596f19b51e79b60926974a;hb=HEAD

Comment 20

[Makoto Kato [:m_kato]]

Although I am investigating this during last week, we need several changes.

- We needs change nsXREDirProvider to use JNI wrapper.

- We should add android_npapi.h, ANPSystem_npapi.h and etc to module/plugin/base/public

- NP_Initialize needs 3rd parameter that is JNIEnv.  

flash player uses it.  plugin-container doesn't run under JavaVM, so we create JavaVM on this process, or use IPC to connect chrome process.

Although I don't get success to load flash, I continue to investigate.

Comment 21

[Makoto Kato [:m_kato]]

This code works about:plugins, but NP_Initialize still returns error.

http://hg.mozilla.org/users/m_kato_ga2.so-net.ne.jp/arm/file/c2dd61157d5b/android-plugin

Comment 22

[Doug Turner (:dougt)]

I took brad's work and started on:

http://hg.mozilla.org/users/dougt_mozilla.com/wip/rev/0406bcf2da8f

This kinda renders the sample plugin okay (minor clipping, rect verses oval):

  http://dougt.org/plugin.html



Makoto+Blassy, we should sync up today.

Comment 23

[Doug Turner (:dougt)]

Blass*e*y

Comment 24

[Pete]

(In reply to comment #6)
> Mr. Sturgeon, you seem to have misread comment #4 as saying that we have
> only three requests for Flash.  It actually should be read to say that Flash
> is among our *Top 3* requests/complaints.  I for one take getting Flash in
> pretty seriously.  The big barrier here, as I understand it, is that we are
> having difficulty getting solid documentation from Google/Adobe regarding
> what this API is meant to look and behave like on Android.

It can't be documentation as the Dolphin browser has it working, Mozilla needs to get there act together on this as I was prepared to switch to Firefox but have gone back to Dolphin, until you get flash working.

Comment 25

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #24)
> It can't be documentation as the Dolphin browser has it working, Mozilla
> needs to get there act together on this as I was prepared to switch to
> Firefox but have gone back to Dolphin, until you get flash working.

We're working on it. But please not that Dolphin uses the same webkit as the stock browser, they get it for free.

Comment 26

[Andrew]

Thanks for your work on this.  Once Flash is working in FireFox, we'll have a definitive choice for best mobile browser IMO.

I'm excited to see progress on this.

Comment 27

[Fernando Hartmann]

I believe  that this implementation is one of the most important to Fennec in this days.
All we believe that open standards are the way, and HTML5 is our goal but Flash is very important ...
Why ?
Well, all we know there are a war between IOS and Android, and in IOS side there are a lot of good stuff in hardware and software world, and in Andriod side we have this too.
But it's improbable the we (Fennec) get authorization to run in IOS (Apple politics), then the mobile future for us is Android.
Despite the great value of Android (Open source, open market, etc..) for the end user they two tend to be equivalents, similar.
In this scenario one big difference is tha capability of Android to run ALL sites, even those that run Flash, and of course for the final user doesn't matters if it's Flash or Java or JavaScript !
Today the advertising of Motorola Xoom on TV say .... AND IT RUN FLASH !
Why this ? It's because people want to access all sites.
Today the default Android browser runs Flash, we (Fennec) not, I believe the we are losing a great opportunity of grow our market share on Android devices, because we don't run what a final user that bought a Android device (not an IOS) wants to run !
In the future when we start to run Flash, there would be time to recover the lost space in Android market share ?

Comment 28

[Thomas Arend [:tarend]]

Understood: Flash is a reality today, which is why we are working on it (see above). Android tablets use Flash aggressively in their advertising because it is one of the few differentiators to the iPad. In my opinion though, that does not suddenly make Flash any more or less important. Just saying.

Comment 29

[Paul [pwd]]

Has this stalled? It got P1 five months ago and including Firefox 5 we're looking at releasing at least two major versions without Flash which doesn't look good considering we're being touted as the only browser on Android to not support Flash:
http://www.brighthub.com/mobile/google-android/reviews/119627.aspx
http://www.betanews.com/article/Got-Android-Get-Firefox-5-beta-but-dont-expect-Flash/1306161743
http://www.downloadcrew.com/article/22701-firefox_beta
http://androidtipguys.com/2011/05/is-there-a-better-android-web-browser-than-the-google-one/

I understand this comment isn't the most helpful, but I'd rather we nipped reviews like this in the bud.

Comment 30

[Thomas Arend [:tarend]]

Paul, we are working on it. But it is going to take some time. The "other browsers" are either WebKit clones or proxy browsers. We are on it!

Comment 31

[Mikael Magnusson]

Opera Mobile supports Flash and it is not a WebKit clone. I don't think it's a proxy browser either, is it?

Comment 32

[Doug Turner (:dougt)]

Bugs are the place to discuss technical issues related to the topic.  Although, all of your points are interesting, they are not helping this work move forward.  Please take these conversations else where.

Comment 36

[ekerazha]

(In reply to Doug Turner (:dougt) from comment #32)
> Bugs are the place to discuss technical issues related to the topic. 
> Although, all of your points are interesting, they are not helping this work
> move forward.  Please take these conversations else where.

Where are these bugs? I don't see any "Depends on" bug, I see two "See Also" links, the first one is about opening YouTube videos using the YouTube app (who cares?), the second one is a "Fennec crashes trying to load test plugin" report, but there are no news from 6 months.

Comment 37

[Alex Elsayed]

(In reply to ekerazha from comment #36)
> (In reply to Doug Turner (:dougt) from comment #32)
> > Bugs are the place to discuss technical issues related to the topic. 
> > Although, all of your points are interesting, they are not helping this work
> > move forward.  Please take these conversations else where.
> 
> Where are these bugs? I don't see any "Depends on" bug, I see two "See Also"
> links, the first one is about opening YouTube videos using the YouTube app
> (who cares?), the second one is a "Fennec crashes trying to load test
> plugin" report, but there are no news from 6 months.

He meant that, as *this* is a bug, it is a place for *technical* discussion, and *not* the place for 'me too's, 'program X already does this', or other nontechnical comments. Technical discussion consists of comments that actually move the process of fixing it forward, like "line X in file Y is the source of the bug" or "this patch fixes it for me."

Comment 38

[Doug Turner (:dougt)]

Attachment: front-end patch

Comment 39

[Doug Turner (:dougt)]

Attachment: android plugins

Josh, please review the plugin bits.

Comment 40

[Doug Turner (:dougt)]

Comment on attachment 556872 [details] [diff] [review]
android plugins

chris, please review the android bits

Comment 41

[Doug Turner (:dougt)]

note:  this is froyo only.

Comment 42

[Chris Lord [:cwiiis]]

Comment on attachment 556872 [details] [diff] [review]
android plugins

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

r+ from me, comments aside (which are mostly nit-picking) - If we disable it by default, it might be good to land this to make it a bit easier to work on?

::: dom/plugins/base/ANPAudio.cpp
@@ +90,5 @@
> +{
> +public:
> +  NS_DECL_NSIRUNNABLE
> +
> +  AudioRunnable(ANPAudioTrack* s) {

ANPAudioTrack* aTrack? I suppose it doesn't really matter what with it not being public.

@@ +144,5 @@
> +                                   at.write,
> +                                   bytearray,
> +                                   wroteSoFar,
> +                                   buffer.size - wroteSoFar);
> +      wroteSoFar += retval;

not that we use it afterwards I suppose, but this ought to be below the if (retval < 0) below.

@@ +251,5 @@
> +
> +  s->keepGoing = false;
> +
> +  // deallocation happens in the AudioThread.  There is a
> +  // potential leak if anp_audio_start is never called, but

It wouldn't be a big deal to handle this case though? I could see Flash changing in a later version and this coming back to bite us.

::: dom/plugins/base/ANPPaint.cpp
@@ +295,5 @@
> +  p->textSkewX = skew;
> +}
> +
> +
> +/** Return the typeface ine paint, or null if there is none. This does not

typo (s/ine/in/)

@@ +322,5 @@
> +  if (!paint)
> +    return;
> +
> +  ANPPaintPrivate* p = (ANPPaintPrivate*) paint;
> +  //  p->typeface.mFont = typeface->mFont;

Do we want a not-implemented LOG here?

::: dom/plugins/base/nsNPAPIPluginInstance.cpp
@@ +875,5 @@
>  nsresult
>  nsNPAPIPluginInstance::IsWindowless(PRBool* isWindowless)
>  {
> +#ifdef ANDROID
> +  *isWindowless = PR_TRUE;

Is this correct? Going on the comments in the headers, I'd have thought this depends on the drawing model? (if it is, maybe a comment explaining why would be good?)

@@ +1129,5 @@
>    NPP npp = t->npp;
>    uint32_t id = t->id;
>  
> +  // Some plugins (Flash on Android) calls unscheduletimer
> +  // from this callback.

Does Flash expect the timer to be unscheduled if it calls this though? If this was a repeat timer, it would continue - should we check for this?

::: dom/plugins/base/nsPluginHost.cpp
@@ +2236,5 @@
>        if (NS_FAILED(rv))
>          continue;
> +      
> +      nsCString path;
> +      nextDir->GetNativePath(path);

Unused variable?

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ -1902,5 @@
>  #endif
>  
>  nsEventStatus nsPluginInstanceOwner::ProcessEvent(const nsGUIEvent& anEvent)
>  {
> -  // printf("nsGUIEvent.message: %d\n", anEvent.message);

I guess this was accidentally removed?

@@ +2852,5 @@
> +      aFrameRect.width  != pluginSurface->Width() ||
> +      aFrameRect.height != pluginSurface->Height()) {
> +
> +    pluginSurface = new gfxImageSurface(gfxIntSize(aFrameRect.width, aFrameRect.height), 
> +                                        gfxImageSurface::ImageFormatARGB32);

Why do we use an ARGB32 surface here instead of RGB16?

::: embedding/android/AndroidManifest.xml.in
@@ +22,5 @@
> +    <uses-permission android:name="android.permission.READ_LOGS"/>
> +    <uses-permission android:name="android.permission.WAKE_LOCK"/>
> +    <uses-permission android:name="android.permission.ACCESS_WIFI_STATE"/>
> +    <uses-permission android:name="android.permission.CHANGE_NETWORK_STATE"/> 
> +    <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE"/>

We already have ACCESS_NETWORK_STATE above

@@ +23,5 @@
> +    <uses-permission android:name="android.permission.WAKE_LOCK"/>
> +    <uses-permission android:name="android.permission.ACCESS_WIFI_STATE"/>
> +    <uses-permission android:name="android.permission.CHANGE_NETWORK_STATE"/> 
> +    <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE"/>
> +    <uses-permission android:name="android.permission.CHANGE_NETWORK_STATE"/>

Included twice?

::: embedding/android/GeckoApp.java
@@ +136,5 @@
>  
> +    public static final String PLUGIN_ACTION = "android.webkit.PLUGIN";
> +
> +    /**
> +     * A plugin wishes to be loaded in the WebView must provide this permission

Grammar nit-pick :) (s/A plugin wishes/Plugins that wish/)

@@ +159,5 @@
> +
> +        synchronized(mPackageInfoCache) {
> +
> +            // clear the list of existing packageInfo objects
> +            mPackageInfoCache.clear();

Do we need to do all of this work more than once? I suppose if you install new plugins while fennec is running, but is that likely/are there better ways to handle that? (or have I misunderstood?)

@@ +162,5 @@
> +            // clear the list of existing packageInfo objects
> +            mPackageInfoCache.clear();
> +
> +
> +            for (ResolveInfo info : plugins) {

blimey, didn't know you could do that in Java... Cool!

::: embedding/android/GeckoAppShell.java
@@ +1498,5 @@
> +            n = 2;
> +        else
> +            n = 4;
> +
> +        if (n == 0)

n can never be zero here - is the above meant to be an else if (format == PixelFormat.RGBA_8888)? (this is how it's treated below)

@@ +1518,5 @@
> +            info.width = info.canvas.getWidth();
> +            info.height = info.canvas.getHeight();
> +
> +            info.buffer = ByteBuffer.allocateDirect(bufSizeRequired);  //leak
> +            Log.i("GeckoAppShell", "!!!!!!!!!!!  lockSurfaceANP - Allocating buffer! " + bufSizeRequired);

Could we use Bitmaps instead of ByteBuffer for these? Perhaps when bug #662891 is resolved.

@@ +1582,5 @@
> +        Log.i("GeckoShell", "post to " + (mainThread ? "main " : "") + "java thread");
> +        //        if (mainThread)
> +            getMainHandler().post(new GeckoRunnableCallback());
> +            //        else
> +            //            getHandler().post(new GeckoRunnableCallback());

Comment explaining why this is always posted to the main thread?

::: layout/generic/nsObjectFrame.cpp
@@ +1682,5 @@
>  nsObjectFrame::PaintPlugin(nsDisplayListBuilder* aBuilder,
>                             nsRenderingContext& aRenderingContext,
>                             const nsRect& aDirtyRect, const nsRect& aPluginRect)
>  {
> +  if (mInstanceOwner) {

Should this be in an android-only #ifdef?

::: mobile/app/mobile.js
@@ -427,5 @@
>  pref("browser.ui.touch.bottom", 4);
>  pref("browser.ui.touch.weight.visited", 120); // percentage
>  
> -// plugins
> -#if MOZ_PLATFORM_MAEMO == 6

Maybe this should just be extended with an Android case? This will enable plugins on desktop and Maemo != 6 too.

@@ -432,5 @@
>  pref("plugin.disable", false);
> -#else
> -pref("plugin.disable", true);
> -#endif
> -pref("dom.ipc.plugins.enabled", true);

Has this pref been purposefully removed?

Comment 43

[Josh Aas]

Comment on attachment 556872 [details] [diff] [review]
android plugins

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

Lets put all of the Android files in "dom/plugins/base" into their own sub-directory called "android".

Is "android_npapi.h" an exact copy of the file from the Android source or did you modify it?

::: dom/plugins/base/PluginPRLibrary.cpp
@@ +35,5 @@
>   * the terms of any one of the MPL, the GPL or the LGPL.
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
> +#include "base/basictypes.h"

Can you also make this ifdef ANDROID? Is this even necessary?

::: dom/plugins/base/npfunctions.h
@@ +49,5 @@
>  #include "npruntime.h"
>  
> +#ifdef ANDROID
> +#include <jni.h>
> +#endif

Any changes to npfunctions.h will need to be upstreamed to npapi-sdk, then we can sync npapi-sdk to Mozilla.

http://code.google.com/p/npapi-sdk/

::: dom/plugins/base/nsNPAPIPlugin.cpp
@@ +115,5 @@
>  
>  #include "mozilla/plugins/PluginModuleParent.h"
>  using mozilla::plugins::PluginModuleParent;
>  
> +using namespace mozilla::dom;

What is this for?

@@ +2348,5 @@
>      // fall through
>    default:
> +#ifdef ANDROID
> +    LOG("unhandled get value: %d", variable);
> +#endif

Can we make this a debug-only logging statement for all platforms?

::: dom/plugins/base/nsNPAPIPluginInstance.cpp
@@ +74,5 @@
>  
>  static NS_DEFINE_IID(kIOutputStreamIID, NS_IOUTPUTSTREAM_IID);
>  static NS_DEFINE_IID(kIPluginStreamListenerIID, NS_IPLUGINSTREAMLISTENER_IID);
>  
> +NS_IMPL_THREADSAFE_ISUPPORTS0(nsNPAPIPluginInstance)

I don't think nsNPAPIPluginInstance is thread-safe.

@@ +87,5 @@
>  #endif
>  #endif
> +#ifdef ANDROID
> +    mDrawingModel(0),
> +    mSurface(nsnull),

These initializations are going to be out of order.

@@ +1205,5 @@
>    if (!t)
>      return;
>  
> +  if (t->running)
> +    return;

Seems like you could leave the timer running here. How about instead of just bailing and letting this run indefinitely you post a runnable that unschedules the timer?

::: dom/plugins/base/nsNPAPIPluginInstance.h
@@ +67,5 @@
>    NPP npp;
>    uint32_t id;
>    nsCOMPtr<nsITimer> timer;
>    void (*callback)(NPP npp, uint32_t timerID);
> +  PRBool running;

People will assume that this "running" variable will reflect whether or not the timer is running. It really reflects whether or not we're actually in the callback. Please change the name to reflect that, something like "inCallback".

@@ +213,5 @@
>    NPDrawingModel mDrawingModel;
>  #endif
>  
> +#ifdef ANDROID
> +  PRUint32 mDrawingModel;

Why not just use the same mDrawingModel declaration above, which is currently used for Mac OS X? And why not have the type be mDrawingModel?

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +1707,5 @@
> +    }
> +    else {
> +      NS_ASSERTION(PR_FALSE, "nsPluginInstanceOwner::DispatchFocusToPlugin, wierd eventType");   
> +    }
> +    mInstance->HandleEvent(&event, nsnull);

Are you sure you don't want to call PreventDefault() and/or StopPropagation() here?

::: layout/generic/nsObjectFrame.cpp
@@ +1694,5 @@
> +    gfxContext* ctx = aRenderingContext.ThebesContext();
> +
> +    mInstanceOwner->Paint(ctx, frameGfxRect, dirtyGfxRect);
> +    return;
> +  }

Android ifdef?

Comment 44

[Doug Turner (:dougt)]

Chris -- Thanks for your feedback.  I basically fixed everything in the next patch.  I am probably going to do some thing later (like perf work):

> ANPAudioTrack* aTrack? I suppose it doesn't really matter what with it not being public.

Okay

> not that we use it afterwards I suppose, but this ought to be below the if (retval < 0) below.

We use it in an unlikely log.  Either way, fixed.

> It wouldn't be a big deal to handle this case though? I could see Flash changing in a later version and this coming back to bite us.

There will be honeycomb audio changes which I can address this issue.

> typo (s/ine/in/)

Fixed

> Do we want a not-implemented LOG here?

Fixed

> Going on the comments in the headers, I'd have thought this depends on the drawing model? 

Added comment.  Right now, all we support is windowless.  There is a window interface in honeycomb that allows us to return a Native widow.  So, fix later when we support honeycomb.

> Does Flash expect the timer to be unscheduled if it calls this though? If this was a repeat timer, it would continue - should we check for this?

In any case, it is illegal.  I reported this to Adobe.  They will leak because they are misusing the API.

> I guess this was accidentally removed?

commented out code like this should be removed with fire.

> Why do we use an ARGB32 surface here instead of RGB16?

This is only in the ANP_BITMAP_DRAWING_MODEL case which we should probably consider removing.  Flash doesn't use it, only the sample plugin does.  It is a simpler API, but not a very useful API for flash.

> ACCESS_NETWORK_STATE 

Cleaned that up.

> Grammar nit-pick :) (s/A plugin wishes/Plugins that wish/)

schoolboy.  fixed.


> Do we need to do all of this work more than once?

Plugin scanning should be dynamic.  I don't think I care the much at this point.  I will file a followup!

> n can never be zero here - is the above meant to be an else if (format == PixelFormat.RGBA_8888)? (this is how it's treated below)

Yes, you're right.  I fixed that and we have a specific test for 8888.  If the pixalformat is something else, then we keep n at 0 and return null.  You never know.

> Bitmaps instead of ByteBuffer

Maybe.  Its a lot of work for possibility no gain.  We want to see if the plugins get resized a lot, i know that we did this in the widget code for software rendering, but this may be something different.  I'll keep a comment there and we should get data.  Also, this path will only be on pre-honeycomb.

> postToJavaThread

It only can go to the main handler.  I want to discuss with blassey a bit more when he gets back, but sending to just the main thread wfm.  I will remove the commented out code and just leave the LOG.

> nsObjectFrame::PaintPlugin

Yup.. ifdef missing :)  thanks!

> mobile/app/mobile.js

For this change, i am simply going to disable plugins on Android.  In the front end patch, we can enabled them..



Josh.  Thanks for the quick review.  I have address all of your concerns.


The android_npapi is exactly the same expect for the 4 typedefs at the bottom of the file.  These if'defs are ironically used in the header, but aren't defined on android -- at least not through the standard includes that gecko is using.


> +#include "base/basictypes.h"

removed.

> +using namespace mozilla::dom;

removed.

> Can we make this a debug-only logging statement for all platforms?

Yes, converted it to a NPN_PLUGIN_LOG instead.

> I don't think nsNPAPIPluginInstance is thread-safe.

We only need the isupports to be thread safe.  Take a look at the SufaceGetter
xxxx

> These initializations are going to be out of order.

Fixed.

> runnable that unschedules the timer?

I reported the problem to Adobe.  I really hate to work around shitcrap like this, but it is probably a good idea.  Fixed.

> Please change the name to reflect that, something like "inCallback".

Fixed.

> Are you sure you don't want to call PreventDefault() and/or StopPropagation() here?

I am not sure.  I don't see any reason for that.  I haven't seen any problems that lead me to believe that this is a significant issue right now.

> Why not just use the same mDrawingModel declaration above, which is currently used for Mac OS X? And why not have the type be mDrawingModel?

On android, the drawing model is of type ANPDrawingModels (an enum).  On Mac, it is of type NPDrawingModel (also an enum).  You can't assign one to the other without casting.  Would you rather me change the MacOS one to an PRUint32?

> I remove the npfunction changes.  This is something that google+android should standardize if they want.  To me, this is a crappy workaround.  It really should have gone through the standardization process.  Or - it should have been changed in android_npapi.



New patch coming up...

Comment 45

[Doug Turner (:dougt)]

Attachment: patch v.2

still includes np_function changes.  building without them were kinda cumbersome. Lets see about moving these changes into the proper header.  josh, what can I do there?

Comment 46

[Josh Aas]

Comment on attachment 559709 [details] [diff] [review]
patch v.2

Please just finish the move of android files to their own directory and file a bug on syncing npfunctions with npapi-sdk, like we talked about.

Comment 47

[Josh Aas]

For the record - I only reviewed the interaction of this implementation with existing plugin code. I didn't review the bulk of the android implementation itself.

Comment 48

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 556870 [details] [diff] [review]
front-end patch

Some general changes I'd like to see in the front-end patch:
* Remove the Site Menu entry. We don't want that menuitem shown for all pages and we can't limit it to flash-only pages. And we don't overly promote this feature at this stage.
* We need to make sure session history is used to preserve state when creating the new parent process tab. We don't want the back/forward state to be lost.
* "PluginReloadChromeEvent" -> "PluginDemandLoad"

Comment 49

[Ed Morley [:emorley]]

Comment on attachment 559709 [details] [diff] [review]
patch v.2

This part landed in inbound as:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4bdab069bbc

But had to be backed out again due to qt build bustage and oranges on other platforms:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07ef2c842854

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&usebuildbot=1&rev=d4bdab069bbc

Comment 50

[Doug Turner (:dougt)]

https://hg.mozilla.org/mozilla-central/rev/d4bdab069bbc

Comment 51

[Doug Turner (:dougt)]

minor changes on inbound.  basically didn't need a #include which caused that Bq failure. and needed to ifdef a change for android that dealt with the empty string verses null in one of the mCachedAttrParamValues.  also, i added license headers to the new files.


filed 687265 for the fennec UI bits.
filed 687266 for upstreaming the np_function bits.

Comment 52

[Doug Turner (:dougt)]

Bug 687267 tracks honeycomb support

Comment 53

[Ed Morley [:emorley]]

(In reply to Doug Turner (:dougt) from comment #50)
> https://hg.mozilla.org/mozilla-central/rev/d4bdab069bbc

That changeset was backed out.

The actual landing was:
https://hg.mozilla.org/mozilla-central/rev/09dc9b406bd6

Please can the inbound changesets be added to the bug each time, since otherwise it makes following multiple landings/backouts harder than it need be, for the people doing the merge. Thanks :-)

Comment 54

[Steffen Wilberg]

Comment on attachment 559709 [details] [diff] [review]
patch v.2

Comment 53 says this has been checked in (in a modified form); marking so.

Comment 55

[Steffen Wilberg]

Comment on attachment 556870 [details] [diff] [review]
front-end patch

The front-end part has been moved to bug 687265 apparently, marking this as obsolete.

Comment 56

[Steffen Wilberg]

Tweaking summary per commit message. Blocks the front-end part in bug 687265.

Comment 57

[Edgar]

Hi, I have a fair reason to have Flash working on Fennec.  I have several websites (corproate is one) that has flash in a section that uses NTLM authentication.  So I would benefit from both, the NTLM Authentication (GREAT JOB by the way, works flawlessly) and adding the Flash would be a major + for us.

Thanks!

Comment 58

[Edgar]

Hi, I have a fair reason to have Flash working on Fennec.  I have several websites (corproate is one) that has flash in a section that uses NTLM authentication as well.  So I would benefit from both, the NTLM Authentication (GREAT JOB by the way, works flawlessly) and adding the Flash would be a major + for us.

Thanks!

Comment 59

[Henrik Skupin [:whimboo][⌚️UTC+2]]

If setting the litmus flag was on purpose please reference the appropriate test in a comment.

Comment 60

[Kevin Brosnan [Ex-Mozilla]]

Clearing flags that were set on an old bug.