[webkit-reviews] review denied: [Bug 76063] Suspend/Resume API for pausing timers and animations : [Attachment 123541] Patch

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has denied Allan Sandfeld <sandfeld at kde.org>'s
request for review:
Bug 76063: Suspend/Resume API for pausing timers and animations
https://bugs.webkit.org/show_bug.cgi?id=76063

Attachment 123541: Patch
https://bugs.webkit.org/attachment.cgi?id=123541&action=review

------- Additional Comments from Brady Eidson <beidson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=123541&action=review


In general what I see here is two patches.

One is add WebKit2 API that there's not unanimous agreement we should have. 
That's what this bug represents.
The other is a WebCore patch that fixes
https://bugs.webkit.org/show_bug.cgi?id=53733.

I know all you care about is the WebKit2 API, but I would really like to see
you split off all the WebCore stuff that creates suspending DOM objects, etc. 
Get it reviewed in https://bugs.webkit.org/show_bug.cgi?id=53733.  Do *not*
include the new enum as that is truly part of supporting the WK2 API.

That will make both separate patches easier to review and will also make your
case that the API should be included much easier to make as the patch will be
smaller and more focused on that goal.

> Source/WebCore/ChangeLog:14
> +	   * dom/ActiveDOMObject.h:
> +
> +	       New ReasonForSuspension: DocumentWillBePaused.
> +	       Identical to name in iOS branch of WebCore

Comments for a change normally start on the line of the file where the change
came from.
* dome/ActiveDOMObject.h: New ReasonForSuspension...

At the very least the description shouldn't have an empty line between the file
and the description.  

Same for the whole ChangeLog

> Source/WebCore/dom/ActiveDOMObject.h:66
> -	       DocumentWillBecomeInactive
> +	       DocumentWillBecomeInactive,
> +	       DocumentWillBePaused

Naming!
Pretend I don't know this code at all and I walk up to WebCore for the first
time and look at these enums.  What is the difference between Inactive and
Paused?

Also it would be "BecomePaused" to follow "BecomeInactive", but I still hate
having both "paused" and "inactive" in the same set of enums.