WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(19.13 KB, patch)
2019-11-19 16:58 PST
,
Blaze Burg
hi
: review+
ews-watchlist
: commit-queue-
Details
Formatted Diff
Diff
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
Details
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Blaze Burg
Comment 1
2019-11-19 16:54:41 PST
Created
attachment 383923
[details]
Patch
Blaze Burg
Comment 2
2019-11-19 16:58:25 PST
Created
attachment 383925
[details]
Patch
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
Committed
r252702
: <
https://trac.webkit.org/changeset/252702
>
Radar WebKit Bug Importer
Comment 8
2019-11-20 12:36:21 PST
<
rdar://problem/57368960
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug