Bug 217552

Created attachment 411004 [details] Patch

Created attachment 411020 [details] Fix crashes in layout tests

Comment on attachment 411020 [details] Fix crashes in layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=411020&action=review > Source/WebCore/ChangeLog:9 > + constructor and assignment operator because of the updates on the two getters. Can you describe the benefit of this change? > Source/WebCore/platform/audio/cocoa/CAAudioStreamDescription.cpp:117 > return m_platformDescription; This is a potential change of behavior. m_streamDescription could for instance be modified (for instance using streamDescription() non const getter). This would make m_platformDescription out of sync. > Source/WebCore/platform/audio/cocoa/CAAudioStreamDescription.cpp:122 > + return m_format; Why removing the lazy init of m_format?

Comment on attachment 411020 [details] Fix crashes in layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=411020&action=review >> Source/WebCore/ChangeLog:9 >> + constructor and assignment operator because of the updates on the two getters. > > Can you describe the benefit of this change? Sure. I encountered some weirdness(linker errors) when I tried to move the WEBCORE_EXPORT from the class to methods, so I took a closer look and found this class looks strange. The class defines the function calculateFormat() but does not provide an implementation, and CAAudioStreamDescription::platformDescription() always needs to update m_platformDescription. >> Source/WebCore/platform/audio/cocoa/CAAudioStreamDescription.cpp:117 >> return m_platformDescription; > > This is a potential change of behavior. > m_streamDescription could for instance be modified (for instance using streamDescription() non const getter). > This would make m_platformDescription out of sync. Oops, sounds not a good encapsulation. Should we disable that possibility? >> Source/WebCore/platform/audio/cocoa/CAAudioStreamDescription.cpp:122 >> + return m_format; > > Why removing the lazy init of m_format? So it is just a lazy init? I thought we may need to do some unnecessary work whenever format() is called.

Comment on attachment 411020 [details] Fix crashes in layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=411020&action=review >>> Source/WebCore/platform/audio/cocoa/CAAudioStreamDescription.cpp:117 >>> return m_platformDescription; >> >> This is a potential change of behavior. >> m_streamDescription could for instance be modified (for instance using streamDescription() non const getter). >> This would make m_platformDescription out of sync. > > Oops, sounds not a good encapsulation. Should we disable that possibility? Actually, I was wrong here since we pass m_streamDescription as a pointer and not by value. If we can remove streamDescription() non const getter, we should probably do it in any case. I am not sure platformDescription is called much, so a lazy init seems fine here. >>> Source/WebCore/platform/audio/cocoa/CAAudioStreamDescription.cpp:122 >>> + return m_format; >> >> Why removing the lazy init of m_format? > > So it is just a lazy init? I thought we may need to do some unnecessary work whenever format() is called. It is a half lazy init. Agreed, it is a bit weird also since m_streamDescription could change but we would not change m_format accordingly if m_format is not None. I guess that to make it clearer, we could use an Optional<Format>.

<rdar://problem/70401637>

Comment on attachment 411020 [details] Fix crashes in layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=411020&action=review >>>> Source/WebCore/platform/audio/cocoa/CAAudioStreamDescription.cpp:122 >>>> + return m_format; >>> >>> Why removing the lazy init of m_format? >> >> So it is just a lazy init? I thought we may need to do some unnecessary work whenever format() is called. > > It is a half lazy init. Agreed, it is a bit weird also since m_streamDescription could change but we would not change m_format accordingly if m_format is not None. > I guess that to make it clearer, we could use an Optional<Format>. Yeah, seems better to use an Optional<>. However, it won't solve the problem when m_streamDescription is changed by other objects.

Created attachment 412215 [details] Patch

Looks like it is not a good idea to refactor the CAAudioStreamDescription class now. Let’s remove the unused private function and close this bug.

Committed r268988: <https://trac.webkit.org/changeset/268988> All reviewed patches have been landed. Closing bug and clearing flags on attachment 412215 [details].