Bug 223706 - Allow logging minimal info about uploading media files in the system diagnose
Summary: Allow logging minimal info about uploading media files in the system diagnose
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-03-24 12:09 PDT by Said Abou-Hallawa
Modified: 2021-05-10 22:27 PDT (History)
14 users (show)

See Also:


Attachments
Patch (20.60 KB, patch)
2021-03-24 12:45 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (20.46 KB, patch)
2021-03-24 12:55 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (16.03 KB, patch)
2021-03-24 16:02 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (25.07 KB, patch)
2021-03-24 21:48 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2021-03-24 12:09:12 PDT
Give the WebKit client the ability to log info about the media files which was submitted to a web server through an HTMLFormElement.
Comment 1 Said Abou-Hallawa 2021-03-24 12:45:30 PDT
Created attachment 424169 [details]
Patch
Comment 2 Said Abou-Hallawa 2021-03-24 12:55:41 PDT
Created attachment 424171 [details]
Patch
Comment 3 Simon Fraser (smfr) 2021-03-24 13:29:04 PDT
Comment on attachment 424171 [details]
Patch

Is doing this by extending the C API the right way?
Comment 4 Alex Christensen 2021-03-24 13:46:43 PDT
Comment on attachment 424171 [details]
Patch

Let's not add to the C API.  If someone is using the C API that needs to get this new message, they need to migrate to the ObjC API.
I'm also not sure if we need to add LogDiagnosticMessageWithDomain.  Can one of the other logging methods do?
Comment 5 Said Abou-Hallawa 2021-03-24 14:07:53 PDT
(In reply to Alex Christensen from comment #4)
> Comment on attachment 424171 [details]
> Patch
> 
> Let's not add to the C API.  If someone is using the C API that needs to get
> this new message, they need to migrate to the ObjC API.
> I'm also not sure if we need to add LogDiagnosticMessageWithDomain.  Can one
> of the other logging methods do?

Can you give an example which I can follow?
Comment 6 Radar WebKit Bug Importer 2021-03-24 14:17:49 PDT
<rdar://problem/75804136>
Comment 7 Said Abou-Hallawa 2021-03-24 16:02:57 PDT
Created attachment 424196 [details]
Patch
Comment 8 Tim Horton 2021-03-24 16:49:21 PDT
Comment on attachment 424196 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424196&action=review

> Source/WebCore/html/HTMLFormElement.cpp:394
> +        diagnosticLoggingClient.logDiagnosticMessageWithDomain(message, "WebKit.media"_s);

Where did the domain come from? The capitalization is sort of vaguely weird.
Comment 9 Alex Christensen 2021-03-24 17:05:23 PDT
Comment on attachment 424196 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424196&action=review

>> Source/WebCore/html/HTMLFormElement.cpp:394
>> +        diagnosticLoggingClient.logDiagnosticMessageWithDomain(message, "WebKit.media"_s);
> 
> Where did the domain come from? The capitalization is sort of vaguely weird.

It seems like you want to log an integer.  Would logDiagnosticMessageWithValue work?
Comment 10 Alex Christensen 2021-03-24 17:16:15 PDT
Comment on attachment 424196 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424196&action=review

>>> Source/WebCore/html/HTMLFormElement.cpp:394
>>> +        diagnosticLoggingClient.logDiagnosticMessageWithDomain(message, "WebKit.media"_s);
>> 
>> Where did the domain come from? The capitalization is sort of vaguely weird.
> 
> It seems like you want to log an integer.  Would logDiagnosticMessageWithValue work?

Could the domain be an enum instead, and an NS_ENUM at the API layer?  It would start out with just one thing in it, but we could add more later.
Comment 11 Said Abou-Hallawa 2021-03-24 17:27:54 PDT
Comment on attachment 424196 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424196&action=review

>>>> Source/WebCore/html/HTMLFormElement.cpp:394
>>>> +        diagnosticLoggingClient.logDiagnosticMessageWithDomain(message, "WebKit.media"_s);
>>> 
>>> Where did the domain come from? The capitalization is sort of vaguely weird.
>> 
>> It seems like you want to log an integer.  Would logDiagnosticMessageWithValue work?
> 
> Could the domain be an enum instead, and an NS_ENUM at the API layer?  It would start out with just one thing in it, but we could add more later.

I think the enum is a better approach.
Comment 12 Said Abou-Hallawa 2021-03-24 21:48:35 PDT
Created attachment 424216 [details]
Patch
Comment 13 EWS 2021-03-26 10:08:13 PDT
Committed r275103: <https://commits.webkit.org/r275103>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424216 [details].