1448850 - Remove some message manager XPCOM interfaces

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: Remove nsIContentProcessMessageManager

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: Remove nsIGlobalProcessScriptLoader

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: Remove nsIProcessScriptLoader

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: Remove nsIFrameScriptLoader

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: Empty nsIContentFrameMessageManager and remove nsIInProcessContentFrameMessageManager::CacheFrameLoader

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: Remove nsIMessageManagerGlobal

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: Remove nsISyncMessageSender

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: Remove nsIMessageBroadcaster

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: Empty nsIMessageSender

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: Remove nsIMessageListener and nsIMessageListenerManager

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8962391 [details] [diff] [review]
Remove nsIMessageListener and nsIMessageListenerManager

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

::: dom/base/nsIMessageManager.idl
@@ -16,5 @@
>  // codegen doesn't try to do anything with it.
>  native nativeFrameLoader(nsFrameLoader*);
>  
>  /**
> - * Message managers provide a way for chrome-privileged JS code to

Can we preserve these comments about the message manager design somewhere?  I have found them quite useful as a reference.

Perhaps they could go in dom/chrome-webidl/MessageManager.webidl?

Apologies if a separate patch has copied them somewhere; I scanned quite quickly.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8962334 [details] [diff] [review]
Remove nsIContentProcessMessageManager

>Bug 1448850 - Remove some message manager XPCOM interfaces. Remove nsIContentProcessMessageManager.

Not sure it's worth preserving the bug summary in every commit message...

>+  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIMessageManagerGlobal)

We don't actually QI to nsIMessageManagerGlobal!

Not going to worry about it, since you remove it just down the line a bit.  ;)

r=me

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8962335 [details] [diff] [review]
Remove nsIGlobalProcessScriptLoader

>+    sParentProcessManager->GetInitialProcessData(aCx, &init, IgnoreErrors());

Are we guaranteed this won't throw?  Some of the codepaths in there just NoteJSContextException(), which means we could leave a dangling exception on aCx...

It would make more sense to me to pass aError here and if it fails return immediately.

>+        rv.SuppressException();

Could just use an IgnoredErrorResult to start with, so exceptions are auto-suppressed, since that's the semantic we want here.

r=me with that bit about IgnoreErrors() fixed.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8962343 [details] [diff] [review]
Remove nsIProcessScriptLoader

r=me

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8962344 [details] [diff] [review]
Remove nsIFrameScriptLoader

>-    LoadFrameScript(mPendingScripts[i], false, mPendingScriptsGlobalStates[i]);
>+    LoadScript(mPendingScripts[i], false, mPendingScriptsGlobalStates[i], IgnoreErrors());

Why this change?

Same in the other place in nsFrameMessageManager::LoadPendingScripts.

r=me if the commit message is fixed to answer this question.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8962345 [details] [diff] [review]
Empty nsIContentFrameMessageManager and remove nsIInProcessContentFrameMessageManager::CacheFrameLoader

r=me

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8962349 [details] [diff] [review]
Remove nsIMessageManagerGlobal

r=me

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8962350 [details] [diff] [review]
Remove nsISyncMessageSender

r=me

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8962384 [details] [diff] [review]
Remove nsIMessageBroadcaster

>+++ b/dom/base/ChromeMessageBroadcaster.h
>+  static ChromeMessageBroadcaster* From(nsFrameMessageManager* aManager)
...
>+    if (aManager->IsBroadcaster() && aManager->IsChrome()) {
>+      return static_cast<ChromeMessageBroadcaster*>(aManager);

Can we add an assert that aFlags in the ChromeMessageSender ctor never contains MM_BROADCASTER, just to make sure we can't land in here with a ChromeMessageSender of some sort?

>+++ b/dom/base/nsGlobalWindowInner.cpp
> nsGlobalWindowInner::GetGroupMessageManager(const nsAString& aGroup,
>                                             ErrorResult& aError)

aError seems unused here.  Remove, please?

>+++ b/dom/base/nsGlobalWindowOuter.cpp

So why does GetMessageManager() just return null while GetGroupMessageManager() throws in the no-inner-window case?

>+++ b/dom/base/nsIMessageManager.idl
>- * Message "broadcasters" don't have a single "other side" that they
>- * send messages to, but rather a set of subordinate message managers.
>- * For example, broadcasting a message through a window message
>- * manager will broadcast the message to all frame message managers
>- * within its window.

Please move this comment to the webidl instead of just deleting it.

r=me with those fixed.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8962387 [details] [diff] [review]
Empty nsIMessageSender

>+++ b/dom/base/nsFrameLoader.cpp
>+already_AddRefed<mozilla::dom::MessageSender>

This file "uses namespace mozilla::dom", so drop that bit.

r=me

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8962391 [details] [diff] [review]
Remove nsIMessageListener and nsIMessageListenerManager

>+++ b/dom/base/nsIMessageManager.idl

Please move the overall descriptive comment to MessageManager.webidl, with edits to reflect actual class names.

r=me with that.

Comment 22

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #19)
> Comment on attachment 8962384 [details] [diff] [review]
> Remove nsIMessageBroadcaster

> Can we add an assert that aFlags in the ChromeMessageSender ctor never
> contains MM_BROADCASTER, just to make sure we can't land in here with a
> ChromeMessageSender of some sort?

ChromeMessageSender already has

  MOZ_ASSERT(!(aFlags & ~(MessageManagerFlags::MM_GLOBAL |
                          MessageManagerFlags::MM_PROCESSMANAGER |
                          MessageManagerFlags::MM_OWNSCALLBACK)));

Comment 23

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/79f16e24a288ded97b3b942e71ebfe74f8e7425a
Bug 1448850 - Remove nsIContentProcessMessageManager. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/359e001359498ad8c11e440dfa003a761ba866a1
Bug 1448850 - Remove nsIGlobalProcessScriptLoader. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ccff2e9f083ab33b0be2970f93eeb2b04027ae7
Bug 1448850 - Remove nsIProcessScriptLoader. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/34e39e0ddbec57250220703aa17393033ec9cda2
Bug 1448850 - Remove nsIFrameScriptLoader. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d179fc641fe6f09e207b810a3561c79cde1d0f6
Bug 1448850 - Empty nsIContentFrameMessageManager and remove nsIInProcessContentFrameMessageManager::CacheFrameLoader. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/78f654af7e6f8af6b0349bfe9c56668bb5246510
Bug 1448850 - Remove nsIMessageManagerGlobal. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdc39e79c58b518d2c37658f4bcba06ab5c9680b
Bug 1448850 - Remove nsISyncMessageSender. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2f3dafc8ca0b995f49e74ceee51eddab74ffd94
Bug 1448850 - Remove nsIMessageBroadcaster. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f0042d44d22771cbb428a6f885becc1f1f61f63
Bug 1448850 - Empty nsIMessageSender. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8eff76f4ae6d9cc2166fb04b69331de96a5134b
Bug 1448850 - Remove nsIMessageListener and nsIMessageListenerManager. r=bz.

Comment 24

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/79f16e24a288
https://hg.mozilla.org/mozilla-central/rev/359e00135949
https://hg.mozilla.org/mozilla-central/rev/5ccff2e9f083
https://hg.mozilla.org/mozilla-central/rev/34e39e0ddbec
https://hg.mozilla.org/mozilla-central/rev/0d179fc641fe
https://hg.mozilla.org/mozilla-central/rev/78f654af7e6f
https://hg.mozilla.org/mozilla-central/rev/bdc39e79c58b
https://hg.mozilla.org/mozilla-central/rev/f2f3dafc8ca0
https://hg.mozilla.org/mozilla-central/rev/9f0042d44d22
https://hg.mozilla.org/mozilla-central/rev/b8eff76f4ae6