Bug 44803 – Decompose computing an element's computed language

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ilya Sherman]

Decompose computing an element's computed language

[Ilya Sherman]

Created attachment 65781 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 65781 [details]
Patch

This is not just refactoring code. You're creating chromium API.

Why?

[Ilya Sherman]

Created attachment 65798 [details]
Patch

[Ilya Sherman]

(In reply to comment #2)
> (From update of attachment 65781 [details])
> This is not just refactoring code. You're creating chromium API.
> 
> Why?

Clarified in the ChangeLogs -- want this for Chromium autofill i18n :)

[Eric Seidel (no email)]

I think this looks sane.  I think Darin F. should sign off on the API addition.

[Darin Fisher (:fishd, Google)]

Comment on attachment 65798 [details]
Patch

WebKit/chromium/public/WebElement.h:59
 +          WEBKIT_API WebString computeInheritedLang() const;
for the API at least, i recommend spelling out Language.  you should also break
this method apart from the others and add a comment describing what it does.
it isn't obvious what this method does.

[Alexey Proskuryakov]

> for the API at least, i recommend spelling out Language.

I think that it should be spelled out in internal function names, too.

But also, I don't think that "computedInheritedLanguage" is the right name - this function also takes node's own style into account.

+    // The language property is inherited, so we iterate over the parents to
+    // find the first language.

This is a very short comment, no need to split it into two lines.

I'm not sure how much benefit an API function provides over querying an element style.

[Ilya Sherman]

Created attachment 65933 [details]
Patch

[Ilya Sherman]

Updated API style per Darin's suggestions


(In reply to comment #7)
> > for the API at least, i recommend spelling out Language.
> 
> I think that it should be spelled out in internal function names, too.

I'm fuzzy on the style expectations for WebKit -- if it is labelled as "lang" elsewhere in the codebase, is it more important to be consistent with that or to gradually transition toward the spelled out version?  All of the instances I have seen elsewhere in the codebase label it just as "lang", though this might just be a biased sample on my part.

> 
> But also, I don't think that "computedInheritedLanguage" is the right name - this function also takes node's own style into account.

I'm wide open to suggestions :)

> 
> +    // The language property is inherited, so we iterate over the parents to
> +    // find the first language.
> 
> This is a very short comment, no need to split it into two lines.

Fixed.  In general, what line width should I aim for?  My emacs is currently set to 80-col for work with chromium, so that's what I've been defaulting to... but I have noticed that WebKit lines tend to be much longer.

> 
> I'm not sure how much benefit an API function provides over querying an element style.

AFAICT, there is no way to query an element's style to ask for its language -- only to query whether the language is some predetermined string.  For chromium i18n, we need some API -- though not necessarily this one -- to directly query an element's language.  It also makes some sense for this API to be exported via the DOM rather than CSS, as language codes are part of the HTML specification :)

[Ilya Sherman]

Created attachment 66197 [details]
Patch

[Ilya Sherman]

(In reply to comment #9)
> Updated API style per Darin's suggestions
> 
> 
> (In reply to comment #7)
> > > for the API at least, i recommend spelling out Language.
> > 
> > I think that it should be spelled out in internal function names, too.
> 
> I'm fuzzy on the style expectations for WebKit -- if it is labelled as "lang" elsewhere in the codebase, is it more important to be consistent with that or to gradually transition toward the spelled out version?  All of the instances I have seen elsewhere in the codebase label it just as "lang", though this might just be a biased sample on my part.
> 

Per conversation on IRC, "language" makes sense here but not elsewhere because we are not referring to the tag name or to the CSS property.  Patch updated correspondingly.

> > I'm not sure how much benefit an API function provides over querying an element style.
> 
> AFAICT, there is no way to query an element's style to ask for its language -- only to query whether the language is some predetermined string.  For chromium i18n, we need some API -- though not necessarily this one -- to directly query an element's language.  It also makes some sense for this API to be exported via the DOM rather than CSS, as language codes are part of the HTML specification :)

Alexey suggested that the value of the "lang" attribute can be accessed by JS via "document.defaultView.getComputedStyle(document.body, null).getPropertyValue("lang")"; and so we should be able to do something similar.  However, when I investigated this I found that |getComputedStyle| in JS does not include the "lang" attribute.  That seems like a bug which we should fix.

My sense is that this patch is a good first step in that direction, but perhaps the function should live elsewhere.  Thoughts?  Suggestions?

[Ilya Sherman]

Created attachment 66199 [details]
Patch

[Early Warning System Bot]

Attachment 66197 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3970017

[Ilya Sherman]

(In reply to comment #13)
Update patch should build.

(In reply to comment #11)
> Alexey suggested that the value of the "lang" attribute can be accessed by JS via "document.defaultView.getComputedStyle(document.body, null).getPropertyValue("lang")"; and so we should be able to do something similar.  However, when I investigated this I found that |getComputedStyle| in JS does not include the "lang" attribute.  That seems like a bug which we should fix.

I take it back, I don't think this is a bug -- "lang" is a CSS selector, not a property.

[Darin Fisher (:fishd, Google)]

Comment on attachment 66199 [details]
Patch

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

> WebKit/chromium/public/WebElement.h:61
> +        // Returns the language code specified for the current node.  This
nit: "the current node" -> "this element"

The Chromium API changes LGTM save for the nit above.

[Ilya Sherman]

Created attachment 66280 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 66280 [details]
Patch

Looks OK.

[WebKit Commit Bot]

Comment on attachment 66280 [details]
Patch

Clearing flags on attachment: 66280

Committed r66647: <http://trac.webkit.org/changeset/66647>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.