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

Fujii Hironori
Reported 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.
Attachments
Patch (6.64 KB, patch)
2016-07-12 02:02 PDT, Yoav Weiss
no flags
Patch (6.62 KB, patch)
2016-07-12 08:41 PDT, Yoav Weiss
no flags
Patch (6.98 KB, patch)
2016-07-13 08:58 PDT, Yoav Weiss
no flags
Fujii Hironori
Comment 1 2016-07-11 23:40:05 PDT
Yoav, do you do this?
Yoav Weiss
Comment 2 2016-07-12 00:25:42 PDT
yeah, I'm on it
Yoav Weiss
Comment 3 2016-07-12 02:02:18 PDT
Darin Adler
Comment 4 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.
Yoav Weiss
Comment 5 2016-07-12 08:41:27 PDT
Yoav Weiss
Comment 6 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! :)
Darin Adler
Comment 7 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.
Yoav Weiss
Comment 8 2016-07-13 08:58:16 PDT
WebKit Commit Bot
Comment 9 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
Yoav Weiss
Comment 10 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.
WebKit Commit Bot
Comment 11 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>
WebKit Commit Bot
Comment 12 2016-07-14 22:50:30 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.