Bug 58191 – Use console.log() instead of alert() in unload handlers in tests

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[Sreeram Ramachandran]

Since alerts in unload handlers are going to be suppressed soon (see bug 56397), instead use console.log() in LayoutTests.

[Sreeram Ramachandran]

Created attachment 88934 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 88934 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88934&action=review

> LayoutTests/fast/events/pageshow-pagehide-on-back-uncached.html:28
> -    alert("window.onpagehide" + ", target = " + evt.target + ", persisted = " + evt.persisted);
> +    console.log("window.onpagehide" + ", target = " + evt.target + ", persisted = " + evt.persisted);

Why do we need to change this?  It seems unrelated.

[Sreeram Ramachandran]

Comment on attachment 88934 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88934&action=review

>> LayoutTests/fast/events/pageshow-pagehide-on-back-uncached.html:28
>> +    console.log("window.onpagehide" + ", target = " + evt.target + ", persisted = " + evt.persisted);
> 
> Why do we need to change this?  It seems unrelated.

alerts in pagehide are also affected.

[Alexey Proskuryakov]

This will have a negative impact on the tests, because you can see an alert when opening a test in browser manually, but you can't see console log at all (it's cleared after navigation).

We prefer tests that can be debugged interactively whenever possible.

[Sreeram Ramachandran]

(In reply to comment #4)
> This will have a negative impact on the tests, because you can see an alert when opening a test in browser manually, but you can't see console log at all (it's cleared after navigation).
> 
> We prefer tests that can be debugged interactively whenever possible.

Agreed. Is there something better than console.log (and as easy to use, i.e., just a simple function call)? Perhaps we can make the console output persist across navigations? I don't see why not (Developer Tools recently added the ability to preserve the network waterfall across navigations as well, for example).

[Darth]

While not entirely necessary since this is only dev related, perhaps this then depends on https://bugs.webkit.org/show_bug.cgi?id=53359

[Darth]

Sorry, I am blind, didn't see it was already dependent on it.

[Adam Barth]

Comment on attachment 88934 [details]
Patch

Being able to run the tests manually is important, especially when comparing our behavior with that of other browsers.

[Ojan Vafai]

Once bug 56397 is committed, these tests can't be run manually for the Chromium port since the alert won't fire. Maybe one solution to bug 56397 would be to make alerts still log when fired during unload in layout test mode. That essentially gets the same behavior as this patch while leaving the tests manually testable on non-chromium ports.

Sreeram, does that make sense to you?

[Ryosuke Niwa]

