Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1376143002:
Add a document phase for calculating paint properties (Closed)
|
Created:
5 years, 2 months ago by pdr. Modified:
5 years, 2 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-paint_chromium.org, dglazkov+blink, dshwang, eae+blinkwatch, rwlbuis, sof, slimming-paint-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a document phase for calculating paint properties
This patch adds the document phase for calculating paint properties
but does not actually implement the layout tree walk. For more
information about paint properties, see:
https://docs.google.com/document/d/12I3JD1-Jmnb59ZHKyntFTSsNUzQhPae8QpCECtZt5zs
BUG=537409
Committed: https://crrev.com/dc857b6e854596aec11cf73b63834e06352e6507
Cr-Commit-Position: refs/heads/master@{#351715}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebase (remove GLB check, as was done in synchronizedPaint) #
Total comments: 4
Messages
Total messages: 28 (9 generated)
pdr@chromium.org changed reviewers: + jbroman@chromium.org, trchen@chromium.org
pdr@chromium.org changed reviewers: + chrishtr@chromium.org
lgtm, but I'm not familiar enough with DocumentLifecycle to me a good reviewer for this CL
On 2015/09/30 at 19:58:15, jbroman wrote: > lgtm, but I'm not familiar enough with DocumentLifecycle to me a good reviewer for this CL @chrishtr, are you me a good reviewer?
https://chromiumcodereview.appspot.com/1376143002/diff/1/third_party/WebKit/S... File third_party/WebKit/Source/core/dom/DocumentLifecycle.cpp (right): https://chromiumcodereview.appspot.com/1376143002/diff/1/third_party/WebKit/S... third_party/WebKit/Source/core/dom/DocumentLifecycle.cpp:202: if (nextState == InPaint && RuntimeEnabledFeatures::slimmingPaintSynchronizedPaintingEnabled()) Why this part?
https://chromiumcodereview.appspot.com/1376143002/diff/1/third_party/WebKit/S... File third_party/WebKit/Source/core/dom/DocumentLifecycle.cpp (right): https://chromiumcodereview.appspot.com/1376143002/diff/1/third_party/WebKit/S... third_party/WebKit/Source/core/dom/DocumentLifecycle.cpp:202: if (nextState == InPaint && RuntimeEnabledFeatures::slimmingPaintSynchronizedPaintingEnabled()) On 2015/09/30 at 21:36:33, chrishtr wrote: > Why this part? slimmingPaintSynchronizedPaintingEnabled will soon work for spv1 and we want to skip over the paint property step for spv1, but disallow skipping it for spv2. SPV1 with synchronized painting: InPaintInvalidation->PaintInvalidationClean->InPaint->PaintClean SPV2 (synchronized painting implied): InPaintInvalidation->PaintInvalidationClean->InCalcPaintProps->CalcPaintPropsClean->InPaint->PaintClean
lgtm
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1376143002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1376143002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by pdr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org, jbroman@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1376143002/#ps20001 (title: "Rebase (remove GLB check, as was done in synchronizedPaint)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1376143002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1376143002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1376143002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1376143002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/dc857b6e854596aec11cf73b63834e06352e6507 Cr-Commit-Position: refs/heads/master@{#351715}
Message was sent while issue was closed.
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1376143002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DocumentLifecycle.h (right): https://codereview.chromium.org/1376143002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DocumentLifecycle.h:72: CalcPaintPropsClean, We don't generally abbreviate in blink, this should be CalculatePaintProperties (just like the method name) https://codereview.chromium.org/1376143002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1376143002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2466: // TODO(pdr): Calculate the paint properties by walking the layout tree. That design doc is very light on details, how will you track dirty bits, what invalidates this phase? What avoids walking the entire layout tree?
Message was sent while issue was closed.
https://codereview.chromium.org/1376143002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/DocumentLifecycle.h (right): https://codereview.chromium.org/1376143002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/DocumentLifecycle.h:72: CalcPaintPropsClean, On 2015/10/01 at 09:00:08, esprehn wrote: > We don't generally abbreviate in blink, this should be CalculatePaintProperties (just like the method name) I guess we use Calc and Recalc in other parts of the code, but Props is very weird in blink. https://codereview.chromium.org/1376143002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1376143002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2461: void FrameView::calculatePaintProperties() recalcPaintProperties would match the verbage we use in other parts of the engine (or updatePaintProperties).
Message was sent while issue was closed.
On 2015/10/01 at 09:00:09, esprehn wrote: > https://codereview.chromium.org/1376143002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/FrameView.cpp:2466: // TODO(pdr): Calculate the paint properties by walking the layout tree. > That design doc is very light on details, how will you track dirty bits, what invalidates this phase? What avoids walking the entire layout tree? For staging, this is literally a entire tree walk while we're implementing the details. The expectation is that this will be folded into the existing layout phase. This seems like the least-risky part of the proposal and will let us shake out the insane complexity of the web painting algorithm without adding complexity to the existing layout code. On 2015/10/01 at 09:05:07, esprehn wrote: > https://codereview.chromium.org/1376143002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/FrameView.cpp:2461: void FrameView::calculatePaintProperties() > recalcPaintProperties would match the verbage we use in other parts of the engine > > (or updatePaintProperties). Sure, I can change this to updatePaintProperties.
Message was sent while issue was closed.
On 2015/10/01 at 17:55:39, pdr wrote: > On 2015/10/01 at 09:00:09, esprehn wrote: > > https://codereview.chromium.org/1376143002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/frame/FrameView.cpp:2466: // TODO(pdr): Calculate the paint properties by walking the layout tree. > > That design doc is very light on details, how will you track dirty bits, what invalidates this phase? What avoids walking the entire layout tree? > > For staging, this is literally a entire tree walk while we're implementing the details. The expectation is that this will be folded into the existing layout phase. This seems like the least-risky part of the proposal and will let us shake out the insane complexity of the web painting algorithm without adding complexity to the existing layout code. > Has that been discussed with the layout team? Note that we also skip layout in lots of cases, for example setting a transform or opacity doesn't do a layout.
Message was sent while issue was closed.
On 2015/10/01 at 17:59:44, esprehn wrote: > On 2015/10/01 at 17:55:39, pdr wrote: > > On 2015/10/01 at 09:00:09, esprehn wrote: > > > https://codereview.chromium.org/1376143002/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/frame/FrameView.cpp:2466: // TODO(pdr): Calculate the paint properties by walking the layout tree. > > > That design doc is very light on details, how will you track dirty bits, what invalidates this phase? What avoids walking the entire layout tree? > > > > For staging, this is literally a entire tree walk while we're implementing the details. The expectation is that this will be folded into the existing layout phase. This seems like the least-risky part of the proposal and will let us shake out the insane complexity of the web painting algorithm without adding complexity to the existing layout code. > > > > Has that been discussed with the layout team? Note that we also skip layout in lots of cases, for example setting a transform or opacity doesn't do a layout. Only in passing, but you're right that we should make sure it fits in with their plans and is technically sound. I'll run this by Levi in more detail tomorrow. Yeah, we're aware of property changes that don't cause a layout. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
