Renaming Render to Layout.

[Dan Sinclair]

With the crbug changes landed to update labels I figure it's about time to start on phase two of the grand renaming. Classes and files.

Discussing with Julien, we think the least disruptive is to do each file individually instead of a giant move/rename. To that end, we propose for each file individually in core/rendering to do a git move from core/rendering to core/layout and a rename from Render to Layout at the same time. Update the class name from Render* to Layout* and TBR in the change.

This will require a lot of CLs, for the cases where the renderers are rarely touched we'll batch them, for more active classes we'll do them individually.  Each CL will move the files and update the includes/references. This should save the disruption of breaking every patch that's in progress.

Thoughts, comments, should I start moving/renaming?

dan

[Chris Harrelson]

Sounds good to me. We did something similar but arguably more disruptive for core/paint/ and it went pretty smoothly.

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

[Julien Chaffraix]

On Tue, Jan 20, 2015 at 12:10 PM, Chris Harrelson <chri...@chromium.org> wrote:
> Sounds good to me. We did something similar but arguably more disruptive for
> core/paint/ and it went pretty smoothly.

Just a follow-up as it seems that there was some confusion / push-back
on part of the renaming and we haven't done a good job at sending a
clear plan and explaining some of the details.

* Everything under rendering/ will get moved, the goal is to remove
rendering/ altogether (which involves the subdirectories, e.g.
rendering/style/).

* There are some classes that are not a straight s/Render/Layout.
Below is the list of the controversial renamings, along with the
reason and the proposed names. Comments or improved names are welcome:
- rendering/RenderLayer -> paint/DeprecatedPaintLayer: LayoutLayer is
pretty wrong as RenderLayer is mostly a paint-time object and it would
be better under paint/. The new name doesn't totally cover the class'
responsibilities but it describes where we want to go (this will be
covered by a class comment to explain the rationale).
- RenderTableLayout -> LayoutTableAlgorithm: (same for
RenderTableLayoutAuto.* and RenderTableLayoutFixed.*):
LayoutTableLayoutAuto would make it look like a renderer when it's not
(the old name had this issue already). Those corresponds to layout
table algorithms so it makes sense to use a name reflecting that.
- RenderOverflow -> Overflow: LayoutOverflow is confusing because it
makes it sounds like a renderer but also because we have the concept
of layout vs visual overflow. This is the reason why it's better to
drop the Layout-prefix. A less terse and maybe more descriptive names
would be OverflowBoxes.h or OverflowModel.h

Thanks,
Julien

[Morten Stenshorne]

Julien Chaffraix <jchaf...@chromium.org> writes:

> On Tue, Jan 20, 2015 at 12:10 PM, Chris Harrelson <chri...@chromium.org> wrote:
>> Sounds good to me. We did something similar but arguably more disruptive for
>> core/paint/ and it went pretty smoothly.
>
> Just a follow-up as it seems that there was some confusion / push-back
> on part of the renaming and we haven't done a good job at sending a
> clear plan and explaining some of the details.
>
> * Everything under rendering/ will get moved, the goal is to remove
> rendering/ altogether (which involves the subdirectories, e.g.
> rendering/style/).
>
> * There are some classes that are not a straight s/Render/Layout.
> Below is the list of the controversial renamings, along with the
> reason and the proposed names. Comments or improved names are welcome:
> - rendering/RenderLayer -> paint/DeprecatedPaintLayer: LayoutLayer is
> pretty wrong as RenderLayer is mostly a paint-time object and it would
> be better under paint/.

Someone needs to figure out the stacking order (z-index, etc.) during
layout, and that's currently something (what used to be called)
RenderLayer does, right? But that could easily be handled by some other
class, I suppose.

Apart from that, it's mostly about painting ... oh, and hit-testing.
This is the class that takes painting and hit-testing on a ride through
the layout tree. LayoutTreeTraversalObject? Or something that doesn't
start with "Layout"?

