Bug 193408 – Add Reveal support in iOSMac

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

Add Reveal support in iOSMac

[Megan Gardner]

Created attachment 359079 [details]
Patch

[Tim Horton]

Comment on attachment 359079 [details]
Patch

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

> Source/WebCore/Configurations/WebCore.xcconfig:132
> +OTHER_LDFLAGS = $(inherited) $(WK_RELOCATABLE_FRAMEWORK_LDFLAGS) $(WK_UNDEFINED_SYMBOLS_LDFLAGS) -lsqlite3 -lobjc -lANGLE -allowable_client WebCoreTestSupport -allowable_client WebKitLegacy -force_load $(BUILT_PRODUCTS_DIR)/libPAL.a -framework CFNetwork -framework CoreAudio -framework CoreGraphics -framework CoreText -framework Foundation -framework ImageIO -framework Metal $(OTHER_LDFLAGS_PLATFORM_$(WK_COCOA_TOUCH)) $(OTHER_LDFLAGS_PLATFORM_$(WK_PLATFORM_NAME)) $(WK_APPKIT_LDFLAGS) $(WK_APPSUPPORT_LDFLAGS) $(WK_AUDIO_UNIT_LDFLAGS) $(WK_CARBON_LDFLAGS) $(WK_CORE_UI_LDFLAGS) $(WK_DATA_DETECTORS_CORE_LDFLAGS) $(WK_GRAPHICS_SERVICES_LDFLAGS) $(WK_IOSURFACE_LDFLAGS) $(WK_LIBWEBRTC_LDFLAGS) $(WK_MOBILE_CORE_SERVICES_LDFLAGS) $(WK_MOBILE_GESTALT_LDFLAGS) $(WK_OPENGL_LDFLAGS) $(WK_SYSTEM_CONFIGURATION_LDFLAGS) $(WK_SYSTEM_PREVIEW_LDFLAGS) $(WK_URL_FORMATTING_LDFLAGS) $(WK_UIKITMAC_LDFLAGS);

Sort

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:194
> +- (void)startHighlightingItem:(RVItem *)item

missing newlines between all of these. Also, are they optional? Maybe we don't have to implement the ones that don't do anything?

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:223
> +- (void)drawHighlightContentForItem:(RVItem *)item context:(CGContextRef)context

This needs a lot of style cleanup. Also don't hardcode the .77

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:505
> +    UNUSED_PARAM(createAnimationController); // remove once we create this correctly

No need for the comment

[Megan Gardner]

Can you clarify what of the style is not up to par? It passes the webkit style bot.

All of those selectors are required. They might be optional in the future, but today, they are required.

Do we have somewhere that 0.77 is defined, and if not, is defining it at the top of the file enough, or do we need it somewhere else that's more universal, and if so, where is that?

[Wenson Hsieh]

Comment on attachment 359079 [details]
Patch

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

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:160
> +@property (nonatomic, readonly) UIView* view;

Nit - UIView *

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:162
> +- (instancetype)initWithHighlightRect:(NSRect)highlightRect view:(UIView*)view;

Nit - UIView *

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:172
> +- (instancetype)initWithHighlightRect:(NSRect)highlightRect view:(UIView*)view

Nit - UIView*

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

Nit - these look quite used!

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:228
> +    NSArray <NSValue *> * rects = [self highlightRectsForItem:item];

Nit - no space between * and rects.

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:232
> +    UIView * textContentView = self.view;

Nit - no space after the *

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:240
> +    CGAffineTransform ct = CGContextGetCTM(context);

Nit - WebKit style prefers full words over single letters/abbreviations in variable names.

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:241
> +    CGAffineTransform t = CGAffineTransformIdentity;

