Add UI process WebRTC runtime logging.
<rdar://problem/49071443>
Created attachment 365381 [details] Patch
Created attachment 365383 [details] Patch
Attachment 365383 [details] did not pass style-queue: ERROR: Source/WebKit/UIProcess/UserMediaPermissionRequestProxy.cpp:150: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebCore/page/ChromeClient.h:501: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] ERROR: Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:698: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 3 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 365383 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=365381&action=review > Source/WebCore/inspector/agents/page/PageConsoleAgent.cpp:48 > + , m_inspectedPage(context.inspectedPage) Are we sure context.inspectedPage stays valid for the whole life of PageConsoleAgent? > Source/WebCore/inspector/agents/page/PageConsoleAgent.cpp:59 > +void PageConsoleAgent::getLoggingChannels(ErrorString&, RefPtr<JSON::ArrayOf<Inspector::Protocol::Console::Channel>>& channels) This routine is similar to WebConsoleAgent::getLoggingChannels. Could it be shared? Since we always overwrite channels, should we change it to return a RefPtr<JSON::ArrayOf<Inspector::Protocol::Console::Channel>>? It is a virtual method though, so probably needs more refactoring. > Source/WebCore/inspector/agents/page/PageConsoleAgent.cpp:108 > + level = WTFLogLevel::Error; maybe rewrite to return early, like { { WTFLogChannelState::Off, WTFLogLevel::Error } }; > Source/WebKit/UIProcess/WebPageProxy.cpp:8949 > + m_logger->setEnabled(this, isAlwaysOnLoggingAllowed()); What happens if the web process is crashing. We might still be logging but the web process might think we are not logging. Or WebPageProxy might be attached to a different process. We may need to sync the logging somehow, or maybe it happens automatically by the web inspector? Should we set m_logger to nullptr in resetState? The other thing is when page is navigating cross origin. In that case WebPageProxy will be attached to a different process with potentially different logging values.
Comment on attachment 365383 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=365383&action=review >> Source/WebCore/inspector/agents/page/PageConsoleAgent.cpp:48 >> + , m_inspectedPage(context.inspectedPage) > > Are we sure context.inspectedPage stays valid for the whole life of PageConsoleAgent? Yes, Joe Peck suggested this approach. >> Source/WebCore/inspector/agents/page/PageConsoleAgent.cpp:59 >> +void PageConsoleAgent::getLoggingChannels(ErrorString&, RefPtr<JSON::ArrayOf<Inspector::Protocol::Console::Channel>>& channels) > > This routine is similar to WebConsoleAgent::getLoggingChannels. > Could it be shared? > > Since we always overwrite channels, should we change it to return a RefPtr<JSON::ArrayOf<Inspector::Protocol::Console::Channel>>? > It is a virtual method though, so probably needs more refactoring. All of the logging code was removed from WebConsoleAgent >> Source/WebCore/inspector/agents/page/PageConsoleAgent.cpp:108 >> + level = WTFLogLevel::Error; > > maybe rewrite to return early, like { { WTFLogChannelState::Off, WTFLogLevel::Error } }; Sure. >> Source/WebKit/UIProcess/WebPageProxy.cpp:8949 >> #undef MERGE_WHEEL_EVENTS > > What happens if the web process is crashing. > We might still be logging but the web process might think we are not logging. > Or WebPageProxy might be attached to a different process. > We may need to sync the logging somehow, or maybe it happens automatically by the web inspector? > Should we set m_logger to nullptr in resetState? > > The other thing is when page is navigating cross origin. > In that case WebPageProxy will be attached to a different process with potentially different logging values. As we discussed, lets leave this as is for now and revisit it later.
Created attachment 365386 [details] Patch
Attachment 365386 [details] did not pass style-queue: ERROR: Source/WebKit/UIProcess/UserMediaPermissionRequestProxy.cpp:150: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebCore/page/ChromeClient.h:501: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] ERROR: Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:698: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 3 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 365400 [details] Patch
Attachment 365400 [details] did not pass style-queue: ERROR: Source/WebKit/UIProcess/UserMediaPermissionRequestProxy.cpp:150: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebCore/page/ChromeClient.h:501: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] ERROR: Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:698: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 3 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 365410 [details] Patch
Attachment 365410 [details] did not pass style-queue: ERROR: Source/WebKit/UIProcess/UserMediaPermissionRequestProxy.cpp:150: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebCore/page/ChromeClient.h:501: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] ERROR: Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:698: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 3 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 365418 [details] Patch
Attachment 365418 [details] did not pass style-queue: ERROR: Source/WebKit/UIProcess/UserMediaPermissionRequestProxy.cpp:150: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebCore/page/ChromeClient.h:501: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] ERROR: Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:698: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 3 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 365418 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=365418&action=review > Source/WebCore/inspector/agents/page/PageConsoleAgent.h:46 > virtual ~PageConsoleAgent() = default; This default destructor is probably not needed. > Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.h:89 > + const Logger& logger() const final { return m_logger.get(); } Could be implemented as m_page.logger(); We could then remove m_logger and we would be sure to be in sync with whatever page logger is. > Source/WebKit/UIProcess/WebPageProxy.cpp:8931 > + m_logger->setEnabled(this, isAlwaysOnLoggingAllowed()); When we are changing of WebPageProxy::m_websiteDataStore, we might change of isAlwaysOnLoggingAllowed() return value. Maybe logger should be updated accordingly at that time.
Comment on attachment 365418 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=365418&action=review >> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.h:89 >> + const Logger& logger() const final { return m_logger.get(); } > > Could be implemented as m_page.logger(); > We could then remove m_logger and we would be sure to be in sync with whatever page logger is. Good idea, changed. >> Source/WebKit/UIProcess/WebPageProxy.cpp:8931 >> + m_logger->setEnabled(this, isAlwaysOnLoggingAllowed()); > > When we are changing of WebPageProxy::m_websiteDataStore, we might change of isAlwaysOnLoggingAllowed() return value. > Maybe logger should be updated accordingly at that time. Yes, changed.
Comment on attachment 365418 [details] Patch Attachment 365418 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11587341 New failing tests: inspector/console/webcore-logging.html
Created attachment 365442 [details] Archive of layout-test-results from ews102 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 365418 [details] Patch Attachment 365418 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11588233 New failing tests: inspector/console/webcore-logging.html
Created attachment 365458 [details] Archive of layout-test-results from ews114 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 365418 [details] Patch Attachment 365418 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11589767 New failing tests: inspector/console/webcore-logging.html
Created attachment 365464 [details] Archive of layout-test-results from ews106 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Created attachment 365572 [details] Patch for landing
Created attachment 365581 [details] Patch for landing
Attachment 365581 [details] did not pass style-queue: ERROR: Source/WebKit/UIProcess/UserMediaPermissionRequestProxy.cpp:150: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebCore/page/ChromeClient.h:501: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] ERROR: Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:703: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 3 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 365581 [details] Patch for landing Clearing flags on attachment: 365581 Committed r243328: <https://trac.webkit.org/changeset/243328>