Review: Mark Benjamin -- gtkmain branch

John-Mark Bell jmb at netsurf-browser.org
Fri Jul 31 03:29:35 BST 2009


On Fri, 2009-07-10 at 13:11 +0100, John-Mark Bell wrote:

> Index: render/favicon.c
> ===================================================================
> --- /dev/null	2009-04-16 19:17:07.000000000 +0100
> +++ render/favicon.c	2009-07-10 12:49:13.000000000 +0100
> +unsigned long favicon_hash(char *str)
> +{
> +	if (str == NULL)
> +		return 0;
> +	unsigned long hash = 5381;
> +	int c;
> +	while ((c = (unsigned char) *str++))
> +		hash = ((hash << 5) + hash) + c; /* hash * 33 + c */
> +	return hash;
> +}

Do we really need yet another hash function?

> +/**
> + * retrieve 1 url reference to 1 favicon
> + * \param html xml node of html element
> + * \return pointer to url; NULL for no icon
> + */

Is the result allocated? Does the caller have to free it?

> +char *favicon_get_icon_ref(struct content *c, xmlNode *html)
> +{
> +	xmlNode *node;
> +	char *rel, *type, *href, *url, *suf, *url2;
> +	url2 = NULL;
> +	url_func_result res;
> +	int score, hiscore;
> +	hiscore = 0;
> +	/* hashed values - saves calculating them afresh every time */
> +	#define HHICON 0x7c98572e
> +	/* icon */
> +	#define HHSHORTCUTICON 0xfcbccdca
> +	/* shortcut icon */
> +	#define HHAPPLETOUCHICON 0x024c6ddd
> +	/* apple-touch-icon */
> +	#define HHIMAGEPNG 0x7382417c
> +	/* image/png */
> +	#define HHIMAGEGIF 0x73821a8d
> +	/* image/gif */
> +	#define HHIMAGEVNDMICROSOFTICON 0xdae02bba
> +	/* image.vnd.microsoft.icon */
> +	#define HHIMAGEJPEG 0xe3c72f5d
> +	/* image/jpeg */
> +	#define HHIMAGEJPG 0x73822838
> +	/* image/jpg */
> +	#define HHIMAGEICO 0x73822252
> +	/* image/ico */
> +	#define HHIMAGEICON 0xe3c66d00
> +	/* image/icon */
> +	#define HHIMAGEXICON 0x0e3e78e5
> +	/* image/x-icon */
> +	#define HHTEXTICO 0x17e966a2
> +	/* text/icon */
> +	#define HHAPPLICATIONICO 0x087b6fb4
> +	/*application/icon*/
> +	#define HHSUFICO 0x0b887ec0
> +	/* ico */
> +	#define HHSUFPNG 0x0b889dea
> +	/* png */
> +	#define HHSUFGIF 0x0b8876fb
> +	/* gif */
> +	#define HHSUFJPG 0x0b888486
> +	/* jpg */
> +	#define HHSUFJPEG 0x7c99198b
> +	/* jpeg */

This is unpleasant. What's wrong with a simple LUT?

Actually, having read the rest of this function, I think it's hugely
over-complicated and somewhat dubious in its examination of file
extensions.

I suggest replacing it with something far simpler that simply considers
the value of @rel. I see no point whatsoever in considering either the
value of @type or any filename extension -- these are more likely than
not to be wrong and are thus worthless as an indicator of suitability.

> +	if (url2 == NULL) {
> +		struct url_components comp;
> +		if (url_get_components(c->data.html.base_url, &comp) != 
> +				URL_FUNC_OK)
> +			return NULL;
> +		if (url_normalize(comp.authority, 
> +				&url) != URL_FUNC_OK)
> +			return NULL;
> +		url_destroy_components(&comp);
> +		if (url_join("/favicon.ico", url, &url2) != URL_FUNC_OK)
> +			return NULL;

This is overcomplex and broken, too. You simply want:

url_join("/favicon.ico", c->data.html.base_url, &result);


> +/**
> + * retrieve 1 favicon
> + * \param c content structure
> + * \param html xml node of html element
> + * \return true for success, false for error
> + */
> +
> +bool favicon_get_icon(struct content *c, xmlNode *html)
> +{
> +	union content_msg_data msg_data;
> +	char *url = favicon_get_icon_ref(c, html);
> +	struct content *favcontent = NULL;
> +	if (url != NULL)		
> +		favcontent = fetchcache(url, favicon_callback,
> +				(intptr_t) c, 0, c->width, c->height, true, 0,
> +				0, false, false);
> +	free(url);
> +	if (favcontent == NULL)
> +		return false;

The above would be clearer as:

if (url == NULL)
	return false;

favcontent = fetchcache();

free(url);

if (favcontent == NULL)
	return false;

> +
> +	c->data.html.favicon = favcontent;
> +	
> +	fetchcache_go(favcontent, c->url, favicon_callback,
> +			(intptr_t) c, 0, c->width, c->height,
> +			0, 0, false, c->url);
> +
> +	if (!c->data.html.favicon) {
> +		msg_data.error = "Favicon failed to load";

Should there be a call to content_broadcast here?

> Index: render/favicon.h
> ===================================================================
> --- /dev/null	2009-04-16 19:17:07.000000000 +0100
> +++ render/favicon.h	2009-07-10 12:49:13.000000000 +0100
> +typedef enum {
> +	HHICON = 0x7c98572e,
[...]
> +} favicon_string_hash;

Why is this here? What needs to know about it?

> +
> +bool favicon_get_icon(struct content *c, xmlNode *html);

You need to #include <libxml/tree.h> and forward declare struct content
in this file.

> Index: framebuffer/fb_search.c
> ===================================================================
> --- /dev/null	2009-04-16 19:17:07.000000000 +0100
> +++ framebuffer/fb_search.c	2009-07-10 12:49:34.000000000 +0100
> +/* put new search_web globals here for now */
> +char *search_engines_file_location;
> +char *search_default_ico_location;

These should be core options, just like the cookie file/jar path.

> +/**
> + * display hourglass while searching
> + * \param active start/stop indicator
> + */
> +void gui_search_set_hourglass(bool active)
> +{
> +}

I'm somewhat confused as to the need for this. Nothing else in the core
needs a way of saying "I'm busy", so why is search special?

> +/**
> + * retrieve string being searched for from gui
> + */
> +char *gui_search_get_string(void)

Who owns the result. Does the caller have to free it?
What happens if there isn't a search string?

> +{
> +}

This is missing a return value.

> +/**
> + * add search string to recent searches list
> + * \param string search pattern
> + */
> +void gui_search_add_recent(const char *string)

Presumably, this gets copied? If so, what happens on memory exhaustion?

> +/**
> + * activate search forwards button in gui
> + * \param active activate/inactivate
> + */
> +void gui_search_set_forward_state(bool inactive)

The documentation doesn't match the API. Please make it "bool active",
as it's less prone to misinterpretation.

> +/**
> + * activate search forwards button in gui
> + * \param active activate/inactivate
> + */
> +void gui_search_set_back_state(bool inactive)

Ditto.

> +/**
> + * retrieve state of 'case sensitive' check in gui
> + */
> +bool gui_search_get_case_sens(void)

_case_sensitive, please.

> +{
> +}

Return statement missing.

> +/**
> + * retrieve state of 'show all' check in gui
> + */
> +bool gui_search_get_show_all(void)
> +{
> +}

Ditto.

> Index: gtk/gtk_save.c
> ===================================================================
> --- /dev/null	2009-04-16 19:17:07.000000000 +0100
> +++ gtk/gtk_save.c	2009-07-10 12:49:36.000000000 +0100
> +bool save_complete_gui_save(const char *path, const char *filename, struct content *c, int len, char *sourcedata, int type)

Needs wrapping to 80 columns. Also, needs doxygen comments.

> +{
> +	int res;
> +	int namelen;
> +	namelen = strlen(path) + strlen(filename) + 2;

These can be merged into a single statement. What is the "+ 2" for?

> +	char *fullpath = malloc(namelen);
> +	if (!fullpath) {

if (fullpath == NULL) {

> +		warn_user("NoMemory", 0);
> +		return false;
> +	}
> +	snprintf(fullpath, namelen, "%s/%s", path, filename);

You know the buffer is big enough already, so could reasonably use
memcpy (assuming you retain the length of path).

> +	FILE *f = fopen(fullpath, "w"); /* may need mode 'b' when c != NULL */

Always open it as binary, then.

> +int save_complete_htmlSaveFileFormat(const char *path, const char *filename, 
> +		xmlDocPtr cur, const char *encoding, int format)

Needs doxygen.

> +{
> +	int ret;
> +	int len = strlen(path) + strlen(filename) + 2;
> +	char *finame = malloc(len);

Can this be named better; it's not clear what it's meant to be.

> +	if (!finame){
> +		warn_user("NoMemory", 0);
> +		return -1;
> +	}
> +	snprintf(finame, len, "%s/%s", path, filename);

Again, you know the buffer's big enough.

> +bool save_complete_gui_filetype(const char *path, const char *filename,
> +		int type)

Documentation. What's this for, anyway? You pass what I assume is type
information to save_complete_gui_save().

> Index: gtk/gtk_theme.c
> ===================================================================
> --- /dev/null	2009-04-16 19:17:07.000000000 +0100
> +++ gtk/gtk_theme.c	2009-07-10 12:49:36.000000000 +0100
> +char *current_theme_name = NULL;

I'd prefer to see this declared static. Provide accessors if you must.

> +void nsgtk_theme_init()

Needs void. Also, documentation.

> +{
> +	if (option_current_theme == 0)
> +		return;
> +	char *themefile = g_strconcat(res_dir_location, "themelist", NULL);

No check for failure.

> +	nsgtk_scaffolding *list = scaf_list;
> +	nsgtk_theme_verify(NULL);

I can't work out what special behaviour this triggers. Lack of comments
isn't helping.

> +	FILE *fp = fopen(themefile, "r");
> +	char buf[50];
> +	int row_count = 0;
> +	if (fp == NULL)
> +		return;

I'd prefer to see checks for calls failing immediately after the call
where possible.

> +	while (fgets(buf, sizeof(buf), fp)) {

!= NULL

> +void nsgtk_theme_add(const char *themename)

Documentation.

> +{
> +	GtkWidget *notification, *label;
> +	char *labelcontent, *themefile = g_strconcat(res_dir_location,
> +			"themelist", NULL);

Check for failure?

> +	/* conduct verification here; no adding duplicates to list */
> +	if (nsgtk_theme_verify(themename) == false) {
> +		warn_user(messages_get("gtkThemeDup"), 0);
> +		g_free(themefile);
> +		return;
> +	}
> +	FILE *fp = fopen(themefile, "a");
> +	if (fp == NULL) {
> +		warn_user(messages_get("gtkFileError"), themefile);
> +		g_free(themefile);
> +		return;
> +	}
> +	fprintf(fp, "%s\n", themename);
> +	fclose(fp);
> +	g_free(themefile);
> +	
> +	/* notification that theme was added successfully */
> +	notification = gtk_dialog_new_with_buttons(messages_get("gtkThemeAdd"),
> +			NULL, GTK_DIALOG_DESTROY_WITH_PARENT, GTK_STOCK_OK,
> +			GTK_RESPONSE_NONE, NULL);

Ditto.

> +	labelcontent = g_strconcat("\t\t\t", messages_get("gtkThemeAdd"),
> +			 "\t\t\t", NULL);

Ditto.

> +	label = gtk_label_new(labelcontent);

Ditto.

> +	g_signal_connect_swapped(notification, "response",
> +			G_CALLBACK(gtk_widget_destroy), notification);
> +	gtk_container_add(GTK_CONTAINER(GTK_DIALOG(notification)->vbox), label);
> +	gtk_widget_show_all(notification);
> +	g_free(labelcontent);
> +	
> +	/* update combo */
> +	if (wndPreferences == NULL)
> +		return;
> +	nsgtk_options_combo_theme_add(themename);

I can't work out if themename is being leaked or not.

> +}
> +
> +bool nsgtk_theme_verify(const char *themename)

Documentation.

> +{	
> +	long filelength;
> +	FILE *fp;
> +	size_t val;
> +	char buf[50];
> +	char *themefile = g_strconcat(res_dir_location, "themelist", NULL);

Check for failure?

> +	if (themename == NULL) {
> +		char *filecontent, *testfile;
> +		struct stat sta;
> +		fp = fopen(themefile, "r+");
> +		if (fp == NULL) {
> +			warn_user(messages_get("gtkFileError"), themefile);
> +			g_free(themefile);
> +			return true;
> +		}
> +		fseek(fp, 0L, SEEK_END);
> +		filelength = ftell(fp);
> +		filecontent = malloc(filelength + 2);

Check for failure?

> +		strcpy(filecontent, "gtk default theme\n");
> +		if (filecontent == NULL) {

It's a bit late now.

> +			warn_user(messages_get("NoMemory"), 0);
> +			g_free(themefile);
> +			return true;
> +		}
> +		fseek(fp, 0L, SEEK_SET);
> +		while (fgets(buf, sizeof(buf), fp)) {

!= NULL.

> +		/* iterate list */
> +			buf[strlen(buf) - 1] = '\0';
> +			testfile = g_strconcat(res_dir_location, "themes/", 
> +					buf, NULL);

Check for failure?

> +			/* check every directory */
> +			if (access(testfile, R_OK) == 0) {
> +				stat(testfile, &sta);
> +				if (S_ISDIR(sta.st_mode)) {
> +					free(testfile);
> +					buf[strlen(buf)] = '\n';

You've just replaced the trailing NUL with \n.

> +					strcat(filecontent, buf);

And then used strcat(), which expects a NUL-terminated string.

> +				}
> +			}
> +		}

The above will overrun the filecontent block. The length of "gtk default
theme\n" is not factored into the allocation request.

> +		fclose(fp);
> +		fp = fopen(themefile, "w");
> +		if (fp == NULL) {
> +			warn_user(messages_get("gtkFileError"), themefile);
> +			free(filecontent);
> +			g_free(themefile);
> +			return true;
> +		}
> +		val = fwrite(filecontent, strlen(filecontent), 1, fp);
> +		if (val == 0)
> +			LOG(("empty write themelist"));
> +		fclose(fp);
> +		free(filecontent);
> +		g_free(themefile);
> +		return true;
> +	} else {
> +		fp = fopen(themefile, "r");
> +		if (fp == NULL) {
> +			warn_user(messages_get("gtkFileError"), themefile);
> +			g_free(themefile);
> +			return false;
> +		}
> +		while (fgets(buf, sizeof(buf), fp)) {

!= NULL.

> +			buf[strlen(buf) - 1] = '\0';
> +			if (strcmp(buf, themename) == 0) {
> +				g_free(themefile);
> +				return false;

You don't close the file here.

> +			}
> +		}
> +		fclose(fp);
> +		g_free(themefile);
> +		return true;
> +	}
> +	
> +}
> +
> +void nsgtk_theme_implement(struct gtk_scaffolding *g)

Documentation.

> +{
> +	struct nsgtk_theme *theme[4];
> +	int i;
> +	for (i = 0; i < 3; i++)

3? What is this?

> +		theme[i] = nsgtk_theme_load(GTK_ICON_SIZE_MENU);

> +	theme[3] = nsgtk_theme_load(GTK_ICON_SIZE_LARGE_TOOLBAR);

Ditto.

Can you use an enum here, instead?

> +	for (i = BACK_BUTTON; i < PLACEHOLDER_BUTTON; i++) {
> +		if ((i == URL_BAR_ITEM) || (i == THROBBER_ITEM) || 
> +				(i == WEBSEARCH_ITEM))
> +			continue;
> +		if (nsgtk_scaffolding_button(g, i)->main != NULL) {

Is nsgtk_scaffolding_button guaranteed to return a valid pointer?

> +struct nsgtk_theme *nsgtk_theme_load(GtkIconSize s)

Documentation.

> +{
> +	if (current_theme_name == NULL)
> +		return nsgtk_theme_default(s);
> +	struct nsgtk_theme *theme = malloc(sizeof(struct nsgtk_theme));

Check for failure?

> +	if ((theme_cache_menu == NULL) || (theme_cache_toolbar == NULL))

Are these global?

> +		nsgtk_theme_prepare();

What if this fails? Can it fail?

> +void nsgtk_theme_prepare(void)

Documentation.

> +{
> +	if (current_theme_name == NULL)
> +		return;
> +	if (theme_cache_menu == NULL)
> +		theme_cache_menu = malloc(sizeof(struct nsgtk_theme_cache));

Check for failure?

> +	if (theme_cache_toolbar == NULL)
> +		theme_cache_toolbar = malloc(sizeof(struct nsgtk_theme_cache));

Ditto.

> +	char *path = g_strconcat(res_dir_location, "themes/",
> +			current_theme_name, "/", NULL);

Ditto.

> +	char *filename;
> +#define CACHE_IMAGE(p, q)\
> +	filename = g_strconcat(path, #q, ".png", NULL);\

Ditto.

> +	theme_cache_toolbar->image[p##_BUTTON] =\
> +			gdk_pixbuf_new_from_file_at_size(filename, 24, 24,\
> +			NULL);\
> +	theme_cache_menu->image[p##_BUTTON] =\
> +			gdk_pixbuf_new_from_file_at_size(filename, 16, 16,\
> +			NULL);\
> +	g_free(filename)

Need a blank line here. It's not clear where the macro body ends,
otherwise.

> +	CACHE_IMAGE(BACK, back);

[...]

> +#define CACHE_IMAGE(p, q)\
> +	filename = g_strconcat(path, #q, ".png", NULL);\

Check for failure?

> +	theme_cache_toolbar->searchimage[p] =\
> +			gdk_pixbuf_new_from_file_at_size(filename, 24, 24,\
> +			NULL);\
> +	theme_cache_menu->searchimage[p] =\
> +			gdk_pixbuf_new_from_file_at_size(filename, 16, 16,\
> +			NULL);\
> +	g_free(filename)

Blank line.

> +GtkImage *nsgtk_theme_image_default(int i, GtkIconSize s)

Documentation.

> +{
> +	char *imagefile;
> +	GtkImage *image;
> +	switch(i) {
> +#define BUTTON_IMAGE(p, q)\
> +	case p##_BUTTON:\
> i+		return GTK_IMAGE(gtk_image_new_from_stock(#q, s))

Blank line.

> +	BUTTON_IMAGE(BACK, gtk-go-back);
> +	case HISTORY_BUTTON:
> +		imagefile = g_strconcat(res_dir_location, 
> +				"arrow_down_8x32.png", NULL);

Error handling?

> +		image = GTK_IMAGE(gtk_image_new_from_file(imagefile));
> +		g_free(imagefile);
> +		return image;

> +	default:
> +		imagefile = g_strconcat(res_dir_location, "themes/Alpha.png", 
> +				NULL);

Error handling?

> +		image = GTK_IMAGE(gtk_image_new_from_file(imagefile));
> +		g_free(imagefile);
> +		return image;
> +	}	
> +}
> +
> +
> +#ifdef WITH_THEME_INSTALL
> +/**
> + * Handle CONTENT_THEME
> + */
> +void theme_install_start(struct content *c)

Documentation -- what's there doesn't match the function.

> +{
> +	assert(c);
> +	assert(c->type == CONTENT_THEME);
> +
> +	/* stop theme sitting in memory cache */
> +	c->fresh = false;
> +	if (!content_add_user(c, theme_install_callback, 0, 0)) {

Use == false, instead.

> +/**
> + * Callback for fetchcache() for theme install fetches.
> + */
> +void theme_install_callback(content_msg msg, struct content *c,
> +		intptr_t p1, intptr_t p2, union content_msg_data data)
> +{
> +	switch (msg) {
> +	case CONTENT_MSG_DONE:
> +		theme_install_content = c;
> +		if (!theme_install_read(c->source_data, c->source_size))

== false.

> +/**
> + * handler saves theme data as a local theme
> + */
> +bool theme_install_read(char *data, unsigned long len)

Document parameters. Why is data not const?

> +{
> +	char *filename;
> +	int handle = g_file_open_tmp("nsgtkthemeXXXXXX", &filename, NULL);

Failure?

> +struct nsgtk_theme *nsgtk_theme_default(GtkIconSize s)

Documentation.

> +{
> +	struct nsgtk_theme *theme = malloc(sizeof(struct nsgtk_theme));

Failure?

> +	for (int i = BACK_BUTTON; i <= PLACEHOLDER_BUTTON + 2; i++)

+ 2 seems magical.

> Index: gtk/gtk_toolbar.c
> ===================================================================
> --- /dev/null	2009-04-16 19:17:07.000000000 +0100
> +++ gtk/gtk_toolbar.c	2009-07-10 12:49:36.000000000 +0100
> +static gboolean nsgtk_toolbar_store_return(GtkWidget *widget, GdkDragContext 			*gdc, gint x, gint y, guint time, gpointer data);

80 columns.

> +/**
> + * change behaviour of scaffoldings while editing toolbar

This isn't very informative. What does it change? Why?

> + */
> +void nsgtk_toolbar_customization_init(struct gtk_scaffolding *g)
> +{
> +	int i;
> +	nsgtk_scaffolding *list = scaf_list;;

Spurious semicolon.

> +/**
> + * create store window
> + */
> +void nsgtk_toolbar_window_open(struct gtk_scaffolding *g)
> +{
> +	int x,y;
> +	struct nsgtk_theme *theme =
> +			nsgtk_theme_load(GTK_ICON_SIZE_LARGE_TOOLBAR);

Failure?

> +	window->glade = glade_xml_new(glade_toolbar_file_location, 
> +			"toolbarwindow", NULL);

Failure?
> 	
> +#define GET_TOOLWIDGET(p, q) window->p = glade_xml_get_widget(window->glade,\
> +			#q)

Failure? Also, blank line.

> +	window->numberh = 6;

This field isn't very obvious.

> +	for (int i = BACK_BUTTON; i < PLACEHOLDER_BUTTON; i++) {
> +		if (i == URL_BAR_ITEM)
> +			continue;
> +		window->store_buttons[i] =
> +				nsgtk_toolbar_make_widget(g, i, theme);

Failure?

> +/**
> + * set toolbar logical -> physical

What does this mean?

> + */
> +void nsgtk_toolbar_set_physical(struct gtk_scaffolding *g)
> +{
> +	int i;
> +	struct nsgtk_theme *theme =
> +			nsgtk_theme_load(GTK_ICON_SIZE_LARGE_TOOLBAR);

Failure?

> +/**
> + * physical update of all toolbars; resensitize
> + * \param g the 'front' scaffolding that called customize
> + */

This doesn't appear to match what I'd expect from the function name.

> +void nsgtk_toolbar_close(struct gtk_scaffolding *g)
> +{
> +	int i;
> +	nsgtk_scaffolding *list = scaf_list;
> +	while (list) {
> +		struct nsgtk_theme *theme =
> +				nsgtk_theme_load(GTK_ICON_SIZE_LARGE_TOOLBAR);

Failure?

> +/**
> + * physically add widgets to store window
> + */
> +bool nsgtk_toolbar_add_store_widget(GtkWidget *widget)
> +{
> +	if (window->numberh >= 6) {

What is so magical about 6? Use a #define or similar.

> +		window->currentbar = gtk_toolbar_new();

Failure?

> +	gtk_widget_set_size_request(widget, 111, 70);

These numbers look magical.

> +/**
> + * called when a widget is dropped onto the toolbar
> + */
> +gboolean nsgtk_toolbar_data(GtkWidget *widget, GdkDragContext *gdc, gint x,
> +		gint y, guint time, gpointer data)
> +{
> +	struct gtk_scaffolding *g = (struct gtk_scaffolding *)data;
> +	int ind = gtk_toolbar_get_drop_index(nsgtk_scaffolding_toolbar(g),
> +			x, y);
> +	int q, i;
> +	if (window->currentbutton == -1)
> +		return TRUE;
> +	struct nsgtk_theme *theme =
> +			nsgtk_theme_load(GTK_ICON_SIZE_LARGE_TOOLBAR);

Failure?

> +gboolean nsgtk_toolbar_move_complete(GtkWidget *widget,	GdkDragContext *gdc,
> +		gint x, gint y, GtkSelectionData *selection, guint info, guint
> +		time, gpointer data)

Documentation. Spurious tab.

> +/**
> + * called when hovering an item above the toolbar
> + */
> +gboolean nsgtk_toolbar_action(GtkWidget *widget, GdkDragContext	*gdc, gint x,
> +		gint y, guint time, gpointer data)

Spurious tab.

> +/**
> + * widget factory for creation of toolbar item widgets
> + * \param g the reference scaffolding
> + * \param i the id of the widget
> + * \param theme the theme to make the widgets from
> + */
> +GtkWidget *nsgtk_toolbar_make_widget(struct gtk_scaffolding *g, int i,
> +		struct nsgtk_theme *theme)
> +{
> +	GtkWidget *w = NULL, *image = NULL, *hbox = NULL, *entry = NULL;
> +	char *label;
> +	GtkStockItem item;
> +	switch(i){
> +#define MAKE_STOCKBUTTON(p, q) case p##_BUTTON:\
> +		gtk_stock_lookup(#q, &item);\
> +		label = remove_underscores(item.label, false);\

Can this fail?

> +		w = GTK_WIDGET(gtk_tool_button_new(GTK_WIDGET(\
> +				theme->image[p##_BUTTON]),label));\

Failure?

> +		free(label);\
> +		break

Blank line.

> +	case URL_BAR_ITEM:
> +		label = g_strconcat(res_dir_location, "netsurf-16x16.xpm", NULL);

Failure?

> +		hbox = gtk_hbox_new(FALSE, 0);

Ditto.

> +		image = GTK_WIDGET(gtk_image_new_from_file(label));

Ditto.

> +		g_free(label);
> +		entry = GTK_WIDGET(gtk_entry_new());

Ditto.

> +		gtk_box_pack_start(GTK_BOX(hbox), image, FALSE, FALSE, 0);
> +		gtk_box_pack_end(GTK_BOX(hbox), entry, TRUE, TRUE, 0);
> +		w = GTK_WIDGET(gtk_tool_item_new());

Ditto.

> +		gtk_container_add(GTK_CONTAINER(w), hbox);
> +		gtk_tool_item_set_expand(GTK_TOOL_ITEM(w), TRUE);
> +		break;
> +	case THROBBER_ITEM:
> +		if (edit_mode)
> +			return GTK_WIDGET(gtk_tool_button_new(GTK_WIDGET(
> +					gtk_image_new_from_pixbuf(
> +					nsgtk_throbber->framedata[0])), 
> +					"[throbber]"));

Ditto.

> +		image = GTK_WIDGET(gtk_image_new_from_pixbuf(
> +				nsgtk_throbber->framedata[0]));

Ditto.

> +		w = GTK_WIDGET(gtk_tool_item_new());

Ditto.

> +		gtk_container_add(GTK_CONTAINER(w), image);
> +		break;
> +	case WEBSEARCH_ITEM:
> +		if (edit_mode)
> +			return GTK_WIDGET(gtk_tool_button_new(GTK_WIDGET(
> +					gtk_image_new_from_stock("gtk-find",
> +					GTK_ICON_SIZE_LARGE_TOOLBAR)),
> +					"[websearch]"));

Ditto.

> +		hbox = gtk_hbox_new(FALSE, 0);

Ditto.

> +		image = GTK_WIDGET(gtk_image_new_from_stock("gtk-info",
> +				GTK_ICON_SIZE_LARGE_TOOLBAR));

Ditto.

> +		entry = GTK_WIDGET(gtk_entry_new());

Ditto.

> +		gtk_widget_set_size_request(entry, 77, -1);
> +		gtk_box_pack_start(GTK_BOX(hbox), image, FALSE, FALSE, 0);
> +		gtk_box_pack_end(GTK_BOX(hbox), entry, TRUE, TRUE, 0);
> +		w = GTK_WIDGET(gtk_tool_item_new());

Ditto.

> +		gtk_container_add(GTK_CONTAINER(w), hbox);
> +		break;
> +#define MAKE_MENUBUTTON(p, q) case p##_BUTTON:\
> +		label = remove_underscores(messages_get(#q), false);\

Can this fail?

> +		w = GTK_WIDGET(gtk_tool_button_new(GTK_WIDGET(\
> +				theme->image[p##_BUTTON]), label));\

Failure?

> +		free(label);\
> +		break

Blank line.

> +/**
> + * \return toolbar item id when a widget is an element of the scaffolding
> + * else -1
> + */
> +int nsgtk_toolbar_get_id_from_widget(GtkWidget *widget, struct gtk_scaffolding
> +		*g)

Ugh. Split on commas, not in the middle of a list item.

> +void nsgtk_toolbar_connect_all(struct gtk_scaffolding *g)

Documentation.

> +/**
> + * load toolbar settings from file
> + */
> +void nsgtk_toolbar_customization_load(struct gtk_scaffolding *g)
> +{
> +	int i, ii;
> +	char *val;
> +	char buffer[SLEN("11;|") * 2 * PLACEHOLDER_BUTTON]; /* numbers 0-99 */
> +	buffer[0] = '\0';
> +	char *buffer1, *subbuffer, *ptr, *pter;
> +	FILE *f = fopen(toolbar_indices_file_location, "r");

Failure?

> Index: gtk/gtk_menu.c
> ===================================================================
> --- /dev/null	2009-04-16 19:17:07.000000000 +0100
> +++ gtk/gtk_menu.c	2009-07-10 12:49:36.000000000 +0100
> +static unsigned int key;
> +static GdkModifierType mod;

What are these for?

> +#define IMAGE_ITEM(p, q, r)\
> +	ret->q##_menuitem = GTK_IMAGE_MENU_ITEM(\

Where has ret come from?

> +			gtk_image_menu_item_new_with_mnemonic(\
> +			messages_get(#r)));\

Failure?

> +	gtk_accelerator_parse(messages_get(#r "Accel"), &key, &mod);\
> +	if (key > 0)\
> +		gtk_widget_add_accelerator(GTK_WIDGET(ret->q##_menuitem),\
> +				"activate", group, key, mod, GTK_ACCEL_VISIBLE);\
> +	gtk_menu_shell_append(GTK_MENU_SHELL(ret->p##_menu),\
> +			GTK_WIDGET(ret->q##_menuitem));\
> +	gtk_widget_show(GTK_WIDGET(ret->q##_menuitem))

Blank line.

> +#define CHECK_ITEM(p, q, r)\
> +	ret->q##_menuitem = GTK_CHECK_MENU_ITEM(\
> +			gtk_check_menu_item_new_with_mnemonic(\
> +			messages_get(#r)));\

Failure?

> +	gtk_menu_shell_append(GTK_MENU_SHELL(ret->p##_menu),\
> +			GTK_WIDGET(ret->q##_menuitem));\
> +	gtk_widget_show(GTK_WIDGET(ret->q##_menuitem))
> +	
> +#define SET_SUBMENU(q)\
> +	ret->q##_submenu = nsgtk_menu_##q##_submenu(group);\
> +	gtk_menu_item_set_submenu(GTK_MENU_ITEM(ret->q##_menuitem),\
> +			GTK_WIDGET(ret->q##_submenu->q##_menu))		

Blank line.

> +#define ADD_SEP(q)\
> +	w = gtk_separator_menu_item_new();\

Where has 'w' come from? 

> +	gtk_menu_shell_append(GTK_MENU_SHELL(ret->q##_menu), w);\
> +	gtk_widget_show(w)

Blank line.


> +struct nsgtk_file_menu *nsgtk_menu_file_menu(GtkAccelGroup *group)

Documentation!

> +{
> +	GtkWidget *w;
> +	struct nsgtk_file_menu *ret = malloc(sizeof(struct nsgtk_file_menu));

Failure?

> +	ret->file_menu = GTK_MENU(gtk_menu_new());

Failure?

> +struct nsgtk_edit_menu *nsgtk_menu_edit_menu(GtkAccelGroup *group)

Documentation.

> +{
> +	GtkWidget *w;
> +	struct nsgtk_edit_menu *ret = malloc(sizeof(struct nsgtk_edit_menu));

Failure?

> +	ret->edit_menu = GTK_MENU(gtk_menu_new());

Failure?

> +struct nsgtk_view_menu *nsgtk_menu_view_menu(GtkAccelGroup *group)

Documentation?

> +{
> +	GtkWidget *w;
> +	struct nsgtk_view_menu *ret = malloc(sizeof(struct nsgtk_view_menu));

Failure?

> +	ret->view_menu = GTK_MENU(gtk_menu_new());

Ditto. This is getting tiresome.

> +struct nsgtk_nav_menu *nsgtk_menu_nav_menu(GtkAccelGroup *group)

Documentation.

> +{
> +	GtkWidget *w;
> +	struct nsgtk_nav_menu *ret = malloc(sizeof(struct nsgtk_nav_menu));

Failure?

> +	ret->nav_menu = GTK_MENU(gtk_menu_new());

Failure?

> +struct nsgtk_tabs_menu *nsgtk_menu_tabs_menu(GtkAccelGroup *group)

Documentation.

> +{
> +	struct nsgtk_tabs_menu *ret = malloc(sizeof(struct nsgtk_tabs_menu));

Failure?

> +	ret->tabs_menu = GTK_MENU(gtk_menu_new());

Ditto.

> +struct nsgtk_help_menu *nsgtk_menu_help_menu(GtkAccelGroup *group)

Documentation.

> +{
> +	GtkWidget *w;
> +	struct nsgtk_help_menu *ret = malloc(sizeof(struct nsgtk_help_menu));

Failure?

> +	ret->help_menu = GTK_MENU(gtk_menu_new());

Failure?

> +struct nsgtk_export_submenu *nsgtk_menu_export_submenu(GtkAccelGroup *group)

Documentation.

> +{
> +	struct nsgtk_export_submenu *ret = malloc(sizeof(struct
> +			nsgtk_export_submenu));

Failure?

> +	ret->export_menu = GTK_MENU(gtk_menu_new());

Failure?

> +struct nsgtk_scaleview_submenu *nsgtk_menu_scaleview_submenu(GtkAccelGroup
> +		*group)

Documentation. Hideous linebreak style.

> +{
> +	struct nsgtk_scaleview_submenu *ret = malloc(sizeof(struct
> +			nsgtk_scaleview_submenu));

Failure? Again, nasty linebreak.

> +	ret->scaleview_menu = GTK_MENU(gtk_menu_new());

Failure?

> +struct nsgtk_images_submenu *nsgtk_menu_images_submenu(GtkAccelGroup *group)

Documentation?

> +{
> +	struct nsgtk_images_submenu *ret = malloc(sizeof(struct
> +			nsgtk_images_submenu));

Failure? Line breaking.

> +	ret->images_menu = GTK_MENU(gtk_menu_new());

Failure?

> +struct nsgtk_toolbars_submenu *nsgtk_menu_toolbars_submenu(GtkAccelGroup *group)

Documentation.

> +{
> +	struct nsgtk_toolbars_submenu *ret = malloc(sizeof(struct
> +			nsgtk_toolbars_submenu));

You know what I'm going to write here:

Failure? Line breaking.

> +	ret->toolbars_menu = GTK_MENU(gtk_menu_new());

Failure?

> +struct nsgtk_debugging_submenu *nsgtk_menu_debugging_submenu(GtkAccelGroup
> +		*group)

Documentation. Line breaking.

> +{
> +	struct nsgtk_debugging_submenu *ret = malloc(sizeof(struct
> +			nsgtk_debugging_submenu));

Failure? Line breaking.

> +	ret->debugging_menu = GTK_MENU(gtk_menu_new());

Failure?

> Index: gtk/gtk_search.c
> ===================================================================
> --- /dev/null	2009-04-16 19:17:07.000000000 +0100
> +++ gtk/gtk_search.c	2009-07-10 12:49:36.000000000 +0100
> + /** \file
> + * Free text search (implementation)

Is it? I'd have thought it's the frontend component.

> +void nsgtk_search_init(struct gtk_scaffolding *g);

Is this meant to be static?

> +gboolean nsgtk_search_forward_button_clicked(GtkWidget *widget, gpointer data)

Documentation.

> +gboolean nsgtk_search_back_button_clicked(GtkWidget *widget, gpointer data)

Ditto.

> +void nsgtk_search_init(struct gtk_scaffolding *g)

Yes, then. Documentation.

> +gboolean nsgtk_search_close_button_clicked(GtkWidget *widget, gpointer data)

Documentation.

> +gboolean nsgtk_search_entry_changed(GtkWidget *widget, gpointer data)

Ditto.

> +gboolean nsgtk_search_entry_activate(GtkWidget *widget, gpointer data)

Ditto.

> +gboolean nsgtk_search_entry_key(GtkWidget *widget, GdkEventKey *event, 
> +		gpointer data)

Ditto.

> +gboolean nsgtk_websearch_activate(GtkWidget *widget, gpointer data)

Ditto.

> +gboolean nsgtk_websearch_clear(GtkWidget *widget, GdkEventFocus *f, 
> +		gpointer data)

Ditto.

> +void gui_search_set_status(bool found)

Ditto.

> +void gui_search_set_hourglass(bool active)

Ditto. C.f. framebuffer comments.

> +char *gui_search_get_string(void)

Ditto.

> +void gui_search_add_recent(const char *string)

Ditto.

> +{
> +	recent_search[0] = strdup(string);

Failure?

> +void gui_search_set_forward_state(bool inactive)
> +void gui_search_set_back_state(bool inactive)
> +bool gui_search_get_case_sens(void)
> +bool gui_search_get_show_all(void)

Documentation.

> Index: gtk/gtk_theme.h
> ===================================================================
> --- /dev/null	2009-04-16 19:17:07.000000000 +0100
> +++ gtk/gtk_theme.h	2009-07-10 12:49:37.000000000 +0100
> +extern char *current_theme_name;

C.f. gtk_theme.c

> +struct nsgtk_theme {
> +	GtkImage	*image[PLACEHOLDER_BUTTON];
> +	GtkImage	*searchimage[3]; /* back, forward, close */
> +	/* apng throbber element */
> +};

Haven't I seen this exact struct declared elsewhere? Does it need to be
public?

> Index: beos/beos_search.cpp
> ===================================================================
> --- /dev/null	2009-04-16 19:17:07.000000000 +0100
> +++ beos/beos_search.cpp	2009-07-10 12:50:00.000000000 +0100

c.f. framebuffer.

> Index: riscos/searchweb.c
> ===================================================================
> --- /dev/null	2009-04-16 19:17:07.000000000 +0100
> +++ riscos/searchweb.c	2009-07-10 12:50:10.000000000 +0100
> +char *search_engines_file_location;
> +char *search_default_ico_location;

These should be core options.

> +struct content *search_ico;

What is this?

> Index: desktop/searchweb.c
> ===================================================================
> --- /dev/null	2009-04-16 19:17:07.000000000 +0100
> +++ desktop/searchweb.c	2009-07-10 12:50:16.000000000 +0100
> + /** \file
> + * Free text search (core)

I don't think that's accurate.

> +static struct search_provider {
> +	char *name;
> +	char *hostname;
> +	char *searchstring;
> +	char *ico;
> +} current_search_provider;

Documentation.

> +struct content *search_ico = 0;

NULL. Why does riscos/searchweb.c also define a global named search_ico?

> +bool search_web_new_window(struct browser_window *bw, const char *searchterm)

Documentation!

> +{
> +	char *encsearchterm;
> +	char *url;
> +	if (url_escape(searchterm,0, true, NULL, &encsearchterm) != 
> +			URL_FUNC_OK)
> +		return false;
> +	url = search_web_get_url(encsearchterm);

Failure?

Where is encsearchterm freed?

> +bool search_is_url(const char *url)

Documentation.

> +/**
> + * caches the details of the current web search provider
> + * \param reference the enum value of the provider
> + * browser init code [as well as changing preferences code] should call
> + * search_web_provider_details(option_search_provider)
> + */
> +
> +void search_web_provider_details(int reference)

If reference is an enum value, then why is it an int here?

> +	while (fgets(buf, sizeof(buf), f)) {

!= NULL.

> +	strcpy(delim, "|");

Is this not just a constant?

> +	if (current_search_provider.name)

!= NULL.

> +		free(current_search_provider.name);
> +	current_search_provider.name = strdup(strtok(buf, delim));

Failure?

> +	if (current_search_provider.hostname)
> +		free(current_search_provider.hostname);
> +	current_search_provider.hostname = strdup(strtok(NULL, delim));

Ditto.

> +	if (current_search_provider.searchstring)
> +		free(current_search_provider.searchstring);
> +	current_search_provider.searchstring = strdup(strtok(NULL, delim));

Ditto.

> +	if (current_search_provider.ico)
> +		free(current_search_provider.ico);
> +	current_search_provider.ico = strdup(strtok(NULL, delim));

Ditto.

> +char *search_web_from_term(const char *searchterm)

Documentation.

> +{
> +	char *encsearchterm;
> +	if (url_escape(searchterm, 0, true, NULL, &encsearchterm)
> +			!= URL_FUNC_OK)
> +		return strdup(searchterm);
> +	return search_web_get_url(encsearchterm);
> +}

This leaks encsearchterm.

> +
> +char *search_web_provider_name()
> +char *search_web_provider_host()
> +char *search_web_ico_name()

Documentation, void.

> +char *search_web_get_url(const char *encsearchterm)

Documentation.

> +{
> +	char *pref, *ret;
> +	int len;
> +	if (current_search_provider.searchstring)
> +		pref = strdup(current_search_provider.searchstring);
> +	else
> +		pref = strdup("http://www.google.com/search?q=%s");

Failure?

> +	len = strlen(encsearchterm) + strlen(pref);
> +	ret = malloc(len -1);

Failure? *Minus* 1?

> +struct content *search_web_retrieve_ico(bool localdefault)

Documentation.

> +{
> +	char *url;
> +	if (localdefault) {
> +		if (search_default_ico_location == NULL)
> +			return NULL;
> +		url = malloc(SLEN("file://") + strlen(
> +				search_default_ico_location) + 1);

Failure?

> +		strcpy(url, "file://");
> +		strcat(url, search_default_ico_location);
> +	} else {
> +		url = search_web_ico_name();
> +	}
> +
> +	struct content *icocontent = NULL;
> +	if (url != NULL)		
> +		icocontent = fetchcache(url, search_web_ico_callback,
> +				0, 0, 20, 20, true, 0,
> +				0, false, false);

Failure?

> +	free(url);
> +	if (icocontent == NULL)
> +		return NULL;
> +	
> +	fetchcache_go(icocontent, 0, search_web_ico_callback,
> +			0, 0, 20, 20,
> +			0, 0, false, 0);
> +
> +	if (icocontent == NULL) {
> +		LOG(("web search ico loading delayed"));
> +		return NULL;
> +	}

This is redundant, no?

> +	return icocontent;
> +}
> +
> +void search_web_ico_callback(content_msg msg, struct content *ico,
> +		intptr_t p1, intptr_t p2, union content_msg_data data)

Documentation!

> +	case CONTENT_MSG_DONE:
> +		LOG(("got favicon '%s'", ico->url));
> +		if (ico->type == CONTENT_ICO) {
> +			search_ico = ico; /* cache */
> +			gui_window_set_search_ico();

Set it to what? Please tell me it doesn't expect to extract it from
search_ico.

> Index: desktop/searchweb.h
> ===================================================================
> --- /dev/null	2009-04-16 19:17:07.000000000 +0100
> +++ desktop/searchweb.h	2009-07-10 12:50:17.000000000 +0100
> +typedef enum {
> +	OSP_GOOGLE,
> +	OSP_YAHOO,
> +	OSP_MICROSOFT,
> +	OSP_BUSINESS,
> +	OSP_OMGILI,
> +	OSP_BBC,
> +	OSP_UBUNTU,
> +	OSP_COMMONS,
> +	OSP_ASK,
> +	OSP_ANSWERS,
> +	OSP_DICTIONARYCOM,
> +	OSP_YOUTUBE,
> +	OSP_AEROMP3,
> +	OSP_AOL,
> +	OSP_BAIDU,
> +	OSP_AMAZON,
> +	OSP_EBAY,
> +	OSP_IMDB,
> +	OSP_ESPN,
> +	OSP_WIKIPEDIA
> +} default_search_provider;

So there is a search provider enum. What is OSP? Does this need to be
public?

> +struct content *search_web_retrieve_ico(bool localdefault);
> +
> +void search_web_ico_callback(content_msg msg, struct content *ico,
> +		intptr_t p1, intptr_t p2, union content_msg_data data);

Why is this public?

> Index: desktop/search.c
> ===================================================================
> --- /dev/null	2009-04-16 19:17:07.000000000 +0100
> +++ desktop/search.c	2009-07-10 12:50:19.000000000 +0100

> +struct browser_window *search_current_window = NULL;
> +static char *search_string = NULL;
> +static struct list_entry search_head = { 0, 0, NULL, NULL, NULL, NULL, NULL };
> +static struct list_entry *search_found = &search_head;
> +static struct list_entry *search_current = NULL;
> +static struct content *search_content = NULL;
> +static bool search_prev_case_sens = false;

> +bool search_insert;
> +char *recent_search[RECENT_SEARCHES];

It would be significantly better if there were a search object that
contained all the above data. You'd have one such object per search
context (i.e. one per window or tab).

You'd need some API to create and destroy such objects as well as
changes to all the other APIs to pass the search context around.

> +/**
> + * Begins/continues the search process
> + * Note that this may be called many times for a single search.
> + *
> + * \param  forwards  search forwards from start/current position
> + */
> +
> +void start_search(bool forwards)
> +{
> +	int string_len;
> +	const char *string;
> +	int i = 0;
> +
> +	string = gui_search_get_string();
> +	assert(string);

Can the frontend not pass this in as a parameter?

> +	gui_search_add_recent(string);

What's this for?

> +/**
> + * Search for a string in the box tree
> + *
> + * \param string the string to search for
> + * \param string_len length of search string
> + * \param case_sens whether to perform a case sensitive search
> + * \param forwards direction to search in
> + */
> +void do_search(const char *string, int string_len, bool case_sens,
> +		bool forwards)
> +{
> +	/* check if we need to start a new search or continue an old one */
> +	if (!search_string || c != search_content || !search_found->next ||
> +	    search_prev_case_sens != case_sens ||
> +	    (case_sens && strcmp(string, search_string) != 0) ||
> +	    (!case_sens && strcasecmp(string, search_string) != 0)) {

Dear lord this is awful. The frontend presumably knows this already, so
can pass it in as a parameter, rather than have this hideous,
potentially broken, mess.

> +		bool res;
> +
> +		if (search_string)
> +			free(search_string);
> +		search_current = 0;
> +		free_matches();
> +
> +		search_string = malloc(string_len + 1);
> +		if (search_string) {

!= NULL.

> +		gui_search_set_hourglass(true);

I've already commented on this call.

> +/**
> + * Determines whether any portion of the given text box should be
> + * selected because it matches the current search string.
> + *
> + * \param  g             gui window
> + * \param  start_offset  byte offset within text of string to be checked
> + * \param  end_offset    byte offset within text
> + * \param  start_idx     byte offset within string of highlight start
> + * \param  end_idx       byte offset of highlight end
> + * \return true iff part of the box should be highlighted
> + */
> +
> +bool gui_search_term_highlighted(struct gui_window *g,
> +		unsigned start_offset, unsigned end_offset,
> +		unsigned *start_idx, unsigned *end_idx)

If this is core, it shouldn't start with gui_.

> Index: desktop/search.h
> ===================================================================
> --- /dev/null	2009-04-16 19:17:07.000000000 +0100
> +++ desktop/search.h	2009-07-10 12:50:19.000000000 +0100
> +#define RECENT_SEARCHES 8
> +
> +extern char *recent_search[RECENT_SEARCHES];
> +extern bool search_insert;

These need to die.

> Index: desktop/save_complete.c
> ===================================================================
> --- /dev/null	2009-04-16 19:17:07.000000000 +0100
> +++ desktop/save_complete.c	2009-07-10 12:50:20.000000000 +0100
> +#ifdef RISCOS
> +	static char pathsep = '.';
> +#else
> +	static char pathsep = '/';
> +#endif

I don't like this. Nor, for that matter, do I understand why it exists.

> +/** List of urls seen and saved so far. */
> +static struct save_complete_entry *save_complete_list = 0;

I'd prefer this to not be global. Stick it on the stack in the top-level
call, and pass a pointer to it around internally.

> Index: desktop/save_complete.h
> ===================================================================
> --- /dev/null	2009-04-16 19:17:07.000000000 +0100
> +++ desktop/save_complete.h	2009-07-10 12:50:22.000000000 +0100
> +/**
> + * conducts the filesystem save appropriate to the gui
> + * \param path save path
> + * \param filename name of file to save
> + * \param c content to save, or NULL
> + * \param len data length
> + * \param sourcedata pointer to data to save, NULL when all data in sourcedata
> + * \param type integer filetype [riscos]
> + * \return true for success
> + */
> +bool save_complete_gui_save(const char *path, const char *filename, struct content *c, int len, char *sourcedata, int type);

80 columns.

> Index: render/html.h
> ===================================================================
> --- render/html.h	(revision 8438)
> +++ render/html.h	(working copy)
> @@ -128,6 +128,8 @@
>  	colour background_colour;  /**< Document background colour. */
>  	const struct font_functions *font_func;
>  
> +	struct content *favicon;
> +

Documentation?

> Index: image/ico.c
> ===================================================================
> --- image/ico.c	(revision 8438)
> +++ image/ico.c	(working copy)
> @@ -114,6 +114,15 @@
>  			background_colour, BITMAPF_NONE);
>  }
>  
> +bool nsico_set_bitmap_from_size(struct content *c, int width, int height)

Documentation?

> +{
> +	struct bmp_image *bmp = ico_find(c->data.ico.ico, width, height);

Failure?

> +	if (!bmp->decoded)

== false.

> +	  	if (bmp_decode(bmp) != BMP_OK)

Can be merged with parent "if" through use of &&.

> Index: gtk/gtk_gui.h
> ===================================================================
> --- gtk/gtk_gui.h	(revision 8438)
> +++ gtk/gtk_gui.h	(working copy)
> @@ -25,8 +25,18 @@
>  #include <glade/glade.h>
>  
>  extern bool gui_in_multitask;
> -extern GladeXML *gladeWindows;
> -extern char *glade_file_location;
> +extern GladeXML *gladeNetsurf;
> +extern GladeXML *gladePassword;
> +extern GladeXML *gladeWarning;
> +extern GladeXML *gladeLogin;
> +extern GladeXML *gladeSsl;

Do these things actually need to be global?

> +extern char *glade_netsurf_file_location;
> +extern char *glade_password_file_location;
> +extern char *glade_warning_file_location;
> +extern char *glade_login_file_location;
> +extern char *glade_ssl_file_location;
> +extern char *glade_toolbar_file_location;
> +extern char *toolbar_indices_file_location;
>  extern char *options_file_location;
>  extern char *res_dir_location;
>  extern char *print_options_file_location;

Ditto.

> Index: gtk/gtk_window.c
> ===================================================================
> --- gtk/gtk_window.c	(revision 8438)
> +++ gtk/gtk_window.c	(working copy)
> @@ -35,6 +36,37 @@
>  #include <gdk/gdkkeysyms.h>
>  #include <assert.h>
>  
> +struct gui_window {
> +	/* All gui_window objects have an ultimate scaffold */
> +	nsgtk_scaffolding	*scaffold;
> +	/* A gui_window is the rendering of a browser_window */
> +	struct browser_window	*bw;
> +	struct browser_mouse 	*mouse;
> +
> +	/* These are the storage for the rendering */
> +	int			caretx, carety, careth;
> +	gui_pointer_shape	current_pointer;
> +	int			last_x, last_y;
> +
> +	/* Within GTK, a gui_window is a scrolled window
> +	 * with a viewport inside
> +	 * with a gtkfixed in that
> +	 * with a drawing area in that
> +	 * The scrolled window is optional and only chosen
> +	 * for frames which need it. Otherwise we just use
> +	 * a viewport.
> +	 */
> +	GtkWidget		*tab;
> +	GtkScrolledWindow	*scrolledwindow;
> +	GtkViewport		*viewport;
> +	GtkFixed		*fixed;
> +	GtkDrawingArea		*drawing_area;
> +	gulong			signalhandler[2];

What is this for? Why 2?

> +
> +	/* Keep gui_windows in a list for cleanup later */
> +	struct gui_window	*next, *prev;
> +};

All fields need documenting separately.

>  struct gui_window *window_list = 0;	/**< first entry in win list*/

NULL.

> Index: gtk/gtk_scaffolding.c
> ===================================================================
> --- gtk/gtk_scaffolding.c	(revision 8438)
> +++ gtk/gtk_scaffolding.c	(working copy)
> +nsgtk_scaffolding *scaf_list = NULL; /**< global list for interface changes */

This can be named better.

> +void nsgtk_window_close(struct gtk_scaffolding *g)
>  {
> -	struct gtk_scaffolding *g = data;
> +	/* close all tabs first */
> +	gint numbertabs = gtk_notebook_get_n_pages(g->notebook);
> +	while (numbertabs-- > 1) {
> +		nsgtk_tab_close_current(g->notebook);
> +	}
>  	LOG(("Being Destroyed = %d", g->being_destroyed));
> -	if (g->history_window->window) {
> +/*	if ((g->history_window) && (g->history_window->window)) {
>  		gtk_widget_destroy(GTK_WIDGET(g->history_window->window));
>  	}
> -	gtk_widget_destroy(GTK_WIDGET(g->window));
> -	
> +	if (g->window)
> +		gtk_widget_destroy(GTK_WIDGET(g->window));
> +*/

Why is the above commented out?

>  	if (--open_windows == 0)
>  		netsurf_quit = true;
>  
> @@ -285,38 +236,38 @@
>  		g->being_destroyed = 1;
>  		nsgtk_window_destroy_browser(g->top_level);
>  	}
> +	if (g->prev)

!= NULL.

> +		g->prev->next = g->next;
> +	else
> +		scaf_list = g->next;
> +
> +	if (g->next)

Ditto.

>  static gboolean nsgtk_window_edit_menu_clicked(GtkWidget *widget, 
>  		struct gtk_scaffolding *g)
>  {
> -	nsgtk_scaffolding_update_edit_actions_sensitivity (g, g->xml, FALSE);
> +	nsgtk_scaffolding_update_edit_actions_sensitivity(g, g->xml, false);

Is the third parameter a bool or a gboolean? If the latter, then it
should be FALSE.
 	
>  gboolean nsgtk_window_url_activate_event(GtkWidget *widget, gpointer data)
>  {
>  	struct gtk_scaffolding *g = data;
> -        struct browser_window *bw = nsgtk_get_browser_for_gui(g->top_level);
> +        struct browser_window *bw = gui_window_get_browser_window(g->top_level);
> +	char *url = (char *)gtk_entry_get_text(GTK_ENTRY(g->url_bar));

Does this return an allocated object or a pointer to constant data?

> +	if (!search_is_url(url))

== false.

> +		url = search_web_from_term(url);

This returns an allocated object, no?

> +	browser_window_go(bw, url, 0, true);
>  
> -	browser_window_go(bw, gtk_entry_get_text(GTK_ENTRY(g->url_bar)),
> -                          0, true);
> -

So, in one of the possible routes through this function, you're leaking
memory.

> Index: gtk/dialogs/gtk_options.c
> ===================================================================
> --- gtk/dialogs/gtk_options.c	(revision 8438)
> +++ gtk/dialogs/gtk_options.c	(working copy)
>  void nsgtk_options_load(void) 
>  {
> +	/* populate theme combo from themelist file */
> +	box = GTK_BOX(glade_xml_get_widget(gladeFile, "themehbox"));
> +	combotheme = gtk_combo_box_new_text();

Failure?

> +	themefile = g_strconcat(res_dir_location, "themelist", NULL);

Ditto.

> +	fp = fopen((const char *)themefile, "r");
> +	g_free(themefile);
> +	if (fp == NULL) {
> +		LOG(("Failed opening themes file"));
> +		warn_user("FileError", (const char *) themefile);
> +		return;
> +	}
> +	while (fgets(buf, sizeof(buf), fp)) {

!= NULL.

> +COMBO_CHANGED(comboSearch, option_search_provider)	
> +	nsgtk_scaffolding *current = scaf_list;
> +	char *name;
> +	/* refresh web search prefs from file */
> +	search_web_provider_details(option_search_provider);
> +	/* retrieve ico */
> +	search_ico = search_web_retrieve_ico(false);
> +	/* callback may handle changing gui */
> +	if (search_ico != NULL)
> +		gui_window_set_search_ico();
> +	/* set entry */
> +	name = search_web_provider_name();

Failure?

> +	while (current) {
> +		nsgtk_scaffolding_set_websearch(current, name);
> +		current = nsgtk_scaffolding_iterate(current);
> +	}
> +	free(name);
> +END_HANDLER
> +
> +COMBO_CHANGED(combotheme, option_current_theme)
> +	nsgtk_scaffolding *current = scaf_list;
> +	if (option_current_theme != 0) {
> +		if (current_theme_name != NULL)
> +			free(current_theme_name);
> +		current_theme_name = strdup(gtk_combo_box_get_active_text(
> +				GTK_COMBO_BOX(combotheme)));

Failure?

> +BUTTON_CLICKED(buttonaddtheme)
> +	char *themesfolder, *filename, *directory;
> +	GtkWidget *fc = gtk_file_chooser_dialog_new(
> +			messages_get("gtkAddThemeTitle"),
> +			GTK_WINDOW(wndPreferences),
> +			GTK_FILE_CHOOSER_ACTION_SELECT_FOLDER,
> +			GTK_STOCK_OK, GTK_RESPONSE_ACCEPT,
> +			GTK_STOCK_CANCEL, GTK_RESPONSE_CANCEL, NULL);
> +	themesfolder = g_strconcat(res_dir_location, "themes", NULL);

Failure?

> Index: gtk/dialogs/gtk_options.h
> ===================================================================
> --- gtk/dialogs/gtk_options.h	(revision 8438)
> +++ gtk/dialogs/gtk_options.h	(working copy)
>  GtkDialog* nsgtk_options_init(struct browser_window *bw, GtkWindow *parent);		/** Init options and load window */
>  void nsgtk_options_load(void);		/** Load current options into window */
>  void nsgtk_options_save(void);		/** Save options from window */
> +bool nsgtk_options_combo_theme_add(const char *themename); /** add new theme name to combo */
>  

80 columns.

> Index: gtk/gtk_gui.c
> ===================================================================
> --- gtk/gtk_gui.c	(revision 8438)
> +++ gtk/gtk_gui.c	(working copy)
> @@ -67,15 +67,27 @@
>  char *default_stylesheet_url;
>  char *adblock_stylesheet_url;
>  char *options_file_location;
> -char *glade_file_location;
> +char *glade_netsurf_file_location;
> +char *glade_password_file_location;
> +char *glade_warning_file_location;
> +char *glade_login_file_location;
> +char *glade_ssl_file_location;
> +char *glade_toolbar_file_location;
> +char *toolbar_indices_file_location;
>  char *res_dir_location;
>  char *print_options_file_location;
> +char *search_engines_file_location;
> +char *search_default_ico_location;
>  
> -struct gui_window *search_current_window = 0;
> +/* struct gui_window *search_current_window = 0; */

Why is this commented out? If it's redundant, delete it.

> Index: Docs/Doxyfile
> ===================================================================
> --- Docs/Doxyfile	(revision 8438)
> +++ Docs/Doxyfile	(working copy)
> @@ -896,6 +896,8 @@
>  
>  PREDEFINED             = riscos CSS_INTERNALS WITH_ARTWORKS WITH_BMP WITH_DRAW WITH_DRAW_EXPORT WITH_GIF WITH_JPEG WITH_MMAP WITH_MNG WITH_NSSPRITE WITH_NS_SVG WITH_PLUGIN WITH_RSVG WITH_SAVE_COMPLETE WITH_SPRITE WITH_THEME_INSTALL WITH_PDF_EXPORT
>  
> +PREDEFINED		= gtk WITH_THEME_INSTALL
> +

Does this add to the previous line, or override it?

> Index: utils/utils.c
> ===================================================================
> --- utils/utils.c	(revision 8438)
> +++ utils/utils.c	(working copy)
> @@ -61,6 +61,23 @@
>  	return 1;
>  }
>  
> +char *remove_underscores(const char *s, bool replacespace)

Documentation.

> +{
> +	size_t i, len, offset = 0;
> +	char *ret;
> +	len = strlen(s);
> +	ret = malloc(len + 1);

Failure?

> +	for (i = 0; i < len; i++)
> +		if (s[i] != '_')
> +			ret[i - offset] = s[i];
> +		else if (replacespace)
> +			ret[i - offset] = ' ';
> +		else
> +			offset++;
> +	ret[i - offset] = '\0';

i - offset isn't particularly clear. What's wrong with maintaining an
input buffer index (i) and an output buffer index?

> Index: utils/container.c
> ===================================================================
> --- utils/container.c	(revision 8438)
> +++ utils/container.c	(working copy)
>  	strncpy((char *)ctx->directory[ctx->entries - 1].filename,
> -				(char *)entryname, 16);
> +				(char *)entryname, 64);

use sizeof()

>  struct container_ctx *container_open(const char *filename)
>  {
> +	size_t val;
>  	struct container_ctx *ctx = calloc(sizeof(struct container_ctx), 1);
>  
>  	ctx->fh = fopen(filename, "rb");
>  
>  	if (ctx->fh == NULL) {
>  		free(ctx);
> -		return NULL;
> +	return NULL;

Spurious whitespace change.

> +#ifdef WITH_THEME_INSTALL
> +
> +/**
> + * install theme from container
> + * \param themefile a file containing the containerized theme
> + * \param dirbasename a directory basename including trailing path sep; the
> + * full path of the theme is then a subdirectory of that
> + */
> +
> +char *container_extract_theme(const char *themefile, const char *dirbasename)
> +{
> +	struct stat statbuf;
> +	struct container_ctx *cctx;
> +	FILE *fh;
> +	size_t val;
> +	const unsigned char *e, *d;
> +	char *themename, *dirname;
> +	char path[PATH_MAX];
> +	int state = 0;
> +	unsigned int i;
> +	u_int32_t flen;
> +
> +	cctx = container_open(themefile);
> +	if (cctx == NULL) {
> +		warn_user("FileOpenError", themefile);
> +		return NULL;
> +	}
> +	themename = (char *)container_get_name(cctx);
> +	LOG(("theme name: %s", themename));
> +	LOG(("theme author: %s", container_get_author(cctx)));
> +	
> +	dirname = malloc(strlen(dirbasename) + strlen(themename) + 2);

Failure?

> Index: riscos/save.c
> ===================================================================
> --- riscos/save.c	(revision 8438)
> +++ riscos/save.c	(working copy)
> +bool save_complete_gui_save(const char *path, const char *filename, struct 
> +		content *c, int len, char *sourcedata, int type)
> +{
> +	char *finame;
> +	int namelen = strlen(path) + strlen(filename) + 2;
> +	finame = malloc(namelen);
> +	if (!finame) {

== NULL.

> +int save_complete_htmlSaveFileFormat(const char *path, const char *filename, 
> +		xmlDocPtr cur, const char *encoding, int format)
> +{
> +	int ret;
> +	int len = strlen(path) + strlen(filename) + 2;
> +	char *finame = malloc(len);
> +	if (!finame){

== NULL.

> +bool save_complete_gui_filetype(const char *path, const char *filename, int type)
> +{
> +	os_error *error;
> +	int len = strlen(path) + strlen(filename) + 2;
> +	char *finame = malloc(len);
> +	if (!finame){

== NULL.

> Index: desktop/browser.c
> ===================================================================
> --- desktop/browser.c	(revision 8438)
> +++ desktop/browser.c	(working copy)
> @@ -167,6 +168,23 @@
>  	if (url)
>  		browser_window_go(bw, url, referer, history_add);
>  
> +	else {
> +		LOG(("creating blank content"));
> +		struct content *c = content_create(" ");

Failure?

> +		const char *params[] = { 0 };
> +		int length;
> +		const char *blankcontent = "<html><head><title>blank page \
> +		</title></head><body></body></html>";		
> +		length = strlen(blankcontent);
> +		if (content_set_type(c, CONTENT_HTML, "text/html", params) 
> +				&& content_process_data(c, blankcontent, 
> +				length)) {
> +		/* here need to retrieve width, height from browser */
> +			content_convert(c, c->width, c->height);
> +			c->fresh = false;
> +		}
> +	}

I'm not sure I see the point of this, tbh.

> Index: desktop/browser.h
> ===================================================================
> --- desktop/browser.h	(revision 8438)
> +++ desktop/browser.h	(working copy)
> @@ -213,6 +213,7 @@
>  
> 
>  extern struct browser_window *current_redraw_browser;
> +extern struct browser_window *search_current_window;

Why is this here? It's search specific.

> Index: content/fetchcache.c
> ===================================================================
> --- content/fetchcache.c	(revision 8438)
> +++ content/fetchcache.c	(working copy)
> -void fetchcache_error_page(struct content *c, const char *error)
> +void fetchcache_error_page(struct content *c, const char 
> +		*error)

What's with the spurious line break?

>  {
>  	const char *params[] = { 0 };
>  	int length;
> +	char *host;
> +	char checkmessage[24];
>  
> +	if (option_search_url_bar)
> +	/* get the start of the error message for comparison
> +	 * the potential error message we're identifying is derived
> +	 * directly from libcurl, so hopefully no need for I18n */
> +		snprintf(checkmessage, 24, "%s", error);
> +	if (((url_host(c->url, &host) != URL_FUNC_OK) || (strcasecmp(host,
> +			search_web_provider_host())!= 0)) &&
> +			(strcasecmp(checkmessage, "couldn't resolve host '")
> +			== 0)) {
> +		fetchcache_search_redirect(c, error);
> +		return;
> +	}

I really, really dislike this. Performing such string comparisons is
liable to unexpected failure should the error message change.

checkmessage is never initialised if option_search_url_bar is false.

You leak host, too.

> +void fetchcache_search_redirect(struct content *c, const char *error)
> +{
> +	char *redirurl, *temp;
> +	bool can_fetch;
> +	union content_msg_data msg_data;
>  
> +	/* clear http:// plus trailing / from url, it is already escaped */
> +	temp = strdup(c->url + SLEN("http://"));

Failure?

> +	temp[strlen(temp)-1] = '\0';
> +	redirurl = search_web_get_url(temp);

Ditto.

> +	/* logic borrowed from fetchcache_redirect() */

What's preventing you calling fetchcache_redirect()?



J.




More information about the netsurf-dev mailing list