StyleBuilder should not know about StyleResolver.

StyleBuilder should not know about StyleResolver.

StyleBuilder should only know how to build styles and apply properties.
Carefully, we weaned StyleBuilder off its addiction to StyleResolver.
Victory is ours.

No functional changes, no tests.

BUG=259085
R=eae, eseidel, esprehn

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155446

[dglazkov, 2013-08-01 20:49:38]

This depends on https://codereview.chromium.org/20672003/ and
https://codereview.chromium.org/20598006/ landing first.

[esprehn, 2013-08-01 21:07:13]

LGTM. Qapla!

https://codereview.chromium.org/21625003/diff/1/Source/core/css/resolver/Styl...
File Source/core/css/resolver/StyleBuilder.h (right):

https://codereview.chromium.org/21625003/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleBuilder.h:46: static void
applyProperty(CSSPropertyID, StyleResolverState&, CSSValue*);
This is the public API to the style builder system, I don't think you need the
FIXME.

[eseidel, 2013-08-01 21:09:53]

Cool.  So what's StyleBuilder's job?  W/o an explicit out-parameter it seems his
job is to just muck around in StyleResolverState.  Who knows what he does
there...

https://codereview.chromium.org/21625003/diff/1/Source/core/css/resolver/Styl...
File Source/core/css/resolver/StyleBuilder.h (right):

https://codereview.chromium.org/21625003/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleBuilder.h:45: // FIXME: What's the endgame for
this function?
What does endgame mean here?

https://codereview.chromium.org/21625003/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleBuilder.h:51: static void
oldApplyProperty(CSSPropertyID, StyleResolverState&, CSSValue*, bool isInitial,
bool isInherit);
It's uncclear if it should depend on StyleResolverState either. :)

https://codereview.chromium.org/21625003/diff/1/Source/core/css/resolver/Styl...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/21625003/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleBuilderCustom.cpp:330: if (Settings* settings =
state.elementContext().document()->settings())
Does this still risk changing behavior in the <canvas> case where we aren't
provided an element and thus elementContext().document() is 0?

https://codereview.chromium.org/21625003/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleBuilderCustom.cpp:649: KURL url =
state.elementContext().document()->completeURL(cssURLValue);
Same comment.  It's also not clear that we shoudl be resolving urls at this time
or not...

https://codereview.chromium.org/21625003/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleBuilderCustom.cpp:1168: // Fall back to the old
switch statement, which is now in StyleBuilderCustom.cpp
This comment probably isn't needed anymore. :)

[eseidel, 2013-08-01 21:10:10]

lgtm

[dglazkov, 2013-08-01 21:16:19]

https://codereview.chromium.org/21625003/diff/1/Source/core/css/resolver/Styl...
File Source/core/css/resolver/StyleBuilder.h (right):

https://codereview.chromium.org/21625003/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleBuilder.h:45: // FIXME: What's the endgame for
this function?
On 2013/08/01 21:09:53, eseidel wrote:
> What does endgame mean here?

--> "Will this need further refactoring or is this basically the API?"

I'll remove the comment altogether. It's not informative in any way.

https://codereview.chromium.org/21625003/diff/1/Source/core/css/resolver/Styl...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/21625003/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleBuilderCustom.cpp:330: if (Settings* settings =
state.elementContext().document()->settings())
On 2013/08/01 21:09:53, eseidel wrote:
> Does this still risk changing behavior in the <canvas> case where we aren't
> provided an element and thus elementContext().document() is 0?

The good news is that in <canvas> we're only applying a specific set of
font-related properties, so this case won't hit. However, it will be good to
assert the doc here.

[dglazkov, 2013-08-01 21:19:59]

On 2013/08/01 21:09:53, eseidel wrote:
> Cool.  So what's StyleBuilder's job?  W/o an explicit out-parameter it seems
his
> job is to just muck around in StyleResolverState.  Who knows what he does
> there...

That's right! We should feed it some const things and receive "effects" bag,
which is like Christmas presents from style application gods. The light at the
tunnel seems brighter every day :)

[dglazkov, 2013-08-01 21:47:49]

https://codereview.chromium.org/21625003/diff/1/Source/core/css/resolver/Styl...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/21625003/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleBuilderCustom.cpp:277: if (Frame* frame =
state.elementContext().document()->frame())
not lgtm. This one does indeed cause a crash.

[dglazkov, 2013-08-01 21:48:11]

not lgtm for now. Lemme fix this first.

