Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2555033003:
Add ScopedMacRTL class for Cocoa browser RTL testing (Closed)
DescriptionAdd ScopedForceRTLMac class for Cocoa browser RTL testing
BUG=673362
Committed: https://crrev.com/0390c3aa26a58e473d0b18c3a8df6a28ccbe68ff
Cr-Commit-Position: refs/heads/master@{#438185}
Patch Set 1 #Patch Set 2 : Add deps to base/test:test_support and base:i18n from Mac ui/base:test_support #Patch Set 3 : Add to deps AFTER deps exists #
Total comments: 13
Patch Set 4 : Review comments #
Total comments: 10
Patch Set 5 : Whitespace #
Total comments: 8
Patch Set 6 : Move scoper to c/b/ui/cocoa/test, revert the rest #Patch Set 7 : Actually include moved files #
Total comments: 4
Patch Set 8 : Review comments #
Messages
Total messages: 38 (20 generated)
The CQ bit was checked by lgrey@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: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lgrey@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.
lgrey@chromium.org changed reviewers: + ellyjones@chromium.org, rsesek@chromium.org
PTAL :)
https://codereview.chromium.org/2555033003/diff/40001/ui/base/cocoa/rtl_util.h File ui/base/cocoa/rtl_util.h (right): https://codereview.chromium.org/2555033003/diff/40001/ui/base/cocoa/rtl_util.... ui/base/cocoa/rtl_util.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: No (c) in new files. https://codereview.chromium.org/2555033003/diff/40001/ui/base/cocoa/rtl_util.... ui/base/cocoa/rtl_util.h:6: #define UI_BASE_COCOA_RTL_UTILS_H _utils files have a tendency to turn into a dumping ground. Can this be rtl_feature.h ? https://codereview.chromium.org/2555033003/diff/40001/ui/base/cocoa/rtl_util.... ui/base/cocoa/rtl_util.h:11: namespace cocoa_rtl_util { namespace ui https://codereview.chromium.org/2555033003/diff/40001/ui/base/cocoa/rtl_util.... ui/base/cocoa/rtl_util.h:11: namespace cocoa_rtl_util { nit: blank lines around the inside of the namespace https://codereview.chromium.org/2555033003/diff/40001/ui/base/cocoa/rtl_util.... ui/base/cocoa/rtl_util.h:12: UI_BASE_EXPORT extern const base::Feature kExperimentalMacRTL; nit: blank line after https://codereview.chromium.org/2555033003/diff/40001/ui/base/cocoa/rtl_util.mm File ui/base/cocoa/rtl_util.mm (right): https://codereview.chromium.org/2555033003/diff/40001/ui/base/cocoa/rtl_util.... ui/base/cocoa/rtl_util.mm:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: no (c) in new files https://codereview.chromium.org/2555033003/diff/40001/ui/base/cocoa/rtl_util.... ui/base/cocoa/rtl_util.mm:12: namespace cocoa_rtl_util { nit: blank lines on the inside of the namespace https://codereview.chromium.org/2555033003/diff/40001/ui/base/test/scoped_mac... File ui/base/test/scoped_mac_rtl.h (right): https://codereview.chromium.org/2555033003/diff/40001/ui/base/test/scoped_mac... ui/base/test/scoped_mac_rtl.h:8: #import <Cocoa/Cocoa.h> Move this into the .mm file so that this file can be included from C++ (if necessary), since it's unused in the header. https://codereview.chromium.org/2555033003/diff/40001/ui/base/test/scoped_mac... ui/base/test/scoped_mac_rtl.h:13: namespace cocoa_rtl_util { namespace ui https://codereview.chromium.org/2555033003/diff/40001/ui/base/test/scoped_mac... ui/base/test/scoped_mac_rtl.h:18: class ScopedMacRTL final { ScopedEnableMacRTLLayout ? https://codereview.chromium.org/2555033003/diff/40001/ui/base/test/scoped_mac... File ui/base/test/scoped_mac_rtl.mm (right): https://codereview.chromium.org/2555033003/diff/40001/ui/base/test/scoped_mac... ui/base/test/scoped_mac_rtl.mm:14: } // namespace nit: blank line after https://codereview.chromium.org/2555033003/diff/40001/ui/base/test/scoped_mac... ui/base/test/scoped_mac_rtl.mm:15: namespace cocoa_rtl_util { nit: blank lines around inside of the namespace https://codereview.chromium.org/2555033003/diff/40001/ui/base/test/scoped_mac... ui/base/test/scoped_mac_rtl.mm:28: } nit: blank line after
On 2016/12/07 16:09:59, Robert Sesek wrote: > https://codereview.chromium.org/2555033003/diff/40001/ui/base/cocoa/rtl_util.h > File ui/base/cocoa/rtl_util.h (right): > > https://codereview.chromium.org/2555033003/diff/40001/ui/base/cocoa/rtl_util.... > ui/base/cocoa/rtl_util.h:1: // Copyright (c) 2016 The Chromium Authors. All > rights reserved. > nit: No (c) in new files. > > https://codereview.chromium.org/2555033003/diff/40001/ui/base/cocoa/rtl_util.... > ui/base/cocoa/rtl_util.h:6: #define UI_BASE_COCOA_RTL_UTILS_H > _utils files have a tendency to turn into a dumping ground. Can this be > rtl_feature.h ? > > https://codereview.chromium.org/2555033003/diff/40001/ui/base/cocoa/rtl_util.... > ui/base/cocoa/rtl_util.h:11: namespace cocoa_rtl_util { > namespace ui > > https://codereview.chromium.org/2555033003/diff/40001/ui/base/cocoa/rtl_util.... > ui/base/cocoa/rtl_util.h:11: namespace cocoa_rtl_util { > nit: blank lines around the inside of the namespace > > https://codereview.chromium.org/2555033003/diff/40001/ui/base/cocoa/rtl_util.... > ui/base/cocoa/rtl_util.h:12: UI_BASE_EXPORT extern const base::Feature > kExperimentalMacRTL; > nit: blank line after > > https://codereview.chromium.org/2555033003/diff/40001/ui/base/cocoa/rtl_util.mm > File ui/base/cocoa/rtl_util.mm (right): > > https://codereview.chromium.org/2555033003/diff/40001/ui/base/cocoa/rtl_util.... > ui/base/cocoa/rtl_util.mm:1: // Copyright (c) 2016 The Chromium Authors. All > rights reserved. > nit: no (c) in new files > > https://codereview.chromium.org/2555033003/diff/40001/ui/base/cocoa/rtl_util.... > ui/base/cocoa/rtl_util.mm:12: namespace cocoa_rtl_util { > nit: blank lines on the inside of the namespace > > https://codereview.chromium.org/2555033003/diff/40001/ui/base/test/scoped_mac... > File ui/base/test/scoped_mac_rtl.h (right): > > https://codereview.chromium.org/2555033003/diff/40001/ui/base/test/scoped_mac... > ui/base/test/scoped_mac_rtl.h:8: #import <Cocoa/Cocoa.h> > Move this into the .mm file so that this file can be included from C++ (if > necessary), since it's unused in the header. > > https://codereview.chromium.org/2555033003/diff/40001/ui/base/test/scoped_mac... > ui/base/test/scoped_mac_rtl.h:13: namespace cocoa_rtl_util { > namespace ui > > https://codereview.chromium.org/2555033003/diff/40001/ui/base/test/scoped_mac... > ui/base/test/scoped_mac_rtl.h:18: class ScopedMacRTL final { > ScopedEnableMacRTLLayout ? > > https://codereview.chromium.org/2555033003/diff/40001/ui/base/test/scoped_mac... > File ui/base/test/scoped_mac_rtl.mm (right): > > https://codereview.chromium.org/2555033003/diff/40001/ui/base/test/scoped_mac... > ui/base/test/scoped_mac_rtl.mm:14: } // namespace > nit: blank line after > > https://codereview.chromium.org/2555033003/diff/40001/ui/base/test/scoped_mac... > ui/base/test/scoped_mac_rtl.mm:15: namespace cocoa_rtl_util { > nit: blank lines around inside of the namespace > > https://codereview.chromium.org/2555033003/diff/40001/ui/base/test/scoped_mac... > ui/base/test/scoped_mac_rtl.mm:28: } > nit: blank line after Done all (I guess Rietveld doesn't let you see comments from previous patchsets inline for new files?)
LGTM after you add some more whitespace :) > Done all (I guess Rietveld doesn't let you see comments from previous patchsets > inline for new files?) Not if the new file moves, yeah. https://codereview.chromium.org/2555033003/diff/60001/ui/base/cocoa/rtl_featu... File ui/base/cocoa/rtl_feature.h (right): https://codereview.chromium.org/2555033003/diff/60001/ui/base/cocoa/rtl_featu... ui/base/cocoa/rtl_feature.h:13: UI_BASE_EXPORT extern const base::Feature kExperimentalMacRTL; nit: blank line after https://codereview.chromium.org/2555033003/diff/60001/ui/base/cocoa/rtl_featu... ui/base/cocoa/rtl_feature.h:28: } // namespace ui nit: blank line after https://codereview.chromium.org/2555033003/diff/60001/ui/base/cocoa/rtl_featu... File ui/base/cocoa/rtl_feature.mm (right): https://codereview.chromium.org/2555033003/diff/60001/ui/base/cocoa/rtl_featu... ui/base/cocoa/rtl_feature.mm:12: namespace ui { nit: blank line after https://codereview.chromium.org/2555033003/diff/60001/ui/base/test/scoped_mac... File ui/base/test/scoped_mac_rtl.mm (right): https://codereview.chromium.org/2555033003/diff/60001/ui/base/test/scoped_mac... ui/base/test/scoped_mac_rtl.mm:18: } // namespace nit: blank line after https://codereview.chromium.org/2555033003/diff/60001/ui/base/test/scoped_mac... ui/base/test/scoped_mac_rtl.mm:32: } nit: blank line after
Thanks! + Trent for ui/base/test https://codereview.chromium.org/2555033003/diff/60001/ui/base/cocoa/rtl_featu... File ui/base/cocoa/rtl_feature.h (right): https://codereview.chromium.org/2555033003/diff/60001/ui/base/cocoa/rtl_featu... ui/base/cocoa/rtl_feature.h:13: UI_BASE_EXPORT extern const base::Feature kExperimentalMacRTL; On 2016/12/07 19:21:01, Robert Sesek wrote: > nit: blank line after Done. https://codereview.chromium.org/2555033003/diff/60001/ui/base/cocoa/rtl_featu... ui/base/cocoa/rtl_feature.h:28: } // namespace ui On 2016/12/07 19:21:01, Robert Sesek wrote: > nit: blank line after Done. https://codereview.chromium.org/2555033003/diff/60001/ui/base/cocoa/rtl_featu... File ui/base/cocoa/rtl_feature.mm (right): https://codereview.chromium.org/2555033003/diff/60001/ui/base/cocoa/rtl_featu... ui/base/cocoa/rtl_feature.mm:12: namespace ui { On 2016/12/07 19:21:01, Robert Sesek wrote: > nit: blank line after Done. https://codereview.chromium.org/2555033003/diff/60001/ui/base/test/scoped_mac... File ui/base/test/scoped_mac_rtl.mm (right): https://codereview.chromium.org/2555033003/diff/60001/ui/base/test/scoped_mac... ui/base/test/scoped_mac_rtl.mm:18: } // namespace On 2016/12/07 19:21:01, Robert Sesek wrote: > nit: blank line after Done. https://codereview.chromium.org/2555033003/diff/60001/ui/base/test/scoped_mac... ui/base/test/scoped_mac_rtl.mm:32: } On 2016/12/07 19:21:01, Robert Sesek wrote: > nit: blank line after Done.
tapted@chromium.org changed reviewers: + tapted@chromium.org
Can you add a BUG=? (maybe 642732?) The main thing... I'm not sure this belongs in ui/base [see comment] - if it does there should be something about the rationale for that in the CL description. https://codereview.chromium.org/2555033003/diff/80001/ui/base/BUILD.gn File ui/base/BUILD.gn (right): https://codereview.chromium.org/2555033003/diff/80001/ui/base/BUILD.gn#newcod... ui/base/BUILD.gn:680: "test/scoped_mac_rtl.mm", (maybe it's historical but..) the current convention is to list files in the main `sources = [` list "if that works". I think this will, since it is .mm, but the _mac should be at the end anyways (see next comment), which will make it even more workier. (the deps below should stay here, however) Edit: maybe it's here because of iOS -- see comment in scoped_mac_rtl.h https://codereview.chromium.org/2555033003/diff/80001/ui/base/cocoa/rtl_featu... File ui/base/cocoa/rtl_feature.mm (right): https://codereview.chromium.org/2555033003/diff/80001/ui/base/cocoa/rtl_featu... ui/base/cocoa/rtl_feature.mm:7: #import <Cocoa/Cocoa.h> is this needed? https://codereview.chromium.org/2555033003/diff/80001/ui/base/test/scoped_mac... File ui/base/test/scoped_mac_rtl.h (right): https://codereview.chromium.org/2555033003/diff/80001/ui/base/test/scoped_mac... ui/base/test/scoped_mac_rtl.h:5: #ifndef UI_BASE_TEST_SCOPED_MAC_RTL_H_ Why src/ui/base ? (Is there a plan to use this outside of src/chrome? or even src/chrome/browser/ui?) If not, I think this belongs somewhere under src/chrome -- perhaps src/chrome/test/base or src/chrome/browser/ui/test (e.g. I think iOS uses stuff from here ui/base, which we sometimes need to tread carefully around) https://codereview.chromium.org/2555033003/diff/80001/ui/base/test/scoped_mac... ui/base/test/scoped_mac_rtl.h:16: class ScopedEnableMacRTLLayout final { Since there's only one class in the file, it would be nice to have it match the filename. Perhaps scoped_force_rtl_mac.h/mm ScopedForceRTLMac (ending the file in _mac also means it's affected by the automatic platform globbing more obviously that just having the .mm suffix - e.g. the .h won't be checked for changes on non-mac platforms [i think]) https://codereview.chromium.org/2555033003/diff/80001/ui/base/test/scoped_mac... ui/base/test/scoped_mac_rtl.h:26: DISALLOW_COPY_AND_ASSIGN(ScopedEnableMacRTLLayout); nit: blank line before https://codereview.chromium.org/2555033003/diff/80001/ui/base/test/scoped_mac... File ui/base/test/scoped_mac_rtl.mm (right): https://codereview.chromium.org/2555033003/diff/80001/ui/base/test/scoped_mac... ui/base/test/scoped_mac_rtl.mm:7: #import <Cocoa/Cocoa.h> nit: NSUserDefaults is in <Foundation/Foundation.h> - I think that's all that's needed here https://codereview.chromium.org/2555033003/diff/80001/ui/base/test/scoped_mac... ui/base/test/scoped_mac_rtl.mm:23: scoped_feature_list_.InitAndEnableFeature(ui::kExperimentalMacRTL); nit: remove `ui::` https://codereview.chromium.org/2555033003/diff/80001/ui/base/test/scoped_mac... ui/base/test/scoped_mac_rtl.mm:32: base::i18n::SetICUDefaultLocale("he"); "he" looked really weird the first time I saw it being used for this (it might also confuse people searching for improperly gendered code ;). Perhaps another constant up in the anonymous namespace: const char kHebrewLocale[] = "he"; or const char kDefaultRTLLocale[] = "he"; // Hebrew.
On 2016/12/07 23:20:27, tapted wrote: > The main thing... I'm not sure this belongs in ui/base [see comment] - if it > does there should be something about the rationale for that in the CL > description. rsesek@ and mark@ suggested ui/base to get around a CHECKDEPS issue: This needs ScopedFeatureList (from base/test, can't import from chrome/browser) and the RTL feature (which was in chrome/browser before this, obviously can't import from base). If you're OK with this reasoning, will update CL description in a bit when I address other comments.
On 2016/12/08 15:05:57, lgrey wrote: > On 2016/12/07 23:20:27, tapted wrote: > > > The main thing... I'm not sure this belongs in ui/base [see comment] - if it > > does there should be something about the rationale for that in the CL > > description. > > rsesek@ and mark@ suggested ui/base to get around a CHECKDEPS issue: > This needs ScopedFeatureList (from base/test, can't import from chrome/browser) > and the RTL feature (which was in chrome/browser before this, obviously can't > import from base). > > If you're OK with this reasoning, will update CL description in a bit when I > address > other comments. Hm, is this actually a checkdeps issue? I don't see -base/test in chrome/browser/DEPS. What is going to be downstream of ScopedEnableMacRTLLayout besides //chrome ?
On 2016/12/08 20:02:09, Robert Sesek wrote: > On 2016/12/08 15:05:57, lgrey wrote: > > On 2016/12/07 23:20:27, tapted wrote: > > > > > The main thing... I'm not sure this belongs in ui/base [see comment] - if it > > > does there should be something about the rationale for that in the CL > > > description. > > > > rsesek@ and mark@ suggested ui/base to get around a CHECKDEPS issue: > > This needs ScopedFeatureList (from base/test, can't import from > chrome/browser) > > and the RTL feature (which was in chrome/browser before this, obviously can't > > import from base). > > > > If you're OK with this reasoning, will update CL description in a bit when I > > address > > other comments. > > Hm, is this actually a checkdeps issue? I don't see -base/test in > chrome/browser/DEPS. What is going to be downstream of ScopedEnableMacRTLLayout > besides //chrome ? I got an error to the effect of "illegal import" when I tried to send this CL originally, though I don't remember what the exact structure was at that point. Nothing should be downstream beyond //chrome/browser/ui/cocoa Is there somewhere in //chrome that would be a natural home for the scoper that can import from both //base/test and //chrome/browser/ui/cocoa?
On 2016/12/08 20:17:50, lgrey wrote: > On 2016/12/08 20:02:09, Robert Sesek wrote: > > On 2016/12/08 15:05:57, lgrey wrote: > > > On 2016/12/07 23:20:27, tapted wrote: > > > > > > > The main thing... I'm not sure this belongs in ui/base [see comment] - if > it > > > > does there should be something about the rationale for that in the CL > > > > description. > > > > > > rsesek@ and mark@ suggested ui/base to get around a CHECKDEPS issue: > > > This needs ScopedFeatureList (from base/test, can't import from > > chrome/browser) > > > and the RTL feature (which was in chrome/browser before this, obviously > can't > > > import from base). > > > > > > If you're OK with this reasoning, will update CL description in a bit when I > > > address > > > other comments. > > > > Hm, is this actually a checkdeps issue? I don't see -base/test in > > chrome/browser/DEPS. What is going to be downstream of > ScopedEnableMacRTLLayout > > besides //chrome ? > > I got an error to the effect of "illegal import" when I tried to send this CL > originally, though > I don't remember what the exact structure was at that point. Nothing should be > downstream > beyond //chrome/browser/ui/cocoa > > Is there somewhere in //chrome that would be a natural home for the scoper that > can import > from both //base/test and //chrome/browser/ui/cocoa? I like what you've done here grouping together rtl_feature.h and scoped_mac_rtl.h -- I think that means that so long as they stay together that (wherever it goes) it no longer needs to import from chrome/browser/ui/cocoa. You're correct that there are restrictions on that. I don't think there are any restrictions on base/test (but it would be nice if things importing from base/test are also in a path with "test" in the name. Does either of src/chrome/test/base or src/chrome/browser/ui/test work?
On 2016/12/09 00:03:59, tapted wrote: > Does either of src/chrome/test/base or src/chrome/browser/ui/test work? Ah, except we need a home for rtl_feature.h not in a test folder :). Maybe we just add src/chrome/browser/ui/cocoa/test ? There's a ton of testing stuff currently just dumped in c/b/ui/cocoa: cocoa_profile_test.h cocoa_profile_test.mm cocoa_test_helper.h cocoa_test_helper.mm run_loop_testing.h run_loop_testing.mm styled_text_field_test_helper.h styled_text_field_test_helper.mm They probably belong in a ./test/ subfolder of c/b/ui/cocoa #yak That is, rtl_feature.h lives in c/b/ui/cocoa scoped_mac_rtl.h lives in c/b/ui/cocoa/test
Description was changed from ========== Add ScopedMacRTL class for Cocoa browser RTL testing Also moves RTL utility methods to their own file in ui/base/cocoa so that they can be referred to by ScopedMacRTL BUG= ========== to ========== Add ScopedForceRTLMac class for Cocoa browser RTL testing BUG=673362 ==========
Updated, PTAL Renamed the class to ScopedForceMacRTL and moved to c/b/ui/cocoa/test Reverted everything else[1], since l10n_util.h can now see the scoper. [1] Except the test that now uses the scoper.
LGTM
nice. lgtm (thanks for splitting it up, and sorry for the yaks :) https://codereview.chromium.org/2555033003/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/test/scoped_force_rtl_mac.h (right): https://codereview.chromium.org/2555033003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/test/scoped_force_rtl_mac.h:5: #ifndef CHROME_BROWSER_UI_COCOA__TEST_SCOPED_FORCE_RTL_MAC_H__ nit: remove an extra underscore in COCOA__TEST (3 places) https://codereview.chromium.org/2555033003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/test/scoped_force_rtl_mac.h:26: DISALLOW_COPY_AND_ASSIGN(ScopedForceRTLMac); nit: blank line before
Thanks! https://codereview.chromium.org/2555033003/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/test/scoped_force_rtl_mac.h (right): https://codereview.chromium.org/2555033003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/test/scoped_force_rtl_mac.h:5: #ifndef CHROME_BROWSER_UI_COCOA__TEST_SCOPED_FORCE_RTL_MAC_H__ On 2016/12/12 22:20:38, tapted wrote: > nit: remove an extra underscore in COCOA__TEST (3 places) Done. https://codereview.chromium.org/2555033003/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/test/scoped_force_rtl_mac.h:26: DISALLOW_COPY_AND_ASSIGN(ScopedForceRTLMac); On 2016/12/12 22:20:38, tapted wrote: > nit: blank line before Done.
The CQ bit was checked by lgrey@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: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lgrey@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2555033003/#ps140001 (title: "Review 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": 140001, "attempt_start_ts": 1481642508591140,
"parent_rev": "c6243f520a3821b9a2e8437b171819cf480ab2c0", "commit_rev":
"e32c68a21cffb9c168a07a8d6a87c70577ec4230"}
Message was sent while issue was closed.
Description was changed from ========== Add ScopedForceRTLMac class for Cocoa browser RTL testing BUG=673362 ========== to ========== Add ScopedForceRTLMac class for Cocoa browser RTL testing BUG=673362 Review-Url: https://codereview.chromium.org/2555033003 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add ScopedForceRTLMac class for Cocoa browser RTL testing BUG=673362 Review-Url: https://codereview.chromium.org/2555033003 ========== to ========== Add ScopedForceRTLMac class for Cocoa browser RTL testing BUG=673362 Committed: https://crrev.com/0390c3aa26a58e473d0b18c3a8df6a28ccbe68ff Cr-Commit-Position: refs/heads/master@{#438185} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/0390c3aa26a58e473d0b18c3a8df6a28ccbe68ff Cr-Commit-Position: refs/heads/master@{#438185} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
