stylo: Support symbols() function and string value for list-style-type

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Blocks: 3 bugs)

53 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

Comment hidden (empty)
(Assignee)

Updated

3 months ago
Depends on: 1328319
(Assignee)

Comment 1

3 months ago
Will working on this after bug 1328319 lands.
Assignee: nobody → xidorn+moz
Priority: -- → P1
(Assignee)

Updated

2 months ago
Depends on: 1366735
(Assignee)

Comment 2

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fa4f79e2489ff9b4b6de03bdd50add852f97bb2
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (obsolete)
Comment hidden (obsolete)

Comment 8

2 months ago
mozreview-review
Comment on attachment 8871542 [details]
Bug 1363596 part 1 - Merge nsCOMPtr<nsIAtom> into CounterStylePtr.

https://reviewboard.mozilla.org/r/143014/#review146808
Attachment #8871542 - Flags: review?(cam) → review+

Comment 9

2 months ago
mozreview-review
Comment on attachment 8871543 [details]
Bug 1363596 part 2 - Add support for symbols() function.

https://reviewboard.mozilla.org/r/143016/#review146810

::: layout/style/CounterStyleManager.h:107
(Diff revision 1)
>  
>  class AnonymousCounterStyle final : public CounterStyle
>  {
>  public:
>    explicit AnonymousCounterStyle(const nsSubstring& aContent);
> +  AnonymousCounterStyle(uint8_t aSystem, nsTArray<nsString> aSymbols);

Should the second argument be nsTArray<nsString>&&?

::: servo/components/style/gecko/values.rs:416
(Diff revision 1)
> +                let symbols: Vec<_> = symbols.0.iter().map(|symbol| match *symbol {
> +                    Symbol::String(ref s) => nsCString::from(s),
> +                    Symbol::Ident(_) => unreachable!("Should not have identifier in symbols()"),
> +                }).collect();
> +                let symbols: Vec<_> = symbols.iter().map(|symbol| {
> +                    symbol as &nsACString as *const nsACString

"as *const _" if you want to bit a bit shorter.
Attachment #8871543 - Flags: review?(cam) → review+

Comment 10

2 months ago
mozreview-review
Comment on attachment 8871544 [details]
Bug 1363596 part 3 - Add string support for list-style-type.

https://reviewboard.mozilla.org/r/143018/#review146814
Attachment #8871544 - Flags: review?(cam) → review+
(Assignee)

Comment 11

2 months ago
Servo side: servo/servo#17060
(Assignee)

Comment 12

2 months ago
mozreview-review-reply
Comment on attachment 8871543 [details]
Bug 1363596 part 2 - Add support for symbols() function.

https://reviewboard.mozilla.org/r/143016/#review146810

> Should the second argument be nsTArray<nsString>&&?

Not necessary as far as callsites use `Move`. Might be better using `&&` to avoid accidental copy, though...
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

2 months ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s f8256885e1be -d 1ff36fd81097: rebasing 398629:f8256885e1be "Bug 1363596 part 1 - Merge nsCOMPtr<nsIAtom> into CounterStylePtr. r=heycam"
rebasing 398630:7eccc8ee6a73 "Bug 1363596 part 2 - Add support for symbols() function. r=heycam"
merging layout/reftests/counter-style/reftest.list
warning: conflicts while merging layout/reftests/counter-style/reftest.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 17

2 months ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05b6b590129d
part 1 - Merge nsCOMPtr<nsIAtom> into CounterStylePtr. r=heycam
https://hg.mozilla.org/integration/autoland/rev/bfb3d68ed8c4
part 2 - Add support for symbols() function. r=heycam
https://hg.mozilla.org/integration/autoland/rev/9fcbe35118f1
part 3 - Add string support for list-style-type. r=heycam
https://hg.mozilla.org/mozilla-central/rev/05b6b590129d
https://hg.mozilla.org/mozilla-central/rev/bfb3d68ed8c4
https://hg.mozilla.org/mozilla-central/rev/9fcbe35118f1
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.