Bug 52855 – WebKit2: Implement showModalDialog

• Component: WebKit2
• Status: RESOLVED FIXED

[Darin Adler]

WebKit2: Implement showModalDialog

[Darin Adler]

Created attachment 79675 [details]
Patch

[Darin Adler]

Comment on attachment 79675 [details]
Patch

New patch forthcoming.

[Darin Adler]

Created attachment 79680 [details]
Patch

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.

[Darin Adler]

(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.

[Darin Adler]

(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.

[Early Warning System Bot]

Attachment 79680 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7534275

[Darin Adler]

Created attachment 79683 [details]
Patch addressing Mitz’s comments

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.

(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.

[Build Bot]

Attachment 79680 [details] did not build on win:
Build output: http://queues.webkit.org/results/7640003

[Eric Seidel (no email)]

(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?

[Alexey Proskuryakov]

*** Bug 51259 has been marked as a duplicate of this bug. ***

[WebKit Review Bot]

Attachment 79680 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7605227

[WebKit Review Bot]

Attachment 79683 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7588231

[Darin Adler]

http://trac.webkit.org/changeset/76361
http://trac.webkit.org/changeset/76362

[Darin Adler]

(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.

[Eric Seidel (no email)]

(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.)