Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1419033008:
Extract language settings methods into a LanguageHelper interface (Closed)
|
Created:
5 years, 1 month ago by michaelpg Modified:
5 years ago Reviewers:
Dan Beam CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, arv+watch_chromium.org, vitalyp+closure_chromium.org, dbeam+watch-settings_chromium.org, dbeam+watch-closure_chromium.org, stevenjb+watch-md-settings_chromium.org, jlklein+watch-closure_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@LanguagePage5InputMethodsAPI Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtract language settings methods into a LanguageHelper interface
and pass the singleton directly to host elements instead of forwarding
every method through a settings-languages instance, because that is
tedious.
BUG=512479
Committed: https://crrev.com/a49c98bb68739710dfac2ebbad7281fe96b9ac53
Cr-Commit-Position: refs/heads/master@{#363284}
Patch Set 1 #Patch Set 2 : #
Total comments: 8
Patch Set 3 : addSingletonGetter #Patch Set 4 : undo unrelated changes #Patch Set 5 : fix comments #Patch Set 6 : cr.exportPath #
Total comments: 8
Patch Set 7 : dbeam fb #
Total comments: 19
Patch Set 8 : closer #Patch Set 9 : rebase #Patch Set 10 : dbeam feedback again -- cf patch 7 #Patch Set 11 : 100% less rebase error #
Total comments: 14
Patch Set 12 : fixes, rename LanguageHelper[Impl] #
Total comments: 12
Patch Set 13 : nit #Dependent Patchsets: Messages
Total messages: 35 (9 generated)
PTAL.
michaelpg@chromium.org changed reviewers: + dbeam@chromium.org
PTAL.
so you call this a singleton, and use it semi-globally, so why not just ... use cr.addSingletonGetter() and call settings.LanguageHelper.getInstance().doSomething(); or cr.makePublic([... list of methods ...]); and then settings.LanguageHelper.doSomething(); ? does this need to be injected for testing? does this need to be bound somehow to Polymer in HTML or JS that it's not possible to just implement a singleton like the rest of WebUI/closure does? your CL description explains *what* you're doing but not /why/. which part of the "language and IMEs" (where BUG= points) does this help implement? https://codereview.chromium.org/1419033008/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/languages_page/languages_types.js (right): https://codereview.chromium.org/1419033008/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_types.js:33: * }} indent off https://codereview.chromium.org/1419033008/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_types.js:51: LanguageSettingsHelper.prototype.setUILanguage = function(languageCode) {}; why not use .prototype = {...}?
On 2015/11/03 04:42:17, Dan Beam wrote: > so you call this a singleton, and use it semi-globally, so why not just ... use > cr.addSingletonGetter() and call > > settings.LanguageHelper.getInstance().doSomething(); > > or > > cr.makePublic([... list of methods ...]); > > and then > > settings.LanguageHelper.doSomething(); > Either way would probably also work. I'll look into it. > ? does this need to be injected for testing? does this need to be bound > somehow to Polymer in HTML or JS that it's not possible to just implement a > singleton like the rest of WebUI/closure does? Well, yes, the singleton Polymer element has local DOM in order to be data bound to prefs: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... But that is probably unrelated to the question of how the singleton gets exposed. The `languagesHelper` property is just a convenient way to expose the singleton to elements which already have a <settings-languages>. It sounds like you would prefer I stick with the existing model for singletons that WebUI already has which is fine too. > > your CL description explains *what* you're doing but not /why/. which part of > the "language and IMEs" (where BUG= points) does this help implement? I added "because that is tedious" to the CL description. See also all the dummy implementations I removed. > > https://codereview.chromium.org/1419033008/diff/20001/chrome/browser/resource... > File chrome/browser/resources/settings/languages_page/languages_types.js > (right): > > https://codereview.chromium.org/1419033008/diff/20001/chrome/browser/resource... > chrome/browser/resources/settings/languages_page/languages_types.js:33: * }} > indent off > > https://codereview.chromium.org/1419033008/diff/20001/chrome/browser/resource... > chrome/browser/resources/settings/languages_page/languages_types.js:51: > LanguageSettingsHelper.prototype.setUILanguage = function(languageCode) {}; > why not use .prototype = {...}?
Description was changed from ========== Extract language settings methods into a LanguageSettingsHelper interface and pass the singleton directly to host elements instead of forwarding every method through a settings-languages instance. BUG=512479 ========== to ========== Extract language settings methods into a LanguageSettingsHelper interface and pass the singleton directly to host elements instead of forwarding every method through a settings-languages instance, because that is tedious. BUG=512479 ==========
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
PTAL. Patch 3 uses a singleton instead of exposing languageHelper as a property (although you'll see I was lazy and continue to cache it as a property on the prototype). Patch 4 reverts some unrelated changes. https://codereview.chromium.org/1419033008/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/languages_page/languages_types.js (right): https://codereview.chromium.org/1419033008/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_types.js:33: * }} On 2015/11/03 04:42:17, Dan Beam wrote: > indent off Done. https://codereview.chromium.org/1419033008/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_types.js:51: LanguageSettingsHelper.prototype.setUILanguage = function(languageCode) {}; On 2015/11/03 04:42:17, Dan Beam wrote: > why not use .prototype = {...}? Just tried it, but then I get: ** Presubmit Warnings ** Found JavaScript style violations in chrome/browser/resources/settings/languages_page/languages_types.js: line 62: E0218: Found @return JsDoc on function that returns nothing * @return {boolean} ^^^^^^^ x4.
ping
is having a singleton element a standard polymer pattern? is there some page I can see in all its glory about this? https://codereview.chromium.org/1419033008/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/languages_page/languages_types.js (right): https://codereview.chromium.org/1419033008/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_types.js:51: LanguageSettingsHelper.prototype.setUILanguage = function(languageCode) {}; On 2015/11/04 03:52:27, michaelpg wrote: > On 2015/11/03 04:42:17, Dan Beam wrote: > > why not use .prototype = {...}? > > Just tried it, but then I get: > > ** Presubmit Warnings ** > Found JavaScript style violations in > chrome/browser/resources/settings/languages_page/languages_types.js: > line 62: E0218: Found @return JsDoc on function that returns nothing > * @return {boolean} > ^^^^^^^ > > x4. https://gist.github.com/anonymous/23c3aba4def2a254ee54 works for me https://codereview.chromium.org/1419033008/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages.js (right): https://codereview.chromium.org/1419033008/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:281: // TODO(michaelpg): replace duplicate docs with @inheritDoc once b/24294625 inheritDoc is dead, I think you mean @override https://codereview.chromium.org/1419033008/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:290: if (cr.isWindows || cr.isChromeOS) can this be assert(cr.isWindows || cr.isChromeOS) instead? https://codereview.chromium.org/1419033008/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:458: * @type {{getInstance: function(): LanguageSettingsHelper}} this @type should not be necessary... ChromePass should pick up the addSingletonGetter call https://codereview.chromium.org/1419033008/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:460: settings.LanguageHelper = SettingsLanguagesSingletonElement; why is this better than just defining the singleton as a ... class? (rather than a Polymer element) if you need to listen to events on it, it can @extend {EventTarget}
ptal. singleton documentation forthcoming. for now, zzzz https://codereview.chromium.org/1419033008/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/languages_page/languages_types.js (right): https://codereview.chromium.org/1419033008/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_types.js:51: LanguageSettingsHelper.prototype.setUILanguage = function(languageCode) {}; On 2015/11/11 07:34:21, Dan Beam wrote: > On 2015/11/04 03:52:27, michaelpg wrote: > > On 2015/11/03 04:42:17, Dan Beam wrote: > > > why not use .prototype = {...}? > > > > Just tried it, but then I get: > > > > ** Presubmit Warnings ** > > Found JavaScript style violations in > > chrome/browser/resources/settings/languages_page/languages_types.js: > > line 62: E0218: Found @return JsDoc on function that returns nothing > > * @return {boolean} > > ^^^^^^^ > > > > x4. > > https://gist.github.com/anonymous/23c3aba4def2a254ee54 works for me Still warns: Found JavaScript style violations in chrome/browser/resources/settings/languages_page/dan.js: line 5: E0218: Found @return JsDoc on function that returns nothing /** @return {boolean} */ ^^^^^^^ https://codereview.chromium.org/1419033008/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages.js (right): https://codereview.chromium.org/1419033008/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:281: // TODO(michaelpg): replace duplicate docs with @inheritDoc once b/24294625 On 2015/11/11 07:34:21, Dan Beam wrote: > inheritDoc is dead, I think you mean @override Done. https://codereview.chromium.org/1419033008/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:290: if (cr.isWindows || cr.isChromeOS) On 2015/11/11 07:34:21, Dan Beam wrote: > can this be assert(cr.isWindows || cr.isChromeOS) instead? Done. https://codereview.chromium.org/1419033008/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:458: * @type {{getInstance: function(): LanguageSettingsHelper}} On 2015/11/11 07:34:21, Dan Beam wrote: > this @type should not be necessary... ChromePass should pick up the > addSingletonGetter call Done. Weird, I was getting an error before. https://codereview.chromium.org/1419033008/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:460: settings.LanguageHelper = SettingsLanguagesSingletonElement; On 2015/11/11 07:34:21, Dan Beam wrote: > why is this better than just defining the singleton as a ... class? (rather > than a Polymer element) > > if you need to listen to events on it, it can @extend {EventTarget} I'll write up documentation, but having it as a Polymer element gives us data binding awesomesauce we would otherwise have to do manually. We should review the model with the Polymer folks and see if they think it's cool or horrible or whatever. That being said, the fact that the thing being singletonized is a Polymer element is tangential to this CL, IMO. https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages.js (right): https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:27: cr.exportPath('settings'); for closure
closer https://codereview.chromium.org/1419033008/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/languages_page/languages_types.js (right): https://codereview.chromium.org/1419033008/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_types.js:51: LanguageSettingsHelper.prototype.setUILanguage = function(languageCode) {}; On 2015/11/13 05:30:17, michaelpg wrote: > On 2015/11/11 07:34:21, Dan Beam wrote: > > On 2015/11/04 03:52:27, michaelpg wrote: > > > On 2015/11/03 04:42:17, Dan Beam wrote: > > > > why not use .prototype = {...}? > > > > > > Just tried it, but then I get: > > > > > > ** Presubmit Warnings ** > > > Found JavaScript style violations in > > > chrome/browser/resources/settings/languages_page/languages_types.js: > > > line 62: E0218: Found @return JsDoc on function that returns nothing > > > * @return {boolean} > > > ^^^^^^^ > > > > > > x4. > > > > https://gist.github.com/anonymous/23c3aba4def2a254ee54 works for me > > Still warns: > > Found JavaScript style violations in > chrome/browser/resources/settings/languages_page/dan.js: > line 5: E0218: Found @return JsDoc on function that returns nothing > /** @return {boolean} */ > ^^^^^^^ then do assertNotReached(); return false; https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages.js (right): https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:459: settings.LanguageHelper = SettingsLanguagesSingletonElement; why not cr.exportPath() here? https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:460: cr.addSingletonGetter(settings.LanguageHelper); do you need to addSingletonGetter() to the settings. namespaced version? https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:493: this._setLanguages(this.singleton_.languages); why are you calling a private Polymer method? https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages_page.js (right): https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages_page.js:70: e.target.checked); indent off https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages_page.js:116: return (cr.isWindows || cr.isChromeOS) && can you somehow explain why this only happens on Windows/ChromeOS? i.e. var platformHasSpellchecking or var respectsUILanguage or a comment, or a private method, or...
PTAL. https://codereview.chromium.org/1419033008/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/languages_page/languages_types.js (right): https://codereview.chromium.org/1419033008/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_types.js:51: LanguageSettingsHelper.prototype.setUILanguage = function(languageCode) {}; On 2015/11/18 18:06:01, Dan Beam wrote: > On 2015/11/13 05:30:17, michaelpg wrote: > > On 2015/11/11 07:34:21, Dan Beam wrote: > > > On 2015/11/04 03:52:27, michaelpg wrote: > > > > On 2015/11/03 04:42:17, Dan Beam wrote: > > > > > why not use .prototype = {...}? > > > > > > > > Just tried it, but then I get: > > > > > > > > ** Presubmit Warnings ** > > > > Found JavaScript style violations in > > > > chrome/browser/resources/settings/languages_page/languages_types.js: > > > > line 62: E0218: Found @return JsDoc on function that returns nothing > > > > * @return {boolean} > > > > ^^^^^^^ > > > > > > > > x4. > > > > > > https://gist.github.com/anonymous/23c3aba4def2a254ee54 works for me > > > > Still warns: > > > > Found JavaScript style violations in > > chrome/browser/resources/settings/languages_page/dan.js: > > line 5: E0218: Found @return JsDoc on function that returns nothing > > /** @return {boolean} */ > > ^^^^^^^ > > then do > > assertNotReached(); > return false; ah, actually setting every function = assertNotReached works for the presubmit and for closure. https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages.js (right): https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:459: settings.LanguageHelper = SettingsLanguagesSingletonElement; On 2015/11/18 18:06:01, Dan Beam wrote: > why not cr.exportPath() here? Why cr.exportPath() here? https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:460: cr.addSingletonGetter(settings.LanguageHelper); On 2015/11/18 18:06:01, Dan Beam wrote: > do you need to addSingletonGetter() to the settings. namespaced version? Not if we make settings.LanguageHelper point to the singleton itself. Done. https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:493: this._setLanguages(this.singleton_.languages); On 2015/11/18 18:06:01, Dan Beam wrote: > why are you calling a private Polymer method? this is how you set a readOnly property. https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages_page.js (right): https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages_page.js:70: e.target.checked); On 2015/11/18 18:06:01, Dan Beam wrote: > indent off Done. https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages_page.js:116: return (cr.isWindows || cr.isChromeOS) && On 2015/11/18 18:06:01, Dan Beam wrote: > can you somehow explain why this only happens on Windows/ChromeOS? i.e. > > var platformHasSpellchecking > > or > > var respectsUILanguage > > or a comment, or a private method, or... Done.
https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages.js (right): https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:459: settings.LanguageHelper = SettingsLanguagesSingletonElement; On 2015/11/22 00:22:40, michaelpg wrote: > On 2015/11/18 18:06:01, Dan Beam wrote: > > why not cr.exportPath() here? > > Why cr.exportPath() here? as low as possible https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:460: cr.addSingletonGetter(settings.LanguageHelper); On 2015/11/22 00:22:40, michaelpg wrote: > On 2015/11/18 18:06:01, Dan Beam wrote: > > do you need to addSingletonGetter() to the settings. namespaced version? > > Not if we make settings.LanguageHelper point to the singleton itself. Done. why is this better than addSingletonGetter? why do you hate committing code? https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:493: this._setLanguages(this.singleton_.languages); On 2015/11/22 00:22:40, michaelpg wrote: > On 2015/11/18 18:06:01, Dan Beam wrote: > > why are you calling a private Polymer method? > > this is how you set a readOnly property. Acknowledged. (though mildly odd to me, guess it's kind of like @synthesize in Cocoa)
Dan: https://media.giphy.com/media/E87jjnSCANThe/giphy.gif https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages.js (right): https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:459: settings.LanguageHelper = SettingsLanguagesSingletonElement; On 2015/11/23 20:14:10, Dan Beam wrote: > On 2015/11/22 00:22:40, michaelpg wrote: > > On 2015/11/18 18:06:01, Dan Beam wrote: > > > why not cr.exportPath() here? > > > > Why cr.exportPath() here? > > as low as possible Oh, I interpreted "why not cr.exportPath() here?" as "Why not change this line here to use cr.exportPath to define settings.LanguageHelper?" Given your latest response, now I think you mean that line 27 (cr.exportPath('settings')) should be down here, because you prefer it be called as late as possible. Correct? https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:460: cr.addSingletonGetter(settings.LanguageHelper); On 2015/11/23 20:14:10, Dan Beam wrote: > On 2015/11/22 00:22:40, michaelpg wrote: > > On 2015/11/18 18:06:01, Dan Beam wrote: > > > do you need to addSingletonGetter() to the settings. namespaced version? > > > > Not if we make settings.LanguageHelper point to the singleton itself. Done. > > why is this better than addSingletonGetter? Sorry, but: Huh? When you asked if I "needed" to addSingletonGetter(), I assumed that meant it would be preferable if I didn't unless I had to. And I don't have to, as I demonstrated in the updated CL. Now you're asking why my update is better than addSingletonGetter. So, I'm confused. I will now attempt to answer your questions directly because the update-feedback-update cycle isn't getting us anywhere. 1. do you need to addSingletonGetter() to the settings. namespaced version? No. This uses addSingletonGetter() to lazily create the LanguageHelper when needed while allowing other objects to reference it without worrying about whether it has been created yet. It's a common pattern as I understand it, but it is not a necessity. 2. why is this better than addSingletonGetter? Well, setting and accessing it directly is a bit less verbose. 3. why do you hate committing code? ....?
https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages.js (right): https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:459: settings.LanguageHelper = SettingsLanguagesSingletonElement; On 2015/11/23 20:26:56, michaelpg wrote: > On 2015/11/23 20:14:10, Dan Beam wrote: > > On 2015/11/22 00:22:40, michaelpg wrote: > > > On 2015/11/18 18:06:01, Dan Beam wrote: > > > > why not cr.exportPath() here? > > > > > > Why cr.exportPath() here? > > > > as low as possible > > Oh, I interpreted "why not cr.exportPath() here?" as "Why not change this line > here to use cr.exportPath to define settings.LanguageHelper?" Given your latest > response, now I think you mean that line 27 (cr.exportPath('settings')) should > be down here, because you prefer it be called as late as possible. Correct? yes https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:460: cr.addSingletonGetter(settings.LanguageHelper); On 2015/11/23 20:26:56, michaelpg wrote: > On 2015/11/23 20:14:10, Dan Beam wrote: > > On 2015/11/22 00:22:40, michaelpg wrote: > > > On 2015/11/18 18:06:01, Dan Beam wrote: > > > > do you need to addSingletonGetter() to the settings. namespaced version? > > > > > > Not if we make settings.LanguageHelper point to the singleton itself. Done. > > > > why is this better than addSingletonGetter? > > Sorry, but: Huh? When you asked if I "needed" to addSingletonGetter(), I assumed > that meant it would be preferable if I didn't unless I had to. And I don't have > to, as I demonstrated in the updated CL. > > Now you're asking why my update is better than addSingletonGetter. So, I'm > confused. > > I will now attempt to answer your questions directly because the > update-feedback-update cycle isn't getting us anywhere. > > 1. do you need to addSingletonGetter() to the settings. namespaced version? > > No. This uses addSingletonGetter() to lazily create the LanguageHelper when > needed while allowing other objects to reference it without worrying about > whether it has been created yet. It's a common pattern as I understand it, but > it is not a necessity. You were doing this: cr.addSingletonGetter(settings.Blah); I wanted: cr.addSingletonGetter(Blah); because the Chrome-specific closure compilation pass is fragile when it comes to type detection and this is how most modules do it: cr.define('namespace', function() { function Blah() {} cr.addSingletonGetter(Blah); return {Blah: Blah}; }); > > 2. why is this better than addSingletonGetter? > > Well, setting and accessing it directly is a bit less verbose. "a bit less" is generally not a good idea to add a new variation to the code base, IMO > > 3. why do you hate committing code? > > ....? continuing to make new paradigms will slow down my approval as I need to understand what's *so much better* about how your new way does things that you're adding a permutation to the codebase (and therefore muddying the waters for future folks to know which way to go).
Patchset #10 (id:220001) has been deleted
PTAL -- compare with patch set 7 to see the changes you actually wanted. Thanks. https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages.js (right): https://codereview.chromium.org/1419033008/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:460: cr.addSingletonGetter(settings.LanguageHelper); On 2015/11/23 20:59:50, Dan Beam wrote: > On 2015/11/23 20:26:56, michaelpg wrote: > > On 2015/11/23 20:14:10, Dan Beam wrote: > > > On 2015/11/22 00:22:40, michaelpg wrote: > > > > On 2015/11/18 18:06:01, Dan Beam wrote: > > > > > do you need to addSingletonGetter() to the settings. namespaced version? > > > > > > > > Not if we make settings.LanguageHelper point to the singleton itself. > Done. > > > > > > why is this better than addSingletonGetter? > > > > Sorry, but: Huh? When you asked if I "needed" to addSingletonGetter(), I > assumed > > that meant it would be preferable if I didn't unless I had to. And I don't > have > > to, as I demonstrated in the updated CL. > > > > Now you're asking why my update is better than addSingletonGetter. So, I'm > > confused. > > > > I will now attempt to answer your questions directly because the > > update-feedback-update cycle isn't getting us anywhere. > > > > 1. do you need to addSingletonGetter() to the settings. namespaced version? > > > > No. This uses addSingletonGetter() to lazily create the LanguageHelper when > > needed while allowing other objects to reference it without worrying about > > whether it has been created yet. It's a common pattern as I understand it, but > > it is not a necessity. > > You were doing this: > > cr.addSingletonGetter(settings.Blah); > > I wanted: > > cr.addSingletonGetter(Blah); > > because the Chrome-specific closure compilation pass is fragile when it comes to > type detection and this is how most modules do it: > > cr.define('namespace', function() { > function Blah() {} > cr.addSingletonGetter(Blah); > return {Blah: Blah}; > }); > > > > > 2. why is this better than addSingletonGetter? > > > > Well, setting and accessing it directly is a bit less verbose. > > "a bit less" is generally not a good idea to add a new variation to the code > base, IMO > > > > > 3. why do you hate committing code? > > > > ....? > > continuing to make new paradigms will slow down my approval as I need to > understand what's *so much better* about how your new way does things that > you're adding a permutation to the codebase (and therefore muddying the waters > for future folks to know which way to go). OK, I changed it to what you wanted originally. I had no intention of creating my own paradigm.
https://codereview.chromium.org/1419033008/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/language_detail_page.js (right): https://codereview.chromium.org/1419033008/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/language_detail_page.js:38: /** @private {LanguageSettingsHelper} */ nit: add ! as this isn't nullable https://codereview.chromium.org/1419033008/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages.js (right): https://codereview.chromium.org/1419033008/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:56: * @implements {LanguageSettingsHelper} does this now compile? @implements on top of Polymer()? https://codereview.chromium.org/1419033008/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:58: var SettingsLanguagesSingletonElement = Polymer({ why can't this just be LanguageHelper? why does it matter that it's implemented by Polymer()? https://codereview.chromium.org/1419033008/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:280: // LanguageSettingsHelper implementation. should this be LanguageHelper? https://codereview.chromium.org/1419033008/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:457: cr.addSingletonGetter(LanguageHelper); so addSingletonGetter [essentially] does this: cr.addSingletonGetter = function(ctor) { ctor.getInstance = function() { return ctor.instance_ || (ctor.instance_ = new ctor()); }; }; and is typically invoked like this: cr.addSingletonGetter(ClassName); if you call it with the equivalent of addSingletonGetter(new Thing) you're essentially doing: new new Thing which seems unexpected to me... [1] https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... https://codereview.chromium.org/1419033008/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:469: * @type {!SettingsLanguagesSingletonElement} so here you're intentionally using the concrete type rather than the interface, right? https://codereview.chromium.org/1419033008/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages_types.js (right): https://codereview.chromium.org/1419033008/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages_types.js:44: var LanguageSettingsHelper = function() {}; typically you see less-specific names for interfaces in Chrome, i.e. Thinger and ThingerImpl where Thinger is the interface and ThingerImpl is it's implementation. Polymer does something similar, I think, by just adding "Impl" when you have to do the dirty work. i understand you needed 2 different names here, but I'm wondering why "Settings" is more appropriate here? could we do LanguageHelper (interface) + LanguageHelperImpl (concrete)? LanguageHelper (interface) + LanguageHelperSingleton (concrete)?
https://codereview.chromium.org/1419033008/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/language_detail_page.js (right): https://codereview.chromium.org/1419033008/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/language_detail_page.js:38: /** @private {LanguageSettingsHelper} */ On 2015/11/25 00:29:38, Dan Beam wrote: > nit: add ! as this isn't nullable Done. https://codereview.chromium.org/1419033008/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages.js (right): https://codereview.chromium.org/1419033008/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:56: * @implements {LanguageSettingsHelper} On 2015/11/25 00:29:38, Dan Beam wrote: > does this now compile? @implements on top of Polymer()? Compile, yes, does it have an effect, no. See the TODO below. Since this doesn't do anything we could leave it out, but I'm tempted to leave it in so that once rictic@'s bug is fixed it would "kick in". https://codereview.chromium.org/1419033008/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:58: var SettingsLanguagesSingletonElement = Polymer({ On 2015/11/25 00:29:38, Dan Beam wrote: > why can't this just be LanguageHelper? why does it matter that it's implemented > by Polymer()? to avoid inconsistency, because the Polymer pass names the @type of an element using UpperCaseTagNameElement unless the Polymer() call is used to set a variable, in which case the variable name is the @type. https://codereview.chromium.org/1419033008/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:280: // LanguageSettingsHelper implementation. On 2015/11/25 00:29:38, Dan Beam wrote: > should this be LanguageHelper? Does renaming LSH -> LH makes sense? Yes. Should the comment have referenced the variable instead of the interface? no. LanguageSettingsHelper was the name of the type/interface. The whole element was supposed to be "cast" to a LanguageSettingsHelper "typed" variable called LanguageHelper. Or in the new lingo, the element is cast to a LanguageHelper type in the variable LanguageHelperImpl. https://codereview.chromium.org/1419033008/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:457: cr.addSingletonGetter(LanguageHelper); On 2015/11/25 00:29:38, Dan Beam wrote: > so addSingletonGetter [essentially] does this: > > cr.addSingletonGetter = function(ctor) { > ctor.getInstance = function() { > return ctor.instance_ || (ctor.instance_ = new ctor()); > }; > }; > > and is typically invoked like this: > > cr.addSingletonGetter(ClassName); > > if you call it with the equivalent of > > addSingletonGetter(new Thing) > > you're essentially doing: > > new new Thing > > which seems unexpected to me... > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... I clearly tested the wrong executable, sorry. https://codereview.chromium.org/1419033008/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:469: * @type {!SettingsLanguagesSingletonElement} On 2015/11/25 00:29:38, Dan Beam wrote: > so here you're intentionally using the concrete type rather than the interface, > right? yes, because this element interacts with the <settings-languages-singleton> as a Polymer element (e.g., listens to it, reads its the languages property). https://codereview.chromium.org/1419033008/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages_types.js (right): https://codereview.chromium.org/1419033008/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages_types.js:44: var LanguageSettingsHelper = function() {}; On 2015/11/25 00:29:38, Dan Beam wrote: > typically you see less-specific names for interfaces in Chrome, i.e. Thinger and > ThingerImpl where Thinger is the interface and ThingerImpl is it's > implementation. Polymer does something similar, I think, by just adding "Impl" > when you have to do the dirty work. > > i understand you needed 2 different names here, but I'm wondering why "Settings" > is more appropriate here? could we do LanguageHelper (interface) + > LanguageHelperImpl (concrete)? LanguageHelper (interface) + > LanguageHelperSingleton (concrete)? good ideas, thanks https://codereview.chromium.org/1419033008/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages.js (right): https://codereview.chromium.org/1419033008/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:27: var SettingsLanguagesSingletonElement; Warning: the last patch had a bug where LanguageHelper was defined inside an IIFE. This is a hacky fix: end the IIFE before defining LanguageHelper, and declare SettingsLanguagesSingletonElement outside the IIFE. Alternatives: * don't use an IIFE, just have the constants below in the global scope * declare LanguageHelper here instead of SettingsLanguagesSingletonElement, then set it to SettingsLanguagesSingletonElement in the IIFE (meh) * define LanguageHelper on a namespace like settings (back to where we started...)
lgtm w/nits https://codereview.chromium.org/1419033008/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages.js (right): https://codereview.chromium.org/1419033008/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:27: var SettingsLanguagesSingletonElement; On 2015/11/25 06:20:51, michaelpg wrote: > Warning: the last patch had a bug where LanguageHelper was defined inside an > IIFE. This is a hacky fix: end the IIFE before defining LanguageHelper, and > declare SettingsLanguagesSingletonElement outside the IIFE. > > Alternatives: > * don't use an IIFE, just have the constants below in the global scope > * declare LanguageHelper here instead of SettingsLanguagesSingletonElement, then > set it to SettingsLanguagesSingletonElement in the IIFE (meh) > * define LanguageHelper on a namespace like settings (back to where we > started...) this is fine https://codereview.chromium.org/1419033008/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:143: return; nit: \n https://codereview.chromium.org/1419033008/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:459: var LanguageHelperImpl = SettingsLanguagesSingletonElement; i still don't really understand why we need this step... it just seems to introduce an extra name for no reason https://codereview.chromium.org/1419033008/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages_page.js (right): https://codereview.chromium.org/1419033008/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages_page.js:56: if (!cr.isChromeOS && !cr.isWindows) is there a rhyme or reason as to when we use cr.isThing vs. <if expr="is_thing">? i don't really see one...
Description was changed from ========== Extract language settings methods into a LanguageSettingsHelper interface and pass the singleton directly to host elements instead of forwarding every method through a settings-languages instance, because that is tedious. BUG=512479 ========== to ========== Extract language settings methods into a LanguageHelper interface and pass the singleton directly to host elements instead of forwarding every method through a settings-languages instance, because that is tedious. BUG=512479 ==========
thanks, LMK if you want me to get rid of the extra LanguageHelperImpl name, otherwise i'll check da box. https://codereview.chromium.org/1419033008/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages.js (right): https://codereview.chromium.org/1419033008/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:143: return; On 2015/12/01 04:17:44, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/1419033008/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:459: var LanguageHelperImpl = SettingsLanguagesSingletonElement; On 2015/12/01 04:17:44, Dan Beam wrote: > i still don't really understand why we need this step... it just seems to > introduce an extra name for no reason It's not a dealbreaker for the CL. But the reason for the extra name is to minimize the perceived surface area of the element when it's used by other elements, providing the LanguageHelper interface via a LanguageHelper "pointer" but hiding the PolymerElement interface. For example: SettingsLanguageSingletonElement.getInstance().set('languages.enabled', [...]); is not how the element is supposed to be used, but: LanguageHelperImpl.getInstance().enableLanguage('ar'); works. Also, if we decide in the future to not use this weird singleton polymer pattern, we can still keep the interface and not have to change the other files that use LanguageHelperImpl. (Spoiler alert: this will probably happen to some degree.) https://codereview.chromium.org/1419033008/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages_page.js (right): https://codereview.chromium.org/1419033008/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages_page.js:56: if (!cr.isChromeOS && !cr.isWindows) On 2015/12/01 04:17:44, Dan Beam wrote: > is there a rhyme or reason as to when we use cr.isThing vs. <if > expr="is_thing">? i don't really see one... yeah, i use <if> to exclude whole functions that don't make sense on other platforms, and if() within functions. <if> around functions makes them illegal to call on other platforms. <if> within functions makes them weirder to reason about.
lgtm https://codereview.chromium.org/1419033008/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages.js (right): https://codereview.chromium.org/1419033008/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:459: var LanguageHelperImpl = SettingsLanguagesSingletonElement; On 2015/12/02 21:06:06, michaelpg wrote: > On 2015/12/01 04:17:44, Dan Beam wrote: > > i still don't really understand why we need this step... it just seems to > > introduce an extra name for no reason > > It's not a dealbreaker for the CL. But the reason for the extra name is to > minimize the perceived surface area of the element when it's used by other > elements, providing the LanguageHelper interface via a LanguageHelper "pointer" > but hiding the PolymerElement interface. For example: > > SettingsLanguageSingletonElement.getInstance().set('languages.enabled', [...]); > > is not how the element is supposed to be used, but: > > LanguageHelperImpl.getInstance().enableLanguage('ar'); > > works. > > Also, if we decide in the future to not use this weird singleton polymer > pattern, we can still keep the interface and not have to change the other files > that use LanguageHelperImpl. (Spoiler alert: this will probably happen to some > degree.) so basically you want both names to be the most technically correct (because you want to name it "Element" because Polymer() creates things that are instanceof Element but also "Helper"). is this really necessary? Like I mentioned: the fact that Polymer() is used to implement the Helper[Impl] seems unimportant. https://codereview.chromium.org/1419033008/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages_page.js (right): https://codereview.chromium.org/1419033008/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages_page.js:56: if (!cr.isChromeOS && !cr.isWindows) On 2015/12/02 21:06:06, michaelpg wrote: > On 2015/12/01 04:17:44, Dan Beam wrote: > > is there a rhyme or reason as to when we use cr.isThing vs. <if > > expr="is_thing">? i don't really see one... > > yeah, i use <if> to exclude whole functions that don't make sense on other > platforms, and if() within functions. > > <if> around functions makes them illegal to call on other platforms. <if> within > functions makes them weirder to reason about. fwiw: void DoThing() { #if defined(OS_CHROME) ... #endif } is not uncommon in C++
https://codereview.chromium.org/1419033008/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages.js (right): https://codereview.chromium.org/1419033008/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:459: var LanguageHelperImpl = SettingsLanguagesSingletonElement; On 2015/12/02 22:21:15, Dan Beam wrote: > On 2015/12/02 21:06:06, michaelpg wrote: > > On 2015/12/01 04:17:44, Dan Beam wrote: > > > i still don't really understand why we need this step... it just seems to > > > introduce an extra name for no reason > > > > It's not a dealbreaker for the CL. But the reason for the extra name is to > > minimize the perceived surface area of the element when it's used by other > > elements, providing the LanguageHelper interface via a LanguageHelper > "pointer" > > but hiding the PolymerElement interface. For example: > > > > SettingsLanguageSingletonElement.getInstance().set('languages.enabled', > [...]); > > > > is not how the element is supposed to be used, but: > > > > LanguageHelperImpl.getInstance().enableLanguage('ar'); > > > > works. > > > > Also, if we decide in the future to not use this weird singleton polymer > > pattern, we can still keep the interface and not have to change the other > files > > that use LanguageHelperImpl. (Spoiler alert: this will probably happen to some > > degree.) > > so basically you want both names to be the most technically correct (because you > want to name it "Element" because Polymer() creates things that are instanceof > Element but also "Helper"). is this really necessary? Like I mentioned: the > fact that Polymer() is used to implement the Helper[Impl] seems unimportant. Oh, no, the reason I want to name it "Element" is because this is the type name Closure's PolymerPass would create by default if I didn't assign the Polymer call to a value, and I don't want to stray from that pattern. I don't particularly care that the thing derives from Polymer.Base or whatever. settings-languages does require the thing to be implemented as a Polymer element, but that's internal to this file, so i'm not concerned about its name here.
https://codereview.chromium.org/1419033008/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages.js (right): https://codereview.chromium.org/1419033008/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:459: var LanguageHelperImpl = SettingsLanguagesSingletonElement; On 2015/12/02 22:32:56, michaelpg wrote: > On 2015/12/02 22:21:15, Dan Beam wrote: > > On 2015/12/02 21:06:06, michaelpg wrote: > > > On 2015/12/01 04:17:44, Dan Beam wrote: > > > > i still don't really understand why we need this step... it just seems to > > > > introduce an extra name for no reason > > > > > > It's not a dealbreaker for the CL. But the reason for the extra name is to > > > minimize the perceived surface area of the element when it's used by other > > > elements, providing the LanguageHelper interface via a LanguageHelper > > "pointer" > > > but hiding the PolymerElement interface. For example: > > > > > > SettingsLanguageSingletonElement.getInstance().set('languages.enabled', > > [...]); > > > > > > is not how the element is supposed to be used, but: > > > > > > LanguageHelperImpl.getInstance().enableLanguage('ar'); > > > > > > works. > > > > > > Also, if we decide in the future to not use this weird singleton polymer > > > pattern, we can still keep the interface and not have to change the other > > files > > > that use LanguageHelperImpl. (Spoiler alert: this will probably happen to > some > > > degree.) > > > > so basically you want both names to be the most technically correct (because > you > > want to name it "Element" because Polymer() creates things that are instanceof > > Element but also "Helper"). is this really necessary? Like I mentioned: the > > fact that Polymer() is used to implement the Helper[Impl] seems unimportant. > > Oh, no, the reason I want to name it "Element" is because this is the type name > Closure's PolymerPass would create by default if I didn't assign the Polymer > call to a value, and I don't want to stray from that pattern. I don't > particularly care that the thing derives from Polymer.Base or whatever. but we are assigning it to a value? > > settings-languages does require the thing to be implemented as a Polymer > element, but that's internal to this file, so i'm not concerned about its name > here.
There was a second independent clause after the part about assigning it to a value, before the period marking the end of the sentence. On Wed, Dec 2, 2015 at 3:18 PM <dbeam@chromium.org> wrote: > > > https://codereview.chromium.org/1419033008/diff/280001/chrome/browser/resourc... > File chrome/browser/resources/settings/languages_page/languages.js > (right): > > > https://codereview.chromium.org/1419033008/diff/280001/chrome/browser/resourc... > chrome/browser/resources/settings/languages_page/languages.js:459 > <https://codereview.chromium.org/1419033008/diff/280001/chrome/browser/resourc...>: > var > LanguageHelperImpl = SettingsLanguagesSingletonElement; > On 2015/12/02 22:32:56, michaelpg wrote: > > On 2015/12/02 22:21:15, Dan Beam wrote: > > > On 2015/12/02 21:06:06, michaelpg wrote: > > > > On 2015/12/01 04:17:44, Dan Beam wrote: > > > > > i still don't really understand why we need this step... it just > seems to > > > > > introduce an extra name for no reason > > > > > > > > It's not a dealbreaker for the CL. But the reason for the extra > name is to > > > > minimize the perceived surface area of the element when it's used > by other > > > > elements, providing the LanguageHelper interface via a > LanguageHelper > > > "pointer" > > > > but hiding the PolymerElement interface. For example: > > > > > > > > > SettingsLanguageSingletonElement.getInstance().set('languages.enabled', > > > [...]); > > > > > > > > is not how the element is supposed to be used, but: > > > > > > > > LanguageHelperImpl.getInstance().enableLanguage('ar'); > > > > > > > > works. > > > > > > > > Also, if we decide in the future to not use this weird singleton > polymer > > > > pattern, we can still keep the interface and not have to change > the other > > > files > > > > that use LanguageHelperImpl. (Spoiler alert: this will probably > happen to > > some > > > > degree.) > > > > > > so basically you want both names to be the most technically correct > (because > > you > > > want to name it "Element" because Polymer() creates things that are > instanceof > > > Element but also "Helper"). is this really necessary? Like I > mentioned: the > > > fact that Polymer() is used to implement the Helper[Impl] seems > unimportant. > > > Oh, no, the reason I want to name it "Element" is because this is the > type name > > Closure's PolymerPass would create by default if I didn't assign the > Polymer > > call to a value, and I don't want to stray from that pattern. I don't > > particularly care that the thing derives from Polymer.Base or > whatever. > > but we are assigning it to a value? > > > > settings-languages does require the thing to be implemented as a > Polymer > > element, but that's internal to this file, so i'm not concerned about > its name > > here. > > https://codereview.chromium.org/1419033008/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/12/02 23:29:57, chromium-reviews wrote: > There was a second independent clause after the part about assigning it to > a value, before the period marking the end of the sentence. The fact that it can be overridden is exactly for situations like this.
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1419033008/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1419033008/300001
Message was sent while issue was closed.
Description was changed from ========== Extract language settings methods into a LanguageHelper interface and pass the singleton directly to host elements instead of forwarding every method through a settings-languages instance, because that is tedious. BUG=512479 ========== to ========== Extract language settings methods into a LanguageHelper interface and pass the singleton directly to host elements instead of forwarding every method through a settings-languages instance, because that is tedious. BUG=512479 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Extract language settings methods into a LanguageHelper interface and pass the singleton directly to host elements instead of forwarding every method through a settings-languages instance, because that is tedious. BUG=512479 ========== to ========== Extract language settings methods into a LanguageHelper interface and pass the singleton directly to host elements instead of forwarding every method through a settings-languages instance, because that is tedious. BUG=512479 Committed: https://crrev.com/a49c98bb68739710dfac2ebbad7281fe96b9ac53 Cr-Commit-Position: refs/heads/master@{#363284} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/a49c98bb68739710dfac2ebbad7281fe96b9ac53 Cr-Commit-Position: refs/heads/master@{#363284} |
