RESOLVED FIXED 204371
[Cocoa] Add _WKRemoteWebInspectorViewController SPI to set diagnostic logging delegate
https://bugs.webkit.org/show_bug.cgi?id=204371
Summary [Cocoa] Add _WKRemoteWebInspectorViewController SPI to set diagnostic logging...
Blaze Burg
Reported 2019-11-19 13:52:32 PST
.
Attachments
Patch (19.77 KB, patch)
2019-11-19 16:54 PST, Blaze Burg
no flags
Patch (19.13 KB, patch)
2019-11-19 16:58 PST, Blaze Burg
hi: review+
ews-watchlist: commit-queue-
Archive of layout-test-results from ews212 for win-future (14.25 MB, application/zip)
2019-11-20 00:24 PST, EWS Watchlist
no flags
Blaze Burg
Comment 1 2019-11-19 16:54:41 PST
Blaze Burg
Comment 2 2019-11-19 16:58:25 PST
Devin Rousso
Comment 3 2019-11-19 17:19:21 PST
Comment on attachment 383925 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383925&action=review r=me, overall looks good :) > Source/WebKit/WebProcess/WebPage/RemoteWebInspectorUI.cpp:186 > + // Inspector's diagnostic logging client will never be used unless the page setting is also enabled. I think it's more accurate to say "should never be used". > Source/WebKit/WebProcess/WebPage/RemoteWebInspectorUI.cpp:190 > + m_frontendAPIDispatcher.dispatchCommand("setDiagnosticLoggingAvailable"_s, available); NIT: do you want to use `m_diagnosticLoggingAvailable` instead, as that's this object's actual state, instead of a parameter? > Source/WebKit/WebProcess/WebPage/WebInspectorUI.cpp:306 > + // Inspector's diagnostic logging client will never be used unless the page setting is also enabled. Ditto (RemoteWebInspectorUI.cpp:186) > Source/WebKit/WebProcess/WebPage/WebInspectorUI.cpp:310 > m_frontendAPIDispatcher.dispatchCommand("setDiagnosticLoggingAvailable"_s, available); Ditto (RemoteWebInspectorUI.cpp:190) > Source/WebKit/WebProcess/WebPage/WebInspectorUI.h:87 > +#if ENABLE(INSPECTOR_TELEMETRY) > + void setDiagnosticLoggingAvailable(bool avaliable); > +#endif Why did this move? > Source/WebKit/WebProcess/WebPage/WebInspectorUI.h:160 > + > + Oops?
EWS Watchlist
Comment 4 2019-11-20 00:24:11 PST
Comment on attachment 383925 [details] Patch Attachment 383925 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/13268669 New failing tests: fast/dom/domTokenListIterator.html fast/dom/nodeListIterator.html fast/dom/NodeList/nodelist-iterable.html http/tests/security/cross-frame-access-enumeration.html
EWS Watchlist
Comment 5 2019-11-20 00:24:14 PST
Created attachment 383950 [details] Archive of layout-test-results from ews212 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews212 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Blaze Burg
Comment 6 2019-11-20 10:34:07 PST
(In reply to Devin Rousso from comment #3) > Comment on attachment 383925 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=383925&action=review > > r=me, overall looks good :) > > > Source/WebKit/WebProcess/WebPage/RemoteWebInspectorUI.cpp:186 > > + // Inspector's diagnostic logging client will never be used unless the page setting is also enabled. > > I think it's more accurate to say "should never be used". OK > > > Source/WebKit/WebProcess/WebPage/RemoteWebInspectorUI.cpp:190 > > + m_frontendAPIDispatcher.dispatchCommand("setDiagnosticLoggingAvailable"_s, available); > > NIT: do you want to use `m_diagnosticLoggingAvailable` instead, as that's > this object's actual state, instead of a parameter? I never know which is preferable if the state is simply copied. I'll use the member. > > > Source/WebKit/WebProcess/WebPage/WebInspectorUI.cpp:306 > > + // Inspector's diagnostic logging client will never be used unless the page setting is also enabled. > > Ditto (RemoteWebInspectorUI.cpp:186) > > > Source/WebKit/WebProcess/WebPage/WebInspectorUI.cpp:310 > > m_frontendAPIDispatcher.dispatchCommand("setDiagnosticLoggingAvailable"_s, available); > > Ditto (RemoteWebInspectorUI.cpp:190) > > > Source/WebKit/WebProcess/WebPage/WebInspectorUI.h:87 > > +#if ENABLE(INSPECTOR_TELEMETRY) > > + void setDiagnosticLoggingAvailable(bool avaliable); > > +#endif > > Why did this move? Oops forgot to mention in ChangeLog. I mistakenly put it below in the last patch, but it should be in the upper part with other methods that are mainly called via IPC messages. > > > Source/WebKit/WebProcess/WebPage/WebInspectorUI.h:160 > > + > > + > > Oops?
Blaze Burg
Comment 7 2019-11-20 12:35:15 PST
Radar WebKit Bug Importer
Comment 8 2019-11-20 12:36:21 PST
Note You need to log in before you can comment on or make changes to this bug.