[webkit-reviews] review denied: [Bug 22333] Text wrapping algorithm for small screens (eg. Mobile screens) : [Attachment 25233] Text Wrapping algorithm

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Biment Srivastav
<biment.srivastav at nokia.com>'s request for review:
Bug 22333: Text wrapping algorithm for small screens (eg. Mobile screens)
https://bugs.webkit.org/show_bug.cgi?id=22333

Attachment 25233: Text Wrapping algorithm
https://bugs.webkit.org/attachment.cgi?id=25233&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>

> +	   void setTextWrappingEnabled(bool);
> +	   bool textWrappingEnabled() const { return m_textWrappingEnabled; }

Can you elaborate a bit what exactly this feature/setting implements?

I have doubts about the term "TextWrapping" as WebKit certainly wraps text by
default. I think this needs a more specific name.

> @@ -1579,6 +1581,54 @@
>      if (resolver.position().atEnd())
>	   return resolver.position();
>  
> +    int frame_width;
> +    RenderView* v = view();
> +    Settings *settings = NULL;
> +    if(v && v->frameView()){
> +	  frame_width = v->frameView()->visibleWidth();
> +	  frame_width = max(frame_width - leftOffset(m_height), 0);
> +	  settings = v->frameView()->frame()->page()->settings(); 
> +    }
> +    if((frame_width < width) && settings &&
settings->textWrappingEnabled()){
> +	  // try to figure out if we want to use text width limit  
> +	  bool fixedParent = false;
> +	  RenderObject* p = this;
> +	  // if we have a fixed height parent box, dont use it	
> +	  for (int n=4;p && n>0;n--){
> +	       if (p->style()->height().isFixed()) {
> +		   fixedParent = true;
> +		   break;
> +	       }
> +	       p = p->parent();
> +	  }
> +	  // check if there is enough text  
> +	  if (!fixedParent) {
> +	      RenderObject *o = resolver.position().obj;
> +	      unsigned charsFound = 0;
> +	      unsigned mostCharsInElement = 0;
> +	      while (o) {
> +		    if (o->isText()) {
> +			RenderText *t = static_cast<RenderText *>(o);
> +			mostCharsInElement = max(mostCharsInElement,
t->textLength());
> +			charsFound += t->textLength();
> +			if (charsFound>100 && mostCharsInElement>20) break;
> +		    }
> +		    if (o->isWidget()) {
> +			charsFound = 0;
> +			break;
> +		    }
> +		o = bidiNext( resolver.position().block, o);
> +	      }
> +	     // need at least x amount of characters in the block  
> +	     // also if longest single text node (not broken by inlines) is
very short  
> +	     // it is likely some menu thing and we don't apply the limit  
> +	     if (charsFound > 30 && mostCharsInElement > 25)
> +		 width = frame_width;
> +
> +	  }
> +    }

This code has some coding style issues. There are for example spaces missing
between the semicolons separating the arguments to the for() statement,
misplaced '*' signs for pointers or the use of NULL instead of 0 for declare a
null pointer.