pinkshirt / firebreath

Automatically exported from code.google.com/p/firebreath
0 stars 0 forks source link

PluginCore::initDefaultParams() can't be effectively overriden by subclasses #122

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?
1. subclass PluginCore
2. override initDefaultParams() {  m_supportedParamSet.insert("hello"); }
3. later try to list m_supportedParamSet

What is the expected output? What do you see instead?
you should expect m_supportedParamSet to contain just 1 element, "Hello",
but instead you still get 3 elements, "onload", "onerror", "onlog" which are 
the default specified in PluginCore::initDefaultParams.

What version of FireBreath are you using? On what operating system and
browsers?

bug is present in both git trunk and release 1.3.2a version. 

Please provide any additional information below.

reason is that method PluginCore::initDefaultParams() is called from 
PluginCore's constructor(s). C++ specifies that virtual method table at 
costructing (and destructing) stages is consistent with the object that is 
being constructed. In other words it means that virtual methods behave as if 
they aren't virtual if called from constructor or destructor.

Original issue reported on code.google.com by l.bartol...@gmail.com on 21 Dec 2010 at 6:05

GoogleCodeExporter commented 9 years ago
This is not a bug and there is no "fix" for this.  The correct way to add 
params is to do so in the constructor.

Is there some way we can change the documentation to clarify this?  Better yet, 
would you be willing to clarify this in the documentation? (the docs are 
editable by anyone).

Original comment by richarda...@gmail.com on 21 Dec 2010 at 10:58

GoogleCodeExporter commented 9 years ago
using constructor to add params doesn't allow to remove predefined parameters 
(onload, onerror, onlog). Is required for those parameters to always exist?

Original comment by l.bartol...@gmail.com on 22 Dec 2010 at 10:30

GoogleCodeExporter commented 9 years ago
you can reset them with:
m_supportedParamSet.clear()

and then add the ones you need.  I realize that this is slightly inefficient, 
but the performance hit is so minuscule in comparison to everything else going 
to (and constant time to boot) that it seemed preferable to not providing any 
defaults.

Original comment by richarda...@gmail.com on 22 Dec 2010 at 5:21

GoogleCodeExporter commented 9 years ago
As a sidenote, this is now a non-issue in 1.4 since m_supportedParamSet is no 
longer needed and has been removed.

Original comment by richarda...@gmail.com on 23 Jan 2011 at 2:29