Moving fontSelector from StyleResolver to StyleEngine.

Moving fontSelector from StyleResolver to StyleEngine.

To avoid dispatching fontLoaded event multiple times for the same font (caused by clearResolver), we should change fontSelector's owner.

BUG=236298, 305885
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163369

[tasak, 2013-11-26 09:56:22]

I temporary added StyleEngine::resetFontSelector() and
CSSFontSelector::hasAnyFontFaceRule() to move fontSelector.

I will remove the methods in another patch (using removeFontFace to avoid
reseting fontSelector).

[ojan, 2013-11-26 17:22:23]

lgtm

[commit-bot: I haz the power, 2013-11-26 23:10:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tasak@google.com/87503003/40001

[commit-bot: I haz the power, 2013-11-26 23:10:43]

Failed to apply patch for Source/core/dom/StyleEngine.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/dom/StyleEngine.cpp
  Hunk #3 FAILED at 429.
  Hunk #4 succeeded at 431 (offset -19 lines).
  Hunk #5 succeeded at 468 (offset -19 lines).
  1 out of 5 hunks FAILED -- saving rejects to file
Source/core/dom/StyleEngine.cpp.rej

Patch:       Source/core/dom/StyleEngine.cpp
Index: Source/core/dom/StyleEngine.cpp
diff --git a/Source/core/dom/StyleEngine.cpp b/Source/core/dom/StyleEngine.cpp
index
8a1d0c8ed93c83fc2d6a3389d92582a982c4fad5..ed77b83fe66fabfc964b1a3b9cd4865f5d8c2e13
100644
--- a/Source/core/dom/StyleEngine.cpp
+++ b/Source/core/dom/StyleEngine.cpp
@@ -30,6 +30,7 @@
 
 #include "HTMLNames.h"
 #include "SVGNames.h"
+#include "core/css/CSSFontSelector.h"
 #include "core/css/CSSStyleSheet.h"
 #include "core/css/StyleInvalidationAnalysis.h"
 #include "core/css/StyleSheetContents.h"
@@ -69,11 +70,16 @@ StyleEngine::StyleEngine(Document& document)
     , m_didCalculateResolver(false)
     , m_lastResolverAccessCount(0)
     , m_resolverThrowawayTimer(this, &StyleEngine::resolverThrowawayTimerFired)
+    , m_fontSelector(CSSFontSelector::create(&document))
 {
 }
 
 StyleEngine::~StyleEngine()
 {
+    if (m_resolver)
+        m_fontSelector->unregisterForInvalidationCallbacks(m_resolver.get());
+    m_fontSelector->clearDocument();
+
     for (unsigned i = 0; i < m_injectedAuthorStyleSheets.size(); ++i)
         m_injectedAuthorStyleSheets[i]->clearOwnerNode();
     for (unsigned i = 0; i < m_authorStyleSheets.size(); ++i)
@@ -423,12 +429,15 @@ void StyleEngine::createResolver()
     ASSERT(m_document.frame());
 
     m_resolver = adoptPtr(new StyleResolver(m_document));
+    m_fontSelector->registerForInvalidationCallbacks(m_resolver.get());
     combineCSSFeatureFlags(m_resolver->ensureRuleFeatureSet());
 }
 
 void StyleEngine::clearResolver()
 {
     ASSERT(!m_document.inStyleRecalc());
+    if (m_resolver)
+        m_fontSelector->unregisterForInvalidationCallbacks(m_resolver.get());
     m_resolver.clear();
 }
 
@@ -444,11 +453,6 @@ void
StyleEngine::resolverThrowawayTimerFired(Timer<StyleEngine>*)
     m_lastResolverAccessCount = resolverAccessCount();
 }
 
-CSSFontSelector* StyleEngine::fontSelector()
-{
-    return m_resolver ? m_resolver->fontSelector() : 0;
-}
-
 void StyleEngine::didAttach()
 {
     m_resolverThrowawayTimer.startRepeating(60);
@@ -486,4 +490,21 @@ StyleResolverChange
StyleEngine::resolverChanged(StyleResolverUpdateMode mode)
     return change;
 }
 
+// FIXME: should use removeFontFace to avoid loading the same font multiple
times.
+// So resetFontSelector() and hasAnyFontFaceRule() will be removed.
+void StyleEngine::resetFontSelector()
+{
+    if (!m_fontSelector->hasAnyFontFaceRule())
+        return;
+
+    m_fontSelector->clearDocument();
+    if (m_resolver) {
+        m_fontSelector->unregisterForInvalidationCallbacks(m_resolver.get());
+        m_resolver->invalidateMatchedPropertiesCache();
+    }
+    m_fontSelector = CSSFontSelector::create(&m_document);
+    if (m_resolver)
+        m_fontSelector->registerForInvalidationCallbacks(m_resolver.get());
+}
+
 }