[dglazkov, 2013-08-01 22:16:52]

Don't crash me, bro.

[dglazkov, 2013-08-01 22:19:41]

PTAL.

https://codereview.chromium.org/21625003/diff/15001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolverState.h (right):

https://codereview.chromium.org/21625003/diff/15001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolverState.h:50: Document* document() const {
return m_document; }
This is where things got a bit funky. StyleResolverState and
ElementResolveContext work well with each other in 99.999% cases. In 2 cases,
they disagree on what the value of document is. I had to bring back a
StyleResolverState::m_document to compensate for it.

[eseidel, 2013-08-01 22:53:48]

lgtm

https://codereview.chromium.org/21625003/diff/15001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolverState.h (right):

https://codereview.chromium.org/21625003/diff/15001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolverState.h:50: Document* document() const {
return m_document; }
On 2013/08/01 22:19:41, Dimitri Glazkov wrote:
> This is where things got a bit funky. StyleResolverState and
> ElementResolveContext work well with each other in 99.999% cases. In 2 cases,
> they disagree on what the value of document is. I had to bring back a
> StyleResolverState::m_document to compensate for it.

Seems like we should have a bug filed, or some sort of path forward here.

[commit-bot: I haz the power, 2013-08-02 05:08:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dglazkov@chromium.org/21625003/15001

[commit-bot: I haz the power, 2013-08-02 05:08:36]

Failed to apply patch for Source/core/css/resolver/StyleResolver.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/css/resolver/StyleResolver.h
  Hunk #1 FAILED at 304.
  Hunk #2 succeeded at 332 (offset 7 lines).
  Hunk #3 succeeded at 362 (offset 7 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
Source/core/css/resolver/StyleResolver.h.rej

Patch:       Source/core/css/resolver/StyleResolver.h
Index: Source/core/css/resolver/StyleResolver.h
diff --git a/Source/core/css/resolver/StyleResolver.h
b/Source/core/css/resolver/StyleResolver.h
index
b6778dffbcfdce6357d05fac2be2cf4f7eee3dec..a811d03aa071db6c99f86e9acfdd2dc7a9b1576b
100644
--- a/Source/core/css/resolver/StyleResolver.h
+++ b/Source/core/css/resolver/StyleResolver.h
@@ -304,7 +304,6 @@ private:
     void applyProperties(StyleResolverState&, const StylePropertySet*
properties, StyleRule*, bool isImportant, bool inheritedOnly,
PropertyWhitelistType = PropertyWhitelistNone);
     template <StyleApplicationPass pass>
     void applyAnimatedProperties(StyleResolverState&, const Element*, const
DocumentTimeline*);
-    void resolveVariables(StyleResolverState&, CSSPropertyID, CSSValue*,
Vector<std::pair<CSSPropertyID, String> >& knownExpressions);
     void matchPageRules(MatchResult&, RuleSet*, bool isLeftPage, bool
isFirstPage, const String& pageName);
     void matchPageRulesForList(Vector<StyleRulePage*>& matchedRules, const
Vector<StyleRulePage*>&, bool isLeftPage, bool isFirstPage, const String&
pageName);
     void collectViewportRules();
@@ -325,8 +324,6 @@ private:
 
     void cacheBorderAndBackground();
 
-    void applyProperty(StyleResolverState&, CSSPropertyID, CSSValue*);
-
     MatchedPropertiesCache m_matchedPropertiesCache;
 
     OwnPtr<MediaQueryEvaluator> m_medium;
@@ -357,9 +354,6 @@ private:
 #ifdef STYLE_STATS
     static StyleSharingStats m_styleSharingStats;
 #endif
-
-    friend void StyleBuilder::oldApplyProperty(CSSPropertyID, StyleResolver*,
StyleResolverState&, CSSValue*, bool isInitial, bool isInherit);
-
 };
 
 inline bool checkRegionSelector(const CSSSelector* regionSelector, Element*
regionElement)

[dglazkov, 2013-08-02 15:57:39]

Rebased

[commit-bot: I haz the power, 2013-08-02 15:58:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dglazkov@chromium.org/21625003/22001

[dglazkov, 2013-08-02 16:36:47]

On 2013/08/01 22:53:48, eseidel wrote:
> Seems like we should have a bug filed, or some sort of path forward here.

Filed http://crbug.com/267622 and http://crbug.com/267612.

[commit-bot: I haz the power, 2013-08-02 17:27:17]

Change committed as 155446