Closed Bug 669012 Opened 9 years ago Closed 8 years ago

Disentangle nsIContent::DoneAddingChildren and script handling

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Details

Attachments

(7 files)

No description provided.
Could you explain the issues? The summary isn't really self-explanatory.
I was trying to explain why my upcoming patch is correct, but it turned out not to be, because we look at the for and event attributes of SVG elements. (Yes, really.)
Attachment #543822 - Flags: review?(Olli.Pettay)
Comment on attachment 543822 [details] [diff] [review]
Part a: ignore for and event attributes for SVG elements

Make sure other browsers behave the same way.
Attachment #543822 - Flags: review?(Olli.Pettay) → review+
tl;dr: I'm right.

First, why I think we should do this: at first sight, it may seem like a good idea (or at least, not a bad one) to use the same signature for the script elements and the other implementations of this method (nsHTMLObjectElement, nsHTMLTextAreaElement, nsHTMLSharedObjectElement, nsHTMLTitleElement, nsHTMLSelectElement, nsSVGTitleElement, and nsXTFElementWrapper). After all, they need to be called around the same time for roughly the same reason.

It turns out that's not really true. Looking at the patch, only XSLT actually calls it at the same time for both sets of elements. Also, they only superficially use the same signature: script elements ignore the aHaveNotified parameter, and the other implementation always return NS_OK (and indeed, all callers ignore the return value). The script implementations don't always return NS_OK, but their only other return value NS_ERROR_HTMLPARSER_BLOCK (see below), and as such it would be clearer to return a boolean (and that would prevent bugs such as introduced in bug 383383, where a returned NS_ERROR_DOM_NOT_SUPPORTED_ERR shut down the XML parser).

nsScriptLoader::ProcessScriptElement either returns a failure nsresult, NS_CONTENT_SCRIPT_IS_EVENTHANDLER (only for HTML scripts since part a), or NS_OK. nsScriptElement::MaybeProcessScript then filters out all failure nsresults except for NS_ERROR_HTMLPARSER_BLOCK, and nsHTMLScriptElement::MaybeProcessScript turns NS_CONTENT_SCRIPT_IS_EVENTHANDLER into NS_OK. That makes the return value of MaybeProcessScript effectively two-state.

To make the code more understandable, I've written patches to split nsresult nsIContent::DoneAddingChildren(PRBool) into two methods more suited for their respective uses: bool nsIScriptElement::AttemptToExecute() (part b) and void nsIContent::DoneAddingChildren(PRBool) (part c). Because nsIScriptElement::AttemptToExecute is its only caller, I'll then make nsIScriptElement::MaybeProcessScript a boolean as well, and remove nsHTMLScriptElement::MaybeProcessScript as it's no longer necessary (part d), and do the same to nsScriptLoader::ProcessScriptElement (part e). (After that, I've got a couple of cleanups I noticed coming up as well.)
Attachment #551336 - Flags: review?(Olli.Pettay)
I'd like Henri to have a look at the patches as well, and he's away until the 29th, so no hurry here.
Splitting the patch makes reviewing a bit harder this case...
Attachment #551343 - Flags: review?(Olli.Pettay) → review+
Attachment #551342 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 551336 [details] [diff] [review]
Part b: Introduce nsIScriptElement::AttemptToExecute


>+  /**
>+   * This method is called when the parser finishes creating the script
>+   * element's children, if any are present.
>+   *
>+   * @return whether the script will be loaded asynchronously
Is that what NS_ERROR_HTMLPARSER_BLOCK is really about?
It is easy to misunderstood "asynchronously" since there is also <script async>.
So, could you rephrase that sentence.
Attachment #551336 - Flags: review?(Olli.Pettay) → review+
Attachment #551337 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 551339 [details] [diff] [review]
Part d: Make nsIScriptElement::MaybeProcessScript return a boolean

>   /**
>    * Processes the script if it's in the document-tree and links to or
>    * contains a script. Once it has been evaluated there is no way to make it
>    * reevaluate the script, you'll have to create a new element. This also means
>    * that when adding a src attribute to an element that already contains an
>    * inline script, the script referenced by the src attribute will not be
>    * loaded.
>    *
>    * In order to be able to use multiple childNodes, or to use the
>    * fallback mechanism of using both inline script and linked script you have
>    * to add all attributes and childNodes before adding the element to the
>    * document-tree.
>    */
>-  virtual nsresult MaybeProcessScript() = 0;
>+  virtual bool MaybeProcessScript() = 0;

Please add a comment what the return value means.
Attachment #551339 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 551340 [details] [diff] [review]
Part e: Make nsScriptLoader::ProcessScriptElement return a boolean

In general I'm not strongly against or for this change.

Though, it is nice to have DoneAddingChildren just a simple
notification which doesn't need to return anything.
Attachment #551340 - Flags: review?(Olli.Pettay) → review+
Henri, could you have a look at the patches here?
Attachment #543822 - Flags: review+
Attachment #551336 - Flags: review+
Attachment #551337 - Flags: review+
Attachment #551339 - Flags: review+
Attachment #551340 - Flags: review+
Attachment #551342 - Flags: review+
Attachment #551343 - Flags: review+
(In reply to Ms2ger from comment #15)
> Henri, could you have a look at the patches here?

They look good.
(In reply to Olli Pettay [:smaug] from comment #12)
> Comment on attachment 551336 [details] [diff] [review] [diff] [details] [review]
> Part b: Introduce nsIScriptElement::AttemptToExecute
> 
> 
> >+  /**
> >+   * This method is called when the parser finishes creating the script
> >+   * element's children, if any are present.
> >+   *
> >+   * @return whether the script will be loaded asynchronously
> Is that what NS_ERROR_HTMLPARSER_BLOCK is really about?
> It is easy to misunderstood "asynchronously" since there is also <script
> async>.
> So, could you rephrase that sentence.

Something along the lines of "whether the parser will be blocked while this script is being loaded", perhaps?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.