Issue 1498473004: CSS.escape('\0') should not throw.

Issue 1498473004: CSS.escape('\0') should not throw. (Closed)

Created:
5 years ago by ramya.v

Modified:
5 years ago

Reviewers:
Timothy Loh

CC:
chromium-reviews, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews, rwlbuis

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

CSS.escape('\0') should not throw. If the character is NULL(U+0000), then the REPLACEMENT CHARACTER (U+FFFD). https://drafts.csswg.org/cssom/#serialize-an-identifier BUG=563971 Committed: https://crrev.com/98b56298a07eed2fd9d3875e5f9ef9c69f5ad1e9 Cr-Commit-Position: refs/heads/master@{#364010}

Patch Set 2 : Modified return type to void #

Patch Set 3 : Updated as per review comments #

Patch Set 4 : Spec changed If the character is NULL then the REPLACEMENT CHARACTER. #

Patch Set 5 : Adding idl file changes #

Created: 5 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+26 lines, -25 lines)			Patch
M	third_party/WebKit/LayoutTests/css3/escape-dom-api.html	View	1 2 3	1 chunk	+9 lines, -4 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/escape-dom-api-expected.txt	View	1 2 3	1 chunk	+8 lines, -4 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/CSS.idl	View	1 2 4	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/css/CSSMarkup.h	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/css/CSSMarkup.cpp	View	1 2 3	3 chunks	+4 lines, -5 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/DOMWindowCSS.h	View	1 2 3	1 chunk	+1 line, -3 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/DOMWindowCSS.cpp	View	1 2 3	2 chunks	+2 lines, -7 lines	0 comments	Download

Messages

