Specs only require language objects have lang attributes. For all other objects the language should be stored internally to enable ':lang' pseudo-class.
Created attachment 186486 [details] Proposed fix 0.1
<rdar://problem/13148532>
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
Created attachment 186498 [details] Proposed fix 0.2 Is it better not to rename?
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
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.
Created attachment 186508 [details] Proposed fix 0.3
(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.
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?
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.
Created attachment 186514 [details] Proposed fix 0.4
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.
(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.
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.
Created attachment 186704 [details] Proposed fix 0.5 Okay, it's probably better to leave this code the way it was.
Comment on attachment 186704 [details] Proposed fix 0.5 Clearing flags on attachment: 186704 Committed r142043: <http://trac.webkit.org/changeset/142043>
All reviewed patches have been landed. Closing bug.
*** Bug 101351 has been marked as a duplicate of this bug. ***