Fix the flaky PasswordManagerBrowserTest.PasswordValueAccessible test

Fix the flaky PasswordManagerBrowserTest.PasswordValueAccessible test

This test was flaky, because of a race condition between JavaScript
snippets and input events sent to a renderer via IPC.

This CL modifies the JavaScript bits to check whether chagnes in DOM
caused by the expected input events have been carried out.

See also https://groups.google.com/a/chromium.org/d/msg/chromium-dev/rDxk2D824QI/bMleOa0DcQcJ

This CL also enables the test on all platforms.

BUG=346297
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276045

[vabr (Chromium), 2014-06-05 12:53:24]

Hi bartfab@,
Would you mind reviewing this one for me?

Thanks!
Vaclav

[bartfab (slow), 2014-06-05 16:45:54]

https://codereview.chromium.org/316163003/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/316163003/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:177: // Waits
until the "value" atribute of the HTML element with |element_id| is
Nit: s/atribute/attribute/

https://codereview.chromium.org/316163003/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:664:
WaitForElementValue("username_field", "temp");
Nit: That's a bit hacky. Since you expect the user name field to go from "temp"
to "temp," you have no way to know whether any pending changes have actually
happened already, other than by waiting for some other field to change and
concluding that when that happens, all pending changes are done.

If the user name field actually does *change* from "temp" to "temp," you could
improve your code as follows:
- Run JS that sets up an onchange listener.
- Trigger the mouse click.
- Run JS that waits for the onchange listener to fire and sends back the result.

This way, you will know for sure that the transition, even if just from "temp"
to "temp," has happened.

[vabr (Chromium), 2014-06-05 17:58:13]

Thanks, Bartosz!

Comments addressed, PTAL.

Cheers,
Vaclav

https://codereview.chromium.org/316163003/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/316163003/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:177: // Waits
until the "value" atribute of the HTML element with |element_id| is
On 2014/06/05 16:45:54, bartfab wrote:
> Nit: s/atribute/attribute/

Done.

https://codereview.chromium.org/316163003/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:664:
WaitForElementValue("username_field", "temp");
On 2014/06/05 16:45:54, bartfab wrote:
> Nit: That's a bit hacky. Since you expect the user name field to go from
"temp"
> to "temp," you have no way to know whether any pending changes have actually
> happened already, other than by waiting for some other field to change and
> concluding that when that happens, all pending changes are done.
> 
> If the user name field actually does *change* from "temp" to "temp," you could
> improve your code as follows:
> - Run JS that sets up an onchange listener.
> - Trigger the mouse click.
> - Run JS that waits for the onchange listener to fire and sends back the
result.
> 
> This way, you will know for sure that the transition, even if just from "temp"
> to "temp," has happened.

Following an off-line clarification, I tried to make explicit what I wait for
and why.

[bartfab (slow), 2014-06-06 11:19:34]

lgtm

https://codereview.chromium.org/316163003/diff/60001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/316163003/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:200:
RETURN_CODE_INVALID };
Nit: Put the }; on its own line.

[vabr (Chromium), 2014-06-06 17:21:58]

Thanks, Bartosz.

Comment addressed, I'll send this to CQ now.

Cheers,
Vaclav

https://codereview.chromium.org/316163003/diff/60001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/316163003/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:200:
RETURN_CODE_INVALID };
On 2014/06/06 11:19:34, bartfab wrote:
> Nit: Put the }; on its own line.

Yeah, this one also looked strange to me, but that's how clang-format formatted
it.

Anyway, I did as you said, and will file a ticket for a bug in clang-format.

[vabr (Chromium), 2014-06-06 17:22:02]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-06-06 17:23:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vabr@chromium.org/316163003/80001

[vabr (Chromium), 2014-06-06 17:25:56]

The CQ bit was unchecked by vabr@chromium.org

[vabr (Chromium), 2014-06-06 17:28:27]

Just to explain patch set 4:

It looks like clang-format would put the closing "}" of an enum on a separate
line, if the last element has a trailing comma.

Looking at the style guide, all the examples have last elements with trailing
commas.

I'd love to have the code in a state when clang-format agrees with every line,
because that makes sem-automated refactoruing involving clang-format much
easier.

Therefore I did the little change above.

Re-sending to CQ.

Cheers,
Vaclav

[vabr (Chromium), 2014-06-06 17:28:32]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-06-06 17:29:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vabr@chromium.org/316163003/100001

[vabr (Chromium), 2014-06-06 17:34:14]

FYI: clang-format bug filed -- (bug id 15467886).

[commit-bot: I haz the power, 2014-06-06 22:15:51]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/14012)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/17128)

[commit-bot: I haz the power, 2014-06-06 22:53:25]

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

[commit-bot: I haz the power, 2014-06-06 22:53:26]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[vabr (Chromium), 2014-06-10 07:01:28]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-06-10 07:04:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vabr@chromium.org/316163003/120001

[vabr (Chromium), 2014-06-10 14:59:54]

The CQ bit was unchecked by vabr@chromium.org

[vabr (Chromium), 2014-06-10 15:00:04]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-06-10 15:00:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vabr@chromium.org/316163003/120001

[commit-bot: I haz the power, 2014-06-10 16:03:07]

Change committed as 276045