Fix HandleJavaScriptDialog.

Fix HandleJavaScriptDialog.

The new dialogs had a bug where they misinterpreted a null pointer in HandleJavaScriptDialog as a request to return an empty string rather than the contents in the dialog.

Turning on the new dialogs for trunk revealed this on the Chromedriver bots, but there was no test coverage in Chromium proper to catch this.

This change adds testing for HandleJavaScriptDialog in the Chromium repo, and fixes the bug.

BUG=chromedriver:1792, 719551
TEST=re-enabled chromedriver test, new JavaScriptDialogTest.HandleJavaScriptDialog

Review-Url: https://codereview.chromium.org/2867613002
Cr-Commit-Position: refs/heads/master@{#470020}
Committed: https://chromium.googlesource.com/chromium/src/+/b38dc9a7d0f5457f892a031b407f0346b666430d

[Avi (use Gerrit), 2017-05-06 20:57:35]

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

[commit-bot: I haz the power, 2017-05-06 20:57:51]

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-05-06 22:15:49]

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

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

Dry run: 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_...)

[Avi (use Gerrit), 2017-05-06 22:22:01]

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

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

Dry run: CQ is trying da patch.

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

[Avi (use Gerrit), 2017-05-06 23:22:04]

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

[commit-bot: I haz the power, 2017-05-06 23:37:07]

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

[commit-bot: I haz the power, 2017-05-06 23:37:08]

Dry run: 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_...)

[Avi (use Gerrit), 2017-05-06 23:37:32]

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

[commit-bot: I haz the power, 2017-05-06 23:37:37]

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-05-07 02:16:56]

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

[commit-bot: I haz the power, 2017-05-07 02:16:57]

Dry run: This issue passed the CQ dry run.

[Nico, 2017-05-08 13:55:45]

code lgtm, but please write a CL description (afaiu after reading the bug,
something like "we've always had a bug where we dropped dialog input data in
case $foo, but until https://codereview.chromium.org/2784533002 nobody noticed.
after that change, we always have case $foo, so add some plumbing to make sure
we don't lose data in this case". it would've been nice if the cl description
had said that in the first place)

[Avi (use Gerrit), 2017-05-08 15:48:36]

Description was changed from

==========
Fix HandleJavaScriptDialog.

BUG=chromedriver:1792
TEST=re-enabled chromedriver test, new
JavaScriptDialogTest.HandleJavaScriptDialog
==========

to

==========
Fix HandleJavaScriptDialog.

The new dialogs handled it incorrectly.

BUG=chromedriver:1792
TEST=re-enabled chromedriver test, new
JavaScriptDialogTest.HandleJavaScriptDialog
==========

[Avi (use Gerrit), 2017-05-08 15:48:52]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2017-05-08 15:49:21]

CQ is trying da patch.

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

[Nico, 2017-05-08 16:03:45]

On 2017/05/08 15:49:21, commit-bot: I haz the power wrote:
> CQ is trying da patch.
> 
> Follow status at:
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

(CL description still doesn't say what went wrong, why, and how it's fixed)

[Avi (use Gerrit), 2017-05-08 16:05:39]

On 2017/05/08 16:03:45, Nico wrote:
> On 2017/05/08 15:49:21, commit-bot: I haz the power wrote:
> > CQ is trying da patch.
> > 
> > Follow status at:
> >
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> 
> (CL description still doesn't say what went wrong, why, and how it's fixed)

It was a bug in the new dialogs. I didn't fully understand how Handle was
supposed to work, I turned on the tests on trunk, got a bug, added a test
directly in the Chromium repo, and fixed it.

[Nico, 2017-05-08 16:12:07]

Cl descriptions are for people reading them years later, wondering why a
change was made. A cl description that literally says "fix bug" is not very
useful for that. As said above, please say what went wrong, why, and how
it's fixed. I included a possible template above.

On May 8, 2017 12:05 PM, <avi@chromium.org> wrote:

> On 2017/05/08 16:03:45, Nico wrote:
> > On 2017/05/08 15:49:21, commit-bot: I haz the power wrote:
> > > CQ is trying da patch.
> > >
> > > Follow status at:
> > >
> >
> https://chromium-cq-status.appspot.com/v2/patch-status/
> codereview.chromium.org/2867613002/1
> >
> > (CL description still doesn't say what went wrong, why, and how it's
> fixed)
>
> It was a bug in the new dialogs. I didn't fully understand how Handle was
> supposed to work, I turned on the tests on trunk, got a bug, added a test
> directly in the Chromium repo, and fixed it.
>
> https://codereview.chromium.org/2867613002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2017-05-08 16:16:04]

See also
https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list...

On May 8, 2017 12:12 PM, "Nico Weber" <thakis@chromium.org> wrote:

> Cl descriptions are for people reading them years later, wondering why a
> change was made. A cl description that literally says "fix bug" is not very
> useful for that. As said above, please say what went wrong, why, and how
> it's fixed. I included a possible template above.
>
> On May 8, 2017 12:05 PM, <avi@chromium.org> wrote:
>
>> On 2017/05/08 16:03:45, Nico wrote:
>> > On 2017/05/08 15:49:21, commit-bot: I haz the power wrote:
>> > > CQ is trying da patch.
>> > >
>> > > Follow status at:
>> > >
>> >
>> https://chromium-cq-status.appspot.com/v2/patch-status/coder
>> eview.chromium.org/2867613002/1
>> >
>> > (CL description still doesn't say what went wrong, why, and how it's
>> fixed)
>>
>> It was a bug in the new dialogs. I didn't fully understand how Handle was
>> supposed to work, I turned on the tests on trunk, got a bug, added a test
>> directly in the Chromium repo, and fixed it.
>>
>> https://codereview.chromium.org/2867613002/
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Avi (use Gerrit), 2017-05-08 16:19:39]

