Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2822863002:
MD Settings: Languages; limit platform specific code to the respective platform. (Closed)
|
Created:
3 years, 8 months ago by dpapad Modified:
3 years, 8 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, michaelpg, stevenjb+watch-md-settings_chromium.org, stevenjb Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Languages; limit platform specific code to the respective platform.
languages_page.js has a lot of platform specific code, but it was not properly
marked, so it was being included in unrelated platforms. This CLs cleans up the
code such that is only included in the platforms that it is actually used.
- chromeos
- chromeos or is_win
- not is_macosx
This is a step towards deflaking/re-enabling the corresponding tests(for example
this makes it clear which platforms need a browser proxy abstraction).
BUG=692356
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2822863002
Cr-Commit-Position: refs/heads/master@{#465662}
Committed: https://chromium.googlesource.com/chromium/src/+/eb8e8df2be632be5824072b96b3159bc2dacbab3
Patch Set 1 #Patch Set 2 : Undo #
Total comments: 1
Patch Set 3 : More #Patch Set 4 : More #Patch Set 5 : Undo accidental remove #Patch Set 6 : Types #
Total comments: 15
Patch Set 7 : Address @michaelpg feedback #
Total comments: 4
Patch Set 8 : Address comments. #
Dependent Patchsets: Messages
Total messages: 38 (29 generated)
Description was changed from ========== MD Settings: Limit platform specific code to the respective platform. BUG=692356 ========== to ========== MD Settings: Limit platform specific code to the respective platform. BUG=692356 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Limit platform specific code to the respective platform. BUG=692356 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Limit platform specific code to the respective platform. languages_page.js has a lot of platform specific code, but it was not properly marked, so it was being included in unrelated platforms. This CLs cleans up the code such that code is only included in the platforms that it is actually used. - chromeos - chromeos or is_win - not is_macosx BUG=692356 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== MD Settings: Limit platform specific code to the respective platform. languages_page.js has a lot of platform specific code, but it was not properly marked, so it was being included in unrelated platforms. This CLs cleans up the code such that code is only included in the platforms that it is actually used. - chromeos - chromeos or is_win - not is_macosx BUG=692356 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Languages; limit platform specific code to the respective platform. languages_page.js has a lot of platform specific code, but it was not properly marked, so it was being included in unrelated platforms. This CLs cleans up the code such that code is only included in the platforms that it is actually used. - chromeos - chromeos or is_win - not is_macosx BUG=692356 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Languages; limit platform specific code to the respective platform. languages_page.js has a lot of platform specific code, but it was not properly marked, so it was being included in unrelated platforms. This CLs cleans up the code such that code is only included in the platforms that it is actually used. - chromeos - chromeos or is_win - not is_macosx BUG=692356 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Languages; limit platform specific code to the respective platform. languages_page.js has a lot of platform specific code, but it was not properly marked, so it was being included in unrelated platforms. This CLs cleans up the code such that code is only included in the platforms that it is actually used. - chromeos - chromeos or is_win - not is_macosx This is a step towards deflaking/re-enabling the corresponding tests. BUG=692356 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dpapad@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2822863002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/languages_page/languages_page.js (left): https://codereview.chromium.org/2822863002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/languages_page.js:96: onBackTap_: function() { Dead code.
Description was changed from ========== MD Settings: Languages; limit platform specific code to the respective platform. languages_page.js has a lot of platform specific code, but it was not properly marked, so it was being included in unrelated platforms. This CLs cleans up the code such that code is only included in the platforms that it is actually used. - chromeos - chromeos or is_win - not is_macosx This is a step towards deflaking/re-enabling the corresponding tests. BUG=692356 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Languages; limit platform specific code to the respective platform. languages_page.js has a lot of platform specific code, but it was not properly marked, so it was being included in unrelated platforms. This CLs cleans up the code such that code is only included in the platforms that it is actually used. - chromeos - chromeos or is_win - not is_macosx This is a step towards deflaking/re-enabling the corresponding tests(for example this makes it clear which platforms need a browser proxy abstraction). BUG=692356 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== MD Settings: Languages; limit platform specific code to the respective platform. languages_page.js has a lot of platform specific code, but it was not properly marked, so it was being included in unrelated platforms. This CLs cleans up the code such that code is only included in the platforms that it is actually used. - chromeos - chromeos or is_win - not is_macosx This is a step towards deflaking/re-enabling the corresponding tests(for example this makes it clear which platforms need a browser proxy abstraction). BUG=692356 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Languages; limit platform specific code to the respective platform. languages_page.js has a lot of platform specific code, but it was not properly marked, so it was being included in unrelated platforms. This CLs cleans up the code such is only included in the platforms that it is actually used. - chromeos - chromeos or is_win - not is_macosx This is a step towards deflaking/re-enabling the corresponding tests(for example this makes it clear which platforms need a browser proxy abstraction). BUG=692356 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Languages; limit platform specific code to the respective platform. languages_page.js has a lot of platform specific code, but it was not properly marked, so it was being included in unrelated platforms. This CLs cleans up the code such is only included in the platforms that it is actually used. - chromeos - chromeos or is_win - not is_macosx This is a step towards deflaking/re-enabling the corresponding tests(for example this makes it clear which platforms need a browser proxy abstraction). BUG=692356 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Languages; limit platform specific code to the respective platform. languages_page.js has a lot of platform specific code, but it was not properly marked, so it was being included in unrelated platforms. This CLs cleans up the code such that is only included in the platforms that it is actually used. - chromeos - chromeos or is_win - not is_macosx This is a step towards deflaking/re-enabling the corresponding tests(for example this makes it clear which platforms need a browser proxy abstraction). BUG=692356 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dpapad@chromium.org changed reviewers: + michaelpg@chromium.org - dbeam@chromium.org
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
i think this generally lgtm but michaelpg@ is probably more familiar
Some questions/suggestions. https://codereview.chromium.org/2822863002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages.js (left): https://codereview.chromium.org/2822863002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:492: assert(cr.isChromeOS || cr.isWindows); I used runtime checks instead of grit because that came up in the original reviews. Is grit what we prefer now for webui? (maybe my original understanding was incorrect in the first place) https://codereview.chromium.org/2822863002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages.js (right): https://codereview.chromium.org/2822863002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:85: inputMethodPrivate: Object, Wondering if this and the input method properties below could be if'd out as well. https://codereview.chromium.org/2822863002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:177: if (cr.isChromeOS) { this stuff is platform specific for the same reasons -- should it be grit too? https://codereview.chromium.org/2822863002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:493: chrome.send('setProspectiveUILanguage', [languageCode]); Does it makes sense to #ifdef these handlers in C++ now? https://codereview.chromium.org/2822863002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages_page.js (right): https://codereview.chromium.org/2822863002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages_page.js:359: assert(cr.isChromeOS || cr.isWindows); remove assert https://codereview.chromium.org/2822863002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages_page.js:517: this.initializeMenu_(menu); if it shakes out this way, rename initializeMenu_ to something obviously CrOS-specific, e.g. tweakMenuForCrOS -- otherwise it's not clear why we aren't "initializing" the menu on other platforms. https://codereview.chromium.org/2822863002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages_types.js (right): https://codereview.chromium.org/2822863002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages_types.js:65: whenReady: assertNotReached, Would you mind re-ordering LanguageHelper's function declarations to match up with languages.js? That'll make it easier to track which functions are included or excluded in the <if>s, IMO.
Patchset #7 (id:120001) has been deleted
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2822863002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages.js (left): https://codereview.chromium.org/2822863002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:492: assert(cr.isChromeOS || cr.isWindows); On 2017/04/18 at 05:03:35, michaelpg wrote: > I used runtime checks instead of grit because that came up in the original reviews. Is grit what we prefer now for webui? (maybe my original understanding was incorrect in the first place) AFAIK, GRIT is preferred. This guarantees for example that our bundled JS file is as small as possible for any given platform, since it does not include code only meant to run in other platforms. https://codereview.chromium.org/2822863002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages.js (right): https://codereview.chromium.org/2822863002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:85: inputMethodPrivate: Object, On 2017/04/18 at 05:03:35, michaelpg wrote: > Wondering if this and the input method properties below could be if'd out as well. I am addressing those in my next CL, which hides them behind a proxy, still WIP, but you can take a look https://codereview.chromium.org/2827553002 (or you can wait until I send it out later today). https://codereview.chromium.org/2822863002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:177: if (cr.isChromeOS) { On 2017/04/18 at 05:03:35, michaelpg wrote: > this stuff is platform specific for the same reasons -- should it be grit too? The platform specific code and the generic code are too tangled here. The |promises| array indices 3 and 4 are assigned in ChromeOS, but then they are used at line 207 regardless (guessing results[3] and results[4] are undefined in other platforms). Either way, this refactoring does not contribute directly to my effort of deflaking the tests, so I'd rather leave for later. https://codereview.chromium.org/2822863002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:493: chrome.send('setProspectiveUILanguage', [languageCode]); On 2017/04/18 at 05:03:35, michaelpg wrote: > Does it makes sense to #ifdef these handlers in C++ now? Done. https://codereview.chromium.org/2822863002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages_page.js (right): https://codereview.chromium.org/2822863002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages_page.js:359: assert(cr.isChromeOS || cr.isWindows); On 2017/04/18 at 05:03:35, michaelpg wrote: > remove assert Done. https://codereview.chromium.org/2822863002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages_page.js:517: this.initializeMenu_(menu); On 2017/04/18 at 05:03:35, michaelpg wrote: > if it shakes out this way, rename initializeMenu_ to something obviously CrOS-specific, e.g. tweakMenuForCrOS -- otherwise it's not clear why we aren't "initializing" the menu on other platforms. Done. https://codereview.chromium.org/2822863002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages_types.js (right): https://codereview.chromium.org/2822863002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages_types.js:65: whenReady: assertNotReached, On 2017/04/18 at 05:03:36, michaelpg wrote: > Would you mind re-ordering LanguageHelper's function declarations to match up with languages.js? That'll make it easier to track which functions are included or excluded in the <if>s, IMO. I prefer to do this in the next CL https://codereview.chromium.org/2827553002 to avoid conflicts. The latter CL also removes a couple of methods from LanguageHelper's API.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM! https://codereview.chromium.org/2822863002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages.js (right): https://codereview.chromium.org/2822863002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:177: if (cr.isChromeOS) { On 2017/04/18 18:31:20, dpapad wrote: > On 2017/04/18 at 05:03:35, michaelpg wrote: > > this stuff is platform specific for the same reasons -- should it be grit too? > > The platform specific code and the generic code are too tangled here. The > |promises| array indices 3 and 4 are assigned in ChromeOS, but then they are > used at line 207 regardless (guessing results[3] and results[4] are undefined in > other platforms). Oof, you're right, and to make things worse the conditional below means another promise is added on Windows in the 3'rd slot, at line 193. I verified (with tommy's help) that it doesn't affect how createModel_ is called on Windows, since the promise doesn't return anything. But we reeeally should clear that up. Can you add a TODO here? I'd be happy to handle it once the dust settles from your patches. > > Either way, this refactoring does not contribute directly to my effort of > deflaking the tests, so I'd rather leave for later. https://codereview.chromium.org/2822863002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages_page.js (right): https://codereview.chromium.org/2822863002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages_page.js:153: tweakMenuForCrOs_: function(menu) { nit: CrOS not CrOs (sorry to bikeshed, but it's CrOS everywhere in the code by convention) https://codereview.chromium.org/2822863002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/md_settings_ui.cc (right): https://codereview.chromium.org/2822863002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/md_settings_ui.cc:48: #include "chrome/browser/ui/webui/settings/languages_handler.h" Nice! We can go further and make languages_handler.* conditional in the GRD too, and remove some NOTREACHED() from that languages_handler.cc, but doing that in a later CL is fine (and again, I'm happy to do it if you want to stay focused on what you're doing)
https://codereview.chromium.org/2822863002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages_page.js (right): https://codereview.chromium.org/2822863002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages_page.js:153: tweakMenuForCrOs_: function(menu) { On 2017/04/18 at 22:47:58, michaelpg wrote: > nit: CrOS not CrOs (sorry to bikeshed, but it's CrOS everywhere in the code by convention) Done. https://codereview.chromium.org/2822863002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/md_settings_ui.cc (right): https://codereview.chromium.org/2822863002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/md_settings_ui.cc:48: #include "chrome/browser/ui/webui/settings/languages_handler.h" On 2017/04/18 at 22:47:58, michaelpg wrote: > Nice! We can go further and make languages_handler.* conditional in the GRD too, and remove some NOTREACHED() from that languages_handler.cc, but doing that in a later CL is fine (and again, I'm happy to do it if you want to stay focused on what you're doing) Agreed. Prefer to do it as a follow up cleanup after the immediate goal of re-enabling the tests has been achieved.
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/2822863002/#ps160001 (title: "Address comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1492619527373420,
"parent_rev": "3dd276cc350cc3b5c9972da71ce0ac3b9ffc0ebe", "commit_rev":
"eb8e8df2be632be5824072b96b3159bc2dacbab3"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Languages; limit platform specific code to the respective platform. languages_page.js has a lot of platform specific code, but it was not properly marked, so it was being included in unrelated platforms. This CLs cleans up the code such that is only included in the platforms that it is actually used. - chromeos - chromeos or is_win - not is_macosx This is a step towards deflaking/re-enabling the corresponding tests(for example this makes it clear which platforms need a browser proxy abstraction). BUG=692356 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Languages; limit platform specific code to the respective platform. languages_page.js has a lot of platform specific code, but it was not properly marked, so it was being included in unrelated platforms. This CLs cleans up the code such that is only included in the platforms that it is actually used. - chromeos - chromeos or is_win - not is_macosx This is a step towards deflaking/re-enabling the corresponding tests(for example this makes it clear which platforms need a browser proxy abstraction). BUG=692356 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2822863002 Cr-Commit-Position: refs/heads/master@{#465662} Committed: https://chromium.googlesource.com/chromium/src/+/eb8e8df2be632be5824072b96b31... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as https://chromium.googlesource.com/chromium/src/+/eb8e8df2be632be5824072b96b31... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
