Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2881003002:
Cleanup StyleRareInheritedData. (Closed)
|
Created:
3 years, 7 months ago by shend Modified:
3 years, 7 months ago Reviewers:
alancutter (OOO until 2018) CC:
blink-reviews, blink-reviews-style_chromium.org, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCleanup StyleRareInheritedData.
This patch changes StyleRareInheritedData to make it easier to generate:
- Remove unused destructor
- Replace explicit copy constructor with RefCountedCopyable
- Replace forward declarations with #includes.
This is in preparation for generating StyleRareInheritedData. While this
patch may increase compile times, we would have to pay this cost anyway
when we generate StyleRareInheritedData in ComputedStyleBase.
BUG=628043
Review-Url: https://codereview.chromium.org/2881003002
Cr-Commit-Position: refs/heads/master@{#472234}
Committed: https://chromium.googlesource.com/chromium/src/+/885a5dea20c9790ed62081a7909b8f675da54eac
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 3
Patch Set 3 : Rebase #
Messages
Total messages: 31 (24 generated)
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...
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...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_tsan_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 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
shend@chromium.org changed reviewers: + alancutter@chromium.org
Hi Alan, PTAL :)
https://codereview.chromium.org/2881003002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/StyleRareInheritedData.cpp (right): https://codereview.chromium.org/2881003002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/StyleRareInheritedData.cpp:33: #include "core/style/StyleInheritedVariables.h" Some of these are now redundant. https://codereview.chromium.org/2881003002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/StyleRareInheritedData.h (right): https://codereview.chromium.org/2881003002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/StyleRareInheritedData.h:55: class StyleInheritedVariables; These need removing.
https://codereview.chromium.org/2881003002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/StyleRareInheritedData.h (right): https://codereview.chromium.org/2881003002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/StyleRareInheritedData.h:173: StyleRareInheritedData(const StyleRareInheritedData&) = default; Can = default be used in CPP files? Perhaps it's better in the interest of incremental build times to keep the includes to CPP files where possible.
On 2017/05/16 at 06:24:36, alancutter wrote: > https://codereview.chromium.org/2881003002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/style/StyleRareInheritedData.h (right): > > https://codereview.chromium.org/2881003002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/style/StyleRareInheritedData.h:173: StyleRareInheritedData(const StyleRareInheritedData&) = default; > Can = default be used in CPP files? Perhaps it's better in the interest of incremental build times to keep the includes to CPP files where possible. Sorry, this wasn't clear in the CL description, but when we generate this class, it will all be in the header file anyway. And the generator doesn't support forward declarations yet, although it probably can with some changes. So we have to bear these compilation costs when we generate the class anyway.
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...
On 2017/05/16 at 06:30:42, shend wrote: > On 2017/05/16 at 06:24:36, alancutter wrote: > > https://codereview.chromium.org/2881003002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/style/StyleRareInheritedData.h (right): > > > > https://codereview.chromium.org/2881003002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/style/StyleRareInheritedData.h:173: StyleRareInheritedData(const StyleRareInheritedData&) = default; > > Can = default be used in CPP files? Perhaps it's better in the interest of incremental build times to keep the includes to CPP files where possible. > > Sorry, this wasn't clear in the CL description, but when we generate this class, it will all be in the header file anyway. And the generator doesn't support forward declarations yet, although it probably can with some changes. So we have to bear these compilation costs when we generate the class anyway. In that case don't worry about it, but still mention that aspect in the description. lgtm after code comments addressed.
Description was changed from ========== Cleanup StyleRareInheritedData. This patch changes StyleRareInheritedData to make it easier to generate: - Remove unused destructor - Replace explicit copy constructor with RefCountedCopyable - Replace forward declarations with #includes. BUG=628043 ========== to ========== Cleanup StyleRareInheritedData. This patch changes StyleRareInheritedData to make it easier to generate: - Remove unused destructor - Replace explicit copy constructor with RefCountedCopyable - Replace forward declarations with #includes. This is in preparation for generating StyleRareInheritedData. While this patch may increase compile times, we would have to pay this cost anyway when we generate StyleRareInheritedData in ComputedStyleBase. BUG=628043 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by shend@chromium.org to run a CQ dry run
The CQ bit was unchecked by shend@chromium.org
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by shend@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alancutter@chromium.org Link to the patchset: https://codereview.chromium.org/2881003002/#ps40001 (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": 40001, "attempt_start_ts": 1494973023761910,
"parent_rev": "cb86cbdb812080c795bef2b2b21089d30c1797fc", "commit_rev":
"885a5dea20c9790ed62081a7909b8f675da54eac"}
Message was sent while issue was closed.
Description was changed from ========== Cleanup StyleRareInheritedData. This patch changes StyleRareInheritedData to make it easier to generate: - Remove unused destructor - Replace explicit copy constructor with RefCountedCopyable - Replace forward declarations with #includes. This is in preparation for generating StyleRareInheritedData. While this patch may increase compile times, we would have to pay this cost anyway when we generate StyleRareInheritedData in ComputedStyleBase. BUG=628043 ========== to ========== Cleanup StyleRareInheritedData. This patch changes StyleRareInheritedData to make it easier to generate: - Remove unused destructor - Replace explicit copy constructor with RefCountedCopyable - Replace forward declarations with #includes. This is in preparation for generating StyleRareInheritedData. While this patch may increase compile times, we would have to pay this cost anyway when we generate StyleRareInheritedData in ComputedStyleBase. BUG=628043 Review-Url: https://codereview.chromium.org/2881003002 Cr-Commit-Position: refs/heads/master@{#472234} Committed: https://chromium.googlesource.com/chromium/src/+/885a5dea20c9790ed62081a7909b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/885a5dea20c9790ed62081a7909b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
