Closed Bug 1300045 Opened 4 years ago Closed 4 years ago

Move KeyframeEffectReadOnly into KeyframeEffectReadOnly.cpp


(Core :: DOM: Animation, defect, P3)




Tracking Status
firefox51 --- fixed


(Reporter: birtles, Assigned: birtles)



(2 files)

It's a bit confusing the KeyframeEffect and KeyframeEffectReadOnly both live in KeyframeEffect.cpp. It would be easier to find KeyframeEffectReadOnly's implementation if it lives in KeyframeEffectReadOnly.cpp and would also make the files smaller--KeyframeEffect.cpp would be just for adding the missing setters.
I went to split KeyframeEffect.webidl as well but came across trouble because:

1. Both KeyframeEffect.webidl and KeyframeEffectReadOnly.webidl include the following union:
   (unrestricted double or KeyframeEffectOptions)
2. As a result includes KeyframeEffectReadOnlyBinding.h in UnionTypes.h[1]
3. AnimationEffectReadOnlyBinding.h includes UnionTypes.h
4. KeyframeEffectReadOnlyBinding.h includes AnimationEffectReadOnlyBinding.h

As a result, there's a cyclic dependency so when we go to include AnimationEffectReadOnlyBinding.h from UnionTypes.h, it lacks the definitions for the union types it needs like OwningUnrestrictedDoubleOrString.

We could factor out (unrestricted double or KeyframeEffectOptions) into a typedef in a separate file I guess. Or perhaps it would be enough to move KeyframeEffectOptions to a separate file? For now I've just left everything in KeyframeEffect.webidl.

Attachment #8787517 - Flags: review?(hiikezoe)
Attachment #8787518 - Flags: review?(hiikezoe)
Attachment #8787518 - Flags: review?(bugs)
Olli, would you mind reviewing just the webidl changes in part 2? Thanks.
Regarding the changes to some of the includes, I've generally been taking the approach that for:

  #include "ABC.h" // for ABC

we don't need the "for ABC" since it is redundant and just clutters the file. Instead I've been mostly following the following pattern:

  #include "ABC.h"
  #include "ABC.h" // for XYZ
  #include "ABC.h" // for ABC::SubType
Comment on attachment 8787518 [details]
Bug 1300045 part 2 - Split KeyframeEffect.cpp into KeyframeEffect{ReadOnly}.cpp

r+ for the webidl
Attachment #8787518 - Flags: review?(bugs) → review+
Comment on attachment 8787517 [details]
Bug 1300045 part 1 - Tidy up some missing includes
Attachment #8787517 - Flags: review?(hiikezoe) → review+
Comment on attachment 8787518 [details]
Bug 1300045 part 2 - Split KeyframeEffect.cpp into KeyframeEffect{ReadOnly}.cpp

::: dom/animation/Animation.h:19
(Diff revision 1)
> -#include "mozilla/dom/KeyframeEffect.h" // for KeyframeEffectReadOnly
> -#include "mozilla/dom/Promise.h" // for Promise
> +#include "mozilla/DOMEventTargetHelper.h"
> +#include "mozilla/dom/Promise.h"

Nit:  I think DOMEventTargetHelper.h may be prior dom/XX.h.
I am not sure about our sorting rules of include headers though.
Attachment #8787518 - Flags: review?(hiikezoe) → review+
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.