Description was changed from

==========
Fix HandleJavaScriptDialog.

The new dialogs handled it incorrectly.

BUG=chromedriver:1792
TEST=re-enabled chromedriver test, new
JavaScriptDialogTest.HandleJavaScriptDialog
==========

to

==========
Fix HandleJavaScriptDialog.

The new dialogs had a bug where they misinterpreted a null pointer in
HandleJavaScriptDialog as a request to return an empty string rather than the
contents in the dialog.

Turning on the new dialogs for trunk revealed this on the Chromedriver bots, but
there was no test coverage in Chromium proper to catch this.

This change adds testing for HandleJavaScriptDialog in the Chromium repo, and
fixes the bug.

BUG=chromedriver:1792, 719551
TEST=re-enabled chromedriver test, new
JavaScriptDialogTest.HandleJavaScriptDialog
==========

[Avi (use Gerrit), 2017-05-08 16:20:11]

On 2017/05/08 16:16:04, Nico wrote:
> See also
>
https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list...
> 
> On May 8, 2017 12:12 PM, "Nico Weber" <mailto:thakis@chromium.org> wrote:
> 
> > Cl descriptions are for people reading them years later, wondering why a
> > change was made. A cl description that literally says "fix bug" is not very
> > useful for that. As said above, please say what went wrong, why, and how
> > it's fixed. I included a possible template above.
> >
> > On May 8, 2017 12:05 PM, <mailto:avi@chromium.org> wrote:
> >
> >> On 2017/05/08 16:03:45, Nico wrote:
> >> > On 2017/05/08 15:49:21, commit-bot: I haz the power wrote:
> >> > > CQ is trying da patch.
> >> > >
> >> > > Follow status at:
> >> > >
> >> >
> >> https://chromium-cq-status.appspot.com/v2/patch-status/coder
> >> eview.chromium.org/2867613002/1
> >> >
> >> > (CL description still doesn't say what went wrong, why, and how it's
> >> fixed)
> >>
> >> It was a bug in the new dialogs. I didn't fully understand how Handle was
> >> supposed to work, I turned on the tests on trunk, got a bug, added a test
> >> directly in the Chromium repo, and fixed it.
> >>
> >> https://codereview.chromium.org/2867613002/
> >>
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

PTAL.

[Nico, 2017-05-08 16:23:21]

On 2017/05/08 16:20:11, Avi (ping after 24h) wrote:
> On 2017/05/08 16:16:04, Nico wrote:
> > See also
> >
>
https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list...
> > 
> > On May 8, 2017 12:12 PM, "Nico Weber" <mailto:thakis@chromium.org> wrote:
> > 
> > > Cl descriptions are for people reading them years later, wondering why a
> > > change was made. A cl description that literally says "fix bug" is not
very
> > > useful for that. As said above, please say what went wrong, why, and how
> > > it's fixed. I included a possible template above.
> > >
> > > On May 8, 2017 12:05 PM, <mailto:avi@chromium.org> wrote:
> > >
> > >> On 2017/05/08 16:03:45, Nico wrote:
> > >> > On 2017/05/08 15:49:21, commit-bot: I haz the power wrote:
> > >> > > CQ is trying da patch.
> > >> > >
> > >> > > Follow status at:
> > >> > >
> > >> >
> > >> https://chromium-cq-status.appspot.com/v2/patch-status/coder
> > >> eview.chromium.org/2867613002/1
> > >> >
> > >> > (CL description still doesn't say what went wrong, why, and how it's
> > >> fixed)
> > >>
> > >> It was a bug in the new dialogs. I didn't fully understand how Handle was
> > >> supposed to work, I turned on the tests on trunk, got a bug, added a test
> > >> directly in the Chromium repo, and fixed it.
> > >>
> > >> https://codereview.chromium.org/2867613002/
> > >>
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> PTAL.

Looks great, thanks much!

[commit-bot: I haz the power, 2017-05-08 16:55:10]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1494258532747190, "parent_rev":
"121980770b3b9c81971b1277180e303c64905c1c", "commit_rev":
"b38dc9a7d0f5457f892a031b407f0346b666430d"}

[commit-bot: I haz the power, 2017-05-08 16:55:22]

Description was changed from

==========
Fix HandleJavaScriptDialog.

The new dialogs had a bug where they misinterpreted a null pointer in
HandleJavaScriptDialog as a request to return an empty string rather than the
contents in the dialog.

Turning on the new dialogs for trunk revealed this on the Chromedriver bots, but
there was no test coverage in Chromium proper to catch this.

This change adds testing for HandleJavaScriptDialog in the Chromium repo, and
fixes the bug.

BUG=chromedriver:1792, 719551
TEST=re-enabled chromedriver test, new
JavaScriptDialogTest.HandleJavaScriptDialog
==========

to

==========
Fix HandleJavaScriptDialog.

The new dialogs had a bug where they misinterpreted a null pointer in
HandleJavaScriptDialog as a request to return an empty string rather than the
contents in the dialog.

Turning on the new dialogs for trunk revealed this on the Chromedriver bots, but
there was no test coverage in Chromium proper to catch this.

This change adds testing for HandleJavaScriptDialog in the Chromium repo, and
fixes the bug.

BUG=chromedriver:1792, 719551
TEST=re-enabled chromedriver test, new
JavaScriptDialogTest.HandleJavaScriptDialog

Review-Url: https://codereview.chromium.org/2867613002
Cr-Commit-Position: refs/heads/master@{#470020}
Committed:
https://chromium.googlesource.com/chromium/src/+/b38dc9a7d0f5457f892a031b407f...
==========

[commit-bot: I haz the power, 2017-05-08 16:55:23]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/b38dc9a7d0f5457f892a031b407f...