(In reply to comment #9)
> Once bug 56397 is committed, these tests can't be run manually for the Chromium port since the alert won't fire. Maybe one solution to bug 56397 would be to make alerts still log when fired during unload in layout test mode. That essentially gets the same behavior as this patch while leaving the tests manually testable on non-chromium ports.

But that would make these tests not manually testable on Chromium (or any other port that decides to disallow alerts), right?  Given that we want to eventually disallow alerts in all ports, that doesn't seem like a great solution.

[Ojan Vafai]

(In reply to comment #10)
> (In reply to comment #9)
> > Once bug 56397 is committed, these tests can't be run manually for the Chromium port since the alert won't fire. Maybe one solution to bug 56397 would be to make alerts still log when fired during unload in layout test mode. That essentially gets the same behavior as this patch while leaving the tests manually testable on non-chromium ports.
> 
> But that would make these tests not manually testable on Chromium (or any other port that decides to disallow alerts), right?  Given that we want to eventually disallow alerts in all ports, that doesn't seem like a great solution.

Do you have a suggestion for how to make these tests manually testable once bug 56397 is fixed? I don't see a good way to accomplish that and it's unfortunate that bug 56397 is now blocked on this.

[Ryosuke Niwa]

(In reply to comment #11)
> Do you have a suggestion for how to make these tests manually testable once bug 56397 is fixed? I don't see a good way to accomplish that and it's unfortunate that bug 56397 is now blocked on this.

Leaving some persistent console message per alert/confirm/etc... during unload/beforeunload would probably do the trick.

[Ojan Vafai]

(In reply to comment #12)
> (In reply to comment #11)
> > Do you have a suggestion for how to make these tests manually testable once bug 56397 is fixed? I don't see a good way to accomplish that and it's unfortunate that bug 56397 is now blocked on this.
> 
> Leaving some persistent console message per alert/confirm/etc... during unload/beforeunload would probably do the trick.

I don't know the code that well, but that seems non-trivial to me. The web inspector ought to have a way to have console messages persist across page loads as per bug 53359, but I don't think we need to make bug 56397 block on getting that implemented.

[Ryosuke Niwa]

(In reply to comment #13)
> (In reply to comment #12)
> > Leaving some persistent console message per alert/confirm/etc... during unload/beforeunload would probably do the trick.
> 
> I don't know the code that well, but that seems non-trivial to me. The web inspector ought to have a way to have console messages persist across page loads as per bug 53359, but I don't think we need to make bug 56397 block on getting that implemented.

But not having an easy to see whether unload or beforeunload have been called seems like a severe restriction for developers to me.  I think we need to give some alternative before killing alerts.

[Ojan Vafai]

(In reply to comment #14)
> But not having an easy to see whether unload or beforeunload have been called seems like a severe restriction for developers to me.  I think we need to give some alternative before killing alerts.

Developers can still put a "debugger;" line in their beforeunload/unload handlers and break into the debugger. Is that not sufficient?

[Ryosuke Niwa]

(In reply to comment #15)
> Developers can still put a "debugger;" line in their beforeunload/unload handlers and break into the debugger. Is that not sufficient?

Mn... I'd imagine that's much harder to do than just inserting alerts.  Like I would be annoyed if I had to attach a breakpoint just to see if my function is called when I can just insert alert in other browsers.  Also, that means people need to do different things to debug on Chromium and other browsers.

But then I do understand that fixing the bug 56397 will benefit ordinary users.  I guess I'm just not sure if that's a good trade off or not.  I'm also afraid that if we fix the bug 56397 first, then we end up not doing persistent alert thing because that's much harder to do and there's very little incentive to implement it.

[Alexey Proskuryakov]

I know that I would be confused to no end if alert-debugging didn't ever work for me.

[Sreeram Ramachandran]

Created attachment 97152 [details]
Same patch as before

[Sreeram Ramachandran]

(In reply to comment #18)
> Created an attachment (id=97152) [details]
> Same patch as before

Now that bug 53359 has been fixed (i.e., console messages can be made persistent across page loads), I think it's reasonable to expect debugging unload handlers to be done with console.log() instead of alert().

[WebKit Review Bot]

Comment on attachment 97152 [details]
Same patch as before

Attachment 97152 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8843271

New failing tests:
fast/history/timed-refresh-in-cached-frame.html

[WebKit Review Bot]

Created attachment 97154 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Sreeram Ramachandran]

Created attachment 97157 [details]
Updated test expectation

[Ojan Vafai]

Comment on attachment 97157 [details]
Updated test expectation

Can we instead make it so that an alert call during an unload handler logs an error to the console? That would make it so we don't need to modify these tests and would have the added benefit of being more friendly to web developers who try to use alert in unload handlers for debugging.

[Sreeram Ramachandran]

(In reply to comment #23)
> (From update of attachment 97157 [details])
> Can we instead make it so that an alert call during an unload handler logs an error to the console? That would make it so we don't need to modify these tests and would have the added benefit of being more friendly to web developers who try to use alert in unload handlers for debugging.

It's probably not very hard, but certainly not trivial, as you pointed out yourself in comment 13.

More importantly, I don't see the benefit. Developers will still have to open the console manually to see the messages. If they are going to do that, why not use console.log() instead?

In general, I think we should encourage developers to use console.log() over alert():
1. It will be consistent across the various webkit platforms. Otherwise, they'll see a console message in Chrome, but a dialog in Safari.
2. It can be used in places where alert() would mess up what you are trying to observe, for example in onblur().
3. It is non-flooding. As a web developer, I've occasionally thrown an alert() in the middle of a loop, only to regret it soon after. If a console message is repeated, the console helpfully adds a repeat count instead of actually repeating the message.
4. Firefox also seems keen on killing alerts in unload (see https://bugzilla.mozilla.org/show_bug.cgi?id=391834). There's no guarantee that they will shim those alerts through to the console.

I think modifying these tests is not a big deal. In the two months since I uploaded the first patch, no new instances of alert() in unload handlers (beforeunload, unload, pagehide) have been introduced. Besides, unless we make the console message exactly the same as the alert ("ALERT: message"), test expectations will still have to be modified.

[Ryosuke Niwa]

(In reply to comment #24)
> More importantly, I don't see the benefit. Developers will still have to open the console manually to see the messages. If they are going to do that, why not use console.log() instead?

This will be a benefit if any other browser does not implement persistent console log.  Developers typically want to be able to test the same code on multiple browsers so having to use console.log only for WebKit-based browsers will be undesirable.

[Ryosuke Niwa]

(In reply to comment #24)
> I think modifying these tests is not a big deal. In the two months since I uploaded the first patch, no new instances of alert() in unload handlers (beforeunload, unload, pagehide) have been introduced. Besides, unless we make the console message exactly the same as the alert ("ALERT: message"), test expectations will still have to be modified.

Expectations change will be much less controversial than test change because you can just land port-specific results.

[Ojan Vafai]

My main concern is that developers using alerts in unload handlers will be confused why they aren't firing. They'll open the developer console to see if they just had a JS error or something and be pleasantly surprised by our helpful logging of the alert to the console instead of needing to spend hours/days figuring out why their alert isn't firing in Chrome but is in other browsers.

[Sreeram Ramachandran]

Okay. I'll abandon this, and work on adding a console message for such alerts as part of fixing bug 56397.