[webkit-reviews] review denied: [Bug 225636] Allow logging minimal info about sending media files through XMLHttpRequest : [Attachment 428302] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 225636: Allow logging minimal info about sending media files through
XMLHttpRequest
https://bugs.webkit.org/show_bug.cgi?id=225636

Attachment 428302: Patch

https://bugs.webkit.org/attachment.cgi?id=428302&action=review




--- Comment #5 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 428302
  --> https://bugs.webkit.org/attachment.cgi?id=428302
Patch

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

> Source/WebCore/html/HTMLFormElement.cpp:408
> +   
document().page()->logSubmittedImageOrMediaFiles(imageOrMediaFilesCount);

Let's null check page:
if (auto* page = document().page())
    page->...

> Source/WebCore/page/Page.cpp:1056
> +void Page::logSubmittedImageOrMediaFiles(unsigned imageOrMediaFilesCount)

Why don't we just make one function and pass a FormData& as a parameter instead
of one function to count then another function to log?

> Source/WebCore/xml/XMLHttpRequest.cpp:549
> +	   if (auto page = scriptExecutionContext()->isDocument() ?
document()->page() : nullptr)

auto*

It's also not great to introduce code in XHR that only works in document
context.  This fixes another case of unreported FormData uploading, but workers
are the next case and fetch is probably the one after that.  This might be
nicer in a more central location, like if we added a request parameter to
addParametersShared and checked its HTTP body there, instead of one in
HTMLFormElement and one in xhr.