New issue
Advanced search Search tips

Issue 897772 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 20
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 836902



Sign in to add a comment

BlinkGenPropertyTrees is not enabled in layout tests even though the feature is marked as experimental

Project Member Reported by pdr@chromium.org, Oct 22

Issue description

There's a bug in SetRuntimeFeaturesDefaultsAndUpdateFromArgs that resets the BlinkGenPropertyTreesEnabled runtime enabled feature.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 22

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/128c4cabb40b976e13545cdf97a96b1cc4900463

commit 128c4cabb40b976e13545cdf97a96b1cc4900463
Author: Philip Rogers <pdr@chromium.org>
Date: Mon Oct 22 17:11:52 2018

Revert "[BlinkGenPropertyTrees] Promote BGPT to experimental"

This reverts commit 3d1759c957a0b1420e4638fe907b82611b880b9c.

Reason for revert: Due to  crbug.com/897772 , tests were not run.

Original change's description:
> [BlinkGenPropertyTrees] Promote BGPT to experimental
>
> BlinkGenPropertyTrees (BGPT) is an incremental step towards making
> compositing decisions after the paint lifecycle phase. The primary
> changes are to build property trees in blink and send a layer list to
> the compositor, rather than building property trees in cc from a layer
> tree. This patch marks the project as experimental which will give us
> test and perf coverage on the bots.
>
> Sheriffs: This shifts time (e.g., we now run PaintArtifactCompositor
> and no longer run the cc property tree builder) and will likely change
> performance benchmarks. Because this is an initial trial, feel free to
> roll this patch out.
>
> Bug: 836884
> Change-Id: Ia1073bdd0922503925c17161f052bdfcd94196b7
> Reviewed-on: https://chromium-review.googlesource.com/c/1292555
> Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
> Commit-Queue: Philip Rogers <pdr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#601429}

TBR=pdr@chromium.org,chrishtr@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 836884,  897772 
Change-Id: Iee4fb4fc15e2400c85c9855f904bc4f4225eed71
Reviewed-on: https://chromium-review.googlesource.com/c/1293992
Commit-Queue: Philip Rogers <pdr@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601624}
[modify] https://crrev.com/128c4cabb40b976e13545cdf97a96b1cc4900463/third_party/blink/renderer/platform/runtime_enabled_features.json5

Project Member

Comment 2 by bugdroid1@chromium.org, Oct 23

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bbc3359105666ae9aa17a403ab0b64b423fef037

commit bbc3359105666ae9aa17a403ab0b64b423fef037
Author: Philip Rogers <pdr@chromium.org>
Date: Tue Oct 23 13:55:17 2018

[BlinkGenPropertyTrees] Do not disable feature without command line flag

When we promoted BlinkGenPropertyTrees to experimental in
https://crrev.com/601429, the layout tests did not pick up the change
because SetRuntimeFeaturesDefaultsAndUpdateFromArgs would reset the
flag to disabled if --enable-blink-gen-property-trees was not passed on
the command line. This patch updates the code so that the command line
enabled flag only turns the feature on instead of directly controlling
the feature.

This patch also updates the SPV2 flag which has the same bug. I audited
other cases in runtime_features.cc and do not see other instances of
this bug.

Bug:  897772 
Change-Id: I8902e88afd0c811096c4336cd6fcdc40218e3ec7
Reviewed-on: https://chromium-review.googlesource.com/c/1294200
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601931}
[modify] https://crrev.com/bbc3359105666ae9aa17a403ab0b64b423fef037/content/child/runtime_features.cc

Status: Fixed (was: Started)

Sign in to add a comment