refactor(ads-client): move a constructor onto its type and retire the common drawer

[Almaju]

Two small refactors in ads-client, no behaviour change.

pop_request_hash_from_url built a RequestHash but lived in mars::ad_response, which otherwise holds MARS wire types. It's now RequestHash::pop_from_url over in http_cache::request_hash, next to the type it builds, and its test moved with it. Both call sites are still commented out pending the Nimbus cache invalidation work, so the allow(dead_code) came across unchanged.

common held bytesize and clock: two unrelated modules under a name that says nothing about either. They're now crate::bytesize and crate::clock. Imports updated at 14 call sites, nothing inside either module touched.

Independent of #7612, doesn't depend on it.

Pull Request checklist

• Breaking changes: none for the bindings. crate::common::{bytesize, clock} becomes crate::{bytesize, clock}, but consumers use the uniffi API.
• Quality: fmt and clippy --all-targets -- -D warnings clean, 101 unit tests pass.
• Tests: no new behaviour. The moved test is renamed for what it asserts.
• Changelog: no entry, nothing user-visible changes.
• Dependencies: none added.

[jonesetc]

At first being a method on the impl felt a bit odd because it was static and felt a little bit javaish, but since it is returning an optional instance out of it it does make more sense. It's kinda like a try_from or try_into, but not quite either. Is there any sort of general interface that matches this that could simplify the concept? Something that takes a mutable ref and returns a new instance?

[thesuzerain]

This makes sense to me. The 'ideal' place for such a mutable pop method would be on Url itself, but obviously that's not so possible here as we don't own Url. I can't think off-hand of an equivalent std constructor though where the passed field is mutable. We could newtype or create a trait to be able to put the function on Url but those seem like overkill to me- I'm fine with it either as a function or on RequestHash.

[Almaju]

Sorry for the late response.

That specific change came from the free-function rule. When a free function takes or returns a local type, the rule expects that function to live on the type, usually in an impl block.

I agree this looks a lot like TryFrom, but it's less constrained. With TryFrom we'd have to switch the return type from Option to Result, which is annoying. The rule doesn't care which one we pick. We can use TryFrom, a custom trait or an associated function. The only thing it flags is a free function that is really a constructor.

So the rule doesn't conflict with TryFrom or custom traits, and it doesn't stop us from using them. Like the orphan rule, it only looks at local types. If none of the types are local, it won't fire, so fn add(a: i32, b: i32) -> i32 is still fine.