Bug 203286 – Add a mechanism to find and manipulate text by paragraphs

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

Create a mechanism to find & replace paragraph of text in a WebPage.

[Ryosuke Niwa]

Created attachment 381664 [details]
WIP

[Ryosuke Niwa]

Created attachment 381728 [details]
WIP2

[Ryosuke Niwa]

Created attachment 381739 [details]
Patch

[Ryosuke Niwa]

Created attachment 381745 [details]
More build fixes

[Tim Horton]

Comment on attachment 381745 [details]
More build fixes

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:7468
> +    WebCore::FrameIdentifier frameID;

Just a frame ID isn't enough to be globally unique. See ElementContext; I think you want a pageIdentifier as well. Or maybe a GlobalFrameIdentifier

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:7505
> +        NSMutableArray *wkTokens = [NSMutableArray arrayWithCapacity: tokens.size()];

No space after :

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:7508
> +            wkToken.get().identifier = String::number(token.identifier.toUInt64());

I (and some others) generally prefer [wkToken setIdentifier:] in the RetainPtr case (without a get()), but you can go either way.

> Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationDelegate.h:37
> +- (void)_webView:(WKWebView *)webView didFindItem:(_WKTextManipulationItem *)item forFrame:(_WKFrameHandle *)frame;

This sounds like find-in-page. Can it have a more specific name that's less likely to overlap?

> Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationItem.h:34
> +

You probably want to NS_UNAVAILABLE -init.

> Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationItem.mm:42
> +    _identifier = identifier;
> +    _tokens = tokens;

Should these copy the incoming objects?

> Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationToken.mm:29
> +#import <wtf/RetainPtr.h>

Why?

> Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationToken.mm:32
> +@implementation _WKTextManipulationToken {
> +}

No need for these braces

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:11245
> +				9B02E0D1235EBCCA004044B2 /* _WKTextManipulationItem.mm in Sources */,
> +				9B02E0CF235EBB14004044B2 /* _WKTextManipulationToken.mm in Sources */,

These should not be here (uncheck them in the Xcode project), they should be in SourcesCocoa.txt or something instead

> Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:62
> +    [_items.get() addObject:item];

No need for .get() here.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:67
> +    return _items.autorelease();

Really? This means _items will be nil after this. I think you just want .get()?

[Wenson Hsieh]

Comment on attachment 381745 [details]
More build fixes

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

> Source/WebCore/editing/TextManipulationController.cpp:67
> +                tokensInCurrentParagraph.append(ManipulationToken { m_tokenIdentifier.generate(), currentText.substring(endOfLastNewLine, offsetOfNextNewLine).toString() });

StringView::substring takes (unsigned start, unsigned length). Unless I’m missing something, it would appear that offsetOfNextNewLine isn’t a length.

> Source/WebCore/editing/TextManipulationController.cpp:77
> +                addItem(startOfCurrentParagraph, endOfCurrentParagraph, WTFMove(tokensInCurrentParagraph));

Is it guaranteed that tokensInCurrentParagraph is in a good state after the WTFMove here? I think in these cases where we will reuse the variable later on, we generally prefer to use std::exchange().

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:7464
> +    StringBuilder result;
> +    result.appendNumber(frameID.toUInt64());
> +    result.append(':');
> +    result.appendNumber(itemID.toUInt64());
> +    return result.toString();

Nit - could also consider makeString here.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:7501
> +        auto delegate = retainedSelf.get()._textManipulationDelegate;

Nit - [retainedSelf _textManipulationDelegate]

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:7508
>> +            wkToken.get().identifier = String::number(token.identifier.toUInt64());
> 
> I (and some others) generally prefer [wkToken setIdentifier:] in the RetainPtr case (without a get()), but you can go either way.

