Move the FontFaceCache stored in CSSFontSelector to be stored in Document.

Move the FontFaceCache stored in CSSFontSelector to be stored in Document.

This addresses a FIXME found in the code. This FIXME was added in
2013 by dglazkov in 2013, ad02388f5fcffaab45b9ada7a7dfdcae13ed460f.

BUG=685945
Review-Url: https://codereview.chromium.org/2624893003
Cr-Commit-Position: refs/heads/master@{#452734}
Committed: https://chromium.googlesource.com/chromium/src/+/e07e958d62db5bb606c0dbbb446d2a25e33c3db1

[meade_UTC10, 2017-01-11 04:17:08]

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

[commit-bot: I haz the power, 2017-01-11 04:17:31]

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

[meade_UTC10, 2017-01-11 04:18:39]

meade@chromium.org changed reviewers:
+ sashab@chromium.org

[meade_UTC10, 2017-01-11 04:18:40]

PTAL for a first look? Thanks!

[sashab, 2017-01-11 04:20:36]

Cool idea! 1 quick question

https://codereview.chromium.org/2624893003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/2624893003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.h:1658: FontFaceCache
m_fontFaceCache;
What is FontFaceCache? Should this be a Member<>

[sashab, 2017-01-11 04:21:09]

Also... Are we concerned about adding stuff to Document? Do we care about mem
increases from that? Not sure how big FontFaceCache is but probably worth
considering, unless mem usage is going to be same as before

[meade_UTC10, 2017-01-11 05:15:42]

On 2017/01/11 04:21:09, sashab wrote:
> Also... Are we concerned about adding stuff to Document? Do we care about mem
> increases from that? Not sure how big FontFaceCache is but probably worth
> considering, unless mem usage is going to be same as before

The memory usage should be the same as before, I just moved it from one place to
another...

[meade_UTC10, 2017-01-11 05:15:49]

https://codereview.chromium.org/2624893003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/2624893003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.h:1658: FontFaceCache
m_fontFaceCache;
On 2017/01/11 04:20:35, sashab wrote:
> What is FontFaceCache? Should this be a Member<>

FontFaceCache is essentially a map of StyleRuleFontFace to FontFace, plus some
metadata. It's tagged as DISALLOW_NEW, and it isn't oilpanned, so it can't be a
Member<>.

[meade_UTC10, 2017-01-11 06:21:15]

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

[commit-bot: I haz the power, 2017-01-11 06:21:28]

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, 2017-01-11 08:04:06]

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

[commit-bot: I haz the power, 2017-01-11 08:04:07]

Dry run: This issue passed the CQ dry run.

[sashab, 2017-01-11 22:48:53]

Lg*m but not confident to OWNERs stamp this :) Would be great to hear from a
fonts expert

[meade_UTC10, 2017-01-12 00:37:02]

meade@chromium.org changed reviewers:
+ eae@chromium.org

[meade_UTC10, 2017-01-12 00:37:03]

Hi Emil,

I wanted to try and understand the Fonts code better so I can implement the
CSSFontFaceValue object for Typed OM. I saw this FIXME in the code, and decided
to try it out. How does this look?

Thanks!
Eddy

[eae, 2017-01-12 00:45:14]

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

[eae, 2017-01-12 00:45:14]

Thank you for working on this! I think this makes sense but I'd love for drott
to take a look before landing it. He's been thinking a lot about our caching
strategy for fonts lately.

LGTM

[drott, 2017-01-18 09:34:50]

Thanks Eddy for addressing this old FIXME from Dimitri in 2013, ad02388f5fcffa.
Could you mention this hash in the CL description, and perhaps elaborate a bit
more on the relations, see below.

LGTM - on condition that your and my understanding of the relations between
document and FontFaceCache is identical and this reduces the StyleEngine and
FontSelector indirections between this cache and document.

Could you file a bug to track this and the dependent the CL? In case this gets
reverted, we can continue the discussion there. 

In the dependent CL, is there any chance we can reduce the number of additional
Document* arguments to the load callbacks?

Can you give some more details of what your plan is with Typed OM for
CSSFontFaceValue? How does this CL help with this?

