1455885 - The context-paint stuff could avoid using node properties.

Status: RESOLVED FIXED

(Core :: SVG, enhancement)

Description

[Emilio Cobos Álvarez [:emilio]]

Node properties are slow hashtable lookups. No need for them if you just want to stash a pointer in the document object.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1455885: Inline and make document casts fatally assert.

For consistency with AsElement / AsContent / AsDocumentFragment, etc.

Review commit: https://reviewboard.mozilla.org/r/238718/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/238718/

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1455885: Make the SVG context paint not use a node property, but a member in SVGDocument.

Review commit: https://reviewboard.mozilla.org/r/238720/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/238720/

Comment 3

[Jonathan Watt [:jwatt]]

Comment on attachment 8969912 [details]
Bug 1455885: Make the SVG context paint not use a node property, but a member in SVGDocument.

https://reviewboard.mozilla.org/r/238720/#review244582

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8969911 [details]
Bug 1455885: Inline and make document casts fatally assert.

https://reviewboard.mozilla.org/r/238718/#review244806

::: dom/xul/nsXULContentSink.cpp:678
(Diff revision 1)
>      return NS_OK;
>    };
>  
> -  XULDocument* doc = idoc ? idoc->AsXULDocument() : nullptr;
> -  if (doc && !doc->OnDocumentParserError()) {
> +  if (idoc &&
> +      idoc->IsXULDocument() &&
> +      idoc->AsXULDocument()->OnDocumentParserError()) {

Err, this needs to be !idoc->OnDocumentParseError... Fixed locally.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8969911 [details]
Bug 1455885: Inline and make document casts fatally assert.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238718/diff/1-2/

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8969912 [details]
Bug 1455885: Make the SVG context paint not use a node property, but a member in SVGDocument.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238720/diff/1-2/

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8969911 [details]
Bug 1455885: Inline and make document casts fatally assert.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238718/diff/2-3/

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8969912 [details]
Bug 1455885: Make the SVG context paint not use a node property, but a member in SVGDocument.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238720/diff/2-3/

Comment 9

[Emilio Cobos Álvarez [:emilio]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=81d26c05ef9179b1dffb90fb12a411c8ffb60390

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8969911 [details]
Bug 1455885: Inline and make document casts fatally assert.

https://reviewboard.mozilla.org/r/238718/#review245602

::: dom/base/nsGlobalWindowInner.cpp:5238
(Diff revision 3)
>    }
>    // The event is fired at the body element, or if there is no body element,
>    // at the document.
>    nsCOMPtr<EventTarget> eventTarget = mDoc.get();
> -  nsHTMLDocument* htmlDoc = mDoc->AsHTMLDocument();
> -  if (htmlDoc) {
> +  if (mDoc->IsHTMLOrXHTML()) {
> +    if (Element* body = mDoc->AsHTMLDocument()->GetBody()) {

GetBody() is on nsIDocument, so you don't need AsHTMLDocument() here.

Please file a followup to check whether the IsHTMLOrXHTML() check should even be here?  For that matter, per spec it looks like this event should be targeted at the window itself, not at some element...  Please do file a followup to sort this out.

::: dom/base/nsIDocument.h:3352
(Diff revision 3)
>    // ParentNode
>    nsIHTMLCollection* Children();
>    uint32_t ChildElementCount();
>  
> -  virtual nsHTMLDocument* AsHTMLDocument() { return nullptr; }
> -  virtual mozilla::dom::SVGDocument* AsSVGDocument() { return nullptr; }
> +  /**
> +   * Asserts IsHTMLDocument, and can't return null.

I would hope it asserts IsHTMLOrXHTML()

::: dom/webauthn/WebAuthnManager.cpp:136
(Diff revision 3)
>    nsAutoCString originHost;
>    if (NS_FAILED(uri->GetAsciiHost(originHost))) {
>      return NS_ERROR_FAILURE;
>    }
>    nsCOMPtr<nsIDocument> document = aParent->GetDoc();
>    if (!document || !document->IsHTMLDocument()) {

The IsHTMLDocument tests in this directory are likely bogus (e.g. webauthn should work in XHTML too)... Maybe file a followup?

::: editor/composer/nsEditingSession.cpp:667
(Diff revision 3)
>          nsCOMPtr<mozIDOMWindowProxy> window;
>          aWebProgress->GetDOMWindow(getter_AddRefs(window));
>  
>          auto* piWindow = nsPIDOMWindowOuter::From(window);
>          nsCOMPtr<nsIDocument> doc = piWindow->GetDoc();
> -        nsHTMLDocument* htmlDoc = doc ? doc->AsHTMLDocument() : nullptr;
> +        nsHTMLDocument* htmlDoc = doc && doc->IsHTMLDocument()

Should this test for HTML-or-XHTML?  The old code did....

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8969911 [details]
Bug 1455885: Inline and make document casts fatally assert.

https://reviewboard.mozilla.org/r/238718/#review245602

> GetBody() is on nsIDocument, so you don't need AsHTMLDocument() here.
> 
> Please file a followup to check whether the IsHTMLOrXHTML() check should even be here?  For that matter, per spec it looks like this event should be targeted at the window itself, not at some element...  Please do file a followup to sort this out.

Filed bug 1457166.

> The IsHTMLDocument tests in this directory are likely bogus (e.g. webauthn should work in XHTML too)... Maybe file a followup?

Filed bug 1457171.

> Should this test for HTML-or-XHTML?  The old code did....

Whoops, yes, great catch.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4bba542fa45
Inline and make document casts fatally assert. r=bz

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0fe3a00acc7
Make the SVG context paint not use a node property, but a member in SVGDocument. r=jwatt

Comment 14

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/e4bba542fa45
https://hg.mozilla.org/mozilla-central/rev/e0fe3a00acc7