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.
