Move PDF heads-up display to UI process on macOS
Created attachment 407125 [details] Patch
Created attachment 407127 [details] Patch
Comment on attachment 407127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407127&action=review > Source/WTF/wtf/PlatformHave.h:678 > +#define HAVE_UI_PROCESS_PDF_HUD 1 Don't think this is a HAVE, it's an ENABLE (WebKit feature, not existence of a platform feature). > Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h:136 > + HashMap<WebKit::PDFPluginIdentifier, RetainPtr<WKPDFHUDView>> _pdfHUDViews; This should probably be in WebViewImpl > Source/WebKit/UIProcess/API/mac/WKView.mm:998 > + // Unsupported in WKView. Is this OK? I'm not sure what the remaining WKView clients are. > Source/WebKit/UIProcess/API/mac/WKWebViewMac.mm:459 > + [hud mouseMoved:event]; If the HUD eats the event, we also send it to the page?? That seems wrong. ALSO, all of this code should be in WebViewImpl, WKWebViewMac is quite intentionally a thin wrapper. > Source/WebKit/UIProcess/API/mac/WKWebViewPrivateForTestingMac.h:53 > +- (NSSet<NSView *> *)pdfHUDs; Underscore! > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1725 > + [m_view _createPDFHUD:identifier rect:rect]; This is humorously backwards :) The code should be here. > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:612 > + [m_pdfLayerController setDisplaysPDFHUDController:NO]; Indented wrong :) > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:2440 > + if ([m_pdfLayerController respondsToSelector:@selector(zoomIn:)]) What are these respondsToSelector checks about? Won't things be super broken if the feature flag is on + we don't respond? Do we want to runtime-switch based on availability of these, or something? > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:2453 > + completionHandler(m_suggestedFilename, m_frame.url(), IPC:: DataReference { static_cast<const uint8_t*>(data.bytes), data.length }); What's going on with spacing in `IPC:: DataReference` > Source/WebKit/WebProcess/Plugins/PDF/PDFPluginIdentifier.h:2 > + * Copyright (C) 2019 Apple Inc. All rights reserved. 2020!
Created attachment 407136 [details] Patch
Created attachment 407137 [details] Patch
Created attachment 407138 [details] Patch
Created attachment 407142 [details] Patch
Created attachment 407143 [details] Patch
I can't figure out why the linux bots can't find the new header.
(In reply to Alex Christensen from comment #9) > I can't figure out why the linux bots can't find the new header. I pointed out to Alex elsewhere that this is probably due to the capitalization mismatch in the filename.
Created attachment 407150 [details] Patch
Created attachment 407154 [details] Patch
Created attachment 407157 [details] Patch
Created attachment 407162 [details] Patch
Created attachment 407188 [details] Patch
Comment on attachment 407188 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407188&action=review > Source/WebKit/UIProcess/PDF/WKPDFHUDView.h:31 > +class WebPageProxy; Maybe add PDFPluginIdentifier here as well? > Source/WebKit/UIProcess/PDF/WKPDFHUDView.h:36 > +- (instancetype)initWithFrame:(NSRect)frame pluginIdentifier:(WebKit::PDFPluginIdentifier)pluginIdentifier page:(WebKit::WebPageProxy&)page; Nit. "WebKit::" can be removed here. > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:81 > +static NSColor* PDFCGColorCreateGenericRGB(CGFloat red, CGFloat green, CGFloat blue, CGFloat alpha) Nit. "NSColor *". > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:98 > + RetainPtr<NSMutableDictionary<NSString*, NSImage*>> _cachedIcons; Nit. "NSString *", "NSImage *". > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:100 > + bool _mouseMovedToHUD; Can we use only BOOL or bool here? > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:183 > + [_activeLayer setOpacity: kControlLayerDownAlpha]; Nit. The space here can be removed? > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:239 > + auto* page = _page.get(); Nit. "auto *". > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:255 > + [self _getImageForControlName: controlName]; Nit. Remove the space before controlName? > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:269 > + for (NSImage* image in [_cachedIcons allValues]) Nit. "NSImage *". > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:286 > + NSImage* controlImage = [self _getImageForControlName:controlName]; Ditto. > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:288 > + [controlLayer setOpacity: kControlLayerNormalAlpha]; The extra space can be removed? > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:306 > + [parentLayer addSublayer: _layer.get()]; Ditto. > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:318 > +- (NSImage*)_getImageForControlName:(NSString *)control Nit. "NSImage *". > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:320 > + NSImage* iconImage = _cachedIcons.get()[control]; Ditto. > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:324 > + iconImage = [NSImage _imageWithSystemSymbolName: control]; Nit. The space before "control" can be removed. > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:334 > + NSImageRep* iconImageRep = [iconImage bestRepresentationForRect:iconImageRect context:nil hints:nil]; Nit. "NSImageRep *". > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:335 > + iconImage = [NSImage imageWithImageRep: iconImageRep]; Nit. The space before iconImageRep can be removed.
Comment on attachment 407188 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407188&action=review >> Source/WebKit/UIProcess/PDF/WKPDFHUDView.h:31 >> +class WebPageProxy; > > Maybe add PDFPluginIdentifier here as well? Our strongly typed identifiers can't really be forward declared without duplicating the entire contents of the header. >> Source/WebKit/UIProcess/PDF/WKPDFHUDView.h:36 >> +- (instancetype)initWithFrame:(NSRect)frame pluginIdentifier:(WebKit::PDFPluginIdentifier)pluginIdentifier page:(WebKit::WebPageProxy&)page; > > Nit. "WebKit::" can be removed here. It can't. This is not in a namespace. >> Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:239 >> + auto* page = _page.get(); > > Nit. "auto *". auto* is correct WebKit style here because we aren't pointing to an ObjC type.
Created attachment 407255 [details] Patch
Created attachment 407263 [details] Patch
Comment on attachment 407263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407263&action=review > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:48 > +static const CGFloat kLayerVerticalOffset = 40; I feel like we don't usually k- our constants. > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:81 > +static NSColor *PDFCGColorCreateGenericRGB(CGFloat red, CGFloat green, CGFloat blue, CGFloat alpha) Get rid of this, like I say below > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:113 > + _deviceScaleFactor = page.deviceScaleFactor(); I'm not sure of the lifetime of WKPDFHUDView; if it is long-lived, you'll need to respect deviceScaleFactor changes (dragging window between 1x and 2x monitor). > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:140 > + [CATransaction begin]; > + [CATransaction setDisableActions:YES]; > + CGRect layerBounds = [_layer bounds]; > + [_layer setFrame:CGRectMake(rect.size.width / 2.0 - layerBounds.size.width / 2.0, kLayerVerticalOffset, layerBounds.size.width, layerBounds.size.height)]; > + [CATransaction commit]; Everywhere you do an explicit actions-disabled commit could be removed if you assign WebActionDisablingCALayerDelegate as your layer's delegate. > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:213 > + for (NSUInteger index = 0; index < [_layer sublayers].count; index++) { > + CALayer *subLayer = [_layer sublayers][index]; There's always enumerateObjectsUsingBlock, but it's probably not a win > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:263 > + auto color = adoptNS(PDFCGColorCreateGenericRGB(kLayerGrayComponent, kLayerGrayComponent, kLayerGrayComponent, 1.0)); This PDFCGColorCreateGenericRGB function should go away and this line can be replaced by `cachedCGColor({ kLayerGrayComponent, kLayerGrayComponent, kLayerGrayComponent });` or some such (and then remove the -CGColor on the next line). > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:279 > + if ([controlName isEqualToString: PDFHUDSeperatorControl]) { more colons with spaces after them (there are a bunch) > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:309 > +- (void)_redrawLayer This seems to never be called?? > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:330 > + NSImageAlternateCriterionFont : [NSFont preferredFontForTextStyle:NSFontTextStyleTitle2 options:@{ }], > + NSImageAlternateCriterionSymbolScale : @(NSImageSymbolScaleLarge) What in the world is this. V. surprising, deserves a comment.
(In reply to Tim Horton from comment #20) > > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:309 > > +- (void)_redrawLayer > > This seems to never be called?? This was called in the device scale factor update code, which I'm restoring.
(In reply to Tim Horton from comment #20) > > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:140 > > + [CATransaction begin]; > > + [CATransaction setDisableActions:YES]; > > + CGRect layerBounds = [_layer bounds]; > > + [_layer setFrame:CGRectMake(rect.size.width / 2.0 - layerBounds.size.width / 2.0, kLayerVerticalOffset, layerBounds.size.width, layerBounds.size.height)]; > > + [CATransaction commit]; > > Everywhere you do an explicit actions-disabled commit could be removed if > you assign WebActionDisablingCALayerDelegate as your layer's delegate. There are lots of places we use setDisableActions in WebKit. I'd prefer to leave it as unchanged as possible so we don't have any surprises. > > > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:213 > > + for (NSUInteger index = 0; index < [_layer sublayers].count; index++) { > > + CALayer *subLayer = [_layer sublayers][index]; > > There's always enumerateObjectsUsingBlock, but it's probably not a win This would require returning something from the block indicating to return something, which is too complicated. This is much simpler.
Created attachment 407489 [details] Patch
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Created attachment 407490 [details] Patch
Comment on attachment 407490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407490&action=review > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1727 > + auto hud = adoptNS([[WKPDFHUDView alloc] initWithFrame:rect pluginIdentifier:identifier page:m_page.get()]); My (maybe) last and most annoying piece of feedback: Maybe we should call it something other than HUD :) > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:165 > + if (CGRectContainsPoint([self convertRect:CGRectInset([_layer frame], -16.0, -16.0) toView:nil], NSPointToCGPoint(event.locationInWindow))) { Where do the 16s come from? Can they be a constant? > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:170 > +} Technically you're missing the classic macOS "button de-highlights if you move out of it while the mouse is down" behavior, but I think the old implementation was too? > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:329 > + // This was introduced in rdar://problem/59118412 The comment /still/ doesn't explain why this isn't using the normal symbol API.
(In reply to Tim Horton from comment #26) > Comment on attachment 407490 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=407490&action=review > > > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1727 > > + auto hud = adoptNS([[WKPDFHUDView alloc] initWithFrame:rect pluginIdentifier:identifier page:m_page.get()]); > > My (maybe) last and most annoying piece of feedback: Maybe we should call it > something other than HUD :) Any suggestions? WKPDFButtons? > > Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:329 > > + // This was introduced in rdar://problem/59118412 > > The comment /still/ doesn't explain why this isn't using the normal symbol > API. I don't know why it isn't using the normal symbol API. Maybe we could change it to, but I would like to land this first as it is then consider changing it in the future.
<rdar://problem/68094474>
Committed r266654: <https://trac.webkit.org/changeset/266654> All reviewed patches have been landed. Closing bug and clearing flags on attachment 407490 [details].