Bug 162947

Created attachment 290669 [details] First pass

Comment on attachment 290669 [details] First pass View in context: https://bugs.webkit.org/attachment.cgi?id=290669&action=review > Source/WebCore/dom/InputEvent.h:47 > + static Ref<InputEvent> createForBindings() Why do we need this variant? > Source/WebCore/dom/InputEvent.idl:28 > +] interface InputEvent : UIEvent { We should start using this interface where we fire input events for input and textarea elements, and we should have a test validating that the interface type names. > Source/WebCore/dom/InputEvent.idl:32 > + [InitializedByEventConstructor] readonly attribute DOMString inputType; > + > + readonly attribute DataTransfer? dataTransfer; > + sequence<StaticRange> getTargetRanges(); We shouldn't add these attributes until we have a concrete implementation. Otherwise, we'd end up with a broken API when some port, e.g. GTK+ port, ships trunk before we fully bake this feature. > Source/WebCore/dom/InputEventTypeNames.h:32 > +struct InputEventTypeNames { I don't think having a list of AtomicString like this is useful because we'd need a switch statement, etc... to concert between editing command name and these input event type names. I think a better approach would be associating each AtomicString to a specific editor command inside createCommandMap(). > Source/WebCore/dom/Range.h:177 > +Node* checkNodeWOffset(Node&, unsigned offset, ExceptionCode&); Can we just make this a static public member of Range or rename it to checkRangeBoundaryNodeOffset? > Source/WebCore/dom/StaticRange.h:52 > + void setStart(Ref<Node>&& container, int offset); > + void setEnd(Ref<Node>&& container, int offset); Use unsigned instead. > Source/WebCore/dom/StaticRange.idl:31 > +] interface StaticRange { We should hide this feature behind a runtime flag until we have baked the feature enough. Also, we definitely need a test for this. r- because of the lack of tests. > Source/WebCore/dom/StaticRange.idl:42 > + [NewObject, MayThrowLegacyException] Range toRange(); Why don't we use ExceptionOr type in the return type of toRange to avoid using MayThrowLegacyException here?

Comment on attachment 290669 [details] First pass View in context: https://bugs.webkit.org/attachment.cgi?id=290669&action=review > Source/WebCore/dom/StaticRange.h:61 > + Document& m_document; This needs to be a Ref. You can test this by creating a StaticRange inside an iframe, delete the iframe, and then accessing it.

After talking with Ryosuke in person, we agree that we should hold off on implementing StaticRange in WebKit until the spec is closer to being finalized. In the meantime, we should focus on making InputEvent.inputType work as expected.

