HaikuDepot: suggested code improvements from Adrien

src/apps/haikudepot/server/ServerIconExportUpdateProcess.cpp

@@ -10,9 +10,11 @@
 #include <sys/stat.h>
 #include <time.h>
 
+#include <AutoDeleter.h>
 #include <HttpRequest.h>
 #include <Json.h>
 #include <Url.h>
+#include <UrlProtocolRoster.h>
 #include <support/ZlibCompressionAlgorithm.h>
 
 #include "ServerSettings.h"
@@ -130,8 +132,8 @@ ServerIconExportUpdateProcess::_IfModifiedSinceHeaderValue(BString& headerValue)
 status_t
 ServerIconExportUpdateProcess::_Download(BPath& tarGzFilePath)
 {
-	BString urlString = ServerSettings::CreateFullUrl("/__pkgicon/all.tar.gz");
+	return _Download(tarGzFilePath,
-	return _Download(tarGzFilePath, BUrl(urlString), 0, 0);
+		ServerSettings::CreateFullUrl("/__pkgicon/all.tar.gz"), 0, 0);
 }
 
 
@@ -152,10 +154,8 @@ ServerIconExportUpdateProcess::_Download(BPath& tarGzFilePath, const BUrl& url,
 	fprintf(stdout, "will stream '%s' to [%s]\n", url.UrlString().String(),
 		tarGzFilePath.Path());
 
-	bool isSecure = url.Protocol() == BString("https");
 	ToFileUrlProtocolListener listener(tarGzFilePath, "icon-export",
 		ServerSettings::UrlConnectionTraceLoggingEnabled());
-	BUrlContext context;
 
 	BHttpHeaders headers;
 	ServerSettings::AugmentHeaders(headers);
@@ -169,55 +169,51 @@ ServerIconExportUpdateProcess::_Download(BPath& tarGzFilePath, const BUrl& url,
 		headers.AddHeader("If-Modified-Since", ifModifiedSinceHeader);
 	}
 
-	BHttpRequest request(url, isSecure, "HTTP", &listener, &context);
+	BHttpRequest *request = dynamic_cast<BHttpRequest *>(
-	request.SetMethod(B_HTTP_GET);
+		BUrlProtocolRoster::MakeRequest(url, &listener));
-	request.SetHeaders(headers);
+	ObjectDeleter<BHttpRequest> requestDeleter(request);
-	request.SetTimeout(TIMEOUT_MICROSECONDS);
+	request->SetHeaders(headers);
+	request->SetMaxRedirections(0);
+	request->SetTimeout(TIMEOUT_MICROSECONDS);
 
-	thread_id thread = request.Run();
+	thread_id thread = request->Run();
 	wait_for_thread(thread, NULL);
 
 	const BHttpResult& result = dynamic_cast<const BHttpResult&>(
-		request.Result());
+		request->Result());
 
 	int32 statusCode = result.StatusCode();
 