> The new name doesn't totally cover the class'
> responsibilities but it describes where we want to go (this will be
> covered by a class comment to explain the rationale).
> - RenderTableLayout -> LayoutTableAlgorithm: (same for
> RenderTableLayoutAuto.* and RenderTableLayoutFixed.*):
> LayoutTableLayoutAuto would make it look like a renderer when it's not
> (the old name had this issue already). Those corresponds to layout
> table algorithms so it makes sense to use a name reflecting that.

Actually, it used to be called TableLayout*, not RenderTableLayout*, so
there wasn't really any confusion when this lived in rendering/. But
yeah, having both LayoutTable and TableLayout would probably please
nobody. If it's the bikeshedding hour already, though, I would like to
point out that TableLayoutAlgorithm* would have been better than
LayoutTableAlgorithm*, just to make it clear that it's still not a
layout object. Open up chapter 17 of the CSS2.1 spec, and search for
"table layout algorithm". :) Yeah, so that would be the name I would
choose.

> - RenderOverflow -> Overflow: LayoutOverflow is confusing because it
> makes it sounds like a renderer but also because we have the concept
> of layout vs visual overflow. This is the reason why it's better to
> drop the Layout-prefix. A less terse and maybe more descriptive names
> would be OverflowBoxes.h or OverflowModel.h

Or, if ClipRects has been found to be a good name for our various clip
rectangles, maybe we could have OverflowRects for overflow rectangles?

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

[Levi Weintraub]

Thanks for calling out these trickier bits, and dealing with my annoying push-back on Layer!

I'll also take this opportunity to also express a slight preference for OverflowModel.h over just Overflow.h.

 

Thanks,
Julien

[Elliott Sprehn]

z-index is a paint concept, layout doesn't use it.

 

[Julien Chaffraix]

On Fri, Feb 20, 2015 at 11:37 AM, Morten Stenshorne <mste...@opera.com> wrote:
> Julien Chaffraix <jchaf...@chromium.org> writes:
>
>> On Tue, Jan 20, 2015 at 12:10 PM, Chris Harrelson <chri...@chromium.org> wrote:
>>> Sounds good to me. We did something similar but arguably more disruptive for
>>> core/paint/ and it went pretty smoothly.
>>
>> Just a follow-up as it seems that there was some confusion / push-back
>> on part of the renaming and we haven't done a good job at sending a
>> clear plan and explaining some of the details.
>>
>> * Everything under rendering/ will get moved, the goal is to remove
>> rendering/ altogether (which involves the subdirectories, e.g.
>> rendering/style/).
>>
>> * There are some classes that are not a straight s/Render/Layout.
>> Below is the list of the controversial renamings, along with the
>> reason and the proposed names. Comments or improved names are welcome:
>> - rendering/RenderLayer -> paint/DeprecatedPaintLayer: LayoutLayer is
>> pretty wrong as RenderLayer is mostly a paint-time object and it would
>> be better under paint/.
>
> Someone needs to figure out the stacking order (z-index, etc.) during
> layout, and that's currently something (what used to be called)
> RenderLayer does, right? But that could easily be handled by some other
> class, I suppose.

You're thinking of RenderLayerStackingNode here, which while not
RenderLayer is closely associated with it.

Ideally we would want those satellite objects off RenderLayer (at
least the one unrelated to painting / hit-testing) so that we don't
have an object with several features. Also as Elliott pointed out,
those are paint-time concepts, not layout.

> Apart from that, it's mostly about painting ... oh, and hit-testing.
> This is the class that takes painting and hit-testing on a ride through
> the layout tree. LayoutTreeTraversalObject? Or something that doesn't
> start with "Layout"?

Well this is definitely a paint (and hit-testing) object, which is why
I think it's better fitted into the paint directory. FWIW I would
rather have better designed object handling the tree traversal, with
clear responsibilities, thus the push for deprecating this object
(also slimming paint may remove some of its usage).