It looks like we need to use dictionary instead so IDL / binding here needs to be completely rewritten :(

<rdar://problem/28853079>

Created attachment 292107 [details] Test patch

Comment on attachment 292107 [details] Test patch Attachment 292107 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2326610 New failing tests: js/dom/global-constructors-attributes.html

Created attachment 292117 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 292107 [details] Test patch View in context: https://bugs.webkit.org/attachment.cgi?id=292107&action=review > Source/WebCore/dom/StaticRange.idl:28 > +[ > + ImplementationLacksVTable, > +] interface StaticRange { This IDL needs to be under a runtime flag or a build flag. > Source/WebCore/editing/CompositeEditCommand.cpp:378 > +Vector<RefPtr<StaticRange>> CompositeEditCommand::contentEditableInputEventTargetRanges() const I'd call this simply targetRanges(). > Source/WebCore/editing/CompositeEditCommand.cpp:390 > +Vector<RefPtr<StaticRange>> CompositeEditCommand::inputEventTargetRanges() const And this one targetRangesForBindings. > Source/WebCore/editing/EditCommand.h:70 > + const Frame& frame() const; > + const Document& document() const { return m_document; } Why don't we define these two next to non-const variant? > Source/WebCore/editing/TypingCommand.cpp:458 > + if (range) > + m_currentAffectedRange = StaticRange::createFromRange(*range); > + else > + m_currentAffectedRange = nullptr; It's totally unclear why range is sometimes null and when m_currentAffectedRange is used and when it's not. Why is this needed? > Source/WebCore/editing/TypingCommand.cpp:631 > + if (shouldBreakOutOfEmptyListItem()) { > + if (willAddTypingToOpenCommand(DeleteKey, granularity, { }, Range::create(document(), selection.selection().start().previous(BackwardDeletion), selection.selection().end()))) { We should probably make shouldBreakOutOfEmptyListItem return a selection to delete instead.

Comment on attachment 292107 [details] Test patch View in context: https://bugs.webkit.org/attachment.cgi?id=292107&action=review >> Source/WebCore/dom/StaticRange.idl:28 >> +] interface StaticRange { > > This IDL needs to be under a runtime flag or a build flag. Added a runtime IDL flag for InputEvents, and guarded StaticRange/InputEvents with it. >> Source/WebCore/editing/CompositeEditCommand.cpp:378 >> +Vector<RefPtr<StaticRange>> CompositeEditCommand::contentEditableInputEventTargetRanges() const > > I'd call this simply targetRanges(). Done. >> Source/WebCore/editing/CompositeEditCommand.cpp:390 >> +Vector<RefPtr<StaticRange>> CompositeEditCommand::inputEventTargetRanges() const > > And this one targetRangesForBindings. Done. >> Source/WebCore/editing/EditCommand.h:70 >> + const Document& document() const { return m_document; } > > Why don't we define these two next to non-const variant? Good point -- moved. >> Source/WebCore/editing/TypingCommand.cpp:458 >> + m_currentAffectedRange = nullptr; > > It's totally unclear why range is sometimes null and when m_currentAffectedRange is used and when it's not. > Why is this needed? On second thought, it's not. I could just plumb the range as an argument on Editor::willApplyEditing -- I'll change it to do this. >> Source/WebCore/editing/TypingCommand.cpp:631 >> + if (willAddTypingToOpenCommand(DeleteKey, granularity, { }, Range::create(document(), selection.selection().start().previous(BackwardDeletion), selection.selection().end()))) { > > We should probably make shouldBreakOutOfEmptyListItem return a selection to delete instead. Done.

Created attachment 292149 [details] Addresses EWS failures and adds layout tests.

Created attachment 292155 [details] Added ChangeLog entries.

Created attachment 292157 [details] Tweaked LayoutTest expectations again for WK1.

Comment on attachment 292157 [details] Tweaked LayoutTest expectations again for WK1. Attachment 292157 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2329253 New failing tests: js/dom/global-constructors-attributes.html

Created attachment 292158 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 292157 [details] Tweaked LayoutTest expectations again for WK1. Attachment 292157 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2329272 New failing tests: fast/events/before-input-delete-text-target-ranges.html fast/events/before-input-replace-text-target-ranges.html

Created attachment 292160 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Created attachment 292180 [details] Tweaked iOS simulator expectations

Created attachment 292185 [details] Patch

Created attachment 292311 [details] Test Patch (includes 162947)

Created attachment 292312 [details] Patch

Comment on attachment 292312 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292312&action=review > Source/WebCore/dom/InputEvent.idl:28 > + EnabledAtRuntime=InputEvents, > Constructor(DOMString type, optional InputEventInit eventInitDict), Please keep extended attributes in alphabetical order. > Source/WebCore/dom/StaticRange.h:30 > +#include "Node.h" > +#include <wtf/RefCounted.h> > +#include <wtf/RefPtr.h> Please make the includes be minimal. I do not think Node.h needs to be included, but I am not sure. Would need to try it and see if adding that include elsewhere was sufficient. If Node.h is included, then RefCounted.h and RefPtr.h do not need to be included. > Source/WebCore/dom/StaticRange.h:53 > + RefPtr<Node> m_startContainer; This should be Ref<Node>, not RefPtr<Node>. > Source/WebCore/dom/StaticRange.h:55 > + RefPtr<Node> m_endContainer; Ditto. > Source/WebCore/editing/CompositeEditCommand.cpp:381 > + if (auto firstRange = frame().selection().selection().firstRange()) { We normally use early return, which means the error case comes first, rather than nesting the normal case inside an if statement. > Source/WebCore/editing/CompositeEditCommand.cpp:384 > + Vector<RefPtr<StaticRange>> ranges; > + ranges.append(StaticRange::createFromRange(*firstRange)); > + return ranges; I would write this instead: RefPtr<StaticRange> range = StaticRange::createFromRange(*firstRange); return { 1, &range }; > Source/WebCore/editing/CompositeEditCommand.cpp:1542 > + RefPtr<Node> emptyListItem = enclosingEmptyListItem(endingSelection().visibleStart()); > + RefPtr<ContainerNode> listNode = emptyListItem->parentNode(); > + RefPtr<EditingStyle> style = EditingStyle::create(endingSelection().start()); These should use auto. > Source/WebCore/editing/CompositeEditCommand.h:32 > +#include "StaticRange.h" This include is not needed here. A forward declaration should suffice. > Source/WebCore/editing/ReplaceRangeWithTextCommand.cpp:80 > + Vector<RefPtr<StaticRange>> ranges; > + ranges.append(StaticRange::createFromRange(*m_rangeToBeReplaced)); > + return ranges; I would write this instead: RefPtr<StaticRange> range = StaticRange::createFromRange(*m_rangeToBeReplaced); return { 1, &range }; > Source/WebCore/editing/SpellingCorrectionCommand.cpp:126 > + Vector<RefPtr<StaticRange>> ranges; > + ranges.append(StaticRange::createFromRange(*m_rangeToBeCorrected)); > + return ranges; I would write this instead: RefPtr<StaticRange> range = StaticRange::createFromRange(*m_rangeToBeCorrected); return { 1, &range }; > Source/WebCore/editing/TypingCommand.cpp:450 > + Vector<RefPtr<StaticRange>> staticRanges; > + staticRanges.append(StaticRange::createFromRange(*range)); > + return frame().editor().willApplyEditing(*this, WTFMove(staticRanges)); I would write something more like this instead: RefPtr<StaticRange> staticRange = StaticRange::createFromRange(*range); return frame().editor().willApplyEditing(*this, { 1, &staticRanges });

Comment on attachment 292312 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292312&action=review >> Source/WebCore/dom/InputEvent.idl:28 >> Constructor(DOMString type, optional InputEventInit eventInitDict), > > Please keep extended attributes in alphabetical order. Fixed. >> Source/WebCore/dom/StaticRange.h:30 >> +#include <wtf/RefPtr.h> > > Please make the includes be minimal. I do not think Node.h needs to be included, but I am not sure. Would need to try it and see if adding that include elsewhere was sufficient. > > If Node.h is included, then RefCounted.h and RefPtr.h do not need to be included. Good point. Removed Node.h include and moved the implementation of StaticRange::create to StaticRange.cpp. >> Source/WebCore/dom/StaticRange.h:53 >> + RefPtr<Node> m_startContainer; > > This should be Ref<Node>, not RefPtr<Node>. Changed to use Ref<Node> >> Source/WebCore/dom/StaticRange.h:55 >> + RefPtr<Node> m_endContainer; > > Ditto. Changed to use Ref<Node> >> Source/WebCore/editing/CompositeEditCommand.cpp:381 >> + if (auto firstRange = frame().selection().selection().firstRange()) { > > We normally use early return, which means the error case comes first, rather than nesting the normal case inside an if statement. Done. >> Source/WebCore/editing/CompositeEditCommand.cpp:384 >> + return ranges; > > I would write this instead: > > RefPtr<StaticRange> range = StaticRange::createFromRange(*firstRange); > return { 1, &range }; Good call! Done. >> Source/WebCore/editing/CompositeEditCommand.cpp:1542 >> + RefPtr<EditingStyle> style = EditingStyle::create(endingSelection().start()); > > These should use auto. Done. >> Source/WebCore/editing/CompositeEditCommand.h:32 >> +#include "StaticRange.h" > > This include is not needed here. A forward declaration should suffice. Done. >> Source/WebCore/editing/ReplaceRangeWithTextCommand.cpp:80 >> + return ranges; > > I would write this instead: > > RefPtr<StaticRange> range = StaticRange::createFromRange(*m_rangeToBeReplaced); > return { 1, &range }; This is really clean! Changed all similar call sites to do this. >> Source/WebCore/editing/SpellingCorrectionCommand.cpp:126 >> + return ranges; > > I would write this instead: > > RefPtr<StaticRange> range = StaticRange::createFromRange(*m_rangeToBeCorrected); > return { 1, &range }; Done. >> Source/WebCore/editing/TypingCommand.cpp:450 >> + return frame().editor().willApplyEditing(*this, WTFMove(staticRanges)); > > I would write something more like this instead: > > RefPtr<StaticRange> staticRange = StaticRange::createFromRange(*range); > return frame().editor().willApplyEditing(*this, { 1, &staticRanges }); Done.

Created attachment 292347 [details] Patch for landing

Comment on attachment 292347 [details] Patch for landing Clearing flags on attachment: 292347 Committed r207670: <http://trac.webkit.org/changeset/207670>

All reviewed patches have been landed. Closing bug.