Bug 196042 – Stop storing raw pointers to Documents

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Stop storing raw pointers to Documents

[Alex Christensen]

Created attachment 365427 [details]
Patch

[Chris Dumez]

Comment on attachment 365427 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365427&action=review

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:372
> +        didCloseSocketStream(handle);

Did you mean to include this?

[Geoffrey Garen]

Comment on attachment 365427 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365427&action=review

r=me

>> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:372
>> +        didCloseSocketStream(handle);
> 
> Did you mean to include this?

Plz delete.

[Alex Christensen]

Created attachment 365440 [details]
Patch

[Daniel Bates]

Comment on attachment 365440 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365440&action=review

> Source/WebCore/loader/MediaResourceLoader.h:55
> +    Document* document() const { return m_document.get(); }

Why? To fix the build? :D  If so, what's the caller that needs this? If not, this code is correct as-is and you're making it wrong.

[Daniel Bates]

(In reply to Daniel Bates from comment #5)
> Comment on attachment 365440 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365440&action=review
> 
> > Source/WebCore/loader/MediaResourceLoader.h:55
> > +    Document* document() const { return m_document.get(); }
> 
> Why? To fix the build? :D  If so, what's the caller that needs this? If not,
> this code is correct as-is and you're making it wrong.

Talk to me in person if you need to know why it's wrong.

[Alex Christensen]

Comment on attachment 365440 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365440&action=review

>>> Source/WebCore/loader/MediaResourceLoader.h:55
>>> +    Document* document() const { return m_document.get(); }
>> 
>> Why? To fix the build? :D  If so, what's the caller that needs this? If not, this code is correct as-is and you're making it wrong.
> 
> Talk to me in person if you need to know why it's wrong.

I'm not sure what you're referring to.  I'm adding const because I can and because it's an accessor that doesn't modify the object.  I'm adding the .get() because m_document is now a WeakPtr and this function returns a pointer, so I need the .get().

[Geoffrey Garen]

Comment on attachment 365440 [details]
Patch

r=me, seems worth following up with Dan to understand why he though const was "wrong"

[Chris Dumez]

Comment on attachment 365440 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365440&action=review

>>>> Source/WebCore/loader/MediaResourceLoader.h:55
>>>> +    Document* document() const { return m_document.get(); }
>>> 
>>> Why? To fix the build? :D  If so, what's the caller that needs this? If not, this code is correct as-is and you're making it wrong.
>> 
>> Talk to me in person if you need to know why it's wrong.
> 
> I'm not sure what you're referring to.  I'm adding const because I can and because it's an accessor that doesn't modify the object.  I'm adding the .get() because m_document is now a WeakPtr and this function returns a pointer, so I need the .get().

I think people have different views about what const-correct means. I personally think the new code is less const correct since it is returning a non const data member in a const method.

[Daniel Bates]

(In reply to Chris Dumez from comment #9)

> I
> personally think the new code is less const correct since it is returning a
> non const data member in a const method.

Bingo! 👍

[Alex Christensen]

Removed the controversial const and committed to http://trac.webkit.org/r243459

[Radar WebKit Bug Importer]

<rdar://problem/49229100>