John-Mark Bell wrote:
Hi,
first of all let me say thanks for taking the time to review the diff;
judging from the amount of time it has taken me to implement all the
little changes, that must have been not inconsiderable
> 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?
as I seem to recall the discussion at the time, there were people
encouraging me to hash the strings here; perhaps a better question would
be 'is it time to make one global hash function to replace the static
hash functions dotted around?' - aside from that, I suppose a show of
hands as to whether there are really that many people who think hashing
here is worthwhile after all? I may have to teach myself profiling as
James suggested
> +/**
> + * 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?
the documentation for url_join() is unclear :-)
more seriously, though, is there ever a case that a call to a function
returning char * doesn't need the caller to free() it when non-NULL?
Occasionally the function specifically says it accepts a param of that
buffer that must have space allocation already, so that it is clear that
the caller has to allocate as well as free; however I have added 'caller
owns returned pointer' is that right?
> +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 0x0b88848
> +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?
well as James suggested an LUT I've implemented an LUT + hashes for now;
as I say there were people suggesting hashes at the time;
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.
what do we do then when a page says 'link rel="myfavrel"
type="application/ico" href="favicon.ico"'? The trouble with
favicons is
that it is one area where standards took time catching up with practice,
so that standards are breached more often than observed; then there is
the case of multiple link rels, usually all 3 kinds, you then need to
try to pick the best as [aside from apple-touch] the judgement of the
page writer may be entirely arbitrary, such is the very large variance
in respect for standards
> + 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);
I had the impression that for an url such as
http://www.mysite.com/path/to/file/file.html then base_url would be
http://www.mysite.com/path/to/file ? I need
http://www.mysite.com/favicon.ico
> +/**
> + * 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;
understood
> +
> + 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?
Your call; although I understand the fetch system a bit better now, I
seem to recall clearing the call to content_broadcast from the place I
borrowed the logic, as it was inappropriate here; subsequent to James'
suggestions back in May
> 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?
It was moved to favicon.c to replace the #defines in r8717-8718
> +
> +bool favicon_get_icon(struct content *c, xmlNode *html);
>
You need to #include <libxml/tree.h> and forward declare struct content
in this file.
understood
> 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.
done; need to double-check beos/framebuffer/amiga compile
> +/**
> + * 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?
really I tried to maintain the integrity of the search implementation in
riscos/search.c during its migration to the core; hence rather than
disrupting functions, I migrate the functions as they are to the core as
much as possible; as the function call was present in the riscos
implementation, I suppose as it may take a while for riscos to look for
matches, the function call has moved to the core; besides, rather than
displaying a pointer hourglass, a front may want to display a toolbar
hourglass etc; moreover wasn't there a suggestion to make a core widget
for search bar? Then perhaps tinkering with the actual search functions
could wait
> +/**
> + * 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?
comment amended
> +{
> +}
>
This is missing a return value.
amended
> +/**
> + * 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?
A presumably else it wouldn't be const, no? B wouldn't that be up to
the
implementation? For instance as it stands for now, I seem to think
riscos implements a list of searches, no-one else
> +/**
> + * 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.
blame riscos :-) amended
> +/**
> + * 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.
all sorted
> 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.
doxygen comments are in desktop/save_complete.h, is that not correct?
> +{
> + int res;
> + int namelen;
> + namelen = strlen(path) + strlen(filename) + 2;
>
These can be merged into a single statement. What is the "+ 2" for?
'/', '\0'; comment added
> + 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).
save that I need to add '/', '\0' so snprintf does all of that
nicely;
hence its apparent popularity among NetSurf devs :-)
> + FILE *f = fopen(fullpath, "w"); /* may need mode
'b' when c != NULL */
>
Always open it as binary, then.
are you sure? Filesystem work being mildly tricky, release 2.1 is fresh
in the mind :-D
> +int save_complete_htmlSaveFileFormat(const char *path, const
char *filename,
> + xmlDocPtr cur, const char *encoding, int format)
>
Needs doxygen.
see desktop/save_complete.h, I hope that doxygen itself makes the
necessary reference
> +{
> + 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.
fullpathfilename? There is me trying to save space, you say NetSurf
should be compact, then I'm hearing sens->sensitive, ( != NULL ) etc
> + if (!finame){
> + warn_user("NoMemory", 0);
> + return -1;
> + }
> + snprintf(finame, len, "%s/%s", path, filename);
>
Again, you know the buffer's big enough.
again snprintf does it better :-D
> +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().
save_complete.h; as I recall, riscos sets the filetype *after* saving
for the base html document, so needs an additional call to set the filetype
> 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.
done
> +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.
as g_strconcat only ever breaks the program in low memory; amended to
malloc/snprintf
> + 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.
I try to put the declarations together; even though we're freeer in gtk
than in the core, it seems good practice; however, amended :-)
> + while (fgets(buf, sizeof(buf), fp)) {
>
!= NULL
> +void nsgtk_theme_add(const char *themename)
>
Documentation.
Mmm :-)
> +{
> + 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.
as it's being passed as const, I hope not :-) some alterations to
container_* made accordingly :-D
> +}
> +
> +bool nsgtk_theme_verify(const char *themename)
>
Documentation.
so it would seem :-D
> +{
> + 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.
there are at this point [pre-replacement] 2 trailing NULs; I had had to
clear the trailing \n to compare names
> + strcat(filecontent, buf);
>
And then used strcat(), which expects a NUL-terminated string.
It gets precisely what it asks for :-)
> + }
> + }
> + }
>
The above will overrun the filecontent block. The length of "gtk default
theme\n" is not factored into the allocation request.
the length of "gtk default theme\n" in every normal case is already
in
filelength modulo deliberate corruption of the themelist file that is of
course possible; however as it does no harm I've added more space then;
aside from that the length of filecontent must be <= the length of the
file as every entry in filecontent is read from the file
> + 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?
as a result of a need to maintain integrity of object references it is
necessary to load 3 sets of gtk images then set 1 set for the main menu
images, 1 set for the right click menu [visible when the main menu is
hidden], 1 set for the popup menu [visible 'at all times' subject to
hiding items; it *may* be possible to manage that with g_object_ref() /
unref() though honestly I would have hoped to save more delicate
'optimisation' improvements until the main branch is stably merged
> + 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
currently it is
although it's a delicate task of making sure the calls
are properly ordered; additional condition added
> +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?
static globals for cache
> + nsgtk_theme_prepare();
>
What if this fails? Can it fail?
It shouldn't as there is a test for current_theme_name == NULL that
should call nsgtk_theme_default(); however I've added a condition for
low memory
> +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.
I rather think that's stylish; kind of thinking of c++ :-D
> +/**
> + * 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?
is not a possibility :-)
however, in case of massive system-wide memory depletion, I'll add a
check :-D
> + 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?
that the physically visible toolbar buttons are made to correspond to
the logically stored toolbar buttons in terms of location, visibility, etc
> + */
> +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.
Haha well the alternative is a thorn in your eye :-)
more detail added to the comment
> +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
right you are
> + window->currentbar = gtk_toolbar_new();
>
Failure?
> + gtk_widget_set_size_request(widget, 111, 70);
>
These numbers look magical.
they are kind of, aren't they brilliant? :-)
they seem to fit the 'normal' width of a label so that sufficient of it
is recognisable, without setting all the widths to the width of the
widest etc; height is 'normal' wrt the height of the images
> +/**
> + * 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?
it could return NULL as I've now changed it; so a test before free()ing
is in order
> + w = GTK_WIDGET(gtk_tool_button_new(GTK_WIDGET(\
> + theme->image[p##_BUTTON]),label));\
>
Failure?
NULL
> + 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?
to accept parsed key/modifier values
> +#define IMAGE_ITEM(p, q, r)\
> + ret->q##_menuitem = GTK_IMAGE_MENU_ITEM(\
>
Where has ret come from?
it's defined in the individual functions
> + 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.
Hence the macros :-)
generally I have now added checks for no memory, as well as NULL returns
from gtk calls; some parameters to gtk calls may be sent as NULL in the
remaining cases without harm as far as I know; documentation added all
round; you could have simply said that there are many dittos in menu.c :-D
> +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.
really? well you're the boss, I'd have said it's nicer that way
> +{
> + 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.
You do that :-)
> + 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?
I suppose not as the button needs connecting to it
> +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.
No really :-D
it's accessible to the gtk area for the reasons I say
> +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.
documentation in the desktop header file
> +char *gui_search_get_string(void)
>
Ditto.
> +void gui_search_add_recent(const char *string)
>
Ditto.
> +{
> + recent_search[0] = strdup(string);
>
Failure?
well as it's a very bare implementation for now, NULL is acceptable
> +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?
No, perhaps you're thinking of nsgtk_theme_cache that caches a different
kind of image?
the reason it is public is that toolbar.c needs access as well as theme.c
> 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?
cleared
> 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.
what do you want me to say?
> +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?
as it's simpler?
> + 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.
quite right; I remember at the time thinking whether url_escape needed
free()ing afterwards, in hindsight it's very
clear :-)
> +
> +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?
quite so; + '\0' - "%s"
> +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.
well the reason search_ico is global rather than being passed is that it
may be cached for asynchronous updates of the front
> 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.
definitely; as I say I'd rather avoid breaking what worked until it's
time for optimization; besides the review is sufficiently monolithic
already isn't it?
> +/**
> + * 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?
it could; I'm avoiding tampering with it though until it's time for
optimization
> + gui_search_add_recent(string);
>
What's this for?
legacy from riscos; could be optimized
> +/**
> + * 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.
ask the bloke who wrote the riscos implementation, what was his name? :-)
> + 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.
save_complete_inventory needs it
> +/** 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.
optimization? Honestly I'd rather that the original writer of the search
code modified it as it looks kind of delicate to me
> 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?
as far as I know they do; various aspects of the gtk front need access
to them
> +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?
one for window clicks, one for repaints
> +
> + /* Keep gui_windows in a list for cleanup later */
> + struct gui_window *next, *prev;
> +};
>
All fields need documenting separately.
I simply moved them
> struct gui_window *window_list = 0; /**< first entry in win
list*/
>
NULL.
ditto
> 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?
it looks as though it is code that I had to comment when I changed the
logic so that it was reached at all [the gtk bug]; in view of the core
widgets business I left it there as an indicator for now; comment added
> 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.
it's a bool
> 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?
const; however as I reassign url as the result of a function it should
be good
> + 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.
possibly although search_is_url() is very partial in its implementation;
modified
> 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?
adds to it from my reading of the documentation; needs the gtk at the
front of the string as I understand 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?
modified
> + 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?
2 indices slows it down?
> 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()
it all looks a bit delicate to me; I'd rather the original writer
optimized it when he has time
> 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.
It does look that way :-)
> +#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.
well it would be nice once it is working properly
> 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.
well it's a browser_window :-) you're the boss
> 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?
it must have got in incognito :-D
> {
> 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.
however unless there's a better suggestion at least it works for now;
one more for the optimization queue I'd say
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()?
there are minor modifications :-)
J.
Once more thanks for making time I really understand how much of a
monolithic task it it I hope my occasional mildly cheeky comments are
taken in good spirit :-D
Best regards
Mark
http://www.halloit.com
Key ID 046B65CF