[webkit-reviews] review granted: [Bug 201086] Privacy issue and spec violation in handling URLs in WebVTT STYLE blocks : [Attachment 388602] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brent Fulgham <bfulgham at webkit.org>'s
request for review:
Bug 201086: Privacy issue and spec violation in handling URLs in WebVTT STYLE
blocks
https://bugs.webkit.org/show_bug.cgi?id=201086

Attachment 388602: Patch

https://bugs.webkit.org/attachment.cgi?id=388602&action=review




--- Comment #25 from Darin Adler <darin at apple.com> ---
Comment on attachment 388602
  --> https://bugs.webkit.org/attachment.cgi?id=388602
Patch

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

I’m going to say r=me but I do still have open questions listed above.

> Source/WebCore/css/parser/CSSParserContext.h:75
> +    URL completeURL(const String& url) const;

I think it’s a little strange that this function member is stuck in between two
data members of this struct. I’d move it down so it’s last for style’s sake.

> Source/WebCore/css/parser/CSSParserContext.h:78
>  };

Should come back and fix some other things in this header. For example, I don’t
think CSSParserContextHash::hash should be inlined in the header; should be
moved to the .cpp file. And we should use WTF_MAKE_STRUCT_FAST_ALLOCATED so we
don't need to say "public:".

>> Source/WebCore/css/parser/CSSParserMode.h:47
>> +	WebVTTMode
> 
> Why do we want to allow internal properties and values in a VTT style?

I really don’t understand why a WebVTT stylesheet is more like the user agent
stylesheet in this respect. The special user-agent stylesheet only internal
stuff seems like it should not be allowed in WebVTT.

> Source/WebCore/dom/InlineStyleSheetOwner.cpp:62
> +	   if (auto* parent = element.parentNode()) {
> +	       if (is<VTTCueBox>(*parent))
> +		   result.mode = WebVTTMode;
> +	   }

Seems too mysterious to do this without a comment.