Bug 74165 – Fix inter-document interactions in requestAnimationFrame servicing algorithm

• Status: NEW
• Component: New Bugs

[James Robinson]

Fix inter-document interactions in requestAnimationFrame servicing algorithm

[James Robinson]

Created attachment 118532 [details]
Patch

[James Robinson]

Posting for initial feedback only, this isn't ready to land without more tests and updates to the non-chromium build systems.

[James Robinson]

Oh, this also drops the element parameter. We haven't been able to reach agreement on this issue in the WebPerf WG and I've never got around to implementing it in a way that would actually help users. Keeping the half-done implementation around would make things insanely more complicated without providing any real benefit to users, so I've taken it out for now. If we can come to some agreement in the standard and/or someone has time to implement a better version of it we can add it back in. I don't believe this refactor will make things any more difficult to add this back in if someone were to write a good version of the visibility checks.

[Simon Fraser (smfr)]

Can you explain a bit more what the current inter-document issues are?

[Chris Marrin]

I have a few preliminary comments.

1) It looks like this is a redesign of the logic, getting rid of the Element param, which has the side effect of fixing some bug. What bug does it fix? Can you supply a test case? If it's not fixing any known bugs, please rename the title from Fix… to Restructure… or something and supply some explanation of the redesign.

2) Please split the patch into at least 3 pieces: a) the move of ScriptedAnimationController, b) the rename of the function calls, c) the addition of the new CallbackList class, d) the API change to get rid of Element*. It will make it much more clear to review

3) I don't see the moved ScriptedAnimationController represented in any but the gypi build file. I assume that's an error. ScriptedAnimationController is at least still needed in the xcode build.

4) Are the changes to InspectorInstrumentation needed by this patch? 

I know this is a preliminary patch. I'm just mentioning the issues I see.

[James Robinson]

(In reply to comment #4)
> Can you explain a bit more what the current inter-document issues are?

I expect the test cases (coming shortly) will cover this(In reply to comment #5)

> I have a few preliminary comments.
> 
> 1) It looks like this is a redesign of the logic, getting rid of the Element param, which has the side effect of fixing some bug. What bug does it fix? Can you supply a test case? If it's not fixing any known bugs, please rename the title from Fix… to Restructure… or something and supply some explanation of the redesign.
> 

I covered the element bit in comment #3

> 2) Please split the patch into at least 3 pieces: a) the move of ScriptedAnimationController, b) the rename of the function calls, c) the addition of the new CallbackList class, d) the API change to get rid of Element*. It will make it much more clear to review
> 
> 3) I don't see the moved ScriptedAnimationController represented in any but the gypi build file. I assume that's an error. ScriptedAnimationController is at least still needed in the xcode build.
> 

I covered this in comment #2

> 4) Are the changes to InspectorInstrumentation needed by this patch? 

Yes, or it won't compile.  All it is really doing it changing from passing a Document* to InspectorInstrumentation and having it walk document->page() to passing a Page* to InspectorInstrementation.

[Chris Marrin]

(In reply to comment #6)
> (In reply to comment #4)
> > Can you explain a bit more what the current inter-document issues are?
> 
> I expect the test cases (coming shortly) will cover this(In reply to comment #5)
> 
> > I have a few preliminary comments.
> > 
> > 1) It looks like this is a redesign of the logic, getting rid of the Element param, which has the side effect of fixing some bug. What bug does it fix? Can you supply a test case? If it's not fixing any known bugs, please rename the title from Fix… to Restructure… or something and supply some explanation of the redesign.
> > 
> 
> I covered the element bit in comment #3

I see that. But what is the rationale behind that change, for those of us who aren't on the list. What did element buy you in the first place? What features do we lose if we remove it?

> 
> > 2) Please split the patch into at least 3 pieces: a) the move of ScriptedAnimationController, b) the rename of the function calls, c) the addition of the new CallbackList class, d) the API change to get rid of Element*. It will make it much more clear to review
> > 
> > 3) I don't see the moved ScriptedAnimationController represented in any but the gypi build file. I assume that's an error. ScriptedAnimationController is at least still needed in the xcode build.
> > 
> 
> I covered this in comment #2

Right, sorry. I was confused by the fact that they had some of the changes but not all…

> 
> > 4) Are the changes to InspectorInstrumentation needed by this patch? 
> 
> Yes, or it won't compile.  All it is really doing it changing from passing a Document* to InspectorInstrumentation and having it walk document->page() to passing a Page* to InspectorInstrementation.

Right, that's a small change that doesn't add to the noise much. But please try to do the other restructuring in separate patches to make it more clear what the real patch is doing...

[James Robinson]

(In reply to comment #5)
> 2) Please split the patch into at least 3 pieces:
> b) the rename of the function calls,

https://bugs.webkit.org/show_bug.cgi?id=74231

> d) the API change to get rid of Element*. It will make it much more clear to review

https://bugs.webkit.org/show_bug.cgi?id=74232


> a) the move of ScriptedAnimationController,
> c) the addition of the new CallbackList class,

Haven't got to split these out yet. I think these two steps will have to be done in the same patch, but I'll see what I can do to keep things small and easily reviewable.

Thanks for the feedback so far.  I'll leave this patch here so people can take a look at the big picture if they like and work on most or perhaps all of the logic in dependent bugs.