(Here too)

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:247
> +    for (NSValue * v in rects) {

Nit - No space between * and v

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:249
> +        RetainPtr<CGImageRef> imgRef = _image->nativeImage();

Nit - imageRef (or just nativeImage) instead of imgRef

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:250
> +        CGRect origin = CGRectMake(backingScale*(imgSrcRect.origin.x - highlightRect.origin.x), backingScale*(imgSrcRect.origin.y - highlightRect.origin.y),  backingScale*(highlightRect.size.width), backingScale*(highlightRect.size.height));

Nit - spaces around the infix operators!

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:503
> +    UNUSED_PARAM(view);
> +    UNUSED_PARAM(textIndicatorInstallationCallback);
> +    UNUSED_PARAM(rootViewToViewConversionCallback);
> +    UNUSED_PARAM(clearTextIndicator);

Nit - at least the view looks used.

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:509
> +    RetainPtr<WebRevealHighlight> webHighlight =  adoptNS([[WebRevealHighlight alloc] initWithHighlightRect:[view convertRect:textIndicator->selectionRectInRootViewCoordinates() toView:nil] view:view]);

Nit - double space after =

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:514
> +    [UINSSharedRevealController() revealItem:item.get() locationInWindow:dictionaryPopupInfo.origin window:view.window highlighter: (id<UIRVPresenterHighlightDelegate>) webHighlight.get()];

Nit - extra space before (id<

> Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:140
> +    

Nit - stray whitespace change?

> Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:179
> +    dictionaryPopupInfo.attributedString = [[NSMutableAttributedString alloc] initWithString:range.text()];

Does this leak? It's not clear to me when this is released (aside from RetainPtr).

[Wenson Hsieh]

Comment on attachment 359079 [details]
Patch

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

>> Source/WebCore/editing/cocoa/DictionaryLookup.mm:172
>> +- (instancetype)initWithHighlightRect:(NSRect)highlightRect view:(UIView*)view
> 
> Nit - UIView*

Rather, UIView *

[Megan Gardner]

Created attachment 359093 [details]
Patch

[Megan Gardner]

Created attachment 359102 [details]
Patch

[Megan Gardner]

Created attachment 359103 [details]
Patch

[Tim Horton]

(In reply to Megan Gardner from comment #3)
> Can you clarify what of the style is not up to par? It passes the webkit
> style bot.

I think Wenson did this for me :) The stylebot covers a tiny minority of style rules. https://webkit.org/code-style-guidelines/ is a good start for others, and for more you probably just want to take an average file and follow that.

> All of those selectors are required. They might be optional in the future,
> but today, they are required.

OK!

> Do we have somewhere that 0.77 is defined, and if not, is defining it at the
> top of the file enough, or do we need it somewhere else that's more
> universal, and if so, where is that?

We do, see the code that you borrowed...

[Sam Weinig]

Comment on attachment 359103 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:179
> +    dictionaryPopupInfo.attributedString = adoptNS([[NSMutableAttributedString alloc] initWithString:range.text()]);

Why is this dictionaryPopupInfo.attributedString so different that the one used on Mac?

[Tim Horton]

Comment on attachment 359103 [details]
Patch

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

>> Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:179
>> +    dictionaryPopupInfo.attributedString = adoptNS([[NSMutableAttributedString alloc] initWithString:range.text()]);
> 
> Why is this dictionaryPopupInfo.attributedString so different that the one used on Mac?

We don't need the attributes because we'll never paint it (we in odd cases actually paint the attributed string on macOS). That said, we might want to generate with one with editingAttributedStringFromRange() and then just not apply the attributes instead of taking a totally different path to creating it.

[Wenson Hsieh]

(In reply to Tim Horton from comment #11)
> Comment on attachment 359103 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=359103&action=review
> 
> >> Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:179
> >> +    dictionaryPopupInfo.attributedString = adoptNS([[NSMutableAttributedString alloc] initWithString:range.text()]);
> > 
> > Why is this dictionaryPopupInfo.attributedString so different that the one used on Mac?
> 
> We don't need the attributes because we'll never paint it (we in odd cases
> actually paint the attributed string on macOS). That said, we might want to
> generate with one with editingAttributedStringFromRange() and then just not
> apply the attributes instead of taking a totally different path to creating
> it.

Unfortunately, editingAttributedStringFromRange() appears to be guarded by !IOS_FAMILY :(

This may just be because it uses NSFontManager. One avenue might be to define out the only part that uses the fontManager in that function (it appears to be used as a fallback when the font cascade's font is missing, and we try to grab a default font of the same size).

I think we could do all this in a followup though...

[Megan Gardner]

Created attachment 359184 [details]
Patch

[Tim Horton]

Comment on attachment 359184 [details]
Patch

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

> Source/WebCore/Configurations/WebCore.xcconfig:119
> +WK_UIKITMAC_LDFLAGS = $(WK_UIKITMAC_LDFLAGS_$(WK_PLATFORM_NAME));
> +WK_UIKITMAC_LDFLAGS_iosmac = -framework UIKitMacHelper;

I think this is going to introduce a build dependency cycle, you'll need to softlink this framework as well.

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:65
> +#include <pal/spi/ios/UIKitSPI.h>

This is oddly placed, you can put it up with the other headers.

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:230
> +    return _highlighting;

Is this a part of the protocol? Or why do we have this?

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:511
> +    UNUSED_PARAM(createAnimationController);

Probably should ASSERT_UNUSED(!createAnimationController, createAnimationController); (or however that macro works, I never get it right). We never want to get here with createAnimationController set (at the moment).

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:519
> +    RetainPtr<RVItem> item = adoptNS([allocRVItemInstance() initWithText:dictionaryPopupInfo.attributedString.get().string selectedRange:NSMakeRange(0, 0)]);
> +    
> +    [webHighlight setImage:textIndicator->contentImage()];

Why is image not just another parameter to the initializer?

[Simon Fraser (smfr)]

Comment on attachment 359184 [details]
Patch

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

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:246
> +    CGContextSaveGState(context);

CGContextStateSaver!

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:251
> +    transform = CGAffineTransformScale(transform, iOSMacScaleFactor, iOSMacScaleFactor);

Why not CGAffineTransform transform = CGAffineTransformMakeScale()

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:257
> +        CGRect origin = CGRectMake(backingScale * (imgSrcRect.origin.x - highlightRect.origin.x), backingScale * (imgSrcRect.origin.y - highlightRect.origin.y),  backingScale * (highlightRect.size.width), backingScale*(highlightRect.size.height));

Why do this backingScale math here, rather than putting it into the context scale?

[Megan Gardner]

Comment on attachment 359184 [details]
Patch

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

>> Source/WebCore/editing/cocoa/DictionaryLookup.mm:230
>> +    return _highlighting;
> 
> Is this a part of the protocol? Or why do we have this?

Yes, it is part of the protocol

>> Source/WebCore/editing/cocoa/DictionaryLookup.mm:257
>> +        CGRect origin = CGRectMake(backingScale * (imgSrcRect.origin.x - highlightRect.origin.x), backingScale * (imgSrcRect.origin.y - highlightRect.origin.y),  backingScale * (highlightRect.size.width), backingScale*(highlightRect.size.height));
> 
> Why do this backingScale math here, rather than putting it into the context scale?

I copied this wholesale from the UIKit implementation, per Guillaume's suggestion. I changed as little as possible to get it working.

>> Source/WebCore/editing/cocoa/DictionaryLookup.mm:519
>> +    [webHighlight setImage:textIndicator->contentImage()];
> 
> Why is image not just another parameter to the initializer?

Because I didn't think we could have a @property Image

[Tim Horton]

Comment on attachment 359184 [details]
Patch

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

>>> Source/WebCore/editing/cocoa/DictionaryLookup.mm:230
>>> +    return _highlighting;
>> 
>> Is this a part of the protocol? Or why do we have this?
> 
> Yes, it is part of the protocol

OK.

>>> Source/WebCore/editing/cocoa/DictionaryLookup.mm:257
>>> +        CGRect origin = CGRectMake(backingScale * (imgSrcRect.origin.x - highlightRect.origin.x), backingScale * (imgSrcRect.origin.y - highlightRect.origin.y),  backingScale * (highlightRect.size.width), backingScale*(highlightRect.size.height));
>> 
>> Why do this backingScale math here, rather than putting it into the context scale?
> 
> I copied this wholesale from the UIKit implementation, per Guillaume's suggestion. I changed as little as possible to get it working.

This is one piece of cleanup you can likely safely do.

>>> Source/WebCore/editing/cocoa/DictionaryLookup.mm:519
>>> +    [webHighlight setImage:textIndicator->contentImage()];
>> 
>> Why is image not just another parameter to the initializer?
> 
> Because I didn't think we could have a @property Image

1) you can definitely have C++ types in properties
2) why do you need a property? Can't they just be ivars?

[Megan Gardner]

Created attachment 359212 [details]
Patch

[Tim Horton]

Comment on attachment 359212 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=193408

Where's the radar

> Source/WebCore/Configurations/WebCore.xcconfig:120
> +WK_UIKITMAC_LDFLAGS = $(WK_UIKITMAC_LDFLAGS_$(WK_PLATFORM_NAME));
> +WK_UIKITMAC_LDFLAGS_iosmac = -framework UIKitMacHelper;
> +

Remove this.

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:68
> +#include <pal/spi/ios/UIKitSPI.h>

I said something about this before.

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:180
> +    _highlighting = NO;

Technically not needed; ObjC zero fills objects.

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:232
> +    NSArray <NSValue *> *rects = [self highlightRectsForItem:item];

NSArray<

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:236
> +    UIView *textContentView = _view;

Maybe just use _view elsewhere instead of this local that is *more*  typing?

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:239
> +    for (NSUInteger i = 1; i < rects.count; i++)

This could use fast iteration! (for (NSValue *rect in rects)...)

> Source/WebCore/editing/cocoa/DictionaryLookup.mm:256
> +    

No newline :)

[Megan Gardner]

Created attachment 359219 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 359219 [details]
Patch for landing

Rejecting attachment 359219 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 359219, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=359219&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=193408&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Updating working directory
Processing patch 359219 from bug 193408.
Fetching: https://bugs.webkit.org/attachment.cgi?id=359219
Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/PAL/ChangeLog
	M	Source/WebCore/PAL/pal/spi/cocoa/RevealSPI.h
	M	Source/WebCore/PAL/pal/spi/ios/UIKitSPI.h
	M	Source/WebCore/editing/cocoa/DictionaryLookup.mm
	M	Source/WebCore/editing/mac/DictionaryLookup.h
	M	Source/WebKit/ChangeLog

ERROR from SVN:
Item is out of date: File '/trunk/Source/WebKit/ChangeLog' is out of date
W: 8b6fb47134c77a4ef657c35a4147c6d273583138 and refs/remotes/origin/master differ, using rebase:
:040000 040000 8efbe8fa869a5c8c32565e31ec4ae494f4b65cc7 0577f0ce766d9cc7c9796288d1735dd8ef4ea8dd M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/PAL/ChangeLog
	M	Source/WebCore/PAL/pal/spi/cocoa/RevealSPI.h
	M	Source/WebCore/PAL/pal/spi/ios/UIKitSPI.h
	M	Source/WebCore/editing/cocoa/DictionaryLookup.mm
	M	Source/WebCore/editing/mac/DictionaryLookup.h
	M	Source/WebKit/ChangeLog

ERROR from SVN:
Item is out of date: File '/trunk/Source/WebKit/ChangeLog' is out of date
W: 8b6fb47134c77a4ef657c35a4147c6d273583138 and refs/remotes/origin/master differ, using rebase:
:040000 040000 8efbe8fa869a5c8c32565e31ec4ae494f4b65cc7 0577f0ce766d9cc7c9796288d1735dd8ef4ea8dd M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
From https://git.webkit.org/git/WebKit
   a364fa76fb3..758647a3573  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 240017 = a364fa76fb35b7cc445c092b976a5d2e121a43d0
r240018 = 758647a35739e72e66b5fef7d286095cecc36e23
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: https://webkit-queues.webkit.org/results/10765244

[Megan Gardner]

Created attachment 359223 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 359223 [details]
Patch for landing

Clearing flags on attachment: 359223

Committed r240021: <https://trac.webkit.org/changeset/240021>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47303063>