Bug 138552 – WK1: Support default actions for read-only text

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Beth Dakin]

WebKit 1 should support action menus for non-editable text.

rdar://problem/18877483

[Beth Dakin]

Created attachment 241265 [details]
Patch

[WebKit Commit Bot]

Attachment 241265 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebView/WebActionMenuController.mm:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit/mac/WebView/WebActionMenuController.mm:320:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/mac/WebView/WebActionMenuController.mm:409:  Missing space before ( in if(  [whitespace/parens] [5]
ERROR: Source/WebCore/editing/DictionaryLookup.h:40:  The parameter name "position" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/editing/DictionaryLookup.h:40:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/editing/DictionaryLookup.h:41:  The parameter name "position" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/editing/DictionaryLookup.h:41:  The parameter name "selection" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/editing/DictionaryLookup.h:43:  The parameter name "position" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/editing/DictionaryLookup.h:44:  The parameter name "selection" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/editing/DictionaryLookup.h:45:  The parameter name "hitTestResult" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/editing/DictionaryLookup.mm:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 11 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Comment on attachment 241265 [details]
Patch

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

Lots of stylebot comments.

> Source/WebCore/editing/DictionaryLookup.h:43
> +bool isPositionInRange(const VisiblePosition& position, Range* range);
> +bool shouldUseSelection(const VisiblePosition& position, const VisibleSelection& selection);
> +
> +PassRefPtr<Range> rangeExpandedAroundPositionByCharacters(const VisiblePosition& position, int numberOfCharactersToExpand);

isPositionInRange and rangeExpandedAroundPositionByCharacters should probably be on VisiblePosition or Range or something, they're pretty generically useful (maybe there's already something there that we can replace these with?)

> Source/WebCore/editing/DictionaryLookup.mm:29
> +#include "Document.h"

all of these should be 'import'

> Source/WebCore/editing/DictionaryLookup.mm:77
> +PassRefPtr<Range> rangeExpandedAroundPositionByCharacters(const VisiblePosition& position, int numberOfCharactersToExpand)

Remind me to make use of this in Editor, where this code was originally stolen from.

> Source/WebCore/editing/DictionaryLookup.mm:116
> +    wkExtractWordDefinitionTokenRangeFromContextualString(fullPlainTextString, rangeToPass, options);

We should probably ditch the WKSI wrappers and add some LookUp SPI headers.

> Source/WebKit/mac/WebView/WebActionMenuController.mm:297
> +static DictionaryPopupInfo performDictionaryLookupForRange(Frame* frame, Range& range, NSDictionary *options)

Can we put more of this in the WebCore?

[Beth Dakin]

(In reply to comment #3)
> Comment on attachment 241265 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241265&action=review
> 
> Lots of stylebot comments.
> 

Fixed!

> > Source/WebCore/editing/DictionaryLookup.h:43
> > +bool isPositionInRange(const VisiblePosition& position, Range* range);
> > +bool shouldUseSelection(const VisiblePosition& position, const VisibleSelection& selection);
> > +
> > +PassRefPtr<Range> rangeExpandedAroundPositionByCharacters(const VisiblePosition& position, int numberOfCharactersToExpand);
> 
> isPositionInRange and rangeExpandedAroundPositionByCharacters should
> probably be on VisiblePosition or Range or something, they're pretty
> generically useful (maybe there's already something there that we can
> replace these with?)
> 

Agreed. I filed https://bugs.webkit.org/show_bug.cgi?id=138567

> > Source/WebCore/editing/DictionaryLookup.mm:29
> > +#include "Document.h"
> 
> all of these should be 'import'
> 

Fixed!

> > Source/WebCore/editing/DictionaryLookup.mm:77
> > +PassRefPtr<Range> rangeExpandedAroundPositionByCharacters(const VisiblePosition& position, int numberOfCharactersToExpand)
> 
> Remind me to make use of this in Editor, where this code was originally
> stolen from.
> 

Will do! I made a note of that in the bug I filed!

> > Source/WebCore/editing/DictionaryLookup.mm:116
> > +    wkExtractWordDefinitionTokenRangeFromContextualString(fullPlainTextString, rangeToPass, options);
> 
> We should probably ditch the WKSI wrappers and add some LookUp SPI headers.
> 

Agreed. Will do this in the future.

> > Source/WebKit/mac/WebView/WebActionMenuController.mm:297
> > +static DictionaryPopupInfo performDictionaryLookupForRange(Frame* frame, Range& range, NSDictionary *options)
> 
> Can we put more of this in the WebCore?

We talked about this in person. The answer is yes, but not trivially. There are small differences in the code for WK1, unfortunately.

[Beth Dakin]

http://trac.webkit.org/changeset/175814