Bug 191097 – Adopt Reveal Framework to replace Lookup

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Adopt Reveal Framework to replace Lookup

[Megan Gardner]

Created attachment 353541 [details]
Patch

[Tim Horton]

Comment on attachment 353541 [details]
Patch

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

> Source/WebCore/editing/mac/DictionaryLookup.mm:48
> +#import <pal/spi/mac/RevealSPI.h>

You're missing a bunch of files.

> Source/WebCore/editing/mac/DictionaryLookup.mm:69
> +    _highlightRect = hightlightRect;

"hightlight"

> Source/WebCore/editing/mac/DictionaryLookup.mm:92
> +        NSMutableDictionary* attributes = [[NSMutableDictionary alloc] initWithDictionary:[self.attributedString fontAttributesInRange:NSMakeRange(0, [self.attributedString length])]];

This is a leak.

> Source/WebCore/editing/mac/DictionaryLookup.mm:93
> +        NSMutableParagraphStyle* paragraph = [[NSMutableParagraphStyle alloc] init];

This is a leak.

> Source/WebCore/editing/mac/DictionaryLookup.mm:100
> +        for (int attempt = 0; attempt < 50 && [font pointSize] > 0 && [[self.attributedString string] sizeWithAttributes:attributes].width >= rect.size.width; attempt++) {

I realize you didn't write this, but this is truly insane, text layout up to 50 times in a loop. Do we still need to scale to fit now that we adjust the rect to be based on the size of the string?

> Source/WebCore/editing/mac/DictionaryLookup.mm:102
> +            font = [NSFont fontWithName: [font fontName] size: ( [font pointSize] - 1.0 )];

Lots of style problems here; no space after a colon, or inside parens. And lots of stars are on the wrong side.

> Source/WebCore/editing/mac/DictionaryLookup.mm:103
> +            if (font == nil)

And this should just be if (!font)

> Source/WebCore/editing/mac/DictionaryLookup.mm:108
> +        NSAttributedString* string = [[NSAttributedString alloc] initWithString: [self.attributedString string] attributes: attributes];

This is a leak.

> Source/WebCore/editing/mac/DictionaryLookup.mm:125
> +    [[NSNotificationCenter defaultCenter] postNotificationName:@"WKRevealPopoverWillClose" object:self];

Notifications are a pretty bad smell. Can you pass a reference to some object into the constructor that you call a function on instead?

> Source/WebCore/editing/mac/DictionaryLookup.mm:190
> +    if (selection.selectionType() == VisibleSelection::RangeSelection) {

How well do we handle "Reveal.framework is not available" (i.e. the BaseSystem case)? This has been a source of trouble in the past

> Source/WebCore/editing/mac/DictionaryLookup.mm:-210
> -    if (!getLULookupDefinitionModuleClass())
> -        return nil;

See: one remnant of the mechanism that avoided crashes in the base system.

> Source/WebCore/editing/mac/DictionaryLookup.mm:335
> +    WebRevealHighlight * webHighlight =  [[WebRevealHighlight alloc] initWithHighlightRect:textIndicator->selectionRectInRootViewCoordinates() useDefaultHighlight:!textIndicator.get().contentImage() attributedString:dictionaryPopupInfo.attributedString.get()];

This is a leak and also the star is in the wrong place and also there's two spaces where there should be one and also...

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:230
> +#if ENABLE(REVEAL)

As above, let's find a non-notification way to do this.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:396
> +- (void)_revealPopoverWillClose:(NSNotification *)notification

Seems oddly unnecessary to have both of these

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10127
> +			shellScript = "if [[ ${WK_PLATFORM_NAME} != \"iphoneos\" ]]; then\n    exit 0;\nfi\n\nif [[ ! -d \"${INSTALL_ROOT}/${SYSTEM_LIBRARY_DIR}/PrivateFrameworks\" ]]; then\n    mkdir -p \"${INSTALL_ROOT}/${SYSTEM_LIBRARY_DIR}/PrivateFrameworks\"\nfi\n\nln -s -h -f ../Frameworks/WebKit.framework \"${INSTALL_ROOT}/${SYSTEM_LIBRARY_DIR}/PrivateFrameworks/WebKit.framework\"\n";

😳

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:-526
> -    // Based on TextIndicator implementation:
> -    // TODO(144307): Come up with a better way to share this information than duplicating these values.
> -    CGFloat verticalMargin = 2.5;
> -    CGFloat horizontalMargin = 0.5;

Are the margins still applied?

[Sam Weinig]

Comment on attachment 353541 [details]
Patch

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

> Source/WebCore/editing/mac/DictionaryLookup.mm:58
> +@property (nonatomic, readonly) NSAttributedString* attributedString;

* on the wrong side.

> Source/WebCore/editing/mac/DictionaryLookup.mm:76
> +- (NSArray <NSValue *> *)revealContext:(RVPresentingContext *)context rectsForItem:(RVItem *)item

I believe our style does not have a space between NSArray and the <.

> Source/WebKit/UIProcess/mac/WKImmediateActionController.h:50
> +@class RVItem;

Why is this needed?

> Source/WebKitLegacy/mac/WebView/WebView.mm:9615
> +#if ENABLE(REVEAL)
> +- (void)_revealPopoverWillClose:(NSNotification *)notification
> +{
> +    [self _clearTextIndicatorWithAnimation:TextIndicatorWindowDismissalAnimation::FadeOut];
> +}
> +#else
>  - (void)_dictionaryLookupPopoverWillClose:(NSNotification *)notification
>  {
>      [self _clearTextIndicatorWithAnimation:TextIndicatorWindowDismissalAnimation::FadeOut];
>  }
> +#endif // !ENABLE(REVEAL)

Since these do the same exact thing, can we just have one, more generally named, method?

[Megan Gardner]

Created attachment 353555 [details]
Patch

[EWS Watchlist]

Attachment 353555 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/mac/DictionaryLookup.mm:100:  Missing space around : in range-based for statement  [whitespace/colon] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Megan Gardner]

Created attachment 353676 [details]
Patch

[Tim Horton]

Comment on attachment 353676 [details]
Patch

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

> Source/WebCore/PAL/pal/spi/mac/RevealSPI.h:2
> + * Copyright (C) 2014 Apple Inc.  All rights reserved.

Do you have a time machine!

> Source/WebCore/PAL/pal/spi/mac/RevealSPI.h:32
> +SOFT_LINK_CLASS(Reveal, RVPresenter)

Pretty sure some of these need _OPTIONAL or you'll break the base system.

> Source/WebCore/PAL/pal/spi/mac/RevealSPI.h:37
> +

Where'd all this white space come from?

> Source/WebCore/PAL/pal/spi/mac/RevealSPI.h:49
> +@interface RVItem : NSObject < NSSecureCoding >

No spaces *around* the protocol conformance list, just before it:

NSObject <NSSecureCoding>

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:7602
> +		442956CA218A6D300080DB54 /* DictionaryLookupLegacy.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = DictionaryLookupLegacy.mm; sourceTree = "<group>"; };

Slightly concerning that I don't see a change to SourcesCocoa.txt

> Source/WebCore/editing/mac/DictionaryLookup.mm:73
> +    _highlightRect = hightlightRect;

Still hightlight

> Source/WebCore/editing/mac/DictionaryLookup.mm:90
> +    return @[[NSValue valueWithRect:self.highlightRect]];

Does just @(self.highlightRect) work? I can never remember.

> Source/WebCore/editing/mac/DictionaryLookup.mm:96
> +    UNUSED_PARAM(context);
> +    UNUSED_PARAM(item);

These seem like lies. Or at least the context one is.

> Source/WebCore/editing/mac/DictionaryLookup.mm:98
> +    for (NSValue* rectVal in [context itemRectsInView]) {

Star's on the wrong side.

> Source/WebCore/editing/mac/DictionaryLookup.mm:107
> +        RetainPtr<NSAttributedString> string = adoptNS([[NSAttributedString alloc] initWithString: [self.attributedString string] attributes:attributes.get()]);

Still some weird style here with the spaces around colons

> Source/WebCore/editing/mac/DictionaryLookup.mm:315
> +    NSRect hightlightRect;

hightlight!

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10123
> +			shellScript = "if [[ ${WK_PLATFORM_NAME} != \"iphoneos\" ]]; then\n    exit 0;\nfi\n\nif [[ ! -d \"${INSTALL_ROOT}/${SYSTEM_LIBRARY_DIR}/PrivateFrameworks\" ]]; then\n    mkdir -p \"${INSTALL_ROOT}/${SYSTEM_LIBRARY_DIR}/PrivateFrameworks\"\nfi\n\nln -s -h -f ../Frameworks/WebKit.framework \"${INSTALL_ROOT}/${SYSTEM_LIBRARY_DIR}/PrivateFrameworks/WebKit.framework\"\n";

Still not sure why this change

> Source/WebKitLegacy/mac/WebView/WebView.mm:361
> +#if !ENABLE(REVEAL)

Probably should be PLATFORM(MAC) && !ENABLE(REVEAL), no?

[Wenson Hsieh]

Comment on attachment 353676 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        Rather than try and pipe everything through directly, we just send our own notification at

Nit - probably want to update this message to reflect the new approach!

> Source/WebCore/PAL/pal/spi/mac/RevealSPI.h:54
> ++ (NSRange)revealRangeAtIndex:(NSUInteger)clickIndex selectedRanges:(NSArray <NSValue *>*)selectedRanges shouldUpdateSelection:(BOOL *)shouldUpdateSelection;

Nit - space after <NSValue *>

> Source/WebCore/editing/mac/DictionaryLookup.mm:58
> +@property (nonatomic, readonly) NSAttributedString* attributedString;

Nit - space after NSAttributedString

[Megan Gardner]

Created attachment 353908 [details]
Patch

[Megan Gardner]

Created attachment 353924 [details]
Patch

[Megan Gardner]

Created attachment 353930 [details]
Patch

[Megan Gardner]

Created attachment 353933 [details]
Patch

[Megan Gardner]

Created attachment 353934 [details]
Patch

[EWS Watchlist]

Comment on attachment 353934 [details]
Patch

Attachment 353934 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9873136

New failing tests:
editing/mac/selection/context-menu-select-editability.html
editing/selection/context-menu-text-selection-lookup.html
editing/mac/dictionary-lookup/dictionary-lookup-input.html
editing/mac/dictionary-lookup/dictionary-lookup-outside-selection.html
editing/mac/dictionary-lookup/dictionary-lookup.html
editing/mac/dictionary-lookup/dictionary-lookup-rtl.html

[EWS Watchlist]

Created attachment 353937 [details]
Archive of layout-test-results from ews100 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 353934 [details]
Patch

Attachment 353934 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9873122

New failing tests:
editing/mac/selection/context-menu-select-editability.html
editing/selection/context-menu-text-selection-lookup.html
editing/mac/dictionary-lookup/dictionary-lookup-input.html
editing/mac/dictionary-lookup/dictionary-lookup-outside-selection.html
editing/mac/dictionary-lookup/dictionary-lookup.html
editing/mac/dictionary-lookup/dictionary-lookup-rtl.html

[EWS Watchlist]

Created attachment 353939 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 353934 [details]
Patch

Attachment 353934 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9873311

New failing tests:
editing/mac/selection/context-menu-select-editability.html
editing/selection/context-menu-text-selection-lookup.html
editing/mac/dictionary-lookup/dictionary-lookup-input.html
editing/mac/dictionary-lookup/dictionary-lookup-outside-selection.html
editing/mac/dictionary-lookup/dictionary-lookup.html
editing/mac/dictionary-lookup/dictionary-lookup-rtl.html

[EWS Watchlist]

Created attachment 353943 [details]
Archive of layout-test-results from ews115 for mac-sierra

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

[Wenson Hsieh]

Comment on attachment 353934 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        continue to work. Eventually we will remove this code and also likly switch

Nit - likly

> Source/WebKitLegacy/mac/ChangeLog:8
> +        Passin what it needed to clear the textIndicators. This is better than

Nit - "Pass in what is needed"?

> Source/WebCore/editing/mac/DictionaryLookup.mm:97
> +    for (NSValue* rectVal in [context itemRectsInView]) {

Nit - * on the other side.

> Source/WebCore/editing/mac/DictionaryLookup.mm:331
> +        hightlightRect = selectionBoundingRectInViewCoordinates;

Nit - hightlightRect!

> Source/WebCore/editing/mac/DictionaryLookup.mm:339
> +        textBaselineOrigin = [view.window convertRectToScreen:NSMakeRect(textBaselineOrigin.x, textBaselineOrigin.y, 0, 0)].origin;

Can we use -convertPointToScreen: here instead of converting an NSRect and grabbing just the origin?

[Megan Gardner]

Created attachment 354035 [details]
Patch

[Megan Gardner]

Created attachment 354124 [details]
Patch

[Megan Gardner]

Created attachment 354145 [details]
Patch

[EWS Watchlist]

Comment on attachment 354145 [details]
Patch

Attachment 354145 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9898813

New failing tests:
editing/mac/selection/context-menu-select-editability.html

[EWS Watchlist]

Created attachment 354157 [details]
Archive of layout-test-results from ews100 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 354145 [details]
Patch

Attachment 354145 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9898864

New failing tests:
editing/mac/selection/context-menu-select-editability.html

[EWS Watchlist]

Created attachment 354160 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Megan Gardner]

Created attachment 354167 [details]
Patch

[EWS Watchlist]

Comment on attachment 354167 [details]
Patch

Attachment 354167 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9900842

New failing tests:
editing/mac/selection/context-menu-select-editability.html

[EWS Watchlist]

Created attachment 354177 [details]
Archive of layout-test-results from ews100 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 354167 [details]
Patch

Attachment 354167 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9901147

New failing tests:
editing/mac/selection/context-menu-select-editability.html

[EWS Watchlist]

Created attachment 354182 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Megan Gardner]

Created attachment 354189 [details]
Patch

[EWS Watchlist]

Comment on attachment 354189 [details]
Patch

Attachment 354189 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9902561

New failing tests:
editing/mac/selection/context-menu-select-editability.html

[EWS Watchlist]

Created attachment 354193 [details]
Archive of layout-test-results from ews102 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 354189 [details]
Patch

Attachment 354189 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9902515

New failing tests:
editing/mac/selection/context-menu-select-editability.html

[EWS Watchlist]

Created attachment 354194 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 354189 [details]
Patch

Attachment 354189 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9902766

New failing tests:
editing/mac/selection/context-menu-select-editability.html

[EWS Watchlist]

Created attachment 354202 [details]
Archive of layout-test-results from ews117 for mac-sierra

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

[Wenson Hsieh]

Comment on attachment 354189 [details]
Patch

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

> Source/WebCore/editing/mac/DictionaryLookup.mm:346
> +    RetainPtr<RVPresentingContext> context = adoptNS([allocRVPresentingContextInstance() initWithPointerLocationInView: pointerLocation inView:view highlightDelegate:(id<RVPresenterHighlightDelegate>) webHighlight.get()]);

Hm..it looks like `pointerLocation` is expected to be in `view`'s coordinate space when we create the `RVPresentingContext` here, but in the case where `!textIndicator.get().contentImage()`, we set it to screen coordinates?

[Megan Gardner]

Created attachment 354207 [details]
Patch

[Tim Horton]

Comment on attachment 354207 [details]
Patch

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

> Source/WebCore/ChangeLog:5
> +

Radar!

> Source/WebCore/ChangeLog:10
> +        The Reveal framework does the same job as Lookup and DataDectors.

DataDectors!

> Source/WebCore/ChangeLog:18
> +

Extra space

> Source/WebCore/ChangeLog:23
> +        (-[WebRevealHighlight initWithHighlightRect:useDefaultHighlight:attributedString:]):

Should probably say some words in amongst the method names since I know there were some tricky bits

> Source/WebCore/editing/mac/DictionaryLookup.mm:76
> +    _clearTextIndicator = nullptr;

No need for this, ObjC fields are initialized to null by default.

> Source/WebCore/editing/mac/DictionaryLookup.mm:98
> +    for (NSValue* rectVal in [context itemRectsInView]) {
> +        NSRect rect = [rectVal rectValue];

Stars on the wrong side, more dot notation instead of [], etc.

> Source/WebCore/editing/mac/DictionaryLookup.mm:103
> +        RetainPtr<NSMutableParagraphStyle> paragraph = adoptNS([[NSMutableParagraphStyle alloc] init]);
> +        [paragraph setAlignment:NSTextAlignmentCenter];

Can we use one NSMutableParagraphStyle, constructed outside of the loop, for all of these?

Also, do we really need to center it now that we get the rect *from* the attributed string?

> Source/WebCore/editing/mac/DictionaryLookup.mm:106
> +        RetainPtr<NSAttributedString> string = adoptNS([[NSAttributedString alloc] initWithString:[self.attributedString string] attributes:attributes.get()]);

Aren't you dropping other attributes on the floor here? I think this should make a mutable copy of the attributed string and add some attributes to the relevant range (if we still need this), not bounce through plaintext

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:-10123
> -			shellScript = "if [[ ${WK_PLATFORM_NAME} != \"iphoneos\" ]]; then\n    exit 0;\nfi\n\nif [[ ! -d \"${INSTALL_ROOT}/${SYSTEM_LIBRARY_DIR}/PrivateFrameworks\" ]]; then\n    mkdir -p \"${INSTALL_ROOT}/${SYSTEM_LIBRARY_DIR}/PrivateFrameworks\"\nfi\n\nln -s -h -f ../Frameworks/WebKit.framework \"${INSTALL_ROOT}/${SYSTEM_LIBRARY_DIR}/PrivateFrameworks/WebKit.framework\"";

Still not sure what you did here.

> LayoutTests/platform/mac-highsierra/editing/mac/selection/context-menu-select-editability-expected.txt:1
> +This test checks that conext menu selection allows whitespace for non-editable fields. To test manually, right click on the blank text in the input box.

Where's the layout tests changelog? this needs some justification

[Radar WebKit Bug Importer]

<rdar://problem/45924508>

[Megan Gardner]

Created attachment 354295 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 354295 [details]
Patch for landing

Clearing flags on attachment: 354295

Committed r238014: <https://trac.webkit.org/changeset/238014>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

editing/mac/selection/context-menu-select-editability.html is failing on Mojave after this change:

 PASS getSelectionForId('inputWhitespace') is ""
-PASS getSelectionForId('readOnlyWhitespace') is ""
+FAIL getSelectionForId('readOnlyWhitespace') should be . Was New York, New York.
 PASS successfullyParsed is true
+Some tests failed.
 
 TEST COMPLETE

https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/r238031%20(727)/results.html

[Ryan Haddad]

(In reply to Ryan Haddad from comment #47)
> editing/mac/selection/context-menu-select-editability.html is failing on
> Mojave after this change:
This was addressed by https://trac.webkit.org/changeset/238060/webkit