Closed Bug 1757911 Opened 4 years ago Closed 4 years ago

Clean up `SplitNodeTransaction`

Categories

(Core :: DOM: Editor, task, P3)

Product:
Core
Component:
DOM: Editor
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
100 Branch
Tracking Flags:
Tracking Status
firefox100 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(4 files)

Bug 1757911 - part 1: Clean up `SplitNodeTransaction` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1757911 - part 2: Make `SplitNodeTransaction::DoTransaction` and `RedoTransaction` share the code doing split r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1757911 - part 3: Make `HTMLEditor::DoJoinNodes()` notify `RangeUpdater` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1757911 - part 4: Make `HTMLEditor::DoSplitNode` notify `RangeUpdater` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review

For making the coming patches for bug 1735608 smaller, I'd like to clean up SplitNodeTransaction in this bug.

First, it should not store split point with EditorDOMPoint because it has
child node pointer and other flags but they are never used and they make
the footprint worse. Therefore, this patch makes it exactly same structure
as JoinNodesTransaction.

Additionally, this patch removes "left" and "right" words especially from
the member names and variable names (preparation of bug 1735608).

They have duplicated code so that they should be merged to a new internal
method.

Depends on D140461

Currently, it's notified by JoinNodesTransaction::DoTransactionInternal so
that it's SplitNodeTransaction::UndoTransaction cannot track the DOM point
and this might cause odd editing result if execCommand("undo") is called
while HTMLEditor handles an edit action.

For avoiding to duplicate the code, it should be handled in
HTMLEditor::DoJoinNodes.

Depends on D140462

Similar to the previous patch, in the splitting a node case,
SplitNodeWithTransaction notifies RangeUpdater. Currently, this must not
cause any trouble, but for consistency with DoJoinNodes, DoSplitNode does
it instead.

Different from the previous patch, SelAdjSplitNode is not called if splitting
the node fails. Therefore, it's enough to do it at end of DoSplitNode.

Depends on D140463

Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/e0e951dc9594 part 1: Clean up `SplitNodeTransaction` r=m_kato
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/64ae36546100 part 2: Make `SplitNodeTransaction::DoTransaction` and `RedoTransaction` share the code doing split r=m_kato
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/1edde90f24cc part 3: Make `HTMLEditor::DoJoinNodes()` notify `RangeUpdater` r=m_kato
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/0a12415ff7cf part 4: Make `HTMLEditor::DoSplitNode` notify `RangeUpdater` r=m_kato

Backed out for causing build bustages on HTMLEditor.cpp.

Push with failures

Failure log

Backout link

