Re: Review: Merge branch mmu_man/netsurf-gopher-support-v2

I shall do a brief content review because you did take the time to make a patch. I have explained *at length* why I disagree with your approach. I shall briefly outline those arguemnts once more. I have already stated I do not wish to discuss this futher as you simply will not accept my arguments, this review/summary is provided as a pure courtesy. We have made extensive efforts to streamline the data flow through the browser in the recent past, starting with jmb implementing the low level cache, us making the fetcher interface less troublesome and culminating in the ongoing content handler refactor. I say this as you seem to think this ongoing work is directed at you personally soemhow. Curl ---- We have come to understand that the curl library simply should not be relied upon beyond its http/https implementations. We have implemented fetchers directly for things like file: explicitly. Because the curl libraries in actual common use have numerous bugs and unwanted behaviours (an example would be curl applies proxy settings to file: urls) and the huge memory footprint of curl *in our use case* it looks increasingly like a poor fit. Because of this we have gone to great efforts to *remove* fetcher support from the curl library. As discussed at the develoepr meetings previously Daniel Silverstone is working on creating an http library to eventually completely remove our dependancy on curl. Content ------- As mentioned to you, The way we have chosen for things that generate output (primarily directory listings but the about: handler also) that the browser should display as HTML is that the *fetcher* layer returns the data as generated HTML. We have created helper functions for this generation for directory listings. If the formatted output was not to your liking these content generation routines should have been extended and the formatitng applied using the internal.css. The method of inventing a content type and content handler which then rummages around inside a different content handler is full of problems. Indeed we have worked to completely remove this method of operation and built a data model where it is not supported. Review ------ Will be inline with code thats ok elided, as usual. odd you credit james and adrian in a new file? right there is why you have got no other reviews by the way, not just because it *is* a hack but because you knew it and did nothing about it, or at least provided a rational explanation. as a comment we are trying to move away from forward declaration, secondly every function should be doccommented somewhere. if its static, in its scope here. If its external a comment saying in which header it is docuemnted. arghhhh magic letters! no! an enum if you please yes i know the changes were only applied recently, but named initialisors pls there are six parameters here... BLETCH is there not already a utility function to do this, and if not create one rather than spooging this in and really this ought to be less iffy. didnt you just have this code above? magic letters again docuemnt this in the caller, not in every function yuk well why have you not? whats this magic number? not an enum so fileds[] has meaning? what bug? why isnt it fixed? yuk way too much magic in one printf. ditto and again handle it or a better comment seriously? what? someome else might thave to read this stuff, please, less and more readable ditto dangerous? explain! complex SLEN() overall this function was too long, too complex, too many "magic" value magic value if type is not what? our idom is if (type == NULL) no c++ comemnts please is that supposed to be a todo? magic numbers and please can you try and make variable names meaningful, it is my personal pet peeve that we are writing C not fortran but good variable names are important. not strndup? it has recently been pointe dout to us that macros beginning _ belong to the compiler so we ought not to use them plain NETSURF_RENDER_GOPHER_H would be ok docuemntation? very rude word is this useful in the generic url handling? would this not have been better as an option to extend the generic directory listing to show images inline? various eww in teh curl handler...it really is supposed to be only for http(s) err we already have a way of doing this for file: perhaps you can use that ummm? this is C not C++, apropriate comment syntax please and if its not useful *remove* it magic *again* blergh hmm do we not have an enum for there? bad project, no biccuit Even leaving aside the structural issues, this needs a *lot* of work before its merged. -- Regards Vincent