On Thu, Sep 23, 2010 at 10:09:24PM +0100, John-Mark Bell wrote:
> Precis:
>
> This is part of the treeview-redux patchset.
> It incorporates the changes on that branch today.
I will be reviewing this mostly for glaring hideousness. Any minor niggles
that I raise can be merged and fixed-up on trunk as we clean up any
implementation hiccoughs.
> Index: Docs/09-treeview
As jmb said, this should eventually be transferred to the wiki and removed from
here.
> Index: desktop/sslcert.c
> +#define TREE_ELEMENT_SSL_VERSION 0x01
> +#define TREE_ELEMENT_SSL_VALID_FROM 0x02
> +#define TREE_ELEMENT_SSL_VALID_TO 0x03
> +#define TREE_ELEMENT_SSL_CERT_TYPE 0x04
> +#define TREE_ELEMENT_SSL_SERIAL 0x05
> +#define TREE_ELEMENT_SSL_ISSUER 0x06
It feels like this should be an enum.
> +struct sslcert_session_data {
> + unsigned long num;
> + char *url;
> + struct tree *tree;
> + nserror (*cb)(bool proceed, void *pw);
I'd like to see this typedef'd but it's not critical.
Also, this entire struct could do with documenting as 'num' and 'tree' aren't
terribly obvious names for things.
> +static hlcache_handle *sslcert_icon;
This fills me with the heebie jeebies.
> +bool sslcert_load_tree(struct tree *tree, const struct ssl_cert_info *certs,
> + struct sslcert_session_data *data)
...
> + long i;
...
> + for (i = 0; i < (long)data->num; i++) {
Why not 'unsigned long i' ?
> +struct node *sslcert_create_node(const struct ssl_cert_info *cert)
> +{
> + struct node *node;
> + struct node_element *element;
> + char buffer[356];
> + char *text;
> +
> +
> + snprintf(buffer, 356, messages_get("Subject"), cert->subject);
This 356 is amazingly magical and scary. What is it all about? If it's some
kind of maximum that the certificate COULD hold, then we should be assert()ing
the strlen() before we do anything else. Also, since messages_get("Subject")
might easily exceed that we need something better here I think. I know it's
"safe" but that doesn't make it *good*.
Also, the name 'Subject' is really nasty. Can it be changed to
SSL_Certificate_Subject or similar?
> + text = strdup(buffer);
This idiom is found throughout NetSurf with regard to messages. Perhaps we
should look at a messages_strdup_printf(token, ...) function?
> + snprintf(buffer, 356, messages_get("Issuer"), cert->issuer);
> + text = strdup(buffer);
All the above apply for this.
> + snprintf(buffer, 356, messages_get("Version"), cert->version);
> + text = strdup(buffer);
And this
> + snprintf(buffer, 356, messages_get("ValidFrom"),
> + cert->not_before);
> + text = strdup(buffer);
And this.
> + snprintf(buffer, 356, messages_get("ValidTo"), cert->not_after);
> + text = strdup(buffer);
And this
> + snprintf(buffer, 356, messages_get("Type"), cert->cert_type);
> + text = strdup(buffer);
And this.
> + snprintf(buffer, 356, messages_get("Serial"), cert->serial);
> + text = strdup(buffer);
And this.
> +void sslcert_clanup_session(struct sslcert_session_data *session)
This function needs a spellchecker applying :-)
> Index: desktop/hotlist.c
> +static hlcache_handle *folder_icon;
Once again, I get screaming heebiejeebies at this.
> +
> +static const struct {
> + const char *url;
> + const char *msg_key;
> +} default_entries[] = {
> + { "http://www.netsurf-browser.org/";, "HotlistHomepage" },
> + { "http://www.netsurf-browser.org/downloads/riscos/testbuilds";,
> + "HotlistTestBuild" },
> + { "http://www.netsurf-browser.org/documentation";,
> + "HotlistDocumentation" },
> + { "http://sourceforge.net/tracker/?atid=464312&group_id=51719";,
> + "HotlistBugTracker" },
> + { "http://sourceforge.net/tracker/?atid=464315&group_id=51719";,
> + "HotlistFeatureRequest" }
> +};
> +#define ENTRIES_COUNT (sizeof(default_entries) / sizeof(default_entries[0]))
This also gives me the heebiejeebies although less screaming. Not sure what
I'd want instead, but I don't like it.
> +bool hotlist_initialise(struct tree *tree, const char *hotlist_path)
> + if (hotlist_path != NULL)
> + fp = fopen(hotlist_path, "r");
Either hotlist_path is a path in which case this is okay; or it's a URL in
which case it's not okay.
> + if (fp == NULL) {
Why not move the else{} clause up here, invert the test, and then unindent the
rest of the function which will make it format more pleasantly?
> + /* 1 indicates this is a folder */
This comment makes no sense to me.
> + if (!tree_url_load(hotlist_path, hotlist_tree,
> + hotlist_node_callback, NULL))
Either hotlist_path is a URL in which case this is okay, or it's a path in
which case it's not okay. The function implies that it takes a URL, not a
file-path.
> + if (hotlist_tree)
If hotlist_tree is what?
> +node_callback_resp hotlist_node_callback(void *user_data, struct
> node_msg_data *msg_data)
> +{
> + struct node *node = msg_data->node;
> + const char *text;
> + char *norm_text;
> + bool folder = tree_node_is_folder(node);
This would be better called is_folder
> +
> + switch (msg_data->msg) {
> + case NODE_ELEMENT_EDIT_FINISHED:
> + if (creating_node && !folder &&
Is 'creating_node' a global? If so -- icky!
And '!folder' here would be better as 'is_folder == false'
> +bool hotlist_export(const char *path)
> +{
> + return tree_url_save(hotlist_tree, path, "NetSurf hotlist");
Path? URL?
> +/**
> + * Open the selected entries in seperate browser windows.
In separate? (a. spello, b. there's no indication of separate, c. What about
launching in "current" window?)
> Index: desktop/history_global_core.c
> +static hlcache_handle *folder_icon;
Heebies and jeebies once again screaming.
> +static const char *const weekdays [] =
> +{
> + "Sunday",
> + "Monday",
> + "Tuesday",
> + "Wednesday",
> + "Thursday",
> + "Friday",
> + "Saturday"
> +};
What the frack?! Why are these not Messages? Oh, they are, can we change
'weekdays' to 'weekday_msg_name' then?
> +/**
> + * Initialises the grouping nodes(Today, Yesterday etc.) for the global
> history
> + * tree.
> + *
> + * \return false on memory exhaustion, true otherwise
> + */
> +bool history_global_initialise_nodes(void)
What happens when the day rolls over? It's not uncommon for people to leave
browsers open overnight. Especially if they're suspending/resuming their
laptops. How are we going to cope with that?
> + if (weekday > 0)
> + if (!history_global_initialise_node(
> + messages_get("DateYesterday"), t, -1))
> + return false;
So we don't get a 'yesterday' node on Sundays?
> + for (i = 2; i <= weekday; i++)
> + if (!history_global_initialise_node(NULL, t, -i))
> + return false;
What the frack?
This whole 'week day' thing is very very confusing. POSIX defines days as
being 24*60*60 seconds long. Why not take advantage of that to correctly
initialise the tree?
> +bool history_global_initialise_node(const char *title,
> + time_t base, int days_back)
> +{
> + struct tm *full_time;
> + char *buffer;
> + struct node *node;
> +
> + base += days_back * 60 * 60 * 24;
Oh look! We even assume the length of a day correctly here anyway!
> +/**
> + * Deletes the global history tree.
> + */
> +void history_global_cleanup(void)
> +{
> +}
Go Go Gadget Cleanup!
> +void global_history_add(const char *url)
> +{
> + const struct url_data *data;
> +
> + data = urldb_get_url_data(url);
> + if (!data)
If data is not what?
> +bool global_history_add_internal(const char *url, const struct url_data
> *data)
> + assert(url && data);
Assert that they are what?
Why oh why do people assume NULL == 0 ? (Oh, I know it *is*, but that doesn't
mean it's defined to be so)
One day, I'm going to design a "Coding style enforcement" header which does
things like defining true and false to both be non-zero and defines NULL to be
0xFFFFFFFF and stuff like that, to force comparisons.
> + if (!parent)
If parent is not what?
> + if (!global_history_init) {
If global_history_init is not what? If that was meant to be a boolean, a name
like global_history_initialised would be superior.
> + if (node) {
If node is what?
> +/**
> + * Save the global history in a human-readable form under the given location.
> + *
> + * \param path the path where the history will be saved
> + */
> +bool history_global_export(const char *path)
> +{
> + return tree_url_save(global_history_tree, path, "NetSurf history");
url, path, bwuah?
> +void history_global_delete_all(void)
> +{
> + bool redraw = tree_get_redraw(global_history_tree);
Would that be better as redraw_needed?
> Index: desktop/cookies.c
> +#define TREE_ELEMENT_PERSISTENT 0x01
> +#define TREE_ELEMENT_VERSION 0x02
> +#define TREE_ELEMENT_SECURE 0x03
> +#define TREE_ELEMENT_LAST_USED 0x04
> +#define TREE_ELEMENT_EXPIRES 0x05
> +#define TREE_ELEMENT_PATH 0x06
> +#define TREE_ELEMENT_DOMAIN 0x07
> +#define TREE_ELEMENT_COMMENT 0x08
> +#define TREE_ELEMENT_VALUE 0x09
Again, this really feels like it should be an enum.
> +static hlcache_handle *folder_icon;
> +static hlcache_handle *cookie_icon;
Oh look, jeebies of the screaming heebie variety rear their heads again.
> +bool cookies_initialise(struct tree *tree)
> +
> + folder_icon = tree_load_icon(tree_directory_icon_name);
> + cookie_icon = tree_load_icon(tree_content_icon_name);
> +
> + if (tree == NULL)
> + return false;
Why is this test *after* the resource-leaking setup? If it's dumb to
initialise cookies without a tree to attach to, then why don't we assert(tree
!= NULL) instead?
> +/**
> + * Perform cookie updates and addition. The update is only scheduled here.
> + * The actual update is performed in the callback function.
Given this, why not call it "schedule_cookies_update" ?
> + *
> + * \param data Data of cookie being updated.
> + * \return true (for urldb_iterate_entries)
> + */
> +bool cookies_update(const struct cookie_data *data)
> +{
> +
> + assert(data);
Assert data is what?
> + assert(!user_delete);
Assert user_delete is what? Is it global? Ick! Is it a boolean? If so a
better name would be nice.
> +
> + schedule_remove(cookies_schedule_callback, (void *)data);
> + schedule(100, cookies_schedule_callback, (void *)data);
Why remove the schedule, I was under the impression that schedule() implicitly
removed any pending schedule events which clashed.
> +/**
> + * Deletes a cookie.
> + *
> + * \param data Data of cookie being updated.
> + * \return true (for urldb_iterate_entries)
> + */
> +void cookies_delete(const struct cookie_data *data)
> +{
> +
> + assert(data);
> +
> + schedule_remove(cookies_schedule_callback, (void *)data);
> +}
This hardly seems to delete a cookie. All it does is stop is being updated.
Also the docstring intimates a boolean result, but the function is void.
> +/**
> + * Funtion called when scheduled event gets fired. Performs actual cookie
Function.
> + * update.
> + */
> +void cookies_schedule_callback(void *scheduled_data)
> +{
> + const struct cookie_data *data = scheduled_data;
> + struct node *node = NULL;
> + struct node *cookie_node = NULL;
> + char *domain_cp;
Probably worth assert(data != NULL);
> +/**
> + * Free memory and release all other resources.
> + */
> +void cookies_cleanup(void)
> +{
> +}
Go Go Gadget Cleanup!
> + if (!strcmp(title, tree_node_element_get_text(element)))
using ! and strcmp always upsets me. Please use == 0
> + if ((data->comment) && (strcmp(data->comment, "")))
if data->comment is what?
> +bool cookies_update_cookie_node(struct node *node,
> + const struct cookie_data *data)
> +{
> + struct node_element *element;
> + char buffer[256];
> + char buffer2[16];
> + char *text;
> + const char *old_text;
> +
> +
> + element = tree_node_find_element(node, TREE_ELEMENT_VALUE, NULL);
> + if (element != NULL) {
> + snprintf(buffer, 256, messages_get("TreeValue"),
> + data->value ?
> + data->value : messages_get("TreeUnused"));
Why 256? Also, 'TreeValue' seems odd. Shouldn't it be 'CookieTreeValueTitle'
or similar? Same goes for all the below.
> + if ((data->comment) && (strcmp(data->comment, ""))) {
if data->comment is what?
> + snprintf(buffer, 256, messages_get("TreeComment"),
> + data->comment);
256 again!
> + snprintf(buffer, 256, messages_get("TreeDomain"), data->domain,
> + data->domain_from_set ?
> + messages_get("TreeHeaders") : "");
And again
> + snprintf(buffer, 256, messages_get("TreePath"), data->path,
> + data->path_from_set ?
> + messages_get("TreeHeaders") : "");
And again!
> + snprintf(buffer, 256, messages_get("TreeExpires"),
> + (data->expires > 0)
> + ? (data->expires == 1)
> + ? messages_get("TreeSession")
> + : ctime(&data->expires)
> + : messages_get("TreeUnknown"));
And again. And this one has two nested ?: for added OMG, MY EYES!
> + snprintf(buffer, 256, messages_get("TreeLastUsed"),
> + (data->last_used > 0) ?
> + ctime(&data->last_used) :
> + messages_get("TreeUnknown"));
Another 256.
> + snprintf(buffer, 256, messages_get("TreeSecure"),
> + data->secure ?
> + messages_get("Yes") : messages_get("No"));
This is getting dull. A #define for the buffer length or else sizeof(buffer)
would be good.
> + snprintf(buffer2, 16, "TreeVersion%i", data->version);
Your faith that data->version will never be > 3 digits long impresses me.
> + snprintf(buffer, 256, messages_get("TreeVersion"),
> + messages_get(buffer2));
Also, your faith that there *WILL* be a message for that type of cookie is nice.
But could you please use more descriptive message names?
> +node_callback_resp cookies_node_callback(void *user_data, struct
> node_msg_data *msg_data)
> +{
> + struct node *node = msg_data->node;
> + struct node_element *domain, *path;
> + const char *domain_t, *path_t, *name_t;
> + char *space;
> + bool folder = tree_node_is_folder(node);
is_folder would be better.
> + if (!user_delete)
if user_delete is what? Global? OMG my eyes!
> + if (space)
If space is what?
> + if (space)
Ditto!
> +void cookies_delete_all(void)
> +{
> + bool redraw = tree_get_redraw(cookies_tree);
needs_redraw?
> Index: desktop/tree_url_node.c
> +#define TREE_ELEMENT_URL 0x01
> +#define TREE_ELEMENT_LAST_VISIT 0x02
> +#define TREE_ELEMENT_VISITS 0x03
> +#define TREE_ELEMENT_THUMBNAIL 0x04
enum?
> +#define MAX_ICON_NAME_LEN 256
Why 256?
> +static hlcache_handle *folder_icon;
There's a screaming in my ears! It must also be left over from the static
array containing hlcache handles from earlier too.
> +struct node *tree_create_URL_node(struct tree *tree, struct node *parent,
> + const char *url, const char *title,
> + tree_node_user_callback user_callback, void *callback_data)
> +{
> + if (element) {
If element is what?
> +struct node *tree_create_URL_node_shared(struct tree *tree, struct node
> *parent,
> + const char *url, const struct url_data *data,
> + tree_node_user_callback user_callback, void *callback_data)
> + {
> + if (data->title)
if data->title is what?
> + if (!node)
If node is not what?
> + if (element)
if element is what?
> +void tree_update_URL_node(struct tree *tree, struct node *node,
> + const char *url, const struct url_data *data, bool shared)
> +{
> + struct node_element *element;
> + char buffer[256];
256?
> + assert(node);
assert node is what?
> + snprintf(buffer, 256, messages_get("TreeLast"),
> + (data->last_visit > 0) ?
> + ctime((time_t *)&data->last_visit) :
> + messages_get("TreeUnknown"));
Hideous combination of a fixed length buffer and two possibly badly named
messages, along with a ternary operator. Truly my eyes runneth over with
blood.
> + snprintf(buffer, 256, messages_get("TreeVisits"),
> + data->visits);
Ditto.
> +node_callback_resp tree_url_node_callback(void *user_data,
> + struct node_msg_data *msg_data)
> +{
> + struct tree *tree;
> + struct node_element *element;
> + url_func_result res;
> + const char *text;
> + char *norm_text, *escaped_text;
> + const struct url_data *data;
> +
> + /* TODO: memory leaks on non-shared folder deletion */
/** @todo
Please.
(Or better -- fix the leak)
> +bool tree_url_load(const char *filename, struct tree *tree,
> + tree_node_user_callback callback, void *callback_data)
> +{
> + xmlDoc *doc;
> + xmlNode *html, *body, *ul;
> + struct node *root;
> +
> + doc = htmlParseFile(filename, "iso-8859-1");
Erm, eww!?!?! iso-8859-1 ?
Why the hell are we not storing all of our data in utf-8 ?
Also url_load and filename? Seems confusing to me.
> +void tree_url_load_directory(xmlNode *ul, struct tree *tree,
> + struct node *directory, tree_node_user_callback callback,
> + void *callback_data)
> +{
> + char *title;
> + struct node *dir;
> + xmlNode *n;
> +
> + assert(ul);
Assert ul is what?
> + assert(directory);
Assert directory is what?
> + if (!n || strcmp((const char *) n->name, "ul") != 0) {
!n?
> +void tree_url_load_entry(xmlNode *li, struct tree *tree,
> + struct node *directory, tree_node_user_callback callback,
> + void *callback_data)
> + for (n = li->children; n; n = n->next) {
n?
> + if (!url1 || !title) {
!url1? !title?
> +xmlNode *tree_url_find_xml_element(xmlNode *node, const char *name)
> +{
> + xmlNode *n;
> + if (!node)
> + return 0;
Erm, either assert(node != NULL) or compare to NULL and return NULL if not
available. This combination of treating node as a boolean and returning an
integer instead of a pointer truly upsets me.
> + for (n = node->children;
> + n && !(n->type == XML_ELEMENT_NODE &&
n?
> +bool tree_url_save(struct tree *tree, const char *filename,
> + const char *page_title)
> +{
> + /* Unfortunately the Browse Hotlist format is invalid HTML,
> + * so this is a lie. */
Aah so perhaps we're iso-8859-1 to be browse compatible. Is it time to drop
that crap and store something more efficient, correctly structured and utf-8 ?
> + doc->charset = XML_CHAR_ENCODING_UTF8;
> + res = htmlSaveFileEnc(filename, doc, "iso-8859-1");
Urgh, ick, OMG, pain in my eyes as blood once again spewws forth.
> +bool tree_url_save_directory(struct node *directory, xmlNode *node)
> +{
> + if (!ul)
ul is not what?
> + for (child = tree_node_get_child(directory); child;
child?
> + if (!h4)
h4 is not what?
> +bool tree_url_save_entry(struct node *entry, xmlNode *node)
> +{
> + if (!li)
li is not what?
> + if (!a)
a is not what?
> + if (!href)
href is not what?
> Index: !NetSurf/Resources/de/Messages
I'm going to skip all the messages files because I can't usefully review them
at this time.
> Index: Makefile.sources
Do we need an S_TREEVIEW ?
> Index: desktop/tree.c
> +struct node_element_box {
All these structs have comments, but aren't doccommented.
> +struct node *tree_create_folder_node(struct tree *tree, struct node *parent,
> + const char *title, bool editable, bool retain_in_memory,
> + bool deleted)
> +{
> struct node *node;
>
> + assert(title);
assert that title is what?
> +struct node *tree_create_leaf_node(struct tree *tree, struct node *parent,
> + const char *title, bool editable, bool retain_in_memory,
> + bool deleted)
> +{
> + struct node *node;
>
> + assert(title);
Assert title is what?
> +void tree_link_node(struct tree *tree, struct node *link, struct node *node,
> + bool before)
> +{
>
> + struct node *parent;
> + bool sort = false;
> +
> + assert(link);
> assert(node);
Assert that these are what?
> + if ((!link->folder) || (before)) {
link->folder is not what?
> /**
> + * Gets the redraw property of the given tree.
But what *is* the redraw property? It makes no sense.
> /**
> + * Sets an icon for a node
> *
> + * \param tree The tree to which node belongs, may be NULL
> + * \param node The node for which the icon is set
> + * \param icon the image to use
> + */
An indication of who owns the handle, is it cloned, blahblahblah needed here.
> +void tree_set_node_sort_function(struct tree *tree, struct node *node,
> + int (*sort) (struct node *, struct node *))
> +{
> + while (child) {
While child is what?
> /**
> + * Inserts a node into the correct place according to the parents sort
> function
parent's
> +void tree_sort_insert(struct node *parent, struct node *node)
> + while (after && parent->sort(node, after) == -1)
while after is what?
> + if (parent->child)
if parent->child is what?
> +void tree_update_node_element(struct tree *tree, struct node_element
> *element,
> + const char *text, void *bitmap)
> +{
> + assert(element);
assert element is what?
> +/**
> + * Returns true if the tree is currently being edited
Returns whether the current tree is being edited at this time.
> + return tree->editing == NULL ? false:true;
If you're going to space the ? then also space the :
> +void tree_draw_node(struct tree *tree, struct node *node,
> + int tree_x, int tree_y,
> + int clip_x, int clip_y, int clip_width, int clip_height) {
My only concern with this function is that it has sufficient nested loops that
the code ends up smoosed up against the RH margin.
> +bool tree_mouse_action(struct tree *tree, browser_mouse_state mouse, int x,
> + int y)
This function is huge and could do with being refactored at some point.
> + /* TODO: the tree window has to scroll the tree when
> + mouse reaches border while dragging this isn't
> + solved for the browser window too.
> + */
/** @todo please
> +void tree_handle_node_changed(struct tree *tree, struct node *node,
> + bool recalculate_sizes, bool expansion)
> +{
> + int width, height, tree_height;
> +
> assert(node);
assert node is what?
> +void tree_recalculate_node_sizes(struct tree *tree, struct node *node,
> + bool recalculate_sizes)
> +{
> + assert(node);
assert node is what?
> +hlcache_handle *tree_load_icon(const char *name)
> +{
> + char *url = NULL;
> + const char *icon_url = NULL;
> + int len;
> + hlcache_handle *c;
> +
> + /* TODO: something like bitmap_from_disc is needed here */
/** @todo
> +
> + if (!strncmp(name, "file://", 7)) {
> + icon_url = name;
Erm? I'd prefer it if this took a "bool prepend_path" or similar, rather than
this hideousness. Not least of which, in the future, we may want to retrieve
icons etc from a resources: or theme: scheme.
> + /* Fetch the icon */
> + hlcache_handle_retrieve(icon_url, 0, 0, 0,
> + tree_icon_callback, 0, 0, 0, &c);
error check?
> Index: desktop/tree.h
> +/* Tree flags */
> +#define TREE_NO_FLAGS 0
> +#define TREE_NO_DRAGS 1
> +#define TREE_NO_FURNITURE 2
> +#define TREE_SINGLE_SELECT 4
> +#define TREE_NO_SELECT 8
> +#define TREE_MOVABLE 16
> +/* if the last child of a directory is deleted the directory will be deleted
> + too */
> +#define TREE_DELETE_EMPTY_DIRS 16
It'd be nice if these flags were an enum too.
> +/* the flag for the first node_element in every node, all other flags should
> + be different than this one */
> +#define TREE_ELEMENT_TITLE 0x00
How can zero be a flag?!
> +/* these should be defined in front end code */
> +extern const char tree_directory_icon_name[];
> +extern const char tree_content_icon_name[];
Let's have a function for the frontend:
> typedef enum {
> + NODE_ELEMENT_TEXT, /* Text only */
> + NODE_ELEMENT_TEXT_PLUS_ICON, /* Text and icon */
> + NODE_ELEMENT_BITMAP /* Bitmap only */
> } node_element_type;
Not doccommented?
> +typedef enum {
> + NODE_DELETE_ELEMENT_TXT, /* The text of an element of the node
> + is being deleted */
> + NODE_DELETE_ELEMENT_IMG, /* The bitmap or icon of a node is being
> + deleted */
> + NODE_LAUNCH, /* The node has been launched */
> + NODE_ELEMENT_EDIT_FINISHING, /* New text has to be accepted or
> + rejected */
> + NODE_ELEMENT_EDIT_FINISHED /* Editing of a node_element has been
> + finished */
> +} node_msg;
Ditto
> +typedef enum {
> + NODE_CALLBACK_HANDLED,
> + NODE_CALLBACK_NOT_HANDLED,
> + NODE_CALLBACK_REJECT, /* reject new text for node element and
> + leave editing mode */
> + NODE_CALLBACK_CONTINUE /* don't leave editig mode */
> +} node_callback_resp;
Ditto
> +struct node_msg_data {
> + node_msg msg;
> + unsigned int flag;
> + struct node *node;
> + union {
> + char *text;
> + void *bitmap;
> + } data;
> };
Ditto.
> +struct treeview_table {
> + void (*redraw_request)(int x, int y, int width, int height,
> + void *data);
> + void (*resized)(struct tree *tree, int width, int height,
> + void *data);
> + void (*scroll_visible)(int y, int height, void *data);
> + void (*get_window_dimensions)(int *width, int *height, void *data);
> };
Ditto
> Index: desktop/options.c
> + { "tree_icons_dir", OPTION_STRING, &option_tree_icons_dir },
Why is this an option?! Surely it's the job of the frontend to decide where
these resources are?
> Index: content/urldb.c
> +void urldb_iterate_cookies(bool (*callback)(const struct cookie_data *data))
Typedef for that callback type *please*
> + bool (*cookie_callback)(const struct cookie_data *data))
Although I see it's not the style in this file :-(
> // LOG(("%p: %s=%s", c, c->name, c->value));
No C++ style comments please. This is core code.
D.
--
Daniel Silverstone http://www.netsurf-browser.org/
PGP mail accepted and encouraged. Key Id: 3CCE BABE 206C 3B69
