Last Comment Bug 669012 - Disentangle nsIContent::DoneAddingChildren and script handling
: Disentangle nsIContent::DoneAddingChildren and script handling
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-02 13:05 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2011-11-16 10:20 PST (History)
5 users (show)
Ms2ger: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part a: ignore for and event attributes for SVG elements (2.87 KB, patch)
2011-07-04 14:30 PDT, :Ms2ger (⌚ UTC+1/+2)
bugs: review+
hsivonen: review+
Details | Diff | Splinter Review
Part b: Introduce nsIScriptElement::AttemptToExecute (19.09 KB, patch)
2011-08-07 13:10 PDT, :Ms2ger (⌚ UTC+1/+2)
bugs: review+
hsivonen: review+
Details | Diff | Splinter Review
Part c: Remove the return value from nsIContent::DoneAddingChildren (13.80 KB, patch)
2011-08-07 13:10 PDT, :Ms2ger (⌚ UTC+1/+2)
bugs: review+
hsivonen: review+
Details | Diff | Splinter Review
Part d: Make nsIScriptElement::MaybeProcessScript return a boolean (5.42 KB, patch)
2011-08-07 13:12 PDT, :Ms2ger (⌚ UTC+1/+2)
bugs: review+
hsivonen: review+
Details | Diff | Splinter Review
Part e: Make nsScriptLoader::ProcessScriptElement return a boolean (18.23 KB, patch)
2011-08-07 13:13 PDT, :Ms2ger (⌚ UTC+1/+2)
bugs: review+
hsivonen: review+
Details | Diff | Splinter Review
Part f: Less QIs in nsScriptLoader::ProcessScriptElement (4.82 KB, patch)
2011-08-07 13:14 PDT, :Ms2ger (⌚ UTC+1/+2)
bugs: review+
hsivonen: review+
Details | Diff | Splinter Review
Part g: Remove NS_CONTENT_SCRIPT_IS_EVENTHANDLER (889 bytes, patch)
2011-08-07 13:15 PDT, :Ms2ger (⌚ UTC+1/+2)
bugs: review+
hsivonen: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2011-07-02 13:05:29 PDT

    
Comment 1 Mounir Lamouri (:mounir) 2011-07-04 09:28:49 PDT
Could you explain the issues? The summary isn't really self-explanatory.
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2011-07-04 14:30:53 PDT
Created attachment 543822 [details] [diff] [review]
Part a: ignore for and event attributes for SVG elements

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.)
Comment 3 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-07-13 03:39:22 PDT
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.
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2011-08-07 13:10:07 PDT
Created attachment 551336 [details] [diff] [review]
Part b: Introduce nsIScriptElement::AttemptToExecute

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.)
Comment 5 :Ms2ger (⌚ UTC+1/+2) 2011-08-07 13:10:56 PDT
Created attachment 551337 [details] [diff] [review]
Part c: Remove the return value from nsIContent::DoneAddingChildren
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2011-08-07 13:12:20 PDT
Created attachment 551339 [details] [diff] [review]
Part d: Make nsIScriptElement::MaybeProcessScript return a boolean
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2011-08-07 13:13:04 PDT
Created attachment 551340 [details] [diff] [review]
Part e: Make nsScriptLoader::ProcessScriptElement return a boolean
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2011-08-07 13:14:43 PDT
Created attachment 551342 [details] [diff] [review]
Part f: Less QIs in nsScriptLoader::ProcessScriptElement
Comment 9 :Ms2ger (⌚ UTC+1/+2) 2011-08-07 13:15:13 PDT
Created attachment 551343 [details] [diff] [review]
Part g: Remove NS_CONTENT_SCRIPT_IS_EVENTHANDLER
Comment 10 :Ms2ger (⌚ UTC+1/+2) 2011-08-07 13:16:36 PDT
I'd like Henri to have a look at the patches as well, and he's away until the 29th, so no hurry here.
Comment 11 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-08-29 04:52:52 PDT
Splitting the patch makes reviewing a bit harder this case...
Comment 12 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-08-29 05:09:09 PDT
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.
Comment 13 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-08-29 05:14:27 PDT
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.
Comment 14 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-08-29 05:17:27 PDT
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.
Comment 15 :Ms2ger (⌚ UTC+1/+2) 2011-09-16 10:05:01 PDT
Henri, could you have a look at the patches here?
Comment 16 Henri Sivonen (:hsivonen) 2011-09-19 04:34:21 PDT
(In reply to Ms2ger from comment #15)
> Henri, could you have a look at the patches here?

They look good.
Comment 17 :Ms2ger (⌚ UTC+1/+2) 2011-10-22 07:29:38 PDT
(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?

Note You need to log in before you can comment on or make changes to this bug.