Closed
Bug 1339014
Opened 8 years ago
Closed 8 years ago
Add IProtocol::GetActorEventTarget() to Retrieve the EventTarget of the Actor if Set.
Categories
(Core :: IPC, defect)
Core
IPC
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: bevis, Assigned: bevis)
References
Details
Attachments
(1 file)
(deleted),
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
I'd like to add this method for easier access to actor's event target, when it it's always set, for the following use cases instead of accessing from {CotentChild::GetSingleton(), BackgroundChild::GetForCurrentThread()}->GetActorEventTarget(*aActor):
1. Make EventTarget::Dispatch() easier in the callback of an actor child.
> BackgroundCursorChild::HandleResponse(const void_t& aResponse)
> {
> ...
> MOZ_ALWAYS_SUCCEEDS(this->GetActorEventTarget()->
> Dispatch(deleteRunnable.forget(), NS_DISPATCH_NORMAL));
> }
2. Set event target to a actor managed by another IToplevelProtocol in the callback of an actor child.
> mozilla::ipc::IPCResult
> BackgroundFactoryRequestChild::RecvPermissionChallenge(
> const PrincipalInfo& aPrincipalInfo)
> {
> ...
> RefPtr<TabChild> tabChild = mFactory->GetTabChild();
> MOZ_ASSERT(tabChild);
>
> IPC::Principal ipcPrincipal(principal);
>
> auto* actor = new PermissionRequestChildProcessActor(this, mFactory);
>
> tabChild->SetEventTargetForActor(actor, this->GetActorEventTarget());
> MOZ_ASSERT(actor->GetActorEventTarget());
> tabChild->SendPIndexedDBPermissionRequestConstructor(actor, ipcPrincipal);
> }
3. Assertion to the actors whose event target was set or shall be inherit from its manager:
> tabChild->SetEventTargetForActor(actor, this->GetActorEventTarget());
> MOZ_ASSERT(actor->GetActorEventTarget());
> tabChild->SendPIndexedDBPermissionRequestConstructor(actor, ipcPrincipal);
Or
> BackgroundDatabaseChild::RecvPBackgroundIDBVersionChangeTransactionConstructor(*aActor)
> {
> ...
> MOZ_ASSERT(aActor->GetActorEventTarget(),
> "The event target shall be inherited from its manager actor.");
> }
Or
> nsresult
> IDBDatabase::Transaction(JSContext* aCx,
> const StringOrStringSequence& aStoreNames,
> IDBTransactionMode aMode,
> IDBTransaction** aTransaction)
> {
> ...
> BackgroundTransactionChild* actor =
> new BackgroundTransactionChild(transaction);
>
> MOZ_ALWAYS_TRUE(
> mBackgroundActor->SendPBackgroundIDBTransactionConstructor(actor,
> sortedStoreNames,
> mode));
> MOZ_ASSERT(actor->GetActorEventTarget(),
> "The event target shall be inherited from it manager actor.");
> }
Assignee | ||
Comment 1•8 years ago
|
||
Please see the reasons why this method is needed in comment 0.
Note: a raw nsIEventTarget* is returned instead of the already_addRefed<> one to make the code in call sites more compact. Raw pointer shall be fine because an assertion was added internally which assumes that the EventTarget that was set earlier could be retrieved during the actor's life cycle.
In addition, I assume that the EventTarget of IToplevelProtocol shall never be set according to current implementation to make the override of this method in IToplevelProtocol clear by returning nullptr. Maybe we could override IToplevelProtocol::{GetActorEventTarget(), SetEventTargetForActor(aActor, aEventTarget)} with MOZ_CRASH() instead if it makes more sense.
Attachment #8836637 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 2•8 years ago
|
||
Comment on attachment 8836637 [details] [diff] [review]
Patch: Add IProtocol::GetActorEventTarget() to Retrieve the EventTarget of the Actor if Set.
Review of attachment 8836637 [details] [diff] [review]:
-----------------------------------------------------------------
::: ipc/glue/ProtocolUtils.h
@@ +380,5 @@
>
> virtual void SetEventTargetForActorInternal(IProtocol* aActor, nsIEventTarget* aEventTarget);
>
> + virtual already_AddRefed<nsIEventTarget>
> + GetActorEventTargetInternal(IProtocol* aActor) override;
The *override* here introduces more compiling errors to other member functions that *override* was not explicitly added (-Winconsistent-missing-override):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba8537a80e3672f26b10d9c9b8f4f2c236f240f0&selectedJob=76788640
I should remove it for now in next update.
Attachment #8836637 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 3•8 years ago
|
||
treeherder result looks fine:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=15cc238c59129b510438fe5aa5ec67c73049620a
Pushed by btseng@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/515db19cf529
Add IProtocol::GetActorEventTarget() to Retrieve the EventTarget of the Actor if Set. r=billm
Comment 5•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•