ok thats the response for the first few bits...on to the next bit now but I have not addressed some stuff which probably could do with an answer.
On Fri, Sep 24, 2010 at 04:27:14PM +0100, Daniel Silverstone wrote: > 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. http://wiki.netsurf-browser.org/Documentation/Treeview file removed > > > 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. enum tree_element_ssl{ TREE_ELEMENT_SSL_VERSION = 0x01, TREE_ELEMENT_SSL_VALID_FROM = 0x02, TREE_ELEMENT_SSL_VALID_TO = 0x03, TREE_ELEMENT_SSL_CERT_TYPE = 0x04, TREE_ELEMENT_SSL_SERIAL = 0x05, TREE_ELEMENT_SSL_ISSUER = 0x06, }; *however* it is passed as an opaque unsigned int "flag" to tree.c:tree_create_node_element() and I cannot see a simple way to structure safe data type all the way through from the 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. > done in sslcert.h as its part of the exported interface > Also, this entire struct could do with documenting as 'num' and 'tree' aren't > terribly obvious names for things. > I have not changed teh names, but I have docuemnted them. /** ssl certificate verification context. */ struct sslcert_session_data { unsigned long num; /**< The number of ssl certificates in the chain */ char *url; /**< The url of the certificate */ struct tree *tree; /**< The root of the treeview */ sslcert_session_callback cb; /**< callback when cert is accepted or rejected */ void *cbpw; /**< context passed to callback */ }; > > +static hlcache_handle *sslcert_icon; > > This fills me with the heebie jeebies. Yes, a higher level decision needs to be made. Can we return to this later. I have added a doccomment > > > +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' ? > changed, also neatened the loop variable name unsigned long cert_loop; > > > +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*. > ok I changed all these to use a messages_get_buff() and completely removed teh buffer and its scary sizeing. > Also, the name 'Subject' is really nasty. Can it be changed to > SSL_Certificate_Subject or similar? by your command imperious leader > > > + text = strdup(buffer); > > This idiom is found throughout NetSurf with regard to messages. Perhaps we > should look at a messages_strdup_printf(token, ...) function? > I called it messages_get_buff() though > > + snprintf(buffer, 356, messages_get("Issuer"), cert->issuer); > > + text = strdup(buffer); > > All the above apply for this. > done > > + snprintf(buffer, 356, messages_get("Version"), cert->version); > > + text = strdup(buffer); > > And this done > > > + snprintf(buffer, 356, messages_get("ValidFrom"), > > + cert->not_before); > > + text = strdup(buffer); > > And this. done > > > + 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. done > > > + snprintf(buffer, 356, messages_get("Serial"), cert->serial); > > + text = strdup(buffer); > > And this. done > > > +void sslcert_clanup_session(struct sslcert_session_data *session) > > This function needs a spellchecker applying :-) renamed > > > Index: desktop/hotlist.c > > +static hlcache_handle *folder_icon; > > Once again, I get screaming heebiejeebies at this. Once again, i agree, but am at a bit of a loss how to improve matters > > > + > > +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. > I was considering simply making it a resource file and opening that if we cannot get hold of the users one? for now I simply gave it a better name > > +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. > restructured > > + 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? > I savaged the flow and moved the file check to tree_url_node as this was the only callsite and it makes it infinitely clearer > > + /* 1 indicates this is a folder */ > > This comment makes no sense to me. > me either, removed it > > + 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. > tree_urlfile_load() is what i came up with > > + if (hotlist_tree) > > If hotlist_tree is what? not NULL is what i decided ;-) > > > +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 is now > > > + > > + switch (msg_data->msg) { > > + case NODE_ELEMENT_EDIT_FINISHED: > > + if (creating_node && !folder && > > Is 'creating_node' a global? If so -- icky! > it is global and some form of interlock > And '!folder' here would be better as 'is_folder == false' > changed > > +bool hotlist_export(const char *path) > > +{ > > + return tree_url_save(hotlist_tree, path, "NetSurf hotlist"); > > Path? URL? > tree_urlfile_save() is what i replaced it with > > +/** > > + * 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?) > I have no idea...I fixed the spello, help? > > > Index: desktop/history_global_core.c > > +static hlcache_handle *folder_icon; > > Heebies and jeebies once again screaming. it is the icon reference again > > > +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? it shall be so > > > +/** > > + * 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? You make several excellent points in this function and I do not understand whats going on enough to come up with a comprehensive answer, anyone else got ideas?!? > > > + 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? if (data == NULL) return; > > > +bool global_history_add_internal(const char *url, const struct url_data > > *data) > > + assert(url && data); > > Assert that they are what? assert((url != NULL) && (data != NULL)); > > 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. ahh round tuit supplies are low as usual > > > + if (!parent) > > If parent is not what? > if (parent == NULL) return true; > > + 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. changed > > > + if (node) { > > If node is what? > if (node != NULL) { > > +/** > > + * 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? > changed > > +void history_global_delete_all(void) > > +{ > > + bool redraw = tree_get_redraw(global_history_tree); > > Would that be better as redraw_needed? > yes, changed > > 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. > /** Flags for each type of cookie tree node. */ enum tree_element_cookie { TREE_ELEMENT_PERSISTENT = 0x01, TREE_ELEMENT_VERSION = 0x02, TREE_ELEMENT_SECURE = 0x03, TREE_ELEMENT_LAST_USED = 0x04, TREE_ELEMENT_EXPIRES = 0x05, TREE_ELEMENT_PATH = 0x06, TREE_ELEMENT_DOMAIN = 0x07, TREE_ELEMENT_COMMENT = 0x08, TREE_ELEMENT_VALUE = 0x09, } > > +static hlcache_handle *folder_icon; > > +static hlcache_handle *cookie_icon; > > Oh look, jeebies of the screaming heebie variety rear their heads again. again just used as a way of retriving resources for icons > > > +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? > I moved this up to the start at least. > > +/** > > + * 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" ? > changed to cookies_schedule_update() to fit with the namespace > > + * > > + * \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(data == NULL); > > > + assert(!user_delete); > > Assert user_delete is what? Is it global? Ick! Is it a boolean? If so a > better name would be nice. > assert(user_delete == false); it is some form of locking flag, can you suggest a better name? > > + > > + 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. > gtk, risc os etc. do indeed remove any events matching first so, removed > > +/** > > + * 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. > indeed seems its only ever called from urldb while its deleting cookies and this acessor is better named "remove" as it actaully just removes the cookie from the active set, the actual deletion is done in urldb.c. I cannot tell if the entry requires its node to be deleted. I also did the assert while I was here in cookies.h: /** * Remove a cookie from the active set. * The cookie is to be removed from the active set and no futher * references made to the cookie data. * * \param data Data of cookie being removed. */ void cookies_remove(const struct cookie_data *data); in cookies.c: /* exported interface documented in cookies.h */ void cookies_remove(const struct cookie_data *data) { assert(data == NULL); schedule_remove(cookies_schedule_callback, (void *)data); } > > +/** > > + * Funtion called when scheduled event gets fired. Performs actual cookie > > Function. > /** * Called when scheduled event gets fired. Actually performs the update. */ > > + * 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); > done > > +/** > > + * 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 > done > > + if ((data->comment) && (strcmp(data->comment, ""))) > > if data->comment is what? > if ((data->comment != NULL) && (strcmp(data->comment, "") != 0)) { however would a test against (data->comment[0] != 0) not be cheaper than a strcmp call? > > +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. switched to use a helper function calling messages_get_buff() and restructured it so its no longer evil /** * Helper to update the text of a node if it has changed. * * \param element The node element to update. * \param text The text to update the element with. The ownership of * this string is taken by this function and must not be * referred to after the function exits. */ static bool update_element_text(struct node_element *element, char *text) { const char *node_text; /* existing node text */ if (element == NULL) return false; if (text == NULL) return false; node_text = tree_node_element_get_text(element); if ((node_text == NULL) || (strcmp(node_text, text) != 0)) { tree_update_node_element(cookies_tree, element, text, NULL); } else { /* text does not need changing, free it */ free(text); } return true; } /* update the value text */ element = tree_node_find_element(node, TREE_ELEMENT_VALUE, NULL); update_element_text(element, messages_get_buff("TreeValue", data->value != NULL ? data->value : messages_get("TreeUnused"))); and it seems there are a set of "default" text for trees in general which the cookie stuff uses, need a second look to see if these "generics" really are > > > + if ((data->comment) && (strcmp(data->comment, ""))) { > > if data->comment is what? > > > + snprintf(buffer, 256, messages_get("TreeComment"), > > + data->comment); > > 256 again! update_element_text() now used > > > + snprintf(buffer, 256, messages_get("TreeDomain"), data->domain, > > + data->domain_from_set ? > > + messages_get("TreeHeaders") : ""); > > And again update_element_text() now used > > > + snprintf(buffer, 256, messages_get("TreePath"), data->path, > > + data->path_from_set ? > > + messages_get("TreeHeaders") : ""); > > And again! update_element_text() now used > > > + 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! update_element_text() now used but yeah the ?: evil remains > > > + snprintf(buffer, 256, messages_get("TreeLastUsed"), > > + (data->last_used > 0) ? > > + ctime(&data->last_used) : > > + messages_get("TreeUnknown")); > > Another 256. > update_element_text() now used > > + 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. update_element_text() now used, I think better solution overall > > > + snprintf(buffer2, 16, "TreeVersion%i", data->version); > > Your faith that data->version will never be > 3 digits long impresses me. > made the selection buffer 32bytes the version field is an int it cannot expand larger than 9 digits so allowing 16 should always be ok > > + 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. > well the message system will always return something, even if its just the token you gave it so worst you get is TreeVersion1 or such anyway now uses update_element_text() etc. and is not evil. > But could you please use more descriptive message names? again I need a second pair of eyes on these "generic" 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. > done > > + if (!user_delete) > > if user_delete is what? Global? OMG my eyes! > yah, still that global lock from earlier > > + if (space) > > If space is what? if (space != NULL) > > + if (space) > > Ditto! if (space != NULL) > > > +void cookies_delete_all(void) > > +{ > > + bool redraw = tree_get_redraw(cookies_tree); > > needs_redraw? changed > > > 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? enumed > > +#define MAX_ICON_NAME_LEN 256 > > Why 256? > its used in tree_url_node_init() in the mapping of content type names to icon names basically its larger than any of the icon urls, i think its ok...maybe? > > +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. > yah, still do not have an answer for you, its "just" another icon handle > > +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? > if (element != NULL) { > > +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 (data->title != NULL) { title = data->title; } else { title = url; } > > + if (!node) > > If node is not what? if (node == NULL) return NULL; > > > + if (element) > > if element is what? if (element != NULL) tree_update_node_element(tree, element, url, NULL); > > > +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? I have removed it imperious leader > > > + assert(node); > > assert node is what? > possibly porcine? Though i went with assert(node != NULL); > > + 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. > changed to using teh (slightly) less eye bleachy: /* update last visit text */ element = tree_node_find_element(node, TREE_ELEMENT_LAST_VISIT, element); update_element_text(tree, element, messages_get_buff("TreeLast", (data->last_visit > 0) ? ctime((time_t *)&data->last_visit) : messages_get("TreeUnknown"))); > > + snprintf(buffer, 256, messages_get("TreeVisits"), > > + data->visits); > > Ditto. also changed > > > +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) /** @todo memory leaks on non-shared folder deletion */ its not obvious how...so err you get the text fixed ;-) -- Regards Vincent http://www.kyllikki.org/
signature.asc
Description: Digital signature
