Closed
Bug 1102295
Opened 10 years ago
Closed 9 years ago
Investigate whether we can get country data from mac OS APIs
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: benjamin, Assigned: smichaud)
References
Details
Attachments
(2 files, 1 obsolete file)
614 bytes,
text/plain
|
Details | |
8.74 KB,
patch
|
benjamin
:
review+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
In order to target the new Yahoo! search default, we'd like to be able to change the default of users within the US but not some other countries (particularly asian countries). Our base plan is to do this by time zone, but we'd like to investigate targeting this a little more carefully by country based on information already contained in the OS. Steven, can you provide an overview what other information we can obtain from MacOS about the user's country, and a basic estimate of the effort required? This is extremely urgent, if you can't provide info by today please let me know.
Flags: needinfo?(smichaud)
Assignee | ||
Comment 1•10 years ago
|
||
I *think* we can programmatically get the information entered by the user in the Time Zone panel of the Date & Time system pref panel. What can be set directly is "Closest City". The "Time Zone" setting is derived from that. There are *lots* of cities that can be specified. But I think the "country" is also always specified. If the user hasn't changed this information, I believe it defaults to Cupertino, CA. In the US, at least. But if the user *doesn't* change this information, their clock will almost certainly be incorrect -- so I think it's likely that most users will have set this information. With luck, later I'll post a test app that gets (and prints) this information.
Reporter | ||
Comment 2•10 years ago
|
||
ok, if it's possible to do this as a patch to nsSystemInfo.cpp that would be useful. I'm traveling for the rest of the day, but Gavin can help decide if this is something we want to use in FF34.
Assignee | ||
Comment 3•10 years ago
|
||
> if it's possible to do this as a patch to nsSystemInfo.cpp
Not possible. It will need to use Cocoa syntax.
Assignee | ||
Comment 4•10 years ago
|
||
Here's my test app. I tested it (and it worked fine) on the two endpoints of our current OS X support -- OS X 10.6.8 and OS X 10.10.1. I assume what we want is the "CountryCode". Let me see if I can add code that queries this to Cocoa code, which can then be called from something in nsSystemInfo.cpp. Gavin, do you have any suggestions about how I should do this?
Flags: needinfo?(gavin.sharp)
Assignee | ||
Updated•10 years ago
|
Attachment #8526287 -
Attachment mime type: text/x-troff-mm → text/plain
Flags: needinfo?(smichaud)
Comment 5•10 years ago
|
||
You could add a "countryCode" attribute to widget/nsIGfxInfo.idl, and the corresponding implementation to widget/cocoa/GfxInfo.mm. Abuse of a graphics-related API for something that isn't, but I think that's fine (it's kind of a dumping ground already).
Updated•10 years ago
|
Flags: needinfo?(gavin.sharp)
Assignee | ||
Comment 6•10 years ago
|
||
I followed Gavin's suggestion. nsSystemInfo.cpp seems inappropriate, because none of its values are string values. This works in my tests. I tested it by opening the Scratchpad in Developer Tools, choosing Browser for the environment, running the following lines, then choosing Display. The result on my machine was "US" (as expected). var gfxInfo = Components.classes['@mozilla.org/gfx/info;1']. getService(Components.interfaces.nsIGfxInfo); print(gfxInfo.countryCode);
Attachment #8526415 -
Flags: review?(benjamin)
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8526415 [details] [diff] [review] Fix Actually maybe it's best to ask Gavin to review this, since Benjamin may be unavailable for a while.
Attachment #8526415 -
Flags: review?(benjamin) → review?(gavin.sharp)
Updated•10 years ago
|
status-firefox34:
--- → affected
status-firefox35:
--- → affected
status-firefox36:
--- → affected
tracking-firefox34:
--- → +
tracking-firefox35:
--- → +
tracking-firefox36:
--- → +
Comment 8•10 years ago
|
||
Comment on attachment 8526415 [details] [diff] [review] Fix I think you probably want to use "Append" instead of "AppendASCII" in GfxInfo::GetSelectedCityInfo? I also think we may want to put the new attribute at the bottom of the IDL definition, and avoid changing the IID. This looks good to me. We're going to need a very detailed enumeration of the possible values here - I think we care to identify only "US" and "Canada". This patch will also need no-op implementations for the other platforms (they can just return null, ideally). Is there Apple documentation for that?
Attachment #8526415 -
Flags: review?(gavin.sharp)
Attachment #8526415 -
Flags: review?(benjamin)
Attachment #8526415 -
Flags: feedback+
Assignee | ||
Comment 9•10 years ago
|
||
Comment on attachment 8526415 [details] [diff] [review] Fix > I think you probably want to use "Append" instead of "AppendASCII" > in GfxInfo::GetSelectedCityInfo? Really? I'd think not. But I don't understand our (horribly complex) XPCOM string api very well. I'll let Benjamin decide. > I also think we may want to put the new attribute at the bottom of > the IDL definition, and avoid changing the IID. Why should we avoid changing the IID? There's already a note at the top of nsIGfxInfo.idl which says "this interface is completely undesigned, not stable and likely to change". But once again, I'll let Benjamin decide. > We're going to need a very detailed enumeration of the possible > values here Let me see what I can come up with. > I think we care to identify only "US" and "Canada" But if this is the case, getting a comprehensive list of the possible values is less urgent. Let me check about "Canada" -- I suspect all of these country codes are two-letter values (the same ones that are used for "language" country codes). > This patch will also need no-op implementations for the other > platforms I thought about this but decided not to, at least for the time being. It'd be trivial to do, though. And we might actually want to use the "countryCode" value on other platforms -- if (for example) we can come up with a way to translate timezone names into country codes.
Assignee | ||
Comment 10•10 years ago
|
||
> Let me check about "Canada"
I just checked, and the "country code" for Canada is "CA".
If someone could give me a list of countries we care about, I can just check Apple's "CountryCode" for each of them (by deliberately choosing the wrong "closest city" in the Date & Time preference panel).
Comment 11•10 years ago
|
||
(In reply to Steven Michaud from comment #9) > Really? I'd think not. But I don't understand our (horribly complex) > XPCOM string api very well. I'll let Benjamin decide. Given the variable name I assumed it contained UTF-8 text, so it seems wrong to treat it as ASCII. But it doesn't really matter if all possible values are ASCII anyways. > Why should we avoid changing the IID? Because it has the potential to cause compatibility issues, and isn't strictly necessary if you avoid changing the vtable (by only adding to the end of the IDL). > I thought about this but decided not to, at least for the time being. I don't think your patch will compile on Linux/Windows without it...
Reporter | ||
Comment 12•10 years ago
|
||
Comment on attachment 8526415 [details] [diff] [review] Fix To land this on branches, we should have a new interface: interface nsIGfxInfo2 : nsIGfxInfo { /** * Not really Gfx-related. Here for convenience, * possibly only temporarily. See bug 1102295. */ readonly attribute DOMString countryCode; }; There are several advantages of this short-term: * only the mac implementation needs to implement the interface, so we don't need to touch other platforms * we don't need to bump the IID of nsIGfxInfo and risk compat issues This requires changing the existing NS_IMPL_ISUPPORTS_INHERITED declaration to add nsIGfxInfo2. If countryCodeUTF8 is guaranteed-ASCII, then AppendASCII is fine. Otherwise this should use AppendUTF8toUTF16. Clearing review to see the interface/IID changes.
Attachment #8526415 -
Flags: review?(benjamin)
Assignee | ||
Comment 13•10 years ago
|
||
> Is there Apple documentation for that? This is one of Gavin's questions from comment #8 that I forgot to answer. The answer is "no" -- there isn't any Apple documentation for the user defaults in "com.apple.preferences.timezone.selected_city". But this is yet one more case of something extremely useful (in fact for us crucial) that Apple hasn't bothered to document. As I mentioned in comment #4, the format of this information (and the way to get it) has remained stable since at least OS X 10.6. So it should be entirely safe to use it. And in fact we really have no choice but to use it.
Assignee | ||
Comment 14•10 years ago
|
||
> interface nsIGfxInfo2 : nsIGfxInfo Benjamin, this suggestion of yours didn't work. But this patch does work, and is (I think) a reasonable facsimile of what you want. Note that nsIGfxInfoDebug also inherits from nsISupports (not from nsIGfxInfo). And there are other examples in the tree (e.g. nsIContextMenuListener2) of extension interfaces that don't inherit from the interface they're extending. I'm pretty sure none of Apple's "country codes" contain non-ASCII characters. But on general principles I switched to using AppendUTF8toUTF16(). I did (and tested) both debug and non-debug builds on OS X -- I didn't see any problems. I also started an all-platform set of tryserver builds, here: https://tbpl.mozilla.org/?tree=Try&rev=b642984c1245
Attachment #8526415 -
Attachment is obsolete: true
Attachment #8527049 -
Flags: review?(benjamin)
Assignee | ||
Comment 15•10 years ago
|
||
(Following up comment #13) Apple's AppKit and (private) GeoKit frameworks check the "com.apple.preferences.timezone.selected_city" user defaults, both on OS X 10.8.5 and 10.10.1 (the latest version). And on 10.8.5 so does Apple's iMedia app, as does their Weather "extension" on 10.10.1. So, as with their undocumented APIs, there's nothing unfinished or provisional about this stuff -- it's just that Apple reserves it for their own use. And the fact that they *do* use it makes it much less likely to change (or disappear) suddenly.
Reporter | ||
Updated•10 years ago
|
Attachment #8527049 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 16•10 years ago
|
||
Comment on attachment 8527049 [details] [diff] [review] Rev1 patch Landed on mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/d0d36a9b8b7d
Comment 17•9 years ago
|
||
Thanks for the quick response on this one, Steven. I decided not to take it for Firefox 34 to reduce risk/scope, but we will get it in a later release.
Comment 18•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d0d36a9b8b7d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 19•9 years ago
|
||
If this is low risk, please nominate for Beta uplift approval by Monday Dec 22 - otherwise this will be a wontfix and ship in FF36.
Flags: needinfo?(smichaud)
Assignee | ||
Comment 20•9 years ago
|
||
Comment on attachment 8527049 [details] [diff] [review] Rev1 patch Approval Request Comment [Feature/regressing bug #]: na [User impact if declined]: It won't be possible to use Chrome JavaScript code to query a Mac's country code corresponding to its (timezone-setting based) physical location [Describe test coverage new/current, TBPL]: limited manual testing [Risks and why]: No risks -- the new interface currently isn't used at all. But even if we do start using it, the risk that it won't operate as expected is very low. [String/UUID change made/needed]: None
Flags: needinfo?(smichaud)
Attachment #8527049 -
Flags: approval-mozilla-beta?
Updated•9 years ago
|
Attachment #8527049 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 21•9 years ago
|
||
Comment on attachment 8527049 [details] [diff] [review] Rev1 patch Landed on mozilla-beta: https://hg.mozilla.org/releases/mozilla-beta/rev/caf2307e96a3 This landing was more complicated than usual in a couple of ways: 1) GfxInfoBase.cpp/h are in a different location on the 35 branch. 2) Since I've changed an *.idl file, I wasn't allowed to land on beta until I specified ba=[somebody]. Since I actually didn't change any interface, and since the original idea was Bemjamin's, I just added ba=bsmedberg. Please let me know if I did anything wrong.
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 22•9 years ago
|
||
> Since I've changed an *.idl file
Actually I *added* an *.idl file, but this got counted as a change.
Updated•9 years ago
|
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•