Closed Bug 1363596 Opened 7 years ago Closed 7 years ago

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

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla55

Tracking Flags:

Tracking

Status

firefox55

---

fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(3 files)

Bug 1363596 part 1 - Merge nsCOMPtr<nsIAtom> into CounterStylePtr. 7 years ago Xidorn Quan [:xidorn] UTC+11 59 bytes, text/x-review-board-request	heycam : review+	Details
Bug 1363596 part 2 - Add support for symbols() function. 7 years ago Xidorn Quan [:xidorn] UTC+11 59 bytes, text/x-review-board-request	heycam : review+	Details
Bug 1363596 part 3 - Add string support for list-style-type. 7 years ago Xidorn Quan [:xidorn] UTC+11 59 bytes, text/x-review-board-request	heycam : review+	Details

Xidorn Quan [:xidorn] UTC+11

Assignee

Description

•

7 years ago

      No description provided.

Xidorn Quan [:xidorn] UTC+11

Assignee

Updated

•

7 years ago

Depends on: 1328319

Xidorn Quan [:xidorn] UTC+11

Assignee

Comment 1

•

7 years ago

Will working on this after bug 1328319 lands.

Assignee: nobody → xidorn+moz

Bobby Holley (:bholley)

Updated

•

7 years ago

Priority: -- → P1

Xidorn Quan [:xidorn] UTC+11

Assignee

Updated

•

7 years ago

Depends on: 1366735

Xidorn Quan [:xidorn] UTC+11

Assignee

Comment 2

•

7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fa4f79e2489ff9b4b6de03bdd50add852f97bb2

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/143014/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/143014/

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/143016/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/143016/

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/143018/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/143018/

Comment hidden (obsolete)

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

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

::: layout/style/ServoBindings.cpp:1145
(Diff revision 1)
> +                               uint32_t aSymbolsCount)
> +{
> +  nsTArray<nsString> symbols(aSymbolsCount);
> +  for (uint32_t i = 0; i < aSymbolsCount; i++) {
> +    nsString* symbol = symbols.AppendElement();
> +    AppendUTF8toUTF16(*aSymbols[i], *symbol);

There is still a hazard from this that:
```
[27.28s] #97 Analyzing Gecko_SetCounterStyleToSymbols ...
Error: Field write mozilla::detail::nsStringRepr.mLength nsAString.field:0
Location: _ZN9nsAString9SetLengthEj$void nsAString::SetLength(uint32) @ xpcom/string/nsTSubstring.cpp#730
Stack Trace:
_Z17AppendUTF8toUTF16RK10nsACStringR9nsAStringRKN7mozilla10fallible_tE$uint8 AppendUTF8toUTF16(nsACString*, nsAString*, mozilla::fallible_t*) @ xpcom/string/nsReadableUtils.cpp#381
_Z17AppendUTF8toUTF16RK10nsACStringR9nsAString$void AppendUTF8toUTF16(nsACString*, nsAString*) @ xpcom/string/nsReadableUtils.cpp#344
Gecko_SetCounterStyleToSymbols @ layout/style/ServoBindings.cpp#1145 ### SafeArguments: <arg0>
```

And I have no idea what should I do with it...

Comment hidden (obsolete)

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

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

> There is still a hazard from this that:
> ```
> [27.28s] #97 Analyzing Gecko_SetCounterStyleToSymbols ...
> Error: Field write mozilla::detail::nsStringRepr.mLength nsAString.field:0
> Location: _ZN9nsAString9SetLengthEj$void nsAString::SetLength(uint32) @ xpcom/string/nsTSubstring.cpp#730
> Stack Trace:
> _Z17AppendUTF8toUTF16RK10nsACStringR9nsAStringRKN7mozilla10fallible_tE$uint8 AppendUTF8toUTF16(nsACString*, nsAString*, mozilla::fallible_t*) @ xpcom/string/nsReadableUtils.cpp#381
> _Z17AppendUTF8toUTF16RK10nsACStringR9nsAString$void AppendUTF8toUTF16(nsACString*, nsAString*) @ xpcom/string/nsReadableUtils.cpp#344
> Gecko_SetCounterStyleToSymbols @ layout/style/ServoBindings.cpp#1145 ### SafeArguments: <arg0>
> ```
> 
> And I have no idea what should I do with it...

I think I have a solution now...

Cameron McCormack (:heycam)

Comment 8

•

7 years 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+

Cameron McCormack (:heycam)

Comment 9

•

7 years 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+

Cameron McCormack (:heycam)

Comment 10

•

7 years 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+

Xidorn Quan [:xidorn] UTC+11

Assignee

Comment 11

•

7 years ago

Servo side: servo/servo#17060

Xidorn Quan [:xidorn] UTC+11

Assignee

Comment 12

•

7 years 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 on attachment 8871542 [details]
Bug 1363596 part 1 - Merge nsCOMPtr<nsIAtom> into CounterStylePtr.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143014/diff/1-2/

Comment hidden (mozreview-request)

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143016/diff/1-2/

Comment hidden (mozreview-request)

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143018/diff/1-2/

Mozilla Autoland

Comment 16

•

7 years 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)

Pulsebot

Comment 17

•

7 years 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

Phil Ringnalda (:philor)

Comment 18

•

7 years ago

bugherder

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

Closed: 7 years 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.