Closed Bug 1455885 Opened Last year Closed Last year

The context-paint stuff could avoid using node properties.

Categories

(Core :: SVG, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Node properties are slow hashtable lookups. No need for them if you just want to stash a pointer in the document object.
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
Attachment #8969912 - Flags: review?(jwatt) → review+
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 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....
Attachment #8969911 - Flags: review?(bzbarsky) → review+
Blocks: 1457166
Blocks: 1457171
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.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4bba542fa45
Inline and make document casts fatally assert. r=bz
Pushed by ecoal95@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/e4bba542fa45
https://hg.mozilla.org/mozilla-central/rev/e0fe3a00acc7
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.