May 2008 - Dillo-dev - lists.mailman3.com

[patch]: simplify a_Html_form_event_handler()
by onepoint＠starurchin.org 30 May '08

30 May '08

The a_Html_form_event_handler() function displays an amusing anti-pattern (which I wish I knew the name of) where it's not given enough data and has to go searching for the missing information. Specifically, it gets a pointer to the DilloHtml of the page, but not not the DilloHtmlForm that submitted it, so it has to search the form list of the DilloHtml to find the form. This patch passes the DilloHtmlForm to the handler and adds a pointer from the DilloHtmlForm to the DilloHtml , so the handler can now directly access the form and the html without messing around. Regards, Jeremy Henty

2 3

[patch]: convert Html_submit_form2() to DilloHtmlForm::buildQueryUrl()
by onepoint＠starurchin.org 30 May '08

30 May '08

The function Html_submit_form2() does two jobs rolled into one: most of its code creates a url from a DilloHtmlForm and a few lines push the url to a DilloHtml . This patch takes advantage of the fact that Html_submit_form2() is only called from a_Html_form_event_handler() ; it moves the url push out into the caller and the rest of the code into a new method "DilloUrl *DilloHtmlForm::buildQueryUrl()". This separates concerns nicely: the DilloHtmlForm creates the url and a_Html_form_event_handler() pushes it to the DilloHtml . We're getting close to having a proper API for DilloHtmlForm , unfortunately some of the remaining code is very messy. I'll do the rest of the easy stuff and then summarise what's left. Regards, Jeremy Henty

2 1

Previous patches a la camelCase
by onepoint＠starurchin.org 30 May '08

30 May '08

In case Jorge prefers them, here are revised versions of today's patches with camelCased method names. Regards, Jeremy Henty

2 1

[patch]: html.cc: change Html_add_input() to HtmlForm::add_input()
by onepoint＠starurchin.org 30 May '08

30 May '08

This changes the Html_add_input() function to an HtmlForm::add_input() method. As before I'm submitting this as a "change diff" + "move diff" to make it easier to review. Reviewers please tell me if it makes things harder instead! Regards, Jeremy Henty

2 1

[patch]: html.cc: change Html_make_multipart_boundary() to DilloHtmlForm::make_multipart_boundary()
by onepoint＠starurchin.org 28 May '08

28 May '08

More of the same. I'll hold off sending any more patches until these three have been reviewed. (Especially since I just remembered Jorge will probably want to camelCase the method names. D'oh!) Regards, Jeremy Henty

1 0

[patch]: html.cc: rename Html_build_query_data() to DilloHtmlForm::build_query_data()
by onepoint＠starurchin.org 28 May '08

28 May '08

Does what it says on the tin! There's no "move diff" for this one due to declaration order dependencies. I expect the move will happen after the functions that access DilloHtmlInput internals are converted to DilloHtmlInput methods. Regards, Jeremy Henty

1 0

html.cc: convert Html_*_reset functions to methods
by onepoint＠starurchin.org 28 May '08

28 May '08

This changes Html_reset_form() to DilloHtmlForm::reset() and Html_reset_input() to DilloHtmlInput::reset() . It's part of the plan to move code that accesses DilloHtmlForm and DilloHtmlInput internals into methods and make the internals private. The patch is in two parts: the first makes the changes and the second moves the new methods to the right place in the file. If I roll the changes and the move into one then "diff" gets confused and creates an unreadable monster of a patch. I'd recommend feeding the two attachments to patch separately; it might do the right thing given the whole message in one go, but then again it might not. Comments are welcome. There are several other functions in html.cc that are really just methods in disguise and are future targets for the same treatment as Html_reset_form() and Html_reset_input() get here. Regards, Jeremy Henty

2 2

html.cc: small tweak for consistency and debugging
by onepoint＠starurchin.org 27 May '08

27 May '08

This renames a local variable from "size" to "nf". This is consistent with the names of similar local variables elsewhere in the file and also ensures that the _MSG statement will compile if debugging is enabled. Jeremy Henty

