Closed Bug 1274466 Opened 4 years ago Closed 4 years ago

Discussion about class access modifiers for virtual member functions in sub-classes

Categories

(Core :: Audio/Video: Playback, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

Details

Attachments

(1 file)

https://reviewboard.mozilla.org/r/53108/diff/1#index_header

In bug 1273390 P1, I moved some functions to the private section to uphold the principle of least privilege since we should always access those functions (Input(), Flush(), ...) from the interface of base class (MediaDataDecoder).

However, jya found it confusing/inconsistent seeing those functions are public in the base class while private in the derived class.

I would like to know if we have rules/styles about such patterns. If no, I want to get a consensus from people working on media code to eliminate the inconsistency.
Hi Gerald,
Since you are language expert in C++, I would like to hear from you about the opinions on this issue.
Thanks.
Flags: needinfo?(gsquelart)
I'm more an enthusiast than an expert!

At a glance this pattern seems fine to me, if you want to enforce access to objects through references/pointers to the base class.
Now I'm not too sure if there's a real benefit to it.
What are you really trying to achieve?

And it could have some drawbacks, say when you have a multi-level hierarchy, and you allow casting from a base class to a derived class (e.g. a TrackInfo can be retrieved as a VideoInfo or AudioInfo), in which case the private overrides would prevent using these APIs through the derived class (but still possible through base-class-casting).

A bit of googling led me to this opinionated (but unexplained) FAQ on the official ISO C++ website:
https://isocpp.org/wiki/faq/proper-inheritance#hiding-inherited-public
A bit more details in this SO answer: http://stackoverflow.com/a/484736/42690

So based on all this, I would probably prefer keeping overrides at the same access level as their parent declaration.
Unless you can think of a really compelling argument in your case.
Flags: needinfo?(gsquelart)
Thanks for the info. I think the main issue is that it is a design issue because it breaks Liskov Substitution Prinicple. I will open a new bug to move them back to the public section.
https://reviewboard.mozilla.org/r/58122/#review54992

::: dom/media/platforms/apple/AppleVDADecoder.h:118
(Diff revision 1)
>    // Number of times a sample was queued via Input(). Will be decreased upon
>    // the decoder's callback being invoked.
>    // This is used to calculate how many frames has been buffered by the decoder.
>    Atomic<uint32_t> mQueuedSamples;
>  
>  private:

it's the same here... those should be protected, not private

::: dom/media/platforms/apple/AppleVDADecoder.h:119
(Diff revision 1)
>    // the decoder's callback being invoked.
>    // This is used to calculate how many frames has been buffered by the decoder.
>    Atomic<uint32_t> mQueuedSamples;
>  
>  private:
>    // Flush and Drain operation, always run

side fix, seems that the comment is incomplete, Im guessing it was to be always run in the TaskQueue

::: dom/media/platforms/apple/AppleVTDecoder.h:36
(Diff revision 1)
>      return mIsHardwareAccelerated
>        ? "apple hardware VT decoder"
>        : "apple software VT decoder";
>    }
>  
> +private:

those shouldn't be private either, but protected
Comment on attachment 8760549 [details]
Bug 1274466 - per discussion move some functions back to public. .

https://reviewboard.mozilla.org/r/58122/#review54994
Attachment #8760549 - Flags: review?(jyavenard) → review+
https://reviewboard.mozilla.org/r/58122/#review54992

> it's the same here... those should be protected, not private

Why? Those members are not gonna be accessed by sub-classes.

> those shouldn't be private either, but protected

Why is that? AppleVTDecoder has no sub-class at all.
for the same reason the public virtual was made public in the child class.

To me the logic is identical, you have a virtual override in a subclass.

so Processblah() (Flush, Drain Shutdown) should be protected
Public members are part of the interface and we don't want to break Liskov Substitution Prinicple. However, it is a different story for protected/private members. Members are made protected only when they will be accessed from the sub-classes.

I guess this is another topic we need to discuss about.
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef1c787b08dc
per discussion move some functions back to public. r=jya.
https://hg.mozilla.org/mozilla-central/rev/ef1c787b08dc
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.