Bug 108858 – Store the language internally instead of using lang attribute for WebVTT nodes

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Dima Gorbik]

Specs only require language objects have lang attributes. For all other objects the language should be stored internally to enable ':lang' pseudo-class.

[Dima Gorbik]

Created attachment 186486 [details]
Proposed fix 0.1

[Radar WebKit Bug Importer]

<rdar://problem/13148532>

[WebKit Review Bot]

Comment on attachment 186486 [details]
Proposed fix 0.1

Attachment 186486 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16367469

[Dima Gorbik]

Created attachment 186498 [details]
Proposed fix 0.2

Is it better not to rename?

[Kent Tamura]

Comment on attachment 186498 [details]
Proposed fix 0.2

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

> Source/WebCore/dom/Element.cpp:2156
> -AtomicString Element::computeInheritedLanguage() const
> +AtomicString Element::elementLanguage() const

Please don't rename this function. The name 'compute' indicates this function is not a simple getter and needs higher cost.

> Source/WebCore/html/track/WebVTTElement.h:81
> +    AtomicString language;

m_language

[Alexey Proskuryakov]

Comment on attachment 186498 [details]
Proposed fix 0.2

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

> Source/WebCore/ChangeLog:10
> +        Only language webvtt elements should have a lang attribute so we have to store
> +        the language internally in the element. Refactored the code to make 
> +        computeInheritedLanguage virtual.

Despite this comment, I don't understand why WebVTTElement needs to store language internally. How is it different from every other element with a lang attribute?

> Source/WebCore/dom/Element.h:335
> -    AtomicString computeInheritedLanguage() const;
> +    virtual AtomicString elementLanguage() const;

This function has "compute" as a hint that it's unnecessary slow for most uses. One should almost always take language form the renderer, not from Element.

> Source/WebCore/html/track/WebVTTElement.h:59
> +    virtual AtomicString elementLanguage() const OVERRIDE { return language; }

I don't think that virtualizing core DOM functions is a great approach for WebVTT in general. Virtual function calls are slower, and a lot of effort has been invested in devirtualizing everything we could.

[Dima Gorbik]

Created attachment 186508 [details]
Proposed fix 0.3

[Dima Gorbik]

(In reply to comment #6)
> (From update of attachment 186498 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186498&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Only language webvtt elements should have a lang attribute so we have to store
> > +        the language internally in the element. Refactored the code to make 
> > +        computeInheritedLanguage virtual.
> 
> Despite this comment, I don't understand why WebVTTElement needs to store language internally. How is it different from every other element with a lang attribute?

If we store the language in the attribute then c[lang="en"] rule will work though it shouldn't according to specs. This is a difference between html and webvtt elements.  

> I don't think that virtualizing core DOM functions is a great approach for WebVTT in general. Virtual function calls are slower, and a lot of effort has been invested in devirtualizing everything we could.
In general yes, but what about this case? This function is already quite heavy in terms of performance. Also I will have to call a virtual isWebVTTElement anyway.

[Kent Tamura]

Comment on attachment 186498 [details]
Proposed fix 0.2

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

>>> Source/WebCore/ChangeLog:10
>>> +        computeInheritedLanguage virtual.
>> 
>> Despite this comment, I don't understand why WebVTTElement needs to store language internally. How is it different from every other element with a lang attribute?
> 
> If we store the language in the attribute then c[lang="en"] rule will work though it shouldn't according to specs. This is a difference between html and webvtt elements.

Doesn't :lang pseudo-class work?

[Kent Tamura]

Comment on attachment 186498 [details]
Proposed fix 0.2

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

>>>> Source/WebCore/ChangeLog:10
>>>> +        computeInheritedLanguage virtual.
>>> 
>>> Despite this comment, I don't understand why WebVTTElement needs to store language internally. How is it different from every other element with a lang attribute?
>> 
>> If we store the language in the attribute then c[lang="en"] rule will work though it shouldn't according to specs. This is a difference between html and webvtt elements.
> 
> Doesn't :lang pseudo-class work?

An, I understand.  The language information comes from VTT, not HTML/DOM.

[Dima Gorbik]

Created attachment 186514 [details]
Proposed fix 0.4

[Benjamin Poulain]

Comment on attachment 186514 [details]
Proposed fix 0.4

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

> Source/WebCore/html/track/WebVTTElement.cpp:74
> +    , m_language(nullAtom)

You should not have to write this, AtomicString default constructor is equivalent to the nullAtom.

> Source/WebCore/html/track/WebVTTElement.h:59
> +    virtual AtomicString computeInheritedLanguage() const OVERRIDE { return m_language; }

IMHO, this is abusing inheritance. The language your return has nothing to do with the InheritedLanguage.

[Dima Gorbik]

(In reply to comment #12)
> > Source/WebCore/html/track/WebVTTElement.h:59
> > +    virtual AtomicString computeInheritedLanguage() const OVERRIDE { return m_language; }
> 
> IMHO, this is abusing inheritance. The language your return has nothing to do with the InheritedLanguage.

The language I return is an inherited language but it's being pre-cached. The implementation we have in Element is related to html elements. That's why I suggested renaming at first. Those are different algorithms for getting a language for elements of different types.

[Antti Koivisto]

WebVTT selector matching does not really operate on DOM object at all. That WebVTT nodes are implemented as Elements an implementation detail. They are not conceptually elements. This is why I think it is was appropriate to have explicit test in SelectorChecker rather than hiding the differences behind virtual function.

In other words I don't think there is problem to solve here.

[Dima Gorbik]

Created attachment 186704 [details]
Proposed fix 0.5

Okay, it's probably better to leave this code the way it was.

[WebKit Review Bot]

Comment on attachment 186704 [details]
Proposed fix 0.5

Clearing flags on attachment: 186704

Committed r142043: <http://trac.webkit.org/changeset/142043>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Carlson]

*** Bug 101351 has been marked as a duplicate of this bug. ***