WebKit2: Implement showModalDialog
Created attachment 79675 [details] Patch
Comment on attachment 79675 [details] Patch New patch forthcoming.
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.
(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.
(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.
Attachment 79680 [details] did not build on qt: Build output: http://queues.webkit.org/results/7534275
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.
Attachment 79680 [details] did not build on win: Build output: http://queues.webkit.org/results/7640003
(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?
*** Bug 51259 has been marked as a duplicate of this bug. ***
Attachment 79680 [details] did not build on mac: Build output: http://queues.webkit.org/results/7605227
Attachment 79683 [details] did not build on mac: Build output: http://queues.webkit.org/results/7588231
http://trac.webkit.org/changeset/76361 http://trac.webkit.org/changeset/76362
(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.
(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.)