[Webkit-unassigned] [Bug 91912] Adding "all" to -webkit-user-select

Tue Aug 7 16:29:46 PDT 2012

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

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #157026|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |

--- Comment #25 from Ryosuke Niwa <rniwa at webkit.org>  2012-08-07 16:30:04 PST ---
(From update of attachment 157026)
View in context: https://bugs.webkit.org/attachment.cgi?id=157026&action=review

The code change looks right but we should probably put this on a separate bug.

> Source/WebCore/ChangeLog:3
> +        Part 1 of: Extend -webkit-user-select with a new value "all"

We don't normally attach multiple patches on a single bug. If you're intending to break the patch into small pieces,
then please file a bug that blocks this bug and post a patch there.

> Source/WebCore/ChangeLog:14
> +        (WebCore::isValidKeywordPropertyAndValue): add new value all

Nit: Please capitalize.

> Source/WebCore/rendering/style/StyleRareInheritedData.h:89
> -    unsigned userSelect : 1;  // EUserSelect
> +    unsigned userSelect : 2;  // EUserSelect

Please use single space after before //.

> LayoutTests/editing/selection/user-select-all-parsing-expected.txt:1
> +tests

Where did this text come from?

> LayoutTests/editing/selection/user-select-all-parsing.html:1
> +<html>

Missing DOCTYPE.

> LayoutTests/editing/selection/user-select-all-parsing.html:5
> +    <head>
> +        <style>
> +            .userSelectAll {-webkit-user-select: all;}
> +        </style>

We don't typically indent elements like this.

> LayoutTests/editing/selection/user-select-all-parsing.html:6
> +        <script src="../editing.js"></script>

We don't need editing.js ijn this test.

> LayoutTests/editing/selection/user-select-all-parsing.html:8
> +        <script src="resources/js-test-selection-shared.js"></script>

We don't this js file.

> LayoutTests/editing/selection/user-select-all-parsing.html:9
> +        <title> Test for Parsing new value all for -webkit-user-select </title>

We should be using description() to add a test description.

> LayoutTests/editing/selection/user-select-all-parsing.html:13
> +            this is text of -webkit-user-select all

Did you mean test?

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