Closed Bug 547267 Opened 10 years ago Closed 10 years ago

nsStyleVisibility should default to the document's Content-Language

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jfkthame, Assigned: jfkthame)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Attached patch patch, v1 (obsolete) — Splinter Review
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.
Assignee: nobody → jfkthame
Attachment #427922 - Flags: review?(roc)
Comment on attachment 427922 [details] [diff] [review]
patch, v1

Style system stuff probably needs dbaron...
Attachment #427922 - Flags: review?(roc) → review?(dbaron)
Attachment #427922 - Attachment is obsolete: true
Attachment #428795 - Flags: review?(dbaron)
Attachment #427922 - Flags: review?(dbaron)
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
Attachment #428795 - Flags: review?(dbaron) → review+
(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
(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.
Attachment #429056 - Flags: review?(dbaron)
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...
Attachment #429056 - Flags: review?(dbaron) → review+
(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.
Pushed followup:
http://hg.mozilla.org/mozilla-central/rev/d928716e11bb
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Duplicate of this bug: 122779
You need to log in before you can comment on or make changes to this bug.