[task 2022-03-10T04:23:46.819Z] gmake[4]: Leaving directory '/builds/worker/workspace/obj-analyzed-browser/gfx/skia'
[task 2022-03-10T04:23:46.860Z] gmake[4]: *** Deleting file 'Unified_cpp_editor_libeditor1.o'
[task 2022-03-10T04:23:46.866Z] gmake[4]: Entering directory '/builds/worker/workspace/obj-analyzed-browser/editor/libeditor'
[task 2022-03-10T04:23:46.871Z] /builds/worker/fetches/sixgill/usr/libexec/sixgill/scripts/wrap_gcc/basecc /builds/worker/fetches/gcc/bin/g++ --sysroot /builds/worker/fetches/sysroot-x86_64-linux-gnu -std=gnu++17 -isystem /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/c++/7.5.0 -isystem /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/x86_64-linux-gnu/c++/7.5.0 -isystem /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/x86_64-linux-gnu -isystem /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include -o Unified_cpp_editor_libeditor1.o -c  -I/builds/worker/workspace/obj-analyzed-browser/dist/stl_wrappers -I/builds/worker/workspace/obj-analyzed-browser/dist/system_wrappers -include /builds/worker/checkouts/gecko/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -DDEBUG=1 -DOS_POSIX=1 -DOS_LINUX=1 -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -I/builds/worker/checkouts/gecko/editor/libeditor -I/builds/worker/workspace/obj-analyzed-browser/editor/libeditor -I/builds/worker/checkouts/gecko/dom/base -I/builds/worker/checkouts/gecko/dom/html -I/builds/worker/checkouts/gecko/extensions/spellcheck/src -I/builds/worker/checkouts/gecko/layout/generic -I/builds/worker/checkouts/gecko/layout/style -I/builds/worker/checkouts/gecko/layout/tables -I/builds/worker/checkouts/gecko/layout/xul -I/builds/worker/workspace/obj-analyzed-browser/ipc/ipdl/_ipdlheaders -I/builds/worker/checkouts/gecko/ipc/chromium/src -I/builds/worker/workspace/obj-analyzed-browser/dist/include -I/builds/worker/workspace/obj-analyzed-browser/dist/include/nspr -I/builds/worker/workspace/obj-analyzed-browser/dist/include/nss -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-analyzed-browser/mozilla-config.h -Wno-attributes -Wno-ignored-attributes -Wall -Wempty-body -Wignored-qualifiers -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wno-invalid-offsetof -Wduplicated-cond -Wimplicit-fallthrough -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=coverage-mismatch -Wno-error=free-nonheap-object -Wno-multistatement-macros -Wno-error=class-memaccess -Wno-error=deprecated-copy -Wformat -Wformat-overflow=2 -Wno-psabi -fno-sized-deallocation -fno-aligned-new -Wno-attributes -Wno-ignored-attributes -fno-exceptions -fPIC -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -freorder-blocks -Os -fno-omit-frame-pointer -funwind-tables -Werror -fno-strict-aliasing  -MD -MP -MF .deps/Unified_cpp_editor_libeditor1.o.pp   Unified_cpp_editor_libeditor1.cpp
[task 2022-03-10T04:23:46.871Z] In file included from Unified_cpp_editor_libeditor1.cpp:29:
[task 2022-03-10T04:23:46.871Z] /builds/worker/checkouts/gecko/editor/libeditor/HTMLEditor.cpp: In lambda function:
[task 2022-03-10T04:23:46.871Z] /builds/worker/checkouts/gecko/editor/libeditor/HTMLEditor.cpp:4837:51: error: expected '{' before '->' token
[task 2022-03-10T04:23:46.871Z]  4837 |   nsresult rv = [&]() MOZ_CAN_RUN_SCRIPT_BOUNDARY -> nsresult {
[task 2022-03-10T04:23:46.871Z]       |                                                   ^~
[task 2022-03-10T04:23:46.871Z] /builds/worker/checkouts/gecko/editor/libeditor/HTMLEditor.cpp: In member function 'nsresult mozilla::HTMLEditor::DoJoinNodes(nsIContent&, nsIContent&)':
[task 2022-03-10T04:23:46.872Z] /builds/worker/checkouts/gecko/editor/libeditor/HTMLEditor.cpp:4837:51: error: base operand of '->' has non-pointer type 'mozilla::HTMLEditor::DoJoinNodes(nsIContent&, nsIContent&)::<lambda()>'
[task 2022-03-10T04:23:46.872Z] In file included from /builds/worker/checkouts/gecko/layout/generic/nsIFrame.h:53,
[task 2022-03-10T04:23:46.872Z]                  from /builds/worker/checkouts/gecko/dom/base/nsIContentInlines.h:14,
[task 2022-03-10T04:23:46.872Z]                  from /builds/worker/workspace/obj-analyzed-browser/dist/include/mozilla/EditorBase.h:30,
[task 2022-03-10T04:23:46.873Z]                  from /builds/worker/checkouts/gecko/editor/libeditor/HTMLEditor.h:12,
[task 2022-03-10T04:23:46.873Z]                  from /builds/worker/checkouts/gecko/editor/libeditor/HTMLEditHelpers.cpp:9,
[task 2022-03-10T04:23:46.873Z]                  from Unified_cpp_editor_libeditor1.cpp:2:
[task 2022-03-10T04:23:46.873Z] /builds/worker/workspace/obj-analyzed-browser/dist/include/FrameProperties.h: In instantiation of 'static mozilla::FrameProperties::PropertyType<T> mozilla::FrameProperties::ReinterpretHelper<T>::FromInternalValue(uint64_t) [with T = mozilla::SmallValueHolder<mozilla::FrameBidiData>; mozilla::FrameProperties::PropertyType<T> = mozilla::FrameBidiData; uint64_t = long unsigned int]':
[task 2022-03-10T04:23:46.873Z] /builds/worker/workspace/obj-analyzed-browser/dist/include/FrameProperties.h:202:51:   required from 'mozilla::FrameProperties::PropertyType<T> mozilla::FrameProperties::Get(mozilla::FrameProperties::Descriptor<T>, bool*) const [with T = mozilla::SmallValueHolder<mozilla::FrameBidiData>; mozilla::FrameProperties::PropertyType<T> = mozilla::FrameBidiData; mozilla::FrameProperties::Descriptor<T> = const mozilla::FramePropertyDescriptor<mozilla::SmallValueHolder<mozilla::FrameBidiData> >*]'
[task 2022-03-10T04:23:46.873Z] /builds/worker/checkouts/gecko/layout/generic/nsIFrame.h:4117:51:   required from 'mozilla::FrameProperties::PropertyType<T> nsIFrame::GetProperty(mozilla::FrameProperties::Descriptor<T>, bool*) const [with T = mozilla::SmallValueHolder<mozilla::FrameBidiData>; mozilla::FrameProperties::PropertyType<T> = mozilla::FrameBidiData; mozilla::FrameProperties::Descriptor<T> = const mozilla::FramePropertyDescriptor<mozilla::SmallValueHolder<mozilla::FrameBidiData> >*]'
[task 2022-03-10T04:23:46.874Z] /builds/worker/checkouts/gecko/layout/generic/nsIFrame.h:1327:78:   required from here
[task 2022-03-10T04:23:46.874Z] /builds/worker/workspace/obj-analyzed-browser/dist/include/FrameProperties.h:302:13: warning: 'void* memcpy(void*, const void*, size_t)' copying an object of non-trivial type 'mozilla::FrameProperties::PropertyType<mozilla::SmallValueHolder<mozilla::FrameBidiData> >' {aka 'struct mozilla::FrameBidiData'} from an array of 'uint64_t' {aka 'long unsigned int'} [-Wclass-memaccess]
[task 2022-03-10T04:23:46.874Z]   302 |       memcpy(&value, &aInternalValue, sizeof(value));
[task 2022-03-10T04:23:46.874Z]       |       ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2022-03-10T04:23:46.874Z] In file included from /builds/worker/checkouts/gecko/dom/base/nsIContentInlines.h:14,
[task 2022-03-10T04:23:46.874Z]                  from /builds/worker/workspace/obj-analyzed-browser/dist/include/mozilla/EditorBase.h:30,
<...>
Flags: needinfo?(masayuki)

It seems that it's a bug of the compiler... According to the document, it uses GCC instead of clang.

Flags: needinfo?(masayuki)
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/11ede8a9182f part 1: Clean up `SplitNodeTransaction` r=m_kato
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/2155ccd6f6b4 part 2: Make `SplitNodeTransaction::DoTransaction` and `RedoTransaction` share the code doing split r=m_kato
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/b7b1979c7985 part 3: Make `HTMLEditor::DoJoinNodes()` notify `RangeUpdater` r=m_kato
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/8fb082fb987e part 4: Make `HTMLEditor::DoSplitNode` notify `RangeUpdater` r=m_kato
Regressed by: 1820905
No longer regressed by: 1820905
Regressions: 1820905
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: