Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2934663002:
Disabling if-heavy lofi integration test for M61+ (Closed)
DescriptionDisabling if-heavy lofi integration test for M61+
This behavior is no longer supported.
BUG=731922
Review-Url: https://codereview.chromium.org/2934663002
Cr-Commit-Position: refs/heads/master@{#478804}
Committed: https://chromium.googlesource.com/chromium/src/+/f46becbfa68ab91275ca64480fcacbc5b98d771c
Patch Set 1 #
Messages
Total messages: 30 (17 generated)
The CQ bit was checked by ryansturm@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...
ryansturm@chromium.org changed reviewers: + bustamante@chromium.org, dougarnett@chromium.org, robertogden@chromium.org, tbansal@chromium.org
I believe this test is failing because we no longer support the behavior. Dougarnett: Does this seem correct? My understanding is that this behavior is deprecated. robertogden: PTAL bustamante: Feel free to add more context, also PTAL because I need an OWNER.
Description was changed from ========== Disabling if-heavy lofi integration test for M61+ This behavior is no longer supported. BUG= ========== to ========== Disabling if-heavy lofi integration test for M61+ This behavior is no longer supported. BUG= ==========
ryansturm@chromium.org changed reviewers: - tbansal@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/09 22:13:46, RyanSturm wrote: > I believe this test is failing because we no longer support the behavior. > > Dougarnett: Does this seem correct? My understanding is that this behavior is > deprecated. > > robertogden: PTAL > > bustamante: Feel free to add more context, also PTAL because I need an OWNER. I think this breaks as a result of the issue we had with always getting transforms for the cellular-only flag. Here, this test is using the slow-connections-only flag. The server We may want to see if we can make this test behavior work with field trials instead of flags.
On 2017/06/09 22:27:42, dougarnett wrote: > On 2017/06/09 22:13:46, RyanSturm wrote: > > I believe this test is failing because we no longer support the behavior. > > > > Dougarnett: Does this seem correct? My understanding is that this behavior is > > deprecated. > > > > robertogden: PTAL > > > > bustamante: Feel free to add more context, also PTAL because I need an OWNER. > > I think this breaks as a result of the issue we had with always getting > transforms > for the cellular-only flag. Here, this test is using the slow-connections-only > flag. > The server > > We may want to see if we can make this test behavior work with field trials > instead > of flags. I think we can't hit this -- which is fine IMO. Specifically, we early return if the state is PREVIEWS_OFF or PREVIEWS_NO_TRANSFORM and only add it if its not lite page or server lofi. I don't think we can get into this method if we are client side lofi'ing it. Perhaps, if the type of the main frame is CLIENT_LOFI we could add if-heavy if we also are using DRP for the request, but that is for main frames not images.
Description was changed from ========== Disabling if-heavy lofi integration test for M61+ This behavior is no longer supported. BUG= ========== to ========== Disabling if-heavy lofi integration test for M61+ This behavior is no longer supported. BUG=731922 ==========
On 2017/06/09 22:32:43, RyanSturm wrote: > On 2017/06/09 22:27:42, dougarnett wrote: > > On 2017/06/09 22:13:46, RyanSturm wrote: > > > I believe this test is failing because we no longer support the behavior. > > > > > > Dougarnett: Does this seem correct? My understanding is that this behavior > is > > > deprecated. > > > > > > robertogden: PTAL > > > > > > bustamante: Feel free to add more context, also PTAL because I need an > OWNER. > > > > I think this breaks as a result of the issue we had with always getting > > transforms > > for the cellular-only flag. Here, this test is using the slow-connections-only > > flag. > > The server > > > > We may want to see if we can make this test behavior work with field trials > > instead > > of flags. > > > I think we can't hit this -- which is fine IMO. > > Specifically, we early return if the state is PREVIEWS_OFF or > PREVIEWS_NO_TRANSFORM and only add it if its not lite page or server lofi. I > don't think we can get into this method if we are client side lofi'ing it. > > Perhaps, if the type of the main frame is CLIENT_LOFI we could add if-heavy if > we also are using DRP for the request, but that is for main frames not images. I think this CL is good - we don't intend for if-heavy to be used in M-61 and beyond.
On 2017/06/09 22:41:27, dougarnett wrote: > On 2017/06/09 22:32:43, RyanSturm wrote: > > On 2017/06/09 22:27:42, dougarnett wrote: > > > On 2017/06/09 22:13:46, RyanSturm wrote: > > > > I believe this test is failing because we no longer support the behavior. > > > > > > > > Dougarnett: Does this seem correct? My understanding is that this behavior > > is > > > > deprecated. > > > > > > > > robertogden: PTAL > > > > > > > > bustamante: Feel free to add more context, also PTAL because I need an > > OWNER. > > > > > > I think this breaks as a result of the issue we had with always getting > > > transforms > > > for the cellular-only flag. Here, this test is using the > slow-connections-only > > > flag. > > > The server > > > > > > We may want to see if we can make this test behavior work with field trials > > > instead > > > of flags. > > > > > > I think we can't hit this -- which is fine IMO. > > > > Specifically, we early return if the state is PREVIEWS_OFF or > > PREVIEWS_NO_TRANSFORM and only add it if its not lite page or server lofi. I > > don't think we can get into this method if we are client side lofi'ing it. > > > > Perhaps, if the type of the main frame is CLIENT_LOFI we could add if-heavy if > > we also are using DRP for the request, but that is for main frames not images. > > I think this CL is good - we don't intend for if-heavy to be used in M-61 and > beyond. lgtm
lgtm
The CQ bit was checked by ryansturm@chromium.org
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
ryansturm@chromium.org changed reviewers: + tbansal@chromium.org
Thanks: need a committee review
Tbansal: ptal
The CQ bit was checked by tbansal@chromium.org
lgtm lgtm
The CQ bit was checked by ryansturm@chromium.org
The CQ bit was unchecked by ryansturm@chromium.org
The CQ bit was checked by ryansturm@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": 1, "attempt_start_ts": 1497307462118010, "parent_rev":
"6a2c67b927972a2b38446154bfe9784313701d8c", "commit_rev":
"f46becbfa68ab91275ca64480fcacbc5b98d771c"}
Message was sent while issue was closed.
Description was changed from ========== Disabling if-heavy lofi integration test for M61+ This behavior is no longer supported. BUG=731922 ========== to ========== Disabling if-heavy lofi integration test for M61+ This behavior is no longer supported. BUG=731922 Review-Url: https://codereview.chromium.org/2934663002 Cr-Commit-Position: refs/heads/master@{#478804} Committed: https://chromium.googlesource.com/chromium/src/+/f46becbfa68ab91275ca64480fca... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/f46becbfa68ab91275ca64480fca... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
