Variables and parameters used to hold resource load identifiers are variously defined as ResourceLoadIdentifier, uint64_t, or unsigned long. Standardize all of these on ResourceLoadIdentifier. At the same time, define ResourceLoadIdentifier in terms of WTF::ObjectIdentifier to address type-safety and conversion issue.
<rdar://problem/45390318>
Created attachment 352787 [details] Patch
Attachment 352787 [details] did not pass style-queue: ERROR: Source/WebCore/loader/LoaderStrategy.h:94: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 1 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
That check-webkit-style message is benign in this case.
Comment on attachment 352787 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352787&action=review Looks like it's going to be great but I do have a few questions. > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:445 > + auto response = platformStrategies()->loaderStrategy()->responseFromResourceLoadIdentifier(makeResourceLoadIdentifier(identifier)); Why is this method taking in an unsigned long instead of a ResourceLoadIdentifier? > Source/WebCore/loader/FrameLoader.cpp:2994 > + platformStrategies()->loaderStrategy()->loadResourceSynchronously(*this, makeResourceLoadIdentifier(identifier), newRequest, clientCredentialPolicy, options, originalRequestHeaders, error, response, buffer); Seems like requestFromDelegate() should return a ResourceLoadIdentifier? > Source/WebCore/loader/ResourceLoadIdentifier.h:42 > + return generateObjectIdentifier<ResourceLoadIdentifierType>(); We should ASSERT this is always called from the main thread since you're using the non-thread safe variant. > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:342 > + auto completionHandler = [connection = m_connection.copyRef(), identifier = loadParameters.identifier.toUInt64()] (const ResourceError& error, const ResourceResponse& response) { Seems like we should pipe through the ResourceLoadIdentifier? Why going back to a uint64_t now? > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:969 > + return value ? String::number(value.value().toUInt64()) : String("None"); "None"_s is slightly more efficient iirc since you're constructing from a string literal. Also, value->toUInt64() is shorter. > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:1083 > + send(Messages::WebPage::AddConsoleMessage { m_parameters.webFrameID, messageSource, messageLevel, message, identifier().toUInt64() }, m_parameters.webPageID); Why convert back to a uint64_t here? > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:252 > + WebCore::ResourceLoadIdentifier identifier = WebCore::makeResourceLoadIdentifier(resourceLoader.identifier()); I find it troubling that resourceLoader.identifier() does not return a ResourceLoadIdentifier. Why is that?
Making this change turns out to have extensive implications. The ResourceLoadIdentifier type propagates itself all throughout the sources, and even starts to get into areas where it's not clear if are actually dealing with resource load identifiers. Better just to leave things the way they are for now.
Comment on attachment 352787 [details] Patch This has been requesting review for more than one year. If this is still needed, please rebase and re-request review.