Bug 101245 - Add an extra test for background-position parsing.
Summary: Add an extra test for background-position parsing.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexis Menard (darktears)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-05 12:21 PST by Alexis Menard (darktears)
Modified: 2012-11-06 05:55 PST (History)
5 users (show)

See Also:


Attachments
Patch (14.21 KB, patch)
2012-11-05 12:49 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch for landing (14.25 KB, patch)
2012-11-06 03:35 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexis Menard (darktears) 2012-11-05 12:21:58 PST
Add an extra test for background-position parsing.
Comment 1 Alexis Menard (darktears) 2012-11-05 12:49:29 PST
Created attachment 172382 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-11-05 13:08:34 PST
Comment on attachment 172382 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172382&action=review

> LayoutTests/fast/backgrounds/background-position-parsing-2.html:4
> +<meta charset="utf-8">

Do we normally specify this?

> LayoutTests/fast/backgrounds/background-position-parsing-2.html:10
> +<script>
> +
> +description("Test to make sure background-position is correctly parsed.")

why newline here before description.

is parsed correctly.

> LayoutTests/fast/backgrounds/background-position-parsing-2.html:46
> +// One value.
> +style.backgroundPosition = "70%";
> +// Second value is assuming to be 'center'
> +shouldBe("style.backgroundPosition", "'70% 50%'");
> +shouldBe("computedStyle.backgroundPosition", "'70% 50%'");
> +style.backgroundPosition = "84px";
> +shouldBe("style.backgroundPosition", "'84px 50%'");
> +shouldBe("computedStyle.backgroundPosition", "'84px 50%'");
> +style.backgroundPosition = "left";
> +shouldBe("style.backgroundPosition", "'0% 50%'");
> +shouldBe("computedStyle.backgroundPosition", "'0% 50%'");
> +style.backgroundPosition = "right";
> +shouldBe("style.backgroundPosition", "'100% 50%'");
> +shouldBe("computedStyle.backgroundPosition", "'100% 50%'");
> +style.backgroundPosition = "bottom";
> +shouldBe("style.backgroundPosition", "'50% 100%'");
> +shouldBe("computedStyle.backgroundPosition", "'50% 100%'");
> +style.backgroundPosition = "top";
> +shouldBe("style.backgroundPosition", "'50% 0%'");
> +shouldBe("computedStyle.backgroundPosition", "'50% 0%'");
> +style.backgroundPosition = "center";
> +shouldBe("style.backgroundPosition", "'50% 50%'");
> +shouldBe("computedStyle.backgroundPosition", "'50% 50%'");

Adding a bit of newlines between tests might make this more readable

> LayoutTests/fast/backgrounds/background-position-parsing-2.html:172
> +document.body.removeChild(testContainer);
> +
> +</script>

I would remove that newline
Comment 3 Eric Seidel (no email) 2012-11-05 16:12:58 PST
Comment on attachment 172382 [details]
Patch

LGTM.
Comment 4 Alexis Menard (darktears) 2012-11-06 03:35:38 PST
Created attachment 172541 [details]
Patch for landing
Comment 5 WebKit Review Bot 2012-11-06 05:55:28 PST
Comment on attachment 172541 [details]
Patch for landing

Clearing flags on attachment: 172541

Committed r133591: <http://trac.webkit.org/changeset/133591>
Comment 6 WebKit Review Bot 2012-11-06 05:55:32 PST
All reviewed patches have been landed.  Closing bug.