Last Comment Bug 745685 - Parse <font size> per HTML5
: Parse <font size> per HTML5
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- enhancement with 1 vote (vote)
: mozilla15
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-16 00:12 PDT by :Aryeh Gregor (away until August 15)
Modified: 2012-05-03 13:12 PDT (History)
5 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (9.31 KB, patch)
2012-04-16 02:07 PDT, :Aryeh Gregor (away until August 15)
bzbarsky: review-
Details | Diff | Review
Patch v2 (9.18 KB, patch)
2012-04-18 00:54 PDT, :Aryeh Gregor (away until August 15)
bzbarsky: review+
Details | Diff | Review

Description :Aryeh Gregor (away until August 15) 2012-04-16 00:12:47 PDT
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.
Comment 1 :Aryeh Gregor (away until August 15) 2012-04-16 00:19:41 PDT
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.
Comment 2 :Aryeh Gregor (away until August 15) 2012-04-16 02:07:50 PDT
Created attachment 615274 [details] [diff] [review]
Patch v1

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 -).
Comment 3 Mozilla RelEng Bot 2012-04-16 02:10:49 PDT
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 Mozilla RelEng Bot 2012-04-16 05:30:21 PDT
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
Comment 5 Boris Zbarsky [:bz] 2012-04-17 13:40:11 PDT
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.
Comment 6 :Aryeh Gregor (away until August 15) 2012-04-18 00:54:47 PDT
Created attachment 616042 [details] [diff] [review]
Patch v2

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?
Comment 7 Mozilla RelEng Bot 2012-04-18 00:57:41 PDT
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 :Ms2ger 2012-04-18 05:17:25 PDT
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".
Comment 9 :Aryeh Gregor (away until August 15) 2012-04-18 05:28:39 PDT
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 Mozilla RelEng Bot 2012-04-18 05:47:28 PDT
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
Comment 11 Boris Zbarsky [:bz] 2012-04-18 12:09:02 PDT
Comment on attachment 616042 [details] [diff] [review]
Patch v2

r=me.  Nice.
Comment 12 :Aryeh Gregor (away until August 15) 2012-04-19 01:10:06 PDT
http://hg.mozilla.org/projects/birch/rev/98beb4e935d7
Comment 13 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-24 18:02:03 PDT
https://hg.mozilla.org/mozilla-central/rev/98beb4e935d7
Comment 14 Eric Shepherd [:sheppy] 2012-05-03 13:12:04 PDT
Documented:

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

And listed on Firefox 15 for developers.

Note You need to log in before you can comment on or make changes to this bug.