Closed
Bug 745685
Opened 13 years ago
Closed 13 years ago
Parse <font size> per HTML5
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: ayg, Assigned: ayg)
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 1 obsolete file)
(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.
Assignee | ||
Comment 1•13 years ago
|
||
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.
Assignee | ||
Comment 2•13 years ago
|
||
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 | ||
Updated•13 years ago
|
Whiteboard: [autoland-try:-u all]
Updated•13 years ago
|
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]
Comment 3•13 years ago
|
||
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
Comment 4•13 years ago
|
||
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
Updated•13 years ago
|
Whiteboard: [autoland-in-queue]
![]() |
||
Comment 5•13 years ago
|
||
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-
Assignee | ||
Comment 6•13 years ago
|
||
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)
Assignee | ||
Updated•13 years ago
|
Whiteboard: [autoland-try:-u all]
Updated•13 years ago
|
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]
Comment 7•13 years ago
|
||
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
Comment 8•13 years ago
|
||
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".
Assignee | ||
Comment 9•13 years ago
|
||
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?
Comment 10•13 years ago
|
||
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
Updated•13 years ago
|
Whiteboard: [autoland-in-queue]
![]() |
||
Comment 11•13 years ago
|
||
Comment on attachment 616042 [details] [diff] [review] Patch v2 r=me. Nice.
Attachment #616042 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 12•13 years ago
|
||
http://hg.mozilla.org/projects/birch/rev/98beb4e935d7
Flags: in-testsuite+
Target Milestone: --- → mozilla15
Updated•13 years ago
|
Keywords: dev-doc-needed
Comment 13•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/98beb4e935d7
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 14•13 years ago
|
||
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.
Description
•