Bug 136474

Created attachment 237541 [details] Patch

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.

(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.

Committed revision 173235.

Created attachment 237635 [details] Patch2 Second block of changes.

Committed revision 173287.

Created attachment 237715 [details] Patch3

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.

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.

(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

Committed revision 173340.

(In reply to comment #11) > Committed revision 173340. It made ~40 tests fail. Could you fix?

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

Created attachment 238046 [details] Patch4 Second attempt to cleanup phase 3 that doesn't break the tests.

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.

(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.

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

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

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

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

Committed revision 173669.

Created attachment 238208 [details] Patch4

Committed revision 173670.

(In reply to comment #23) > Committed revision 173670. This was rolled out in http://trac.webkit.org/changeset/173679.

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.

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.