Bug 108114 – Adding iOS specific behavior to the base editing command structure

• Status: CLOSED
• Resolution: WONTFIX
• Component: HTML Editing

[Enrica Casucci]

Changing CompositeEditCommand.cpp, EditorCommand.cpp and EditCommand.cpp to include iOS specific changes.

<rdar://problem/13098252>

[Enrica Casucci]

Including changes to Editor.cpp, DeleteButtonController.cpp, Editor.cpp and DocumentMarkerController.cpp

[Enrica Casucci]

Created attachment 185318 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/Configurations/WebCore.xcconfig', u'Source/WebCore/dom/DocumentMarkerController.cpp', u'Source/WebCore/dom/DocumentMarkerController.h', u'Source/WebCore/editing/CompositeEditCommand.cpp', u'Source/WebCore/editing/CompositeEditCommand.h', u'Source/WebCore/editing/DeleteButtonController.cpp', u'Source/WebCore/editing/DeleteButtonController.h', u'Source/WebCore/editing/EditAction.h', u'Source/WebCore/editing/EditCommand.h', u'Source/WebCore/editing/Editor.cpp', u'Source/WebCore/editing/Editor.h', u'Source/WebCore/editing/EditorCommand.cpp']" exit_code: 1
Source/WebCore/dom/DocumentMarkerController.cpp:125:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/DocumentMarkerController.cpp:154:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/DocumentMarkerController.cpp:624:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/dom/DocumentMarkerController.h:59:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/editing/Editor.h:351:  The parameter name "element" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/editing/Editor.cpp:106:  The parameter name "document" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/editing/Editor.cpp:107:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/editing/Editor.cpp:410:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/editing/Editor.cpp:446:  Consider using toText helper function in WebCore/dom/Text.h instead of static_cast<Text*>  [readability/check] [4]
Source/WebCore/editing/Editor.cpp:567:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/editing/Editor.cpp:568:  Declaration has space between type name and * in Node *focused  [whitespace/declaration] [3]
Source/WebCore/editing/Editor.cpp:569:  Declaration has space between type name and * in Node *composition  [whitespace/declaration] [3]
Source/WebCore/editing/Editor.cpp:576:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/editing/Editor.cpp:591:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/editing/Editor.cpp:601:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/editing/Editor.cpp:602:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/editing/Editor.cpp:635:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/editing/Editor.cpp:1529:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/editing/Editor.cpp:3141:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 19 in 13 files


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

[Enrica Casucci]

Created attachment 185332 [details]
Patch2

Fixes style issues.

[Adam Barth]

Comment on attachment 185332 [details]
Patch2

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

> Source/WebCore/editing/Editor.h:47
> +typedef int WKWritingDirection;

I'm curious what the "WK" here refers to.  I know the WK prefix is used in the WebKit2 API, but I presume you're not introducing WebKit2 API concepts into WebCore.

> Source/WebCore/editing/Editor.h:274
> +    NO_RETURN_DUE_TO_ASSERT

What is this macro?

[Benjamin Poulain]

(In reply to comment #5)
> (From update of attachment 185332 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185332&action=review
> 
> > Source/WebCore/editing/Editor.h:47
> > +typedef int WKWritingDirection;
> 
> I'm curious what the "WK" here refers to.  I know the WK prefix is used in the WebKit2 API, but I presume you're not introducing WebKit2 API concepts into WebCore.

The WK is for WebKit (1). :)

On iOS, we do not use NSWritingDirection because WebCore does not link with CocoaTouch. The name NSWritingDirection would conflict with UIKits headers so the iOS WebKit type are prefixed with WK.


> > Source/WebCore/editing/Editor.h:274
> > +    NO_RETURN_DUE_TO_ASSERT
> 
> What is this macro?

This is equivalent of noreturn. This tells the compiler that particular function never returns. We have no-return Warnings enabled on iOS.

[Adam Barth]

Thanks for the explanations!

[Adam Barth]

Comment on attachment 185332 [details]
Patch2

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

>>> Source/WebCore/editing/Editor.h:47
>>> +typedef int WKWritingDirection;
>> 
>> I'm curious what the "WK" here refers to.  I know the WK prefix is used in the WebKit2 API, but I presume you're not introducing WebKit2 API concepts into WebCore.
> 
> The WK is for WebKit (1). :)
> 
> On iOS, we do not use NSWritingDirection because WebCore does not link with CocoaTouch. The name NSWritingDirection would conflict with UIKits headers so the iOS WebKit type are prefixed with WK.

