Bug 159666

Summary: Change CSSParser::sourceSize returning Optional<CSSParser::SourceSize>
Product: WebKit Reporter: Fujii Hironori <Hironori.Fujii>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, yoav
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Fujii Hironori 2016-07-11 23:26:49 PDT
(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.
Comment 1 Fujii Hironori 2016-07-11 23:40:05 PDT
Yoav, do you do this?
Comment 2 Yoav Weiss 2016-07-12 00:25:42 PDT
yeah, I'm on it
Comment 3 Yoav Weiss 2016-07-12 02:02:18 PDT
Created attachment 283399 [details]
Patch
Comment 4 Darin Adler 2016-07-12 08:32:01 PDT
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.
Comment 5 Yoav Weiss 2016-07-12 08:41:27 PDT
Created attachment 283409 [details]
Patch
Comment 6 Yoav Weiss 2016-07-12 08:42:26 PDT
(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! :)
Comment 7 Darin Adler 2016-07-12 14:42:54 PDT
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.
Comment 8 Yoav Weiss 2016-07-13 08:58:16 PDT
Created attachment 283530 [details]
Patch
Comment 9 WebKit Commit Bot 2016-07-13 09:35:46 PDT
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
Comment 10 Yoav Weiss 2016-07-14 22:29:44 PDT
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.
Comment 11 WebKit Commit Bot 2016-07-14 22:50:25 PDT
Comment on attachment 283530 [details]
Patch

Clearing flags on attachment: 283530

Committed r203269: <http://trac.webkit.org/changeset/203269>
Comment 12 WebKit Commit Bot 2016-07-14 22:50:30 PDT
All reviewed patches have been landed.  Closing bug.