Closed Bug 1382137 Opened 7 years ago Closed 7 years ago

stylo: make 'list-style-type' animatable

Categories

(Core :: CSS Parsing and Computation, enhancement, P2)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: daisuke, Assigned: daisuke)

References

Details

Attachments

(5 files)

Bug 1382137 - Part 1: implement conversion method from Gecko CounterStylePtr to CounterStyleOrNone.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1382137 - Part 2: make list-style-type animatable.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1382137 - Part 3: remove test fail annotations from meta in wpt.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1382137 - Part 4: add Gecko_CounterStyle_GetName and Gecko_CounterStyle_GetName which have a writable paramter into whitelist in analyzeHeapWrites.js.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Servo PR 17985
41 bytes, text/x-github-pull-request
Details | Review 
'list-style-type' should animate discretely.
Priority: -- → P2
Comment on attachment 8893672 [details]
Bug 1382137 - Part 3: remove test fail annotations from meta in wpt.

https://reviewboard.mozilla.org/r/164764/#review170164
Attachment #8893672 - Flags: review?(hikezoe) → review+
Comment on attachment 8893670 [details]
Bug 1382137 - Part 1: implement conversion method from Gecko CounterStylePtr to CounterStyleOrNone.

https://reviewboard.mozilla.org/r/164760/#review170166

::: layout/style/ServoBindings.h:334
(Diff revision 1)
>                                      uint32_t symbols_count);
>  void Gecko_SetCounterStyleToString(mozilla::CounterStylePtr* ptr,
>                                     const nsACString* symbol);
>  void Gecko_CopyCounterStyle(mozilla::CounterStylePtr* dst,
>                              const mozilla::CounterStylePtr* src);
> +bool Gecko_CounterStyle_IsNone(const mozilla::CounterStylePtr* aPtr);

Use 'ptr' for consistency.

::: layout/style/ServoBindings.cpp:1453
(Diff revision 1)
>  {
>    *aDst = *aSrc;
>  }
>  
> +bool
> +Gecko_CounterStyle_IsNone(const mozilla::CounterStylePtr* aPtr) {

we don't need 'mozilla::' here.

::: layout/style/ServoBindings.cpp:1461
(Diff revision 1)
> +}
> +
> +bool
> +Gecko_CounterStyle_IsName(const mozilla::CounterStylePtr* aPtr) {
> +  MOZ_ASSERT(aPtr);
> +  return !(*aPtr)->AsAnonymous();

!(*Ptr)->AsAnonymous() && !(*aPtr)->IsNone() ?

::: servo/components/style/gecko/values.rs:473
(Diff revision 1)
> +        } else {
> +            let system = unsafe { Gecko_CounterStyle_GetSystem(gecko_value) };
> +            let symbol_type = SymbolsType::from_gecko_keyword(system as u32);
> +            let mut symbols = vec![];
> +            let length = unsafe { Gecko_CounterStyle_GetSymbolsLength(gecko_value) };
> +            for index in 0..length {

This seems inefficient. We should call one single FFI and get Symbols array and iterate array in servo side?
Attachment #8893670 - Flags: review?(hikezoe)
Comment on attachment 8893671 [details]
Bug 1382137 - Part 2: make list-style-type animatable.

https://reviewboard.mozilla.org/r/164762/#review170210
Attachment #8893671 - Flags: review?(hikezoe) → review+
Gecko_CounterStyle_GetSingleStringThe try tells that we need to update analyzeHeapWrites.js.  Gecko_CounterStyle_GetName and Gecko_CounterStyle_GetSingleString.
Oops, wrong paste.
Thanks Hiro.

Sorry, may I ask one question?
How did you specify where the problem is?
linux64-haz in the try told us there were 6 heap write hazards instead of 4.  So I just checked the new FFIs in your patch that write argument's value.
Oh, I see, thanks!
Comment on attachment 8894367 [details]
Bug 1382137 - Part 4: add Gecko_CounterStyle_GetName and Gecko_CounterStyle_GetName which have a writable paramter into whitelist in analyzeHeapWrites.js.

https://reviewboard.mozilla.org/r/165534/#review170604

::: commit-message-fdfe1:1
(Diff revision 1)
> +Bug 1382137 - Part 4: add new FFIs which have a writable paramter into whitelist in analyzeHeapWrites.js. r?hiro

Should write function names respectively instead of just saying 'new FFIs'.

::: commit-message-fdfe1:3
(Diff revision 1)
> +Bug 1382137 - Part 4: add new FFIs which have a writable paramter into whitelist in analyzeHeapWrites.js. r?hiro
> +
> +Since we add new FFI to Servo bindings.

This is not a reason why we added the function names here.
Attachment #8894367 - Flags: review?(hikezoe) → review+
Comment on attachment 8893670 [details]
Bug 1382137 - Part 1: implement conversion method from Gecko CounterStylePtr to CounterStyleOrNone.

https://reviewboard.mozilla.org/r/164760/#review170606

::: layout/style/ServoBindings.cpp:1472
(Diff revision 3)
> +  MOZ_ASSERT(!Gecko_CounterStyle_IsNone(aPtr) &&
> +             !Gecko_CounterStyle_IsName(aPtr));

AsAnonymous() is mutually exclusive these conditions? If so, we should just check AsAnonymous(). Or expost IsAnonymous() in public and use it.

::: layout/style/ServoBindings.cpp:1481
(Diff revision 3)
> +  MOZ_ASSERT(!Gecko_CounterStyle_IsNone(aPtr) &&
> +             !Gecko_CounterStyle_IsName(aPtr));

Same as the above assertion.
Attachment #8893670 - Flags: review?(hikezoe) → review+
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 5e46cc657c38 -d c2422a605996: rebasing 411966:5e46cc657c38 "Bug 1382137 - Part 1: implement conversion method from Gecko CounterStylePtr to CounterStyleOrNone. r=hiro"
rebasing 411967:4b5fc213b19e "Bug 1382137 - Part 2: make list-style-type animatable. r=hiro"
rebasing 411968:2dc33cb6342e "Bug 1382137 - Part 3: remove test fail annotations from meta in wpt. r=hiro"
merging testing/web-platform/meta/web-animations/animation-model/animation-types/accumulation-per-property.html.ini
warning: conflicts while merging testing/web-platform/meta/web-animations/animation-model/animation-types/accumulation-per-property.html.ini! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a00698496e8c
Part 1: implement conversion method from Gecko CounterStylePtr to CounterStyleOrNone. r=hiro
https://hg.mozilla.org/integration/autoland/rev/0572352c4de4
Part 2: make list-style-type animatable. r=hiro
https://hg.mozilla.org/integration/autoland/rev/5a21057b51a7
Part 3: remove test fail annotations from meta in wpt. r=hiro
https://hg.mozilla.org/integration/autoland/rev/3e7f81525e57
Part 4: add Gecko_CounterStyle_GetName and Gecko_CounterStyle_GetName which have a writable paramter into whitelist in analyzeHeapWrites.js. r=hiro
Attached file Servo PR 17985 — 
Depends on: 1388652
Depends on: 1390510

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: