Move country code out of nsIGfxInfo2

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: aklotz, Assigned: cmanchester, Mentored)

Tracking

Trunk
mozilla43
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(2 attachments)

The patches landed in bug 1102297 and bug 1102295 implemented a nsIGfxInfo2 interface, which isn't the proper interface for retrieving the system country code. The purpose of this bug is to move it somewhere more sensible, like nsSystemInfo.
Mentor: aklotz
Chris, does this bug interest you at all?
Flags: needinfo?(cmanchester)
I'll take a look, thanks!
Assignee: nobody → cmanchester
Flags: needinfo?(cmanchester)
Bug 1131325 - Move system country code from nsIGfxInfo2 to a more appropriate location.
Attachment #8641782 - Flags: review?(aklotz)
Bug 1131325 - Update search service and tests for moving countryCode from gfxInfo2 to sysinfo.
Attachment #8641783 - Flags: review?(aklotz)
Comment on attachment 8641782 [details]
MozReview Request: Bug 1131325 - Move system country code from nsIGfxInfo2 to a more appropriate location.

Bug 1131325 - Move system country code from nsIGfxInfo2 to a more appropriate location.
Comment on attachment 8641783 [details]
MozReview Request: Bug 1131325 - Update search service and tests for moving countryCode from gfxInfo2 to sysinfo.

Bug 1131325 - Update search service and tests for moving countryCode from gfxInfo2 to sysinfo.
Comment on attachment 8641783 [details]
MozReview Request: Bug 1131325 - Update search service and tests for moving countryCode from gfxInfo2 to sysinfo.

https://reviewboard.mozilla.org/r/14607/#review13947

::: toolkit/components/search/nsSearchService.js:609
(Diff revision 2)
> -  // On Mac and Windows, we can get a country code via nsIGfxInfo2
> +  // On Mac and Windows, we can get a country code via sysinfo

This looks ok to me but I'm not a peer of this code. Please request review from Mark Hammond, as it looks like he was involved with the first cut.
Attachment #8641783 - Flags: review?(aklotz)
Comment on attachment 8641782 [details]
MozReview Request: Bug 1131325 - Move system country code from nsIGfxInfo2 to a more appropriate location.

https://reviewboard.mozilla.org/r/14605/#review13945

::: xpcom/base/nsMacHelpers.h:12
(Diff revision 2)
> +using namespace mozilla;

Don't put using statements in headers. Please put GetSelectedCityInfo inside a namespace block instead.

::: xpcom/base/nsMacHelpers.h:7
(Diff revision 2)
> +#ifndef nsMacHelpers_h

For new headers we prefer to avoid the "ns" prefix. Just name it MacHelpers.h. Change the name of the include guard to mozilla_MacHelpers_h.

::: xpcom/base/moz.build:36
(Diff revision 2)
> +    EXPORTS += [

Please change this to EXPORTS.mozilla

::: widget/cocoa/GfxInfo.mm:31
(Diff revision 2)
> -NS_IMPL_ISUPPORTS_INHERITED(GfxInfo, GfxInfoBase, nsIGfxInfo2, nsIGfxInfoDebug)
> +NS_IMPL_ISUPPORTS_INHERITED(GfxInfo, GfxInfoBase, nsIGfxInfoDebug)

Please request smichaud to look over the mac stuff. He should reivew those parts.
Attachment #8641782 - Flags: review?(aklotz)
Comment on attachment 8641782 [details]
MozReview Request: Bug 1131325 - Move system country code from nsIGfxInfo2 to a more appropriate location.

Bug 1131325 - Move system country code from nsIGfxInfo2 to a more appropriate location.
Attachment #8641782 - Flags: review?(smichaud)
Comment on attachment 8641783 [details]
MozReview Request: Bug 1131325 - Update search service and tests for moving countryCode from gfxInfo2 to sysinfo.

Bug 1131325 - Update search service and tests for moving countryCode from gfxInfo2 to sysinfo.
Attachment #8641783 - Flags: review?(markh)
Attachment #8641783 - Flags: review?(markh) → review+
Comment on attachment 8641783 [details]
MozReview Request: Bug 1131325 - Update search service and tests for moving countryCode from gfxInfo2 to sysinfo.

https://reviewboard.mozilla.org/r/14607/#review13967
Comment on attachment 8641782 [details]
MozReview Request: Bug 1131325 - Move system country code from nsIGfxInfo2 to a more appropriate location.

https://reviewboard.mozilla.org/r/14605/#review14523

The Mac-specific changes look fine to me.
Attachment #8641782 - Flags: review?(smichaud) → review+
(Following up comment #14)

Side note -- UI bug in mozreview:

If you've stayed logged in to mozreview for a long time, your session can expire.  When that happens, some paths to publishing a review no longer work, but without displaying any error message.
Comment on attachment 8641782 [details]
MozReview Request: Bug 1131325 - Move system country code from nsIGfxInfo2 to a more appropriate location.

Thanks for the reviews. Filed comment 15 as bug 1195448.

By my count I still need an r+ of the non-Mac parts of this patch.
Attachment #8641782 - Flags: review?(aklotz)
Comment on attachment 8641782 [details]
MozReview Request: Bug 1131325 - Move system country code from nsIGfxInfo2 to a more appropriate location.

https://reviewboard.mozilla.org/r/14605/#review14553

Ship It!
Attachment #8641782 - Flags: review?(aklotz) → review+
https://hg.mozilla.org/mozilla-central/rev/61dd04d5f88e
https://hg.mozilla.org/mozilla-central/rev/1e2a4c7ee236
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.