Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18553003:
Add a runtime flag for distributing extra logical height to rowspans (Closed)
|
Created:
7 years, 5 months ago by a.suchit Modified:
7 years, 5 months ago Reviewers:
Julien - ping for review CC:
blink-reviews, dglazkov+blink, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionWhile the feature is being worked on, we expect some amount of
regression churn that we don't want to propagate to the official builds.
We also want the new feature to be tested in Blink, which is why a
temporary status=test flag makes the most sense.
R=jchaffraix@chromium.org
BUG=78724
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=153699
Patch Set 1 #
Total comments: 17
Patch Set 2 : Review comments addressed #
Total comments: 2
Patch Set 3 : Using RuntimeEnabledFeature for fixing 'extra height of rowspan is not distributing in all rows und… #
Total comments: 2
Patch Set 4 : Review comments Addressed #
Messages
Total messages: 13 (0 generated)
https://codereview.chromium.org/18553003/diff/1/Source/core/page/RuntimeEnabl... File Source/core/page/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/18553003/diff/1/Source/core/page/RuntimeEnabl... Source/core/page/RuntimeEnabledFeatures.in:71: RowSpanLogicalHeightSpreading I don't think we should have the feature disabled by default when testing. We know the regressions, it's just important that they don't impact our users. That means that this should be status=test so that it's enabled when testing. https://codereview.chromium.org/18553003/diff/1/Source/core/rendering/RenderT... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/18553003/diff/1/Source/core/rendering/RenderT... Source/core/rendering/RenderTableSection.cpp:27: #include "core/rendering/RenderTableSection.h" should be here per our coding style. https://codereview.chromium.org/18553003/diff/1/Source/core/rendering/RenderT... Source/core/rendering/RenderTableSection.cpp:35: #include "RuntimeEnabledFeatures.h" We are not consistent in how we sort the #includes but it RuntimeEnabledFeatures.h shouldn't be among core/ #includes. I would have put it just before wtf/ #includes (to keep everything alphabetical). https://codereview.chromium.org/18553003/diff/1/Source/core/rendering/RenderT... Source/core/rendering/RenderTableSection.cpp:379: // FIXME: We are always adding the height of a rowspan to the last rows which doesn't match We probably want to remove this FIXME but have it mention the crbug and saying that it's a work-in-progress. https://codereview.chromium.org/18553003/diff/1/Source/core/rendering/RenderT... Source/core/rendering/RenderTableSection.cpp:420: if (!rowSpanCells.isEmpty()) rowSpanCells will be empty if the feature is disabled AFAICT so I would rather have an ASSERT instead.
https://codereview.chromium.org/18553003/diff/1/Source/core/page/RuntimeEnabl... File Source/core/page/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/18553003/diff/1/Source/core/page/RuntimeEnabl... Source/core/page/RuntimeEnabledFeatures.in:71: RowSpanLogicalHeightSpreading On 2013/07/02 17:39:28, Julien Chaffraix wrote: > I don't think we should have the feature disabled by default when testing. We > know the regressions, it's just important that they don't impact our users. > > That means that this should be status=test so that it's enabled when testing. If I use status=true for runtimeEnabledFeature then it will show many regression. is it ok for you? https://codereview.chromium.org/18553003/diff/1/Source/core/rendering/RenderT... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/18553003/diff/1/Source/core/rendering/RenderT... Source/core/rendering/RenderTableSection.cpp:27: On 2013/07/02 17:39:28, Julien Chaffraix wrote: > #include "core/rendering/RenderTableSection.h" should be here per our coding > style. It is giving me presubmit failure. https://codereview.chromium.org/18553003/diff/1/Source/core/rendering/RenderT... Source/core/rendering/RenderTableSection.cpp:35: #include "RuntimeEnabledFeatures.h" On 2013/07/02 17:39:28, Julien Chaffraix wrote: > We are not consistent in how we sort the #includes but it > RuntimeEnabledFeatures.h shouldn't be among core/ #includes. I would have put it > just before wtf/ #includes (to keep everything alphabetical). It gives me presubmit failure. https://codereview.chromium.org/18553003/diff/1/Source/core/rendering/RenderT... Source/core/rendering/RenderTableSection.cpp:379: // FIXME: We are always adding the height of a rowspan to the last rows which doesn't match On 2013/07/02 17:39:28, Julien Chaffraix wrote: > We probably want to remove this FIXME but have it mention the crbug and saying > that it's a work-in-progress. Done. https://codereview.chromium.org/18553003/diff/1/Source/core/rendering/RenderT... Source/core/rendering/RenderTableSection.cpp:420: if (!rowSpanCells.isEmpty()) On 2013/07/02 17:39:28, Julien Chaffraix wrote: > rowSpanCells will be empty if the feature is disabled AFAICT so I would rather > have an ASSERT instead. do you want me to use both if(RuntimeEnabledFeature::rowSpanLogicalHeightSpreadingEnabled()) and ASSERT(RuntimeEnabledFeature::rowSpanLogicalHeightSpreadingEnabled())? There is no use of assert if 'if condition' is present.
https://codereview.chromium.org/18553003/diff/1/Source/core/page/RuntimeEnabl... File Source/core/page/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/18553003/diff/1/Source/core/page/RuntimeEnabl... Source/core/page/RuntimeEnabledFeatures.in:71: RowSpanLogicalHeightSpreading On 2013/07/02 18:33:35, a.suchit wrote: > On 2013/07/02 17:39:28, Julien Chaffraix wrote: > > I don't think we should have the feature disabled by default when testing. We > > know the regressions, it's just important that they don't impact our users. > > > > That means that this should be status=test so that it's enabled when testing. > > If I use status=true for runtimeEnabledFeature then it will show many > regression. is it ok for you? That's intended. It will limit the feature to testing thus your code gets tested as you develop but we prevent people from experiencing the regressions in regular builds. BTW, it's status=test not true. https://codereview.chromium.org/18553003/diff/1/Source/core/rendering/RenderT... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/18553003/diff/1/Source/core/rendering/RenderT... Source/core/rendering/RenderTableSection.cpp:27: On 2013/07/02 18:33:35, a.suchit wrote: > On 2013/07/02 17:39:28, Julien Chaffraix wrote: > > #include "core/rendering/RenderTableSection.h" should be here per our coding > > style. > > It is giving me presubmit failure. #include "config.h" #include "core/rendering/RenderTableSection.h" # include <limits> ... doesn't (and shouldn't) give any error. https://codereview.chromium.org/18553003/diff/1/Source/core/rendering/RenderT... Source/core/rendering/RenderTableSection.cpp:35: #include "RuntimeEnabledFeatures.h" On 2013/07/02 18:33:35, a.suchit wrote: > On 2013/07/02 17:39:28, Julien Chaffraix wrote: > > We are not consistent in how we sort the #includes but it > > RuntimeEnabledFeatures.h shouldn't be among core/ #includes. I would have put > it > > just before wtf/ #includes (to keep everything alphabetical). > > It gives me presubmit failure. The way you went triggers a style violation for me. The way to go is #include "RuntimeEnabledFeatures.h" just after <limits>. https://codereview.chromium.org/18553003/diff/1/Source/core/rendering/RenderT... Source/core/rendering/RenderTableSection.cpp:420: if (!rowSpanCells.isEmpty()) On 2013/07/02 18:33:35, a.suchit wrote: > On 2013/07/02 17:39:28, Julien Chaffraix wrote: > > rowSpanCells will be empty if the feature is disabled AFAICT so I would rather > > have an ASSERT instead. > > do you want me to use both > if(RuntimeEnabledFeature::rowSpanLogicalHeightSpreadingEnabled()) and > ASSERT(RuntimeEnabledFeature::rowSpanLogicalHeightSpreadingEnabled())? > > There is no use of assert if 'if condition' is present. That's what I meant as you only collect rowSpanCells if RuntimeEnabledFeatures::rowSpanLogicalHeightSpreadingEnabled(): if (!rowSpanCells.isEmpty()) { ASSERT(RuntimeEnabledFeatures::rowSpanLogicalHeightSpreadingEnabled()); distributeRowSpanHeightToRows(rowSpanCells); }
https://codereview.chromium.org/18553003/diff/1/Source/core/rendering/RenderT... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/18553003/diff/1/Source/core/rendering/RenderT... Source/core/rendering/RenderTableSection.cpp:27: On 2013/07/02 19:08:58, Julien Chaffraix wrote: > On 2013/07/02 18:33:35, a.suchit wrote: > > On 2013/07/02 17:39:28, Julien Chaffraix wrote: > > > #include "core/rendering/RenderTableSection.h" should be here per our coding > > > style. > > > > It is giving me presubmit failure. > > #include "config.h" > #include "core/rendering/RenderTableSection.h" > > # include <limits> > ... > > doesn't (and shouldn't) give any error. Done. https://codereview.chromium.org/18553003/diff/1/Source/core/rendering/RenderT... Source/core/rendering/RenderTableSection.cpp:35: #include "RuntimeEnabledFeatures.h" On 2013/07/02 19:08:58, Julien Chaffraix wrote: > On 2013/07/02 18:33:35, a.suchit wrote: > > On 2013/07/02 17:39:28, Julien Chaffraix wrote: > > > We are not consistent in how we sort the #includes but it > > > RuntimeEnabledFeatures.h shouldn't be among core/ #includes. I would have > put > > it > > > just before wtf/ #includes (to keep everything alphabetical). > > > > It gives me presubmit failure. > > The way you went triggers a style violation for me. The way to go is #include > "RuntimeEnabledFeatures.h" just after <limits>. Done. https://codereview.chromium.org/18553003/diff/1/Source/core/rendering/RenderT... Source/core/rendering/RenderTableSection.cpp:420: if (!rowSpanCells.isEmpty()) On 2013/07/02 19:08:58, Julien Chaffraix wrote: > On 2013/07/02 18:33:35, a.suchit wrote: > > On 2013/07/02 17:39:28, Julien Chaffraix wrote: > > > rowSpanCells will be empty if the feature is disabled AFAICT so I would > rather > > > have an ASSERT instead. > > > > do you want me to use both > > if(RuntimeEnabledFeature::rowSpanLogicalHeightSpreadingEnabled()) and > > ASSERT(RuntimeEnabledFeature::rowSpanLogicalHeightSpreadingEnabled())? > > > > There is no use of assert if 'if condition' is present. > > That's what I meant as you only collect rowSpanCells if > RuntimeEnabledFeatures::rowSpanLogicalHeightSpreadingEnabled(): > > if (!rowSpanCells.isEmpty()) { > ASSERT(RuntimeEnabledFeatures::rowSpanLogicalHeightSpreadingEnabled()); > distributeRowSpanHeightToRows(rowSpanCells); > } Done.
I don't think you need the rebaselines as Ojan did them this week and the flag should be enabled when running the tests. Also please create a meaningful, properly formatted description! The rule of thumb for format is to follow git commit format (see http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html) though we do not enforce the 50 characters limit but the idea is that it should be a _short_ summary of the current change. About the content, all your changes starts with: "The remaining logical height is wrongly added to the last row of a table, not spread on the rows." Which makes NO SENSE for this change as it's unrelated to the main change (arguably it's also a description that is so vague as to be useless). If you want to reference the master bug, you should use a BUG= line. https://codereview.chromium.org/18553003/diff/20001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/18553003/diff/20001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:29: #include "RuntimeEnabledFeatures.h" Nit: Add a FIXME to remove this when the master bug closes.
-t "hello"
Fixed all review comments.
https://codereview.chromium.org/18553003/diff/20001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/18553003/diff/20001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:29: #include "RuntimeEnabledFeatures.h" On 2013/07/03 19:52:26, Julien Chaffraix wrote: > Nit: Add a FIXME to remove this when the master bug closes. Done.
lgtm. Thanks for updating the description but it's still not great: - The summary is way too long (which makes it not really a summary) - We usually want to avoid having lines longer than 80 columns (see https://groups.google.com/a/chromium.org/d/msg/blink-dev/rqGhY7PfkYY/OIVsNxro...) Ideally the summary should state what you did (while being *really* to the point) and you can expand below it to explain the 'why'. Here would be my take: Add a runtime flag for distributing extra logical height to rowspans While the feature is being worked on, we expect some amount of regression churn that we don't want to propagate to the official builds. We also want the new feature to be tested in Blink, which is why a temporary status=test flag makes the most sense. BUG=78724 https://codereview.chromium.org/18553003/diff/47001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/18553003/diff/47001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:29: // FIXME: Remove 'RuntimeEnabledFeatures.h' when the master bug #78724 is closed. Better still: when http://crbug.com/78724 is closed. (this makes the bug number clickable if you are using a smart enough editor) https://codereview.chromium.org/18553003/diff/47001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:383: // other browsers. See crbug.com/78724 for example. Not sure I understand your comment. First you repeat crbug.com/78724 which seems useless but mostly, you are (again) repeating what the code already says in a more concise, clearer way. If you want to put something, here would be my take: // FIXME: We add all the logical row of a rowspan to the last rows // until crbug.com/78724 is fixed and the runtime flag removed. // This avoids propagating temporary regressions while we fix the bug.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a.suchit@samsung.com/18553003/53001
Message was sent while issue was closed.
Change committed as 153699
Message was sent while issue was closed.
Next time, you have to repeat the summary in the description or else the patch lands without a summary which is nasty (as it did here). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
