Closed Bug 1348042 Opened 7 years ago Closed 7 years ago

Make LocaleService operate in client-server mode

Categories

(Core :: Internationalization, enhancement)

Product:
Core
Component:
Internationalization
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1348042 - Refactor LocaleService to operate in server-client mode.
59 bytes, text/x-review-board-request
qdot
: review+
Details 
At the moment we don't handle language negotiation in the content process.

We may either want to connect it to parent process, or get the data required for language negotiation into content process.

For example, in order to have access to `general.useragent.locale` for requestedLocales, we'll need to add it to:

http://searchfox.org/mozilla-central/source/dom/ipc/ContentPrefs.cpp#13
In order to send the data to content process when it's created, :smaug recommends using:

http://searchfox.org/mozilla-central/source/dom/ipc/PContent.ipdl#251
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment on attachment 8851314 [details]
Bug 1348042 - Refactor LocaleService to operate in server-client mode.

This is very much WIP, but should be fully functional.

:ehsan, can you take a quick glance and verify if this approach to keeping child process LocaleServices is what you'd like us to do?

Also, I cannot get the ContentParent::Observe to fire at all :(
I added the printf at the top tried just launching browser console and doing `Services.obs.notifyObservers(this, "intl:app-locales-changed", null);` and the ChromeRegistry observe fires, but ContentProcess' one does not.

I'm receiving observe printfs from Prefs changes, but can't even get other topics to fire, like 'cacheservice:empty-cache' or 'file-watcher-update'.
Can you advice on why it may be?

p.s. This patch is sponsored by letter "j" like "jetlag", and let it be known that it has been submitted after 2.5 hours of work at 7:21am, so I can't rule out that I just don't understand what I'm doing :(
Attachment #8851314 - Flags: feedback?(ehsan)
Comment on attachment 8851314 [details]
Bug 1348042 - Refactor LocaleService to operate in server-client mode.

https://reviewboard.mozilla.org/r/123648/#review130480

So sorry for the long delay in getting back to you (post-work-week backlog).  This looks like the correct approach to me.  f+, but mozreview doesn't allow me to do that, so this will appear as r+.

::: dom/ipc/ContentParent.cpp:2769
(Diff revision 1)
>  #endif
>    else if (!strcmp(aTopic, "cacheservice:empty-cache")) {
>      Unused << SendNotifyEmptyHTTPCache();
>    }
> +  else if (!strcmp(aTopic,  "intl:app-locales-changed")) {
> +    printf("ContentParent::Observer received intl:app-locales-changed!\n");

Could the reason this isn't being called be that we're returning early somehow above here in Observe()?  Have you verified that the observer is successfully added in Init()?

::: intl/locale/LocaleService.cpp:146
(Diff revision 1)
>  }
>  
> +void
> +LocaleService::SetAppLocales(const nsTArray<nsCString>& aAppLocales)
> +{
> +  if (!XRE_IsParentProcess()) {

Please have an assertion here that checks you're being called in the right process too.

::: intl/locale/LocaleService.cpp:151
(Diff revision 1)
> +  if (!XRE_IsParentProcess()) {
> +    printf("Hey! I'm a content process and I just received an update of app locales!\n");
> +    mAppLocales = aAppLocales;
> +    nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +    if (obs) {
> +      obs->NotifyObservers(nullptr, "intl:app-locales-changed", nullptr);

Shouldn't we be calling LocaleService::Refresh() here?

::: intl/locale/LocaleService.cpp:214
(Diff revision 1)
>  
>  void
>  LocaleService::Refresh()
>  {
> +  if (!XRE_IsParentProcess()) {
> +    return;

We need an assertion here also.
Attachment #8851314 - Flags: review+
Attachment #8851314 - Flags: review+
Attachment #8851314 - Flags: feedback?(ehsan)
Attachment #8851314 - Flags: feedback+
:jfkthame - this makes LocaleService work in content process. It doesn't completely fix the problem the datepicker has (bug 1349106) because JS context is created before we can call SetAppLocales.

So I need another patch to make JS context listen to intl:app-locales-changed. I'll file a bug for that.
We may want to go further than just dividing between parent and content process and have LocaleService act in a client-server architecture where one LocaleService may act as a parent and others as a client.

The difference is that at least in one case (Fennec), the parent process LocaleService would *also* act as a client for the Java app that would broadcast language negotiation results.

So I think I'll update the patch to settings the mode independently of which process we're in.
This is the first attempt, I'll document the code and add some tests on Monday.

It actually seems to be fairly easy and doesn't require much changing, so hi5 to us.

In the current code, I just do parent process as a "server" and content processes as "client", but in the future we'll want to set the parent process in Fennec as "client" as well, and retrieve the data from Java process.

I accidentally also incorporated the fix for bug 1345527 here, so it really seems like an overall finalization of the API.

:pike - I'd like to ask for your high level review of the architectural approach before I start finalizing the patch.
Flags: needinfo?(l10n)
Summary: Improve LocaleService in Content process → Make LocaleService operate in client-server mode
Added docs on server/client split.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #13)
> :pike - I'd like to ask for your high level review of the architectural
> approach before I start finalizing the patch.

Not sure I grok the level of feedback you're looking for. The xpcominit piece would be the only one triggering my eyebrowes, but that's for someone that knows the patterns we have around reading and writing info there. Didn't check for pre-existing patterns.
Flags: needinfo?(l10n)
Yeah, I just wanted to run the idea of getting Gecko LocaleService in this dual server/client architecture model through you with an intent to keep ParentProcess as server and ContentProcesses as client on desktop, and all processes of Gecko as clients in Fennec (with Java process becoming the server).

One more realization is that this resolves the unit testing challenge, as now the test can put LocaleService in client mode, feed it with mock data and then test the code for how it behaves, so that's a nice bonus :)
Comment on attachment 8851314 [details]
Bug 1348042 - Refactor LocaleService to operate in server-client mode.

https://reviewboard.mozilla.org/r/123648/#review133666

This looks reasonable to me; one suggestion is to get rid of the `sIsServer` static, and instead record the mode it's operating in within the LocaleService itself.

Note, though, that dom/ipc is really outside my area of the code, and you should probably get a review from someone in that team to check that all's well.

Just clearing the r? flag for now, as I'm not sure how to redirect it in mozreview... :(

::: intl/locale/LocaleService.h:283
(Diff revision 6)
>    nsTArray<nsCString> mAppLocales;
> +  nsTArray<nsCString> mRequestedLocales;
> +  nsTArray<nsCString> mAvailableLocales;
>  
>    static StaticRefPtr<LocaleService> sInstance;
> +  static bool sIsServer;

I don't think this needs to be a static; it can be a normal member variable in the LocaleService.

::: intl/locale/LocaleService.cpp:174
(Diff revision 6)
>  LocaleService*
>  LocaleService::GetInstance()
>  {
>    if (!sInstance) {
>      sInstance = new LocaleService();
> +    sIsServer = XRE_IsParentProcess();

With the static `sIsServer` replaced by an `mIsServer` member, you could initialize it in the LocaleService constructor. In fact, how about passing it as a constructor argument, so

    sInstance = new LocaleService(XRE_IsParentProcess());

in which case the flag can be declared as a `const bool` (reinforcing the fact that it doesn't change for a given instance of the service) and set directly in the LocaleProcess initializer list.

::: intl/locale/LocaleService.cpp:176
(Diff revision 6)
>  {
>    if (!sInstance) {
>      sInstance = new LocaleService();
> +    sIsServer = XRE_IsParentProcess();
>  
> +    if (sIsServer) {

And here, we'd have a `bool IsServer() const` accessor, so you can write

    if (sInstance->IsServer()) {
      ...
    }

I think all the other uses of `sIsServer` are within instance methods, so they'd simply be replaced by `mIsServer` to check the mode of the instance.
Attachment #8851314 - Flags: review?(jfkthame)
Attachment #8851314 - Flags: review?(kyle)
Comment on attachment 8851314 [details]
Bug 1348042 - Refactor LocaleService to operate in server-client mode.

https://reviewboard.mozilla.org/r/123648/#review135016

Marking as r- just because you've got a couple of checks I'd like to see fixed and I don't like unmarking reviews, and I agree on the "you don't need sIsServer to be static" from the other review. I think you'll be ready to land after the next patch revision though!

Since you said you were looking for some architectural guidance earlier in the review... This is more a very light nit for readability than anything, I don't have a ton of experience with locales, but for dividing IPC (or in this case, client/server, which is a fine naming scheme for the mode since this won't always be split along hard parent/content lines) functionality, I'd usually subclass to have Client and Server functions in different classes, and share the same base implementation via inheritance. Since you've already got a factory function in LocaleService via GetInstance, you can put the logic on which type you hand out there, and then there's less logic to follow as you'll either do whatever it is you're supposed to, or just assert/crash if you're somewhere you shouldn't be. That said, what you've got here works fine, and I'm not sure how much work it'd be to rearchitect the LocaleService in general for that.

Finally, has this gone through a try run yet, just to make sure your non-e10s/e10s functionality is there? I don't see where there'd be any problems, and I'm guessing most of the functionality will be covered by tests already in the system, but it never hurts to prove it out.

::: intl/locale/LocaleService.h:136
(Diff revision 6)
> +   * in order to bring the client LocaleService in sync with the server
> +   * LocaleService.
> +   *
> +   * Currently, it's called by the IPC code.
> +   */
> +  void SetAppLocales(const nsTArray<nsCString>& aAppLocales);

Since this is coming in from IPC, I think you want a 

nsTArray<nsCString>&& 

instead of a 

const nsTArray<nsCString>&

Since you won't be using the array in the IPC code again after this, it means your assignment will implicitly be a move, instead of a copy.

::: intl/locale/LocaleService.cpp:113
(Diff revision 6)
> +
> +static bool
> +ReadAvailableLocales(nsTArray<nsCString>& aRetVal)
> +{
> +  nsCOMPtr<nsIToolkitChromeRegistry> cr =
> +    mozilla::services::GetToolkitChromeRegistryService();

cr could be null here. Check for validity before using.
Attachment #8851314 - Flags: review?(kyle) → review-
Comment on attachment 8851314 [details]
Bug 1348042 - Refactor LocaleService to operate in server-client mode.

https://reviewboard.mozilla.org/r/123648/#review135016

> Since this is coming in from IPC, I think you want a 
> 
> nsTArray<nsCString>&& 
> 
> instead of a 
> 
> const nsTArray<nsCString>&
> 
> Since you won't be using the array in the IPC code again after this, it means your assignment will implicitly be a move, instead of a copy.

This review comment can be ignored. This can't actually be a rvalue ref argument, as we call the function both from the IPC message and from XPCOMInit (to initially load the array).
Comment on attachment 8851314 [details]
Bug 1348042 - Refactor LocaleService to operate in server-client mode.

https://reviewboard.mozilla.org/r/123648/#review135058
Attachment #8851314 - Flags: review?(kyle) → review+
Depends on: 1358543
Comment on attachment 8851314 [details]
Bug 1348042 - Refactor LocaleService to operate in server-client mode.

Kyle,

Can you take another look at the patch? I had to update it because I didn't push the requestedLocales updates to the client side, and since I turned off observer on requestedLocales prefs on the client side, it wasn't updated at all.

That caused webextensions tests to fail.

Now I also found a leak and am testing a fix (asan builds oranges) in bug 1358543, but that is not related to this patch, this patch just exposes it. :)

You only need to interdiff 9-11.
Attachment #8851314 - Flags: review+ → review?(kyle)
Comment on attachment 8851314 [details]
Bug 1348042 - Refactor LocaleService to operate in server-client mode.

https://reviewboard.mozilla.org/r/123648/#review136360

Re-r+'d.
Attachment #8851314 - Flags: review?(kyle) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0011bdb0863e
Refactor LocaleService to operate in server-client mode. r=Ehsan,qdot
https://hg.mozilla.org/mozilla-central/rev/0011bdb0863e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1360596
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: