rename nsPresContext::Document() to nsPresContext::GetDocument() since it may return null

NEW
Unassigned

Status

()

P3
normal
2 years ago
7 months ago

People

(Reporter: smaug, Unassigned)

Tracking

50 Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
In Gecko methods which may possibly return null have traditionally had Get* prefix.
For some reason nsPresContext::Document() doesn't have.
Priority: -- → P3

Comment 1

7 months ago
HEY!!! I am a newbie. Can I work on this bug.
(Reporter)

Comment 2

7 months ago
Feel free to. I can review the patch.

Comment 3

7 months ago
Created attachment 8941846 [details] [diff] [review]
bug-1323794-v1.patch
Attachment #8941846 - Flags: review?(bugs)
(In reply to dvabhinav31@gmail.com from comment #3)
> Created attachment 8941846 [details] [diff] [review]
> bug-1323794-v1.patch

That only contains the changes to one file? I'm pretty sure there are many more callers.

Given this can only return null during disconnect, which I think most of the code doesn't care about, I'd prefer a function that asserted when called instead, but not sure...
(Renaming to GetDocument() is fine I guess. I'm just saying that given it's ok for most of the code to not null-check it, feels weird). Anyway, not a huge deal.
(Reporter)

Comment 6

7 months ago
IIRC, long ago we had some crashes because the method did in fact return null but callers didn't expect that (since the name hints that it doesn't return null).
(Reporter)

Comment 7

7 months ago
Comment on attachment 8941846 [details] [diff] [review]
bug-1323794-v1.patch

Yeah, the method is called everywhere
https://searchfox.org/mozilla-central/search?q=symbol:_ZNK13nsPresContext8DocumentEv&redirect=false

Please compile the patch before uploading.
Attachment #8941846 - Flags: review?(bugs) → review-

Comment 8

7 months ago
Created attachment 8942423 [details] [diff] [review]
bug-1323794-v2.patch
Attachment #8942423 - Flags: review?(bugs)
(Reporter)

Comment 9

7 months ago
Comment on attachment 8942423 [details] [diff] [review]
bug-1323794-v2.patch

You need to change all the callers in the tree too, and ensure the patch compiles.
Attachment #8942423 - Flags: review?(bugs) → review-

Comment 10

7 months ago
I have searched for files and was able to find a few.
Is there a way I can find all the files at once?

Comment 11

7 months ago
Created attachment 8947437 [details] [diff] [review]
bug-1323794.patch
Attachment #8947437 - Flags: review?(bugs)
(Reporter)

Comment 12

7 months ago
Comment on attachment 8947437 [details] [diff] [review]
bug-1323794.patch

>@@ -2225,7 +2225,7 @@ nsStyleImageRequest::Resolve(nsPresConte
>   }
> 
>   if (mModeFlags & Mode::Track) {
>-    mImageTracker = aPresContext->Document()->ImageTracker();
>+    mImageTracker = aPresContext-GetDocument()->ImageTracker();

This for example doesn't compile. Please provide patches which do compile.
Attachment #8947437 - Flags: review?(bugs) → review-

Comment 13

7 months ago
sorry. I will take care.
You need to log in before you can comment on or make changes to this bug.