[webkit-reviews] review granted: [Bug 191097] Adopt Reveal Framework to replace Lookup : [Attachment 354207] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Nov 8 11:10:38 PST 2018


Tim Horton <thorton at apple.com> has granted Megan Gardner
<megan_gardner at apple.com>'s request for review:
Bug 191097: Adopt Reveal Framework to replace Lookup
https://bugs.webkit.org/show_bug.cgi?id=191097

Attachment 354207: Patch

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




--- Comment #42 from Tim Horton <thorton at apple.com> ---
Comment on attachment 354207
  --> https://bugs.webkit.org/attachment.cgi?id=354207
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-e
ditability-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


More information about the webkit-reviews mailing list