https://codereview.chromium.org/2624893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/2624893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.h:1658: FontFaceCache
m_fontFaceCache;
Could you elaborate a little bit on whether you think the lifecycle before and
after is identical? Is my following understanding correct: There was a 1:1:1:1
relationship document:styleengine:cssfontselector:fontfacecache? And taking down
a document took down the styleengine, CSSFontSelector and FontFaceCache with it?
And now we're hanging the fontface cache straight into document, where it's
taken down when document goes away?

[meade_UTC10, 2017-01-27 06:58:37]

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

[commit-bot: I haz the power, 2017-01-27 06:58:47]

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

[meade_UTC10, 2017-01-27 07:10:59]

Description was changed from

==========
Move the FontFaceCache stored in CSSFontSelector to be stored in Document.

This addresses a FIXME found in the code.
==========

to

==========
Move the FontFaceCache stored in CSSFontSelector to be stored in Document.

This addresses a FIXME found in the code.

BUG=685945
==========

[commit-bot: I haz the power, 2017-01-27 09:48:10]

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

[commit-bot: I haz the power, 2017-01-27 09:48:11]

Dry run: This issue passed the CQ dry run.

[meade_UTC10, 2017-02-09 07:20:15]

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

[commit-bot: I haz the power, 2017-02-09 07:20:40]

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

[meade_UTC10, 2017-02-09 08:02:27]

https://codereview.chromium.org/2624893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/2624893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.h:1658: FontFaceCache
m_fontFaceCache;
On 2017/01/18 09:34:49, drott wrote:
> Could you elaborate a little bit on whether you think the lifecycle before and
> after is identical? Is my following understanding correct: There was a 1:1:1:1
> relationship document:styleengine:cssfontselector:fontfacecache? And taking
down
> a document took down the StyleEngine, CSSFontSelector and FontFaceCache with
it?
> And now we're hanging the fontface cache straight into document, where it's
> taken down when document goes away?

Looking at the existing code:
- FontFaceCache is only instantiated in CSSFontSelector (in its constructor).
- CSSFontSelector is only instantiated by StyleEngine (in its constructor).
- StyleEngine is only instantiated by Document (in its constructor)

- FontFaceCache uses DISALLOW_NEW, and as far as I can tell, nobody is storing a
pointer to the CSSFontSelector's FontFaceCache.
So that checks out.

The only weird thing is PopupMenuImpl::SelectFontsFromOwnerDocument, which sets
the StyleEngine's CSSFontSelector to a new PopupMenuCSSFontSelector that it
creates. But I _think_ that just wraps the existing FontSelector, so the
FontFaceCache shouldn't be affected. I experimented with adding a DCHECK that
the CSSFontSelector passed into StyleEngine::setFontSelector uses the same
Document as the StyleEngine, and ran all the layout tests and nothing crashed.
I'm pretty sure this is fine.

[meade_UTC10, 2017-02-09 08:05:33]

On 2017/01/18 09:34:50, drott wrote:
> Thanks Eddy for addressing this old FIXME from Dimitri in 2013,
ad02388f5fcffa.
> Could you mention this hash in the CL description, and perhaps elaborate a bit
> more on the relations, see below.
> 
> LGTM - on condition that your and my understanding of the relations between
> document and FontFaceCache is identical and this reduces the StyleEngine and
> FontSelector indirections between this cache and document.
> 
> Could you file a bug to track this and the dependent the CL? In case this gets
> reverted, we can continue the discussion there. 
Done - https://bugs.chromium.org/p/chromium/issues/detail?id=685945
> 
> In the dependent CL, is there any chance we can reduce the number of
additional
> Document* arguments to the load callbacks?
Ahh yeah I don't think that dependent CL is going anywhere. It probably needs
redesign, and I don't think I will have time to work on that any time soon
(sorry).

> 
> Can you give some more details of what your plan is with Typed OM for
> CSSFontFaceValue? How does this CL help with this?
The spec for Typed OM's CSSFontFaceValue is here:
https://drafts.css-houdini.org/css-typed-om/#fontfacevalue-objects
This CL doesn't help with Typed OM at all - I just wanted to try and understand
the code better before attempting to implement the Typed OM thing :)

