Closed Bug 1504643 Opened 3 years ago Closed 3 years ago
Promise Holder::Resolve/Reject bypasses |explicit| keyword somehow
47 bytes, text/x-phabricator-request
|Details | Review|
Similaire to bug 1331137 and observed in bug 1504531. We have a MediaDataDecoder::DecodePromise which is defined as: typedef nsTArray<RefPtr<MediaData>> DecodedData; typedef MozPromise<DecodedData, MediaResult, /* IsExclusive = */ true> DecodePromise; In various places of the code we do: https://searchfox.org/mozilla-central/rev/efc0d9172cb6a5849c6c4fc0f19d7fd5a2da9643/dom/media/platforms/agnostic/eme/EMEDecoderModule.cpp#357 MozPromiseHolder<DecodePromise>::Resolve(aResults) where aResults is a const DecodedData& e.g. const nsTArray<RefPtr<MediaDadata>>& In bug 15404531 we attempted to use forward references throughout the MozPromise code to simplify things. However, adding static_assert to ensure the Resolve/Reject are called with the right arguments as requested by :gerald suddenly enforced the explicit nsTArray copy constructor and caused many compilation errors wherever DecodePromise were used. So at present, a lot of nsTArray copies are made implicitly when the nsTArray request that such copy be made explicitly. So let's fix this, either by allowing implicit copy, or explicitly change all the callers so that the nsTArray copy is now explicit
I think that templated classes like MozPromise should in general honor their template-parameter classes restrictions. So I would personally recommend going for the IsConvertible test. This means having to change lots of callers to do an explicit copy or conversion at the call site -- Which I think is a good thing as it forces the developer to think about that case, and it indicates to reviewers and maintainers that this copy/conversion was deemed necessary and acceptable. An alternative (considered in bug 1331137) would be to provide a different function (e.g., `template<typename... Ts> void EmplaceResolve(const char*, Ts&&...)`) which would both make the copy implicit, and allow in-place construction from a variadic parameter pack. In this case, "bad" call sites would only need to change which function is called instead of having to rework the whole argument temporary copy.
Make thorough use of move semantic for MozPromise using nsTArray
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/157be32d85fe Enforce template-parameter classes restrictions in MozPromise. r=gerald
Backout by email@example.com: https://hg.mozilla.org/integration/autoland/rev/2f55b073ab55 Backed out changeset 157be32d85fe for conflicts while backing out bug 1471535
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/23849b2015ce Enforce template-parameter classes restrictions in MozPromise. r=gerald
You need to log in before you can comment on or make changes to this bug.