The jmb/new-cache branch has been merged as of 10180.
Please note the attached review points are yet to be addressed.
D.
--
Daniel Silverstone http://www.netsurf-browser.org/
PGP mail accepted and encouraged. Key Id: 3CCE BABE 206C 3B69
Index: utils/http.h
Index: content/llcache.h
+/** Flags for low-level cache object retrieval */
+#define LLCACHE_RETRIEVE_FORCE_FETCH (1 << 0) /* Force a new fetch */
+#define LLCACHE_RETRIEVE_VERIFIABLE (1 << 1) /* Requested URL was verified */
+#define LLCACHE_RETRIEVE_SNIFF_TYPE (1 << 2) /* Permit content-type sniffing
*/
+#define LLCACHE_RETRIEVE_NO_ERROR_PAGES (1 << 3) /* No error pages */
enum please -- for documentation joy -- factually inaccurate
+typedef nserror (*llcache_query_response)(bool proceed, void *cbpw);
+typedef nserror (*llcache_query_callback)(const llcache_query *query, void *pw,
+ llcache_query_response cb, void *cbpw);
register me?
+/**
+ * Poll the low-level cache
+ *
+ * \return NSERROR_OK on success, appropriate error otherwise.
+ */
+nserror llcache_poll(void);
What does it do?
+ * \todo Make the key an enumeration, to avoid needless string comparisons
+ * \todo Forcing the client to parse the header value seems wrong.
+ * Better would be to return the actual value part and an array of
+ * key-value pairs for any additional parameters.
+const char *llcache_handle_get_header(const llcache_handle *handle,
+ const char *key);
\todo Deal with multiple headers of the same key. (e.g. Set-Cookie)
Index: content/hlcache.c
+struct hlcache_retrieval_ctx {
+ const char *charset; /**< Fallback charset, or NULL */
+ bool quirks; /**< Whether object should be quirky */
Embed child context from public API, rather than separate fields.
+nserror hlcache_handle_release(hlcache_handle *handle)
+{
+ /** \todo What if this is called during fetch? */
remove todo?
\todo This leaks, like a welshman!
+/**
+ * Handler for low-level cache events
... until such time as the content object can take over for us.
+ *
+ * \param handle Handle for which event is issued
+ * \param event Event data
+ * \param pw Pointer to client-specific data
+ * \return NSERROR_OK on success, appropriate error otherwise
+ */
+nserror hlcache_llcache_callback(llcache_handle *handle,
+ const llcache_event *event, void *pw)
+{
+ hlcache_retrieval_ctx *ctx = pw;
+ nserror error;
+
+ assert(ctx->llcache == handle);
+
+ switch (event->type) {
+ case LLCACHE_EVENT_HAD_HEADERS:
+ error = hlcache_find_content(ctx);
+ if (error != NSERROR_OK)
+ return error;
+ /* No longer require retrieval context */
... because from now on, the content object will handle the callbacks.
+ free(ctx);
+ break;
+ case LLCACHE_EVENT_HAD_DATA:
+ /* fall through */
+ case LLCACHE_EVENT_DONE:
+ /* should never happen: the handler must be changed */
assert(0 == "OMG!");
Index: content/content_protected.h
+ * Copyright 2005-2007 James Bursa <[email protected]>
+ * Copyright 2003 Philip Pemberton <[email protected]>
Add your copyright (if only for the year)
Index: content/llcache.c
+ * Copyright 2009 John-Mark Bell <[email protected]>
This year also?
+#define _GNU_SOURCE /* For strndup. Ugh. */
Libibibibibibiberty?
+/** Low-level cache object user record */
+typedef struct llcache_object_user {
+ /* Must be first in struct */
+ llcache_handle handle; /**< Handle data for client */
+
+ bool iterator_target; /**< This is the an iterator target */
+ bool queued_for_delete; /**< This user is queued for deletion */
+
+ struct llcache_object_user *prev; /**< Previous in list */
+ struct llcache_object_user *next; /**< Next in list */
+} llcache_object_user;
worth using the util/ring.h facilities?
+/** Cache control data */
+typedef struct {
+ time_t req_time; /**< Time of request */
+ time_t res_time; /**< Time of response */
+ time_t date; /**< Date: response header */
+ time_t expires; /**< Expires: response header */
+#define INVALID_AGE -1
*NO* Do not define things inside structs. Bad John-Mark!
Copy and paste is not a valid programming style!
Especially from fetchcache.*
+ int age; /**< Age: response header */
+ int max_age; /**< Max-Age Cache-control parameter */
+ bool no_cache; /**< No-Cache Cache-control parameter */
+ char *etag; /**< Etag: response header */
+ time_t last_modified; /**< Last-Modified: response header */
+} llcache_cache_control;
+/** Low-level cache object */
+/** \todo Consider whether a list is a sane container */
maybe a ring?
+/**
+ * Initialise the low-level cache
+ *
+ * \param cb Query handler
+ * \param pw Pointer to query handler data
+ * \return NSERROR_OK on success, appropriate error otherwise.
Given that you claim that, how about an NSERROR_INITIALISED to prevent multiple
calls?
+/**
+ * Poll the low-level cache
This was already documented in the header I think, please check and deduplicate
docstrings.
+nserror llcache_object_retrieve(const char *url, uint32_t flags,
+ const char *referer, const llcache_post_data *post,
+ llcache_object **result)
+ * \todo Find out if restriction (2) can be removed
I firmly believe it can be.
+nserror llcache_fetch_redirect(llcache_object *object, const char *target,
+ llcache_object **replacement)
+{
+ /** \todo Limit redirect depth, or detect cycles */
Definitely need a depth limit. That'll handle cycles implicitly.
+ /** \todo Ensure that redirects to file:/// don't happen? */
Spec?
+ /** \todo What happens if we've no way of handling this URL? */
Same thing as when we start requesting something we know nothing about.
+nserror llcache_fetch_notmodified(llcache_object *object,
+ llcache_object **replacement)
+ /** \todo Ensure that old object gets flushed from the cache */
Is it dangerous to keep around; or is this simply a chance to thread into
llcache_clean?
+nserror llcache_fetch_split_header(const char *data, size_t len, char **name,
+ char **value)
+{
pls. to use http library to do this?
+nserror llcache_fetch_process_header(llcache_object *object, const char *data,
+ size_t len)
+ /* Append header data to the object's headers array */
+ temp = realloc(object->headers, (object->num_headers + 1) *
+ sizeof(llcache_header));
At this point, I think headers should be a list, not an array.
+nserror llcache_fetch_process_data(llcache_object *object, const uint8_t
*data,
+ size_t len)
+{
\todo consider if the source buffer could be a chain to reduce heap churn
+nserror llcache_fetch_cert_error(llcache_object *object,
+ const struct ssl_cert_info *certs, size_t num)
+{
+ nserror error = NSERROR_OK;
+
+ /* Abort fetch for this object */
+ fetch_abort(object->fetch.fetch);
+ object->fetch.fetch = NULL;
+
+ if (query_cb != NULL) {
There's a lot of code which does "if I have a query_cb..." perhaps just a
default query_cb might help?
Index: content/hlcache.h
===================================================================
--- /dev/null 2009-08-02 12:10:37.056245000 +0100
+++ content/hlcache.h 2010-03-28 12:03:49.722111852 +0100
@@ -0,0 +1,110 @@
+/*
+ * Copyright 2009 John-Mark Bell <[email protected]>
2010?
Index: render/imagemap.c
- * \param c The containing content
+ * \param h The containing content
Bletch(ly?)
Index: desktop/browser.c
@@ -308,16 +310,44 @@ void browser_window_go_post(struct brows
return;
}
+ /* Set up retrieval parameters */
+ if (verifiable)
if it is what?
+//newcache extract charset and quirks from parent content
filth
+ /* Get download out of the way */
+ if (download) {
if download is what?
+//newcache do we not just pass ownership to the theme installation stuff?
moar filth
+// newcache
filth
+#if 0
Filth beyond compare
Index: content/content.c
// LOG(("%p %s", c, c->url));
FIXME
+//newcache
+#if 0
KILL MAIM
Please svn rm fetchcache.[ch]
