Demote 'line-height' to a low priority property.

Demote 'line-height' to a low priority property.

Our special handling of the 'line-height' property is no longer
required, since the 'font' shorthand is now expanded in the parser
in all cases (also for system fonts).

R=timloh@chromium.org

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

[andersr, 2014-10-23 15:29:26]

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

[Timothy Loh, 2014-10-23 15:41:33]

This doesn't work because of crbug.com/353932.

If you set "line-height: 1234px; font: caption", then the line-height from the
font property should be the resulting cascaded value. However since system fonts
aren't expanded, we'd get a line-height of 1234px.

alancutter tried to fix this in the past, crrev.com/205743004 (I basically had
the same patch as this ready to go, crrev.com/206083003). IIRC there were
problems with system fonts in UA stylesheets getting parsed in web workers on
mac where the font data was unavailable. You could probably take over the issue
if you wanted.

[Timothy Loh, 2014-10-23 15:42:06]

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

[Timothy Loh, 2014-10-23 15:42:06]

whoops, meant to cc alancutter on the last message

[rune, 2014-10-23 15:42:57]

rune@opera.com changed reviewers:
- alancutter@chromium.org

[rune, 2014-10-23 15:42:58]

lgtm

[rune, 2014-10-23 15:45:38]

On 2014/10/23 at 15:42:58, rune wrote:
> lgtm

ignore this then.

[andersr, 2014-10-23 16:26:32]

On 2014/10/23 15:41:33, Timothy Loh wrote:
> This doesn't work because of crbug.com/353932.
> 
> If you set "line-height: 1234px; font: caption", then the line-height from the
> font property should be the resulting cascaded value. However since system
fonts
> aren't expanded, we'd get a line-height of 1234px.

Right, of course. Actually, you've explained this before.

Ugh, the ugliness. :(

> IIRC there were
> problems with system fonts in UA stylesheets getting parsed in web workers on
> mac where the font data was unavailable.

Heh.

[andersr, 2014-10-27 14:24:52]

Re-opened this, and updated description.

Now that https://codereview.chromium.org/677103002/ has landed, this should be
landable too.

PTAL.

[Timothy Loh, 2014-10-27 14:28:19]

On 2014/10/27 14:24:52, andersr wrote:
> Re-opened this, and updated description.
> 
> Now that https://codereview.chromium.org/677103002/ has landed, this should be
> landable too.
> 
> PTAL.

lgtm, yay.

[Timothy Loh, 2014-10-27 14:29:05]

On 2014/10/27 14:28:19, Timothy Loh wrote:
> On 2014/10/27 14:24:52, andersr wrote:
> > Re-opened this, and updated description.
> > 
> > Now that https://codereview.chromium.org/677103002/ has landed, this should
be
> > landable too.
> > 
> > PTAL.
> 
> lgtm, yay.

But maybe wait a day or two in case you're worried about the font-shorthand
patch being reverted for breaking anything (I think it should be fine though)

[andersr, 2014-10-27 14:34:06]

On 2014/10/27 14:29:05, Timothy Loh wrote:
> On 2014/10/27 14:28:19, Timothy Loh wrote:
> > On 2014/10/27 14:24:52, andersr wrote:
> > > Re-opened this, and updated description.
> > > 
> > > Now that https://codereview.chromium.org/677103002/ has landed, this
should
> be
> > > landable too.
> > > 
> > > PTAL.
> > 
> > lgtm, yay.
> 
> But maybe wait a day or two in case you're worried about the font-shorthand
> patch being reverted for breaking anything (I think it should be fine though)

Yes, I was thinking the same thing.

[andersr, 2014-10-30 08:37:12]

The CQ bit was checked by andersr@opera.com

[commit-bot: I haz the power, 2014-10-30 08:38:14]

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

[commit-bot: I haz the power, 2014-10-30 09:13:10]

Committed patchset #2 (id:20001) as 184629