Remove FontDescriptionChangeScope, and let FontBuilder partially apply values.

Remove FontDescriptionChangeScope, and let FontBuilder partially apply values.

FontBuilder is now so simple that we don't need a scoped object to keep track
of changes to the FontDescription.

This patch adds a PropertySetFlag for each property on FontBuilder/FontDescription,
such that we only write the touched values to the output FontDescription.
Hence, the FontDescription we base the output FontDescription on is provided at the
time of FontBuilder::createFont, rather than during FontBuilder construction.

This has the benefit of callers not having to update FontBuilder's FontDescription
whenever the FontDescription of the output RenderStyle changes, as we currently
have to do in e.g. StyleResolverState::setStyle.

Notes:

 * Calculating the specified size from a keyword size has been delayed to
   createFont, since it depends on the font family.
 * Re-calculating the specified size from a keyword is no longer necessary
   in setFamilyDescription.
 * The parent style is no longer passed to the FontBuilder, because the parent
   FontDescription is effectively available through the current RenderStyle at
   the time of createFont.
 * Removed unused FontBuilder::inheritFrom.
 * Changed FontBuilder::checkForOrientationChange (now updateOrientation),
   to always update the flags. Not doing so just adds extra comparisons,
   even before this patch.

R=dglazkov@chromium.org

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

[dglazkov, 2014-11-18 15:37:02]

lgtm

[andersr, 2014-12-18 13:01:59]

PTAL when you return. Description updated. (Warning: Patch #3/4 is quite
different from #1/#2).

Had to take a different approach, since the old patch is incompatible with
animating word-spacing/letter-spacing. The problem is that word-spacing and
letter-spacing are not handled by FontBuilder, yet they are stored on the
FontDescription. This led to a situation where word-spacing and letter-spacing
would be set directly on the RenderStyle::fontDescription, but then restored to
the previous value later during FontBuilder::createFont. Note that I think it's
correct that these properties are not handled through FontBuilder, since they
depend on the built font.

The new patch fixes this by partially applying the FontBuilder vaules on top of
a FontDescription provided during createFont instead of during FontBuilder
construction. This has some other benefits too. (See description).

https://codereview.chromium.org/715633006/diff/60001/Source/core/css/resolver...
File Source/core/css/resolver/FontBuilder.cpp (right):

https://codereview.chromium.org/715633006/diff/60001/Source/core/css/resolver...
Source/core/css/resolver/FontBuilder.cpp:341: if (isSet(IsSetFlag::Family)) {
Note: I've verified with a microbenchmark that all these 'if' statements do not
degrade font building performance measurably. (Actually, it increased slightly).

[andersr, 2015-02-03 11:44:24]

andersr@opera.com changed reviewers:
+ rune@opera.com

[andersr, 2015-02-03 11:44:24]

@rune, summary:

 * Yes, this makes FontBuilder more complicated, but:
 * Much less FontDescription creation/copying is going on, and, more
importantly:
 * There is less "protocol" for the calling code to follow, i.e. we don't have
to worry about keeping the FontDescription of the RenderStyle and the
FontDescription of the FontBuilder in sync during style building.

[rune, 2015-02-03 12:25:40]

https://codereview.chromium.org/715633006/diff/60001/Source/core/css/resolver...
File Source/core/css/resolver/FontBuilder.h (right):

https://codereview.chromium.org/715633006/diff/60001/Source/core/css/resolver...
Source/core/css/resolver/FontBuilder.h:48: void didChangeWritingMode(bool);
I would have removed the bool parameters and checked before calling since
passing false is a no-op.

https://codereview.chromium.org/715633006/diff/60001/Source/core/css/resolver...
Source/core/css/resolver/FontBuilder.h:102: enum class IsSetFlag {
How about something more descriptive like DescriptorFlag?

[andersr, 2015-02-03 15:35:08]

https://codereview.chromium.org/715633006/diff/60001/Source/core/css/resolver...
File Source/core/css/resolver/FontBuilder.h (right):

https://codereview.chromium.org/715633006/diff/60001/Source/core/css/resolver...
Source/core/css/resolver/FontBuilder.h:48: void didChangeWritingMode(bool);
On 2015/02/03 12:25:40, rune wrote:
> I would have removed the bool parameters and checked before calling since
> passing false is a no-op.

Done.

https://codereview.chromium.org/715633006/diff/60001/Source/core/css/resolver...
Source/core/css/resolver/FontBuilder.h:102: enum class IsSetFlag {
On 2015/02/03 12:25:40, rune wrote:
> How about something more descriptive like DescriptorFlag?

Do you mean DescriptionFlag?

[rune, 2015-02-03 21:52:35]

On 2015/02/03 15:35:08, andersr wrote:
>
https://codereview.chromium.org/715633006/diff/60001/Source/core/css/resolver...
> File Source/core/css/resolver/FontBuilder.h (right):
> 
>
https://codereview.chromium.org/715633006/diff/60001/Source/core/css/resolver...
> Source/core/css/resolver/FontBuilder.h:48: void didChangeWritingMode(bool);
> On 2015/02/03 12:25:40, rune wrote:
> > I would have removed the bool parameters and checked before calling since
> > passing false is a no-op.
> 
> Done.
> 
>
https://codereview.chromium.org/715633006/diff/60001/Source/core/css/resolver...
> Source/core/css/resolver/FontBuilder.h:102: enum class IsSetFlag {
> On 2015/02/03 12:25:40, rune wrote:
> > How about something more descriptive like DescriptorFlag?
> 
> Do you mean DescriptionFlag?

I meant DescriptorFlag like in font descriptors. Or perhaps DescriptorSetFlag or
PropertySetFlag, since these come from properties, not descriptors?

[andersr, 2015-02-04 09:06:43]

On 2015/02/03 21:52:35, rune wrote:
> On 2015/02/03 15:35:08, andersr wrote:
> >
>
https://codereview.chromium.org/715633006/diff/60001/Source/core/css/resolver...
> > File Source/core/css/resolver/FontBuilder.h (right):
> > 
> >
>
https://codereview.chromium.org/715633006/diff/60001/Source/core/css/resolver...
> > Source/core/css/resolver/FontBuilder.h:48: void didChangeWritingMode(bool);
> > On 2015/02/03 12:25:40, rune wrote:
> > > I would have removed the bool parameters and checked before calling since
> > > passing false is a no-op.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/715633006/diff/60001/Source/core/css/resolver...
> > Source/core/css/resolver/FontBuilder.h:102: enum class IsSetFlag {
> > On 2015/02/03 12:25:40, rune wrote:
> > > How about something more descriptive like DescriptorFlag?
> > 
> > Do you mean DescriptionFlag?
> 
> I meant DescriptorFlag like in font descriptors. Or perhaps DescriptorSetFlag
or
> PropertySetFlag, since these come from properties, not descriptors?

Done (PropertySetFlag). Commit description also updated with new name.

[rune, 2015-02-04 09:32:26]

lgtm

[andersr, 2015-02-04 10:14:40]

The CQ bit was checked by andersr@opera.com

[commit-bot: I haz the power, 2015-02-04 10:16:58]

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

[commit-bot: I haz the power, 2015-02-04 11:49:20]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189488