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?
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?
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.
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?
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
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
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
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
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?
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
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
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
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
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
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
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
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?
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
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
2018-10-31 15:27 PDT, Megan Gardner
2018-10-31 16:42 PDT, Megan Gardner
2018-11-01 19:30 PDT, Megan Gardner
2018-11-05 14:23 PST, Megan Gardner
2018-11-05 16:47 PST, Megan Gardner
2018-11-05 17:14 PST, Megan Gardner
2018-11-05 17:20 PST, Megan Gardner
2018-11-05 17:38 PST, Megan Gardner
2018-11-05 18:41 PST, EWS Watchlist
2018-11-05 18:46 PST, EWS Watchlist
2018-11-05 19:41 PST, EWS Watchlist
2018-11-06 17:19 PST, Megan Gardner
2018-11-07 12:06 PST, Megan Gardner
2018-11-07 13:59 PST, Megan Gardner
2018-11-07 14:43 PST, EWS Watchlist
2018-11-07 14:52 PST, EWS Watchlist
2018-11-07 15:24 PST, Megan Gardner
2018-11-07 16:28 PST, EWS Watchlist
2018-11-07 16:56 PST, EWS Watchlist
2018-11-07 17:34 PST, Megan Gardner
2018-11-07 18:36 PST, EWS Watchlist
2018-11-07 18:38 PST, EWS Watchlist
2018-11-07 19:52 PST, EWS Watchlist
2018-11-07 21:22 PST, Megan Gardner
2018-11-08 17:21 PST, Megan Gardner