Layout Team & Project Warden Status - March 30, 2015

[Emil A Eklund]

The layout team is a long-term team that owns the layout code in blink.
The current focus is primarily on code-health (building on Project
Warden) and layout performance.
See https://sites.google.com/a/chromium.org/dev/blink/layout-team for
more information.

Warden is a project to cleanup and fix long standing issues in order
to increase developer productivity.
See goo.gl/VEv1YQ for more information.


We had Rune Lillesveen (rune) and David Vest (davve) from Opera join the
meeting this week in an effort to better coordinate our work. We also had
Ian Killpatrick (ikilpatrick) from the Chrome Sydney team join us.

The massive Rendering -> Layout rename project is done, thanks to
everyone who helped out and to Dan in particular who did most of the
heavy lifting!

The new multi-column implementation was enabled for tests and exposed as
an experimental web platform feature on Friday! Congrats Morten!


Updates since last meeting (on Monday, March 23rd):

Performance Tracking (benjhayden)
- Continuing work on LayoutAnalyzer and counters. Counters will be
exposed via trace events allowing us to see the number of layouts on a
per element type basis.
- Working on adding a command line flag for android to allow metrics to
be collected on an attached phone.

Scrolling (skobes) [crbug.com/417782]
- Fixed the scroll animation test that was failing on the leak bots.
- Fixed a bug with scrolling to fixed-position #anchor targets.
- Still tests failing that I need to dig into, and I need to add more
virtual test suites as right now it is only running fast/scrolling.

Rename Rendering -> Layout (dsinclair) [crbug.com/450612]
- Done. All files have been moved and renamed. The rendering/ directory
is gone!

First letter refactoring (dsinclair) [crbug.com/391288]
- Fixed first-letter hit testing, discovered a couple of new bugs.

List marker refactoring [crbug.com/370461]
- Getting back to list-marker work, will turn the marker into a ::marker
psedo element. [crbug.com/457718]

Measure API (jchaffraix)
- Been thinking about supporting measure out-of-tree and trying to come
up with a good strategy. So far none of the approaches tried have
panned out. Will continue to experiment and evaluate ideas to guide
future work.

Flexbox (cbiesinger) [crbug.com/426898]
- Finished implementing spec changes for min-width auto.
- Started working on positioning of position: absolute elements.
- Working on importing w3c test suite for flexbox.

Line Boxes (szager) [crbug.com/321237]
- Finished going through the layout test failures for Glenn's patch, and
landed a few patches to fix tests. All of the remaining test failures
are OK to rebaseline.
- Ran the patch through the perf bots, and the results were not as I had
hoped. There appeared to be significant performance regressions in the
blink_perf.layout test suite.
- Working on eliminating wrapper classes (FloatWillBeLayoutUnit) to test
theory that they explain at least part of the regression.
- Found a number of places where we use the wrong unit, we need to
decide exactly where to draw the line between LayoutUnit and float.

Isolate core/fetch (japhet) [crbug.com/458222]
- Continuing work to isolate core/fetch from the rest of core.

Page scale handling (bokan) [crbug.com/459591]
- Remove old pinch-zoom paths from Blink.

Blink componentization (pilgrim) [crbug.com/428284]
- Moving things from core to modules; core/storage and core/timing.

Text (kojii, wjmaclean, eae, rune)
- Continued work on text iterator cleanup, fixed naming to be more
descriptive. (wjmaclean)
- Working on a change that pulls out the part of TextIterator that
handles text from the part that handles traversing. (wjmaclean)
- Profiling line breaking. (eae)
- Started working on design doc for removing simple text. (eae)
- Fixed text overflow box bug that blocked the Slimming Paint work.
(eae)
- Fixed font cache bug that caused an extra layout due to the
FontCache version() invaliding cache entries when a new @font-face
rule is encountered. (rune)
- Fix vertical flow regression when a major web font site provides
broken tables. (kojii)
- Default to complex path more cases in vertical flow. (kojii)
- Working with W3C and internal experts for bi-di and vertical flow
issues. (kojii)

Style resolution (rune)
- Cache element indices for :nth-child and :nth-last-child selectors.

Content sizing (davve)
- Working on content sizing/intrinsic size for HTML and SVG.

Region-based multi-column support (mstensho) [crbug.com/334335]
- New multi-column implementation enabled as experimental web platform
feature!

