MD Settings: Deflake attempt for CrSettingsPrivacyPageTest.PrivacyPage.

MD Settings: Deflake attempt for CrSettingsPrivacyPageTest.PrivacyPage.

In one of the test cases, showModal() was called after the dialog had already
been removed from the DOM from tearDown().

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

Committed: https://crrev.com/5e840a17185f5ee1fd26750fefff0770dd510a64
Cr-Commit-Position: refs/heads/master@{#418953}

[dpapad, 2016-09-14 00:09:51]

Description was changed from

==========
MD Settings: Deflake attempt for CrSettingsPrivacyPageTest.PrivacyPage.

In one of the test cases, showModal() was called after the dialog had already
been removed from the DOM from tearDown().

BUG=642574
==========

to

==========
MD Settings: Deflake attempt for CrSettingsPrivacyPageTest.PrivacyPage.

In one of the test cases, showModal() was called after the dialog had already
been removed from the DOM from tearDown().

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

[dpapad, 2016-09-14 00:15:48]

Patchset #1 (id:1) has been deleted

[dpapad, 2016-09-14 00:18:08]

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

[Dan Beam, 2016-09-14 01:15:59]

lgtm

https://codereview.chromium.org/2341493003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/privacy_page_test.js (right):

https://codereview.chromium.org/2341493003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/privacy_page_test.js:109:
'clear-browsing-data-dialog-opened', function() {
can we just use 'show' instead of making this event?

[dpapad, 2016-09-15 16:56:17]

https://codereview.chromium.org/2341493003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/privacy_page_test.js (right):

https://codereview.chromium.org/2341493003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/privacy_page_test.js:109:
'clear-browsing-data-dialog-opened', function() {
On 2016/09/14 at 01:15:58, Dan Beam wrote:
> can we just use 'show' instead of making this event?

Do you mean use show() instead of showModal() to display the dialog? If so, no
can't do that (non-modal dialogs look and behave differently).

We could avoid the event though, by using a MutationObserver for the dialog's
'open' attribute. We already do that at
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/certific....
If you prefer that, perhaps we could reuse the MutationObserver code, instead of
repeating across files.

[Dan Beam, 2016-09-15 17:11:06]

https://codereview.chromium.org/2341493003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/privacy_page_test.js (right):

https://codereview.chromium.org/2341493003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/privacy_page_test.js:109:
'clear-browsing-data-dialog-opened', function() {
On 2016/09/15 16:56:17, dpapad wrote:
> On 2016/09/14 at 01:15:58, Dan Beam wrote:
> > can we just use 'show' instead of making this event?
> 
> Do you mean use show() instead of showModal() to display the dialog? If so, no
> can't do that (non-modal dialogs look and behave differently).

i thought, for some reason, that a 'show' event was dispatched.  dang, guess
not.

> 
> We could avoid the event though, by using a MutationObserver for the dialog's
> 'open' attribute. We already do that at
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/certific....
> If you prefer that, perhaps we could reuse the MutationObserver code, instead
of
> repeating across files.

that would make sense.  it /might/ even make sense to add a onAttributeChange()
generic method to util.js, as michaelpg@ just did with: listenOnce
https://cs.chromium.org/chromium/src/ui/webui/resources/js/util.js?q=util.js&...

or we could just add a show() method override in CrDialogElement and call
this.fire('show').

[dpapad, 2016-09-15 17:58:22]

PTAL, since I changed this CL since you LGTMed.

https://codereview.chromium.org/2341493003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/privacy_page_test.js (right):

https://codereview.chromium.org/2341493003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/privacy_page_test.js:109:
'clear-browsing-data-dialog-opened', function() {
> that would make sense.  it /might/ even make sense to add a
onAttributeChange() generic method to util.js, as michaelpg@ just did with:
listenOnce
https://cs.chromium.org/chromium/src/ui/webui/resources/js/util.js?q=util.js&...

I followed similar approach, but I did not add it in util.js, since it is only
needed by tests. I created a test_util.js file (similar to
chrome/test/data/webui/md_history/test_util.js), and I moved there the new
shared method, as well as addressed a TODO to stop duplicating the
eventToPromise() helper method.

[Dan Beam, 2016-09-15 18:17:09]

lgtm w/suggestion because technically your code is checking for a change but we
probably want to wait for a condition to be fulfilled instead

https://codereview.chromium.org/2341493003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/test_util.js (right):

https://codereview.chromium.org/2341493003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/test_util.js:5: 
nit: remove extra \n

https://codereview.chromium.org/2341493003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/test_util.js:13: function
whenAttributeChanges(target, attributeName) {
whenAttributeIs(target, attributeName, attributeValue) {
  function done() { return target[attributeName] === attributeValue; }

  return done() ? Promise.resolve() : new Promise(function(resolve) {
    new MutationObserver(function(mutations, observer) {
      for (var mutation of mutations) {
        if (mutation.attributeName == attributeName && done()) {
          observer.disconnect();
          resolve();
          return;
        }
      }
    }).observe(target, {...});
  });

[dpapad, 2016-09-15 18:50:09]

Patchset #3 (id:60001) has been deleted

[dpapad, 2016-09-15 18:52:12]

https://codereview.chromium.org/2341493003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/test_util.js (right):

https://codereview.chromium.org/2341493003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/test_util.js:5: 
On 2016/09/15 at 18:17:08, Dan Beam wrote:
> nit: remove extra \n

Done.

https://codereview.chromium.org/2341493003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/test_util.js:13: function
whenAttributeChanges(target, attributeName) {
On 2016/09/15 at 18:17:08, Dan Beam wrote:
> whenAttributeIs(target, attributeName, attributeValue) {
>   function done() { return target[attributeName] === attributeValue; }
> 
>   return done() ? Promise.resolve() : new Promise(function(resolve) {
>     new MutationObserver(function(mutations, observer) {
>       for (var mutation of mutations) {
>         if (mutation.attributeName == attributeName && done()) {
>           observer.disconnect();
>           resolve();
>           return;
>         }
>       }
>     }).observe(target, {...});
>   });

Done.

[dpapad, 2016-09-15 18:53:58]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-09-15 18:53:59]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2341493003/#ps80001
(title: "Check for specific value.")

[commit-bot: I haz the power, 2016-09-15 18:54:30]

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-15 19:06:53]

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

[commit-bot: I haz the power, 2016-09-15 19:06:54]

Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[dpapad, 2016-09-15 20:17:51]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-09-15 20:18:25]

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-15 20:25:01]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-09-15 20:27:23]

Description was changed from

==========
MD Settings: Deflake attempt for CrSettingsPrivacyPageTest.PrivacyPage.

In one of the test cases, showModal() was called after the dialog had already
been removed from the DOM from tearDown().

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

to

==========
MD Settings: Deflake attempt for CrSettingsPrivacyPageTest.PrivacyPage.

In one of the test cases, showModal() was called after the dialog had already
been removed from the DOM from tearDown().

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

Committed: https://crrev.com/5e840a17185f5ee1fd26750fefff0770dd510a64
Cr-Commit-Position: refs/heads/master@{#418953}
==========

[commit-bot: I haz the power, 2016-09-15 20:27:24]

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