If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

CSS @charset is not parsed correctly because of the incorrect offset calculation

VERIFIED FIXED in mozilla0.9.1

Status

()

Core
CSS Parsing and Computation
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: nhottanscp, Assigned: nhottanscp)

Tracking

Trunk
mozilla0.9.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

17 years ago
Parsing CSS "@charset" is implemented by using nsString::Find() the
nsString::Mid() to extract the charset. But the current code does incorrect
calculation to get an offset for Mid(), it passes 4 (size of a pointer) instead
of 8 (actual string length of "@charset"). So the parsed result prepended by "rset".
(Assignee)

Comment 1

17 years ago
forgot to mention that this is separated from bug 72658
(Assignee)

Comment 2

17 years ago
Created attachment 33951 [details] [diff] [review]
Patch, make it array so sizeof returns number of characters instead of a size of a pointer.

Comment 3

17 years ago
This is fine, but when using sizeof on an array, I think you should use the
familiar construct 'sizeof(array)/sizeof(array[0])' to get the number of
elements in the array (instead of assuming that the number of bytes is the same
as the number of elements), since you want the number of characters, not the
number of bytes in the array (yes I know, they are the same in this case on all
known platforms, but I'm feeling like a purist today). Since it is a nit,
r=attinasi anyway.
(Assignee)

Comment 4

17 years ago
Created attachment 33968 [details] [diff] [review]
Patch, changed offset to number of characters instead of number of bytes

Comment 5

17 years ago
Nope, Here is what you need:

aHTTPHeader.Mid(str, charsetOffset + (sizeof(charsetStr)/sizeof(charsetStr[0])),
-1);

because 'sizeof(charsetStr)/sizeof(charsetStr[0])' will always give you the
number of elements in the array, and that is what you want. (the same comment
applies to the other spot too, of course.)

attinasi: scc cited chapter and verse in bug 72658, sizeof(char)==1.  It doesn't
hurt or help to divide, except it hurts readability a bit.  No big either way.

nhotta: thanks for splitting these bugs, but which one should be 0.9.1 and which
0.9.2?  Which bug stands for the HTTP content-type ;charset= parameter not being
used if there is no @charset in a style sheet?

/be
attinasi: you're forgetting the NUL terminator -- see bug 72658.

nhotta: I really think the patch from bug 72658 should not have moved over here.
I thought you were going to file a sequel bug on the lack of HTTP content-type
;charset= precedence.

/be
(Assignee)

Comment 8

17 years ago
HTTP content-type case I am not sure if that's really broken or not (i have no
test case). So I leave it in bug 72658 for now and may file a separate one later.
I separated this because the patch is not really fixing bug 72658 and I wanted
to also ask layout people for a review. I mark this as 0.9.1.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.1

Comment 9

17 years ago
be - you are right, you are right. Sorry to promote obfuscation, and thanks for
the tip to Scott's sermon. Patch 33951 is better, r=attinasi on that patch.

sr=blizzard
(Assignee)

Comment 11

17 years ago
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
Sending to I18N QA, since I'm not exactly sure how to test this...
QA Contact: ian → andreasb
(Assignee)

Comment 13

17 years ago
Created attachment 37525 [details]
zipped test data

Comment 14

17 years ago
Changing QA contact to ftang@netscape.com for now.  Frank, can you verify this
within development or provide IQA with a test case?
QA Contact: andreasb → ftang
(Assignee)

Comment 15

17 years ago
Andreas, test data is attached (06/07/01 11:11).

Comment 16

17 years ago
Changing QA contact to teruko for now.
QA Contact: ftang → teruko

Comment 17

17 years ago
Verified in 6-18 Mtrunk build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.