[Webkit-unassigned] [Bug 27032] document.title does not replace or remove space characters

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27032





--- Comment #19 from Christian Sejersen <christian.webkit at gmail.com>  2010-01-06 05:42:25 PST ---
(In reply to comment #18)
> (From update of attachment 45574 [details])
> > +static inline String canonicalizedTitle(Document* doc, const String& title)
> 
> We normally do not abbreviate names like "doc". Using "document" would be
> better.

Fixed.

> 
> > +}
> > +
> > +
> >  void Document::updateTitle()
> 
> Other functions in this file have one blank line between them.

Fixed.

> >  {
> > +    m_title = canonicalizedTitle(this, m_rawTitle);
> >      if (Frame* f = frame())
> >          f->loader()->setTitle(m_title);
> >  }
> 
> It would be good to call setTitle only if m_title is different than it was
> previously.

In setTitle there is the following check:

    if (m_rawTitle == title)
        return;

So updateTitle() isn't called unless they are different.

> > +debug('Test with Unicode whitespace in the space separator category');
> > +document.title = "\u0020\u0020one\u0020\u0020\u0020space\u0020\u0020";
> > +shouldBeEqualToString("document.title","one space");
> 
> This test does not do what it says. The spaces are all "\u0020" which is just a
> normal space expressed with "\u" syntax. It would be good to have tests of
> other whitespace characters as the comment indicates.

I took out this case as the only whitespace that we need to remove in the space
separator category is "\u0020". I have added "\u0020" to the next test. 

> 
> > Index: LayoutTests/fast/dom/HTMLOptionElement/option-text-expected.txt
> > ===================================================================
> > --- LayoutTests/fast/dom/HTMLOptionElement/option-text-expected.txt	(revision 52602)
> > +++ LayoutTests/fast/dom/HTMLOptionElement/option-text-expected.txt	(working copy)
> > @@ -6,4 +6,5 @@ PASS: got "foo" as expected.
> >  PASS: got "Â¥" as expected.
> >  PASS: got "foo bar" as expected.
> >  PASS: got "label" as expected.
> > +PASS: got "one space" as expected.
> >  
> > Index: LayoutTests/fast/dom/HTMLOptionElement/option-text.html
> > ===================================================================
> > --- LayoutTests/fast/dom/HTMLOptionElement/option-text.html	(revision 52602)
> > +++ LayoutTests/fast/dom/HTMLOptionElement/option-text.html	(working copy)
> > @@ -20,6 +20,7 @@
> >          <option id="o3">\</option>
> >          <option id="o4">foo   bar</option>
> >          <option id="o5" label=" label ">ignored</option>
> > +	<option id="o6">&#x0020;one&#x0009;&#x000a;&#x000c;&#x000d;space&#x0020;</option>
> >      </select>
> >      <pre id="console"></pre>
> >      <script>
> > @@ -42,6 +43,7 @@
> >          test("o3", "\u00a5");
> >          test("o4", "foo bar");
> >          test("o5", "label");
> > +	test("o6", "one space");
> >      </script>
> >  </body>
> >  </html>
> 
> This change to this test seems unrelated, and has tab characters in it. I think
> these tab characters are the reason this patch won't auto-apply.

I reverted the changes.

> 
> I'm going to say review-, but *only* because of the option-text mistake.
> Otherwise seems good.

Thanks for your patience with me.

> 
> I think we could follow this up by eliminating m_rawTitle entirely. And perhaps
> make some of the other changes from earlier versions of your patch if there are
> good reasons to do so.

Sounds like a good idea. I will file a follow-up bug once this gets in.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.