(I also have a mild preference for using -[] in this case)

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6860
> +    auto completeManipulation = [&] () {

Nit - I don’t think you need the () here.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:72
> +namespace TestWebKitAPI {

It might be good to tweak one of these test cases to have two or more line breaks, as well.

[Ryosuke Niwa]

(In reply to Tim Horton from comment #5)
> Comment on attachment 381745 [details]
> More build fixes
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381745&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:7468
> > +    WebCore::FrameIdentifier frameID;
> 
> Just a frame ID isn't enough to be globally unique. See ElementContext; I
> think you want a pageIdentifier as well. Or maybe a GlobalFrameIdentifier

I don't think this is true in the absence of process per frame but I'm going to get rid of the frame ID tracking for now since we don't support subframes at all for now.

> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:7505
> > +        NSMutableArray *wkTokens = [NSMutableArray arrayWithCapacity: tokens.size()];
> 
> No space after :

Fixed.

> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:7508
> > +            wkToken.get().identifier = String::number(token.identifier.toUInt64());
> 
> I (and some others) generally prefer [wkToken setIdentifier:] in the
> RetainPtr case (without a get()), but you can go either way.

Okay. Done.

> > Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationDelegate.h:37
> > +- (void)_webView:(WKWebView *)webView didFindItem:(_WKTextManipulationItem *)item forFrame:(_WKFrameHandle *)frame;
> 
> This sounds like find-in-page. Can it have a more specific name that's less
> likely to overlap?

Did rename to didFindTextManipulationItem.

> > Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationItem.h:34
> > +
> 
> You probably want to NS_UNAVAILABLE -init.

Done.

> > Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationItem.mm:42
> > +    _identifier = identifier;
> > +    _tokens = tokens;
> 
> Should these copy the incoming objects?

Maybe? I don't think it matters.

> > Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationToken.mm:29
> > +#import <wtf/RetainPtr.h>
> 
> Why?

Because I'm using RetainPtr??

> > Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationToken.mm:32
> > +@implementation _WKTextManipulationToken {
> > +}
> 
> No need for these braces

Okay.

> > Source/WebKit/WebKit.xcodeproj/project.pbxproj:11245
> > +				9B02E0D1235EBCCA004044B2 /* _WKTextManipulationItem.mm in Sources */,
> > +				9B02E0CF235EBB14004044B2 /* _WKTextManipulationToken.mm in Sources */,
> 
> These should not be here (uncheck them in the Xcode project), they should be
> in SourcesCocoa.txt or something instead

Fixed.

> > Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:62
> > +    [_items.get() addObject:item];
> 
> No need for .get() here.

Removed.

> > Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:67
> > +    return _items.autorelease();
> 
> Really? This means _items will be nil after this. I think you just want
> .get()?

Fixed. Honestly, I have no idea what this does. It complied and ran LOL. I really shouldn't be writing any Objective-C code.

[Ryosuke Niwa]

(In reply to Wenson Hsieh from comment #6)
> Comment on attachment 381745 [details]
> More build fixes
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381745&action=review
> 
> > Source/WebCore/editing/TextManipulationController.cpp:67
> > +                tokensInCurrentParagraph.append(ManipulationToken { m_tokenIdentifier.generate(), currentText.substring(endOfLastNewLine, offsetOfNextNewLine).toString() });
> 
> StringView::substring takes (unsigned start, unsigned length). Unless I’m
> missing something, it would appear that offsetOfNextNewLine isn’t a length.

Oops, this is clearly a bug. Will fix & add a test.

> > Source/WebCore/editing/TextManipulationController.cpp:77
> > +                addItem(startOfCurrentParagraph, endOfCurrentParagraph, WTFMove(tokensInCurrentParagraph));
> 
> Is it guaranteed that tokensInCurrentParagraph is in a good state after the
> WTFMove here? I think in these cases where we will reuse the variable later
> on, we generally prefer to use std::exchange().

Yes. WTF::Vector is guaranteed to be in a good state after WTFMove.

> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:7464
> > +    StringBuilder result;
> > +    result.appendNumber(frameID.toUInt64());
> > +    result.append(':');
> > +    result.appendNumber(itemID.toUInt64());
> > +    return result.toString();
> 
> Nit - could also consider makeString here.

Removed this code entirely.

> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:7501
> > +        auto delegate = retainedSelf.get()._textManipulationDelegate;
> 
> Nit - [retainedSelf _textManipulationDelegate]

Fixed.

> >> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:7508
> >> +            wkToken.get().identifier = String::number(token.identifier.toUInt64());
> > 
> > I (and some others) generally prefer [wkToken setIdentifier:] in the RetainPtr case (without a get()), but you can go either way.
> 
> (I also have a mild preference for using -[] in this case)

Okay, done that.

> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6860
> > +    auto completeManipulation = [&] () {
> 
> Nit - I don’t think you need the () here.

Sure. Removed.

> > Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:72
> > +namespace TestWebKitAPI {
> 
> It might be good to tweak one of these test cases to have two or more line
> breaks, as well.

Some of them already have but I don't have any test case where a single text node has multiple line breaks (inside pre for example). Will add a test case for that.

[Ryosuke Niwa]

Created attachment 381771 [details]
Addressed review comments

[Radar WebKit Bug Importer]

<rdar://problem/56566991>

[Ryosuke Niwa]

Ugh... unified builds strikes again.

[Ryosuke Niwa]

Created attachment 381773 [details]
iOS build fix

[Ryosuke Niwa]

Created attachment 381774 [details]
iOS build fix

[Wenson Hsieh]

Comment on attachment 381774 [details]
iOS build fix

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

> Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationDelegate.h:30
> +@class _WKFrameHandle;

Nit - do you need this forward declaration?

> Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationItem.mm:27
> +#include "config.h"
> +#include "_WKTextManipulationItem.h"

Nit - #include => #import

> Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationToken.mm:27
> +#include "config.h"
> +#include "_WKTextManipulationToken.h"

Nit - #include => #import

> Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationToken.mm:29
> +#import <wtf/RetainPtr.h>

Nit - Is this #import needed? I don’t see RetainPtr being used in this file.

> Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationToken.mm:38
> +- (instancetype)init
> +{
> +    if (!(self = [super init]))
> +        return nil;
> +    return self;
> +}

Nit - I’m not sure you need to override and implement -init here, since it’s not doing anything special.

[Ryosuke Niwa]

(In reply to Wenson Hsieh from comment #14)
> Comment on attachment 381774 [details]
> iOS build fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381774&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationDelegate.h:30
> > +@class _WKFrameHandle;
> 
> Nit - do you need this forward declaration?

Oops, that's a left over from when I had forFrame:. Removed.

> > Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationItem.mm:27
> > +#include "config.h"
> > +#include "_WKTextManipulationItem.h"
> 
> Nit - #include => #import

Fixed.

> > Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationToken.mm:27
> > +#include "config.h"
> > +#include "_WKTextManipulationToken.h"
> 
> Nit - #include => #import

Fixed.

> > Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationToken.mm:29
> > +#import <wtf/RetainPtr.h>
> 
> Nit - Is this #import needed? I don’t see RetainPtr being used in this file.

Good point. Removed.

> > Source/WebKit/UIProcess/API/Cocoa/_WKTextManipulationToken.mm:38
> > +- (instancetype)init
> > +{
> > +    if (!(self = [super init]))
> > +        return nil;
> > +    return self;
> > +}
> 
> Nit - I’m not sure you need to override and implement -init here, since it’s
> not doing anything special.

Removed.

[Ryosuke Niwa]

Committed r251558: <https://trac.webkit.org/changeset/251558>

[Matt Lewis]

This broke internal builds. See radar for details.

[Matt Lewis]

Reverted r251558 for reason:

This broke internal builds

Committed r251569: <https://trac.webkit.org/changeset/251569>

[Ryosuke Niwa]

(In reply to Matt Lewis from comment #18)
> Reverted r251558 for reason:
> 
> This broke internal builds
> 
> Committed r251569: <https://trac.webkit.org/changeset/251569>

I don't think that's accurate description. It seems that this patch broke all iOS builds even on build.webkit.org. What happened is that someone must have added another objective-C file between the time EWS processed my patch and I landed it this afternoon. And the offending file is nothing to do with this patch.

We really need a bot which builds everything with unified builds turned off.

[Ryosuke Niwa]

Created attachment 381851 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 381851 [details]
Patch for landing

Wait for EWS.

[Ryosuke Niwa]

Created attachment 381857 [details]
More iOS build fix

[Ryosuke Niwa]

Committed r251574: <https://trac.webkit.org/changeset/251574>