> 
>
https://codereview.chromium.org/2624893003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Document.h (right):
> 
>
https://codereview.chromium.org/2624893003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Document.h:1658: FontFaceCache
> m_fontFaceCache;
> Could you elaborate a little bit on whether you think the lifecycle before and
> after is identical? Is my following understanding correct: There was a 1:1:1:1
> relationship document:styleengine:cssfontselector:fontfacecache? And taking
down
> a document took down the styleengine, CSSFontSelector and FontFaceCache with
it?
> And now we're hanging the fontface cache straight into document, where it's
> taken down when document goes away?

[meade_UTC10, 2017-02-09 08:07:05]

Description was changed from

==========
Move the FontFaceCache stored in CSSFontSelector to be stored in Document.

This addresses a FIXME found in the code.

BUG=685945
==========

to

==========
Move the FontFaceCache stored in CSSFontSelector to be stored in Document.

This addresses a FIXME found in the code. This FIXME was added in
2013 by dglazkov in 2013, ad02388f5fcffaab45b9ada7a7dfdcae13ed460f.

BUG=685945
==========

[commit-bot: I haz the power, 2017-02-09 09:13:15]

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

[commit-bot: I haz the power, 2017-02-09 09:13:16]

Dry run: This issue passed the CQ dry run.

[meade_UTC10, 2017-02-14 00:24:49]

The CQ bit was checked by meade@chromium.org

[meade_UTC10, 2017-02-14 00:24:50]

The patchset sent to the CQ was uploaded after l-g-t-m from drott@chromium.org,
eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2624893003/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-14 00:25:41]

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, 2017-02-14 01:16:36]

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

[commit-bot: I haz the power, 2017-02-14 01:16:37]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[meade_UTC10, 2017-02-14 05:37:33]

The CQ bit was checked by meade@chromium.org

[commit-bot: I haz the power, 2017-02-14 05:37:49]

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, 2017-02-14 12:28:29]

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

[commit-bot: I haz the power, 2017-02-14 12:28:31]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[meade_UTC10, 2017-02-16 03:00:27]

The CQ bit was checked by meade@chromium.org

[commit-bot: I haz the power, 2017-02-16 03:01:25]

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, 2017-02-16 03:48:54]

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

[commit-bot: I haz the power, 2017-02-16 03:48:56]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[meade_UTC10, 2017-02-22 05:18:43]

The CQ bit was checked by meade@chromium.org

[commit-bot: I haz the power, 2017-02-22 05:19:28]

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, 2017-02-22 07:34:52]

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

[commit-bot: I haz the power, 2017-02-22 07:34:53]

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

[meade_UTC10, 2017-02-24 01:26:07]

The CQ bit was checked by meade@chromium.org

[commit-bot: I haz the power, 2017-02-24 01:26:41]

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, 2017-02-24 03:15:58]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487899567658570,
"parent_rev": "67c468135356b4a7ab2ebb9eb4a76568bccb9ba1", "commit_rev":
"e07e958d62db5bb606c0dbbb446d2a25e33c3db1"}

[commit-bot: I haz the power, 2017-02-24 03:16:32]

Description was changed from

==========
Move the FontFaceCache stored in CSSFontSelector to be stored in Document.

This addresses a FIXME found in the code. This FIXME was added in
2013 by dglazkov in 2013, ad02388f5fcffaab45b9ada7a7dfdcae13ed460f.

BUG=685945
==========

to

==========
Move the FontFaceCache stored in CSSFontSelector to be stored in Document.

This addresses a FIXME found in the code. This FIXME was added in
2013 by dglazkov in 2013, ad02388f5fcffaab45b9ada7a7dfdcae13ed460f.

BUG=685945

Review-Url: https://codereview.chromium.org/2624893003
Cr-Commit-Position: refs/heads/master@{#452734}
Committed:
https://chromium.googlesource.com/chromium/src/+/e07e958d62db5bb606c0dbbb446d...
==========

[commit-bot: I haz the power, 2017-02-24 03:16:34]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/e07e958d62db5bb606c0dbbb446d...

[erikchen, 2017-03-01 01:29:55]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2728513002/ by erikchen@chromium.org.

The reason for reverting is: Reverted on suspicion of causing the top renderer
crash on Mac. 

https://bugs.chromium.org/p/chromium/issues/detail?id=696231.