1 0

Re: (working) prototype of charset decoding in cache.c
by jcid＠dillo.org 27 May '08

27 May '08

Hi, On Fri, May 02, 2008 at 08:02:14PM +0000, corvid wrote: > I've been playing around with this now and then over the past few months > when I haven't had anything else going on. I've been running on it for > a day or so without incident, but I haven't gone through it with a > fine-toothed comb because I know there will be changes. > > It's 35K, so I realize I'm in little danger of hearing back too quickly :) > Besides, I know you said you have some nbsp thing that needs attention... I've made a second review over the code, and it doesn't look that bad to me. This is, the decoding in cache, even the stuffed cache-entry; if it can be isolated/tidied in the future better. The part that looks inconvenient to me is the charset choosing dialog. I'd prefer Dillo do this automatically. There's no need to try to recognize every charset in the world, but just a few "most used" ones like koi8-r. BTW, UTF-8 has become the preferred solution. The idea is that we can leave space for a expandable function just in case. BTW, there's an improved version of the data guesser in the ftp dpi. Please send me a koi8-r encoded page to test (or just let me know how to find it with google :). IIRC you sent me an explanation message on what this patch was for and remaining issues. I can't find it :(. Please resend it to me. > > There are plenty of issues. Going in order through the diff: > > - *_get_content_type(). I searched for some Russian text files where a > charset is given in the HTTP header. None of them were regarded as > text by the old test for ascii chars. > Maybe the way to go is to have tests for more filetypes and treat > unknown as text. Checking for all possible charsets would be no fun, > anyway. (Tests for more filetypes might be needed for file uploading, > as well.) The problem with "unknown as text" is that some HTTP servers send downloadable files as text. It is quite practical in this case to get a download dialog from dillo, intead of a binary rendering (especially for newbies). That's why the current data guesser is conservative. The data guesser can be improved. e.g. taking ASCCI control characters as a strong indication of a binary, and their absence as a text hint. I can give it a look after you send me the files. > - CacheEntry_t now has a horrifying amount of junk in it. Maybe it > would be good to pull out about half of it into some sort of Data thing. It's stuffed but still understandable. If it can be tidied in a second pass, better. > o TypeSet to be set from Meta or a user dialog. Meta can set it once; > the dialog can set it repeatedly. I'd like to avoid that dialog if possible. > o UTF8Data (LocalData?) that's been translated. cache.c has a lot of > "entry->UTF8Data ? entry->UTF8Data : entry->Data" in it now that > perhaps should be hidden behind something. Yes. > o DataReaders reference count. UTF8Data is destroyed when not needed. > o CharsetDecoder > > - a_Cache_unref_buf, a_Capi_unref_buf, a_Nav_unref_buf all just to > provide notification that the view source window isn't reading it anymore. > Ugh. Not pretty, but tolerable. I'm thinking along the line of getting the core of the patch committed and then polish the rough edges. > - a_Cache_[gs]et_content_type and capi and nav. Ugh again. > I don't know whether there's a timing issue with changing decoders > due to a meta tag at the same time that a view source window is being > opened. Yes there is. Simple to solve by allowing view source after parsing the HEAD element only. > (Why content-type and not charset? I find switching to text/plain to > use findtext on the source to be useful.) Sorry, I don't understand the question. > - Decode_charset() no longer free()s its input. This inconsistency would > be an excellent source of bugs, so it would probably be good to make it so > that none of the decoders free input. Yes, this is a must. Consistency should be our goal/horizon/friend/helper. :-) -- Cheers Jorge.-

2 6

[patch]: html.cc: DilloHtmlForm::getCurrentInput()
by onepoint＠starurchin.org 27 May '08

27 May '08

This refactors DilloHtmlForm code into a new "inline DilloHtmlInput *getCurrentInput()" method. It's just like the earlier patch that created DilloHtml::getCurrentForm() . Comments are welcome, although I hope it is uncontroversial. Regards, Jeremy Henty

2 4