Disentangle nsIContent::DoneAddingChildren and script handling

RESOLVED FIXED in mozilla11

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

Trunk
mozilla11
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

Comment hidden (empty)
Could you explain the issues? The summary isn't really self-explanatory.
(Assignee)

Comment 2

6 years ago
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.)
Attachment #543822 - Flags: review?(Olli.Pettay)

Comment 3

6 years ago
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+
(Assignee)

Comment 4

6 years ago
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.)
Attachment #551336 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 5

6 years ago
Created attachment 551337 [details] [diff] [review]
Part c: Remove the return value from nsIContent::DoneAddingChildren
Attachment #551337 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 6

6 years ago
Created attachment 551339 [details] [diff] [review]
Part d: Make nsIScriptElement::MaybeProcessScript return a boolean
Attachment #551339 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 7

6 years ago
Created attachment 551340 [details] [diff] [review]
Part e: Make nsScriptLoader::ProcessScriptElement return a boolean
Attachment #551340 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 8

6 years ago
Created attachment 551342 [details] [diff] [review]
Part f: Less QIs in nsScriptLoader::ProcessScriptElement
Attachment #551342 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 9

6 years ago
Created attachment 551343 [details] [diff] [review]
Part g: Remove NS_CONTENT_SCRIPT_IS_EVENTHANDLER
Attachment #551343 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 10

6 years ago
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...

Updated

6 years ago
Attachment #551343 - Flags: review?(Olli.Pettay) → review+

Updated

6 years ago
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+

Updated

6 years ago
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+
(Assignee)

Comment 15

6 years ago
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.
(Assignee)

Comment 17

6 years ago
(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?
(Assignee)

Comment 18

6 years ago
https://hg.mozilla.org/mozilla-central/rev/acf6f9a14854
https://hg.mozilla.org/mozilla-central/rev/dcf7c1b0a756
https://hg.mozilla.org/mozilla-central/rev/a47f8a6f6705
https://hg.mozilla.org/mozilla-central/rev/ec02dc79904a
https://hg.mozilla.org/mozilla-central/rev/6b58c2e56c26
https://hg.mozilla.org/mozilla-central/rev/ce3ad0e12408
https://hg.mozilla.org/mozilla-central/rev/52951fda6786
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.