I am currently working on adding CSS support for "display: inline" tags. They are commonly used for navigation menus. As these menus are nothing more than simple unordered lists (<ul>), the items are printed one below the other which wastes lots of space. The patch I came up with does only partially implement support for these tags: The items are correctly displayed within one line but I cannot figure out why there is so much empty space after each item. Perhaps somebody with a better understanding of the dw-code than me might have a look into the issue. If I am correct, the dw/textblock.cc is responsible for calculating the widths. Textblock::wordWrap() looks pretty promising as there is a variable (hasListitemValue) determining whether it is a list item and calculates values for lastLine->leftOffset and words->getRef(0)->effSpace depending on this state. --Tim
Hi Tim, On Sat, Dec 05, 2009 at 09:10:45PM +0100, Tim Nieradzik wrote:
I am currently working on adding CSS support for "display: inline" tags. They are commonly used for navigation menus. As these menus are nothing more than simple unordered lists (<ul>), the items are printed one below the other which wastes lots of space.
The patch I came up with does only partially implement support for these tags: The items are correctly displayed within one line but I cannot figure out why there is so much empty space after each item.
Perhaps somebody with a better understanding of the dw-code than me might have a look into the issue. If I am correct, the dw/textblock.cc is responsible for calculating the widths. Textblock::wordWrap() looks pretty promising as there is a variable (hasListitemValue) determining whether it is a list item and calculates values for lastLine->leftOffset and words->getRef(0)->effSpace depending on this state.
I think we need to be a bit more radical. Consider this slightly modified patch (attached). If I read the CSS 2.1 Spec correctly then the idea is that every element can be a list-item if it has display: list-item set. So <li> elements would have to be handled as any other element but get display: list-item from our user-agent style. So for the real implementation we would need to change html.cc in a way that not the current element name, but the value of the display property determines whether to show a list item, or to show nothing at all (display: none) and so on. That said, I think the modified version of your patch is a good start until we fully support the display property, as lists with display: inline seem to be pretty common. So please test this with your favorite web sites. Cheers, Johannes
Hi Johannes, thank you very much. The patch works great for <li> elements but for each <ul> there is still some unused space. See http://www.heise.de/. I added "display: list-item" to the ul-item in Dillo's stylesheet definition and in Html_tag_open_ul() I am comparing the the display-attribute with the "list-item"-value. Strangely enough it worked! I am still wondering why it does. --Tim
Hi, I started to experiment with the real solution, which means that the display member of Style determines whether a (Text)block is created or a ListItem, or whether just normal text is added. The patch is still small, but as the change is pretty fundamental, so I decided to put it in a separate repo for now. You can find it at: http://www.flpsed.org/hgweb/dillo_display/ Please let me know what you think, and I would especially like to get opinions on how to implement the various display types: inline | block | list-item | run-in | inline-block | none I've intentionally left out the table stuff for now ... Cheers, Johannes
Johannes wrote:
Please let me know what you think, and I would especially like to get opinions on how to implement the various display types:
inline | block | list-item | run-in | inline-block | none
I don't know about details, but I suspect the logic will all end up in dw. And that we will end up having to make a lot of boxes eventually. In bug 929, someone wants to know why padding-left doesn't work for P elements, for instance. This might be what Sebastian meant in the top of textblock.hh about collapsing spaces being unnecessary in a CSS-ized dillo.
On Tue, Dec 08, 2009 at 08:36:35PM +0000, corvid wrote:
Johannes wrote:
Please let me know what you think, and I would especially like to get opinions on how to implement the various display types:
inline | block | list-item | run-in | inline-block | none
I don't know about details, but I suspect the logic will all end up in dw.
I thought that the display handling would be done in html.cc - by choosing the proper dw widget structure. I think this is what Sebastian calls "complex properties" in http://www.dillo.org/CSS.html
And that we will end up having to make a lot of boxes eventually.
Yes definately. Currently we sometimes just add a parbreak, where we would need a new box. That's also the reason for the inheritBackgroundColor hack. On the other hand more Textblock widgets consume more memory on large pages.
In bug 929, someone wants to know why padding-left doesn't work for P elements, for instance. This might be what Sebastian meant in the top of textblock.hh about collapsing spaces being unnecessary in a CSS-ized dillo.
I don't know enough about this space collapsing...
In my opinion, these changes make sense indeed as they provide a more generic interface. Looking at quirkmode's "display: list-item"-demo, listStylePosition seems to have a wrong value. The bullets are supposed to be drawn outside the box but they appear inside. Did I understand your changes correctly that listStylePosition is either 0 ("outside") or 1 ("inside") because of that enum? See http://www.quirksmode.org/css/display.html I tried to implement "display: none" but the contents are still visible. I also worked on getting rid of tagIsDisplayEnabled but I am yet uncertain whether it is safe how approached it. Any comments? --Tim
Hi Tim, On Wed, Dec 09, 2009 at 12:54:54AM +0100, Tim Nieradzik wrote:
In my opinion, these changes make sense indeed as they provide a more generic interface.
Looking at quirkmode's "display: list-item"-demo, listStylePosition seems to have a wrong value. The bullets are supposed to be drawn outside the box but they appear inside. Did I understand your changes correctly that listStylePosition is either 0 ("outside") or 1 ("inside") because of that enum?
I guess you're right, that the bullets should be outside the box, but due to the current implementation of ListItem it's inside even if listStylePosition is outside. However as long as you don't look at the border of the box, the behaviour is correct now, see e.g. http://de.selfhtml.org/css/eigenschaften/anzeige/list_style_position.htm
I tried to implement "display: none" but the contents are still visible. I also worked on getting rid of tagIsDisplayEnabled but I am yet uncertain whether it is safe how approached it. Any comments?
Just yesterday, I added the DISPLAY_NONE to dillo mainline, just as you did here :-) I'm still uncertain on what to do in case that DISPLAY_NONE is set. As you noticed it's not enough to not call the tag open functions, as text will still be added. Also I don't know, whether e.g. inputs in forms which have DISPLAY_NONE, should send their contents to the server in case of submit, or should they be just non-existent? Regarding tagIsDisplayEnabled, I don't quite follow what you intended? I had added this flag to gradually transform the Html_tag_open_*() functions to the new display handling. It can only be removed once all these functions have been converted. Hm, maybe you are right and as long as the corresponding tags are set to DISPLAY_INLINE, which is the default, it would work even without the tagIsDisplayEnabled flag. Anyway, I think it's still a good marker to see which functions have been converted. Regards, Johannes
--Tim
diff -r 440fa7e952bd dw/style.hh --- a/dw/style.hh Tue Dec 08 18:46:31 2009 +0100 +++ b/dw/style.hh Wed Dec 09 00:48:03 2009 +0100 @@ -253,7 +253,8 @@ DISPLAY_TABLE_FOOTER_GROUP, DISPLAY_TABLE_ROW, DISPLAY_TABLE_CELL, - DISPLAY_LAST + DISPLAY_LAST, + DISPLAY_NONE };
enum ListStylePosition { diff -r 440fa7e952bd src/cssparser.cc --- a/src/cssparser.cc Tue Dec 08 18:46:31 2009 +0100 +++ b/src/cssparser.cc Wed Dec 09 00:48:03 2009 +0100 @@ -58,10 +58,10 @@ "w-resize", "text", "wait", "help", NULL };
-static const char *const Css_display_enum_vals[DISPLAY_LAST + 1] = { +static const char *const Css_display_enum_vals[] = { "block", "inline", "list-item", "table", "table-row-group", "table-header-group", "table-footer-group", "table-row", - "table-cell", NULL + "table-cell", "last", "none", NULL };
static const char *const Css_font_size_enum_vals[] = { diff -r 440fa7e952bd src/html.cc --- a/src/html.cc Tue Dec 08 18:46:31 2009 +0100 +++ b/src/html.cc Wed Dec 09 00:48:03 2009 +0100 @@ -1282,7 +1282,7 @@ TagInfo toptag = Tags[toptag_idx]; if (s_sz > idx + 1 && toptag.EndTag != 'O') BUG_MSG(" - forcing close of open tag: <%s>\n", toptag.name); - _MSG("Close: %*s%s\n", size," ", toptag.name); + _MSG("Close: %*s%s\n", s_sz, " ", toptag.name); toptag.close(html, toptag_idx); Html_real_pop_tag(html); } @@ -1528,13 +1528,6 @@ }
/* - * By setting this to true, a Html_tag_open* function indicates that - * the common display handling code should be used. - * TODO: convert all Html_tag_open* functions and remove this variable. - */ -static bool tagIsDisplayEnabled; - -/* * Handle open HTML element */ static void Html_tag_open_html(DilloHtml *html, const char *tag, int tagsize) @@ -1876,8 +1869,6 @@ a_Html_stash_init(html); S_TOP(html)->parse_mode = DILLO_HTML_PARSE_MODE_STASH_AND_BODY; - - tagIsDisplayEnabled = true; }
/* @@ -1916,8 +1907,6 @@
html->styleEngine->setNonCssHints (&props); dFree(fontFamily); - - tagIsDisplayEnabled = true; }
/* @@ -1926,7 +1915,7 @@ static void Html_tag_open_abbr(DilloHtml *html, const char *tag, int tagsize) { const char *attrbuf; - + if (prefs.show_tooltip && (attrbuf = a_Html_get_attr(html, tag, tagsize, "title"))) { CssPropertyList props; @@ -1936,8 +1925,6 @@ html->styleEngine->setNonCssHints (&props); dFree(tooltip_str); } - - tagIsDisplayEnabled = true; }
/* @@ -2183,8 +2170,6 @@ BUG_MSG("name attribute is required for <map>\n"); } } - - tagIsDisplayEnabled = true; }
/* @@ -2453,8 +2438,6 @@ dFree(nameVal); } } - - tagIsDisplayEnabled = true; }
/* @@ -2528,8 +2511,6 @@ S_TOP(html)->list_type = HTML_LIST_UNORDERED; S_TOP(html)->list_number = 0; S_TOP(html)->ref_list_item = NULL; - - tagIsDisplayEnabled = true; }
/* @@ -2590,10 +2571,9 @@ BUG_MSG( "illegal '-' character in START attribute; Starting from 0\n"); n = 0; } + S_TOP(html)->list_number = n; S_TOP(html)->ref_list_item = NULL; - - tagIsDisplayEnabled = true; }
/* @@ -2621,8 +2601,6 @@ *list_number = 0; } } - - tagIsDisplayEnabled = true; }
/* @@ -3011,7 +2989,6 @@ static void Html_tag_open_default(DilloHtml *html,const char *tag,int tagsize) { html->styleEngine->inheritBackgroundColor(); - tagIsDisplayEnabled = true; }
/* @@ -3023,7 +3000,6 @@
a_Html_tag_set_align_attr (html, &props, tag, tagsize); html->styleEngine->setNonCssHints (&props); - tagIsDisplayEnabled = true; }
/* @@ -3418,11 +3394,11 @@ ListItem *list_item; int *list_number; char buf[16]; - + /* Get our parent tag's variables (used as state storage) */ list_number = &html->stack->getRef(html->stack->size()-2)->list_number; ref_list_item = &html->stack->getRef(html->stack->size()-2)->ref_list_item; - + HT2TB(html)->addParbreak (0, wordStyle);
list_item = new ListItem ((ListItem*)*ref_list_item,prefs.limit_text_width); @@ -3430,7 +3406,7 @@ HT2TB(html)->addParbreak (0, wordStyle); *ref_list_item = list_item; S_TOP(html)->textblock = html->dw = list_item; - + if (style->listStyleType == LIST_STYLE_TYPE_NONE) { // none } else if (style->listStyleType >= LIST_STYLE_TYPE_DECIMAL) { @@ -3496,15 +3472,11 @@ /* Parse attributes that can appear on any tag */ Html_parse_common_attrs(html, tag, tagsize);
- /* Html_tag_open* function can set this to true to enable - * generic CSS display handling. - */ - tagIsDisplayEnabled = false; - /* Call the open function for this tag */ - _MSG("Open : %s\n", Tags[ni].name); - Tags[ni].open (html, tag, tagsize); - - if (tagIsDisplayEnabled) { + if (! html->styleEngine->initialized () + || html->styleEngine->style ()->display == DISPLAY_NONE) { + /* Call the open function for this tag */ + Tags[ni].open (html, tag, tagsize); + switch (html->styleEngine->style ()->display) { case DISPLAY_BLOCK: Html_display_block(html); @@ -3549,7 +3521,6 @@ (strchr(" \"'", tag[tagsize-3]) || /* [ "']/> */ (size_t)tagsize == strlen(Tags[ni].name) + 3))) { /* <x/> */
- _MSG("Close: %s\n", Tags[ni].name); Html_tag_cleanup_at_close(html, ni); /* This was a close tag */ html->ReqTagClose = false; diff -r 440fa7e952bd src/styleengine.cc --- a/src/styleengine.cc Tue Dec 08 18:46:31 2009 +0100 +++ b/src/styleengine.cc Wed Dec 09 00:48:03 2009 +0100 @@ -567,6 +567,10 @@ return Style::create (layout, &attrs); }
+bool StyleEngine::initialized () { + return stack->getRef (stack->size () - 1)->style != NULL; +} + /** * \brief Create a new style object based on the previously opened / closed * HTML elements and the nonCssProperties that have been set. @@ -585,7 +589,7 @@ // If this assertion is hit, you need to rearrange the code that is // doing styleEngine calls to call setNonCssHints() before calling // style() or wordStyle() for each new element. - assert (stack->getRef (stack->size () - 1)->style == NULL); + assert (! StyleEngine::initialized());
// reset values that are not inherited according to CSS attrs.resetValues (); diff -r 440fa7e952bd src/styleengine.hh --- a/src/styleengine.hh Tue Dec 08 18:46:31 2009 +0100 +++ b/src/styleengine.hh Wed Dec 09 00:48:03 2009 +0100 @@ -59,6 +59,7 @@ return NULL; };
+ bool initialized (); void parse (DilloHtml *html, DilloUrl *url, const char *buf, int buflen, CssOrigin origin); void startElement (int tag);
Just yesterday, I added the DISPLAY_NONE to dillo mainline, just as you did here :-)
Oh, I just had a look at your private repository and noticed that it wasn't yet implemented. :)
I'm still uncertain on what to do in case that DISPLAY_NONE is set. As you noticed it's not enough to not call the tag open functions, as text will still be added.
I'd suggest to add the elements to the dw representation regardless of their "display" value. But we should introduce a 'flag' that disables drawing of elements with "display: none".
Also I don't know, whether e.g. inputs in forms which have DISPLAY_NONE, should send their contents to the server in case of submit, or should they be just non-existent?
Since CSS only "describes the presentation semantics" (Wikipedia), the form elements are still available but 'hidden'. In Gecko, the DOM contains hidden elements as well. "display: none" is commonly used for menus: With the help of JavaScript, you could fiddle inside the DOM and toggle "display"-value to make the submenu visible.
Regarding tagIsDisplayEnabled, I don't quite follow what you intended? I had added this flag to gradually transform the Html_tag_open_*() functions to the new display handling. It can only be removed once all these functions have been converted.
I thought the only sense in implementing tagIsDisplayEnabled was to prevent Dillo to crash? While playing around with DISPLAY_NONE detection, the "stack->getRef (stack->size () - 1)->style == NULL" assert failed for Html_tag_open_body(). Checking html->styleEngine->style ()->display for DISPLAY_NONE resulted in initializing the style engine too early.
Hm, maybe you are right and as long as the corresponding tags are set to DISPLAY_INLINE, which is the default, it would work even without the tagIsDisplayEnabled flag. Anyway, I think it's still a good marker to see which functions have been converted.
To what extent would they need to be converted? Why do we care at all if a tag supports the "display" attribute? --Tim
Tim wrote:
I'm still uncertain on what to do in case that DISPLAY_NONE is set. As you noticed it's not enough to not call the tag open functions, as text will still be added.
I'd suggest to add the elements to the dw representation regardless of their "display" value.
This sounds right to me, too. I'll take a look at preventing them from being drawn and taking up space. Also, iterators would have to ignore undisplayed words for copying text. Is there any case where iterators would need to be able to notice them?
On Wed, Dec 09, 2009 at 08:47:46PM +0000, corvid wrote:
Tim wrote:
I'm still uncertain on what to do in case that DISPLAY_NONE is set. As you noticed it's not enough to not call the tag open functions, as text will still be added.
I'd suggest to add the elements to the dw representation regardless of their "display" value.
This sounds right to me, too. I'll take a look at preventing them from being drawn and taking up space.
Great, it showing and hiding of hidden form entries works nicely already.
Also, iterators would have to ignore undisplayed words for copying text. Is there any case where iterators would need to be able to notice them?
No, I don't thinks so. For searching and text selection it should not be needed.
On Wed, Dec 09, 2009 at 09:07:53PM +0100, Tim Nieradzik wrote:
Just yesterday, I added the DISPLAY_NONE to dillo mainline, just as you did here :-)
Oh, I just had a look at your private repository and noticed that it wasn't yet implemented. :)
I'm still uncertain on what to do in case that DISPLAY_NONE is set. As you noticed it's not enough to not call the tag open functions, as text will still be added.
I'd suggest to add the elements to the dw representation regardless of their "display" value. But we should introduce a 'flag' that disables drawing of elements with "display: none".
Also I don't know, whether e.g. inputs in forms which have DISPLAY_NONE, should send their contents to the server in case of submit, or should they be just non-existent?
Since CSS only "describes the presentation semantics" (Wikipedia), the form elements are still available but 'hidden'. In Gecko, the DOM contains hidden elements as well. "display: none" is commonly used for menus: With the help of JavaScript, you could fiddle inside the DOM and toggle "display"-value to make the submenu visible.
Yes, that's a problem. We definately need a way to toggle the hidden state. Otherwise pages get less accessible as we don't support JavaScript.
Regarding tagIsDisplayEnabled, I don't quite follow what you intended? I had added this flag to gradually transform the Html_tag_open_*() functions to the new display handling. It can only be removed once all these functions have been converted.
I thought the only sense in implementing tagIsDisplayEnabled was to prevent Dillo to crash? While playing around with DISPLAY_NONE detection, the "stack->getRef (stack->size () - 1)->style == NULL" assert failed for Html_tag_open_body(). Checking html->styleEngine->style ()->display for DISPLAY_NONE resulted in initializing the style engine too early.
Hm, maybe you are right and as long as the corresponding tags are set to DISPLAY_INLINE, which is the default, it would work even without the tagIsDisplayEnabled flag. Anyway, I think it's still a good marker to see which functions have been converted.
To what extent would they need to be converted? Why do we care at all if a tag supports the "display" attribute?
Currently the Html_tag_open_*() functions add textblocks, or parbreaks as it fits. The idea is to convert them so that they only do HTML specific attribute parsing, setNonCssHints()-handling, and HTML error reporting. Building the dw widget tree would be solely determined by the current style (mainly the display part). I want to make this transition gradually, leaving e.g. forms and tables as they are atm. Cheers, Johannes
Johannes wrote:
Also I don't know, whether e.g. inputs in forms which have DISPLAY_NONE, should send their contents to the server in case of submit, or should they be just non-existent?
I'm not aware of anything in html4 about that case. I think css2 says somewhere that it doesn't specify what to do with forms.
On Tue, Dec 08, 2009 at 06:56:55PM +0100, Johannes Hofmann wrote:
Hi,
I started to experiment with the real solution, which means that the display member of Style determines whether a (Text)block is created or a ListItem, or whether just normal text is added. The patch is still small, but as the change is pretty fundamental, so I decided to put it in a separate repo for now. You can find it at:
I've updated this prototype a bit. It works quite well, the only drawback is the added complexity in html.cc. In addition to Html_tag_open_*() and Html_tag_clone_*(), there now is an optional Html_tag_content_*() function for tags that create widgets or other content. This function is not called in the display:none case. The reason for the splitting into Html_tag_open_*() and Html_tag_content_*() is that styleEngine->setNonCssHints() is needed before we can get the current style. Only after that is done, we know the value of the display property and can decide whether to call Html_tag_content_*() or not. I'd like to get this into main line soon if you agree. Please let me know what you think. Cheers, Johannes
Johannes wrote:
On Tue, Dec 08, 2009 at 06:56:55PM +0100, Johannes Hofmann wrote:
Hi,
I started to experiment with the real solution, which means that the display member of Style determines whether a (Text)block is created or a ListItem, or whether just normal text is added. The patch is still small, but as the change is pretty fundamental, so I decided to put it in a separate repo for now. You can find it at:
I've updated this prototype a bit. It works quite well, the only drawback is the added complexity in html.cc. In addition to Html_tag_open_*() and Html_tag_clone_*(), there now is an optional Html_tag_content_*() function for tags that create widgets or other content. This function is not called in the display:none case. The reason for the splitting into Html_tag_open_*() and Html_tag_content_*() is that styleEngine->setNonCssHints() is needed before we can get the current style. Only after that is done, we know the value of the display property and can decide whether to call Html_tag_content_*() or not.
I'd like to get this into main line soon if you agree. Please let me know what you think.
Maybe if Tags[] gets complicated enough, it will become possible to talk Jorge into using NULL for the other default fields as well. At the mention of setNonCssHints, I was hoping that whatever the prototype did would magically make it so that I could implement tooltips on arbitrary elements easily, but no such luck.
On Tue, Jan 12, 2010 at 08:43:41PM +0000, corvid wrote:
Johannes wrote:
On Tue, Dec 08, 2009 at 06:56:55PM +0100, Johannes Hofmann wrote:
Hi,
I started to experiment with the real solution, which means that the display member of Style determines whether a (Text)block is created or a ListItem, or whether just normal text is added. The patch is still small, but as the change is pretty fundamental, so I decided to put it in a separate repo for now. You can find it at:
I've updated this prototype a bit. It works quite well, the only drawback is the added complexity in html.cc. In addition to Html_tag_open_*() and Html_tag_clone_*(), there now is an optional Html_tag_content_*() function for tags that create widgets or other content. This function is not called in the display:none case. The reason for the splitting into Html_tag_open_*() and Html_tag_content_*() is that styleEngine->setNonCssHints() is needed before we can get the current style. Only after that is done, we know the value of the display property and can decide whether to call Html_tag_content_*() or not.
I'd like to get this into main line soon if you agree. Please let me know what you think.
Maybe if Tags[] gets complicated enough, it will become possible to talk Jorge into using NULL for the other default fields as well.
At the mention of setNonCssHints, I was hoping that whatever the prototype did would magically make it so that I could implement tooltips on arbitrary elements easily, but no such luck.
Is the title attribute allowed on all elements and always results in a tooltip? If so, we could rearrange things to parse common attributes in a special function.
Johannes wrote:
On Tue, Jan 12, 2010 at 08:43:41PM +0000, corvid wrote:
At the mention of setNonCssHints, I was hoping that whatever the prototype did would magically make it so that I could implement tooltips on arbitrary elements easily, but no such luck.
Is the title attribute allowed on all elements and always results in a tooltip? If so, we could rearrange things to parse common attributes in a special function.
title is part of %coreattrs along with id, class, and style. (and then there's also a title attr on the STYLE element) %coreattrs is part of %attrs, And what has %coreattrs or %attrs? Most things. As for behaviour, the spec says 'visual browsers frequently display the title as a "tool tip"'.
On Tue, Jan 12, 2010 at 06:48:19PM +0100, Johannes Hofmann wrote:
On Tue, Dec 08, 2009 at 06:56:55PM +0100, Johannes Hofmann wrote:
Hi,
I started to experiment with the real solution, which means that the display member of Style determines whether a (Text)block is created or a ListItem, or whether just normal text is added. The patch is still small, but as the change is pretty fundamental, so I decided to put it in a separate repo for now. You can find it at:
I've updated this prototype a bit. It works quite well, the only drawback is the added complexity in html.cc. In addition to Html_tag_open_*() and Html_tag_clone_*(), there now is an optional Html_tag_content_*() function for tags that create widgets or other content. This function is not called in the display:none case. The reason for the splitting into Html_tag_open_*() and Html_tag_content_*() is that styleEngine->setNonCssHints() is needed before we can get the current style. Only after that is done, we know the value of the display property and can decide whether to call Html_tag_content_*() or not.
I'd like to get this into main line soon if you agree. Please let me know what you think.
I checked the last repo, and AFAIS it's OK to merge. Notes: - I agree with corvid on using NULL instead of Html_tag_close_default. - There's a comment on *open and *close() function being always called but there's still a conditional close on display_none. Please check it. - Please don't merge the IPPROTO_TCP patch with it. -- Cheers Jorge.-
On Mon, Jan 18, 2010 at 03:40:31PM -0300, Jorge Arellano Cid wrote:
On Tue, Jan 12, 2010 at 06:48:19PM +0100, Johannes Hofmann wrote:
On Tue, Dec 08, 2009 at 06:56:55PM +0100, Johannes Hofmann wrote:
Hi,
I started to experiment with the real solution, which means that the display member of Style determines whether a (Text)block is created or a ListItem, or whether just normal text is added. The patch is still small, but as the change is pretty fundamental, so I decided to put it in a separate repo for now. You can find it at:
I've updated this prototype a bit. It works quite well, the only drawback is the added complexity in html.cc. In addition to Html_tag_open_*() and Html_tag_clone_*(), there now is an optional Html_tag_content_*() function for tags that create widgets or other content. This function is not called in the display:none case. The reason for the splitting into Html_tag_open_*() and Html_tag_content_*() is that styleEngine->setNonCssHints() is needed before we can get the current style. Only after that is done, we know the value of the display property and can decide whether to call Html_tag_content_*() or not.
I'd like to get this into main line soon if you agree. Please let me know what you think.
I checked the last repo, and AFAIS it's OK to merge.
I agreed with corvid to leave it out until after the release.
Notes:
- I agree with corvid on using NULL instead of Html_tag_close_default.
Ok.
- There's a comment on *open and *close() function being always called but there's still a conditional close on display_none. Please check it.
Ok.
- Please don't merge the IPPROTO_TCP patch with it.
If you mean the TCP_NODELAY stuff, it's in the main repo already. Normally the TCP stack waits a little until there is enough data to send. This is good for throughput but bad for response time. It is especially bad for client server stuff like DPI over local loopback. It really slows down pages with cookies. But I can back it out if you like. It would no longer be necessary when we switch back to UNIX domain sockets anyway. Cheers, Johannes
On Tue, Jan 19, 2010 at 08:17:01PM +0100, Johannes Hofmann wrote:
On Mon, Jan 18, 2010 at 03:40:31PM -0300, Jorge Arellano Cid wrote:
On Tue, Jan 12, 2010 at 06:48:19PM +0100, Johannes Hofmann wrote:
On Tue, Dec 08, 2009 at 06:56:55PM +0100, Johannes Hofmann wrote:
Hi,
I started to experiment with the real solution, which means that the display member of Style determines whether a (Text)block is created or a ListItem, or whether just normal text is added. The patch is still small, but as the change is pretty fundamental, so I decided to put it in a separate repo for now. You can find it at:
I've updated this prototype a bit. It works quite well, the only drawback is the added complexity in html.cc. In addition to Html_tag_open_*() and Html_tag_clone_*(), there now is an optional Html_tag_content_*() function for tags that create widgets or other content. This function is not called in the display:none case. The reason for the splitting into Html_tag_open_*() and Html_tag_content_*() is that styleEngine->setNonCssHints() is needed before we can get the current style. Only after that is done, we know the value of the display property and can decide whether to call Html_tag_content_*() or not.
I'd like to get this into main line soon if you agree. Please let me know what you think.
[...]
- Please don't merge the IPPROTO_TCP patch with it.
If you mean the TCP_NODELAY stuff, it's in the main repo already. Normally the TCP stack waits a little until there is enough data to send. This is good for throughput but bad for response time. It is especially bad for client server stuff like DPI over local loopback. It really slows down pages with cookies. But I can back it out if you like. It would no longer be necessary when we switch back to UNIX domain sockets anyway.
OK, leave it then. BTW, is the connection-number-limit patch in the next release? -- Cheers Jorge.-
On Wed, Jan 20, 2010 at 08:59:12AM -0300, Jorge Arellano Cid wrote:
On Tue, Jan 19, 2010 at 08:17:01PM +0100, Johannes Hofmann wrote:
On Mon, Jan 18, 2010 at 03:40:31PM -0300, Jorge Arellano Cid wrote:
On Tue, Jan 12, 2010 at 06:48:19PM +0100, Johannes Hofmann wrote:
On Tue, Dec 08, 2009 at 06:56:55PM +0100, Johannes Hofmann wrote:
Hi,
I started to experiment with the real solution, which means that the display member of Style determines whether a (Text)block is created or a ListItem, or whether just normal text is added. The patch is still small, but as the change is pretty fundamental, so I decided to put it in a separate repo for now. You can find it at:
I've updated this prototype a bit. It works quite well, the only drawback is the added complexity in html.cc. In addition to Html_tag_open_*() and Html_tag_clone_*(), there now is an optional Html_tag_content_*() function for tags that create widgets or other content. This function is not called in the display:none case. The reason for the splitting into Html_tag_open_*() and Html_tag_content_*() is that styleEngine->setNonCssHints() is needed before we can get the current style. Only after that is done, we know the value of the display property and can decide whether to call Html_tag_content_*() or not.
I'd like to get this into main line soon if you agree. Please let me know what you think.
[...]
- Please don't merge the IPPROTO_TCP patch with it.
If you mean the TCP_NODELAY stuff, it's in the main repo already. Normally the TCP stack waits a little until there is enough data to send. This is good for throughput but bad for response time. It is especially bad for client server stuff like DPI over local loopback. It really slows down pages with cookies. But I can back it out if you like. It would no longer be necessary when we switch back to UNIX domain sockets anyway.
OK, leave it then.
BTW, is the connection-number-limit patch in the next release?
No, it's not ready yet - unfortunately. We urgently need a connection limit, but it needs to be per remote host, as otherwise dillo can have all of it's 4 connections open to a slow server and then there is no connection left for other servers, e.g. used for other windows or tabs. We alse might need to make a special case for connections to proxies. Ah, and while at it it might be interesting to look into persistent connections... Cheers, Johannes
Hi, I made some progress on the connection limit stuff. On Wed, Jan 20, 2010 at 02:43:17PM +0100, Johannes Hofmann wrote:
On Wed, Jan 20, 2010 at 08:59:12AM -0300, Jorge Arellano Cid wrote:
BTW, is the connection-number-limit patch in the next release?
No, it's not ready yet - unfortunately. We urgently need a connection limit, but it needs to be per remote host, as otherwise dillo can have all of it's 4 connections open to a slow server and then there is no connection left for other servers, e.g. used for other windows or tabs.
The limit is now per remote host, so slow servers don't block connections to other (faster) ones. Unfortunately the hash table implementation from lout can't be used, as http.c is C file. However the number of hosts to which we have open connections at a time seems to be quite small (maximum about 4 in my tests), so the simple list I'm using instead should be ok.
We alse might need to make a special case for connections to proxies.
Connections to proxies are also limited - with the same limit as for normal hosts (4 atm). Please give it a try. If it works out ok we might even consider it for the release. Cheers, Johannes
Johannes Hofmann (2010-01-21 21:07):
Hi,
I made some progress on the connection limit stuff.
On Wed, Jan 20, 2010 at 02:43:17PM +0100, Johannes Hofmann wrote:
On Wed, Jan 20, 2010 at 08:59:12AM -0300, Jorge Arellano Cid wrote:
BTW, is the connection-number-limit patch in the next release?
No, it's not ready yet - unfortunately. We urgently need a connection limit, but it needs to be per remote host, as otherwise dillo can have all of it's 4 connections open to a slow server and then there is no connection left for other servers, e.g. used for other windows or tabs.
The limit is now per remote host, so slow servers don't block connections to other (faster) ones. Unfortunately the hash table implementation from lout can't be used, as http.c is C file. However the number of hosts to which we have open connections at a time seems to be quite small (maximum about 4 in my tests), so the simple list I'm using instead should be ok.
We alse might need to make a special case for connections to proxies.
Connections to proxies are also limited - with the same limit as for normal hosts (4 atm).
Please give it a try. If it works out ok we might even consider it for the release.
I have skimmed over hundreds of photos on flickr.com with both, vanilla dillo-2.1.1 and dillo-hg+connlimit2, with proxy and without. Browsing seems to work fine with all variations (I didn't try to analyze the packets), but I had a hard time trying to see the difference: I guess my internet connection is too fast and my X server too slow. Since I have a good internet connection, the images loaded faster without the patch, but sometimes the ones at the top of a page appeared sooner when the patch was applied. Since dillo and the servers seem to work with unlimited connections, perhaps defaulting to a limit of 6 wouldn't hurt? Dillo crashed twice in the beginning of my testing, but I am not sure anymore which build it was (because I've applied the gnutls-0.patch at first - https seemed to work). I couldn't reproduce the crashes afterwards. -- -- Rogut?s Sparnuotos
Rogut?s Sparnuotos :
I have skimmed over hundreds of photos on flickr.com with both, vanilla dillo-2.1.1 and dillo-hg+connlimit2, with proxy and without. Browsing seems to work fine with all variations (I didn't try to analyze the packets), but I had a hard time trying to see the difference: I guess my internet connection is too fast and my X server too slow. Since I have a good internet connection, the images loaded faster without the patch, but sometimes the ones at the top of a page appeared sooner when the patch was applied.
Yes, that's expected. The purpose of the patch is not to improve loading speed or user experience, but to play nice with servers and infrastructure. Modern sites especially big ones like flickr are well prepared to handle the load caused by the few dillo users out there. Nevertheless it's important to use network resources in a sensible and compliant way.
Since dillo and the servers seem to work with unlimited connections, perhaps defaulting to a limit of 6 wouldn't hurt?
Yes, the current limit is certainly debatable. We could even make it configurable. Any opinions on this?
Dillo crashed twice in the beginning of my testing, but I am not sure anymore which build it was (because I've applied the gnutls-0.patch at first - https seemed to work). I couldn't reproduce the crashes afterwards.
Please keep an eye on potential crashes. Those need to be ironed out of course. Thanks for reporting back, Johannes -- Haiti-Nothilfe! Helfen Sie per SMS: Sende UIHAITI an die Nummer 81190. Von 5 Euro je SMS (zzgl. SMS-Geb?hr) gehen 4,83 Euro an UNICEF.
On Sat, Jan 23, 2010 at 09:07:10PM +0100, Johannes Hofmann wrote:
Rogut??s Sparnuotos :
I have skimmed over hundreds of photos on flickr.com with both, vanilla dillo-2.1.1 and dillo-hg+connlimit2, with proxy and without. Browsing seems to work fine with all variations (I didn't try to analyze the packets), but I had a hard time trying to see the difference: I guess my internet connection is too fast and my X server too slow. Since I have a good internet connection, the images loaded faster without the patch, but sometimes the ones at the top of a page appeared sooner when the patch was applied.
Openning all the connections at once helps speed by making the wait for connections parallel for all images. This is noticeable with a big network latency.
Yes, that's expected. The purpose of the patch is not to improve loading speed or user experience, but to play nice with servers and infrastructure. Modern sites especially big ones like flickr are well prepared to handle the load caused by the few dillo users out there. Nevertheless it's important to use network resources in a sensible and compliant way.
I expect most of the mainstream servers to queue multiple requests and thence serve from their queue as much as they want. Other applications may have trouble with a herd of requests at the same time and hence prefer a connection limit. Pages with hundreds of non-tiny images (e.g. some blogs) may *feel* better with throttled requests; here users see the nice effect of images loading "in order".
Since dillo and the servers seem to work with unlimited connections, perhaps defaulting to a limit of 6 wouldn't hurt?
Yes, the current limit is certainly debatable. We could even make it configurable. Any opinions on this?
Configurable with a dillorc option would be welcomed. We could comment suggested values for dialup, low band, broadband, ...
Dillo crashed twice in the beginning of my testing, but I am not sure anymore which build it was (because I've applied the gnutls-0.patch at first - https seemed to work). I couldn't reproduce the crashes afterwards.
Please keep an eye on potential crashes. Those need to be ironed out of course.
I remember when making the first connection limit patch to have apprehensions on the stop/abort process. This is very complex inside Dillo. If you want to crash-test, try going to pages then pressing stop (or quiting that tab), or opening a new one while the other loads, then aborting the first one. Things like that will make dillo stop, clean and keep going. That's the gist. Thanks for your feedback. -- Cheers Jorge.-
2010/1/22 Johannes Hofmann <Johannes.Hofmann@gmx.de>:
Connections to proxies are also limited - with the same limit as for normal hosts (4 atm).
Please give it a try. If it works out ok we might even consider it for the release.
I'm using the new patch for a few days and it seems that I don't see the problem. Actually I changed proxy connection limit to 10 and this may also help with patched Dillo. Regards, furaisanjin
On Mon, Jan 25, 2010 at 06:50:44PM +0900, furaisanjin wrote:
2010/1/22 Johannes Hofmann <Johannes.Hofmann@gmx.de>:
Connections to proxies are also limited - with the same limit as for normal hosts (4 atm).
Please give it a try. If it works out ok we might even consider it for the release.
I'm using the new patch for a few days and it seems that I don't see the problem. Actually I changed proxy connection limit to 10 and this may also help with patched Dillo.
Good. It's in the main repo now with a configurable connection limit (http_max_conns in dillorc). Regards, Johannes
participants (6)
-
corvid＠lavabit.com -
furaisanjin＠gmail.com -
jcid＠dillo.org -
Johannes.Hofmann＠gmx.de -
rogutes＠googlemail.com -
tim.nieradzik＠gmx.de