MD Settings: Close edit startup URL dialog, if underlying URL list changes.

MD Settings: Close edit startup URL dialog, if underlying URL list changes.

BUG=661867
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/05f574e9836e8345bbb59f9b828bf2d773331f82
Cr-Commit-Position: refs/heads/master@{#430455}

[dpapad, 2016-11-04 17:36:27]

Description was changed from

==========
MD Settings: Close add/edit startup URL dialog, if underlying URL list changes.

BUG=661867
==========

to

==========
MD Settings: Close add/edit startup URL dialog, if underlying URL list changes.

BUG=661867
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-11-04 17:36:31]

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

[commit-bot: I haz the power, 2016-11-04 17:37:00]

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

[dpapad, 2016-11-04 17:37:09]

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

[dpapad, 2016-11-04 17:37:19]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-11-04 17:37:28]

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

[dpapad, 2016-11-04 17:42:53]

Patchset #1 (id:20001) has been deleted

[dpapad, 2016-11-04 17:43:09]

Description was changed from

==========
MD Settings: Close add/edit startup URL dialog, if underlying URL list changes.

BUG=661867
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Close edit startup URL dialog, if underlying URL list changes.

BUG=661867
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-11-04 17:43:40]

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

[commit-bot: I haz the power, 2016-11-04 17:44:15]

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

[dpapad, 2016-11-04 19:25:35]

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

[dpapad, 2016-11-04 19:26:07]

dpapad@chromium.org changed reviewers:
+ dschuyler@chromium.org

[dpapad, 2016-11-04 19:26:08]

Green tests at patch 1, new test added at patch 2.

[commit-bot: I haz the power, 2016-11-04 19:26:42]

Dry run: 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-11-04 20:57:02]

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

[commit-bot: I haz the power, 2016-11-04 20:57:03]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dschuyler, 2016-11-04 22:22:19]

lgtm

[dpapad, 2016-11-05 00:23:21]

On 2016/11/04 at 22:22:19, dschuyler wrote:
> lgtm

I verified that the test I am adding is unfortunately flaky, because of
CrScrollableBehavior executing code after the test has finished. This causes a
null dereference at
https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_scroll...,
which causes the test to fail (only sometimes, depends on timing). Given
that this is fixing a crash, and given that updateScrollableContents() logic is
attempted to be removed as part of crbug.com/657336, how about I land this fix
without the test
for now, instead of trying to tweak updateScrollableContents() logic to take
into account the element disappearing?

Any suggestions/workarounds that will deflake the test welcome.

[dschuyler, 2016-11-05 00:26:35]

On 2016/11/05 00:23:21, dpapad wrote:
> On 2016/11/04 at 22:22:19, dschuyler wrote:
> > lgtm
> 
> I verified that the test I am adding is unfortunately flaky, because of
> CrScrollableBehavior executing code after the test has finished. This causes a
> null dereference at
>
https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_scroll...,
> which causes the test to fail (only sometimes, depends on timing). Given
> that this is fixing a crash, and given that updateScrollableContents() logic
is
> attempted to be removed as part of crbug.com/657336, how about I land this fix
> without the test
> for now, instead of trying to tweak updateScrollableContents() logic to take
> into account the element disappearing?
> 
> Any suggestions/workarounds that will deflake the test welcome.

Maybe add a 

detached: function() {
  clearInterval(this.intervalId)
},

after scoping intervalId on |this|

in cr_scrollable_behavior.js

[dpapad, 2016-11-05 00:37:31]

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

[commit-bot: I haz the power, 2016-11-05 00:38:06]

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

[dpapad, 2016-11-05 00:38:26]

On 2016/11/05 at 00:26:35, dschuyler wrote:
> On 2016/11/05 00:23:21, dpapad wrote:
> > On 2016/11/04 at 22:22:19, dschuyler wrote:
> > > lgtm
> > 
> > I verified that the test I am adding is unfortunately flaky, because of
> > CrScrollableBehavior executing code after the test has finished. This causes
a
> > null dereference at
> >
https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_scroll...,
> > which causes the test to fail (only sometimes, depends on timing). Given
> > that this is fixing a crash, and given that updateScrollableContents() logic
is
> > attempted to be removed as part of crbug.com/657336, how about I land this
fix
> > without the test
> > for now, instead of trying to tweak updateScrollableContents() logic to take
> > into account the element disappearing?
> > 
> > Any suggestions/workarounds that will deflake the test welcome.
> 
> Maybe add a 
> 
> detached: function() {
>   clearInterval(this.intervalId)
> },
> 
> after scoping intervalId on |this|
> 
> in cr_scrollable_behavior.js

Thanks, done! This should in theory remove the flakiness. Let's see how the bots
react.

[commit-bot: I haz the power, 2016-11-05 01:41:51]

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

[commit-bot: I haz the power, 2016-11-05 01:41:52]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dpapad, 2016-11-07 19:17:48]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-11-07 19:17:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2477693005/#ps80001
(title: "Fix flakiness.")

[commit-bot: I haz the power, 2016-11-07 19:18:41]

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

[dpapad, 2016-11-07 19:58:34]

The CQ bit was unchecked by dpapad@chromium.org

[dpapad, 2016-11-07 22:34:31]

Thanks Dave for helping investigating the test failure. So it turns out that
during the test, iron-list was never registered as a custom element with the
browser (HTML import of iron-list.html was mssing), and therefore it did not
have a notifyResize method.

[dpapad, 2016-11-07 22:37:35]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-11-07 22:37:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2477693005/#ps100001
(title: "Add missing dependency to iron-list.html")

[commit-bot: I haz the power, 2016-11-07 22:38:15]

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-11-07 23:52:25]

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

[commit-bot: I haz the power, 2016-11-07 23:52:26]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dpapad, 2016-11-08 00:18:56]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-11-08 00:19:29]

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-11-08 01:10:07]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-11-08 01:17:28]

Description was changed from

==========
MD Settings: Close edit startup URL dialog, if underlying URL list changes.

BUG=661867
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Close edit startup URL dialog, if underlying URL list changes.

BUG=661867
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/05f574e9836e8345bbb59f9b828bf2d773331f82
Cr-Commit-Position: refs/heads/master@{#430455}
==========

[commit-bot: I haz the power, 2016-11-08 01:17:29]

Patchset 4 (id:??) landed as
https://crrev.com/05f574e9836e8345bbb59f9b828bf2d773331f82
Cr-Commit-Position: refs/heads/master@{#430455}