Bug 58191

Summary: Use console.log() instead of alert() in unload handlers in tests
Product: WebKit Reporter: Sreeram Ramachandran <gro.mareers>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Enhancement CC: ap, dglazkov, ojan, priyajeet.hora, rniwa, sreeram, webkit.review.bot
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 53359    
Bug Blocks: 56397    
Attachments:
Description Flags
Patch
abarth: review-
Same patch as before
none
Archive of layout-test-results from ec2-cr-linux-03
none
Updated test expectation ojan: review-, ojan: commit-queue-

Description Sreeram Ramachandran 2011-04-09 15:35:10 PDT
Since alerts in unload handlers are going to be suppressed soon (see bug 56397), instead use console.log() in LayoutTests.
Comment 1 Sreeram Ramachandran 2011-04-09 16:02:22 PDT
Created attachment 88934 [details]
Patch
Comment 2 Ryosuke Niwa 2011-04-09 16:06:42 PDT
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.
Comment 3 Sreeram Ramachandran 2011-04-09 16:08:17 PDT
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.
Comment 4 Alexey Proskuryakov 2011-04-09 23:41:24 PDT
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.
Comment 5 Sreeram Ramachandran 2011-04-10 09:07:00 PDT
(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).
Comment 6 Darth 2011-04-13 11:16:37 PDT
While not entirely necessary since this is only dev related, perhaps this then depends on https://bugs.webkit.org/show_bug.cgi?id=53359
Comment 7 Darth 2011-04-13 11:21:24 PDT
Sorry, I am blind, didn't see it was already dependent on it.
Comment 8 Adam Barth 2011-05-05 01:37:41 PDT
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.
Comment 9 Ojan Vafai 2011-06-02 10:58:59 PDT
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?
Comment 10 Ryosuke Niwa 2011-06-02 11:03:24 PDT
(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.
Comment 11 Ojan Vafai 2011-06-02 11:43:56 PDT
(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.
Comment 12 Ryosuke Niwa 2011-06-02 11:53:37 PDT
(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.
Comment 13 Ojan Vafai 2011-06-02 14:28:13 PDT
(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.
Comment 14 Ryosuke Niwa 2011-06-02 14:39:43 PDT
(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.
Comment 15 Ojan Vafai 2011-06-02 14:57:54 PDT
(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?
Comment 16 Ryosuke Niwa 2011-06-02 15:10:28 PDT
(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.
Comment 17 Alexey Proskuryakov 2011-06-02 17:10:53 PDT
I know that I would be confused to no end if alert-debugging didn't ever work for me.
Comment 18 Sreeram Ramachandran 2011-06-14 12:01:59 PDT
Created attachment 97152 [details]
Same patch as before
Comment 19 Sreeram Ramachandran 2011-06-14 12:04:49 PDT
(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().
Comment 20 WebKit Review Bot 2011-06-14 12:17:04 PDT
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
Comment 21 WebKit Review Bot 2011-06-14 12:17:10 PDT
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
Comment 22 Sreeram Ramachandran 2011-06-14 12:58:37 PDT
Created attachment 97157 [details]
Updated test expectation
Comment 23 Ojan Vafai 2011-06-14 13:14:15 PDT
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.
Comment 24 Sreeram Ramachandran 2011-06-14 13:29:54 PDT
(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.
Comment 25 Ryosuke Niwa 2011-06-14 13:35:16 PDT
(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.
Comment 26 Ryosuke Niwa 2011-06-14 13:37:32 PDT
(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.
Comment 27 Ojan Vafai 2011-06-14 15:01:50 PDT
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.
Comment 28 Sreeram Ramachandran 2011-06-14 15:20:52 PDT
Okay. I'll abandon this, and work on adding a console message for such alerts as part of fixing bug 56397.