[webkit-reviews] review requested: [Bug 35350] DumpRenderTree should allow tests with modal dialogs : [Attachment 58829] Patch to add a showModalDialog support for DumpRenderTree on Mac.

[bugzilla-daemon at webkit.org]

Prasad Tammana <prasadt at chromium.org> has asked  for review:
Bug 35350: DumpRenderTree should allow tests with modal dialogs
https://bugs.webkit.org/show_bug.cgi?id=35350

Attachment 58829: Patch to add a showModalDialog support for DumpRenderTree on
Mac.
https://bugs.webkit.org/attachment.cgi?id=58829&action=review

------- Additional Comments from Prasad Tammana <prasadt at chromium.org>
Darin - I updated a new patch with my alternate solution which is to add
abortModal method to LayoutTestController and expose it to script.  And undid
my change to the performSelector method.

So, in summary I have tried out three approaches I'm listing with what in my
opinion are the pros and cons.

1) My earlier patch while does the inModes version of performSelector but
enabling it for only two modes - normal loop and panel mode.
    Pros:
      a) It works.
      b) Impact is narrow and well defined.
      c) You'll now be able to close modal dialogs from script without
overriding NSApplication in your app.  I consider that bug fix.
    Cons:
      a) Its technically a backward incompatible change.  Apps meeting all the
conditions below will break:
	  - Its a non-safari app AND
	  - Its built on top of webkit AND
	  - It does not override NSApplication or does some other trick to make
closing from script work in modal loop AND
	  - It implements and uses showModalDialog AND
	  - It has code trying to close the modal dialog from script (that
would basically be dead code currently)
	  If all the above conditions are met, then the modal dialog will now
be closing where it didn't before.

2) My new patch which adds abortModal method to LayoutTestController and makes
it available to script.
    Pros:
      a) It works.
      b) No change to non-test code.
    Cons:
      a) Test implementation slightly different from Safari.  Probably doesn't
mean much since its already different anyway.

3) Your suggestion about overriding NSApplication method to pretend to be in
normal mode when actually in panel mode.
     Pros:
       a) Test matches Safari implementation.
     Cons:
       a) Doesn't work for me.	I tried this again to be sure and I'm getting
consistent crashes with the following stack.  Not sure what trick I'm missing
in terms of setting this up right.
	    objc[46326]: FREED(id): message isSheet sent to freed
object=0x10c40b0
	    #0 0x976dabfa in _objc_error
	    #1 0x976dac30 in __objc_error
	    #2 0x976d9637 in _freedHandler
	    #3 0x902a5c7e in runModalCleanup
	    #4 0x903c9b04 in -[NSApplication runModalForWindow:]
	    #5 0x00031c2e in -[UIDelegate webViewRunModal:] at UIDelegate.mm:80

	    #6 0x00bf0d5e in CallDelegate at
WebDelegateImplementationCaching.mm:89
	    #7 0x00bf0dc8 in CallUIDelegate at
WebDelegateImplementationCaching.mm:420
	    #8 0x00be3d09 in WebChromeClient::runModal at
WebChromeClient.mm:267
       b) Overriding NSApplication to make this work in theory impacts all the
tests which could be risky.

I'm fine going with either 1) or 2).  Its obviously going to be your call if
you think the small risk of backward incompatibility of 1) is acceptable or not
- based on your response
so far, I'm guessing its not.  2) could be the way to go.  If it has to be 3)
for some reason, I'll need help moving forward with that.

Let me know your thoughts.  Thanks.