Closed Bug 745685 Opened 13 years ago Closed 13 years ago

Parse <font size> per HTML5

Categories

(Core :: DOM: Core & HTML, enhancement)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla15

People

(Reporter: ayg, Assigned: ayg)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

Patch v2
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review 
Our current algorithm for parsing <font size> is something like:

1) Strip whitespace from start and end.
2) If the string begins with "+" or "-":
  a) Truncate the string at the first non-digit character after the first.
  b) If the remaining string is exactly one of "-10", "-9", ..., "-0", "+0", "+1", ..., "+10", then add three and clamp to [1, 7].
  c) Otherwise, parse error.
3) Otherwise, parse as an integer, dropping any trailing non-digit characters.

HTML5 says what to do here:

http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#phrasing-content-1

It's almost the same, except that it will accept any integer with a plus or minus sign, not just -10 to +10.  So it treats +11 the same as +10.  This makes more sense than what Gecko currently does.
While we're at it: is there a reason we normalize during serialization here?  This:

data:text/html,<!doctype html>
<font size="+1x">
<script>
document.body.textContent =
document.body.firstChild.getAttribute("size")
</script>

produces "+1x" in Chrome 19 dev and Opera Next 12.00 alpha, but "+1" in IE10 Developer Preview and in Gecko.  Normalizing to "+1" doesn't follow any spec that I know of, and seems unnecessarily confusing.
Attached patch Patch v1 (obsolete) (deleted) — — Splinter Review 
I patterned the new function after nsAttrValue::StringToInteger, but it's different enough that I didn't think code reuse was practical (in particular, different handling of initial + and -).
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attachment #615274 - Flags: review?(bzbarsky)
Whiteboard: [autoland-try:-u all]
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]
Autoland Patchset:
	Patches: 615274
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=d761b0aa52bd
Try run started, revision d761b0aa52bd. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=d761b0aa52bd
Try run for d761b0aa52bd is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d761b0aa52bd
Results (out of 219 total builds):
    exception: 2
    success: 186
    warnings: 31
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-d761b0aa52bd
Whiteboard: [autoland-in-queue]
Comment on attachment 615274 [details] [diff] [review]
Patch v1

Reparsing this on every mapping operation seems like a pretty noticeable performance hit...  I would really prefer we did not do that.
Attachment #615274 - Flags: review?(bzbarsky) → review-
Attached patch Patch v2 (deleted) — — Splinter Review 
Okay, so this patch version stores both the parsed and unparsed values in the nsAttrValue.  Really we should be doing this for all attributes.  Throwing out the original string means that in general, if you do el.setAttribute(x, y), immediately calling el.getAttribute(x) will not return y, which is certainly a violation of the DOM specs.  Is there a reason we do it this way?  Are there significant memory savings, given that we're usually using nsIAtom anyway?
Attachment #615274 - Attachment is obsolete: true
Attachment #616042 - Flags: review?(bzbarsky)
Whiteboard: [autoland-try:-u all]
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]
Autoland Patchset:
	Patches: 616042
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=21ec0c73d987
Try run started, revision 21ec0c73d987. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=21ec0c73d987
In most cases, we can serialize directly from the parsed value; e.g., we don't need to store "10", but we need to (and do) store "10foo".
Hmm, that's true.  But we don't store "+1foo" without my patch.  Should I try to optimize for the case where the value is exactly "1", "2", "3", "4", "5", "6", or "7"?  I could, but would that really save much?
Try run for 21ec0c73d987 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=21ec0c73d987
Results (out of 221 total builds):
    exception: 1
    success: 187
    warnings: 33
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-21ec0c73d987
Whiteboard: [autoland-in-queue]
Comment on attachment 616042 [details] [diff] [review]
Patch v2

r=me.  Nice.
Attachment #616042 - Flags: review?(bzbarsky) → review+
http://hg.mozilla.org/projects/birch/rev/98beb4e935d7
Flags: in-testsuite+
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/98beb4e935d7
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Documented:

https://developer.mozilla.org/en/HTML/Element/font#Gecko_notes

And listed on Firefox 15 for developers.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: