Bug 215780 – Move PDF heads-up display to UI process on macOS

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Move PDF heads-up display to UI process on macOS

[Alex Christensen]

Created attachment 407125 [details]
Patch

[Alex Christensen]

Created attachment 407127 [details]
Patch

[Tim Horton]

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!

[Alex Christensen]

Created attachment 407136 [details]
Patch

[Alex Christensen]

Created attachment 407137 [details]
Patch

[Alex Christensen]

Created attachment 407138 [details]
Patch

[Alex Christensen]

Created attachment 407142 [details]
Patch

[Alex Christensen]

Created attachment 407143 [details]
Patch

[Alex Christensen]

I can't figure out why the linux bots can't find the new header.

[Tim Horton]

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

[Alex Christensen]

Created attachment 407150 [details]
Patch

[Alex Christensen]

Created attachment 407154 [details]
Patch

[Alex Christensen]

Created attachment 407157 [details]
Patch

[Alex Christensen]

Created attachment 407162 [details]
Patch

[Alex Christensen]

Created attachment 407188 [details]
Patch

[Peng Liu]

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.

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 407255 [details]
Patch

[Alex Christensen]

Created attachment 407263 [details]
Patch

[Tim Horton]

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.

[Alex Christensen]

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

[Alex Christensen]

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

[Alex Christensen]

Created attachment 407489 [details]
Patch

[EWS Watchlist]

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

[Alex Christensen]

Created attachment 407490 [details]
Patch

[Tim Horton]

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.

[Alex Christensen]

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

[Radar WebKit Bug Importer]

<rdar://problem/68094474>

[EWS]

Committed r266654: <https://trac.webkit.org/changeset/266654>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407490 [details].