Bug 2005007 Comment 8 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

Let's not do that, for several reasons:

1. Bug 1726232 is being used as a tracker-bug, and we'll use its dependency-list as a way to track followup items and newly-discovered issues until (and perhaps after) the implementation is high-quality enough to be shippable.  So it's best not to attach any patches there -- even a potentially-complete patch or patch-series -- since we automatically close bugs once their attached patches land, and don't want to close bug 1726232 until we've gotten enough Nightly testing & fixed any identified issues & decided the feature is shippable.

2. Breaking up the patch helps ease cognitive load for reviewers (think about one set of things at a time and sign off on that set of things without having to get through everything)

3. Breaking up the patch helps helps direct the different pieces to different experts (folks reviewing the CSS pieces may not be experts in the webrender clipping/painting pieces)

4. Breaking up the patch lets "finished" parts get checked in, even as issues or open-questions are worked out in later parts.  (So hypothetically we could check in your CSS parsing pieces -- behind an off-by-default about:config preference -- without needing to block that on having the rendering bits all done.)

5. It's a useful exercise to split up your patch (for a feature like this) into two or more pieces, as a way of making sure your patch's approach is right; that helps to identify unused parts or parts that don't fit together.

I'd suggest splitting this into at least two pieces:
(A) the css parts where we parse the property but it doesn't impact rendering
(B) the rendering parts
Those should be in entirely different files, probably -- and if you've got a working patch, I'd expect you could just look at the files-touched and split them into part (A) or part (B), and generate two patches, without too much trouble.
Let's not do that, for several reasons:

1. Bug 1726232 is being used as a tracker-bug, and we'll use its dependency-list as a way to track followup items and newly-discovered issues until (and perhaps after) the implementation is high-quality enough to be shippable.  So it's best not to attach any patches there -- even a potentially-complete patch or patch-series -- since we automatically close bugs once their attached patches land, and don't want to close bug 1726232 until we've gotten enough Nightly testing & fixed any identified issues & decided the feature is shippable.

2. Breaking up the patch helps ease cognitive load for reviewers (think about one set of things at a time and sign off on that set of things without having to get through everything)

3. Breaking up the patch helps helps direct the different pieces to different experts (folks reviewing the CSS pieces may not be experts in the webrender clipping/painting pieces)

4. Breaking up the patch lets "finished" parts get checked in, even as issues or open-questions are worked out in later parts.  (So hypothetically we could check in your CSS parsing pieces -- behind an off-by-default about:config preference -- without needing to block that on having the rendering bits all done.)

5. It's a useful exercise to split up your patch (for a feature like this) into two or more pieces, as a way of making sure your patch's approach is right; that helps to identify unused parts or parts that don't fit together, and it's a useful bit of "self-review".

I'd suggest splitting this into at least two pieces:
(A) the css parts where we parse the property but it doesn't impact rendering
(B) the rendering parts
Those should be in entirely different files, probably -- and if you've got a working patch, I'd expect you could just look at the files-touched and split them into part (A) or part (B), and generate two patches, without too much trouble.
Let's not do that, for several reasons:

1. Bug 1726232 is being used as a tracker-bug, and we'll use its dependency-list as a way to track followup items and newly-discovered issues until (and perhaps after) the implementation is high-quality enough to be shippable.  So it's best not to attach any patches there -- even a potentially-complete patch or patch-series -- since we automatically close bugs once their attached patches land, and don't want to close bug 1726232 until we've gotten enough Nightly testing & fixed any identified issues & decided the feature is shippable.

2. Breaking up the patch helps ease cognitive load for reviewers (think about one set of things at a time and sign off on that set of things without having to get through everything)

3. Breaking up the patch helps helps direct the different pieces to different experts (folks reviewing the CSS pieces may not be experts in the webrender clipping/painting pieces)

4. Breaking up the patch lets "finished" parts get checked in, even as issues or open-questions are worked out in later parts.  (So hypothetically we could check in your CSS parsing pieces -- behind an off-by-default about:config preference -- without needing to block that on having the rendering bits all done.)

5. It's a useful exercise to split up your patch (for a feature like this) into two or more pieces, as a way of making sure your patch's approach is right; that helps to identify unused parts or parts that don't fit together, and it's a useful bit of "self-review".

I'd suggest splitting this into at least two pieces:
(A) the css parts where we parse the property but it doesn't impact rendering
(B) the rendering parts

Those parts should be touching entirely different sets of files, probably -- and if you've got a working patch, I'd expect you could just look at the files-touched and split them into part (A) or part (B), and generate two patches, without too much trouble.

Back to Bug 2005007 Comment 8