Bug 144761 – [Mac] Playback target clients do not unregister on page reload

• Status: ASSIGNED
• Component: Media

[Eric Carlson]

Playback target clients do not unregister on page reload

[Radar WebKit Bug Importer]

<rdar://problem/20862460>

[Eric Carlson]

Created attachment 252630 [details]
Proposed patch.

[WebKit Commit Bot]

Attachment 252630 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Comment on attachment 252630 [details]
Proposed patch.

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

r=me, with nit.

> Source/WebCore/dom/Document.cpp:2254
> +#if ENABLE(WIRELESS_PLAYBACK_TARGET)
> +    if (!m_clientToIDMap.isEmpty() && page()) {
> +        Vector<WebCore::MediaPlaybackTargetClient*> clients;
> +        copyKeysToVector(m_clientToIDMap, clients);
> +        for (auto client : clients)
> +            removePlaybackTargetPickerClient(*client);
> +    }
> +#endif

this can just be:

for (auto client : m_clientToIDMap.keys()) ....

[Brady Eidson]

Comment on attachment 252630 [details]
Proposed patch.

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

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

OOPs!

> Source/WebCore/ChangeLog:11
> +        (WebCore::Document::prepareForDestruction): Unregister all target picker clients.

Doing it in prepareForDestruction is reasonable, and fixes the letter of the bug, but not the spirit of the bug.

In Frame::setDocument():
    if (m_doc && !m_doc->inPageCache())
        m_doc->prepareForDestruction();

What about if the current document goes into the page cache?  Should probably be unregistered then as well.

Simple notifications: 
void Document::documentWillSuspendForPageCache()
void Document::documentDidResumeFromPageCache()

[Eric Carlson]

(In reply to comment #4)
> Comment on attachment 252630 [details]
> Proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252630&action=review
> 
> r=me, with nit.
> 
> > Source/WebCore/dom/Document.cpp:2254
> > +#if ENABLE(WIRELESS_PLAYBACK_TARGET)
> > +    if (!m_clientToIDMap.isEmpty() && page()) {
> > +        Vector<WebCore::MediaPlaybackTargetClient*> clients;
> > +        copyKeysToVector(m_clientToIDMap, clients);
> > +        for (auto client : clients)
> > +            removePlaybackTargetPickerClient(*client);
> > +    }
> > +#endif
> 
> this can just be:
> 
> for (auto client : m_clientToIDMap.keys()) ....

That won't work because .keys() returns an iterator and removePlaybackTargetPickerClient mutates the hash.

[Eric Carlson]

(In reply to comment #5)
> Comment on attachment 252630 [details]
> Proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252630&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        (WebCore::Document::prepareForDestruction): Unregister all target picker clients.
> 
> Doing it in prepareForDestruction is reasonable, and fixes the letter of the
> bug, but not the spirit of the bug.
> 
> In Frame::setDocument():
>     if (m_doc && !m_doc->inPageCache())
>         m_doc->prepareForDestruction();
> 
> What about if the current document goes into the page cache?  Should
> probably be unregistered then as well.
> 
> Simple notifications: 
> void Document::documentWillSuspendForPageCache()
> void Document::documentDidResumeFromPageCache()

Great suggestion, thanks!

[Eric Carlson]

Created attachment 252720 [details]
Proposed patch.

[Eric Carlson]

Created attachment 252721 [details]
Updated patch.

[Brady Eidson]

Comment on attachment 252721 [details]
Updated patch.

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

R+ with the guards fixed.

> Source/WebCore/html/HTMLMediaElement.cpp:465
> +#if ENABLE(WIRELESS_PLAYBACK_TARGET)
> +    document.registerForPageCacheSuspensionCallbacks(this);
> +#endif

Page cache registration is #if guarded here...

> Source/WebCore/html/HTMLMediaElement.cpp:485
> +#if ENABLE(WIRELESS_PLAYBACK_TARGET)
>      document.unregisterForVisibilityStateChangedCallbacks(this);
> +#endif

I think these #if guards were meant to go down below...

> Source/WebCore/html/HTMLMediaElement.cpp:497
> +    document.unregisterForPageCacheSuspensionCallbacks(this);

Unregistration is *not* #if guarded.

[Eric Carlson]

Created attachment 252725 [details]
Patch for landing.

[Eric Carlson]

Created attachment 252726 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 252726 [details]
Patch for landing.

Rejecting attachment 252726 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 252726, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/5435357743022080

[Eric Carlson]

Created attachment 252730 [details]
Patch for landing, this time with one less OOPs

[WebKit Commit Bot]

Comment on attachment 252730 [details]
Patch for landing, this time with one less OOPs

Clearing flags on attachment: 252730

Committed r184001: <http://trac.webkit.org/changeset/184001>