Move KeyframeEffectReadOnly into KeyframeEffectReadOnly.cpp

RESOLVED FIXED in Firefox 51

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

Trunk
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(2 attachments)

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 Codegen.py 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.

[1] https://dxr.mozilla.org/mozilla-central/rev/3ba5426a03b495b6417fffb872d42874edb80855/dom/bindings/Codegen.py#1224
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

https://reviewboard.mozilla.org/r/76240/#review74344

r+ for the webidl
Attachment #8787518 - Flags: review?(bugs) → review+
Comment on attachment 8787517 [details]
Bug 1300045 part 1 - Tidy up some missing includes

https://reviewboard.mozilla.org/r/76238/#review74354
Attachment #8787517 - Flags: review?(hiikezoe) → review+
Comment on attachment 8787518 [details]
Bug 1300045 part 2 - Split KeyframeEffect.cpp into KeyframeEffect{ReadOnly}.cpp

https://reviewboard.mozilla.org/r/76240/#review74362

::: 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+
https://hg.mozilla.org/mozilla-central/rev/8739a7982cee
https://hg.mozilla.org/mozilla-central/rev/0768e4f23ebf
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.