Add LayoutLocale class

Add LayoutLocale class

This patch adds LayoutLocale class that centralize conversions and
caching layout-related data for locale/script/language.

BUG=586517
Committed: https://crrev.com/d9e0aefc99858f93ba216a3cfab66ed13d3fdb2e
Cr-Commit-Position: refs/heads/master@{#407433}

[kojii, 2016-07-18 16:22:14]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 16:22:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 16:47:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-18 16:47:44]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kojii, 2016-07-19 13:45:21]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 13:45:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-07-19 14:00:15]

Description was changed from

==========
font-locale

BUG=
==========

to

==========
Add FontLocale class

This patch adds FontLocale class that centralize conversions and
caching data for locale/script/language.

BUG=586517
==========

[kojii, 2016-07-19 14:00:54]

kojii@chromium.org changed reviewers:
+ drott@chromium.org, eae@chromium.org

[kojii, 2016-07-19 14:00:55]

Still WIP but thoughts/comments appreciated.

[commit-bot: I haz the power, 2016-07-19 14:20:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-19 14:20:22]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[eae, 2016-07-19 14:36:33]

This is exciting, overall approach seems fine.

[drott, 2016-07-19 15:28:53]

General approach looks good to me too, good to see some cleaning up and bringing
sanity to this! Makes it easier to understand. 

Some comments on methods on FontDescription, and a mild naming suggestion.

https://codereview.chromium.org/2161683002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/FontDescription.h (left):

https://codereview.chromium.org/2161683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontDescription.h:223:
m_fields.m_script = localeToScriptCodeForFontSelection(locale);
Good to see this going away.

https://codereview.chromium.org/2161683002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/FontDescription.h (right):

https://codereview.chromium.org/2161683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontDescription.h:181: const
FontLocale* locale() const { return m_locale.get(); }
Perhaps we can reduce this to one method on FontDescription, and then have a
valueOrDefault() and initialLocaleString() and script() or something on
FontLocale itself?

https://codereview.chromium.org/2161683002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/FontLocale.h (right):

https://codereview.chromium.org/2161683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontLocale.h:6: #define FontLocale_h
I am thinking about the naming, I don't have a strong opinion but is it a
FontLocale or rather a TextLocale? We happen to keep it in FontDescription, but
it's not coming from the font* CSS but from lang tags, headers, etc.

[kojii, 2016-07-19 15:29:39]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 15:29:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 15:55:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-19 15:55:09]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[kojii, 2016-07-19 16:40:11]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 16:41:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 17:07:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-19 17:07:51]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[dglazkov, 2016-07-19 22:06:02]

dglazkov@chromium.org changed reviewers:
+ dglazkov@chromium.org

[kojii, 2016-07-20 05:43:03]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 05:43:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-07-20 06:01:53]

Thank you for your comments, replies inline below:

https://codereview.chromium.org/2161683002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/FontDescription.h (right):

https://codereview.chromium.org/2161683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontDescription.h:181: const
FontLocale* locale() const { return m_locale.get(); }
On 2016/07/19 at 15:28:53, drott wrote:
> Perhaps we can reduce this to one method on FontDescription, and then have a
valueOrDefault() and initialLocaleString() and script() or something on
FontLocale itself?

Thanks, that's a good idea, added static methods and removed localeString().

However, I'm in favor to keep locale() and localeOrDefault(). Most of current
use is includeDefault=true, and because it's an optional argument, I see several
incorrect usages. Having these methods help developers to be aware of the
distinction.

https://codereview.chromium.org/2161683002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/FontLocale.h (right):

https://codereview.chromium.org/2161683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontLocale.h:6: #define FontLocale_h
On 2016/07/19 at 15:28:53, drott wrote:
> I am thinking about the naming, I don't have a strong opinion but is it a
FontLocale or rather a TextLocale? We happen to keep it in FontDescription, but
it's not coming from the font* CSS but from lang tags, headers, etc.

