Make WKMenuItemIdentifiersPrivate.h private header

• Component: WebKit2
• Status: RESOLVED FIXED

[Aakash Jain]

WebKit2/UIProcess/API/Cocoa/WKMenuItemIdentifiersPrivate.h is a project header, it contains various NSStrings which are exported in the binary. This header should be private so that symbols from headers matches with binary exactly.

[Aakash Jain]

Created attachment 302964 [details]
Proposed patch

[Tim Horton]

If nobody is depending on them should we unexport or maybe delete them?

[Aakash Jain]

Created attachment 302967 [details]
Updated patch

Yeah. Un-exporting them. They seems to be used within WebKit2, so can't delete them.

[Tim Horton]

Wait, is there other SPI that returns these? Your first patch might have been better, sorry.

[Alexey Proskuryakov]

If you look at the patch that added these, it's fairly complex. I'm not sure if it can be rolled back, but it seem preferable to roll back if it can be. The usage in WebKit may still be API related.

Also, that patch first landed with a private header, broke things, and got re-landed with a project one. Need to look into was broke.

[Alexey Proskuryakov]

I now have the context for why these constants were added. They are needed for clients to identify menu items in these WebKit delegate methods:

@protocol WKUIDelegatePrivate <WKUIDelegate>
...
- (NSMenu *)_webView:(WKWebView *)webView contextMenu:(NSMenu *)menu forElement:(_WKContextMenuElementInfo *)element WK_API_AVAILABLE(macosx(10.12));
- (NSMenu *)_webView:(WKWebView *)webView contextMenu:(NSMenu *)menu forElement:(_WKContextMenuElementInfo *)element userInfo:(id <NSSecureCoding>)userInfo WK_API_AVAILABLE(macosx(10.12));

So it seems best to make the header private to complement that, even though the constants are still not used by anyone.

[Aakash Jain]

Created attachment 303224 [details]
Previous patch again

[Alexey Proskuryakov]

Comment on attachment 302964 [details]
Proposed patch

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

> Source/WebKit2/ChangeLog:3
> +        Make WKMenuItemIdentifiersPrivate.h private header

This initial patch looks good, but we should also clean up another change from bug 151272.

This change is definitely not needed when using 10.12 SDK. I do not think that it is needed with 10.11 SDK either, so let's see if it can be undone.

Index: /trunk/Source/WebCore/platform/spi/mac/NSMenuSPI.h
===================================================================
--- /trunk/Source/WebCore/platform/spi/mac/NSMenuSPI.h	(revision 192480)
+++ /trunk/Source/WebCore/platform/spi/mac/NSMenuSPI.h	(revision 192481)
@@ -46,3 +46,6 @@
 @end
 
+@interface NSMenuItem () <NSUserInterfaceItemIdentification>
+@end
+
 #endif

[Alexey Proskuryakov]

Thinking about it a bit more, I think that it is needed with 10.11 - that's where we get setIdentifier. So let's make it conditional on __MAC_OS_X_VERSION_MAX_ALLOWED.

[Aakash Jain]

Created attachment 303372 [details]
Updated patch

[Tim Horton]

Comment on attachment 303372 [details]
Updated patch

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

> Source/WebCore/ChangeLog:8
> +        * platform/spi/mac/NSMenuSPI.h: NSMenuItem is not needed with 10.12 or greater SDK.

What? Why not? *ImmediateActionController use these, at this point I think on all platforms?

[Build Bot]

Comment on attachment 303372 [details]
Updated patch

Attachment 303372 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3235244

New failing tests:
media/modern-media-controls/media-documents/click-on-video-should-not-pause.html

[Build Bot]

Created attachment 303383 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Alexey Proskuryakov]

> What? Why not? 

The fact that NSMenuItem implements NSUserInterfaceItemIdentification is now in the public header.

[Tim Horton]

(In reply to comment #14)
> > What? Why not? 
> 
> The fact that NSMenuItem implements NSUserInterfaceItemIdentification is now
> in the public header.

Ok, but the patch also removed some SPI declarations...

Comment on attachment 303372 [details]
Updated patch

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

> Source/WebCore/platform/spi/mac/NSMenuSPI.h:47
> +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MAX_ALLOWED < 101200

The entire header is guarded with #if PLATFORM(MAC) so this is redundant.

[Alexey Proskuryakov]

> Ok, but the patch also removed some SPI declarations...

Can you elaborate?

The patch looks right to me, with the exception of the redundancy that Dan pointed out.

[Tim Horton]

(In reply to comment #17)
> > Ok, but the patch also removed some SPI declarations...
> 
> Can you elaborate?
> 
> The patch looks right to me, with the exception of the redundancy that Dan
> pointed out.

There are things inside the category. Those things are still SPI. They are still used in current versions of macOS. They cannot be #if'd out. You can #if out the conformance if you want, but you'll have to move it to its own @interface.

[Alexey Proskuryakov]

Indeed, selective blindness on my part. Probably caused by the fact that the patch built (of course it did in EWS).

[Aakash Jain]

Created attachment 303549 [details]
Updated patch

[Build Bot]

Comment on attachment 303549 [details]
Updated patch

Attachment 303549 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3254381

New failing tests:
media/modern-media-controls/tracks-support/tracks-support-show-panel-after-dragging-controls.html

[Build Bot]

Created attachment 303560 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[WebKit Commit Bot]

Comment on attachment 303549 [details]
Updated patch

Clearing flags on attachment: 303549

Committed r213482: <http://trac.webkit.org/changeset/213482>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.