Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(455)

Issue 21625003: StyleBuilder should not know about StyleResolver. (Closed)

Created:
7 years, 4 months ago by dglazkov
Modified:
7 years, 4 months ago
Reviewers:
esprehn, eae, eseidel
CC:
blink-reviews, apavlov+blink_chromium.org, dglazkov+blink, eae+blinkwatch, darktears, Timothy Loh
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 9

Patch Set 2 : Don't crash me, bro. #

Patch Set 3 : Don't crash me, bro. #

Total comments: 2

Patch Set 4 : Rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+225 lines, -223 lines) Patch
M Source/core/css/resolver/StyleBuilder.h View 1 1 chunk +5 lines, -3 lines 0 comments Download
M Source/core/css/resolver/StyleBuilderCustom.cpp View 1 2 3 29 chunks +183 lines, -65 lines 0 comments Download
M Source/core/css/resolver/StyleResolver.h View 1 2 3 3 chunks +0 lines, -6 lines 0 comments Download
M Source/core/css/resolver/StyleResolver.cpp View 1 2 3 6 chunks +6 lines, -124 lines 0 comments Download
M Source/core/css/resolver/StyleResolverState.h View 1 2 2 chunks +5 lines, -2 lines 0 comments Download
M Source/core/css/resolver/StyleResolverState.cpp View 1 2 chunks +8 lines, -6 lines 0 comments Download
M Source/core/scripts/templates/StyleBuilder.cpp.tmpl View 6 chunks +8 lines, -8 lines 0 comments Download
M Source/core/scripts/templates/StyleBuilderFunctions.cpp.tmpl View 2 chunks +7 lines, -5 lines 0 comments Download
M Source/core/scripts/templates/StyleBuilderFunctions.h.tmpl View 2 chunks +3 lines, -4 lines 0 comments Download

Messages

Total messages: 17 (0 generated)
dglazkov
This depends on https://codereview.chromium.org/20672003/ and https://codereview.chromium.org/20598006/ landing first.
7 years, 4 months ago (2013-08-01 20:49:38 UTC) #1
esprehn
LGTM. Qapla! https://codereview.chromium.org/21625003/diff/1/Source/core/css/resolver/StyleBuilder.h File Source/core/css/resolver/StyleBuilder.h (right): https://codereview.chromium.org/21625003/diff/1/Source/core/css/resolver/StyleBuilder.h#newcode46 Source/core/css/resolver/StyleBuilder.h:46: static void applyProperty(CSSPropertyID, StyleResolverState&, CSSValue*); This is ...
7 years, 4 months ago (2013-08-01 21:07:13 UTC) #2
eseidel
Cool. So what's StyleBuilder's job? W/o an explicit out-parameter it seems his job is to ...
7 years, 4 months ago (2013-08-01 21:09:53 UTC) #3
eseidel
lgtm
7 years, 4 months ago (2013-08-01 21:10:10 UTC) #4
dglazkov
https://codereview.chromium.org/21625003/diff/1/Source/core/css/resolver/StyleBuilder.h File Source/core/css/resolver/StyleBuilder.h (right): https://codereview.chromium.org/21625003/diff/1/Source/core/css/resolver/StyleBuilder.h#newcode45 Source/core/css/resolver/StyleBuilder.h:45: // FIXME: What's the endgame for this function? On ...
7 years, 4 months ago (2013-08-01 21:16:19 UTC) #5
dglazkov
On 2013/08/01 21:09:53, eseidel wrote: > Cool. So what's StyleBuilder's job? W/o an explicit out-parameter ...
7 years, 4 months ago (2013-08-01 21:19:59 UTC) #6
dglazkov
https://codereview.chromium.org/21625003/diff/1/Source/core/css/resolver/StyleBuilderCustom.cpp File Source/core/css/resolver/StyleBuilderCustom.cpp (right): https://codereview.chromium.org/21625003/diff/1/Source/core/css/resolver/StyleBuilderCustom.cpp#newcode277 Source/core/css/resolver/StyleBuilderCustom.cpp:277: if (Frame* frame = state.elementContext().document()->frame()) not lgtm. This one ...
7 years, 4 months ago (2013-08-01 21:47:49 UTC) #7
dglazkov
not lgtm for now. Lemme fix this first.
7 years, 4 months ago (2013-08-01 21:48:11 UTC) #8
dglazkov
Don't crash me, bro.
7 years, 4 months ago (2013-08-01 22:16:52 UTC) #9
dglazkov
PTAL. https://codereview.chromium.org/21625003/diff/15001/Source/core/css/resolver/StyleResolverState.h File Source/core/css/resolver/StyleResolverState.h (right): https://codereview.chromium.org/21625003/diff/15001/Source/core/css/resolver/StyleResolverState.h#newcode50 Source/core/css/resolver/StyleResolverState.h:50: Document* document() const { return m_document; } This ...
7 years, 4 months ago (2013-08-01 22:19:41 UTC) #10
eseidel
lgtm https://codereview.chromium.org/21625003/diff/15001/Source/core/css/resolver/StyleResolverState.h File Source/core/css/resolver/StyleResolverState.h (right): https://codereview.chromium.org/21625003/diff/15001/Source/core/css/resolver/StyleResolverState.h#newcode50 Source/core/css/resolver/StyleResolverState.h:50: Document* document() const { return m_document; } On ...
7 years, 4 months ago (2013-08-01 22:53:48 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dglazkov@chromium.org/21625003/15001
7 years, 4 months ago (2013-08-02 05:08:25 UTC) #12
commit-bot: I haz the power
Failed to apply patch for Source/core/css/resolver/StyleResolver.h: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
7 years, 4 months ago (2013-08-02 05:08:36 UTC) #13
dglazkov
Rebased
7 years, 4 months ago (2013-08-02 15:57:39 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dglazkov@chromium.org/21625003/22001
7 years, 4 months ago (2013-08-02 15:58:35 UTC) #15
dglazkov
On 2013/08/01 22:53:48, eseidel wrote: > Seems like we should have a bug filed, or ...
7 years, 4 months ago (2013-08-02 16:36:47 UTC) #16
commit-bot: I haz the power
7 years, 4 months ago (2013-08-02 17:27:17 UTC) #17
Message was sent while issue was closed.
Change committed as 155446

Powered by Google App Engine
This is Rietveld 408576698