It's certainly not a big deal, but I think this is a minor layering violation.  Certainly if you had to include a WebKit (1) API header to get this definition, you'd see a problem with including a WebKit-layer header in WebCore.  If I'm understanding correctly, you're copy-and-pasting the definition instead of including the header, but the net result is the same.

Generally, the way that we've approached these sorts of things in the past is to introduce a WebCore type and then translate between the WebCore type and the API type at the API boundary.  Often we arrange things (especially for enums) so that the WebCore and the API type have the same values.  That way we can convert them for free using static_cast.  We then use AssertMatchingEnums.cpp to COMPILE_ASSERT that the values are the same.

(I should say that I'm very supportive of your upstreaming iOS code.  I just mention the above in case it's useful to you.)

[Benjamin Poulain]

> It's certainly not a big deal, but I think this is a minor layering violation.  Certainly if you had to include a WebKit (1) API header to get this definition, you'd see a problem with including a WebKit-layer header in WebCore.  If I'm understanding correctly, you're copy-and-pasting the definition instead of including the header, but the net result is the same.
> 
> Generally, the way that we've approached these sorts of things in the past is to introduce a WebCore type and then translate between the WebCore type and the API type at the API boundary.  Often we arrange things (especially for enums) so that the WebCore and the API type have the same values.  That way we can convert them for free using static_cast.  We then use AssertMatchingEnums.cpp to COMPILE_ASSERT that the values are the same.

That is indeed what we have.
We have a (a little weird) abstraction in WebCore and we convert everything to native in the upper layers.

None of that layer is upstreamed yet and won't be for a while.

Enrica, since the stubs are not upstreamed, I think we could use WebCore's WritingDirection in this particular case. In the WebKit layer, we would convert to WKWritingDirection. That's an extra branch but it is not a hot path. What do you think?

> (I should say that I'm very supportive of your upstreaming iOS code.  I just mention the above in case it's useful to you.)

Your feedback is welcome.

[Enrica Casucci]

(In reply to comment #9)
> > It's certainly not a big deal, but I think this is a minor layering violation.  Certainly if you had to include a WebKit (1) API header to get this definition, you'd see a problem with including a WebKit-layer header in WebCore.  If I'm understanding correctly, you're copy-and-pasting the definition instead of including the header, but the net result is the same.
> > 
> > Generally, the way that we've approached these sorts of things in the past is to introduce a WebCore type and then translate between the WebCore type and the API type at the API boundary.  Often we arrange things (especially for enums) so that the WebCore and the API type have the same values.  That way we can convert them for free using static_cast.  We then use AssertMatchingEnums.cpp to COMPILE_ASSERT that the values are the same.
> 
> That is indeed what we have.
> We have a (a little weird) abstraction in WebCore and we convert everything to native in the upper layers.
> 
> None of that layer is upstreamed yet and won't be for a while.
> 
> Enrica, since the stubs are not upstreamed, I think we could use WebCore's WritingDirection in this particular case. In the WebKit layer, we would convert to WKWritingDirection. That's an extra branch but it is not a hot path. What do you think?

Sounds good to me. I'll double check to make sure nothing breaks and I'll post another patch.

> 
> > (I should say that I'm very supportive of your upstreaming iOS code.  I just mention the above in case it's useful to you.)
> 
> Your feedback is welcome.

Adam, thanks a lot for the feedback!

[Enrica Casucci]

I looked in detail into the issue of using WKWritingDirection and NSWritingDirection.
I'm fairly convinced that neither one should be in WebCore and both Mac and iOS implementations should deal with WritingDirection only. We should leave it up to the WebKit layer to do the conversion to NSWritingDirection and WKWritingDirection.
To be more precise, NSWritingDirection is not needed at all, because selectionBaseWritingDirection is used only in toggleBaseWritingDirection in WebHTMLView.mm.
It's kind of difficult to make this change while the rest of the code has not yet been upstreamed.
I would prefer to land this with the WKWritingDirection in the header, adding a FIXME to indicate what needs to be done.
I'll make the complete fix for both Mac and iOS in a separate patch.
What do you think?

[Benjamin Poulain]

> I would prefer to land this with the WKWritingDirection in the header, adding a FIXME to indicate what needs to be done.
> I'll make the complete fix for both Mac and iOS in a separate patch.
> What do you think?

I am fine with that.

[Adam Barth]

(In reply to comment #12)
> > I would prefer to land this with the WKWritingDirection in the header, adding a FIXME to indicate what needs to be done.
> > I'll make the complete fix for both Mac and iOS in a separate patch.
> > What do you think?
> 
> I am fine with that.

Sounds reasonable to me too.  Thanks for digging into the issue.

[Enrica Casucci]

The merge of the iOS code to OpenSource is complete. The bug is no longer needed.