1441009 - Don't null-check OwnerDoc.

Status: RESOLVED FIXED

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

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1441009: Don't null-check OwnerDoc.

Can't return null.

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1441009: bonus: Don't read garbage in CanvasUtils.

While reading who did that, I also found this, which is just fantastic.

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

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8953836 [details]
Bug 1441009: Don't null-check OwnerDoc.

https://reviewboard.mozilla.org/r/223010/#review228876

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8953837 [details]
Bug 1441009: bonus: Don't read garbage in CanvasUtils.

https://reviewboard.mozilla.org/r/223012/#review228878

In practice this doesn't change anything, since isFileURL is always wet if valid scheme is passed as param.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7019615c84b2
Don't null-check OwnerDoc. r=smaug
https://hg.mozilla.org/integration/autoland/rev/c0ef85232ad3
Use a bit less dangerous error handling in CanvasUtils. r=smaug

Comment 6

[Masatoshi Kimura [:emk]]

OwnerDoc() should return NotNull<nsIDocument*> if we know it will never be null.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to Masatoshi Kimura [:emk] from comment #6)
> OwnerDoc() should return NotNull<nsIDocument*> if we know it will never be
> null.

Or a reference, just sayin ;)

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

In DOM especially methods which return Foo* but don't have Get* in the method name aren't supposed to return null ever.
NotNull is way newer code than that convention. But using NotNull sounds reasonable if one wants to write the patch.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Yeah, returning reference might make even more sense.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Though, then people might start to write code like
nsIDocument& doc = element->OwnerDoc(); without thinking whether doc should have been stored in a strong reference. So, don't like reference after all.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

(In reply to Olli Pettay [:smaug] from comment #10)
> Though, then people might start to write code like
> nsIDocument& doc = element->OwnerDoc(); without thinking whether doc should
> have been stored in a strong reference. So, don't like reference after all.

FWIW, the way WebKit deals with this is there's a Ref<T> type, which holds a strong reference just like nsCOMPtr / RefPtr, but is constructible from a reference.

Obviously it internally uses a pointer, but that means that you no longer need pointers or awkwardness (&) to store strong refs. Maybe we could do something like that eventually, though given the differences between COM / Ref ptrs it may not be so trivial.

Comment 12

[Masatoshi Kimura [:emk]]

We have OwningNonNull<T> for non-null strong references.

Comment 13

[Masatoshi Kimura [:emk]]

Or NotNull<RefPtr<T>>.

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7019615c84b2
https://hg.mozilla.org/mozilla-central/rev/c0ef85232ad3