Fix PrintPreviewDestinationSearchTest.Select

Fix PrintPreviewDestinationSearchTest.Select

This reverts commit d2672f44f61002cfff2d7fd5a4bc70d54d903304.

Uninitialized booleans are causing the test to crash when used to build
a JSON string on the ASAN bot.  Problem surfaced after clang was uprevved.

BUG=702640
Review-Url: https://codereview.chromium.org/2788283002
Cr-Commit-Position: refs/heads/master@{#462312}
Committed: https://chromium.googlesource.com/chromium/src/+/67d0cd0e97bb647deaf65a7adb3dfc76ff71dacc

[skau, 2017-04-01 01:00:03]

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

[commit-bot: I haz the power, 2017-04-01 01:00:41]

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, 2017-04-01 02:51:37]

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

[commit-bot: I haz the power, 2017-04-01 02:51:38]

Dry run: This issue passed the CQ dry run.

[skau, 2017-04-03 18:33:17]

skau@chromium.org changed reviewers:
+ thakis@chromium.org

[skau, 2017-04-03 18:33:17]

Please review.  It looks like uninitialized booleans are now causing failures in
the ASAN bot.

[skau, 2017-04-03 18:39:47]

Description was changed from

==========
Fix PrintPreviewDestinationSearchTest.Select

This reverts commit d2672f44f61002cfff2d7fd5a4bc70d54d903304.

Uninitialized booleans are causing the test to crash when used to build
a JSON string.  I suspect a compiler bug.

BUG=702640
==========

to

==========
Fix PrintPreviewDestinationSearchTest.Select

This reverts commit d2672f44f61002cfff2d7fd5a4bc70d54d903304.

Uninitialized booleans are causing the test to crash when used to build
a JSON string on the ASAN bot.  Problem surfaced after clang was uprevved.

BUG=702640
==========

[skau, 2017-04-03 19:56:07]

skau@chromium.org changed reviewers:
+ thestig@chromium.org
- thakis@chromium.org

[skau, 2017-04-03 19:56:07]

thestig: Can you take a look?  thakis should probably review, but he's out
today.

[Lei Zhang, 2017-04-03 20:32:42]

https://codereview.chromium.org/2788283002/diff/1/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview/print_preview_ui.h (right):

https://codereview.chromium.org/2788283002/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_ui.h:178: bool
source_is_modifiable_ = false;
This is also being set in the constructor... to true. Can we leave this alone?
It's clearly not the cause of the uninit error.

https://codereview.chromium.org/2788283002/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_ui.h:184: bool
print_selection_only_ = false;
This is the missing initialization. Let's move it to the ctor for consistency
with the rest.

[skau, 2017-04-04 17:36:06]

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

[commit-bot: I haz the power, 2017-04-04 17:36:55]

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

[skau, 2017-04-04 17:37:02]

Thanks for the review. I forgot to look at the ctor before making the change.

https://codereview.chromium.org/2788283002/diff/1/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview/print_preview_ui.h (right):

https://codereview.chromium.org/2788283002/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_ui.h:178: bool
source_is_modifiable_ = false;
On 2017/04/03 20:32:42, Lei Zhang wrote:
> This is also being set in the constructor... to true. Can we leave this alone?
> It's clearly not the cause of the uninit error.

Done.

https://codereview.chromium.org/2788283002/diff/1/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_ui.h:184: bool
print_selection_only_ = false;
On 2017/04/03 20:32:42, Lei Zhang wrote:
> This is the missing initialization. Let's move it to the ctor for consistency
> with the rest.

Done.

[commit-bot: I haz the power, 2017-04-04 18:26:05]

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

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

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_...)

[Lei Zhang, 2017-04-04 18:45:26]

Initializing |print_selection_only_| lgtm, but the test is still failing on
trybots. :[

[skau, 2017-04-04 21:06:08]

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

[skau, 2017-04-04 21:06:31]

The recent change to the UI invalidated the test.  I've fixed it now.  I'll wait
until tomorrow to submit if you want to take another look.

[commit-bot: I haz the power, 2017-04-04 21:06:51]

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

[Lei Zhang, 2017-04-04 21:22:32]

++lgtm

[skau, 2017-04-04 21:38:53]

The CQ bit was checked by skau@chromium.org

[commit-bot: I haz the power, 2017-04-04 21:39:09]

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-04 22:19:25]

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

[commit-bot: I haz the power, 2017-04-04 22:19:26]

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_...)

[skau, 2017-04-04 23:04:44]

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

[commit-bot: I haz the power, 2017-04-04 23:05:25]

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

[skau, 2017-04-04 23:24:49]

Looks like the codepath for the non-CrOS platforms are no longer compatible. 
I'll need to tweak the test to ignore some of the platform specific behavior.

[Lei Zhang, 2017-04-04 23:29:45]

Does it make sense to have a non-ChromeOS version of
PrintPreviewDestinationSearchTest.Select as well?

[skau, 2017-04-05 00:45:16]

There's some similarity between the ChromeOS and non-ChromeOS flow so I'm trying
to see if there's a sensible way to test all platforms with the same code.

[commit-bot: I haz the power, 2017-04-05 03:08:12]

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

[commit-bot: I haz the power, 2017-04-05 03:08:13]