[commit-bot: I haz the power, 2013-11-27 05:26:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tasak@google.com/87503003/60001

[commit-bot: I haz the power, 2013-11-27 09:13:54]

Retried try job too often on mac_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[tasak, 2013-11-27 10:17:14]

On 2013/11/27 09:13:54, I haz the power (commit-bot) wrote:
> Retried try job too often on mac_blink_rel for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

This patch affects mac-blink-rel (and mac-blink)'s:
- printing/width-overflow.html
- printing/setPrinting.html

I will investigate what happens.

[tasak, 2013-11-29 09:03:20]

Would you review this CL again?

On 2013/11/27 10:17:14, tasak wrote:
> On 2013/11/27 09:13:54, I haz the power (commit-bot) wrote:
> > Retried try job too often on mac_blink_rel for step(s) webkit_tests
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...
> 
> This patch affects mac-blink-rel (and mac-blink)'s:
> - printing/width-overflow.html
> - printing/setPrinting.html
> 
> I will investigate what happens.

I think, I should just move CSSFontSelector from StyleResolver to StyleEngine.
I should not add hasAnyFontFaceRule now.

[tasak, 2013-12-02 05:30:50]

On 2013/11/29 09:03:20, tasak wrote:

> > This patch affects mac-blink-rel (and mac-blink)'s:
> > - printing/width-overflow.html
> > - printing/setPrinting.html

Since FontDescription::setUsePrinterFont is disabled in Mac platform,
re-layouting for printing depends on whether fontSelector is null or not (c.f.
Font::operator==).

However, if we avoid recreating FontSelector, Font::operator== returns true when
setPrinting(true).

I think, we should do:

- #if OS(MACOSX)
-     void setUsePrinterFont(bool) { }
- #else
      void setUsePrinterFont(bool p) { m_usePrinterFont = p; }
- #endif

+ #if OS(MACOSX)
      bool usePrinterFont() const { return false; }
+ #else
      bool usePrinterFont() const { return m_usePrinterFont; }
+ #endif

[dglazkov, 2013-12-02 17:47:41]

lgtm++ with nits

https://codereview.chromium.org/87503003/diff/80001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/87503003/diff/80001/Source/core/dom/Document....
Source/core/dom/Document.cpp:1959: CSSFontSelector* Document::fontSelector()
const
This function doesn't do anything useful and hides the relationship of Document
and FontSelector (that is, the fact that Document owns StyleEngine which in turn
owns FontSelector). Would you remove it?

https://codereview.chromium.org/87503003/diff/80001/Source/core/dom/StyleEngi...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/87503003/diff/80001/Source/core/dom/StyleEngi...
Source/core/dom/StyleEngine.cpp:477:
m_fontSelector->unregisterForInvalidationCallbacks(m_resolver.get());
This is fishy, but we'll fix this in another patch.

[dglazkov, 2013-12-02 17:48:19]

On 2013/12/02 05:30:50, tasak wrote:
> I think, we should do:
> 
> - #if OS(MACOSX)
> -     void setUsePrinterFont(bool) { }
> - #else
>       void setUsePrinterFont(bool p) { m_usePrinterFont = p; }
> - #endif
> 
> + #if OS(MACOSX)
>       bool usePrinterFont() const { return false; }
> + #else
>       bool usePrinterFont() const { return m_usePrinterFont; }
> + #endif

SGTM. This is still nasty, but we'll fix this in a different patch.

[Nico, 2013-12-02 18:36:24]

(nit: according to comment 76 on crbug.com/236298 this patch changes behavior,
hence the cl description shouldn't start with "refactoring" since refactoring by
definition doesn't change behavior. Since it apparently affects bug 236298, add
that to the BUG= line. The CL description seems generally pretty terse too.)

[tasak, 2013-12-05 05:09:43]

Thank you for reviewing.

I modified StyleEngine not to create CSSFontSelector when document.frame() is
null.

I also removed document.fontSelector().

https://codereview.chromium.org/87503003/diff/80001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/87503003/diff/80001/Source/core/dom/Document....
Source/core/dom/Document.cpp:1959: CSSFontSelector* Document::fontSelector()
const
On 2013/12/02 17:47:42, Dimitri Glazkov wrote:
> This function doesn't do anything useful and hides the relationship of
Document
> and FontSelector (that is, the fact that Document owns StyleEngine which in
turn
> owns FontSelector). Would you remove it?

Done.

https://codereview.chromium.org/87503003/diff/80001/Source/core/dom/StyleEngi...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/87503003/diff/80001/Source/core/dom/StyleEngi...
Source/core/dom/StyleEngine.cpp:477:
m_fontSelector->unregisterForInvalidationCallbacks(m_resolver.get());
On 2013/12/02 17:47:42, Dimitri Glazkov wrote:
> This is fishy, but we'll fix this in another patch.

Yeah, I will remove this method.

[dglazkov, 2013-12-05 05:41:16]

godspeed.

[ojan, 2013-12-05 18:57:14]

A couple more questions. Sorry to slow this down. I'm really eager for this
patch to land.

https://codereview.chromium.org/87503003/diff/140001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/87503003/diff/140001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:74: , m_fontSelector(document.frame() ?
CSSFontSelector::create(&document) : 0)
Is this null checking document.frame() for import documents? If so, I think it
could use an explanatory comment.

