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

Hi, Le 10 mai 2011 à 14:32, Vincent Sanders a écrit : I'll keep the branch around as reference and rewrite it on the -v2 one, but here is what I have to say. I also have tried to explain the reasons why I went this route, I'll summarize them again below... Besides, I started it as a joke to see if it would work, so I originally just wanted something that work before getting things cleaned up. I actually started with the fetcher way inside the curl fetcher, and it almost worked. However the data chunks sent from the server did not always end on a line boundary, so the simple parser missed entire items. Refusing part of the data by indicating less bytes consumed than presented also resulted in the fetch being aborted (would actually be handy if it would just keep the unconsumed data for the next call). Fixing it would have meant adding caching in the fetcher itself. I looked at how llcache could be used for this, but I couldn't find any way to use it without presenting data that would end up in a content object. So I tried the content handler route, and was directed at the old plaintext renderer which used the same trick as the current code. Being able to get all the data at once simplifies the parser as well, though of course it is not as fast probably. While it might not follow the current design goal, it does the job, which was the idea for a first version. Also, concerning the fetcher vs handler way, I think putting things in the fetcher is as bad, if not incorrect semantically. Putting something that massages *content* in a *fetcher* seems wrong. While I agree it makes some sense for file:, gopher data has a standardized format which makes up a content that needs to be *handled*. Having to dig the HTML handler function and masquerade oneself to it is just a result of not being able to create content objects ex nihilo from arbitrary data instead of llcache object tied to a url. Besides, "fetching" html content from gopher directories has side effects like, one won't be able to view the source data for example. Not very important but still... No I just happened to have pop up at the wrong time and got a bit irritated. I understand the effort and will try to follow the same route for the v2 branch, but I still think it's not totally logic either to put gopher htmlization in the "fetcher". At least for gopher I didn't find any bug in curl. The fact that the data comes in in chunks is likely caused by the server doing so. The only thing I thought was a bug in curl was actually a bug in the server at floodgap.com that has been fixed since then. While curl could have tried to map gopher errors to HTTP codes, they don't do it probably because it's just an heuristic of checking if the 1st char is '3' (error item type), which is not really telling much. Is there anything I could use to handle the caching of partial item lines or will I have to write it myself btw ? Don't have much experience with cURL, so I can't tell. I actually didn't check how proxies should be used for gopher (not gopher-to-http proxies), at least some browser provide a setting for it, but gopher doesn't have any provision for vhosts... http://source.netsurf-browser.org/branches/dsilvers/netsurf-remove-curl/ ? I should probably have a look then... If possible it should have a lower level API for doing socket I/O to reuse in non-http things. Meanwhile I'll try to isolate the curl dependency in my code. Again this goes beyond the job of a *fetcher*, but anyway. I already added things to internal.css for gopher. cf. above. I originally forked textplain.c as a template, though indeed I didn't keep much of it, and also reused stuff from dirlist.c. That is because I was hoping some cleaner way would be introduced, like a content delegate object of some sort. As I said it's the result of not being able to create a content without an llcache backing object, which would have been cleaner. I originally forked textplain.c ... Ok... But they are not magic though, the protocol explicitely uses letters. Will use an enum next time. Right, pure laziness, I admit. Didn't find one at first sight, I just stripped things from gen_nice_title to get it done. No, this one comes from dirlist.c, and gave birth to the one above. I agree it should probably be put to utils.c I originally forked textplain.c ... but yeah. What ? Failed attempts. Actually I did, forgot to remove the TODO. Hmm ok... Don't think so. It's visible on: http://revolf.free.fr/beos/shots/shot_netsurf_gopher_vs_fx2_001.png lines like "(plus using..." and "Get your own..." are shifted on the right by a parasite whitespace. I don't have much knowledge of the html renderer to fix that. until someone fixes it :p I can do much worse :P When I said I could do much worse :D Some research queries return gopher directories with a lot of images, which tend to stale the GUI while fetching them if we enable inlining them (with <img>). Some clients like the OverbiteFF Firefox plugin inlines them inside an iframe that is only shown (and fetched) when clicked, but we can't do this yet. An alternative would be to only inline the first N images... It kinda grew organically... No it's an undecided part, it seems we don't really need it but part of the review was to answer this. Rather, to the OS vender IIRC, but yeah. netsurf/render/textplain.h should be fixed then... "... not being able to create a content object without llcache object ..." You tell me, that's why the review is for. I don't think it has any use outside gopher, but it was more related to url handling, so... Maybe, but as I said, doing this for gopher is dangerous at least in the current state. Then why is it named *curl.c* ? ;-) Right, by ext, though I'm not sure every platform code actually likes being called with non-existing files, it isn't obvious from the documentation. For example, the BeOS code tries to open the file to read its MIME type attribute. I added some hardcoded types for known extensions, before noticing the BMimeType::GuessMimeType() methods that take a filename and data buffer. Maybe we should have one for real files and one that only checks extensions ? OTH we could just assume the file might not exist and fallback to extension matching for those. Dead code leftover. I like numbers :p François.