1540398 - Port bug 1539356: More functions in Core::Editor are MOZ_CAN_RUN_SCRIPT now

Status: RESOLVED FIXED

(MailNews Core :: General, task)

Description

[Jorg K (CEST = GMT+2)]

We can prepare for this by applying
https://hg.mozilla.org/integration/autoland/rev/c7ec5a129506

Comment 1

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

Sorry for making your unexpected job due this kind of bugs. I guess that this is the biggest change because a lot of methods are now marked as MOZ_CAN_RUN_SCRIPT. Perhaps, follow up patches would reach one of them in most cases.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1540398-MOZ_CAN_RUN_SCRIPT.patch

It's no problem, I can easily follow this. And I see the errors in the local compile.

How do you mark nsMsgCompose::SetIdentity() properly, which comes from
https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mailnews/compose/public/nsIMsgCompose.idl#184
attribute nsIMsgIdentity identity;?

Sorry about the messy patch, I fixed some NS_IMETHOD(IMP) declarations which were wrong.

Comment 3

[Jorg K (CEST = GMT+2)]

Let's see that it compiles on Linux, too:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1fdf52fbabbde210c6d59d9acda3a3c155ede24c

Note the pinning to autoland here:
https://hg.mozilla.org/try-comm-central/rev/1fdf52fbabbde210c6d59d9acda3a3c155ede24c#l1.6

Comment 4

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

If you cannot use [can_run_script] for attributes, use MOZ_CAN_RUN_SCRIPT_FOR_DEFINITION.
https://searchfox.org/mozilla-central/rev/73e1da0906e711285c15b0fa5472263c01cbd3f2/layout/base/gtest/TestAccessibleCaretEventHub.cpp#267

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 9054685 [details] [diff] [review]
1540398-MOZ_CAN_RUN_SCRIPT.patch

Thanks, but that doesn't compile:
error: functions marked as MOZ_CAN_RUN_SCRIPT cannot override functions that are not marked MOZ_CAN_RUN_SCRIPT
Something needs to go into the IDL. Looks like you can actually put `[can_run_script]` in front of an attribute. That compiles, but later on, I get problems in JS Account, the same which are described in bug 1536535. So we need so stay with `MOZ_CAN_RUN_SCRIPT_BOUNDARY` for now.

I won't burden you with the review, I'll move it to BenC. Anyway, that needs to land soon as "bustage fix".

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cb02187d7568
Port bug 1539356: mark more functions MOZ_CAN_RUN_SCRIPT. rs=bustage-fix

Comment 7

[Ben Campbell]

Comment on attachment 9054685 [details] [diff] [review]
1540398-MOZ_CAN_RUN_SCRIPT.patch

Review of attachment 9054685 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.
(And I'm curious now to know how many other places NS_IMETHOD is used erroneously!).

Comment 8

[Jorg K (CEST = GMT+2)]

MS VS 2015(?) used to complain since NS_IMETHOD and nsresult are different types. I wonder why it didn't complain back then.