[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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #45574|review?                     |review-
               Flag|                            |




--- Comment #18 from Darin Adler <darin at apple.com>  2010-01-04 09:05:28 PST ---
(From update of attachment 45574)
> +static inline String canonicalizedTitle(Document* doc, const String& title)

We normally do not abbreviate names like "doc". Using "document" would be
better.

> +}
> +
> +
>  void Document::updateTitle()

Other functions in this file have one blank line between them.

>  {
> +    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.

> +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.

> 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'm going to say review-, but *only* because of the option-text mistake.
Otherwise seems good.

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.

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