Bug 44803 - Decompose computing an element's computed language
Summary: Decompose computing an element's computed language
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Windows 2000
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-27 16:09 PDT by Ilya Sherman
Modified: 2010-09-02 00:53 PDT (History)
7 users (show)

See Also:


Attachments
Patch (5.87 KB, patch)
2010-08-27 16:12 PDT, Ilya Sherman
no flags Details | Formatted Diff | Diff
Patch (6.10 KB, patch)
2010-08-27 18:11 PDT, Ilya Sherman
no flags Details | Formatted Diff | Diff
Patch (6.36 KB, patch)
2010-08-30 11:41 PDT, Ilya Sherman
no flags Details | Formatted Diff | Diff
Patch (6.39 KB, patch)
2010-09-01 04:23 PDT, Ilya Sherman
no flags Details | Formatted Diff | Diff
Patch (6.40 KB, patch)
2010-09-01 04:44 PDT, Ilya Sherman
no flags Details | Formatted Diff | Diff
Patch (6.37 KB, patch)
2010-09-01 14:56 PDT, Ilya Sherman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ilya Sherman 2010-08-27 16:09:51 PDT
Decompose computing an element's computed language
Comment 1 Ilya Sherman 2010-08-27 16:12:05 PDT
Created attachment 65781 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-08-27 17:28:33 PDT
Comment on attachment 65781 [details]
Patch

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

Why?
Comment 3 Ilya Sherman 2010-08-27 18:11:45 PDT
Created attachment 65798 [details]
Patch
Comment 4 Ilya Sherman 2010-08-27 18:12:36 PDT
(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 :)
Comment 5 Eric Seidel (no email) 2010-08-27 18:25:48 PDT
I think this looks sane.  I think Darin F. should sign off on the API addition.
Comment 6 Darin Fisher (:fishd, Google) 2010-08-27 23:17:31 PDT
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.
Comment 7 Alexey Proskuryakov 2010-08-28 22:43:23 PDT
> 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.
Comment 8 Ilya Sherman 2010-08-30 11:41:10 PDT
Created attachment 65933 [details]
Patch
Comment 9 Ilya Sherman 2010-08-30 11:56:25 PDT
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 :)
Comment 10 Ilya Sherman 2010-09-01 04:23:04 PDT
Created attachment 66197 [details]
Patch
Comment 11 Ilya Sherman 2010-09-01 04:33:30 PDT
(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?
Comment 12 Ilya Sherman 2010-09-01 04:44:59 PDT
Created attachment 66199 [details]
Patch
Comment 13 Early Warning System Bot 2010-09-01 04:45:09 PDT
Attachment 66197 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3970017
Comment 14 Ilya Sherman 2010-09-01 04:54:34 PDT
(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.
Comment 15 Darin Fisher (:fishd, Google) 2010-09-01 14:48:41 PDT
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.
Comment 16 Ilya Sherman 2010-09-01 14:56:44 PDT
Created attachment 66280 [details]
Patch
Comment 17 Eric Seidel (no email) 2010-09-01 16:13:38 PDT
Comment on attachment 66280 [details]
Patch

Looks OK.
Comment 18 WebKit Commit Bot 2010-09-02 00:53:07 PDT
Comment on attachment 66280 [details]
Patch

Clearing flags on attachment: 66280

Committed r66647: <http://trac.webkit.org/changeset/66647>
Comment 19 WebKit Commit Bot 2010-09-02 00:53:13 PDT
All reviewed patches have been landed.  Closing bug.