>> The new name doesn't totally cover the class'
>> responsibilities but it describes where we want to go (this will be
>> covered by a class comment to explain the rationale).
>> - RenderTableLayout -> LayoutTableAlgorithm: (same for
>> RenderTableLayoutAuto.* and RenderTableLayoutFixed.*):
>> LayoutTableLayoutAuto would make it look like a renderer when it's not
>> (the old name had this issue already). Those corresponds to layout
>> table algorithms so it makes sense to use a name reflecting that.
>
> Actually, it used to be called TableLayout*, not RenderTableLayout*, so
> there wasn't really any confusion when this lived in rendering/. But
> yeah, having both LayoutTable and TableLayout would probably please
> nobody. If it's the bikeshedding hour already, though, I would like to
> point out that TableLayoutAlgorithm* would have been better than
> LayoutTableAlgorithm*, just to make it clear that it's still not a
> layout object. Open up chapter 17 of the CSS2.1 spec, and search for
> "table layout algorithm". :) Yeah, so that would be the name I would
> choose.

Fair point. I am fine with TableLayoutAlgorithm, which on top of
matching the English word order, doesn't seem like a LayoutObject :-)

>> - RenderOverflow -> Overflow: LayoutOverflow is confusing because it
>> makes it sounds like a renderer but also because we have the concept
>> of layout vs visual overflow. This is the reason why it's better to
>> drop the Layout-prefix. A less terse and maybe more descriptive names
>> would be OverflowBoxes.h or OverflowModel.h
>
> Or, if ClipRects has been found to be a good name for our various clip
> rectangles, maybe we could have OverflowRects for overflow rectangles?

OverflowRects seems better than OverflowBoxes. Thinking about it more,
OverflowModel has a big advantage: it allows us to change how we model
overflow (we currently don't track overflow from positioned objects as
it would yield to a giant united rectangle so we could decide that
overflow is not a rectangle anymore but a Vector<Rect> or a region).

Julien

[Dan Sinclair]

One other non-substitution rename I think makes sense is:

RenderCombineText -> LayoutTextCombine.

This will group it with the rest of the LayoutText objects and fits with the RenderTextFragment we also have.

dan

[Dan Sinclair]

Came up with a plan for landing the RenderBlock change. The issue with this change is there are ~27k expectation files and several image files which need to be rebaselined. The CQ is locked to a max of 5k files in a given change so this can't run through the CQ as is.

One way to do it is to dcommit the whole thing in one go without going through the CQ.

I think, better way is:

Rename RenderBlock* -> LayoutBlock* without changing renderName(). This shouldn't have any effect on layout tests and can land through the CQ.

I'll then change the renderName() values and run through the trybots to get a list of failing tests. I'll then generate the ~10 CLs that will be needed to fixup all of the names, these CLs will be uploaded individually to rietveld. I then closed the tree and dcommit the test expectation text changes and the TestExpectation change to do a NeedsRebaseline of any image failures. Once those are all in, I'll do a pull and make sure I didn't miss any text expectation changes, and fixup if needed. At this point, we can re-open the tree.

There may still be a few straggler image failures that will need to be added to test expectations to green the tree which can be added to TestExpectations and committed.

I think this is the safest route. It closes the tree while the changes happen. The code change to revert is small, and just a lot of test expectations to revert.

I can do this on a Saturday or Sunday to minimize the tree being closed during working hours.

(We can do this after all of the other renames are done so if anything else needs a large rebaseline it can happen at the same time and in the same way so we only close the tree once)

Thoughts?

[Dan Sinclair]

And just to followup, of the files left to move, the ones that won't fit below the 5k boundary are:

RenderBlock(Flow)  27k files

RenderInline              9k files

RenderText              26k files

RenderView             29k files

So those would all go through the above procedure. Have their files moved with renderName left as Render* and then when the tree is closed I'd update all the files and commit.

With the tree closed, I'd do all of the changes at the same time, so one CL to change all the renderNames() and each batch of expectation changes would update all of the Render names in those files.

dan

[Stefan Zager]

It sounds more or less fine to me, although I don't really understand why you want to jump through hoops to upload the test expectations to rietveld before you dcommit them.  Since you're not running try jobs or using the CQ for those, what is gained by sharding them into ten patches just so you can upload them to rietveld?

[Dan Sinclair]

Maybe nothing? Other then having a history of what happened. Just though it would be nice if the commits were attached to a code review.

