Bug 193408 - Add Reveal support in iOSMac
Summary: Add Reveal support in iOSMac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-14 14:20 PST by Megan Gardner
Modified: 2019-01-15 19:02 PST (History)
6 users (show)

See Also:


Attachments
Patch (21.09 KB, patch)
2019-01-14 14:29 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (20.74 KB, patch)
2019-01-14 16:06 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (20.83 KB, patch)
2019-01-14 17:15 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (20.83 KB, patch)
2019-01-14 17:27 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (21.52 KB, patch)
2019-01-15 11:39 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (19.31 KB, patch)
2019-01-15 15:15 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (18.52 KB, patch)
2019-01-15 16:19 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (18.46 KB, patch)
2019-01-15 16:41 PST, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2019-01-14 14:20:47 PST
Add Reveal support in iOSMac
Comment 1 Megan Gardner 2019-01-14 14:29:30 PST
Created attachment 359079 [details]
Patch
Comment 2 Tim Horton 2019-01-14 14:33:31 PST
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
Comment 3 Megan Gardner 2019-01-14 15:17:33 PST
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?
Comment 4 Wenson Hsieh 2019-01-14 15:31:21 PST
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).
Comment 5 Wenson Hsieh 2019-01-14 15:32:16 PST
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 *
Comment 6 Megan Gardner 2019-01-14 16:06:53 PST
Created attachment 359093 [details]
Patch
Comment 7 Megan Gardner 2019-01-14 17:15:58 PST
Created attachment 359102 [details]
Patch
Comment 8 Megan Gardner 2019-01-14 17:27:38 PST
Created attachment 359103 [details]
Patch
Comment 9 Tim Horton 2019-01-14 17:36:04 PST
(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...
Comment 10 Sam Weinig 2019-01-15 10:12:59 PST
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?
Comment 11 Tim Horton 2019-01-15 10:46:07 PST
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.
Comment 12 Wenson Hsieh 2019-01-15 10:52:53 PST
(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...
Comment 13 Megan Gardner 2019-01-15 11:39:13 PST
Created attachment 359184 [details]
Patch
Comment 14 Tim Horton 2019-01-15 13:17:22 PST
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?
Comment 15 Simon Fraser (smfr) 2019-01-15 13:26:44 PST
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?
Comment 16 Megan Gardner 2019-01-15 14:36:07 PST
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
Comment 17 Tim Horton 2019-01-15 14:41:35 PST
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?
Comment 18 Megan Gardner 2019-01-15 15:15:36 PST
Created attachment 359212 [details]
Patch
Comment 19 Tim Horton 2019-01-15 16:07:15 PST
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 :)
Comment 20 Megan Gardner 2019-01-15 16:19:08 PST
Created attachment 359219 [details]
Patch for landing
Comment 21 WebKit Commit Bot 2019-01-15 16:35:47 PST
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 commit-queue@webkit.org...
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
Comment 22 Megan Gardner 2019-01-15 16:41:45 PST
Created attachment 359223 [details]
Patch for landing
Comment 23 WebKit Commit Bot 2019-01-15 17:19:26 PST
Comment on attachment 359223 [details]
Patch for landing

Clearing flags on attachment: 359223

Committed r240021: <https://trac.webkit.org/changeset/240021>
Comment 24 WebKit Commit Bot 2019-01-15 17:19:28 PST
All reviewed patches have been landed.  Closing bug.
Comment 25 Radar WebKit Bug Importer 2019-01-15 17:21:29 PST
<rdar://problem/47303063>