Temporarily enable scroll anchoring on trunk.

Temporarily enable scroll anchoring on trunk.

This is to get feedback from canary users.  It will be reverted on 2016-09-16.

BUG=624074
Committed: https://crrev.com/73693c4b2eb516e55b700c0317b38d672a0bdb29
Cr-Commit-Position: refs/heads/master@{#418715}

[skobes, 2016-09-14 00:48:51]

The CQ bit was checked by skobes@chromium.org to run a CQ dry run

[skobes, 2016-09-14 00:49:09]

Description was changed from

==========
Temporarily enable scroll anchoring on trunk.

This is to get feedback from canary users.  It will be reverted by 2016-09-16.

BUG=624074
==========

to

==========
Temporarily enable scroll anchoring on trunk.

This is to get feedback from canary users.  It will be reverted on 2016-09-16.

BUG=624074
==========

[commit-bot: I haz the power, 2016-09-14 00:49:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skobes, 2016-09-14 01:36:08]

skobes@chromium.org changed reviewers:
+ ojan@chromium.org

[ojan, 2016-09-14 02:11:35]

lgtm

[skobes, 2016-09-14 02:29:06]

skobes@chromium.org changed reviewers:
+ rbyers@chromium.org

[skobes, 2016-09-14 02:29:07]

+rbyers for OWNERS

[commit-bot: I haz the power, 2016-09-14 02:36:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 02:36:18]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[skobes, 2016-09-14 03:02:03]

It looks like this breaks AutoScrollExtensionSettingsWebUITest.testAutoScroll. 
Hold off on reviewing until I investigate this.

[Rick Byers, 2016-09-14 15:15:47]

On 2016/09/14 03:02:03, skobes wrote:
> It looks like this breaks AutoScrollExtensionSettingsWebUITest.testAutoScroll.

> Hold off on reviewing until I investigate this.

Typically we'd use Finch for a trial like this (eg. to ensure it's only enabled
on canary, not any dev channel builds that happen to go out).  I suspect you'll
eventually want to ship this intervention under finch anyway (so it can be
easily disabled without pushing a new build), if so it's no more work in the end
to setup the finch trial machinery now.

That said, if you/ojan prefer this hack to example on canary for a couple days
that's OK with me.

[ojan, 2016-09-14 15:28:45]

There's disagreement on best practice here. Although, I prefer the Finch
approach as well in general, some other chrome leads don't (for good
reasons fwiw). In order to not stall this, let's move forward with this
patch as is for this case. I'll followup with Rick offline to resolve a
policy for future decisions.

On Wed, Sep 14, 2016, 8:16 AM <rbyers@chromium.org> wrote:

> On 2016/09/14 03:02:03, skobes wrote:
> > It looks like this breaks
> AutoScrollExtensionSettingsWebUITest.testAutoScroll.
>
> > Hold off on reviewing until I investigate this.
>
> Typically we'd use Finch for a trial like this (eg. to ensure it's only
> enabled
> on canary, not any dev channel builds that happen to go out). I suspect
> you'll
> eventually want to ship this intervention under finch anyway (so it can be
> easily disabled without pushing a new build), if so it's no more work in
> the end
> to setup the finch trial machinery now.
>
> That said, if you/ojan prefer this hack to example on canary for a couple
> days
> that's OK with me.
>
> https://codereview.chromium.org/2335363003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[ojan, 2016-09-14 15:28:47]

There's disagreement on best practice here. Although, I prefer the Finch
approach as well in general, some other chrome leads don't (for good
reasons fwiw). In order to not stall this, let's move forward with this
patch as is for this case. I'll followup with Rick offline to resolve a
policy for future decisions.

On Wed, Sep 14, 2016, 8:16 AM <rbyers@chromium.org> wrote:

> On 2016/09/14 03:02:03, skobes wrote:
> > It looks like this breaks
> AutoScrollExtensionSettingsWebUITest.testAutoScroll.
>
> > Hold off on reviewing until I investigate this.
>
> Typically we'd use Finch for a trial like this (eg. to ensure it's only
> enabled
> on canary, not any dev channel builds that happen to go out). I suspect
> you'll
> eventually want to ship this intervention under finch anyway (so it can be
> easily disabled without pushing a new build), if so it's no more work in
> the end
> to setup the finch trial machinery now.
>
> That said, if you/ojan prefer this hack to example on canary for a couple
> days
> that's OK with me.
>
> https://codereview.chromium.org/2335363003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Rick Byers, 2016-09-14 15:46:13]

On 2016/09/14 15:28:47, ojan wrote:
> There's disagreement on best practice here. Although, I prefer the Finch
> approach as well in general, some other chrome leads don't (for good
> reasons fwiw). In order to not stall this, let's move forward with this
> patch as is for this case. I'll followup with Rick offline to resolve a
> policy for future decisions.

Ok, LGTM

[skobes, 2016-09-14 19:43:09]

The CQ bit was checked by skobes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 19:43:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ymalik, 2016-09-14 19:54:17]

ymalik@chromium.org changed reviewers:
+ ymalik@chromium.org

[ymalik, 2016-09-14 19:54:18]

https://codereview.chromium.org/2335363003/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js
(right):

https://codereview.chromium.org/2335363003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:330:
document.body.scrollLeft = 0;
Now that we already know of the bug, should this be done in a different CL?

[skobes, 2016-09-14 19:56:59]

https://codereview.chromium.org/2335363003/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js
(right):

https://codereview.chromium.org/2335363003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:330:
document.body.scrollLeft = 0;
On 2016/09/14 19:54:18, ymalik wrote:
> Now that we already know of the bug, should this be done in a different CL?

Yes it should... see http://crrev.com/2343683002

[ymalik, 2016-09-14 20:01:44]

https://codereview.chromium.org/2335363003/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/extension_settings_browsertest.js
(right):

https://codereview.chromium.org/2335363003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_settings_browsertest.js:330:
document.body.scrollLeft = 0;
On 2016/09/14 19:56:59, skobes wrote:
> On 2016/09/14 19:54:18, ymalik wrote:
> > Now that we already know of the bug, should this be done in a different CL?
> 
> Yes it should... see http://crrev.com/2343683002

Ah I see. I was confused because you have it here as well. But presumably its
because you are testing here?

[skobes, 2016-09-14 21:11:40]

The CQ bit was checked by skobes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 21:12:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skobes, 2016-09-14 21:12:44]

This is ready to go after http://crrev.com/2343683002 lands.

[commit-bot: I haz the power, 2016-09-14 23:09:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 23:09:39]

Dry run: This issue passed the CQ dry run.

[skobes, 2016-09-14 23:14:43]

The CQ bit was checked by skobes@chromium.org

[skobes, 2016-09-14 23:14:44]

The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org,
ojan@chromium.org
Link to the patchset: https://codereview.chromium.org/2335363003/#ps40001
(title: "depend on http://crrev.com/2343683002")

[commit-bot: I haz the power, 2016-09-14 23:15:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 23:33:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 23:33:14]

Failed to apply the patch.
On branch working_branch
Your branch is up-to-date with 'origin/refs/pending/heads/master'.
nothing to commit, working tree clean

[commit-bot: I haz the power, 2016-09-14 23:33:53]

Description was changed from

==========
Temporarily enable scroll anchoring on trunk.

This is to get feedback from canary users.  It will be reverted on 2016-09-16.

BUG=624074
==========

to

==========
Temporarily enable scroll anchoring on trunk.

This is to get feedback from canary users.  It will be reverted on 2016-09-16.

BUG=624074

Committed: https://crrev.com/73693c4b2eb516e55b700c0317b38d672a0bdb29
Cr-Commit-Position: refs/heads/master@{#418715}
==========

[commit-bot: I haz the power, 2016-09-14 23:33:54]

Patchset 3 (id:??) landed as
https://crrev.com/73693c4b2eb516e55b700c0317b38d672a0bdb29
Cr-Commit-Position: refs/heads/master@{#418715}