Experimental change: Remove DirectWrite 1750 font limit.

Experimental change: Remove DirectWrite 1750 font limit.

As things seem to have stabilized after Cache check-in. I want to see and experiment with removing this limit.
Modified Cache file version, so that it will be regenerated.

BUG=441477
R=scottmg,cpu

Committed: https://crrev.com/fc41a66cb9b6e1e1ad827248101d5870ac9a2e27
Cr-Commit-Position: refs/heads/master@{#308181}

[scottmg, 2014-12-11 21:18:13]

I think this won't help because of the similar code in ShouldUseDirectWrite
at
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/win/direct_...

Or do you just want to build and not use it at first?

On Thu, Dec 11, 2014 at 1:10 PM, <shrikant@chromium.org> wrote:

> Reviewers: cpu, scottmg,
>
> Description:
> Experimental change: Remove DirectWrite 1750 font limit.
>
> As things seem to have stabilized after Cache check-in. I want to see and
> experiment with removing this limit.
> Modified Cache file version, so that it will be regenerated.
>
> BUG=441477
> R=scottmg,cpu
>
> Please review this at https://codereview.chromium.org/796643004/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+9, -19 lines):
>   M content/common/dwrite_font_platform_win.cc
>
>
> Index: content/common/dwrite_font_platform_win.cc
> diff --git a/content/common/dwrite_font_platform_win.cc
> b/content/common/dwrite_font_platform_win.cc
> index 5d56ce4b55549cf41cbca6d840421b801ff1961f..
> c2947ab5ee5a089a978fcaa2c15925704dcf6711 100644
> --- a/content/common/dwrite_font_platform_win.cc
> +++ b/content/common/dwrite_font_platform_win.cc
> @@ -84,7 +84,7 @@ const double kArbitraryCacheFileSizeLimit = (20 * 1024 *
> 1024);
>  // that file directly from system fonts folder.
>  const unsigned int kMaxFontFileNameLength = 34;
>
> -const DWORD kCacheFileVersion = 101;
> +const DWORD kCacheFileVersion = 102;
>  const DWORD kFileSignature = 0x4D4F5243; // CROM
>  const DWORD kMagicCompletionSignature = 0x454E4F44; // DONE
>
> @@ -1028,16 +1028,11 @@ IDWriteFontCollection*
GetCustomFontCollection(IDWriteFactory*
> factory) {
>
>    bool cache_file_loaded = g_font_loader->LoadCacheFile();
>
> -  // We try here to put arbitrary limit on max number of fonts that could
> -  // be loaded, otherwise we fallback to restricted set of fonts.
> -  const UINT32 kMaxFontThreshold = 1750;
>    HRESULT hr = E_FAIL;
> -  if (g_font_loader->GetFontMapSize() < kMaxFontThreshold) {
> -    g_font_loader->EnableCollectionBuildingMode(true);
> -    hr = factory->CreateCustomFontCollection(
> -        g_font_loader.Get(), NULL, 0, g_font_collection.GetAddressOf());
> -    g_font_loader->EnableCollectionBuildingMode(false);
> -  }
> +  g_font_loader->EnableCollectionBuildingMode(true);
> +  hr = factory->CreateCustomFontCollection(
> +      g_font_loader.Get(), NULL, 0, g_font_collection.GetAddressOf());
> +  g_font_loader->EnableCollectionBuildingMode(false);
>
>    bool loading_restricted = false;
>    if (FAILED(hr) || !g_font_collection.Get()) {
> @@ -1105,16 +1100,11 @@ bool BuildFontCacheInternal(const WCHAR*
> file_name) {
>
>    mswr::ComPtr<IDWriteFontCollection> font_collection;
>
> -  // We try here to put arbitrary limit on max number of fonts that could
> -  // be loaded, otherwise we fallback to restricted set of fonts.
> -  const UINT32 kMaxFontThreshold = 1750;
>    HRESULT hr = E_FAIL;
> -  if (g_font_loader->GetFontMapSize() < kMaxFontThreshold) {
> -    g_font_loader->EnableCollectionBuildingMode(true);
> -    hr = factory->CreateCustomFontCollection(
> -        g_font_loader.Get(), NULL, 0, font_collection.GetAddressOf());
> -    g_font_loader->EnableCollectionBuildingMode(false);
> -  }
> +  g_font_loader->EnableCollectionBuildingMode(true);
> +  hr = factory->CreateCustomFontCollection(
> +      g_font_loader.Get(), NULL, 0, font_collection.GetAddressOf());
> +  g_font_loader->EnableCollectionBuildingMode(false);
>
>    bool loading_restricted = false;
>    if (FAILED(hr) || !font_collection.Get()) {
>
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[cpu_(ooo_6.6-7.5), 2014-12-11 23:31:25]

note scott's comment.

https://codereview.chromium.org/796643004/diff/1/content/common/dwrite_font_p...
File content/common/dwrite_font_platform_win.cc (left):

https://codereview.chromium.org/796643004/diff/1/content/common/dwrite_font_p...
content/common/dwrite_font_platform_win.cc:1109: // be loaded, otherwise we
fallback to restricted set of fonts.
we had the same constant twice?

https://codereview.chromium.org/796643004/diff/1/content/common/dwrite_font_p...
File content/common/dwrite_font_platform_win.cc (right):

https://codereview.chromium.org/796643004/diff/1/content/common/dwrite_font_p...
content/common/dwrite_font_platform_win.cc:1104:
g_font_loader->EnableCollectionBuildingMode(true);
can it handle 20K fonts? is so go ahead. lgtm.

[Shrikant Kelkar, 2014-12-11 23:43:42]

On 2014/12/11 21:18:13, scottmg wrote:
> I think this won't help because of the similar code in ShouldUseDirectWrite
> at
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/win/direct_...
> 
> Or do you just want to build and not use it at first?
> 
> On Thu, Dec 11, 2014 at 1:10 PM, <mailto:shrikant@chromium.org> wrote:
> 
> > Reviewers: cpu, scottmg,
> >
> > Description:
> > Experimental change: Remove DirectWrite 1750 font limit.
> >
> > As things seem to have stabilized after Cache check-in. I want to see and
> > experiment with removing this limit.
> > Modified Cache file version, so that it will be regenerated.
> >
> > BUG=441477
> > R=scottmg,cpu
> >
> > Please review this at https://codereview.chromium.org/796643004/
> >
> > Base URL: https://chromium.googlesource.com/chromium/src.git@master
> >
> > Affected files (+9, -19 lines):
> >   M content/common/dwrite_font_platform_win.cc
> >
> >
> > Index: content/common/dwrite_font_platform_win.cc
> > diff --git a/content/common/dwrite_font_platform_win.cc
> > b/content/common/dwrite_font_platform_win.cc
> > index 5d56ce4b55549cf41cbca6d840421b801ff1961f..
> > c2947ab5ee5a089a978fcaa2c15925704dcf6711 100644
> > --- a/content/common/dwrite_font_platform_win.cc
> > +++ b/content/common/dwrite_font_platform_win.cc
> > @@ -84,7 +84,7 @@ const double kArbitraryCacheFileSizeLimit = (20 * 1024 *
> > 1024);
> >  // that file directly from system fonts folder.
> >  const unsigned int kMaxFontFileNameLength = 34;
> >
> > -const DWORD kCacheFileVersion = 101;
> > +const DWORD kCacheFileVersion = 102;
> >  const DWORD kFileSignature = 0x4D4F5243; // CROM
> >  const DWORD kMagicCompletionSignature = 0x454E4F44; // DONE
> >
> > @@ -1028,16 +1028,11 @@ IDWriteFontCollection*
> GetCustomFontCollection(IDWriteFactory*
> > factory) {
> >
> >    bool cache_file_loaded = g_font_loader->LoadCacheFile();
> >
> > -  // We try here to put arbitrary limit on max number of fonts that could
> > -  // be loaded, otherwise we fallback to restricted set of fonts.
> > -  const UINT32 kMaxFontThreshold = 1750;
> >    HRESULT hr = E_FAIL;
> > -  if (g_font_loader->GetFontMapSize() < kMaxFontThreshold) {
> > -    g_font_loader->EnableCollectionBuildingMode(true);
> > -    hr = factory->CreateCustomFontCollection(
> > -        g_font_loader.Get(), NULL, 0, g_font_collection.GetAddressOf());
> > -    g_font_loader->EnableCollectionBuildingMode(false);
> > -  }
> > +  g_font_loader->EnableCollectionBuildingMode(true);
> > +  hr = factory->CreateCustomFontCollection(
> > +      g_font_loader.Get(), NULL, 0, g_font_collection.GetAddressOf());
> > +  g_font_loader->EnableCollectionBuildingMode(false);
> >
> >    bool loading_restricted = false;
> >    if (FAILED(hr) || !g_font_collection.Get()) {
> > @@ -1105,16 +1100,11 @@ bool BuildFontCacheInternal(const WCHAR*
> > file_name) {
> >
> >    mswr::ComPtr<IDWriteFontCollection> font_collection;
> >
> > -  // We try here to put arbitrary limit on max number of fonts that could
> > -  // be loaded, otherwise we fallback to restricted set of fonts.
> > -  const UINT32 kMaxFontThreshold = 1750;
> >    HRESULT hr = E_FAIL;
> > -  if (g_font_loader->GetFontMapSize() < kMaxFontThreshold) {
> > -    g_font_loader->EnableCollectionBuildingMode(true);
> > -    hr = factory->CreateCustomFontCollection(
> > -        g_font_loader.Get(), NULL, 0, font_collection.GetAddressOf());
> > -    g_font_loader->EnableCollectionBuildingMode(false);
> > -  }
> > +  g_font_loader->EnableCollectionBuildingMode(true);
> > +  hr = factory->CreateCustomFontCollection(
> > +      g_font_loader.Get(), NULL, 0, font_collection.GetAddressOf());
> > +  g_font_loader->EnableCollectionBuildingMode(false);
> >
> >    bool loading_restricted = false;
> >    if (FAILED(hr) || !font_collection.Get()) {
> >
> >
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Done.

[Shrikant Kelkar, 2014-12-11 23:43:50]

https://codereview.chromium.org/796643004/diff/1/content/common/dwrite_font_p...
File content/common/dwrite_font_platform_win.cc (left):

https://codereview.chromium.org/796643004/diff/1/content/common/dwrite_font_p...
content/common/dwrite_font_platform_win.cc:1109: // be loaded, otherwise we
fallback to restricted set of fonts.
On 2014/12/11 23:31:24, cpu wrote:
> we had the same constant twice?

Acknowledged.

https://codereview.chromium.org/796643004/diff/1/content/common/dwrite_font_p...
File content/common/dwrite_font_platform_win.cc (right):

https://codereview.chromium.org/796643004/diff/1/content/common/dwrite_font_p...
content/common/dwrite_font_platform_win.cc:1104:
g_font_loader->EnableCollectionBuildingMode(true);
On 2014/12/11 23:31:24, cpu wrote:
> can it handle 20K fonts? is so go ahead. lgtm.

It may not.. but in this experiment let's see to what level hangs/crashes come
back.

[scottmg, 2014-12-12 00:16:41]

lgtm

[Shrikant Kelkar, 2014-12-12 00:19:05]

The CQ bit was checked by shrikant@chromium.org

[commit-bot: I haz the power, 2014-12-12 00:20:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/796643004/20001

[commit-bot: I haz the power, 2014-12-12 00:25:10]

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

[commit-bot: I haz the power, 2014-12-12 00:25:10]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[Shrikant Kelkar, 2014-12-12 01:55:24]

The CQ bit was checked by shrikant@chromium.org

[commit-bot: I haz the power, 2014-12-12 01:57:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/796643004/40001

[commit-bot: I haz the power, 2014-12-12 03:37:39]

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

[commit-bot: I haz the power, 2014-12-12 03:37:40]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Shrikant Kelkar, 2014-12-12 21:16:16]

The CQ bit was checked by shrikant@chromium.org

[commit-bot: I haz the power, 2014-12-12 21:17:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/796643004/60001

[commit-bot: I haz the power, 2014-12-12 22:24:28]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-12-12 22:25:15]

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