[e10s] Name mac content process

RESOLVED FIXED in mozilla34



5 years ago
5 years ago


(Reporter: BenWa, Assigned: jaas)


Dependency tree / graph

Firefox Tracking Flags




(1 attachment, 4 obsolete attachments)

Posted patch patch (obsolete) — Splinter Review
No description provided.
Attachment #8423403 - Flags: review?(mstange)
Attachment #8423403 - Flags: review?(mstange) → review+
Backed out:

Apparently ./toolkit/components/aboutmemory/tests/test_memoryReporters2.xul checks the process name.
Posted patch patch v2 (obsolete) — Splinter Review
njn can you review the change to test_memoryReporters2.xul and also are there other places where about:memory will rely on this process name?
Assignee: nobody → bgirard
Attachment #8423403 - Attachment is obsolete: true
Attachment #8424120 - Flags: review?(n.nethercote)
Attachment #8424120 - Flags: review?(n.nethercote) → review+
Benoit: did you forget to land this r+'d patch from two months ago? :)
tracking-e10s: --- → +
Flags: needinfo?(bgirard)
It keeps having small failure with the about:memory code which uses these process name. Then I got moved back to b2g. If someone want to adjust how about:memory this shouldn't be very much effort to land.
Flags: needinfo?(bgirard)
Assignee: bgirard → joshmoz
With the current patch, the name of the process in Activity Viewer is "Nightly Plugin Process (Content Process)". This seems less than ideal.
The reason I picked this is:
- To be able to filter by 'Firefox/Nightly' in the process manager and group the processes, originally for plugins.
- To match the plugin process name.

Feel free to change it.
OK, I'll try to come up with a new scheme that works well for both plugins and content processes.
Posted patch patch v3 (obsolete) — Splinter Review
This implements a better process naming scheme and fixes another memory reporter test.

With this, using Nightly, content processes are called "Nightly Content Process" and plugins processes are called "Nightly Plugin Process (Shockwave Flash)".
Attachment #8424120 - Attachment is obsolete: true
Attachment #8478824 - Flags: review?(n.nethercote)
Comment on attachment 8478824 [details] [diff] [review]
patch v3

Review of attachment 8478824 [details] [diff] [review]:

Seems fine. BenWa appears to have stronger opinions than me about the exact naming scheme, so you might like to get his approval too.
Attachment #8478824 - Flags: review?(n.nethercote) → review+
We talked on IRC, I think this is better.
I've been thinking that having the word "Process" in the name is a bit redundant, since these names show up in a list of processes, and it takes up a lot of space. I'm thinking of changing to "Nightly Web Content" and "Nightly Plugin Content (Shockwave Flash)" instead, which is also much closer to what Safari does.

Any objections?
Posted patch patch v4 (obsolete) — Splinter Review
Attachment #8478824 - Attachment is obsolete: true
Attachment #8479111 - Flags: review?(bgirard)
Comment on attachment 8479111 [details] [diff] [review]
patch v4

Review of attachment 8479111 [details] [diff] [review]:

::: dom/plugins/ipc/PluginModuleChild.cpp
@@ +184,5 @@
>      }
>  #else // defined(OS_MACOSX)
> +    const char* namePrefix = "Plugin Content";
> +    char nameBuffer[60];

optional nit: IMO 60 is a bit short for allowing longer APP_NAME. I don't see the harm in requesting 1024.

::: ipc/app/macbuild/Contents/Resources/English.lproj/InfoPlist.strings.in
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  /* Localized versions of Info.plist keys */
> +CFBundleName = "%APP_NAME%";

I'm not familiar with our localization setup. If we localized this in the past we wont be able to do it now.

Observation: If we can't name the process then it wont default to Plugin Process. I don't think it's happens however.
Attachment #8479111 - Flags: review?(bgirard) → review+
Posted patch patch v5Splinter Review
Upped character limit to 80.

Attachment #8479111 - Attachment is obsolete: true
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.