Closed Bug 1441009 Opened 2 years ago Closed 2 years ago

Don't null-check OwnerDoc.

Categories

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

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(2 files)

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

https://reviewboard.mozilla.org/r/223010/#review228876
Attachment #8953836 - Flags: review?(bugs) → review+
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.
Attachment #8953837 - Flags: review?(bugs) → review+
Pushed by ecoal95@gmail.com:
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
OwnerDoc() should return NotNull<nsIDocument*> if we know it will never be null.
(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 ;)
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.
Yeah, returning reference might make even more sense.
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.
(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.
We have OwningNonNull<T> for non-null strong references.
Or NotNull<RefPtr<T>>.
https://hg.mozilla.org/mozilla-central/rev/7019615c84b2
https://hg.mozilla.org/mozilla-central/rev/c0ef85232ad3
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Blocks: 1441165
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.