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




2 years ago
7 months ago


(Reporter: smaug, Unassigned)


50 Branch

Firefox Tracking Flags

(Not tracked)



(3 attachments)



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.

Comment 2

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

Comment 3

7 months ago
Created attachment 8941846 [details] [diff] [review]
Attachment #8941846 - Flags: review?(bugs)
(In reply to 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.

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).

Comment 7

7 months ago
Comment on attachment 8941846 [details] [diff] [review]

Yeah, the method is called everywhere

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

Comment 8

7 months ago
Created attachment 8942423 [details] [diff] [review]
Attachment #8942423 - Flags: review?(bugs)

Comment 9

7 months ago
Comment on attachment 8942423 [details] [diff] [review]

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]
Attachment #8947437 - Flags: review?(bugs)

Comment 12

7 months ago
Comment on attachment 8947437 [details] [diff] [review]

>@@ -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.