Total messages: 15 (5 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Timothy Loh

https://codereview.chromium.org/1498473004/diff/1/third_party/WebKit/LayoutTests/css3/escape-dom-api.html File third_party/WebKit/LayoutTests/css3/escape-dom-api.html (right): https://codereview.chromium.org/1498473004/diff/1/third_party/WebKit/LayoutTests/css3/escape-dom-api.html#newcode11 third_party/WebKit/LayoutTests/css3/escape-dom-api.html:11: shouldBeEqualToString("CSS.escape('\0')", "\\0 "); Shouldn't the expectation be "\\fffd "? ...

5 years ago (2015-12-03 04:08:03 UTC) #2

ramya.v

5 years ago (2015-12-03 06:33:46 UTC) #3

Timothy Loh

https://codereview.chromium.org/1498473004/diff/1/third_party/WebKit/Source/core/css/CSSMarkup.cpp File third_party/WebKit/Source/core/css/CSSMarkup.cpp (left): https://codereview.chromium.org/1498473004/diff/1/third_party/WebKit/Source/core/css/CSSMarkup.cpp#oldcode215 third_party/WebKit/Source/core/css/CSSMarkup.cpp:215: return false; On 2015/12/03 06:33:46, ramya.v wrote: > On ...

5 years ago (2015-12-03 07:16:52 UTC) #4

ramya.v

On 2015/12/03 07:16:52, Timothy Loh wrote: > https://codereview.chromium.org/1498473004/diff/1/third_party/WebKit/Source/core/css/CSSMarkup.cpp > File third_party/WebKit/Source/core/css/CSSMarkup.cpp (left): > > https://codereview.chromium.org/1498473004/diff/1/third_party/WebKit/Source/core/css/CSSMarkup.cpp#oldcode215 ...

5 years ago (2015-12-03 08:13:58 UTC) #5

Timothy Loh

On 2015/12/03 08:13:58, ramya.v wrote: > On 2015/12/03 07:16:52, Timothy Loh wrote: > > > ...

5 years ago (2015-12-03 09:26:49 UTC) #6

On 2015/12/03 08:13:58, ramya.v wrote:
> On 2015/12/03 07:16:52, Timothy Loh wrote:
> >
>
https://codereview.chromium.org/1498473004/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/css/CSSMarkup.cpp (left):
> > 
> >
>
https://codereview.chromium.org/1498473004/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/css/CSSMarkup.cpp:215: return false;
> > On 2015/12/03 06:33:46, ramya.v wrote:
> > > On 2015/12/03 04:08:03, Timothy Loh wrote:
> > > > If this no longer returns false, we should just make the return type
void
> > and
> > > > remove the RaisesException attribute from the idl.
> > > 
> > > Made changes to return type.
> > > RaisesException is used in case no arguments are provided to the escape
> > method.
> > > (From v8css.cpp  setMinimumArityTypeError(exceptionState, 1,
> info.Length());)
> > 
> > I checked and the code works without RaisesException.
> > 
> >
>
https://codereview.chromium.org/1498473004/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/css/CSSMarkup.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1498473004/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/css/CSSMarkup.cpp:220: else if (c == 0)
> > Your code is returning \0 because the previous if statement will catch 0 and
> > we'll never get into the branch here.
> 
> 
> Hi can you please check your intention is to modify CSS.idl only? Getting
> following build error
> In file included from
> gen/blink/bindings/core/v8/V8GeneratedCoreBindings19.cpp:33:
> gen/blink/bindings/core/v8/V8CSS.cpp:114:60: error: too few arguments to
> function call, expected 2, have 1
>     v8SetReturnValueString(info, DOMWindowCSS::escape(ident),
> info.GetIsolate());
>                                  ~~~~~~~~~~~~~~~~~~~~      ^
> ../../third_party/WebKit/Source/core/css/DOMWindowCSS.h:48:5: note: 'escape'
> declared here
>     static String escape(const String& ident, ExceptionState&);
> 
> Other things I updated as per review comments

You'll also have to remove the ExceptionState argument from
DOMWindowCSS::escape. Basically the RaisesException attribute means the bindings
generator will try and call the relevant function with an ExceptionState
argument, but since we are never using it we should just remove it and the
attribute.

ramya.v

On 2015/12/03 09:26:49, Timothy Loh wrote: > On 2015/12/03 08:13:58, ramya.v wrote: > > On ...

5 years ago (2015-12-08 07:57:35 UTC) #8

On 2015/12/03 09:26:49, Timothy Loh wrote:
> On 2015/12/03 08:13:58, ramya.v wrote:
> > On 2015/12/03 07:16:52, Timothy Loh wrote:
> > >
> >
>
https://codereview.chromium.org/1498473004/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/css/CSSMarkup.cpp (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/1498473004/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/css/CSSMarkup.cpp:215: return false;
> > > On 2015/12/03 06:33:46, ramya.v wrote:
> > > > On 2015/12/03 04:08:03, Timothy Loh wrote:
> > > > > If this no longer returns false, we should just make the return type
> void
> > > and
> > > > > remove the RaisesException attribute from the idl.
> > > > 
> > > > Made changes to return type.
> > > > RaisesException is used in case no arguments are provided to the escape
> > > method.
> > > > (From v8css.cpp  setMinimumArityTypeError(exceptionState, 1,
> > info.Length());)
> > > 
> > > I checked and the code works without RaisesException.
> > > 
> > >
> >
>
https://codereview.chromium.org/1498473004/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/css/CSSMarkup.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1498473004/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/css/CSSMarkup.cpp:220: else if (c == 0)
> > > Your code is returning \0 because the previous if statement will catch 0
and
> > > we'll never get into the branch here.
> > 
> > 
> > Hi can you please check your intention is to modify CSS.idl only? Getting
> > following build error
> > In file included from
> > gen/blink/bindings/core/v8/V8GeneratedCoreBindings19.cpp:33:
> > gen/blink/bindings/core/v8/V8CSS.cpp:114:60: error: too few arguments to
> > function call, expected 2, have 1
> >     v8SetReturnValueString(info, DOMWindowCSS::escape(ident),
> > info.GetIsolate());
> >                                  ~~~~~~~~~~~~~~~~~~~~      ^
> > ../../third_party/WebKit/Source/core/css/DOMWindowCSS.h:48:5: note: 'escape'
> > declared here
> >     static String escape(const String& ident, ExceptionState&);
> > 
> > Other things I updated as per review comments
> 
> You'll also have to remove the ExceptionState argument from
> DOMWindowCSS::escape. Basically the RaisesException attribute means the
bindings
> generator will try and call the relevant function with an ExceptionState
> argument, but since we are never using it we should just remove it and the
> attribute.


Updated the patchset as per review comments and spec change. PTAL!

Thanks

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1498473004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1498473004/80001

5 years ago (2015-12-09 06:15:37 UTC) #11

commit-bot: I haz the power

5 years ago (2015-12-09 08:50:49 UTC) #15

Message was sent while issue was closed.

Patchset 5 (id:??) landed as
https://crrev.com/98b56298a07eed2fd9d3875e5f9ef9c69f5ad1e9
Cr-Commit-Position: refs/heads/master@{#364010}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages