We should rename plugin processes on Mac OS X.
I get 'Minefield Plugin Process' under Activity Monitor. Do we still want a patch for this?
I think we still want to fix this but we won't block on it. I'd approve a patch.
Isn't this already fixed?
I should have been more specific - I meant for this bug to be about making the process name reflect the particular plugin that the process is for.
Created attachment 466209 [details] [diff] [review]
Uses the method linked by Josh. I just need to know what format we want to use and how to support localization?
Would really like to see this fix finished. Willing to mentor someone fix up this patch. Otherwise I may look at it myself.
I really don't think we need to support localization for process names.
I have this working renaming the process to the plugin's name, such as 'Shockwave Flash'. Do we have any preference for the exact name?
'Plugin Container: Shockwave Flash'?
I'd like to get this landed soon.
Created attachment 539824 [details] [diff] [review]
Let's get the patch reviewed even I'm still waiting on feedback for the process title.
Comment on attachment 539824 [details] [diff] [review]
This is basically fine with me, but I think it could be improved.
1) Why not make getASNFunc and setInformationItemFunc static, so they
only have to be initialized once?
2) What about the case where aProcessName is NULL or (more likely) an
empty string? This is probably unusual, but I've seen it myself in
the last 2-3 days: On one of my partititions, the QuickTime
plugin's nsPluginInfo.fName somehow got set to an empty string (I
saw this in Tools : Add Ons).
3) Unlike the original Chromium code (from SetProcessName() in
mac_util.mm) your patch doesn't explain why it uses
GetCurrentProcess(). With no explanation, the call to
GetCurrentProcess() seems completely unnecessary.
Finally, for the record, I found where this code exists in WebKit --
in the non-open-source WebKitLibraries, under the following thin
wrapper (defined in WebKitSystemInterface.h):
> 1) Why not make getASNFunc and setInformationItemFunc static, so
> they only have to be initialized once?
Now that I think more about it, this code will probably only be called
once. If so, this change isn't needed.
(In reply to comment #11)
> > 1) Why not make getASNFunc and setInformationItemFunc static, so
> > they only have to be initialized once?
> Now that I think more about it, this code will probably only be called
> once. If so, this change isn't needed.
I'm going to make the change anyways since this code will probably get moved and reused when we have content processes by default.
I haven't heard any suggestion about the name format. Should I post to dev.platform?
Created attachment 540462 [details] [diff] [review]
Thanks for the review Steven. Just waiting on a decision for the process name and I will upload a final version for review.
Created attachment 541723 [details] [diff] [review]
I fixed 1, 2. I tried removing 3 but the rename failed so I added a comment to indicate that this is required. I'm not sure why it is required.
I couldn't get the appName from the info service in the plugin process so I opted to use cocoa to get the current process name.
The format for the process name is '<PROCESSNAME> (<PLUGIN NAME>)".
Comment on attachment 541723 [details] [diff] [review]
This looks fine to me.
One very small nit: formatedName should be formattedName :-)
Green try run, ready for checkin.
Created attachment 541933 [details] [diff] [review]
SetProcessName v4.0 (Fixed typo)
Conflicts with bug 587370.
With this patch, I get a compile error using clang:
/Users/ehsanakhgari/bin/clang/bin/clang++ -o PluginUtilsOSX.o -c -fvisibility=hidden -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES -DSTATIC_EXPORTABLE_JS_API -DOSTYPE=\"Darwin10.8.0\" -DOSARCH=Darwin -DEXCLUDE_SKIA_DEPENDENCIES -DCHROMIUM_MOZILLA_BUILD -DOS_MACOSX=1 -DOS_POSIX=1 -DFORCE_PR_LOG -I/Users/ehsanakhgari/moz/inbound/dom/plugins/ipc/../base -I/Users/ehsanakhgari/moz/inbound/xpcom/base/ -I/Users/ehsanakhgari/moz/inbound/ipc/chromium/src -I/Users/ehsanakhgari/moz/inbound/ipc/glue -I../../../ipc/ipdl/_ipdlheaders -I/Users/ehsanakhgari/moz/inbound/dom/plugins/ipc -I. -I../../../dist/include -I../../../dist/include/nsprpub -I/Users/ehsanakhgari/moz/inbound/obj-ff-dbg/dist/include/nspr -I/Users/ehsanakhgari/moz/inbound/obj-ff-dbg/dist/include/nss -fPIC -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-invalid-offsetof -Wno-variadic-macros -Werror=return-type -fno-strict-aliasing -fno-common -fshort-wchar -pthread -DNO_X11 -pipe -DDEBUG -D_DEBUG -DTRACING -g -DNO_X11 -DMOZILLA_CLIENT -include ../../../mozilla-config.h -MD -MF .deps/PluginUtilsOSX.pp -fobjc-exceptions /Users/ehsanakhgari/moz/inbound/dom/plugins/ipc/PluginUtilsOSX.mm
/Users/ehsanakhgari/moz/inbound/dom/plugins/ipc/PluginUtilsOSX.mm:177:48: error: no member named 'sApplicationASN' in namespace 'mozilla::plugins::PluginUtilsOSX'
static void *mozilla::plugins::PluginUtilsOSX::sApplicationASN = NULL;
/Users/ehsanakhgari/moz/inbound/dom/plugins/ipc/PluginUtilsOSX.mm:178:48: error: no member named 'sApplicationInfoItem' in namespace 'mozilla::plugins::PluginUtilsOSX'
static void *mozilla::plugins::PluginUtilsOSX::sApplicationInfoItem = NULL;
2 errors generated.
I think clang is right here, these two variables have not been declared *anywhere* <http://mxr.mozilla.org/mozilla-central/search?string=sApplicationASN>. I have no idea why gcc doesn't choke on this.
Benoit, can you please fix this? This is currently blocking me from building, which kind of sucks. ;-)
Created attachment 543003 [details] [diff] [review]
Remove static vars for clang
I'm not entirely sure why the previous were not proper declaration of global variables with static linkage. If someone cares to explain I'd love to know?
I don't think you cared much for having it static so I figured I would take the simplest approach and revert to what I previous had since it has no impact. We're trading off storage size to cache something that wont be called again.
You're trying to define a variable which was never declared. It has to be declared in the class, e.g.
static void* mozilla::plugins::PluginUtilsOSX::sApplicationASN;
And then you define it in the .cpp file without a "static".
Created attachment 543313 [details] [diff] [review]
Comment on attachment 543313 [details] [diff] [review]
Landed on inbound.
QA tracking to verify in Firefox 7.
Setting resolution to Verified Fixed on MacOS X 10.6 and 10.7.
To verify that the process from the description is renamed, I have loaded http://www.shockwave.com/home.jsp and opened Activity Monitor. The 'Firefox','Aurora','Nightly' Plugin Process (Shockwave Flash) process is present and renamed.
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:9.0a1) Gecko/20110919 Firefox/9.0a1
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:8.0a2) Gecko/20110921 Firefox/8.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0a1) Gecko/20110922 Firefox/9.0a1
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0a2) Gecko/20110921 Firefox/8.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0