|
|
Created:
3 years, 8 months ago by pdr. Modified:
3 years, 8 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd LayoutObject::RarePaintData for rare paint data
This patch adds a rare data struct to LayoutObject for paint-related
data. In this patch we just use it for ObjectPaintProperties, but this
sets the groundwork for splitting LocalBorderBoxProperties out of
ObjectPaintProperties so it can be managed independently. Data in
crbug.com/700452 [1] shows that LayoutBorderBoxProperties is needed
much more than ObjectPaintProperties so this will save memory.
In addition, a future patch will use store PaintLayer in this rare
data, reducing one pointer from LayoutBoxModelObject.
[1] https://docs.google.com/spreadsheets/d/1IC1JI0sX7QsR9FmA4G1-F9E_c3xRKDNoV3SkDKQ822U
BUG=700452
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2785603002
Cr-Commit-Position: refs/heads/master@{#460885}
Committed: https://chromium.googlesource.com/chromium/src/+/7f0d97dcab059b98a6825ee172c7140ff938f667
Patch Set 1 #Patch Set 2 : PaintRareData->RarePaintData, plus cleanup #Patch Set 3 : Rebase #Patch Set 4 : CORE_EXPORT private struct because compilers #Patch Set 5 : Compilers, how do they work? #Patch Set 6 : Add back missing CORE_EXPORT #Patch Set 7 : Use default unique_ptr ctor #
Total comments: 4
Patch Set 8 : struct->class #
Messages
Total messages: 32 (24 generated)
Description was changed from ========== i# Enter a description of the change. Add LayoutObject::PaintRareData for rare paint data This patch adds a rare data struct to LayoutObject for paint-related data. In this patch we just use it for ObjectPaintProperties, but this sets the groundwork for splitting LayoutBorderBoxProperties out of ObjectPaintProperties so it can be managed independently. Data in crbug.com/700452 shows that LayoutBorderBoxProperties is needed much more than ObjectPaintProperties so this will save memory. In addition, a future patch will use store PaintLayer in this rare data instead of having it owned by LayoutBoxModelObject. BUG=700452 ========== to ========== Add LayoutObject::RarePaintData for rare paint data This patch adds a rare data struct to LayoutObject for paint-related data. In this patch we just use it for ObjectPaintProperties, but this sets the groundwork for splitting LayoutBorderBoxProperties out of ObjectPaintProperties so it can be managed independently. Data in crbug.com/700452 shows that LayoutBorderBoxProperties is needed much more than ObjectPaintProperties so this will save memory. In addition, a future patch will use store PaintLayer in this rare data, reducing one pointer from LayoutBoxModelObject. BUG=700452 ==========
The CQ bit was checked by pdr@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 checked by pdr@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Description was changed from ========== Add LayoutObject::RarePaintData for rare paint data This patch adds a rare data struct to LayoutObject for paint-related data. In this patch we just use it for ObjectPaintProperties, but this sets the groundwork for splitting LayoutBorderBoxProperties out of ObjectPaintProperties so it can be managed independently. Data in crbug.com/700452 shows that LayoutBorderBoxProperties is needed much more than ObjectPaintProperties so this will save memory. In addition, a future patch will use store PaintLayer in this rare data, reducing one pointer from LayoutBoxModelObject. BUG=700452 ========== to ========== Add LayoutObject::RarePaintData for rare paint data This patch adds a rare data struct to LayoutObject for paint-related data. In this patch we just use it for ObjectPaintProperties, but this sets the groundwork for splitting LocalBorderBoxProperties out of ObjectPaintProperties so it can be managed independently. Data in crbug.com/700452 shows that LayoutBorderBoxProperties is needed much more than ObjectPaintProperties so this will save memory. In addition, a future patch will use store PaintLayer in this rare data, reducing one pointer from LayoutBoxModelObject. BUG=700452 ==========
Description was changed from ========== Add LayoutObject::RarePaintData for rare paint data This patch adds a rare data struct to LayoutObject for paint-related data. In this patch we just use it for ObjectPaintProperties, but this sets the groundwork for splitting LocalBorderBoxProperties out of ObjectPaintProperties so it can be managed independently. Data in crbug.com/700452 shows that LayoutBorderBoxProperties is needed much more than ObjectPaintProperties so this will save memory. In addition, a future patch will use store PaintLayer in this rare data, reducing one pointer from LayoutBoxModelObject. BUG=700452 ========== to ========== Add LayoutObject::RarePaintData for rare paint data This patch adds a rare data struct to LayoutObject for paint-related data. In this patch we just use it for ObjectPaintProperties, but this sets the groundwork for splitting LocalBorderBoxProperties out of ObjectPaintProperties so it can be managed independently. Data in crbug.com/700452 shows that LayoutBorderBoxProperties is needed much more than ObjectPaintProperties so this will save memory. In addition, a future patch will use store PaintLayer in this rare data, reducing one pointer from LayoutBoxModelObject. BUG=700452 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
pdr@chromium.org changed reviewers: + chrishtr@chromium.org, wangxianzhu@chromium.org
Description was changed from ========== Add LayoutObject::RarePaintData for rare paint data This patch adds a rare data struct to LayoutObject for paint-related data. In this patch we just use it for ObjectPaintProperties, but this sets the groundwork for splitting LocalBorderBoxProperties out of ObjectPaintProperties so it can be managed independently. Data in crbug.com/700452 shows that LayoutBorderBoxProperties is needed much more than ObjectPaintProperties so this will save memory. In addition, a future patch will use store PaintLayer in this rare data, reducing one pointer from LayoutBoxModelObject. BUG=700452 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Add LayoutObject::RarePaintData for rare paint data This patch adds a rare data struct to LayoutObject for paint-related data. In this patch we just use it for ObjectPaintProperties, but this sets the groundwork for splitting LocalBorderBoxProperties out of ObjectPaintProperties so it can be managed independently. Data in crbug.com/700452 [1] shows that LayoutBorderBoxProperties is needed much more than ObjectPaintProperties so this will save memory. In addition, a future patch will use store PaintLayer in this rare data, reducing one pointer from LayoutBoxModelObject. [1] https://docs.google.com/spreadsheets/d/1IC1JI0sX7QsR9FmA4G1-F9E_c3xRKDNoV3SkD... BUG=700452 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by pdr@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: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by pdr@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 checked by pdr@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...
lgtm https://codereview.chromium.org/2785603002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2785603002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:3555: LayoutObject::RarePaintData::RarePaintData() {} Nit: I think we can just use the default constructor and constructor which are inlined by default. https://codereview.chromium.org/2785603002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2785603002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.h:408: struct CORE_EXPORT RarePaintData { Nit: This data structure seems like a class more than a struct.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2785603002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2785603002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:3555: LayoutObject::RarePaintData::RarePaintData() {} On 2017/03/30 at 03:11:18, Xianzhu wrote: > Nit: I think we can just use the default constructor and constructor which are inlined by default. This does have to be here. Windows puts the default ctor in the header and we don't have full type info for ObjectPaintProperties in the header. The failure from commenting out this line is: FAILED: blink_core.dll blink_core.dll.lib blink_core.dll.pdb C:/python_27_amd64/files/python.exe ../../build/toolchain/win/tool_wrapper.py link-wrapper environment.x86 False link.exe /nologo /IMPLIB:./blink_core.dll.lib /DLL /OUT:./blin k_core.dll /PDB:./blink_core.dll.pdb @./blink_core.dll.rsp LayoutObject.obj : error LNK2019: unresolved external symbol "public: __thiscall blink::LayoutObject::RarePaintData::RarePaintData(void)" (??0RarePaintData@LayoutObject@blink@ @QAE@XZ) referenced in function "class std::unique_ptr<class blink::LayoutObject::RarePaintData,struct std::default_delete<class blink::LayoutObject::RarePaintData> > __cdecl base::MakeUnique<class blink::LayoutObject::RarePaintData>(void)" (??$MakeUnique@VRarePaintData@LayoutObject@blink@@$$V@base@@YA?AV?$unique_ptr@VRarePaintData@LayoutObject@bli nk@@U?$default_delete@VRarePaintData@LayoutObject@blink@@@std@@@std@@XZ) ./blink_core.dll : fatal error LNK1120: 1 unresolved externals ninja: build stopped: subcommand failed. https://codereview.chromium.org/2785603002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2785603002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.h:408: struct CORE_EXPORT RarePaintData { On 2017/03/30 at 03:11:18, Xianzhu wrote: > Nit: This data structure seems like a class more than a struct. Sure, done.
PTAL, if you like this please send to the CQ
The CQ bit was checked by wangxianzhu@chromium.org
lgtm
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": 140001, "attempt_start_ts": 1490900418529180, "parent_rev": "4588bf925fa2b0aae2462cda08c7066886b09dcc", "commit_rev": "7f0d97dcab059b98a6825ee172c7140ff938f667"}
Message was sent while issue was closed.
Description was changed from ========== Add LayoutObject::RarePaintData for rare paint data This patch adds a rare data struct to LayoutObject for paint-related data. In this patch we just use it for ObjectPaintProperties, but this sets the groundwork for splitting LocalBorderBoxProperties out of ObjectPaintProperties so it can be managed independently. Data in crbug.com/700452 [1] shows that LayoutBorderBoxProperties is needed much more than ObjectPaintProperties so this will save memory. In addition, a future patch will use store PaintLayer in this rare data, reducing one pointer from LayoutBoxModelObject. [1] https://docs.google.com/spreadsheets/d/1IC1JI0sX7QsR9FmA4G1-F9E_c3xRKDNoV3SkD... BUG=700452 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Add LayoutObject::RarePaintData for rare paint data This patch adds a rare data struct to LayoutObject for paint-related data. In this patch we just use it for ObjectPaintProperties, but this sets the groundwork for splitting LocalBorderBoxProperties out of ObjectPaintProperties so it can be managed independently. Data in crbug.com/700452 [1] shows that LayoutBorderBoxProperties is needed much more than ObjectPaintProperties so this will save memory. In addition, a future patch will use store PaintLayer in this rare data, reducing one pointer from LayoutBoxModelObject. [1] https://docs.google.com/spreadsheets/d/1IC1JI0sX7QsR9FmA4G1-F9E_c3xRKDNoV3SkD... BUG=700452 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2785603002 Cr-Commit-Position: refs/heads/master@{#460885} Committed: https://chromium.googlesource.com/chromium/src/+/7f0d97dcab059b98a6825ee172c7... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/7f0d97dcab059b98a6825ee172c7... |