Remove RenderStyle member from FontBuilder.

Remove RenderStyle member from FontBuilder.

The FontBuilder currently operates directly on RenderStyle every time a
font property is set. As indicated by the now-removed FIXME, this
is most unclean, and makes the FontBuilder not very attractive to use in
cases where you don't wish to spam the RenderStyle with setFontDescription
calls. (Example: FontBuilder::createFontForDocument).

This patch adds a FontDescription member instead, and changes
FontDescriptionChangeScope to act as a pure mark-if-dirty class to let
functions modify m_fontDescription directly, rather than through 'scope'.

R=dglazkov@chromium.org

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

[dglazkov, 2014-11-18 15:52:49]

lgtm

[andersr, 2014-11-19 14:22:58]

Patchset #2 (id:20001) has been deleted

[andersr, 2014-11-19 22:39:33]

On 2014/11/18 15:52:49, dglazkov wrote:
> lgtm

Hmm, maybe I should find another way of doing this, I had to make some changes
to StyleResolver which I didn't expect. (See Diff between Patch 4 and Patch 2).

 * When inheriting cached properties, the Font is replaced completely after the
FontBuilder is created, and so the FontDescription of the builder must be
updated. (This is acceptable, IMO).
 * When creating a RenderStyle snapshot for animations, we apply the value for a
single property, and then use the computed value on RenderStyle for that
property. However, with this change, font properties are not really set on the
RenderStyle until the Font is created via FontBuilder, so that needs to happen
before we involve CSSAnimatableValueFactory. (I'm not sure how acceptable this
is, especially since we don't really need the computed size and Font object in
this case).

WDYT?

[dglazkov, 2014-11-20 19:22:14]

https://codereview.chromium.org/714163002/diff/80001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/714163002/diff/80001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:719:
state.fontBuilder().createFont(state.document().styleEngine()->fontSelector(),
state.style(), state.parentStyle());
Let's ask animation folks? :)

Mike, who would be the right person to check on this?

[andersr, 2014-11-21 14:02:30]

andersr@opera.com changed reviewers:
+ timloh@chromium.org

[andersr, 2014-11-21 14:52:12]

On 2014/11/20 19:22:14, dglazkov wrote:
>
https://codereview.chromium.org/714163002/diff/80001/Source/core/css/resolver...
> File Source/core/css/resolver/StyleResolver.cpp (right):
> 
>
https://codereview.chromium.org/714163002/diff/80001/Source/core/css/resolver...
> Source/core/css/resolver/StyleResolver.cpp:719:
> state.fontBuilder().createFont(state.document().styleEngine()->fontSelector(),
> state.style(), state.parentStyle());
> Let's ask animation folks? :)
> 
> Mike, who would be the right person to check on this?

Possibly timloh@.

The problem is that the design of Font (immutable FontDescription) calls for a
builder pattern, whereas createAnimatableValueSnapshot assumes that a
"sufficiently" computed RenderStyle exists after any call to
StyleBuilder::applyProperty.

::createAnimatableValueSnapshot doesn't even really need to create a full
RenderStyle, it's just using this structure to store the one (ish) value it
needs, which is immediately extracted by CSSAnimatableValueFactory. Ideally, we
would have a generated API to convert any one CSSValue into the corresponding
internal representation without creating an entire RenderStyle, and
CSSAnimatableValueFactory could use that directly. Unfortunately this is not
currently (easily) possible for various reasons.

Updating the Font *is* the right thing to do here in principle, but it comes off
as noise, which sort of negates the point of doing code cleanup in the first
place.

( Note: two related patches, depending on this one:
  * https://codereview.chromium.org/715633006/ and
  * https://codereview.chromium.org/712333004/ )

[Timothy Loh, 2014-11-24 06:55:07]

timloh@chromium.org changed reviewers:
+ dstockwell@chromium.org

[Timothy Loh, 2014-11-24 06:55:08]

This is probably fine, we only go through here when starting
animations/transitions. Is createFont a no-op if we haven't changed font
parameters?

[andersr, 2014-11-24 09:39:41]

On 2014/11/24 06:55:08, Timothy Loh wrote:
> Is createFont a no-op if we haven't changed font parameters?

Yes.

[Timothy Loh, 2014-11-24 22:58:29]

On 2014/11/24 09:39:41, andersr wrote:
> On 2014/11/24 06:55:08, Timothy Loh wrote:
> > Is createFont a no-op if we haven't changed font parameters?
> 
> Yes.

lgtm

[andersr, 2014-11-26 09:07:22]

The CQ bit was checked by andersr@opera.com

[commit-bot: I haz the power, 2014-11-26 09:08:08]

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

[commit-bot: I haz the power, 2014-11-26 10:24:46]

Committed patchset #4 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186023