Bug 240326 – Web Inspector: support EventSource resource type in Network Panel

• Component: Web Inspector
• Status: RESOLVED FIXED

[Yury Semikhatsky]

Web Inspector: support EventSource resource type in Network Panel

[Yury Semikhatsky]

Created attachment 459193 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Devin Rousso]

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?

[Yury Semikhatsky]

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.

[Yury Semikhatsky]

Created attachment 459249 [details]
Patch

[Yury Semikhatsky]

Created attachment 459250 [details]
Patch

[Yury Semikhatsky]

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.

[Devin Rousso]

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 :)

[Yury Semikhatsky]

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.

[Yury Semikhatsky]

> 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?

[Yury Semikhatsky]

Created attachment 459307 [details]
Patch

[Yury Semikhatsky]

(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.

[Yury Semikhatsky]

Created attachment 459441 [details]
Patch

[Yury Semikhatsky]

(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.

[Fujii Hironori]

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.

[Yury Semikhatsky]

Created attachment 459456 [details]
Patch

[Yury Semikhatsky]

(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.

[Yury Semikhatsky]

Devin, I've added Requester enum to the list of exceptions which are allowed to have all upper case values. PTAL.

[Fujii Hironori]

(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.

[Fujii Hironori]

(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

[Yury Semikhatsky]

Created attachment 459520 [details]
Patch

[Yury Semikhatsky]

Pull request: https://github.com/WebKit/WebKit/pull/707

[EWS]

Committed r294374 (250671@main): <https://commits.webkit.org/250671@main>

Reviewed commits have been landed. Closing PR #707 and removing active labels.

[Radar WebKit Bug Importer]

<rdar://problem/93467481>