Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2669433002:
Moved table-layout property to be generated in ComputedStyleBase. (Closed)
|
Created:
3 years, 10 months ago by shend Modified:
3 years, 10 months ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-style_chromium.org, chromium-reviews, dglazkov+blink, rwlbuis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMoved table-layout property to be generated in ComputedStyleBase.
Move the property table-layout and its enum, ETableLayout,
to be generated in ComputedStyleBase. Also moved getter / setter /
initial value methods to be generated in ComputedStyleBase.
BUG=628043
Review-Url: https://codereview.chromium.org/2669433002
Cr-Commit-Position: refs/heads/master@{#447937}
Committed: https://chromium.googlesource.com/chromium/src/+/14a1ac0a3d3be32ac70c9cf16b8f9405355e5d79
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebase #
Total comments: 1
Patch Set 3 : Rebase correctly #
Total comments: 2
Patch Set 4 : Address comments on comments #
Total comments: 2
Patch Set 5 : Fix comments #Patch Set 6 : Rebase #
Messages
Total messages: 42 (21 generated)
shend@chromium.org changed reviewers: + bugsnash@chromium.org
Hey Bugs, PTAL :)
On 2017/01/31 at 04:30:01, shend wrote: > Hey Bugs, PTAL :) Deleting more code that you're adding: feels good
https://codereview.chromium.org/2669433002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/style/ComputedStyle.h (left): https://codereview.chromium.org/2669433002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/style/ComputedStyle.h:230: m_tableLayout == other.m_tableLayout && could we add some comment to the code that indicates that some of these properties are being generated so that developers don't get confused as to why they see some but not all of them?
Thanks Bugs, PTAL again. https://codereview.chromium.org/2669433002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/style/ComputedStyle.h (left): https://codereview.chromium.org/2669433002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/style/ComputedStyle.h:230: m_tableLayout == other.m_tableLayout && On 2017/01/31 at 19:49:47, Bugs Nash wrote: > could we add some comment to the code that indicates that some of these properties are being generated so that developers don't get confused as to why they see some but not all of them? Good idea! I get confused sometimes as well. Done.
The CQ bit was checked by shend@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2669433002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2669433002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:232: m_tableLayout == other.m_tableLayout && looks like you lost some changes in this patch? I believe this line was meant to be removed
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
On 2017/01/31 at 22:13:54, bugsnash wrote: > https://codereview.chromium.org/2669433002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/style/ComputedStyle.h (right): > > https://codereview.chromium.org/2669433002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/style/ComputedStyle.h:232: m_tableLayout == other.m_tableLayout && > looks like you lost some changes in this patch? I believe this line was meant to be removed omg thanks for that. Bad rebase. Well spotted :)
Take two :P PTAL
lgtm with suggestions for extra places to add comments https://codereview.chromium.org/2669433002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2669433002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:299: void setBitDefaults() { maybe add another comment about generated properties here https://codereview.chromium.org/2669433002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyleConstants.h (right): https://codereview.chromium.org/2669433002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyleConstants.h:40: maybe add a comment to this file too about some properties being generated
On 2017/02/01 at 00:13:02, bugsnash wrote: > lgtm with suggestions for extra places to add comments > > https://codereview.chromium.org/2669433002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/style/ComputedStyle.h (right): > > https://codereview.chromium.org/2669433002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/style/ComputedStyle.h:299: void setBitDefaults() { > maybe add another comment about generated properties here > > https://codereview.chromium.org/2669433002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/style/ComputedStyleConstants.h (right): > > https://codereview.chromium.org/2669433002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/style/ComputedStyleConstants.h:40: > maybe add a comment to this file too about some properties being generated Done both :)
The CQ bit was checked by shend@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hey Sasha PTAL
shend@chromium.org changed reviewers: + sashab@chromium.org
Hey Sasha PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Nice extra comments :) LGTM https://codereview.chromium.org/2669433002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyleConstants.h (right): https://codereview.chromium.org/2669433002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyleConstants.h:36: // Some properties are automatically generated in ComputedStyleBase properties -> enums
https://codereview.chromium.org/2669433002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyleConstants.h (right): https://codereview.chromium.org/2669433002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyleConstants.h:36: // Some properties are automatically generated in ComputedStyleBase On 2017/02/01 at 18:16:08, sashab wrote: > properties -> enums done.
The CQ bit was checked by shend@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bugsnash@chromium.org, sashab@chromium.org Link to the patchset: https://codereview.chromium.org/2669433002/#ps80001 (title: "Fix comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by shend@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by shend@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
third_party/WebKit/Source/core/style/ComputedStyleConstants.h:
While running git apply --index -p1;
error: patch failed:
third_party/WebKit/Source/core/style/ComputedStyleConstants.h:167
error: third_party/WebKit/Source/core/style/ComputedStyleConstants.h: patch
does not apply
Patch: third_party/WebKit/Source/core/style/ComputedStyleConstants.h
Index: third_party/WebKit/Source/core/style/ComputedStyleConstants.h
diff --git a/third_party/WebKit/Source/core/style/ComputedStyleConstants.h
b/third_party/WebKit/Source/core/style/ComputedStyleConstants.h
index
8731135f4c642d6dcd6c77499488257a2f2ae16e..de35f30307548a050e725eaead5820330c8f4638
100644
--- a/third_party/WebKit/Source/core/style/ComputedStyleConstants.h
+++ b/third_party/WebKit/Source/core/style/ComputedStyleConstants.h
@@ -33,6 +33,8 @@
namespace blink {
+// Some enums are automatically generated in ComputedStyleBaseConstants
+
// TODO(sashab): Change these enums to enum classes with an unsigned underlying
// type. Enum classes provide better type safety, and forcing an unsigned
// underlying type prevents msvc from interpreting enums as negative numbers.
@@ -167,8 +169,6 @@ enum class EVerticalAlign : unsigned {
Length
};
-enum class ETableLayout : unsigned { kAuto, kFixed };
-
enum TextCombine { TextCombineNone, TextCombineAll };
enum EFillAttachment {
The CQ bit was checked by shend@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sashab@chromium.org, bugsnash@chromium.org Link to the patchset: https://codereview.chromium.org/2669433002/#ps100001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486077815808400,
"parent_rev": "2e13e1dcc2eb31ef2e98302312ab245b31844419", "commit_rev":
"14a1ac0a3d3be32ac70c9cf16b8f9405355e5d79"}
Message was sent while issue was closed.
Description was changed from ========== Moved table-layout property to be generated in ComputedStyleBase. Move the property table-layout and its enum, ETableLayout, to be generated in ComputedStyleBase. Also moved getter / setter / initial value methods to be generated in ComputedStyleBase. BUG=628043 ========== to ========== Moved table-layout property to be generated in ComputedStyleBase. Move the property table-layout and its enum, ETableLayout, to be generated in ComputedStyleBase. Also moved getter / setter / initial value methods to be generated in ComputedStyleBase. BUG=628043 Review-Url: https://codereview.chromium.org/2669433002 Cr-Commit-Position: refs/heads/master@{#447937} Committed: https://chromium.googlesource.com/chromium/src/+/14a1ac0a3d3be32ac70c9cf16b8f... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/14a1ac0a3d3be32ac70c9cf16b8f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
