Bug 47041 – Magical true/false parameters are difficult to understand when creating ResourceHandle

• Component: New Bugs
• Status: RESOLVED LATER

[Adam Barth]

Magical true/false parameters are difficult to understand when creating ResourceHandle

[Adam Barth]

Created attachment 69562 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 69562 [details]
Patch

+        m_loader = SubresourceLoader::create(m_document->frame(), this, request, securityCheck, sendLoadCallbacks, contentSniffingOption);

It's challenging to come up with good names for these enums and arguments, but I don't remember us ever using "Option" for that. Maybe it's better than what we did before, and we should standardize on it?

+#include "ResourceHandle.h"

Please remove forward declaration of ResourceHandle from ResourceLoader.h, now that you include the header.

The need for additional includes is a common problem with these enums, too. Logically, code that constructs a ResourceLoader shouldn't need to know that there are ResourceHandles below, or to include ResourceHandle.h.

-    if (!shouldContentSniff) {
+    if (contentSniffingOption == ShouldNotSniffContent) {

I don't think that this part is an improvement. All kinds of questions like "are there any other possible values" immediately arise, making the code less readable.

At some sufficiently low level, bools should probably remain bools.

I chose to be extra picky in this review, despite the fact that these problems are well known and not well solved in WebKit code base, because I suspect that you're up to a large scale refactoring here. That would be a good moment to pause and take a second look.

[Adam Barth]

> I chose to be extra picky in this review, despite the fact that these problems are well known and not well solved in WebKit code base, because I suspect that you're up to a large scale refactoring here. That would be a good moment to pause and take a second look.

Oh, I appreciate your being extra picky.  These patches are my way of exploring the code while improving it.  Do you think the enum approach isn't worth doing?  It seems helpful at the call sites where we just pass raw bools today.

In general, I like the way ThreadableLoader works where you pass in an options object.  Would you like me try an approach like that?

[Darin Adler]

Comment on attachment 69562 [details]
Patch

Looks fine. In some other cases we have called these “policies” rather than “options”, so that sort of alternate terminology might be worth considering.

[Eric Seidel (no email)]

What's the status here Adam?

[Adam Barth]

This patch requires some changes before landing, but I'm busy with other patches at the moment.  I'd like to come back to this patch, but for now I'm markings this as LATER so it doesn't pollute the pending-commit list.