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

Status: NEW

(Core :: Layout, defect, P3)

Description

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

In Gecko methods which may possibly return null have traditionally had Get* prefix.
For some reason nsPresContext::Document() doesn't have.

Comment 1

[abhinavdv]

HEY!!! I am a newbie. Can I work on this bug.

Comment 2

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

Feel free to. I can review the patch.

Comment 3

[abhinavdv]

Attachment: bug-1323794-v1.patch

Comment 4

[Emilio Cobos Álvarez (:emilio)]

(In reply to [e-mail] 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...

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(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

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

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

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

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.

Comment 8

[abhinavdv]

Attachment: bug-1323794-v2.patch

Comment 9

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

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.

Comment 10

[abhinavdv]

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

[abhinavdv]

Attachment: bug-1323794.patch

Comment 12

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

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.

Comment 13

[abhinavdv]

sorry. I will take care.