Rename non-idl OnLookupComplete methods to something better

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: valentin, Assigned: valentin)

Tracking

(Blocks 1 bug)

Trunk
mozilla59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(2 attachments)

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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

a year ago
mozreview-review
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

a year ago
mozreview-review
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 hidden (mozreview-request)
(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

a year ago
mozreview-review
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.
Attachment #8935535 - Flags: review?(honzab.moz) → review+

Comment 8

a year ago
mozreview-review
Comment on attachment 8935536 [details]
Bug 1424066 - Rename nsResolveHostCallback.OnLookupComplete and nsHostResolver.OnLookupComplete

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

like it!  thanks.
Attachment #8935536 - Flags: review?(honzab.moz) → review+

Comment 9

a year ago
Pushed by valentin.gosu@gmail.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c5d7fc09608f
https://hg.mozilla.org/mozilla-central/rev/cca2e98fefc8
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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
You need to log in before you can comment on or make changes to this bug.