1620504 - Improve warning under libeditor for making investigation of web-compat and regressions easier

Status: RESOLVED FIXED

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

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Currently, libeditor uses NS_WARN_IF(NS_FAILED(rv)) in a lot of places, but meaningful information of it is only the line number. So, basically, we should rewrite the warnings as:

nsresult rv = Foo();
if (NS_FAILED(rv)) {
  NS_WARNING("TextEditor::Foo() failed");
  return rv;
}

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Mirko, when are you going to land your MOZ_CAN_RUN_SCRIPT patches for Selection? They may conflict with patches for this bug.

Comment 2

[Mirko Brodesser (:mbrodesser-Igalia)]

Part 1 and part 2 of https://bugzilla.mozilla.org/show_bug.cgi?id=1619617 I intend to land now. Is that OK for you? I don't plan to land other MOZ_CAN_RUN_SCRIPT patches soon (including the one about SelectAllChildren).

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Mirko Brodesser (:mbrodesser) from comment #2)

Part 1 and part 2 of https://bugzilla.mozilla.org/show_bug.cgi?id=1619617 I intend to land now. Is that OK for you? I don't plan to land other MOZ_CAN_RUN_SCRIPT patches soon (including the one about SelectAllChildren).

Yeah, go ahead. Anyway, my patches are risky to land at this moment.

FYI: If you write one line declaration as MOZ_CAN_RUN_SCRIPT void Foo(); in header if possible, developers can check whether the method may run script or not with grep or searchfox's search like:
https://searchfox.org/mozilla-central/search?q=EditorBase%3A%3ADeleteNodeWithTransaction&case=false&regexp=false&path=

Comment 4

[Mirko Brodesser (:mbrodesser-Igalia)]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #3)

