547267 - nsStyleVisibility should default to the document's Content-Language

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jonathan Kew [:jfkthame]]

See Bug 524107 comment #12.

If there's no explicit 'lang' attribute in scope, nsStyleVisibility should fall back to the document's Content-Language in preference to a language (group) guessed from the charset.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: patch, v1

Based on instrumenting the nsStyleVisibility constructor to see what language codes it finds, this seems to be doing the right thing. I guess we should really have a unit test for it though.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 427922 [details] [diff] [review]
patch, v1

Style system stuff probably needs dbaron...

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: patch, v2 - updated after landing of bug 524107

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 428795 [details] [diff] [review]
patch, v2 - updated after landing of bug 524107

>+  /**
>+   * Give access to our nsILanguageAtomService, so that others don't
>+   * have to look up the service separately and manage their own
>+   * references.
>+   */
>+  nsILanguageAtomService* GetLangService() const { return mLangService; }

It looks like this isn't actually used.  Remove it?


Other than that, this looks fine, although I think the code would be easier to understand if nsPresContext::GetLanguage were called nsPresContext::GetLanguageFromCharset or something ugly like that.  Maybe worth a followup patch?

r=dbaron

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to comment #4)
> >+  nsILanguageAtomService* GetLangService() const { return mLangService; }
> 
> It looks like this isn't actually used.  Remove it?

You're right - that was left over from the first version of the patch, but no longer needed since bug 524107 landed.

Pushed http://hg.mozilla.org/mozilla-central/rev/433f8ac169b1

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: followup: rename nsPresContext::GetLanguage to GetLanguageFromCharset

(In reply to comment #4)
> I think the code would be easier to
> understand if nsPresContext::GetLanguage were called
> nsPresContext::GetLanguageFromCharset or something ugly like that.  Maybe worth
> a followup patch?

Yes, I think that would be good. Turns out this is only used in two places, AFAICT, so the followup is trivial.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 429056 [details] [diff] [review]
followup: rename nsPresContext::GetLanguage to GetLanguageFromCharset

r=dbaron

The canvas caller could probably be improved at some point, too...

Comment 8

[Jonathan Kew [:jfkthame]]

(In reply to comment #7)
> The canvas caller could probably be improved at some point, too...

Yes, I was just looking at that, but will file a separate bug on it.

Comment 9

[Jonathan Kew [:jfkthame]]

Pushed followup:
http://hg.mozilla.org/mozilla-central/rev/d928716e11bb