https://codereview.chromium.org/87503003/diff/140001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:80: if (m_fontSelector) {
Why do we need to do this?

https://codereview.chromium.org/87503003/diff/140001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:471: void StyleEngine::createResolver()
Should this ASSERT(m_fontSelector)?

https://codereview.chromium.org/87503003/diff/140001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:485: void StyleEngine::clearResolver()
Should this ASSERT(m_fontSelector)?

https://codereview.chromium.org/87503003/diff/140001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:557: if (!m_document.frame())
Should this be if (!m_fontSelector)?

[tasak, 2013-12-06 07:13:46]

https://codereview.chromium.org/87503003/diff/140001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/87503003/diff/140001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:74: , m_fontSelector(document.frame() ?
CSSFontSelector::create(&document) : 0)
On 2013/12/05 18:57:15, ojan wrote:
> Is this null checking document.frame() for import documents? If so, I think it
> could use an explanatory comment.

Yes, and also for HTMLTemplateElement.
I found, if we have no null checking here, http/tests/htmlimports and tests
containing <template> will crash.

c.f.
STDERR: Received signal 11 SEGV_MAPERR 000000000028
STDERR:  [0x7fe9da748168] base::debug::StackTrace::StackTrace()
STDERR:  [0x7fe9da747a6f] base::debug::(anonymous
namespace)::StackDumpSignalHandler()
STDERR:  [0x7fe9cf6c6cb0] <unknown>
STDERR:  [0x7fe9d6d6678e] WebCore::Frame::settings()
STDERR:  [0x7fe9d6a03cad] WebCore::CSSFontSelector::CSSFontSelector()
STDERR:  [0x7fe9d6452b85] WebCore::CSSFontSelector::create()
STDERR:  [0x7fe9d6450338] WebCore::StyleEngine::StyleEngine()
STDERR:  [0x7fe9d6375596] WebCore::StyleEngine::create()
STDERR:  [0x7fe9d635f5b7] WebCore::Document::Document()
STDERR:  [0x7fe9d65ce4a0] WebCore::HTMLDocument::HTMLDocument()
STDERR:  [0x7fe9d536d260] WebCore::HTMLDocument::create()
STDERR:  [0x7fe9d65f354a] WebCore::HTMLImportData::startWritingAndParsing()
STDERR:  [0x7fe9d65f3365] WebCore::HTMLImportData::responseReceived()

and

STDERR: Received signal 11 SEGV_MAPERR 000000000028
STDERR:  [0x7f2d201c3168] base::debug::StackTrace::StackTrace()
STDERR:  [0x7f2d201c2a6f] base::debug::(anonymous
namespace)::StackDumpSignalHandler()
STDERR:  [0x7f2d15141cb0] <unknown>
STDERR:  [0x7f2d1c7e178e] WebCore::Frame::settings()
STDERR:  [0x7f2d1c47ecad] WebCore::CSSFontSelector::CSSFontSelector()
STDERR:  [0x7f2d1becdb85] WebCore::CSSFontSelector::create()
STDERR:  [0x7f2d1becb338] WebCore::StyleEngine::StyleEngine()
STDERR:  [0x7f2d1bdf0596] WebCore::StyleEngine::create()
STDERR:  [0x7f2d1bdda5b7] WebCore::Document::Document()
STDERR:  [0x7f2d1c0494a0] WebCore::HTMLDocument::HTMLDocument()
STDERR:  [0x7f2d1ade8260] WebCore::HTMLDocument::create()
STDERR:  [0x7f2d1bdee2f8] WebCore::Document::ensureTemplateDocument()
STDERR:  [0x7f2d1c0bd313] WebCore::HTMLTemplateElement::content()

https://codereview.chromium.org/87503003/diff/140001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:80: if (m_fontSelector) {
On 2013/12/05 18:57:15, ojan wrote:
> Why do we need to do this?

I see. Looking at ~CSSFontSelector, we don't need this.

https://codereview.chromium.org/87503003/diff/140001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:471: void StyleEngine::createResolver()
On 2013/12/05 18:57:15, ojan wrote:
> Should this ASSERT(m_fontSelector)?

I see. Done.

https://codereview.chromium.org/87503003/diff/140001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:485: void StyleEngine::clearResolver()
On 2013/12/05 18:57:15, ojan wrote:
> Should this ASSERT(m_fontSelector)?

I added ASSERT(m_fontSelector || !m_resolver).

https://codereview.chromium.org/87503003/diff/140001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:557: if (!m_document.frame())
On 2013/12/05 18:57:15, ojan wrote:
> Should this be if (!m_fontSelector)?

I think, should be if (!m_fontSelector || !m_document.isActive()).

I also added ASSERT(m_document.frame()).

[ojan, 2013-12-07 01:04:32]

lgtm

[commit-bot: I haz the power, 2013-12-07 01:06:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tasak@google.com/87503003/200001

[commit-bot: I haz the power, 2013-12-07 03:05:31]

Change committed as 163369