urls [nfc]: Pass around Uri objects more (cut out some unneeded re-stringification)

[chrisbobbe]

Helpful anyway, I think, but this came up when thinking about avatar URLs, for #119.

[gnprice]

Thanks! Looks good; one nit below. In a commit message:

The most interesting consequence of this is that _LightboxHeroTag
now compares `src` using Uri's == operator override, instead of
string equality on the result of Uri.toString(). I expect that to
produce the same result, and at least as efficiently:
  https://github.com/dart-lang/sdk/blob/9bf4af004/sdk/lib/core/uri.dart

It looks like specifically that's this:

  bool operator ==(Object other) {
    if (identical(this, other)) return true;
    return other is Uri &&
        scheme == other.scheme &&
        hasAuthority == other.hasAuthority &&
        userInfo == other.userInfo &&
        host == other.host &&
        port == other.port &&
        path == other.path &&
        hasQuery == other.hasQuery &&
        query == other.query &&
        hasFragment == other.hasFragment &&
        fragment == other.fragment;

That looks slower to me than a string comparison — it's doing a bunch of separate string comparisons of different pieces. But it's fine, because this shouldn't be a hot path.

[gnprice]

Suggested change

	required Uri src
	required Uri src,

(since we're here)

[chrisbobbe]

Thanks for the review! Revision pushed.

That looks slower to me than a string comparison — it's doing a bunch of separate string comparisons of different pieces.

I guess that makes sense. Each string comparison might come with some overhead, making it slower even if all the pieces add up to make the whole string.

There's also this implementation:

  bool operator ==(Object other) {
    if (identical(this, other)) return true;
    return other is Uri && _uri == other.toString();
  }

That's on class _SimpleUri implements Uri, whereas the one you quoted was on class _Uri implements Uri.

[chrisbobbe]

Updated to change the type of RealmContentNetworkImageChecks.src, added in #246.

[gnprice]

I guess that makes sense. Each string comparison might come with some overhead, making it slower even if all the pieces add up to make the whole string.

Right, exactly. And even if the overhead is negligible, it won't be negative ­— the multiple-strings version won't be faster.

There's also this implementation:

Hmm, yeah. Studying the parser a bit, it looks like most URLs we encounter are likely to end up as that _SimpleUri. (In particular _createTables describes which URLs the implementation is designed to treat as "simple".) That's good, then.

Still not any faster than the string comparison, as the heart of it is just the same string comparison.

[gnprice]

Thanks! Looks good; merging.

[chrisbobbe]

Thanks!

[chrisbobbe]

Updated to change the type of RealmContentNetworkImageChecks.src, added in #246.

Hmm, did you accidentally clobber this part in the revision you merged?

RealmContentNetworkImageChecks should read:

extension RealmContentNetworkImageChecks on Subject<RealmContentNetworkImage> {
  Subject<Uri> get src => has((i) => i.src, 'src');
  // TODO others
}

(with Subject<Uri>, not Subject<String>).

[gnprice]

Hmm, did you accidentally clobber this part in the revision you merged?

Oops, yeah. I must have not re-fetched after your last revision, and then not rerun tests after rebase. Please go ahead and send a fix, sorry.

[chrisbobbe]

Makes sense; just sent #257.