Dry run: This issue passed the CQ dry run.

[skau, 2017-04-05 20:13:33]

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

[commit-bot: I haz the power, 2017-04-05 20:14:22]

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

[skau, 2017-04-05 21:43:01]

PTAL.  I've update the test to verify behavior on all the platforms.  The
codepaths have some differences so the tests have some conditional logic in
them.  Let me know if you think I should separate it into two files.

[skau, 2017-04-05 21:43:01]

PTAL.  I've update the test to verify behavior on all the platforms.  The
codepaths have some differences so the tests have some conditional logic in
them.  Let me know if you think I should separate it into two files.

[skau, 2017-04-05 21:43:02]

PTAL.  I've update the test to verify behavior on all the platforms.  The
codepaths have some differences so the tests have some conditional logic in
them.  Let me know if you think I should separate it into two files.

[Lei Zhang, 2017-04-05 21:54:32]

https://codereview.chromium.org/2788283002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/print_preview_destination_search_test.js (right):

https://codereview.chromium.org/2788283002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview_destination_search_test.js:104: if
(!cr.isChromeOS) {
If it's all the same, put the isChromeOS case inside the if block, as it is
shorter.

https://codereview.chromium.org/2788283002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview_destination_search_test.js:151: if
(!cr.isChromeOS) {
Why omit the success key/value for non-ChromeOS now?

https://codereview.chromium.org/2788283002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview_destination_search_test.js:236: if
(cr.isChromeOS) {
How about the non-ChromeOS case?

[commit-bot: I haz the power, 2017-04-05 22:05:50]

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

[commit-bot: I haz the power, 2017-04-05 22:05:51]

Dry run: This issue passed the CQ dry run.

[skau, 2017-04-05 23:40:38]

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

[skau, 2017-04-05 23:41:09]

Comments addressed.  PTAL.

https://codereview.chromium.org/2788283002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/print_preview_destination_search_test.js (right):

https://codereview.chromium.org/2788283002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview_destination_search_test.js:104: if
(!cr.isChromeOS) {
On 2017/04/05 21:54:31, Lei Zhang wrote:
> If it's all the same, put the isChromeOS case inside the if block, as it is
> shorter.

Done.

https://codereview.chromium.org/2788283002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview_destination_search_test.js:151: if
(!cr.isChromeOS) {
On 2017/04/05 21:54:31, Lei Zhang wrote:
> Why omit the success key/value for non-ChromeOS now?

non-ChromeOS doesn't read the success value.  But it doesn't matter if it's
included.

https://codereview.chromium.org/2788283002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview_destination_search_test.js:236: if
(cr.isChromeOS) {
On 2017/04/05 21:54:32, Lei Zhang wrote:
> How about the non-ChromeOS case?

I've added the non-CrOS case.  It's a bit weird as selection still occurs.  I'll
look into adding an appropriate check in a later CL.

[commit-bot: I haz the power, 2017-04-05 23:41:23]

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

[Lei Zhang, 2017-04-05 23:46:05]

lgtm += 2?

https://codereview.chromium.org/2788283002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/print_preview_destination_search_test.js (right):

https://codereview.chromium.org/2788283002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview_destination_search_test.js:236: if
(cr.isChromeOS) {
On 2017/04/05 23:41:09, skau wrote:
> On 2017/04/05 21:54:32, Lei Zhang wrote:
> > How about the non-ChromeOS case?
> 
> I've added the non-CrOS case.  It's a bit weird as selection still occurs. 
I'll
> look into adding an appropriate check in a later CL.

Thanks.

[skau, 2017-04-06 00:00:07]

The CQ bit was checked by skau@chromium.org

[commit-bot: I haz the power, 2017-04-06 00:00:35]

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

[skau, 2017-04-06 00:17:08]

Logged crbug.com/708834 to address the behavior in print preview.

[commit-bot: I haz the power, 2017-04-06 01:36:37]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491436807426710,
"parent_rev": "e792e0e91d5b20121b9eb634b43af7eeb04b612f", "commit_rev":
"67d0cd0e97bb647deaf65a7adb3dfc76ff71dacc"}

[commit-bot: I haz the power, 2017-04-06 01:37:30]

Description was changed from

==========
Fix PrintPreviewDestinationSearchTest.Select

This reverts commit d2672f44f61002cfff2d7fd5a4bc70d54d903304.

Uninitialized booleans are causing the test to crash when used to build
a JSON string on the ASAN bot.  Problem surfaced after clang was uprevved.

BUG=702640
==========

to

==========
Fix PrintPreviewDestinationSearchTest.Select

This reverts commit d2672f44f61002cfff2d7fd5a4bc70d54d903304.

Uninitialized booleans are causing the test to crash when used to build
a JSON string on the ASAN bot.  Problem surfaced after clang was uprevved.

BUG=702640

Review-Url: https://codereview.chromium.org/2788283002
Cr-Commit-Position: refs/heads/master@{#462312}
Committed:
https://chromium.googlesource.com/chromium/src/+/67d0cd0e97bb647deaf65a7adb3d...
==========

[commit-bot: I haz the power, 2017-04-06 01:37:31]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/67d0cd0e97bb647deaf65a7adb3d...