(In reply to Mirko Brodesser (:mbrodesser) from comment #2)

Part 1 and part 2 of https://bugzilla.mozilla.org/show_bug.cgi?id=1619617 I intend to land now. Is that OK for you? I don't plan to land other MOZ_CAN_RUN_SCRIPT patches soon (including the one about SelectAllChildren).

Yeah, go ahead. Anyway, my patches are risky to land at this moment.

Ok.

FYI: If you write one line declaration as MOZ_CAN_RUN_SCRIPT void Foo(); in header if possible, developers can check whether the method may run script or not with grep or searchfox's search like:
https://searchfox.org/mozilla-central/search?q=EditorBase%3A%3ADeleteNodeWithTransaction&case=false&regexp=false&path=

Ah, great, I didn't think about it. I assumed the automatic formatting would enforce one style anyway. Thanks!

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 1: Clean up warnings in CSSEditUtils r=m_kato!

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 2: Clean up warnings in ChangeStyleTransaction r=m_kato!

Depends on D65866

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 3: Clean up warnings in CompositionTransaction r=m_kato!

Depends on D65867

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 4: Clean up warnings in CreateElementTransaction r=m_kato!

Depends on D65869

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 5: Clean up warnings in ChangeAttributeTransaction r=m_kato!

Depends on D65870

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 6: Clean up warnings in DeleteNodeTransaction r=m_kato!

Depends on D65871

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 7: Clean up warnings in DeleteRangeTransaction r=m_kato!

Depends on D65872

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 8: Clean up warnings in DeleteTextTransaction r=m_kato!

Depends on D65873

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 9: Clean up warnings in EditAggregateTransaction r=m_kato!

This also changes mChildren to array of OwningNonNull for removing
unnecessary nullptr checks.

Depends on D65874

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 10: Clean up warnings in EditorBase r=m_kato!

Depends on D65875

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 11: Clean up warnings in editor command classes r=m_kato!

Depends on D65876

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI: If you think the warnings are messy when you read the editor code and you use VSCode, install "Highlight" extension and add these lines into your settings.json.

    "highlight.regexFlags": "g",
    "highlight.regexes": {
      "(\\n)([\\s\\t]*(NS_WARNING(_ASSERTION)?|MOZ_ASSERT(_IF)?|NS_ERROR|NS_ASSERTION|MOZ_LOG)\\([^;]*(\\n[^;]*){0,10}\\);)(\\n)": {
        "decorations": [
          {},
          {
            "opacity": "0.5",
          },
          {},
        ],
      },
    },

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c6e0bc9fd0e
part 1: Clean up warnings in CSSEditUtils r=m_kato

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/03cf82cff709
part 2: Clean up warnings in ChangeStyleTransaction r=m_kato

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08f178b30e00
part 3: Clean up warnings in CompositionTransaction r=m_kato

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9aac8d5c94e5
part 4: Clean up warnings in CreateElementTransaction r=m_kato

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3004bc7597a0
part 5: Clean up warnings in ChangeAttributeTransaction r=m_kato

Comment 22

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/5c6e0bc9fd0e
https://hg.mozilla.org/mozilla-central/rev/03cf82cff709
https://hg.mozilla.org/mozilla-central/rev/08f178b30e00
https://hg.mozilla.org/mozilla-central/rev/9aac8d5c94e5

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c9ad4a094e1
part 6: Clean up warnings in DeleteNodeTransaction r=m_kato

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/368f8bcf6a2a
part 7: Clean up warnings in DeleteRangeTransaction r=m_kato

Comment 25

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/3004bc7597a0
https://hg.mozilla.org/mozilla-central/rev/9c9ad4a094e1

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 12: Clean up warnings in EditorEventListener r=m_kato!

Depends on D65877

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 13: Clean up warnings in editor/txmgr r=m_kato!

Depends on D66174

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 14: Clean up warnings in InsertNodeTransaction r=m_kato!

Depends on D66175

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 15: Clean up warnings in InsertTextTransaction r=m_kato!

Depends on D66176

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 16: Clean up warnings in JoinNodeTransaction r=m_kato!

Depends on D66177

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 17: Clean up warnings in PlaceholderTransaction r=m_kato!

Additionally, this makes some nsIAbsorbingTransaction methods infallible for
reducing unnecessary warnings.

Depends on D66178

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b93a79fabc4d
part 8: Clean up warnings in DeleteTextTransaction r=m_kato

Comment 33

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/368f8bcf6a2a

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/209a53e284c0
part 9: Clean up warnings in EditAggregateTransaction r=m_kato

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7e571adbb563
part 10: Clean up warnings in EditorBase r=m_kato

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 18: Clean up warnings in SplitNodeTransaction r=m_kato!

Depends on D66179

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 19: Clean up warnings in WSRunScanner and WSRunObject r=m_kato!

Depends on D66226

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9258b327ce4f
part 11: Clean up warnings in editor command classes r=m_kato

Comment 39

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/b93a79fabc4d
https://hg.mozilla.org/mozilla-central/rev/209a53e284c0
https://hg.mozilla.org/mozilla-central/rev/7e571adbb563

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f4214b66b8e
part 12: Clean up warnings in EditorEventListener r=m_kato

Comment 41

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/9258b327ce4f

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22be6018155b
part 13: Clean up warnings in editor/txmgr r=m_kato

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01fbea31677b
part 14: Clean up warnings in InsertNodeTransaction r=m_kato

Comment 44

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/3f4214b66b8e

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f180663e776
part 15: Clean up warnings in InsertTextTransaction r=m_kato

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/09ab08286858
part 16: Clean up warnings in JoinNodeTransaction r=m_kato

Comment 47

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/22be6018155b
https://hg.mozilla.org/mozilla-central/rev/01fbea31677b
https://hg.mozilla.org/mozilla-central/rev/2f180663e776
https://hg.mozilla.org/mozilla-central/rev/09ab08286858

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/894ed79841fa
part 17: Clean up warnings in PlaceholderTransaction r=m_kato

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fb2b4c3c40ca
part 18: Clean up warnings in SplitNodeTransaction r=m_kato

Comment 50

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/894ed79841fa

Comment 51

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/fb2b4c3c40ca

Comment 52

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 20: Clean up warnings in SelectionState and related classes r=m_kato!

This changes a lot of arguments from pointer to reference.

Depends on D66227

Comment 53

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 21: Clean up warnings in TextEditor

Depends on D66851

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/848201741563
part 19: Clean up warnings in WSRunScanner and WSRunObject r=m_kato

Comment 55

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/848201741563

Comment 56

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 22-1: Clean up warnings in `HTMLEditor.cpp` and `HTMLEditor.h`

Depends on D66852

Comment 57

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 22-2: Clean up warnings in HTMLAbsPositionEditor.cpp

Depends on D66979

Comment 58

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 22-3: Clean up warnings in HTMLAnonymousNodeEditor.cpp

Depends on D66980

Comment 59

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 22-4: Clean up warnings in HTMLEditSubActionHandler.cpp

Depends on D66981

Comment 60

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 22-5: Clean up warnings in HTMLEditorDataTransfer.cpp

Depends on D66983

Comment 61

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 22-6: Clean up warnings in HTMLEditorObjectResizer.cpp

Depends on D66984

Comment 62

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 22-7: Clean up warnings in HTMLInlineTableEditor.cpp

Depends on D66985

Comment 63

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ec64a5f264f
part 20: Clean up warnings in SelectionState and related classes r=m_kato

Comment 64

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/0ec64a5f264f

Comment 65

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 22-8: Clean up warnings in HTMLStyleEditor.cpp

Depends on D66986

Comment 66

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 22-9: Clean up warnings in HTMLTableEditor.cpp

Depends on D67130

Comment 67

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1620504 - part 23: Fix some inconsistent coding style in editor

Depends on D67131

Comment 68

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01641a691d4d
part 21: Clean up warnings in TextEditor r=m_kato

Comment 69

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8780cfdbd06b
part 22-1: Clean up warnings in `HTMLEditor.cpp` and `HTMLEditor.h` r=m_kato

Comment 70

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/51c701a153b7
part 22-2: Clean up warnings in HTMLAbsPositionEditor.cpp r=m_kato

Comment 71

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f03337bce666
part 22-3: Clean up warnings in HTMLAnonymousNodeEditor.cpp r=m_kato

Comment 72

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/01641a691d4d
https://hg.mozilla.org/mozilla-central/rev/8780cfdbd06b

Comment 73

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/51c701a153b7

Comment 74

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/f03337bce666

Comment 75

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c9f5c4e0d563
part 22-4: Clean up warnings in HTMLEditSubActionHandler.cpp r=m_kato

Comment 76

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b118cad2e62
part 22-5: Clean up warnings in HTMLEditorDataTransfer.cpp r=m_kato

Comment 77

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7133c2c0450d
part 22-6: Clean up warnings in HTMLEditorObjectResizer.cpp r=m_kato

Comment 78

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/c9f5c4e0d563
https://hg.mozilla.org/mozilla-central/rev/6b118cad2e62
https://hg.mozilla.org/mozilla-central/rev/7133c2c0450d

Comment 79

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/97d2197a3659
part 22-7: Clean up warnings in HTMLInlineTableEditor.cpp r=m_kato

Comment 80

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8bd3c154bb79
part 22-8: Clean up warnings in HTMLStyleEditor.cpp r=m_kato

Comment 81

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/97d2197a3659
https://hg.mozilla.org/mozilla-central/rev/8bd3c154bb79

Comment 82

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/550f335d1a24
part 22-9: Clean up warnings in HTMLTableEditor.cpp r=m_kato
https://hg.mozilla.org/integration/autoland/rev/e75be1e21827
part 23: Fix some inconsistent coding style in editor r=m_kato

Comment 83

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/550f335d1a24
https://hg.mozilla.org/mozilla-central/rev/e75be1e21827