Update readyForTesting var even on Oobe.initialize() failure.

Update readyForTesting var even on Oobe.initialize() failure.

|readyForTesting| was introduced to let callers know whether
the DOM loading is done so testing APIs accessing DOM can be
successfully called.
Although, in some cases Oobe.initialize() is failed, so
it timed out.
Instead, this CL sets the value always. Then, callers can be notified
even on failure cases, so they can avoid timeout.

BUG=710975
TEST=Ran bots.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2825453002
Cr-Commit-Position: refs/heads/master@{#465126}
Committed: https://chromium.googlesource.com/chromium/src/+/341333dfb5561bc987995a3cfe05aefc3ecb17e7

[hidehiko, 2017-04-17 08:01:39]

Description was changed from

==========
Update readyForTesting var even on Oobe.initialize() failure.

|readyForTesting| was introduced to let callers know whether
the DOM loading is done so testing APIs accessing DOM can be
successfully called.
Although, in some cases Oobe.initialize() is failed, so
it timed out.
Instead, this CL sets the value always. Then, callers can be notified
even on failure cases, so they can avoid timeout.

BUG=710975
TEST=Ran bots.
==========

to

==========
Update readyForTesting var even on Oobe.initialize() failure.

|readyForTesting| was introduced to let callers know whether
the DOM loading is done so testing APIs accessing DOM can be
successfully called.
Although, in some cases Oobe.initialize() is failed, so
it timed out.
Instead, this CL sets the value always. Then, callers can be notified
even on failure cases, so they can avoid timeout.

BUG=710975
TEST=Ran bots.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[hidehiko, 2017-04-17 08:01:47]

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

[commit-bot: I haz the power, 2017-04-17 08:02:08]

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

[hidehiko, 2017-04-17 08:07:14]

hidehiko@chromium.org changed reviewers:
+ achuith@chromium.org

[hidehiko, 2017-04-17 08:07:14]

achuith@, PTAL.

The corresponding CL is https://codereview.chromium.org/2824703003/.

The strategy to fix the last breakage was;
- Initialize the var with "null".
- Set "true" or "false", depending on the initialize() success.
- Telemetry script will look at whether the var is "null" or not. So, that even
on failure, it is unblocked. (This part is in the corresponding CL).
Temporarily, ignores the value to avoid test failure regression.
- TODO: Raise an exception on initialize() failure (i.e. if var is set to
false), when we make a proper fix for the failure of tests. Note that this is
not the regression.

Thank you for review in advance,
- hidehiko

[achuithb, 2017-04-17 08:42:08]

I'm not a fan of having a hack here, and another hack in the catapult repo.

If you can move the readyForTesting line before initialize(), that may be
sufficient to fix the bug we're looking at.

If that doesn't work, maybe something like:
try {
  Oobe.initialize();
  Oobe.readyForTesting = true;
} catch (e) {
  Oobe.readyForTesting = true;
  throw e;
}

And initialize readyForTesting to false as you had before.

Having readyForTesting have a ternary value makes this more complicated than it
needs to be.

If the Oobe initialization fails, most likely this will not affect the ability
to run test scripts, so we should set readyForTesting to true.

https://codereview.chromium.org/2825453002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/login_shared.js (left):

https://codereview.chromium.org/2825453002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/login_shared.js:453:
Oobe.readyForTesting = true;
What happens if you move this line to before Oobe.initialize?

[commit-bot: I haz the power, 2017-04-17 08:53:58]

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

[commit-bot: I haz the power, 2017-04-17 08:53:58]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-04-17 09:24:05]

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

[commit-bot: I haz the power, 2017-04-17 09:24:17]

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

[hidehiko, 2017-04-17 09:27:16]

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

[hidehiko, 2017-04-17 09:27:17]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-04-17 09:27:27]

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

[hidehiko, 2017-04-17 09:32:27]

Thank you for review!

Had offline chat with achuith@, and agreed to use finally for the short term
workaround, which should be fixed after proper fix of the tests.

Thanks to cywang@, the proper fix is on track (crrev.com/2819183003), AFAIK.
Though, this is cherry pick target, so I'd like to avoid depending it, to make
cherry-pick size as small as possible.

PTAL.

https://codereview.chromium.org/2825453002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/login_shared.js (left):

https://codereview.chromium.org/2825453002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/login_shared.js:453:
Oobe.readyForTesting = true;
On 2017/04/17 08:42:08, achuithb wrote:
> What happens if you move this line to before Oobe.initialize?

Acknowledged.

[commit-bot: I haz the power, 2017-04-17 11:20:34]

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

[commit-bot: I haz the power, 2017-04-17 11:20:35]

Dry run: This issue passed the CQ dry run.

[achuithb, 2017-04-17 18:40:22]

lgtm

[hidehiko, 2017-04-18 03:21:48]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2017-04-18 03:22:14]

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, 2017-04-18 03:26:53]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492485708749690,
"parent_rev": "b497f749f667199fc86edee499bdb36519fa4407", "commit_rev":
"341333dfb5561bc987995a3cfe05aefc3ecb17e7"}

[commit-bot: I haz the power, 2017-04-18 03:27:04]

Description was changed from

==========
Update readyForTesting var even on Oobe.initialize() failure.

|readyForTesting| was introduced to let callers know whether
the DOM loading is done so testing APIs accessing DOM can be
successfully called.
Although, in some cases Oobe.initialize() is failed, so
it timed out.
Instead, this CL sets the value always. Then, callers can be notified
even on failure cases, so they can avoid timeout.

BUG=710975
TEST=Ran bots.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Update readyForTesting var even on Oobe.initialize() failure.

|readyForTesting| was introduced to let callers know whether
the DOM loading is done so testing APIs accessing DOM can be
successfully called.
Although, in some cases Oobe.initialize() is failed, so
it timed out.
Instead, this CL sets the value always. Then, callers can be notified
even on failure cases, so they can avoid timeout.

BUG=710975
TEST=Ran bots.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2825453002
Cr-Commit-Position: refs/heads/master@{#465126}
Committed:
https://chromium.googlesource.com/chromium/src/+/341333dfb5561bc987995a3cfe05...
==========

[commit-bot: I haz the power, 2017-04-18 03:27:06]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/341333dfb5561bc987995a3cfe05...