Bug 196020 – Add UI process WebRTC runtime logging.

• Status: ASSIGNED
• Component: WebRTC

[Eric Carlson]

Add UI process WebRTC runtime logging.

[Radar WebKit Bug Importer]

<rdar://problem/49071443>

[Eric Carlson]

Created attachment 365381 [details]
Patch

[Eric Carlson]

Created attachment 365383 [details]
Patch

[EWS Watchlist]

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.

[youenn fablet]

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.

[Eric Carlson]

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.

[Eric Carlson]

Created attachment 365386 [details]
Patch

[EWS Watchlist]

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.

[Eric Carlson]

Created attachment 365400 [details]
Patch

[EWS Watchlist]

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.

[Eric Carlson]

Created attachment 365410 [details]
Patch

[EWS Watchlist]

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.

[Eric Carlson]

Created attachment 365418 [details]
Patch

[EWS Watchlist]

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.

[youenn fablet]

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.

[Eric Carlson]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Eric Carlson]

Created attachment 365572 [details]
Patch for landing

[Eric Carlson]

Created attachment 365581 [details]
Patch for landing

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 365581 [details]
Patch for landing

Clearing flags on attachment: 365581

Committed r243328: <https://trac.webkit.org/changeset/243328>