Yeah, thought about it, but we have text/PlatformLocale that handles number/date
formatting etc., which is also "text".

What about LayoutLocale? We will use this class to store layout-related
information.

[commit-bot: I haz the power, 2016-07-20 06:17:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-20 06:17:36]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[kojii, 2016-07-20 13:44:44]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 13:45:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 14:22:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-20 14:22:26]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[kojii, 2016-07-21 03:51:36]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 03:51:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 04:10:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 04:10:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[drott, 2016-07-21 09:23:50]

https://codereview.chromium.org/2161683002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/FontDescription.h (right):

https://codereview.chromium.org/2161683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontDescription.h:181: const
FontLocale* locale() const { return m_locale.get(); }
> However, I'm in favor to keep locale() and localeOrDefault(). Most of current
use is includeDefault=true, and because it's an optional argument, I see several
incorrect usages. Having these methods help developers to be aware of the
distinction.


Sounds good to me.

https://codereview.chromium.org/2161683002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/FontLocale.h (right):

https://codereview.chromium.org/2161683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontLocale.h:6: #define FontLocale_h
On 2016/07/20 at 06:01:53, kojii wrote:
> On 2016/07/19 at 15:28:53, drott wrote:
> > I am thinking about the naming, I don't have a strong opinion but is it a
FontLocale or rather a TextLocale? We happen to keep it in FontDescription, but
it's not coming from the font* CSS but from lang tags, headers, etc.
> 
> Yeah, thought about it, but we have text/PlatformLocale that handles
number/date formatting etc., which is also "text".
> 
> What about LayoutLocale? We will use this class to store layout-related
information.

Yes, that'd work.

[kojii, 2016-07-21 21:11:49]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 21:12:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-07-21 21:14:45]

Description was changed from

==========
Add FontLocale class

This patch adds FontLocale class that centralize conversions and
caching data for locale/script/language.

BUG=586517
==========

to

==========
Add LayoutLocale class

This patch adds LayoutLocale class that centralize conversions and
caching layout-related data for locale/script/language.

BUG=586517
==========

[kojii, 2016-07-21 21:53:09]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 21:53:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 23:29:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 23:29:11]

Dry run: This issue passed the CQ dry run.

[kojii, 2016-07-22 13:34:20]

PTAL. There are more we can cleanup, such as in font fallback, but I think I can
do them in following patches.

[kojii, 2016-07-22 15:10:41]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 15:10:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 16:22:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 16:22:43]

Dry run: This issue passed the CQ dry run.

[eae, 2016-07-22 17:39:47]

LGTM but please give drott another chance to review it.

[kojii, 2016-07-23 16:01:58]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-23 16:02:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-23 17:20:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-23 17:20:26]

Dry run: This issue passed the CQ dry run.

[drott, 2016-07-25 07:37:18]

LGTM!

[kojii, 2016-07-25 07:38:11]

Thank you!

[kojii, 2016-07-25 07:38:15]

The CQ bit was checked by kojii@chromium.org

[kojii, 2016-07-25 07:38:15]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2161683002/#ps200001
(title: "Test stability")

[commit-bot: I haz the power, 2016-07-25 07:38:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 08:48:13]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-07-25 08:49:22]

Description was changed from

==========
Add LayoutLocale class

This patch adds LayoutLocale class that centralize conversions and
caching layout-related data for locale/script/language.

BUG=586517
==========

to

==========
Add LayoutLocale class

This patch adds LayoutLocale class that centralize conversions and
caching layout-related data for locale/script/language.

BUG=586517

Committed: https://crrev.com/d9e0aefc99858f93ba216a3cfab66ed13d3fdb2e
Cr-Commit-Position: refs/heads/master@{#407433}
==========

[commit-bot: I haz the power, 2016-07-25 08:49:23]

Patchset 11 (id:??) landed as
https://crrev.com/d9e0aefc99858f93ba216a3cfab66ed13d3fdb2e
Cr-Commit-Position: refs/heads/master@{#407433}