-	switch (statusCode) {
+	if (BHttpRequest::IsSuccessStatusCode(statusCode)) {
-		case HTTP_STATUS_OK:
+		fprintf(stdout, "did complete streaming data\n");
-			fprintf(stdout, "did complete streaming data\n");
+		return B_OK;
-			return B_OK;
+	} else if (statusCode == HTTP_STATUS_NOT_MODIFIED) {
+		fprintf(stdout, "remote data has not changed since [%s]\n",
+			ifModifiedSinceHeader.String());
+		return APP_ERR_NOT_MODIFIED;
+	} else if (BHttpRequest::IsRedirectionStatusCode(statusCode)) {
+		const BHttpHeaders responseHeaders = result.Headers();
+		const char *locationValue = responseHeaders["Location"];
 
-		case HTTP_STATUS_NOT_MODIFIED:
+		if (NULL != locationValue && 0 != strlen(locationValue)) {
-			fprintf(stdout, "remote data has not changed since [%s]\n",
+			BUrl location(locationValue);
-				ifModifiedSinceHeader.String());
+			fprintf(stdout, "will redirect to; %s\n",
-			return APP_ERR_NOT_MODIFIED;
+				location.UrlString().String());
-
-		case HTTP_STATUS_FOUND: // redirect
-		{
-			const BHttpHeaders responseHeaders = result.Headers();
-			const char *locationValue = responseHeaders["Location"];
-
-			if (NULL != locationValue && 0 != strlen(locationValue)) {
-				BUrl location(locationValue);
-				fprintf(stdout, "will redirect to; %s\n",
-					location.UrlString().String());
 				return _Download(tarGzFilePath, location, redirects + 1, 0);
-			}
-
-			fprintf(stdout, "unable to find 'Location' header for redirect\n");
-			return B_IO_ERROR;
 		}
 
-		default:
+		fprintf(stdout, "unable to find 'Location' header for redirect\n");
-			if (0 == statusCode || 5 == (statusCode / 100)) {
+		return B_IO_ERROR;
-				fprintf(stdout, "error response from server; %" B_PRId32 " --> "
+	} else {
-					"retry...\n", statusCode);
+		if (0 == statusCode || 5 == (statusCode / 100)) {
-				return _Download(tarGzFilePath, url, redirects, failures + 1);
+			fprintf(stdout, "error response from server; %" B_PRId32 " --> "
-			}
+				"retry...\n", statusCode);
+			return _Download(tarGzFilePath, url, redirects, failures + 1);
+		}
 
-			fprintf(stdout, "unexpected response from server; %" B_PRId32 "\n",
+		fprintf(stdout, "unexpected response from server; %" B_PRId32 "\n",
-				statusCode);
+			statusCode);
-			return B_IO_ERROR;
+		return B_IO_ERROR;
 	}
 }
 

src/apps/haikudepot/server/ServerSettings.cpp

@@ -13,74 +13,69 @@
 #include <Roster.h>
 #include <Url.h>
 
-#include "AutoLocker.h"
-
 
 #define BASEURL_DEFAULT "https://depot.haiku-os.org"
 #define USERAGENT_FALLBACK_VERSION "0.0.0"
 
 
-BString ServerSettings::fBaseUrl = BString(BASEURL_DEFAULT);
+BUrl ServerSettings::sBaseUrl = BUrl(BASEURL_DEFAULT);
-BString ServerSettings::fUserAgent = BString();
+BString ServerSettings::sUserAgent = BString();
-BLocker ServerSettings::fUserAgentLocker;
+pthread_once_t ServerSettings::sUserAgentInitOnce = PTHREAD_ONCE_INIT;
-bool ServerSettings::fUrlConnectionTraceLogging = false;
+bool ServerSettings::sUrlConnectionTraceLogging = false;
 
 
 status_t
-ServerSettings::SetBaseUrl(const BString& value)
+ServerSettings::SetBaseUrl(const BUrl& value)
 {
-	BUrl url(value);
+	if (!value.IsValid()) {
-
-	if (!url.IsValid()) {
 		fprintf(stderr, "the url is not valid\n");
 		return B_BAD_VALUE;
 	}
 
-	if (url.Protocol() != "http" && url.Protocol() != "https") {
+	if (value.Protocol() != "http" && value.Protocol() != "https") {
 		fprintf(stderr, "the url protocol must be 'http' or 'https'\n");
 		return B_BAD_VALUE;
 	}
 
-	fBaseUrl.SetTo(value);
+	sBaseUrl = value;
-
-	if (fBaseUrl.EndsWith("/")) {
-		fprintf(stderr, "will remove trailing '/' character in url base\n");
-		fBaseUrl.Remove(fBaseUrl.Length() - 1, 1);
-	}
 
 	return B_OK;
 }
 
 
-BString
+BUrl
 ServerSettings::CreateFullUrl(const BString urlPathComponents)
 {
-	return BString(fBaseUrl) << urlPathComponents;
+	return BUrl(sBaseUrl, urlPathComponents);
 }
 
 
 const BString
 ServerSettings::GetUserAgent()
 {
-	AutoLocker<BLocker> lock(&fUserAgentLocker);
+	if (sUserAgent.IsEmpty())
+		pthread_once(&sUserAgentInitOnce, &ServerSettings::_InitUserAgent);
 
-	if (fUserAgent.IsEmpty()) {
+	return sUserAgent;
-		fUserAgent.SetTo("HaikuDepot/");
+}
-		fUserAgent.Append(_GetUserAgentVersionString());
-	}
 
-	return fUserAgent;
+
+const void
+ServerSettings::_InitUserAgent()
+{
+	sUserAgent.SetTo("HaikuDepot/");
+	sUserAgent.Append(_GetUserAgentVersionString());
 }
 
 
 void
 ServerSettings::EnableUrlConnectionTraceLogging() {
-	fUrlConnectionTraceLogging = true;
+	sUrlConnectionTraceLogging = true;
 }
 
 bool
 ServerSettings::UrlConnectionTraceLoggingEnabled() {
-	return fUrlConnectionTraceLogging;
+	return sUrlConnectionTraceLogging;
 }
 
 

src/apps/haikudepot/server/ServerSettings.h

@@ -8,27 +8,28 @@
 
 #include <File.h>
 #include <HttpHeaders.h>
-#include <Locker.h>
 #include <String.h>
+#include <Url.h>
 
 
 class ServerSettings {
 public:
-		static status_t					SetBaseUrl(const BString& baseUrl);
+		static status_t					SetBaseUrl(const BUrl& baseUrl);
 		static const BString			GetUserAgent();
 		static void						AugmentHeaders(BHttpHeaders& headers);
-		static BString					CreateFullUrl(
+		static BUrl						CreateFullUrl(
 											const BString urlPathComponents);
 		static void						EnableUrlConnectionTraceLogging();
 		static bool						UrlConnectionTraceLoggingEnabled();
 
 private:
+		static const void				_InitUserAgent();
 		static const BString			_GetUserAgentVersionString();
 
-		static BString					fBaseUrl;
+		static BUrl						sBaseUrl;
-		static BString					fUserAgent;
+		static BString					sUserAgent;
-		static BLocker					fUserAgentLocker;
+		static pthread_once_t			sUserAgentInitOnce;
-		static bool						fUrlConnectionTraceLogging;
+		static bool						sUrlConnectionTraceLogging;
 };
 
 #endif // SERVER_SETTINGS_H

src/apps/haikudepot/server/WebAppInterface.cpp

@@ -516,11 +516,11 @@ status_t
 WebAppInterface::RetrieveScreenshot(const BString& code,
 	int32 width, int32 height, BDataIO* stream)
 {
-	BString urlString = ServerSettings::CreateFullUrl(BString("/__pkgscreenshot/") << code
+	BUrl url = ServerSettings::CreateFullUrl(
-		<< ".png" << "?tw=" << width << "&th=" << height);
+		BString("/__pkgscreenshot/") << code << ".png" << "?tw="
-	bool isSecure = 0 == urlString.FindFirst("https://");
+			<< width << "&th=" << height);
 
-	BUrl url(urlString);
+	bool isSecure = url.Protocol() == "https";
 
 	ProtocolListener listener(
 		ServerSettings::UrlConnectionTraceLoggingEnabled());
@@ -545,7 +545,7 @@ WebAppInterface::RetrieveScreenshot(const BString& code,
 		return B_OK;
 
 	fprintf(stderr, "failed to get screenshot from '%s': %" B_PRIi32 "\n",
-		urlString.String(), statusCode);
+		url.UrlString().String(), statusCode);
 	return B_ERROR;
 }
 
@@ -629,9 +629,8 @@ WebAppInterface::_SendJsonRequest(const char* domain, BString jsonString,
 	if (ServerSettings::UrlConnectionTraceLoggingEnabled())
 		printf("_SendJsonRequest(%s)\n", jsonString.String());
 
-	BString urlString = ServerSettings::CreateFullUrl(BString("/__api/v1/") << domain);
+	BUrl url = ServerSettings::CreateFullUrl(BString("/__api/v1/") << domain);
-	bool isSecure = 0 == urlString.FindFirst("https://");
+	bool isSecure = url.Protocol() == "https";
-	BUrl url(urlString);
 
 	ProtocolListener listener(
 		ServerSettings::UrlConnectionTraceLoggingEnabled());

src/apps/haikudepot/ui/App.cpp

@@ -197,7 +197,7 @@ App::ArgvReceived(int32 argc, char* argv[])
 					Quit();
 				}
 
-				if (ServerSettings::SetBaseUrl(argv[i + 1]) != B_OK) {
+				if (ServerSettings::SetBaseUrl(BUrl(argv[i + 1])) != B_OK) {
 					fprintf(stdout, "malformed web app base url; %s\n",
 						argv[i + 1]);
 					Quit();

src/apps/haikudepot/util/ToFileUrlProtocolListener.cpp

@@ -6,6 +6,7 @@
 #include "ToFileUrlProtocolListener.h"
 
 #include <File.h>
+#include <HttpRequest.h>
 
 #include <stdio.h>
 
@@ -16,6 +17,7 @@ ToFileUrlProtocolListener::ToFileUrlProtocolListener(BPath path,
 	fDownloadIO = new BFile(path.Path(), O_WRONLY | O_CREAT);
 	fTraceLoggingIdentifier = traceLoggingIdentifier;
 	fTraceLogging = traceLogging;
+	fShouldDownload = true;
 }
 
 
@@ -48,6 +50,19 @@ void
 ToFileUrlProtocolListener::HeadersReceived(BUrlRequest* caller,
 	const BUrlResult& result)
 {
+
+	// check that the status code is success.  Only if it is successful
+	// should the payload be streamed to the file.
+
+	const BHttpResult& httpResult = dynamic_cast<const BHttpResult&>(result);
+	int32 statusCode = httpResult.StatusCode();
+
+	if (!BHttpRequest::IsSuccessStatusCode(statusCode)) {
+		fprintf(stdout, "received %" B_PRId32
+			" --> will not store download to file\n", statusCode);
+		fShouldDownload = false;
+	}
+
 }
 
 
@@ -55,12 +70,13 @@ void
 ToFileUrlProtocolListener::DataReceived(BUrlRequest* caller, const char* data,
 	off_t position, ssize_t size)
 {
-	if (fDownloadIO != NULL && size > 0) {
+	if (fShouldDownload && fDownloadIO != NULL && size > 0) {
 		size_t remaining = size;
 		size_t written = 0;
 
 		do {
-			written = fDownloadIO->Write(&data[size - remaining], remaining);
+			written = fDownloadIO->WriteAt(position, &data[size - remaining],
+				remaining);
 			remaining -= written;
 		} while (remaining > 0 && written > 0);
 

src/apps/haikudepot/util/ToFileUrlProtocolListener.h

@@ -33,9 +33,10 @@ public:
 									const char* text);
 
 private:
+			bool				fShouldDownload;
 			bool				fTraceLogging;
 			BString				fTraceLoggingIdentifier;
-			BDataIO*			fDownloadIO;
+			BPositionIO*		fDownloadIO;
 
 
 };