Web Inspector: support EventSource resource type in Network Panel
Created attachment 459193 [details] Patch
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment on attachment 459193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=459193&action=review > Source/WebInspectorUI/UserInterface/Models/Resource.js:178 > + return WI.UIString("EventSources"); > + return WI.UIString("EventSource"); Can you please add a comment to these localized strings so that localizers can understand this comes from the JS EventSource API (i.e. a named term/thing, not the programmer forgetting to add a space)? > LayoutTests/http/tests/inspector/network/eventsource-type.html:8 > + internals.settings.setHyperlinkAuditingEnabled(true); is this related? > LayoutTests/http/tests/inspector/network/eventsource-type.html:11 > + const eventSource = new EventSource('resources/event-source.py'); Style: please use four spaces > LayoutTests/http/tests/inspector/network/eventsource-type.html:28 > + test(resolve, reject) { NIT: can we rewrite this with an `async test()` instead so that it's a bit nicer to read? > LayoutTests/http/tests/inspector/network/eventsource-type.html:40 > + return resource.requestContentFromBackend(); ditto (:11) > LayoutTests/http/tests/inspector/network/eventsource-type.html:43 > + resourceHandler(resource, content); ditto (:11) > LayoutTests/http/tests/inspector/network/eventsource-type.html:51 > + function alwaysTest(resource) { > + } oops? > LayoutTests/http/tests/inspector/network/eventsource-type.html:54 > + name: "Resource.Type.EventSource.200", are there any other cases we should test? what about an `EventSource` that has multiple messages? > LayoutTests/http/tests/inspector/network/eventsource-type.html:59 > + InspectorTest.expectEqual(JSON.stringify(content), '{"body":"Success\\r\\n","base64Encoded":false}', "Resource should receive 'Success' in the response."); This seems a bit fragile to change if we modify `WI.SourceCode`. Can we instead check that `content.body` is a certain value and `content.base64Encoded` is a certain value (i.e. allow for other keys to be added that are not relevant)? > LayoutTests/http/tests/inspector/network/resources/event-source.py:7 > +# status = int(parse_qs(os.environ.get('QUERY_STRING', ''), keep_blank_values=True).get('status', [200])[0]) oops? > LayoutTests/http/tests/inspector/network/resources/event-source.py:13 > + 'status: 200\r\n' do we have to use a carriage return? or can we just use only newlines?
Comment on attachment 459193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=459193&action=review >> Source/WebInspectorUI/UserInterface/Models/Resource.js:178 >> + return WI.UIString("EventSource"); > > Can you please add a comment to these localized strings so that localizers can understand this comes from the JS EventSource API (i.e. a named term/thing, not the programmer forgetting to add a space)? Added to localizedStrings.js, I don't see comments for any other types so please let me know if you meant a different location. >> LayoutTests/http/tests/inspector/network/eventsource-type.html:8 >> + internals.settings.setHyperlinkAuditingEnabled(true); > > is this related? Removed. >> LayoutTests/http/tests/inspector/network/eventsource-type.html:11 >> + const eventSource = new EventSource('resources/event-source.py'); > > Style: please use four spaces Done. >> LayoutTests/http/tests/inspector/network/eventsource-type.html:28 >> + test(resolve, reject) { > > NIT: can we rewrite this with an `async test()` instead so that it's a bit nicer to read? Done. >> LayoutTests/http/tests/inspector/network/eventsource-type.html:40 >> + return resource.requestContentFromBackend(); > > ditto (:11) Done. >> LayoutTests/http/tests/inspector/network/eventsource-type.html:43 >> + resourceHandler(resource, content); > > ditto (:11) Done. >> LayoutTests/http/tests/inspector/network/eventsource-type.html:51 >> + } > > oops? Removed. >> LayoutTests/http/tests/inspector/network/eventsource-type.html:54 >> + name: "Resource.Type.EventSource.200", > > are there any other cases we should test? > > what about an `EventSource` that has multiple messages? Added another test with 3 events. >> LayoutTests/http/tests/inspector/network/eventsource-type.html:59 >> + InspectorTest.expectEqual(JSON.stringify(content), '{"body":"Success\\r\\n","base64Encoded":false}', "Resource should receive 'Success' in the response."); > > This seems a bit fragile to change if we modify `WI.SourceCode`. Can we instead check that `content.body` is a certain value and `content.base64Encoded` is a certain value (i.e. allow for other keys to be added that are not relevant)? Fixed. >> LayoutTests/http/tests/inspector/network/resources/event-source.py:7 >> +# status = int(parse_qs(os.environ.get('QUERY_STRING', ''), keep_blank_values=True).get('status', [200])[0]) > > oops? Removed. >> LayoutTests/http/tests/inspector/network/resources/event-source.py:13 >> + 'status: 200\r\n' > > do we have to use a carriage return? or can we just use only newlines? I believe we do by the HTTP spec.
Created attachment 459249 [details] Patch
Created attachment 459250 [details] Patch
Comment on attachment 459193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=459193&action=review >>> LayoutTests/http/tests/inspector/network/resources/event-source.py:13 >>> + 'status: 200\r\n' >> >> do we have to use a carriage return? or can we just use only newlines? > > I believe we do by the HTTP spec. To clarify, I updated the code to send events separated by \n as the spec permits and preserved \r\n for the headers.
Comment on attachment 459250 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=459250&action=review r=me, neat! > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:647 > +/* Display name for the type of network requests sent via EventSource API (https://developer.mozilla.org/en-US/docs/Web/API/EventSource) */ This needs to be added in the `WI.UIString` in the code as well, as otherwise the next time someone generates this file it wont have this comment. ``` WI.UIString("EventSource", "Display name for the type of network requests sent via EventSource API (https://developer.mozilla.org/en-US/docs/Web/API/EventSource)") ``` > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:649 > +localizedStrings["EventSources"] = "EventSources"; please also add a similar comment for this too :)
Comment on attachment 459250 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=459250&action=review >> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:647 >> +/* Display name for the type of network requests sent via EventSource API (https://developer.mozilla.org/en-US/docs/Web/API/EventSource) */ > > This needs to be added in the `WI.UIString` in the code as well, as otherwise the next time someone generates this file it wont have this comment. > ``` > WI.UIString("EventSource", "Display name for the type of network requests sent via EventSource API (https://developer.mozilla.org/en-US/docs/Web/API/EventSource)") > ``` Done. >> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:649 >> +localizedStrings["EventSources"] = "EventSources"; > > please also add a similar comment for this too :) Done.
> EWS has detected build failure on Windows-EWS while testing Patch 459250 for Bug 240326. > > Full details are available at: https://ews-> build.webkit.org/#/builders/10/builds/134348 > > Error lines: > > C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector/agents/InspectorPageAgent.cpp(239,46): error C2838: 'EventSource': illegal qualified name in member declaration (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-3.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj] > C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector/agents/InspectorPageAgent.cpp(239,1): error C2440: 'return': cannot con vert from 'WebCore::InspectorPageAgent::ResourceType' to 'Inspector::Protocol::Page::ResourceType' (compiling source file > C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-3.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj] I'm not sure what to do about these errors. Looks like Windows needs a clean build, is there a way to trigger that?
Created attachment 459307 [details] Patch
(In reply to Yury Semikhatsky from comment #10) > > EWS has detected build failure on Windows-EWS while testing Patch 459250 for Bug 240326. > > > > Full details are available at: https://ews-> build.webkit.org/#/builders/10/builds/134348 > > > > Error lines: > > > > C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector/agents/InspectorPageAgent.cpp(239,46): error C2838: 'EventSource': illegal qualified name in member declaration (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-3.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj] > > C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector/agents/InspectorPageAgent.cpp(239,1): error C2440: 'return': cannot con vert from 'WebCore::InspectorPageAgent::ResourceType' to 'Inspector::Protocol::Page::ResourceType' (compiling source file > C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-3.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj] > > I'm not sure what to do about these errors. Looks like Windows needs a clean > build, is there a way to trigger that? Fuji, any advice how to deal with this? The patch compiles locally on my Windows box without errors.
Created attachment 459441 [details] Patch
(In reply to Yury Semikhatsky from comment #12) > (In reply to Yury Semikhatsky from comment #10) > > > EWS has detected build failure on Windows-EWS while testing Patch 459250 for Bug 240326. > > > > > > Full details are available at: https://ews-> build.webkit.org/#/builders/10/builds/134348 > > > > > > Error lines: > > > > > > C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector/agents/InspectorPageAgent.cpp(239,46): error C2838: 'EventSource': illegal qualified name in member declaration (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-3.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj] > > > C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector/agents/InspectorPageAgent.cpp(239,1): error C2440: 'return': cannot con vert from 'WebCore::InspectorPageAgent::ResourceType' to 'Inspector::Protocol::Page::ResourceType' (compiling source file > C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-3.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj] > > > > I'm not sure what to do about these errors. Looks like Windows needs a clean > > build, is there a way to trigger that? > > Fuji, any advice how to deal with this? The patch compiles locally on my > Windows box without errors. Ok, I cannot make sense of that error, looks like MSVS compiler problem to me given that it compiles with gcc and clang. Renamed the enum value to EventSource -> EventSourceResource which matches other names in the same enum and will hopefully fix the windows compilation.
It looks like a incremental build problem. AppleWin is still using CMake Visual Studio generator which is causing some incremental build problems. Fix the bug if you have a time. I think the next incremental build will succeed. Most WebKit developers just ignore the problem.
Created attachment 459456 [details] Patch
(In reply to Fujii Hironori from comment #15) > It looks like a incremental build problem. AppleWin is still using CMake > Visual Studio generator which is causing some incremental build problems. > Fix the bug if you have a time. > I think the next incremental build will succeed. Most WebKit developers just > ignore the problem. Got it, I let's see if commit-queue allows to submit with that bot failure, otherwise I guess I'll have to figure out how to do it manually.
Devin, I've added Requester enum to the list of exceptions which are allowed to have all upper case values. PTAL.
(In reply to Yury Semikhatsky from comment #17) > let's see if commit-queue allows to submit with that bot failure, The commit bot is checking only mac builds before landing.
(In reply to Fujii Hironori from comment #15) > It looks like a incremental build problem. AppleWin is still using CMake > Visual Studio generator which is causing some incremental build problems. Filed: Bug 240540 – [CMake][Visual Studio] PrivateHeaders/JavaScriptCore/InspectorProtocolObjects.h isn't regenerated in incremental builds
Created attachment 459520 [details] Patch
Pull request: https://github.com/WebKit/WebKit/pull/707
Committed r294374 (250671@main): <https://commits.webkit.org/250671@main> Reviewed commits have been landed. Closing PR #707 and removing active labels.
<rdar://problem/93467481>