Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign uplayout: Implement ordered lists, CSS counters, and `quotes` per CSS 2.1 § 12.3-12.5. #5067
Conversation
highfive
commented
Feb 25, 2015
|
|
hoppipolla-critic-bot
commented
Feb 25, 2015
|
Critic review: https://critic.hoppipolla.co.uk/r/4104 This is an external review system which you may optionally use for the code review of your pull request. In order to help critic track your changes, please do not make in-place history rewrites (e.g. via |
|
@SimonSapin Did you want me to review the content property parsing refactor? |
|
Yes please. (I’m still reviewing the rest of your changes.) |
|
Done with the first round of review. I had rebased this when opening this new PR but it looks like it already conflicts with master again. Please wait until after review to rebase again. |
§ 12.3-12.5. Only simple alphabetic and numeric counter styles are supported. (This is most of them though.) Although this PR adds a sequential pass to layout, I verified that on pages that contain a reasonable number of ordered lists (Reddit `/r/rust`), the time spent in generated content resolution is dwarfed by the time spent in the parallelizable parts of layout. So I don't expect this to negatively affect our parallelism expect perhaps in pathological cases.
It wasn’t wrong, but it could be a lot shorter. * Use try! and match_ignore_ascii_case! macros whenever possible * Use expect_comma() instead of parse_comma_separated() when comma-separated values don’t have the same syntax * Prefer Parser::expect_* methods over doing the same with Parser::next * Take advantage of parse_nested_block returnin the return value of the closure * Use try! more.
Only simple alphabetic and numeric counter styles are supported. (This is most of them though.) Although this PR adds a sequential pass to layout, I verified that on pages that contain a reasonable number of ordered lists (Reddit `/r/rust`), the time spent in generated content resolution is dwarfed by the time spent in the parallelizable parts of layout. So I don't expect this to negatively affect our parallelism expect perhaps in pathological cases. Moved from #4544, because Critic.
This comment has been minimized.
This comment has been minimized.
|
r+ |
This comment has been minimized.
This comment has been minimized.
|
saw approval from SimonSapin |
This comment has been minimized.
This comment has been minimized.
|
merging servo/servo/counters = e209f5e into auto |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
some tests failed: |
This comment has been minimized.
This comment has been minimized.
|
r+ |
This comment has been minimized.
This comment has been minimized.
|
saw approval from SimonSapin |
This comment has been minimized.
This comment has been minimized.
|
merging servo/servo/counters = e209f5e into auto |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
some tests failed: |
Only simple alphabetic and numeric counter styles are supported. (This is most of them though.) Although this PR adds a sequential pass to layout, I verified that on pages that contain a reasonable number of ordered lists (Reddit `/r/rust`), the time spent in generated content resolution is dwarfed by the time spent in the parallelizable parts of layout. So I don't expect this to negatively affect our parallelism expect perhaps in pathological cases. Moved from #4544, because Critic.
…pass.
This comment has been minimized.
This comment has been minimized.
|
r+ |
This comment has been minimized.
This comment has been minimized.
|
saw approval from SimonSapin |
This comment has been minimized.
This comment has been minimized.
|
merging servo/servo/counters = 4c8bde5 into auto |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
all tests pass: |
This comment has been minimized.
This comment has been minimized.
|
fast-forwarding master to auto = 5cd6316 |
This comment has been minimized.
This comment has been minimized.
|
r+ |
This comment has been minimized.
This comment has been minimized.
|
saw approval from SimonSapin |
This comment has been minimized.
This comment has been minimized.
|
merging servo/servo/counters = 4c8bde5 into auto |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
all tests pass: |
This comment has been minimized.
This comment has been minimized.
|
fast-forwarding master to auto = 5cd6316 |
Only simple alphabetic and numeric counter styles are supported. (This is most of them though.) Although this PR adds a sequential pass to layout, I verified that on pages that contain a reasonable number of ordered lists (Reddit `/r/rust`), the time spent in generated content resolution is dwarfed by the time spent in the parallelizable parts of layout. So I don't expect this to negatively affect our parallelism expect perhaps in pathological cases. Moved from #4544, because Critic. Fixes #4544.
bors-servo
merged commit 4c8bde5
into
master
SimonSapin commentedFeb 25, 2015
Only simple alphabetic and numeric counter styles are supported. (This
is most of them though.)
Although this PR adds a sequential pass to layout, I verified that on
pages that contain a reasonable number of ordered lists (Reddit
/r/rust), the time spent in generated content resolution is dwarfed bythe time spent in the parallelizable parts of layout. So I don't expect
this to negatively affect our parallelism expect perhaps in pathological
cases.
Moved from #4544, because Critic.
Fixes #4544.