Bug 52855

Summary: WebKit2: Implement showModalDialog
Product: WebKit Reporter: Darin Adler <darin>
Component: WebKit2Assignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aroben, buildbot, eric, mitz, mjs, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
mitz: review-
Patch addressing Mitz’s comments mitz: review+

Description Darin Adler 2011-01-20 17:05:54 PST
WebKit2: Implement showModalDialog
Comment 1 Darin Adler 2011-01-20 18:19:32 PST
Created attachment 79675 [details]
Patch
Comment 2 Darin Adler 2011-01-20 18:55:16 PST
Comment on attachment 79675 [details]
Patch

New patch forthcoming.
Comment 3 Darin Adler 2011-01-20 19:06:03 PST
Created attachment 79680 [details]
Patch
Comment 4 mitz 2011-01-20 19:15:45 PST
Comment on attachment 79680 [details]
Patch

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

Looks good but missing a piece.

> Source/WebKit2/ChangeLog:47
> +        * WebProcess/WebPage/WebPage.h: Defined them canRunModal function

Typo: them

> Source/WebKit2/ChangeLog:48
> +        and declared these runModal function.

Another typo(?): these

> Source/WebKit2/UIProcess/API/C/WKPage.h:173
> -    const void *                                                        clientInfo;
> +    const void*                                                         clientInfo;

I believe “void *” is the CF API convention.

> Tools/ChangeLog:19
> +        * WebKitTestRunner/TestController.cpp:
> +        (WTR::TestController::runModal): Added. Calls through to the
> +        platform-specific version of runModal.
> +        (WTR::TestController::createOtherPage): Changed to be a private
> +        static member function so it can refer to runModal, which is
> +        a private static member function.
> +        (WTR::TestController::initialize): Pass 0 for the runModal
> +        function since we don't need to run the main window modal.
> +        I suspect this is wrong and will need to change.

These changes aren’t included in the patch.
Comment 5 Darin Adler 2011-01-20 19:20:25 PST
(In reply to comment #4)
> These changes aren’t included in the patch.

This must be due to a bug in webkit-patch upload, since I used that to upload the patch. Something like this happened to me another time recently, and resulted in a broken build.
Comment 6 Darin Adler 2011-01-20 19:21:29 PST
(In reply to comment #4)
> > Source/WebKit2/UIProcess/API/C/WKPage.h:173
> > -    const void *                                                        clientInfo;
> > +    const void*                                                         clientInfo;
> 
> I believe “void *” is the CF API convention.

Sam told me my change is OK.
Comment 7 Early Warning System Bot 2011-01-20 19:22:02 PST
Attachment 79680 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7534275
Comment 8 Darin Adler 2011-01-20 19:26:31 PST
Created attachment 79683 [details]
Patch addressing Mitz’s comments
Comment 9 mitz 2011-01-20 19:29:29 PST
Comment on attachment 79683 [details]
Patch addressing Mitz’s comments

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

> Source/WebKit2/UIProcess/API/C/WKPage.h:173
> -    const void *                                                        clientInfo;
> +    const void*                                                         clientInfo;

This change is inconsistent with other client interfaces in WebKit2.
Comment 10 mitz 2011-01-20 19:30:42 PST
(In reply to comment #6)
> (In reply to comment #4)
> > > Source/WebKit2/UIProcess/API/C/WKPage.h:173
> > > -    const void *                                                        clientInfo;
> > > +    const void*                                                         clientInfo;
> > 
> > I believe “void *” is the CF API convention.
> 
> Sam told me my change is OK.

Didn’t see this comment. I think if we want to change this, we should change all interfaces at once and not just one of them as part of an unrelated patch.
Comment 11 Build Bot 2011-01-20 19:34:46 PST
Attachment 79680 [details] did not build on win:
Build output: http://queues.webkit.org/results/7640003
Comment 12 Eric Seidel (no email) 2011-01-20 19:37:59 PST
(In reply to comment #5)
> (In reply to comment #4)
> > These changes aren’t included in the patch.
> 
> This must be due to a bug in webkit-patch upload, since I used that to upload the patch. Something like this happened to me another time recently, and resulted in a broken build.

I assume you use SVN?

Could this be related to the recent "respect the current directory" support Maciej added?  How often have you seen these sorts of mis-uploads?  Do you happen to remember if the changes appeared in the pretty diff it showed you before upload?
Comment 13 Alexey Proskuryakov 2011-01-20 20:03:27 PST
*** Bug 51259 has been marked as a duplicate of this bug. ***
Comment 14 WebKit Review Bot 2011-01-20 20:08:34 PST
Attachment 79680 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7605227
Comment 15 WebKit Review Bot 2011-01-20 21:04:16 PST
Attachment 79683 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7588231
Comment 17 Darin Adler 2011-01-21 12:00:26 PST
(In reply to comment #12)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > These changes aren’t included in the patch.
> > 
> > This must be due to a bug in webkit-patch upload, since I used that to upload the patch. Something like this happened to me another time recently, and resulted in a broken build.
> 
> I assume you use SVN?

Yes.

> Could this be related to the recent "respect the current directory" support Maciej added?

Seems unlikely. The previous time it happened was before Maciej’s change, and my current directory was the top level.

> How often have you seen these sorts of mis-uploads?

Twice.

> Do you happen to remember if the changes appeared in the pretty diff it showed you before upload?

I don’t remember, sorry.
Comment 18 Eric Seidel (no email) 2011-01-21 12:04:41 PST
(In reply to comment #17)
> > How often have you seen these sorts of mis-uploads?
> 
> Twice.

Thank you for the info.  We'll just have to keep our eyes open.

The only bug like this that I know of is webkit-patch upload fails to include rename information when working from Git checkouts.  This is the first I've heard of missing diff information from svn checkouts.  webkit-patch just uses "svn-create-patch" under the covers:

http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/checkout/scm.py#L479

However, these days it does use an explicit list of changed files (which was gathered at an earlier part of the command).  So it's possible it won't pick up modifications to files done while it's stopped showing you the diff.  (I'm not sure of that, but Adam would know.)