Standards (ikilpatrick, slightlyoff)
- Pushing the layout portion of w3c houdini forward in preparation for
the next meeting by prototyping. (ikilpatrick)

ClusterFuzz
- Trying to reduce an obscure clusterfuzz crasher in RenderBlockFlow.
(wkorman)


Discussions:

[ cbiesinger's work to import the w3c test suite prompted a discussion
about w3c tests. ]
<slightlyoff> Are we mostly writing blink specific layout tests or w3c
web platform tests today?
<eae> For regression and performance work we use blink specific
layout tests pretty much exclusively. For feature work
we're trying to move to a world where we use web platform
tests tests but we're not there yet.
<jsbell> Dirk (dpranke) started work in tooling to support w3c web
platform tests, I have headcount for an engineer to take
this over and work on improving the tooling.
<jsbell> Once better tooling and procedures are in place we should
start "strongly encouraging" people to write web platform
tests for new features.
<slightlyoff> Can we do that today?
<jsbell> It is too too painful to do today, tooling needs to be
improved first.

[Rune Lillesveen]

On Mon, Mar 30, 2015 at 10:24 PM, 'Emil A Eklund' via blink-dev
<blin...@chromium.org> wrote:

> - Fixed font cache bug that caused an extra layout due to the
> FontCache version() invaliding cache entries when a new @font-face
> rule is encountered. (rune)

Correction: I fixed an assertion problem with baseLayoutStyle which
lead to finding/reporting the mentioned problem with unnecessary
layouts (crbug.com/471079).

--
Rune Lillesveen

[Emil A Eklund]

Thanks Rune, I've updated the online version of the notes with your correction.

[PhistucK]

On Mon, Mar 30, 2015 at 11:24 PM, 'Emil A Eklund' via blink-dev <blin...@chromium.org> wrote:

Content sizing (davve)
- Working on content sizing/intrinsic size for HTML and SVG.

​Is this http://dev.w3.org/csswg/css-sizing-3/?​ If so, this is great! Perhaps send an intent to implement?

☆PhistucK

[David Vest]

Not anything more than plumbing at this stage. Is there great author
demand for exposing that kind of layout information?

David

[Christian Biesinger]

...don't we already support most of that, though prefixed?

-christian

[Christian Biesinger]

On Mon, Mar 30, 2015 at 4:24 PM, 'Emil A Eklund' via layout-dev
<layou...@chromium.org> wrote:
> The new multi-column implementation was enabled for tests and exposed as
> an experimental web platform feature on Friday! Congrats Morten!

Yay! What are the plans for shipping this and removing the old version?

-christian

[Christian Biesinger]

On Mon, Mar 30, 2015 at 4:24 PM, 'Emil A Eklund' via layout-dev
<layou...@chromium.org> wrote:

> The new multi-column implementation was enabled for tests and exposed as
> an experimental web platform feature on Friday! Congrats Morten!

[David Vest]

On Tue, Mar 31, 2015 at 11:17 PM, Christian Biesinger
<cbies...@chromium.org> wrote:
>> Not anything more than plumbing at this stage. Is there great author
>> demand for exposing that kind of layout information?
>
> ...don't we already support most of that, though prefixed?

Yes, and it looks like you have implemented a big part of it[1]. :)
Though I see no support for the presumably newer parts: the
contain-floats keyword and the column-width extensions. An intent to
implement sounds a little late, I agree.

Also I found the "legacy" values (non-prefixed) |intrinsic| and
|min-intrinsic|. Filed https://codereview.chromium.org/1051573003/ for
adding some use counters.

David

[1] https://codereview.chromium.org/13674002/#ps3001

[Christian Biesinger]

On Wed, Apr 1, 2015 at 10:56 AM, David Vest <da...@opera.com> wrote:
> On Tue, Mar 31, 2015 at 11:17 PM, Christian Biesinger
> <cbies...@chromium.org> wrote:
>>> Not anything more than plumbing at this stage. Is there great author
>>> demand for exposing that kind of layout information?
>>
>> ...don't we already support most of that, though prefixed?
>
> Yes, and it looks like you have implemented a big part of it[1]. :)

Just the height ones :) The width ones already worked.

> Though I see no support for the presumably newer parts: the
> contain-floats keyword and the column-width extensions. An intent to
> implement sounds a little late, I agree.

True, we do not support contain-floats. I don't know how we implement
column-width, so I'll trust you that that support does not come
automatically.

