[webkit-reviews] review denied: [Bug 91912] Adding "all" to -webkit-user-select : [Attachment 157026] part1

Tue Aug 7 16:29:39 PDT 2012

Ryosuke Niwa <rniwa at webkit.org> has denied Alice Cheng
<alice_cheng at apple.com>'s request for review:
Bug 91912: Adding "all" to -webkit-user-select
https://bugs.webkit.org/show_bug.cgi?id=91912

Attachment 157026: part1
https://bugs.webkit.org/attachment.cgi?id=157026&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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?