--nextPart3745021.KasZdrJ3su
Content-Type: text/plain;
  charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline

Roland Harnau wrote:
>2008/6/23, Thiago Macieira <thiago@kde.org>:
>> I didn't like the indention style. Please don't put function opening
>> brackets on the same line as their declaration. You also have
>> whitespace errors (lines ending in whitespace).
>
>The location of the opening brackets is maybe too much of Java style
>for KDE. Attached is somewhat improved patch adhering a bit more to
>kdelibs coding style and  hopefully without superfluous whitespace.

Ok, I have finally got my act together and reviewed the patch.

You made the file a .h (hostinfoagent.h). The class is not public, though.=
=20
Can you make it a private header instead (hostinfoagent_p.h)?

Why do we need a static function to return the instance to call=20
lookupHost()? Can't lookupHost() be static instead?

Please, no private static variables (HostInfoAgent::m_instance). Move the=20
variable to file-static please.

I also don't like the operator<< and operator>> you added. Those become=20
global in any KDE application linking to kio. Please remove them and=20
marshall/demarshall the data manually. (i.e., move the code from those=20
operators into the functions that are calling them now, that is,=20
KIO::SlaveBase::waitForHostInfo and KIO::SlaveInterface::setHostInfo=20
[btw, this last one has a bad name for a slot])

KIO::SlaveInterface is a public class, so please no private members. Use=20
Q_PRIVATE_SLOT.

In:
+        while (it.hasNext()){
+            d->socket.connectToHost(it.next(), port);
+            bool connectOk =3D d->socket.waitForConnected(d->timeout > -1 =
?=20
d->timeout * 1000 : -1);
+            if (connectOk) break;
+        }

You should be decrementing the timeout value there. Otherwise, if a target=
=20
has 6 IP addresses, we could end up waiting up to 3 minutes before a=20
timeout. I suggest:

	QTime timer;
	timer.start();
	while (it.hasNext()) {
		d->socket.connectToHost(it.next(), port);
		int timeout =3D -1;
		if (d->timeout > -1)
			timeout =3D d->timeout - timer.elapsed();
		if (!d->socket.waitForConnected(timeout))
			break;
	}

in HostInfoAgentPrivate::lookupHost
+    if(dnsCache.contains(hostName)) {
+        QPair<QHostInfo, QTime> info (*dnsCache.object(hostName));

Please void the double lookup of contains + object. Simply call object()=20
and test for a 0 return.

The same in:
+    if (openQueries.contains(hostName)) {
+        Query* query =3D openQueries.value(hostName);

Please use an iterator and compare to openQueries.end() to see if the=20
object is there.

You don't need Q_SIGNALS and Q_EMIT in a .cpp file. You can use signals:=20
and emit. Not a big deal though.


In all, your patch is very good and very clever. The only drawback (=3D=20
possible future improvement) is the fact that the IP addresses will be=20
tried always in the same order. Right now, the DNS server will change the=20
order in a round-robin fashion at every request.

=2D-=20
=A0 Thiago Macieira =A0- =A0thiago (AT) macieira.info - thiago (AT) kde.org
=A0 =A0 PGP/GPG: 0x6EF45358; fingerprint:
=A0 =A0 E067 918B B660 DBD1 105C =A0966C 33F5 F005 6EF4 5358

--nextPart3745021.KasZdrJ3su
Content-Type: application/pgp-signature; name=signature.asc 
Content-Description: This is a digitally signed message part.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)

iD8DBQBIareHM/XwBW70U1gRAtb/AJ0erPsKK0GMn0f3A/EE8ZDuqhz6YwCfeF7k
F1zP6sWIqtgrN7L9aXQse10=
=AsoD
-----END PGP SIGNATURE-----

--nextPart3745021.KasZdrJ3su--