Bug 136474 – Remove PLATFORM(IOS) from WebCore/editing

• Status: NEW
• Component: HTML Editing

[Enrica Casucci]

This bug tracks the work to cleanup WebCore editing.
The goal is to remove PLATFORM(IOS).

[Enrica Casucci]

Created attachment 237541 [details]
Patch

[Tim Horton]

Comment on attachment 237541 [details]
Patch

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

Yay!

> Source/WebCore/dom/Range.h:49
>  #if PLATFORM(IOS)

Technically you don't need this #if around a forward declaration.

> Source/WebCore/editing/EditorCommand.cpp:1246
>      return frame.editor().canDHTMLCopy() || frame.editor().canCopy();

Is this right? This means we'll now check canDHTMLCopy on iOS, unlike previously.

[Enrica Casucci]

(In reply to comment #2)
> (From update of attachment 237541 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237541&action=review
> 
> Yay!
> 
> > Source/WebCore/dom/Range.h:49
> >  #if PLATFORM(IOS)
> 
> Technically you don't need this #if around a forward declaration.
> 
> > Source/WebCore/editing/EditorCommand.cpp:1246
> >      return frame.editor().canDHTMLCopy() || frame.editor().canCopy();
> 
> Is this right? This means we'll now check canDHTMLCopy on iOS, unlike previously.
I think this is correct. We added support to handle cut/copy/paste events in JS a while ago and this was probably an oversight.

[Enrica Casucci]

Committed revision 173235.

[Enrica Casucci]

Created attachment 237635 [details]
Patch2

Second block of changes.

[Enrica Casucci]

Committed revision 173287.

[Enrica Casucci]

Created attachment 237715 [details]
Patch3

[WebKit Commit Bot]

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


ERROR: Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:300:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:301:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 13 files


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

[Benjamin Poulain]

Comment on attachment 237715 [details]
Patch3

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

> Source/WebCore/page/mac/WebCoreFrameView.h:32
>      class Frame;
> +    class IntPoint;

Wrong indent here.

> Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:300
> +                                         toDOMRange:kit(toRange) affinity:(selectionAffinity == UPSTREAM) ? NSSelectionAffinityUpstream : NSSelectionAffinityDownstream

I am not a fan of the 3 copies of "(selectionAffinity == UPSTREAM) ? NSSelectionAffinityUpstream : NSSelectionAffinityDownstream".
I would have static inline for that.

[Enrica Casucci]

(In reply to comment #9)
> (From update of attachment 237715 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237715&action=review
> 
> > Source/WebCore/page/mac/WebCoreFrameView.h:32
> >      class Frame;
> > +    class IntPoint;
> 
> Wrong indent here.
> 
> > Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:300
> > +                                         toDOMRange:kit(toRange) affinity:(selectionAffinity == UPSTREAM) ? NSSelectionAffinityUpstream : NSSelectionAffinityDownstream
> 
> I am not a fan of the 3 copies of "(selectionAffinity == UPSTREAM) ? NSSelectionAffinityUpstream : NSSelectionAffinityDownstream".
> I would have static inline for that.

Good idea, I'm adding it to WebEditorClient.h

[Enrica Casucci]

Committed revision 173340.

[Csaba Osztrogonác]

(In reply to comment #11)
> Committed revision 173340.

It made ~40 tests fail. Could you fix?

[Alexey Proskuryakov]

Presumably not right now, so I'm rolling out the patch.

The failures are here:

http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK1%20(Tests)/r173342%20(8391)/results.html

[Enrica Casucci]

Created attachment 238046 [details]
Patch4

Second attempt to cleanup phase 3 that doesn't break the tests.

[WebKit Commit Bot]

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


ERROR: Source/WebKit/mac/WebCoreSupport/WebEditorClient.h:187:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/mac/ScrollViewMac.mm:205:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 2 in 14 files


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

[Enrica Casucci]

(In reply to comment #15)
> Attachment 238046 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebKit/mac/WebCoreSupport/WebEditorClient.h:187:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
> ERROR: Source/WebCore/platform/mac/ScrollViewMac.mm:205:  One space before end of line comments  [whitespace/comments] [5]
> Total errors found: 2 in 14 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

I've already fixed them.

[Build Bot]

Comment on attachment 238046 [details]
Patch4

Attachment 238046 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6067744550158336

New failing tests:
fast/events/wheelevent-in-vertical-scrollbar-in-rtl.html
fast/regions/selection/position-for-point-1-vert-rl.html
fast/dom/vertical-scrollbar-in-rtl.html
fast/regions/selection/position-for-point-vert-rl.html
fast/block/positioning/auto/vertical-rl/007.html
compositing/rtl/rtl-fixed-overflow-scrolled.html
fast/multicol/pagination/BottomToTop-tb.html
fast/multicol/pagination-v-horizontal-bt.html
fast/multicol/vertical-rl/column-break-with-balancing.html
fast/multicol/pagination/RightToLeft-lr.html
fast/dom/vertical-scrollbar-when-dir-change.html
fast/dom/scroll-reveal-top-overflow.html
fast/multicol/pagination/RightToLeft-rl-dynamic.html
fast/multicol/pagination/BottomToTop-bt.html
fast/dom/horizontal-scrollbar-in-rtl.html
fast/block/basic/truncation-rtl.html
fast/multicol/pagination/RightToLeft-tb.html
fast/dom/scroll-reveal-left-overflow.html
fast/multicol/pagination/RightToLeft-rl.html
fast/events/wheelevent-in-horizontal-scrollbar-in-rtl.html
fast/multicol/pagination/BottomToTop-lr.html
fast/multicol/pagination/BottomToTop-rl.html
fast/block/positioning/vertical-rl/fixed-positioning.html
fast/dom/horizontal-scrollbar-when-dir-change.html
fast/multicol/pagination/RightToLeft-bt.html
fast/block/positioning/rtl-fixed-positioning.html
fast/multicol/pagination-h-vertical-rl.html

[Build Bot]

Created attachment 238051 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 238046 [details]
Patch4

Attachment 238046 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6472977935761408

New failing tests:
fast/events/wheelevent-in-vertical-scrollbar-in-rtl.html
fast/regions/selection/position-for-point-1-vert-rl.html
fast/dom/vertical-scrollbar-in-rtl.html
fast/regions/selection/position-for-point-vert-rl.html
fast/block/positioning/auto/vertical-rl/007.html
compositing/rtl/rtl-fixed-overflow-scrolled.html
fast/multicol/pagination/BottomToTop-tb.html
fast/multicol/pagination-v-horizontal-bt.html
fast/multicol/vertical-rl/column-break-with-balancing.html
fast/multicol/pagination/RightToLeft-rl.html
fast/dom/vertical-scrollbar-when-dir-change.html
fast/dom/scroll-reveal-top-overflow.html
fast/multicol/pagination/RightToLeft-rl-dynamic.html
fast/multicol/pagination/BottomToTop-bt.html
fast/dom/horizontal-scrollbar-in-rtl.html
fast/block/basic/truncation-rtl.html
fast/multicol/pagination/RightToLeft-tb.html
fast/dom/scroll-reveal-left-overflow.html
fast/multicol/pagination/RightToLeft-lr.html
fast/events/wheelevent-in-horizontal-scrollbar-in-rtl.html
fast/multicol/pagination/BottomToTop-lr.html
fast/multicol/pagination/BottomToTop-rl.html
fast/block/positioning/vertical-rl/fixed-positioning.html
fast/dom/horizontal-scrollbar-when-dir-change.html
fast/multicol/pagination/RightToLeft-bt.html
fast/block/positioning/rtl-fixed-positioning.html
fast/multicol/pagination-h-vertical-rl.html

[Build Bot]

Created attachment 238069 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Enrica Casucci]

Committed revision 173669.

[Enrica Casucci]

Created attachment 238208 [details]
Patch4

[Enrica Casucci]

Committed revision 173670.

[Jessie Berlin]

(In reply to comment #23)
> Committed revision 173670.

This was rolled out in http://trac.webkit.org/changeset/173679.

[Csaba Osztrogonác]

Comment on attachment 237715 [details]
Patch3

Cleared Benjamin Poulain's review+ from obsolete attachment 237715 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Joseph Pecoraro]

Comment on attachment 238046 [details]
Patch4

Clearing r? flag so this doesn't appear in review queue. It looks like dos too these pieces have already landed anyways.