> Also I found the "legacy" values (non-prefixed) |intrinsic| and
> |min-intrinsic|. Filed https://codereview.chromium.org/1051573003/ for
> adding some use counters.

It would be great if we could remove those!

-christian

[Morten Stenshorne]

Unfortunately that CL caused fuzzer bots to explode, so it had to be
reverted. :(

I'll fix the issues and reland it.

Then: after some reasonable bake time as experimental (I don't
know... one milestone or so?), we can enable it for stable, and as soon
as we're confident that it's good and it's safe to burn all bridges, we
can remove the runtime flag and kill the old implementation along with
it.

--
---- Morten Stenshorne, developer, Opera Software ASA ----
------------------ http://www.opera.com/ -----------------

[Ojan Vafai]

 Tue, Apr 14, 2015 at 2:28 AM Morten Stenshorne <mste...@opera.com> wrote:

Christian Biesinger <cbies...@chromium.org> writes:

> On Mon, Mar 30, 2015 at 4:24 PM, 'Emil A Eklund' via layout-dev
> <layou...@chromium.org> wrote:
>> The new multi-column implementation was enabled for tests and exposed as
>> an experimental web platform feature on Friday! Congrats Morten!
>
> Yay! What are the plans for shipping this and removing the old version?

Unfortunately that CL caused fuzzer bots to explode, so it had to be
reverted. :(

I'll fix the issues and reland it.

Then: after some reasonable bake time as experimental (I don't
know... one milestone or so?), we can enable it for stable,

IMO, give it a week after turning it on as experimental, just to make sure there are no new fuzzer things, then turn it on for stable. There's little benefit in leaving it as experimental just for the sake of real world usage because very few users actually use the experimental flag regularly. The point of experimental is mainly for things we *know* are not ready to ship yet.

 

and as soon
as we're confident that it's good and it's safe to burn all bridges, we
can remove the runtime flag and kill the old implementation along with
it.

Yup. For a big change like this, it probably makes sense to wait until after the release actually makes it to stable before deleting the old code. Don't think you need to wait longer than that.

[Morten Stenshorne]

Ojan Vafai <oj...@chromium.org> writes:

>  Tue, Apr 14, 2015 at 2:28 AM Morten Stenshorne <mste...@opera.com> wrote:
>
> Christian Biesinger <cbies...@chromium.org> writes:
>
> > On Mon, Mar 30, 2015 at 4:24 PM, 'Emil A Eklund' via layout-dev
> > <layou...@chromium.org> wrote:
> >> The new multi-column implementation was enabled for tests and exposed as
> >> an experimental web platform feature on Friday! Congrats Morten!
> >
> > Yay! What are the plans for shipping this and removing the old version?
>
> Unfortunately that CL caused fuzzer bots to explode, so it had to be
> reverted. :(
>
> I'll fix the issues and reland it.
>
> Then: after some reasonable bake time as experimental (I don't
> know... one milestone or so?), we can enable it for stable,
>
> IMO, give it a week after turning it on as experimental, just to make sure there are no new fuzzer things, then turn
> it on for stable. There's little benefit in leaving it as experimental just for the sake of real world usage because
> very few users actually use the experimental flag regularly. The point of experimental is mainly for things we *know*
> are not ready to ship yet.

Works for me. I'll do that. Once we enable it for stable, though, we'll
lose all coverage for the old implementation in layout tests - unless I
do something clever. But I assume that there's no need to retain such
coverage for such a short period for such a moribund implementation?

> and as soon
> as we're confident that it's good and it's safe to burn all bridges, we
> can remove the runtime flag and kill the old implementation along with
> it.
>
> Yup. For a big change like this, it probably makes sense to wait until
> after the release actually makes it to stable before deleting the old
> code. Don't think you need to wait longer than that.

OK, I'll do precisely that, unless someone objects.

Uh, do we need an Intent-to-Ship here, BTW?

We have this on the dashboard:
https://www.chromestatus.com/feature/6526151266664448

Note that we cannot really claim full support until we support nested
multicol, though. [1][2]

[1] https://code.google.com/p/chromium/issues/detail?id=99358
[2] https://code.google.com/p/chromium/issues/detail?id=447718

[Ojan Vafai]

On Wed, Apr 15, 2015 at 4:01 AM Morten Stenshorne <mste...@opera.com> wrote:

Ojan Vafai <oj...@chromium.org> writes:

>  Tue, Apr 14, 2015 at 2:28 AM Morten Stenshorne <mste...@opera.com> wrote:
>
>     Christian Biesinger <cbies...@chromium.org> writes:
>
>     > On Mon, Mar 30, 2015 at 4:24 PM, 'Emil A Eklund' via layout-dev
>     > <layou...@chromium.org> wrote:
>     >> The new multi-column implementation was enabled for tests and exposed as
>     >> an experimental web platform feature on Friday! Congrats Morten!
>     >
>     > Yay! What are the plans for shipping this and removing the old version?
>
>     Unfortunately that CL caused fuzzer bots to explode, so it had to be
>     reverted. :(
>
>     I'll fix the issues and reland it.
>
>     Then: after some reasonable bake time as experimental (I don't
>     know... one milestone or so?), we can enable it for stable,
>
> IMO, give it a week after turning it on as experimental, just to make sure there are no new fuzzer things, then turn
> it on for stable. There's little benefit in leaving it as experimental just for the sake of real world usage because
> very few users actually use the experimental flag regularly. The point of experimental is mainly for things we *know*
> are not ready to ship yet.

Works for me. I'll do that. Once we enable it for stable, though, we'll
lose all coverage for the old implementation in layout tests - unless I
do something clever. But I assume that there's no need to retain such
coverage for such a short period for such a moribund implementation?

Yup.

 

>     and as soon
>     as we're confident that it's good and it's safe to burn all bridges, we
>     can remove the runtime flag and kill the old implementation along with
>     it.
>
> Yup. For a big change like this, it probably makes sense to wait until
> after the release actually makes it to stable before deleting the old
> code. Don't think you need to wait longer than that.

OK, I'll do precisely that, unless someone objects.

Uh, do we need an Intent-to-Ship here, BTW?

It depends, what is changing in terms of API surface?

[Julien Chaffraix]

IIRC there was little change in the API surface. I would have gone for
a PSA as it's just a rewrite (implementation detail).

Julien

[Morten Stenshorne]

Well, it does add support for the column-fill property. But that's
probably minor enough to just PSA it.

[Ojan Vafai]

>>> Uh, do we need an Intent-to-Ship here, BTW?
>>
>>
>> It depends, what is changing in terms of API surface?
>
> IIRC there was little change in the API surface. I would have gone for
> a PSA as it's just a rewrite (implementation detail).

Well, it does add support for the column-fill property. But that's
probably minor enough to just PSA it.

I think column-fill should have an intent-to-ship. The actual flipping of the flag might want a separate PSA. 

[Elliott Sprehn]

If we think this is ready we should enable it for stable right after the branch point. Wait a little bit so it can trickle down through dev channel, and assuming things seem good we can start removing the old multi col system.

--
You received this message because you are subscribed to the Google Groups "layout-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to layout-dev+...@chromium.org.
To post to this group, send email to layou...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/layout-dev/CAC7ek3QoZGjWxvpCwzrHWS%2BVkeDB2K8AR6xuqibwCQ5BWY%2BmZg%40mail.gmail.com.

[Ojan Vafai]

I agree that right after the branch point is a reasonable point to flip from experimental to stable to give maximum bake-time in dev/beta releases. On further reflection, you're probably right that it's worth deleting the old code after one or two dev channel releases (i.e. two weeks) since we won't really be able to safely unflip the flag for much longer after that point anyways because the old code will then be the untested codepath.

[Chris Harrelson]

I would prefer if we go all in now on the feature (i.e. turn it on and delete the old code right away). The existence of two modes continues to exert a tax on teams, and it's not a big risk since multicol only exists on a small fraction of pages.

Chris

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

[Morten Stenshorne]

We have now reached the point where new multicol has been on for
experimental for two weeks (landed on May the fourth). One fuzzer crash
was found and fixed last week.

So, now what? Should I intent-to-ship it right away, or wait until a new
beta branch is created? There seems to be differing opinions.


Elliott Sprehn <esp...@chromium.org> writes (Wed, 15 Apr 2015 18:24:34 -0700):

[Douglas Stockwell]

Ideally, for features that could impact a significant amount of existing content, we would enable by default as far from the branch date as possible. But an intent to ship can be sent at any time.

Conveniently the next branch date is May 15 (https://www.chromium.org/developers/calendar)

- Doug

[Chris Harrelson]

Hi Morten,

First, congratulations!

I think you should go ahead and send the Intent to Ship on Monday and on the same day flip the flag. 

It might be best to wait a week or two before deleting the old code, but for sure I think you should do that as soon as you can, so that we can clean up related code like Slimming Paint etc. We hope to ship Slimming Paint phase one with Chrome 45, so this will definitely help us, and probably other efforts ongoing or starting soon to refactor the layout code.

 

Chris

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

[Ojan Vafai]

+1

[Emil A Eklund]

> I think you should go ahead and send the Intent to Ship on Monday and on the
> same day flip the flag.

I agree, exciting times!

[Robert Hogan]

On Wed, Apr 1, 2015 at 3:56 PM David Vest <da...@opera.com> wrote:

Also I found the "legacy" values (non-prefixed) |intrinsic| and
|min-intrinsic|. Filed https://codereview.chromium.org/1051573003/ for
adding some use counters.

 Is it too early to learn the results of this? There are a bunch of ways to trigger clusterfuzz asserts because our support for these 2 values is inadequate. e.g. https://code.google.com/p/chromium/issues/detail?id=477768

[Philip Jägenstedt]

It is too early, but the counters are in M44 so in a week or so they should have stabilized:

https://www.chromestatus.com/metrics/feature/timeline/popularity/751

https://www.chromestatus.com/metrics/feature/timeline/popularity/752 

Philip

[Christian Biesinger]

Hmm, it looks like LegacyCSSValueIntrinsic is now standing at 0.012%,
whereas LegacyCSSValueMinIntrinsic is at 0.07%.

Someone remind me of our threshold?

-christian

[Robert Hogan]

The threshold applied in the past is 0.03%. That was for prefixed
features - this is an unprefixed feature that isn't documented
anywhere now AFAICT.

[Christian Biesinger]

Not sure I understand your implication -- are you suggesting that for
this undocumented feature it would still be OK to remove despite the
higher usage for min-intrinsic, or not...?

-christian

[Robert Hogan]

I think prefixing a feature suggests a degree of UA ownership outside
what any specification might say. If we support a feature that was
once specified but is not any longer, then it seems the bar to
deprecation should be lower. Authors shouldn't have any reasonable
expectation for an unprefixed, experimental feature that is not in any
specification to be respected.

[L. David Baron]

On Tuesday 2015-08-04 22:17 +0100, Robert Hogan wrote:
> I think prefixing a feature suggests a degree of UA ownership outside
> what any specification might say. If we support a feature that was
> once specified but is not any longer, then it seems the bar to
> deprecation should be lower. Authors shouldn't have any reasonable
> expectation for an unprefixed, experimental feature that is not in any
> specification to be respected.

It's still in a specification; it's just been renamed to min-content
and max-content.

http://www.w3.org/TR/css3-sizing/#width-height-keywords

(I'd link to the editor's draft, but it's broken today.)

-David

--
𝄞 L. David Baron http://dbaron.org/ 𝄂
𝄢 Mozilla https://www.mozilla.org/ 𝄂
Before I built a wall I'd ask to know
What I was walling in or walling out,
And to whom I was like to give offense.
- Robert Frost, Mending Wall (1914)

[Robert Hogan]

On 4 August 2015 at 22:27, L. David Baron <dba...@dbaron.org> wrote:
> On Tuesday 2015-08-04 22:17 +0100, Robert Hogan wrote:
>> I think prefixing a feature suggests a degree of UA ownership outside
>> what any specification might say. If we support a feature that was
>> once specified but is not any longer, then it seems the bar to
>> deprecation should be lower. Authors shouldn't have any reasonable
>> expectation for an unprefixed, experimental feature that is not in any
>> specification to be respected.
>
> It's still in a specification; it's just been renamed to min-content
> and max-content.
>
> http://www.w3.org/TR/css3-sizing/#width-height-keywords
>

Right, and we would continue to support those keywords. We would just
stop recognizing 'intrinsic' and 'min-intrinsic'.

[Philip Jägenstedt]

What kind of breakage is to be expected if they're removed, and is
Blink the only engine with support?

Usage is certainly low enough that starting the deprecation/removal
process seems reasonable.

Philip

[Christian Biesinger]

To drive this forward, I'm going to send an intent to deprecate momentarily.

-christian