Create a mechanism to find & replace paragraph of text in a WebPage.
Created attachment 381664 [details] WIP
Created attachment 381728 [details] WIP2
Created attachment 381739 [details] Patch
Created attachment 381745 [details] More build fixes
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()?
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.
(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.
(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.
Created attachment 381771 [details] Addressed review comments
<rdar://problem/56566991>
Ugh... unified builds strikes again.
Created attachment 381773 [details] iOS build fix
Created attachment 381774 [details] iOS build fix
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.
(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.
Committed r251558: <https://trac.webkit.org/changeset/251558>
This broke internal builds. See radar for details.
Reverted r251558 for reason: This broke internal builds Committed r251569: <https://trac.webkit.org/changeset/251569>
(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.
Created attachment 381851 [details] Patch for landing
Comment on attachment 381851 [details] Patch for landing Wait for EWS.
Created attachment 381857 [details] More iOS build fix
Committed r251574: <https://trac.webkit.org/changeset/251574>