Stop storing raw pointers to Documents
Created attachment 365427 [details] Patch
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?
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.
Created attachment 365440 [details] Patch
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.
(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.
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().
Comment on attachment 365440 [details] Patch r=me, seems worth following up with Dan to understand why he though const was "wrong"
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.
(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! 👍
Removed the controversial const and committed to http://trac.webkit.org/r243459
<rdar://problem/49229100>