Bug 196458 - Remove legacy webkitRequestAnimationFrame time quirk
Summary: Remove legacy webkitRequestAnimationFrame time quirk
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-04-01 12:47 PDT by Simon Fraser (smfr)
Modified: 2019-04-03 10:27 PDT (History)
11 users (show)

See Also:


Attachments
Patch (5.41 KB, patch)
2019-04-02 12:36 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (5.86 KB, patch)
2019-04-02 12:37 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (5.80 KB, patch)
2019-04-03 09:22 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2019-04-01 12:47:33 PDT
We should remove m_useLegacyTimeBase from webkitRequestAnimationFrame() (and eventually remove webkitRequestAnimationFrame altogether).
Comment 1 Radar WebKit Bug Importer 2019-04-01 12:48:24 PDT
<rdar://problem/49490207>
Comment 2 Chris Dumez 2019-04-02 12:36:17 PDT
Created attachment 366524 [details]
Patch
Comment 3 Chris Dumez 2019-04-02 12:37:26 PDT
Created attachment 366525 [details]
Patch
Comment 4 Said Abou-Hallawa 2019-04-02 15:51:52 PDT
Comment on attachment 366525 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366525&action=review

> Source/WebCore/dom/ScriptedAnimationController.cpp:-219
> -            if (callback->m_useLegacyTimeBase)
> -                callback->handleEvent(legacyHighResNowMs);
> -            else
> -                callback->handleEvent(highResNowMs);

This change will conflict with my patch in https://bugs.webkit.org/show_bug.cgi?id=177484. But this is fine I will resolve the conflict if this patches before mine. Actually this code is confusing and it is good that we are going to remove it.
Comment 5 Simon Fraser (smfr) 2019-04-02 17:41:20 PDT
Comment on attachment 366525 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366525&action=review

> Source/WebCore/page/DOMWindow.cpp:1707
> +    if (auto* document = this->document())
> +        document->addConsoleMessage(MessageSource::JS, MessageLevel::Warning, "webkitRequestAnimationFrame() is deprecated and will be removed. Please use requestAnimationFrame() instead."_s);

Maybe this should be logged only once per document per load?
Comment 6 Chris Dumez 2019-04-02 18:23:48 PDT
(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 366525 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366525&action=review
> 
> > Source/WebCore/page/DOMWindow.cpp:1707
> > +    if (auto* document = this->document())
> > +        document->addConsoleMessage(MessageSource::JS, MessageLevel::Warning, "webkitRequestAnimationFrame() is deprecated and will be removed. Please use requestAnimationFrame() instead."_s);
> 
> Maybe this should be logged only once per document per load?

Do we have any precedent for doing this? I thought we usually logged on every call.
Comment 7 Simon Fraser (smfr) 2019-04-02 18:46:57 PDT
(In reply to Chris Dumez from comment #6)
> (In reply to Simon Fraser (smfr) from comment #5)
> > Comment on attachment 366525 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=366525&action=review
> > 
> > > Source/WebCore/page/DOMWindow.cpp:1707
> > > +    if (auto* document = this->document())
> > > +        document->addConsoleMessage(MessageSource::JS, MessageLevel::Warning, "webkitRequestAnimationFrame() is deprecated and will be removed. Please use requestAnimationFrame() instead."_s);
> > 
> > Maybe this should be logged only once per document per load?
> 
> Do we have any precedent for doing this? I thought we usually logged on
> every call.

    static bool firstTime = true;
    if (firstTime && context().scriptExecutionContext()) {
        context().scriptExecutionContext()->addConsoleMessage(MessageSource::JS, MessageLevel::Warning, "AudioBufferSourceNode 'looping' attribute is deprecated.  Use 'loop' instead."_s);
        firstTime = false;
    }
Comment 8 Chris Dumez 2019-04-02 18:53:35 PDT
(In reply to Simon Fraser (smfr) from comment #7)
> (In reply to Chris Dumez from comment #6)
> > (In reply to Simon Fraser (smfr) from comment #5)
> > > Comment on attachment 366525 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=366525&action=review
> > > 
> > > > Source/WebCore/page/DOMWindow.cpp:1707
> > > > +    if (auto* document = this->document())
> > > > +        document->addConsoleMessage(MessageSource::JS, MessageLevel::Warning, "webkitRequestAnimationFrame() is deprecated and will be removed. Please use requestAnimationFrame() instead."_s);
> > > 
> > > Maybe this should be logged only once per document per load?
> > 
> > Do we have any precedent for doing this? I thought we usually logged on
> > every call.
> 
>     static bool firstTime = true;
>     if (firstTime && context().scriptExecutionContext()) {
>        
> context().scriptExecutionContext()->addConsoleMessage(MessageSource::JS,
> MessageLevel::Warning, "AudioBufferSourceNode 'looping' attribute is
> deprecated.  Use 'loop' instead."_s);
>         firstTime = false;
>     }

Ugh, this relies on a static which means it would survive loads and potentially not log when it should.
Comment 9 Chris Dumez 2019-04-03 09:22:26 PDT
Created attachment 366613 [details]
Patch
Comment 10 WebKit Commit Bot 2019-04-03 10:27:25 PDT
Comment on attachment 366613 [details]
Patch

Clearing flags on attachment: 366613

Committed r243810: <https://trac.webkit.org/changeset/243810>
Comment 11 WebKit Commit Bot 2019-04-03 10:27:27 PDT
All reviewed patches have been landed.  Closing bug.