Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2545093002:
setNeedsPaintPropertyUpdate on FrameView contents size change (Closed)
DescriptionsetNeedsPaintPropertyUpdate on FrameView contents size change
This ensures that the bounds of scroll is updated.
BUG=645667
Committed: https://crrev.com/9e577367e4db4678012a625ebfc97b7403adf250
Cr-Commit-Position: refs/heads/master@{#436077}
Patch Set 1 #Patch Set 2 : guard #Messages
Total messages: 16 (8 generated)
wangxianzhu@chromium.org changed reviewers: + pdr@chromium.org
The CQ bit was checked by wangxianzhu@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.
On 2016/12/02 at 17:13:42, wangxianzhu wrote:
>
Overall looks good. Which test passes with this change?
Can you guard this with spv2? Something like:
if (RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled()) {
// The scroll paint property depends on the content size.
setNeedsPaintPropertyUpdate();
}
Also, I suspect this is only needed when
!RuntimeEnabledFeatures::rootLayerScrollingEnabled().
On 2016/12/02 20:37:36, pdr. wrote:
> On 2016/12/02 at 17:13:42, wangxianzhu wrote:
> >
>
> Overall looks good. Which test passes with this change?
>
About 130 svg/ and paint/invalidation/ tests pass (previously crashed) in SPv2
or SlimmingPaintInvalidation mode with DCHECK enabled.
> Can you guard this with spv2? Something like:
> if (RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled()) {
> // The scroll paint property depends on the content size.
> setNeedsPaintPropertyUpdate();
> }
>
> Also, I suspect this is only needed when
> !RuntimeEnabledFeatures::rootLayerScrollingEnabled().
Done.
On 2016/12/02 at 21:01:03, wangxianzhu wrote:
> On 2016/12/02 20:37:36, pdr. wrote:
> > On 2016/12/02 at 17:13:42, wangxianzhu wrote:
> > >
> >
> > Overall looks good. Which test passes with this change?
> >
>
> About 130 svg/ and paint/invalidation/ tests pass (previously crashed) in SPv2
or SlimmingPaintInvalidation mode with DCHECK enabled.
>
> > Can you guard this with spv2? Something like:
> > if (RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled()) {
> > // The scroll paint property depends on the content size.
> > setNeedsPaintPropertyUpdate();
> > }
> >
> > Also, I suspect this is only needed when
> > !RuntimeEnabledFeatures::rootLayerScrollingEnabled().
>
> Done.
LGTM
Can you check one thing before landing--do the checks in
https://codereview.chromium.org/2529293012 already fix these crashes?
On 2016/12/02 21:26:46, pdr. wrote:
> On 2016/12/02 at 21:01:03, wangxianzhu wrote:
> > On 2016/12/02 20:37:36, pdr. wrote:
> > > On 2016/12/02 at 17:13:42, wangxianzhu wrote:
> > > >
> > >
> > > Overall looks good. Which test passes with this change?
> > >
> >
> > About 130 svg/ and paint/invalidation/ tests pass (previously crashed) in
SPv2
> or SlimmingPaintInvalidation mode with DCHECK enabled.
> >
> > > Can you guard this with spv2? Something like:
> > > if (RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled()) {
> > > // The scroll paint property depends on the content size.
> > > setNeedsPaintPropertyUpdate();
> > > }
> > >
> > > Also, I suspect this is only needed when
> > > !RuntimeEnabledFeatures::rootLayerScrollingEnabled().
> >
> > Done.
>
> LGTM
>
> Can you check one thing before landing--do the checks in
> https://codereview.chromium.org/2529293012 already fix these crashes?
Unfortunately it doesn't fix these crashes.
The crash is about bounds change in scroll properties:
STDERR:
[1:1:1202/133351.176044:255571163263:FATAL:FindPropertiesNeedingUpdate.h(75)]
Check failed: *m_originalScroll == *m_frameView->scroll(). Property was updated
without the FrameView needing a paint property update.
The CQ bit was checked by wangxianzhu@chromium.org
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": 20001, "attempt_start_ts": 1480715021215030,
"parent_rev": "c25a813b13ee2f8bd917a858bab2db67a5be66a1", "commit_rev":
"ab91ad575bb829db0ba1188d7663e6537bcef855"}
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== setNeedsPaintPropertyUpdate on FrameView contents size change This ensures that the bounds of scroll is updated. BUG=645667 ========== to ========== setNeedsPaintPropertyUpdate on FrameView contents size change This ensures that the bounds of scroll is updated. BUG=645667 Committed: https://crrev.com/9e577367e4db4678012a625ebfc97b7403adf250 Cr-Commit-Position: refs/heads/master@{#436077} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9e577367e4db4678012a625ebfc97b7403adf250 Cr-Commit-Position: refs/heads/master@{#436077} |
