1541144 - Port Bug 1540965 Make nsIControllerContext builtinclass - remove call to nsIControllerContext.init()

Status: RESOLVED FIXED

(MailNews Core :: General, task)

Description

[Jorg K (CEST = GMT+2)]

See bug 1540965 comment #0:

https://searchfox.org/comm-central/rev/4fd2bb880f8af2c7979b691eea616f587a9f2d4b/editor/ui/composer/content/ComposerCommands.js#126
https://searchfox.org/comm-central/rev/4fd2bb880f8af2c7979b691eea616f587a9f2d4b/editor/ui/composer/content/ComposerCommands.js#198
https://searchfox.org/comm-central/rev/4fd2bb880f8af2c7979b691eea616f587a9f2d4b/editor/ui/composer/content/editingOverlay.js#74

Comment 1

[Jorg K (CEST = GMT+2)]

FRG and Ian, is this only used for SM?

Comment 2

[Ian Neal]

(In reply to Jorg K (GMT+2) from comment #1)

FRG and Ian, is this only used for SM?
Yes, looks like it is used by Composer / DebugQA

Comment 3

[Ian Neal]

Attachment: Remove init() lines

Thanks for looking at the callers, Masakuki-san. Is this all that is required?

Comment 4

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

Comment on attachment 9055252 [details] [diff] [review]
Remove init() lines

Thanks! And sorry for doing such change.

Comment 5

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9055252 [details] [diff] [review]
Remove init() lines

Looks good. Thanks.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d5c0b943ee9b
Port bug 1540965: Make nsIControllerContext builtinclass - remove calls to nsIControllerContext.init(). r=frg

Comment 7

[Jorg K (CEST = GMT+2)]

Which version are you at? TB 68 is equivalent to SM 2.65, no?

Comment 8

[Jorg K (CEST = GMT+2)]

There's more to do here:

C++ bustage here:
https://searchfox.org/comm-central/rev/10bc0c1569fab6ccdce1c31413067d1ec0de8c61/common/src/nsCommonModule.cpp#18

nsCommonModule.cpp(18,32): error: call to constructor of 'nsBaseCommandController' is ambiguous
0:18.05 NS_GENERIC_FACTORY_CONSTRUCTOR(nsBaseCommandController)

I'll have to see how to fix that. The constructor now takes a command table:
https://hg.mozilla.org/mozilla-central/rev/42b4d2faa543#l2.14

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1541144-nsBaseCommandController.patch

OK, this compiles and should be right.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/270dd6e22821
Port bug 1540965: Hand-roll NS_GENERIC_FACTORY_CONSTRUCTOR(nsBaseCommandController). rs=bustage-fix

Comment 11

[Jorg K (CEST = GMT+2)]

Landed without the trailing space ;-)

Comment 12

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

Comment on attachment 9055732 [details] [diff] [review]
1541144-nsBaseCommandController.patch

That's really odd to me because [the default constructor is removed by `= delete`](https://searchfox.org/mozilla-central/rev/9ee63566281365f26e7a4b06c9d4e2219e64c3e8/dom/commandhandler/nsBaseCommandController.h#28). But fine to me for now.

Comment 13

[Boris Zbarsky [:bzbarsky]]

That's really odd to me because [the default constructor is removed by = delete]

It looks like C++ considers that to be a constructor declaration (albeit a deleted one) and then is not sure whether you're calling the deleted constructor (an error) or the one-arg one with no args.

We should just remove that "nsBaseCommandController() = delete;" line.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9055732 [details] [diff] [review]
1541144-nsBaseCommandController.patch

See comment 13 for what is probably the right fix.

Comment 15

[Jorg K (CEST = GMT+2)]

Thanks, Boris.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #13)

It looks like C++ considers that to be a constructor declaration (albeit a deleted one)
and then is not sure whether you're calling the deleted constructor (an error) or the
one-arg one with no args.

Why with no args?
https://hg.mozilla.org/comm-central/rev/270dd6e22821#l1.20
inst = new nsBaseCommandController(new nsControllerCommandTable());

We should just remove that "nsBaseCommandController() = delete;" line.
OK, who can take this on? As far as I can see, I'm calling this one:
https://searchfox.org/mozilla-central/rev/9ee63566281365f26e7a4b06c9d4e2219e64c3e8/dom/commandhandler/nsBaseCommandController.h#29

Comment 16

[Boris Zbarsky [:bzbarsky]]

Why with no args?

That's what the code was doing before the changes in this bug.

Comment 17

[Jorg K (CEST = GMT+2)]

Sure, but now it calls the one-arg version with one argument, no? Am I missing something? It's ugly that the macro was hand-rolled, not not a drama. If you prefer, I can open a bug to remove the one line nsBaseCommandController() = delete;. But you (author and reviewer) put it there for a reason, right?

Comment 18

[Boris Zbarsky [:bzbarsky]]

The patch I reviewed did not have that line. It had:

  nsBaseCommandController();
  explicit nsBaseCommandController(nsControllerCommandTable* aControllerCommandTable);

and my review comments said to just have the second one with a default value for the arg. The fact that the = delete; line is still there is a mistake.

I mean, you can have the unrolled macro in comm-central if you want, it's your code. But the m-c code is wrong anyway: it was intended to allow calling the constructor with no args and it's not doing that.

Comment 19

[Jorg K (CEST = GMT+2)]

Let's be pragmatic and take the line out, then back out the patch here.

Comment 20

[Jorg K (CEST = GMT+2)]

We can back this out when bug 1542352 lands.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ae01508231f6
Backed out changeset 270dd6e22821. Not necessary any more since bug 1542352 restored 0-arg CTOR of nsBaseCommandController. a=backout