Safari needs this to adopt new createPage API.
<rdar://problem/33174155>
Created attachment 314819 [details] Patch
Comment on attachment 314819 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314819&action=review > Source/WebKit2/UIProcess/API/Cocoa/WKWindowFeaturesPrivate.h:37 > +@property (nonatomic, readonly, getter=_isLocationBarVisible) BOOL _locationBarVisible WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA)); > +@property (nonatomic, readonly, getter=_areScrollbarsVisible) BOOL _scrollbarsVisible WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA)); > +@property (nonatomic, readonly, getter=_isFullscreen) BOOL _fullscreen WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA)); > +@property (nonatomic, readonly, getter=_isDialog) BOOL _dialog WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA)); These should be exposed as NSNumber *s the same way the API bool properties are.
Comment on attachment 314819 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314819&action=review >> Source/WebKit2/UIProcess/API/Cocoa/WKWindowFeaturesPrivate.h:37 >> +@property (nonatomic, readonly, getter=_isDialog) BOOL _dialog WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA)); > > These should be exposed as NSNumber *s the same way the API bool properties are. width, height, x, and y are the only API::WindowFeature properties that are optional. All others are always specified, and the documentation on the public API is incorrect because, for example, menuBarVisibility can never be nil. If I'm right, then I don't think we should bring this mistake into the private API. I can change these to NSNumber * if I'm misinterpreting when values can be unspecified or if you think it's better to keep it consistent regardless.
(In reply to Ansh Shukla from comment #4) > Comment on attachment 314819 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=314819&action=review > > >> Source/WebKit2/UIProcess/API/Cocoa/WKWindowFeaturesPrivate.h:37 > >> +@property (nonatomic, readonly, getter=_isDialog) BOOL _dialog WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA)); > > > > These should be exposed as NSNumber *s the same way the API bool properties are. > > width, height, x, and y are the only API::WindowFeature properties that are > optional. This is *currently* true for the WebCore::WindowFeatures object, but it's not actually true for the web API itself. > All others are always specified This is not true. You can call window.open with *no* window features string. https://html.spec.whatwg.org/multipage/window-object.html#dom-open https://html.spec.whatwg.org/multipage/window-object.html#window-open-steps https://html.spec.whatwg.org/multipage/window-object.html#concept-window-open-features-tokenize Or more casually: https://developer.mozilla.org/en-US/docs/Web/API/Window/open "An optional parameter listing the features (size, position, scrollbars, etc.) of the new window as a DOMString." > The documentation on the public API is incorrect because, for example, menuBarVisibility can never be > nil. The documentation on the public API is correct. Our implementation is wrong. > I can change these to NSNumber * if I'm misinterpreting when > values can be unspecified or if you think it's better to keep it consistent > regardless. Please correctly make the new SPI represent the optionality of these features. I filed https://bugs.webkit.org/show_bug.cgi?id=174271 to fix the implementation.
(In reply to Brady Eidson from comment #5) > > Or more casually: > https://developer.mozilla.org/en-US/docs/Web/API/Window/open > "An optional parameter listing the features (size, position, scrollbars, > etc.) of the new window as a DOMString." I saw this page, but the later statement "the features that are not listed will be disabled or removed (except titlebar and close, which are by default yes)" led me to believe that WebCore::WindowFeatures was intentionally reverting to defaults for unspecified values. Since it seems that our implementation is broken, I'll upload a patch with optional properties.
Created attachment 314874 [details] Patch
(In reply to Ansh Shukla from comment #6) > (In reply to Brady Eidson from comment #5) > > > > Or more casually: > > https://developer.mozilla.org/en-US/docs/Web/API/Window/open > > "An optional parameter listing the features (size, position, scrollbars, > > etc.) of the new window as a DOMString." > > I saw this page, but the later statement > > "the features that are not listed will be disabled or removed (except > titlebar and close, which are by default yes)" > > led me to believe that WebCore::WindowFeatures was intentionally reverting > to defaults for unspecified values. Since it seems that our implementation > is broken, I'll upload a patch with optional properties. The spec has recommended default values. But you're confusing the expectations of the window with what the API represents, which is whether or not the value was specified at all. Some examples: Javascript: window.open("about:blank", "_top", "toolbar=yes"); API WKWindowFeatures.toolbarsVisibility: [NSNumber numberWithBool:YES] Javascript: window.open("about:blank", "_top", "toolbar=no"); API WKWindowFeatures.toolbarsVisibility: [NSNumber numberWithBool:NO] Javascript: window.open("about:blank", "_top"); API WKWindowFeatures.toolbarsVisibility: nil Note: This doesn't actually work today as established - that's what the other bug covers.
Comment on attachment 314874 [details] Patch API test? You can add on to the API test OpenAndCloseWindow.mm
Test unearthed: https://bugs.webkit.org/show_bug.cgi?id=174388
Created attachment 315163 [details] Patch
/Volumes/Data/EWS/WebKit/Tools/TestWebKitAPI/Tests/WebKit2Cocoa/OpenAndCloseWindow.mm:237:91: error: use of undeclared identifier 'NSWindowStyleMaskBorderless' auto window = adoptNS([[NSWindow alloc] initWithContentRect:[webView frame] styleMask:NSWindowStyleMaskBorderless backing:NSBackingStoreBuffered defer:YES]); ^ 1 error generated. Can't use 10.12 SDK features as long as WebKit has to build on 10.11
Created attachment 315181 [details] Patch
Comment on attachment 315181 [details] Patch r=me once EWS is happy
Comment on attachment 315181 [details] Patch Clearing flags on attachment: 315181 Committed r219378: <http://trac.webkit.org/changeset/219378>
All reviewed patches have been landed. Closing bug.
(In reply to Brady Eidson from comment #14) > Comment on attachment 315181 [details] > Patch > > r=me once EWS is happy (In reply to WebKit Commit Bot from comment #15) > Comment on attachment 315181 [details]> > Clearing flags on attachment: 315181 > > Committed r219378: <http://trac.webkit.org/changeset/219378> I notice this landed before EWS was all the way done. While I think this particular patch was fine as-is, it's not usually acceptable to disregard reviewer feedback like this.