5:14 p.m.
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
9:19 p.m.
New subject: {Disarmed} Re: Review: Mark Benjamin -- gtkmain branch
On Fri, 2009-07-31 at 19:14 +0200, Mark wrote:
John-Mark Bell wrote:
> 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?'
Not in this branch. I just question whether it's necessary to introduce
another hash function.
- 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
Given you're searching a very small space, bsearch() is fine.
> > +/**
> > + * 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 :-)
Well, that can be improved.
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?
There shouldn't be. That's no reason not to document the semantics,
however.
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?
That'll do.
> > + #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;
I remain to be convinced of the need for hashing. It seems
overcomplicated for what is a trivial task.
> 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"'?
Nothing, I hope -- @rel doesn't contain "icon".
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;
Welcome to browser development. Favicons aren't remotely special in
having no sane specification.
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
The correct way to parse @rel is to tokenise it using whitespace as the
token separator. If any token is "icon", then treat it as an icon
reference. This means that "apple-touch-icon" will be ignored (rightly
so, imo).
As for multiple links, take the last one specified.
> 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
url_join performs URL resolution as per the relevant RFC. Therefore,
given an absolute base URL:
http://www.mysite.com/path/to/file/file.html
and a relative URL:
/favicon.ico
it will produce an absolute URL:
http://www.mysite.com/favicon.ico
base_url will contain either the URL of the page, or the base URL
explicitly specified in a <base> element in the page. It is guaranteed
to be absolute.
> > +
> > + 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
Inability to fetch a favicon doesn't strike me as something that should
result in the page itself not being processed. Thus, it makes no sense
to broadcast an error. Remove the msg_data stuff from this function.
> > +/**
> > + * 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
I'm not sure this answers my question.
> > +/**
> > + * 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?
Not necessarily, no.
B wouldn't that be up to the implementation?
If the core expects it to be copied and the implementation doesn't, then
things will break. Thus, the expected semantics must be documented.
For instance as it stands for now, I seem to think riscos implements
a
list of searches, no-one else
That's orthogonal to actually defining what the API should be doing.
> > 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?
Given none of the rest of NetSurf's source code does this, no.
> > + 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 :-)
At the expense of parsing the format string, yes. Leave it as it is.
It's not speed critical.
> > + FILE *f = fopen(fullpath, "w"); /* may need
mode 'b' when c != NULL */
> >
> Always open it as binary, then.
>
are you sure?
Yes. If you're writing binary data, open files in binary mode.
Filesystem work being mildly tricky, release 2.1 is fresh in the
mind :-D
This makes no sense.
> > +{
> > + 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?
"fullpath" would do.
There is me trying to save space, you say NetSurf should be compact,
then I'm hearing sens->sensitive, ( != NULL ) etc
You're confusing source code and output binary. The length of a variable
name makes precisely no difference to the latter but makes a huge
difference to maintainability of the source.
> > +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
It may well do, but it doesn't need a separate function call to do that.
It can equally well do it when saving the base html document.
> > + /* 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
That wasn't clear. Please document this before someone else gets
confused.
> 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;
You should make precisely one assumption about input data: it's probably
corrupt, so you need to ensure you consider that possibility.
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
Yes, but, as I said, you didn't factor in the length of "gtk default
theme\n". Given a 1-byte-long input file, you allocate a buffer of
length 1 byte. Then promptly write 19 bytes to it.
> > +{
> > + 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
I wasn't expecting such optimisations. I was, however, utterly in the
dark about what was going on here. This needs some documentation. Even
better would be to define an enum:
enum image_sets {
IMAGE_SET_MAIN_MENU = 0,
IMAGE_SET_RIGHT_CLICK_MENU,
IMAGE_SET_POPUP_MENU,
IMAGE_SET_<whatever this is>,
IMAGE_SET_COUNT
};
then use it, thus:
struct nsgtk_theme *theme[IMAGE_SET_COUNT];
for (i = IMAGE_SET_MAIN_MENU; i < IMAGE_SET_<whatever this is>; i++)
...
> > + 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
The word "delicate" worries me.
> > + 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
Memory exhaustion is precisely the case that needs most consideration.
> > +/**
> > + * 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
Let me reiterate, in case it wasn't already clear:
You must always check for, and recover from, exceptional circumstances.
There are no exceptions to this rule.
> > +/**
> > + * 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
Please update the comment appropriately, then.
> > + 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
Please use #defines instead of magic numbers. At least that way, you
have an opportunity to make the defined name meaningful. "111" is not.
> > +#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
free(NULL) is defined as a NOP.
However, that's not quite my point. You call a function that can return
NULL on failure.
> > + w = GTK_WIDGET(gtk_tool_button_new(GTK_WIDGET(\
> > + theme->image[p##_BUTTON]),label));\
Then pass the result into another function without checking for this.
At the very least, this needs some commentary explaining why it's safe
to do so.
> > 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
Why are they global? Is there any reason they can't be local variables
in the function(s) that use them?
> > +#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
Right. Macros should operate only on their parameters. Therefore, "ret"
should be a parameter, instead of relying on a variable of the right
type called "ret" to be in scope.
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;
I hope those cases have explicit documentation for why you're not
checking for NULL.
> > +struct nsgtk_scaleview_submenu
*nsgtk_menu_scaleview_submenu(GtkAccelGroup
> > + *group)
> >
> Documentation. Hideous linebreak style.
>
really?
Yes.
well you're the boss, I'd have said it's nicer that way
struct nsgtk_scaleview_submenu *nsgtk_menu_scaleview_submenu(
GtkAccelGroup *group)
> > +{
> > + struct nsgtk_scaleview_submenu *ret = malloc(sizeof(struct
> > + nsgtk_scaleview_submenu));
> >
> Failure? Again, nasty linebreak.
struct nsgtk_scaleview_submenu *ret = malloc(
sizeof(struct nsgtk_scaleview_submenu));
> > 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
In which case, why is it forward-declared here? It should be in a
header.
> > +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
"For now" can easily turn into behaviour that remains unchanged for
years.
> > 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?
Likely.
the reason it is public is that toolbar.c needs access as well as
theme.c
Fine. Can we please lose the magic 3, and use an enum, as described
earlier.
> > 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?
Well, it's the implementation of searching the web, not free text within
a document.
> > +/**
> > + * 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?
Simpler than what?
> > + ret = malloc(len -1);
> >
> Failure? *Minus* 1?
>
quite so; + '\0' - "%s"
Ugh. That was non-obvious.
> > + 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
I really don't like this. It's horribly fragile.
> > 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?
This has nothing to do with optimisation and everything to do with code
clarity and robustness. This implementation is not robust.
Additionally, this impact of changing the search API is pretty minimal
outside search.c
> > + /* 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? :-)
Just because it's been awful for 5 years doesn't mean it has a licence
to remain awful for another 5 (*particularly* if it's being moved to the
core).
> > 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
I suggest using url_to_path, instead.
> > +/** 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?
No. As a global, you have the potential for dangling pointers. There's
precisely no need for this to be global, so it shouldn't be.
> > + gulong signalhandler[2];
> >
> What is this for? Why 2?
>
one for window clicks, one for repaints
Enum, please.
> > +
> > + /* Keep gui_windows in a list for cleanup later */
> > + struct gui_window *next, *prev;
> > +};
> >
> All fields need documenting separately.
>
I simply moved them
So?
> > +/* 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
I'd rather it was removed completely if it's not needed.
> > 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
Not always, you don't.
> > + 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
Doesn't matter. Each function should treat calls to other API as a black
box and act according to the defined semantics of that API. If that
means that a function can return an allocated object, you need to make
sure that it's either stored somewhere for later use or freed.
> > + 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?
I seriously doubt it.
> > 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;
Which is precisely why I suggested using sizeof().
> > + 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
I'm not entirely sure why. It's perfectly possible to open a window or
tab with no content in it. That should have the same effect as
manufacturing a blank page.
> > + 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
You seem to be confusing optimisation with writing code that doesn't
fall over. They're completely orthogonal.
> > + /* logic borrowed from fetchcache_redirect() */
> >
> What's preventing you calling fetchcache_redirect()?
>
there are minor modifications :-)
I suggest, therefore, that you factor out the common parts into a helper
function.
J.
1:16 p.m.
New subject: {Disarmed} Re: Review: Mark Benjamin -- gtkmain branch
John-Mark Bell wrote:
On Fri, 2009-07-31 at 19:14 +0200, Mark wrote:
> John-Mark Bell wrote:
>
The correct way to parse @rel is to tokenise it using whitespace as the
token separator. If any token is "icon", then treat it as an icon
reference. This means that "apple-touch-icon" will be ignored (rightly
so, imo).
I'd say recognise it when it's the only possibility found; the trouble
being that as was said at the time the alternative of looking for a
default location favicon *then* parsing the page for a reference is
wasting time
As for multiple links, take the last one specified.
well now I've simplified the code, a simple scoring system should work
reasonably don't you think?
Yes. If you're writing binary data, open files in binary mode.
well now I've made that code conditional; the comment was in case a bug
ever arose though testing seemed to have raised none
> 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
>
It may well do, but it doesn't need a separate function call to do that.
It can equally well do it when saving the base html document.
for a reason perhaps obscure to non-riscos people, when the
save_complete code was written, the default filetype sent in
save_complete_gui_save is F79, it would seem for components, while the
main page is saved from libxml, needs its type changed afterwards to FAF
That wasn't clear. Please document this before someone else gets
confused.
I already did :-)
you didn't factor in the length of "gtk default
theme\n"
No, honestly, I already did that too :-D
> currently it is although it's a delicate task of making sure
the calls
> are properly ordered; additional condition added
>
The word "delicate" worries me.
try to get more sleep at night :-) as I say I've added a condition so
now it is far less delicate :-D
Let me reiterate, in case it wasn't already clear:
I've already understood, already acted accordingly; no need; thanks
though really :-)
Please update the comment appropriately, then.
I already did
Please use #defines instead of magic numbers. At least that way, you
have an opportunity to make the defined name meaningful. "111" is not.
well apparently Fred Truman found it a very superstitious number :-)
defines added
You call a function that can return NULL on failure.
Then pass the result into another function without checking for this.
At the very least, this needs some commentary explaining why it's safe
to do so.
hmm, given how well documented gtk functions are, I would hope I could
let the reader check that in the manual normally; however comment added
Why are they global? Is there any reason they can't be local
variables
in the function(s) that use them? Macros should operate only on their parameters.
understood, Macros amended accordingly
why is it forward-declared here? It should be in a header.
it looks as though one of the 5 functions was static indeed, keyword added
"For now" can easily turn into behaviour that remains
unchanged for
years.
I really feel as though the search code says nothing much to me; I've
tried to connect as much as possible of it although that is connecting
code stubs; connecting *to* it is a task as I say for whoever really
appreciates much of a purpose to a recent searches list at all;
Well, it's the implementation of searching the web, not free text
within
a document.
quite right; I seem to have picked up a 24 hour bug Friday [luckily I
recovered very quickly] so I may have overlooked that, thanks
Simpler than what?
changed to Enum then :-)
> quite so; + '\0' - "%s"
>
Ugh. That was non-obvious.
I know, good huh? :-D documentation added incidentally
> well the reason search_ico is global rather than being passed is
that it may be cached for asynchronous updates of the front
>
I really don't like this. It's horribly fragile.
Less fragile than it perhaps sounds; rather than needing to tour the
fetchcache code every time it needs to redraw the current search ico,
there is a global reference to it; the idea being that in a perfect
world NetSurf would permanently store search icos, however there is the
matter of copyright so we make the readily accessible cache
>> 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.
>>
Additionally, this impact of changing the search API is pretty minimal
outside search.c
then please tinker with the search.c aspect in the gtkmain branch,
unless you want to merge then tinker; as you say the 5-year-old legacy
from riscos is less neat than it could be, my stamina for rearranging it
is limited; I've made 2 structs for now, 1 global 1 static to give it
some semblance of order, it could be a while until it's really neat
unless you are prepared to sort it; I'd rather it was treated as less
essential as I see the overview of merging the branch as quite important
in the sense of NetSurf progressing steadily, than the detail of
improving the search aspect from its neglected state
>>> + /* 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.
>>
more seriously looking carefully at that code, that is the point of
migrating the functionality to the core isn't it? that the front
essentially provides buttons while the core manages all of that, hence
the front has no responsibility to do all that kind of checking; I've
got a feeling that when you look at the detail of what needs improving
in the search code, it really is more a case of tidying than making
wholesale changes, the kind of tidying that you do so well while me,
well, let's say I have my moments of tidiness :-D
>>> 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
>
I suggest using url_to_path, instead.
I'll look into 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?
>
No. As a global, you have the potential for dangling pointers. There's
precisely no need for this to be global, so it shouldn't be.
I'll look into it
>>> + gulong signalhandler[2];
>>>
>> What is this for? Why 2?
>>
> one for window clicks, one for repaints
>
Enum, please.
sounds involved an enum with 2 values; however consider it done
> I simply moved them
>
So?
so what I would write may not be very accurately what they are supposed
to represent; however I'll look into it :-D
> 2 indices slows it down?
>
I seriously doubt it.
it seems to me that speed as well as elegance are highly valued in such
kinds of functions; I daresay I would have been criticised either way,
so my own judgment would be to stick to how it is [as now improved to
check for NULL]
>>> 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;
>
which is precisely why I suggested using sizeof()
I'll look into it :-)
I'm not entirely sure why. It's perfectly possible to open a
window or
tab with no content in it. That should have the same effect as
manufacturing a blank page.
not in gtk it isn't; so although there's possibly a way of painting a
blank page rather than seeing the beige canvas, the idea at the time had
been to display a blank page rather than looking at an empty beige canvas
You seem to be confusing optimisation with writing code that
doesn't
fall over. They're completely orthogonal.
that search web redirect is perfectly working code, the idea of it
'falling over' seems to have no connection to what I see, albeit the
style is perhaps undesirable for the very long term; you seem to
consider the possibility of libcurl changing its error messages in a few
years' time as causing imminent instability in code that is the best
alternative that I could see, while although several people have chipped
in, no-one else has provided a more seriously workable suggestion; I
hope we shan't be ditching useful functionality simply for the sake of style
I suggest, therefore, that you factor out the common parts into a
helper
function.
I'll be looking into it :-)
J.
Best regards
Mark
http://www.halloit.com
Key ID 046B65CF
2:56 p.m.
New subject: {Disarmed} Re: Review: Mark Benjamin -- gtkmain branch
On Mon, 2009-08-03 at 15:16 +0200, Mark wrote:
John-Mark Bell wrote:
> On Fri, 2009-07-31 at 19:14 +0200, Mark wrote:
>
> > John-Mark Bell wrote:
> >
> The correct way to parse @rel is to tokenise it using whitespace as the
> token separator. If any token is "icon", then treat it as an icon
> reference. This means that "apple-touch-icon" will be ignored (rightly
> so, imo).
>
I'd say recognise it when it's the only possibility found; the trouble
being that as was said at the time the alternative of looking for a
default location favicon *then* parsing the page for a reference is
wasting time
It is not remotely clear to me that there's any benefit in considering
link relationships that will not be used by other browsers, particularly
when those icons are least likely of all to be suitable for use.
> As for multiple links, take the last one specified.
>
well now I've simplified the code, a simple scoring system should work
reasonably don't you think?
Not really, no.
> Yes. If you're writing binary data, open files in binary
mode.
>
well now I've made that code conditional; the comment was in case a
bug ever arose though testing seemed to have raised none
So, you're certain that when there's no content pointer, the data to be
written is textual? If not, it will break on Windows (at least).
> > 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
> >
> It may well do, but it doesn't need a separate function call to do that.
> It can equally well do it when saving the base html document.
>
for a reason perhaps obscure to non-riscos people, when the
save_complete code was written, the default filetype sent in
save_complete_gui_save is F79, it would seem for components, while the
main page is saved from libxml, needs its type changed afterwards to
FAF
That doesn't address the issue.
1:
bool save_complete_gui_save(const char *path, const char *filename,
struct content *c, int len, char *sourcedata, int type);
should be
bool save_complete_gui_save(const char *path, const char *filename,
struct content *c, int len, char *sourcedata);
Which leaves it up to the frontend to do the right thing about
filetyping by extracting the content type from the content.
Why is sourcedata not const, incidentally?
2:
bool save_complete_gui_filetype(const char *path, const char *filename,
int type);
should not exist at all.
For non-HTML contents used by a page, save_complete_gui_save will be
called, so the frontend can determine the file type from the content's
type.
For HTML contents, save_complete_htmlSaveFileFormat() will be called, so
the frontend knows what file type to use (HTML).
hmm, given how well documented gtk functions are, I would hope I
could
let the reader check that in the manual normally; however comment added
It's better not to force the reader to consult the manual.
> "For now" can easily turn into behaviour that remains
unchanged for
> years.
>
I really feel as though the search code says nothing much to me; I've
tried to connect as much as possible of it although that is connecting
code stubs; connecting *to* it is a task as I say for whoever really
appreciates much of a purpose to a recent searches list at all;
I'm not entirely sure what you're trying to say here -- can you clarify?
> > well the reason search_ico is global rather than being
passed is that
> > it may be cached for asynchronous updates of the front
> >
> I really don't like this. It's horribly fragile.
>
Less fragile than it perhaps sounds; rather than needing to tour the
fetchcache code every time it needs to redraw the current search ico,
there is a global reference to it; the idea being that in a perfect
world NetSurf would permanently store search icos, however there is
the matter of copyright so we make the readily accessible cache
If things really have to be global, then make them static and provide
accessors.
That said, in this particular case, you're updating search_ico, then
calling some frontend code with the expectation that it then retrieves
the thing to handle from the global you just set.
I really don't like this implicit communication between components. Far
better to pass search_ico as a parameter.
> > > 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.
> > >
> Additionally, this impact of changing the search API is pretty minimal
> outside search.c
>
then please tinker with the search.c aspect in the gtkmain branch,
unless you want to merge then tinker; as you say the 5-year-old legacy
from riscos is less neat than it could be, my stamina for rearranging
it is limited; I've made 2 structs for now, 1 global 1 static to give
it some semblance of order, it could be a while until it's really neat
unless you are prepared to sort it; I'd rather it was treated as less
essential as I see the overview of merging the branch as quite
important in the sense of NetSurf progressing steadily, than the
detail of improving the search aspect from its neglected state
My comments had nothing to do with neatness and everything to do with
code stability. There are known cases where this code causes crashes
that are the direct result of global pointers becoming stale. Therefore,
this code is not appropriate for merging into the core in its present
state.
> > > > + /* 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.
> > >
more seriously looking carefully at that code, that is the point of
migrating the functionality to the core isn't it? that the front
essentially provides buttons while the core manages all of that, hence
the front has no responsibility to do all that kind of checking; I've
got a feeling that when you look at the detail of what needs improving
in the search code, it really is more a case of tidying than making
wholesale changes, the kind of tidying that you do so well while me,
well, let's say I have my moments of tidiness :-D
Not really. The frontend knows precisely whether a new search is to
begin -- the user clicked the search button (as opposed to
next/previous). Passing that information as a boolean parameter to the
core reduces the above mess to:
if (new_search) {
...
> > 2 indices slows it down?
> >
> I seriously doubt it.
>
it seems to me that speed as well as elegance are highly valued in
such kinds of functions; I daresay I would have been criticised either
way, so my own judgment would be to stick to how it is [as now
improved to check for NULL]
I don't think so. As it's currently written, it's not actually clear
what is going on.
> I'm not entirely sure why. It's perfectly possible to
open a window or
> tab with no content in it. That should have the same effect as
> manufacturing a blank page.
>
not in gtk it isn't; so although there's possibly a way of painting a
blank page rather than seeing the beige canvas, the idea at the time
had been to display a blank page rather than looking at an empty beige
canvas
Right. The GTK frontend should clear the canvas to white when redrawing
a window with no content. There should not be any need to make the core
generate a blank page, instead.
> You seem to be confusing optimisation with writing code that
doesn't
> fall over. They're completely orthogonal.
>
that search web redirect is perfectly working code, the idea of it
'falling over' seems to have no connection to what I see, albeit the
style is perhaps undesirable for the very long term; you seem to
consider the possibility of libcurl changing its error messages in a
few years' time as causing imminent instability in code that is the
best alternative that I could see, while although several people have
chipped in, no-one else has provided a more seriously workable
suggestion; I hope we shan't be ditching useful functionality simply
for the sake of style
1. ISTR that when this feature was initially added, nobody actually
thought it was a good thing, with most people preferring search prefixes
in the URL bar.
2. If you really must persist with this approach, then the correct thing
to do is to consult the error code returned from the fetch layer. As it
currently doesn't return anything other than an error message, then
you'll need to change the fetch callback API.
Comparing error messages with known strings is never the right solution.
J.
1:39 p.m.
New subject: {Disarmed} Re: Review: Mark Benjamin -- gtkmain branch
On Mon, 2009-08-03 at 15:56 +0100, John-Mark Bell wrote:
On Mon, 2009-08-03 at 15:16 +0200, Mark wrote:
> John-Mark Bell wrote:
> > "For now" can easily turn into behaviour that
remains unchanged for
> > years.
> >
> I really feel as though the search code says nothing much to me; I've
> tried to connect as much as possible of it although that is connecting
> code stubs; connecting *to* it is a task as I say for whoever really
> appreciates much of a purpose to a recent searches list at all;
I'm not entirely sure what you're trying to say here -- can you clarify?
I've not seen any clarification of this, so I'm still confused :)
> > > > 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.
> > > >
> > Additionally, this impact of changing the search API is pretty minimal
> > outside search.c
> >
> then please tinker with the search.c aspect in the gtkmain branch,
> unless you want to merge then tinker; as you say the 5-year-old legacy
> from riscos is less neat than it could be, my stamina for rearranging
> it is limited; I've made 2 structs for now, 1 global 1 static to give
> it some semblance of order, it could be a while until it's really neat
> unless you are prepared to sort it; I'd rather it was treated as less
> essential as I see the overview of merging the branch as quite
> important in the sense of NetSurf progressing steadily, than the
> detail of improving the search aspect from its neglected state
My comments had nothing to do with neatness and everything to do with
code stability. There are known cases where this code causes crashes
that are the direct result of global pointers becoming stale. Therefore,
this code is not appropriate for merging into the core in its present
state.
I still have grave concerns about the current state of the free text
search, even though it's a little better than before.
I would much prefer to see something like this:
struct search_ctx;
typedef enum {
SEARCH_FLAG_NEW_SEARCH = (1 << 0),
SEARCH_FLAG_CASE_SENSITIVE = (1 << 1),
SEARCH_FLAG_FORWARDS = (1 << 2)
} search_flags_t;
struct search_ctx *search_create_context(struct browser_window *bw);
void search_destroy_context(struct search_ctx *ctx);
void search_text(struct search_ctx *ctx, const char *string,
search_flags_t flags);
Then, have search_ctx defined (privately) as:
struct search_ctx {
struct browser_window *bw; /**< Owning window */
/**
* Cursor for current active search
*/
struct {
unsigned start_idx; /**< Start index of match */
unsigned end_idx; /**< End index of match */
/* These two are for HTML contents only */
struct box *start_box; /**< First box for match */
struct box *end_box; /**< Last box for match */
struct selection *sel; /**< Selection object */
} cursor;
};
The implementation of search would have to change so that it no longer
creates a list of matches upfront, but actually performs the search in
the given direction on the live content data structure.
Each browser_window would have its own search_ctx object, thus removing
the ballache of current_search_window.
Other than the above, and assuming the nits in the other emails I've
sent get fixed, then this branch is ready for merge.
J.
1:50 p.m.
New subject: {Disarmed} Re: Review: Mark Benjamin -- gtkmain branch
John-Mark Bell wrote:
>> I really feel as though the search code says nothing much to
me; I've
>> tried to connect as much as possible of it although that is connecting
>> code stubs; connecting *to* it is a task as I say for whoever really
>> appreciates much of a purpose to a recent searches list at all;
>>
> I'm not entirely sure what you're trying to say here -- can you clarify?
>
I've not seen any clarification of this, so I'm still confused :)
I think you answered the question :-)
the ballache
I was trying to say the search code was so messy I was wary of getting
to grips with it in depth; however your very clear description of how to
improve it gives me more inspiration, I'll be improving the search code now
Other than the above, and assuming the nits in the other emails
I've
sent get fixed, then this branch is ready for merge.
all fixed in r9194 as far as I know; the spurious printf had already
been cleared
J.
Best
Mark
http://www.halloit.com
Key ID 046B65CF
4294
days inactive
4305
days old
netsurf-dev@netsurf-browser.org
5 comments
2 participants
participants (2)
-
John-Mark Bell -
Mark