Bug 159666 – Change CSSParser::sourceSize returning Optional<CSSParser::SourceSize>

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Fujii Hironori]

(from Bug 159412 Comment 9)

> Assuming that we want to silently ignore the single size, I think the
> cleanest way to write the sourceSize function would be to have it return
> Optional<CSSParser::SourceSize> and return no source size at all when the
> value is invalid rather than instead using a size with a CSS_UNKNOWN
> primitive value in it.
> 
> The call site in CSSGrammar.y.in could just not append a size in that case.
> 
> But I am not sure what the desired behavior is in this error case.

[Fujii Hironori]

Yoav, do you do this?

[Yoav Weiss]

yeah, I'm on it

[Yoav Weiss]

Created attachment 283399 [details]
Patch

[Darin Adler]

Comment on attachment 283399 [details]
Patch

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

> Source/WebCore/css/CSSGrammar.y.in:580
> +        auto result = parser->sourceSize(WTFMove(*$1), $2);
> +        if (result)

I’d like this slightly better with the expression inside the if, but otherwise the patch looks perfect to me.

[Yoav Weiss]

Created attachment 283409 [details]
Patch

[Yoav Weiss]

(In reply to comment #5)
> Created attachment 283409 [details]
> Patch

(In reply to comment #4)
> Comment on attachment 283399 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283399&action=review
> 
> > Source/WebCore/css/CSSGrammar.y.in:580
> > +        auto result = parser->sourceSize(WTFMove(*$1), $2);
> > +        if (result)
> 
> I’d like this slightly better with the expression inside the if, but
> otherwise the patch looks perfect to me.

Done. Thanks for reviewing! :)

[Darin Adler]

Comment on attachment 283409 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:1560
>      if (!value) {
>          value = parserValue.createCSSValue();
>          if (!value)
> -            value = CSSPrimitiveValue::create(0, CSSPrimitiveValue::CSS_UNKNOWN);
> +            return Nullopt;
>      }
>      destroy(parserValue);

Oops, missed the storage leak here. We have to structure this so destroy(parserValue) is called even if we are going to return Nullopt.

[Yoav Weiss]

Created attachment 283530 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 283530 [details]
Patch

Rejecting attachment 283530 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 283530, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
fs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 203161 = fa38974410e9b628f2311c8da4a778d48636d306
r203162 = f4555b72adc88d17af46507f6d6ff5e5c6f3b81e
r203163 = 1b59d18bf9b48905986998af6522134b11aa6edb
r203164 = 6226acd6e0632c939b28a948f749093ddaa3a0cd
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.webkit.org/results/1674971

[Yoav Weiss]

Comment on attachment 283530 [details]
Patch

CQ failure at https://webkit-queues.webkit.org/results/1674971 seems spurious.

"You have both LayoutTests/imported/w3c and LayoutTests/imported/w3c/ChangeLog" is not true, as this patch doesn't touch LayoutTests/imported.

Trying to CQ it again.

[WebKit Commit Bot]

Comment on attachment 283530 [details]
Patch

Clearing flags on attachment: 283530

Committed r203269: <http://trac.webkit.org/changeset/203269>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.