Bug 22665 – Remove setCreatedByParser(bool) from the few elements that use it

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Julien Chaffraix]

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.

[Julien Chaffraix]

Created attachment 25760 [details]
First part: remove it for script elements

[Darin Adler]

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

[Julien Chaffraix]

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.

[Julien Chaffraix]

Comment on attachment 25760 [details]
First part: remove it for script elements

Landed the first part in r39111.

[Julien Chaffraix]

Created attachment 25895 [details]
Second part - remove style and link elements'

[Julien Chaffraix]

Created attachment 25896 [details]
Third and last part : remove it from frame and iframe

[Eric Seidel (no email)]

Comment on attachment 25895 [details]
Second part - remove style and link elements'

Looks great!

[Eric Seidel (no email)]

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.

[Julien Chaffraix]

(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.

[Darin Adler]

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

[Julien Chaffraix]

(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.

[Julien Chaffraix]

Comment on attachment 25895 [details]
Second part - remove style and link elements'

Landed the second part in r39180.

[Julien Chaffraix]

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.

[Julien Chaffraix]

Landed the last patch in r39292.