When we use setCreatedByParser, it is always just after having created the Element. It would make more sense to embed the logic into the ElementFactory (which already take a createdByParser parameter) and the Elements' constructors. Patch forthcoming.
Created attachment 25760 [details] First part: remove it for script elements
Comment on attachment 25760 [details] First part: remove it for script elements It seems ugly to have to put those details into the .in file -- why not always have a setCreatedByParser function that's an inline that does nothing instead? I don't think this is a real improvement. r=me on this if you really think it's needed
Before replying to your comment, my goal is either harmonize the HTMLElements' signature or tweak the factory generation using the .in file. The final goal is to generate HTMLElementFactory so I am quite open on the path to choose. (In reply to comment #2) > (From update of attachment 25760 [details] [review]) > It seems ugly to have to put those details into the .in file -- why not always > have a setCreatedByParser function that's an inline that does nothing instead? I don't think adding 20 or so empty setCreatedByParser methods is also an improvement. I find the method setCreatedByParser to be artifical as it is called just after the constructor was called so I would rather see the constructor modified to take the createdByParser boolean parameter instead. Unfortunately in both cases, we would not know which element use the createdByParser information anymore. I have chosen this way for 2 reasons: first it would avoid changing all HTMLElements signature (or add the setCreatedByParser method) by just tweaking the code generation for those required. Also removing setCreatedByParser means removing XMLTokenizer::eventuallyMarkAsParserCreated.
Comment on attachment 25760 [details] First part: remove it for script elements Landed the first part in r39111.
Created attachment 25895 [details] Second part - remove style and link elements'
Created attachment 25896 [details] Third and last part : remove it from frame and iframe
Comment on attachment 25895 [details] Second part - remove style and link elements' Looks great!
Comment on attachment 25896 [details] Third and last part : remove it from frame and iframe Why is this right? - : HTMLFrameOwnerElement(tagName, doc) + : HTMLFrameOwnerElement(tagName, doc, false) Please indicate why in the bug, and ideally add a comment to the code explaining why that's correct when landing. Otherwise looks fine.
(In reply to comment #8) > (From update of attachment 25896 [details] [review]) > Why is this right? > > - : HTMLFrameOwnerElement(tagName, doc) > + : HTMLFrameOwnerElement(tagName, doc, false) > > > Please indicate why in the bug, and ideally add a comment to the code > explaining why that's correct when landing. I have kept the original behaviour: we were not calling setCreatedByParser on an HTMLPluginElement so the default value (false) would be used. I am not sure why we are ignoring the createdByParser flag on HTMLPluginElement so I could add a comment to point that it is strange or just fix it now. Both are fine by me.
Comment on attachment 25896 [details] Third and last part : remove it from frame and iframe > HTMLPlugInElement::HTMLPlugInElement(const QualifiedName& tagName, Document* doc) > - : HTMLFrameOwnerElement(tagName, doc) > + : HTMLFrameOwnerElement(tagName, doc, false) > #if ENABLE(NETSCAPE_PLUGIN_API) > , m_NPObject(0) > #endif I think you stumbled on a bug here. You should comment about the fact that this is strange. You should consider filing a bug report. You consider thinking about what the symptom of the bug would be. What does HTMLFrameOwnerElement do differently when created by the parser? And then construct a test case. And then fix the bug. r=me
(In reply to comment #10) > (From update of attachment 25896 [details] [review]) > > HTMLPlugInElement::HTMLPlugInElement(const QualifiedName& tagName, Document* doc) > > - : HTMLFrameOwnerElement(tagName, doc) > > + : HTMLFrameOwnerElement(tagName, doc, false) > > #if ENABLE(NETSCAPE_PLUGIN_API) > > , m_NPObject(0) > > #endif > > I think you stumbled on a bug here. > > You should comment about the fact that this is strange. I will before landing. > You should consider filing a bug report. Sure. > > You consider thinking about what the symptom of the bug would be. What does > HTMLFrameOwnerElement do differently when created by the parser? And then > construct a test case. And then fix the bug. I do not think there is changes at construction time. I think that the return value of HTMLFrameOwnerElement::createdByParser() would modify some methods' behaviour. I have done some investigation and the only method using it is FrameLoader::updateHistoryForStandardLoad(). I will do some more research later before closing this bug.
Comment on attachment 25895 [details] Second part - remove style and link elements' Landed the second part in r39180.
Ok, I have filed a follow up bug about the HTMLPlugInElement issue (bug22851) as I would like to land the changes and move forward. I will add a comment about the odd part in HTMLPlugInElement and point to the bug.
Landed the last patch in r39292.