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.
