|
|
Created:
6 years, 10 months ago by robliao Modified:
6 years, 10 months ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd Locale Request Support to Chrome Now
BUG=164227
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252215
Patch Set 1 #
Total comments: 4
Messages
Total messages: 20 (0 generated)
lgtm
vadimt: Please provide owner approval for this CL.
https://codereview.chromium.org/172733002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/172733002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background.js:585: requestParameters += '&uiLocale=' + navigator.language; https://developer.mozilla.org/en-US/docs/Web/API/NavigatorLanguage.language This says it's based on acceptlanguage? Have you tested this? It might just be out of date.
https://codereview.chromium.org/172733002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/172733002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background.js:585: requestParameters += '&uiLocale=' + navigator.language; Works for me. accept-language:en-US,en;q=0.8,de;q=0.6,de-DE;q=0.4,zh-TW;q=0.2,zh;q=0.2,ko;q=0.2 navigator.language = "ko" Current UI language = "ko" On 2014/02/19 19:18:14, rgustafson wrote: > https://developer.mozilla.org/en-US/docs/Web/API/NavigatorLanguage.language > > This says it's based on acceptlanguage? Have you tested this? It might just be > out of date.
lgtm https://codereview.chromium.org/172733002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/172733002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background.js:585: requestParameters += '&uiLocale=' + navigator.language; On 2014/02/19 19:26:47, robliao wrote: > Works for me. > > accept-language:en-US,en;q=0.8,de;q=0.6,de-DE;q=0.4,zh-TW;q=0.2,zh;q=0.2,ko;q=0.2 > > navigator.language = "ko" > > Current UI language = "ko" > > On 2014/02/19 19:18:14, rgustafson wrote: > > https://developer.mozilla.org/en-US/docs/Web/API/NavigatorLanguage.language > > > > This says it's based on acceptlanguage? Have you tested this? It might just be > > out of date. > Do you know the actual standard that these strings follow is since apparently the documentation isn't that great? From what I gather it's RFC 5646, but this is important so we don't choke on parsing this serverside.
https://codereview.chromium.org/172733002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/172733002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background.js:585: requestParameters += '&uiLocale=' + navigator.language; A bit more context. The reason why this works is because we get no preferred languages while getting navigator.language. Haven't dug into why. This causes us to fall into getting the command line langauge 3:035> k20 ChildEBP RetAddr 0018c094 11b9c660 content!content::RenderThreadImpl::GetLocale+0xb5 0018c120 1b3c70e6 content!content::RendererWebKitPlatformSupportImpl::defaultLocale+0x60 0018c168 1b3c6f2c blink_platform!WebCore::platformLanguage+0xf6 0018c19c 1b3c6c0d blink_platform!WebCore::userPreferredLanguages+0x9c 0018c1d4 165b36ab blink_platform!WebCore::defaultLanguage+0x4d 0018c1ec 1522a0b2 blink_web!WebCore::Navigator::language+0x2b 0018c228 1522a019 blink_web!WebCore::NavigatorV8Internal::languageAttributeGetter+0x72 0018c234 0f905534 blink_web!WebCore::NavigatorV8Internal::languageAttributeGetterCallback+0x19 0018c270 0f6c0f44 v8!v8::internal::PropertyCallbackArguments::Call+0x74 Sure enough, our command line has CommandLine: '"D:\src\out\Debug\chrome.exe" --type=renderer --lang=ko --force-fieldtrials=[Field Trials] --extension-process --enable-threaded-compositing --enable-delegated-renderer --enable-deadline-scheduling --enable-software-compositing --channel=[Channel] So who sets --lang? That gets set up here: const CommandLinePrefStore::StringSwitchToPreferenceMapEntry CommandLinePrefStore::string_switch_map_[] = { { switches::kLang, prefs::kApplicationLocale } On 2014/02/19 19:39:16, rgustafson wrote: > On 2014/02/19 19:26:47, robliao wrote: > > Works for me. > > > > > accept-language:en-US,en;q=0.8,de;q=0.6,de-DE;q=0.4,zh-TW;q=0.2,zh;q=0.2,ko;q=0.2 > > > > navigator.language = "ko" > > > > Current UI language = "ko" > > > > On 2014/02/19 19:18:14, rgustafson wrote: > > > https://developer.mozilla.org/en-US/docs/Web/API/NavigatorLanguage.language > > > > > > This says it's based on acceptlanguage? Have you tested this? It might just > be > > > out of date. > > > > Do you know the actual standard that these strings follow is since apparently > the documentation isn't that great? From what I gather it's RFC 5646, but this > is important so we don't choke on parsing this serverside.
The CQ bit was checked by robliao@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/172733002/1
The CQ bit was unchecked by robliao@chromium.org
The CQ bit was checked by robliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/172733002/1
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/172733002/1
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/172733002/1
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/172733002/1
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/172733002/1
Message was sent while issue was closed.
Change committed as 252215 |