1424066 - Rename non-idl OnLookupComplete methods to something better

Status: RESOLVED FIXED

(Core :: Networking: DNS, enhancement, P3)

Description

[Valentin Gosu [:valentin] (he/him)]

We have several different methods called OnLookupComplete, with similar but different arguments. This makes it difficult to reason about the code, when lots of methods are called the same.

Comment 1

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1424066 - Use RefPtr to hold nsHostResolver in nsHostResolver::ThreadFunc

Review commit: https://reviewboard.mozilla.org/r/206412/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206412/

Comment 2

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1424066 - Rename nsResolveHostCallback.OnLookupComplete and nsHostResolver.OnLookupComplete

We have several different methods called OnLookupComplete, with similar but different arguments.
This makes it difficult to reason about the code, when lots of methods are called the same.

* renames nsResolveHostCallback.OnLookupComplete to nsResolveHostCallback.OnHostRecordAvailable
* renames nsHostResolver.OnLookupComplete to nsHostResolver.CompleteLookup

Review commit: https://reviewboard.mozilla.org/r/206414/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206414/

Comment 3

[Daniel Stenberg [:bagder]]

Comment on attachment 8935536 [details]
Bug 1424066 - Rename nsResolveHostCallback.OnLookupComplete and nsHostResolver.OnLookupComplete

https://reviewboard.mozilla.org/r/206414/#review212040

::: netwerk/dns/nsDNSService2.cpp:337
(Diff revision 1)
>      uint16_t                 mAF;
>      nsCString                mNetworkInterface;
>  };
>  
>  void
> -nsDNSAsyncRequest::OnLookupComplete(nsHostResolver *resolver,
> +nsDNSAsyncRequest::OnHostRecordAvailable(nsHostResolver *resolver,

How about OnResolveHostComplete instead? The "host record" is already available so I think it should be named according to the particular action that the call implies.

Comment 4

[Daniel Stenberg [:bagder]]

Comment on attachment 8935536 [details]
Bug 1424066 - Rename nsResolveHostCallback.OnLookupComplete and nsHostResolver.OnLookupComplete

https://reviewboard.mozilla.org/r/206414/#review212052

::: netwerk/dns/nsDNSService2.cpp:337
(Diff revision 1)
>      uint16_t                 mAF;
>      nsCString                mNetworkInterface;
>  };
>  
>  void
> -nsDNSAsyncRequest::OnLookupComplete(nsHostResolver *resolver,
> +nsDNSAsyncRequest::OnHostRecordAvailable(nsHostResolver *resolver,

Gah, I think I even fell in the trap this is meant to fix and commented about the wrong OnLookupComplete! Sorry, my bad. Comment withdrawn.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8935536 [details]
Bug 1424066 - Rename nsResolveHostCallback.OnLookupComplete and nsHostResolver.OnLookupComplete

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206414/diff/1-2/

Comment 6

[Valentin Gosu [:valentin] (he/him)]

(In reply to Daniel Stenberg [:bagder] from comment #4)
> > +nsDNSAsyncRequest::OnHostRecordAvailable(nsHostResolver *resolver,
> 
> Gah, I think I even fell in the trap this is meant to fix and commented
> about the wrong OnLookupComplete! Sorry, my bad. Comment withdrawn.

Actually it was a good suggestion. Hopefully it stops being confusing after we land it.

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 8935535 [details]
Bug 1424066 - Use RefPtr to hold nsHostResolver in nsHostResolver::ThreadFunc

https://reviewboard.mozilla.org/r/206412/#review212294

a bit out of the scope of the bug, but looks good.

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 8935536 [details]
Bug 1424066 - Rename nsResolveHostCallback.OnLookupComplete and nsHostResolver.OnLookupComplete

https://reviewboard.mozilla.org/r/206414/#review212300

like it!  thanks.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c5d7fc09608f
Use RefPtr to hold nsHostResolver in nsHostResolver::ThreadFunc r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/cca2e98fefc8
Rename nsResolveHostCallback.OnLookupComplete and nsHostResolver.OnLookupComplete r=mayhemer

Comment 10

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/c5d7fc09608f
https://hg.mozilla.org/mozilla-central/rev/cca2e98fefc8

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

there is an improvement in build times from this push:
== Change summary for alert #10954 (as of Fri, 08 Dec 2017 21:34:18 GMT) ==

Improvements:

  6%  build times windows2012-32 opt static-analysis taskcluster-c4.4xlarge     2,512.74 -> 2,362.68

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=10954