[Webkit-unassigned] [Bug 35350] DumpRenderTree should allow tests with modal dialogs

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=35350


Prasad Tammana <prasadt at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #58829|                            |review?
               Flag|                            |




--- Comment #12 from Prasad Tammana <prasadt at chromium.org>  2010-06-15 15:16:51 PST ---
Created an attachment (id=58829)
 --> (https://bugs.webkit.org/attachment.cgi?id=58829)
Patch to add a showModalDialog support for DumpRenderTree on Mac.

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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.