[webkit-reviews] review denied: [Bug 184890] Ensure DNT is set for redirections handled in NetworkProcess : [Attachment 338619] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied youenn fablet <youennf at gmail.com>'s
request for review:
Bug 184890: Ensure DNT is set for redirections handled in NetworkProcess
https://bugs.webkit.org/show_bug.cgi?id=184890

Attachment 338619: Patch

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




--- Comment #9 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 338619
  --> https://bugs.webkit.org/attachment.cgi?id=338619
Patch

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

> Source/WebKit/ChangeLog:13
> +	   Covered by added test.

Please enumerate tests as done in any other change log entry.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:77
> +	   m_dntHeaderValue = "1";

Why don't we use a boolean flag instead? I guess you're thinking that could
potentially use a different value in the future?

> LayoutTests/http/wpt/fetch/dnt-header-after-redirection.html:21
> +    for (var cptr = 0; cptr < 20; cptr++) {

Why do we need to repeat this 20 times? That seems quite arbitrary.

> LayoutTests/http/wpt/fetch/dnt-header-after-redirection.html:22
> +	   waitFor(test, 10);

What is this await for? Like code, test should make it self evident why every
line of code exists.
This waitFor is very mysterious as is.

> LayoutTests/http/wpt/fetch/dnt-header-after-redirection.html:35
> +	   return Promise.reject("Test requires internals API");

You can just throw an exception instead of explicitly creating a rejected
promise.
But you're not using internals at all so what is this check for?
r- because either this code or comment is wrong.