dan

[Stefan Zager]

If you want the historical record to be clear, I think it makes sense to do it in one commit.  Otherwise, we're stuck doing things like: "Hey, I wonder if such-and-such-test.html was rebased during that big Render/Layout naming change?  Lets go look through ten code reviews to see if it's in any of them."  Just my two cents, I don't feel strongly.

[Dan Sinclair]

Fair enough. In that case, I can commit all the text changes as one dcommit that isn't uploaded to rietveld.

dan

[Dirk Pranke]

For what it's worth, when I used to large baseline updates, I would usually review things manually and dcommit them directly. No one would actually want to look at a Rietveld CL that had many thousands of files in it, even if the tool wouldn't fall over on upload :).

I also think there's a lot of value in Stefan's suggestion to try and keep things in one commit. The auto-rebaseline-bot has been generally a huge improvement in our lives, but it does sometimes make me sad when it spends most of the day churning through a big rebaseline, commit after commit.

-- Dirk

[Levi Weintraub]

On Tue, Feb 24, 2015 at 6:52 AM, Dan Sinclair <dsin...@chromium.org> wrote:

Came up with a plan for landing the RenderBlock change. The issue with this change is there are ~27k expectation files and several image files which need to be rebaselined. The CQ is locked to a max of 5k files in a given change so this can't run through the CQ as is.

One way to do it is to dcommit the whole thing in one go without going through the CQ.

I think, better way is:

Rename RenderBlock* -> LayoutBlock* without changing renderName(). This shouldn't have any effect on layout tests and can land through the CQ.

I'll then change the renderName() values and run through the trybots to get a list of failing tests. I'll then generate the ~10 CLs that will be needed to fixup all of the names, these CLs will be uploaded individually to rietveld. I then closed the tree and dcommit the test expectation text changes and the TestExpectation change to do a NeedsRebaseline of any image failures. Once those are all in, I'll do a pull and make sure I didn't miss any text expectation changes, and fixup if needed. At this point, we can re-open the tree.

There may still be a few straggler image failures that will need to be added to test expectations to green the tree which can be added to TestExpectations and committed.

Why would this renaming result in image failures?

[Dan Sinclair]

Because there are tests that say things like: "Fixes bug in RenderBlock::blah" which have pixel results. I've been updating those RenderBlock::blah's to LayoutBlock::blahs so they still make sense, but in a few cases, get IMAGE failures on try runs. There are a very limited number of these if memory serves.

dan

[Levi Weintraub]

Thanks for the explanation :)

[Primiano Tucci]

For what it's worth, when I used to large baseline updates, I would usually review things manually and dcommit them directly. No one would actually want to look at a Rietveld CL that had many thousands of files in it, even if the tool wouldn't fall over on upload :).

Confirm. Regardless of the upload limit, rietveld web UI chokes with just few hundreds file*, even without the extension. See crrev.com/852953002 as an example.

Quoting from crbug.com/456533:

"It looks like Rietveld does linear work per patchset..."

"That is, linear *and* cumulative"

[Dan Sinclair]

I haven't heard any comments, so I'm going to go ahead with the following at EOD (EST) today unless anyone chimes up.

RenderOverflow -> OverflowModel

RenderCombineText -> LayoutTextCombine

Thanks,

dan

[Levi Weintraub]

SGTM.

[Julien Chaffraix]

>> I haven't heard any comments, so I'm going to go ahead with the following
>> at EOD (EST) today unless anyone chimes up.
>>
>> RenderOverflow -> OverflowModel
>> RenderCombineText -> LayoutTextCombine

Dan forgot also 2 proposals that people seemed to agree with. Those
will be taken care of soonish:
- LayoutTableAlgorithm -> TableLayoutAlgorithm (thanks Morten for the
suggestion)
- paint/Layer (aka RenderLayer) -> paint/DeprecatedPaintLayer, a class
comment will be added to DeprecatedPaintLayer explaining the
situation. The satellite objects will also be moved for coherency
(thanks to Levi and Philip for the push-back on this)

Julien