Created attachment 258269 [details] Patch

Created attachment 258277 [details] Patch

Comment on attachment 258277 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258277&action=review This looks good modulo my minor comments, but I am now a WK2 reviewer so someone else will have to give the official r+. > Source/WebCore/Modules/webaudio/AudioContext.cpp:344 > +#if ENABLE(MEDIA_SESSION) > + document()->updateIsPlayingMedia(HTMLMediaElementUnknownID); > +#else > + document()->updateIsPlayingMedia(0); > +#endif Nit: if you don't guard HTMLMediaElementUnknownID with "ENABLE(MEDIA_SESSION)" you won't need the #if here. Also, the name "HTMLMediaElementUnknownID" is not quite correct because it isn't an HTMLMediaElement at all. Maybe HTMLMediaElementInvalidID? > Source/WebCore/Modules/webaudio/AudioContext.cpp:1085 > +#if ENABLE(MEDIA_SESSION) > + strongThis->document()->updateIsPlayingMedia(HTMLMediaElementUnknownID); > +#else > + strongThis->document()->updateIsPlayingMedia(0); > +#endif Ditto. > Source/WebCore/dom/Document.cpp:3505 > +#if ENABLE(MEDIA_SESSION) > + updateIsPlayingMedia(HTMLMediaElementUnknownID); > +#else > + updateIsPlayingMedia(0); > +#endif Ditto. > Source/WebCore/dom/Document.cpp:3516 > +#if ENABLE(MEDIA_SESSION) > + updateIsPlayingMedia(HTMLMediaElementUnknownID); > +#else > + updateIsPlayingMedia(0); > +#endif Ditto. > Source/WebCore/dom/Document.cpp:3529 > + if (HTMLMediaElement* sourceElement = HTMLMediaElement::elementWithID(sourceElementID)) { > + if (sourceElement->isPlaying()) > + state |= MediaProducer::IsSourcePlaying; > + } Nit: you can prevent the failed lookup by checking for HTMLMediaElementUnknownID (or HTMLMediaElementInvalidID, or whatever you use). > Source/WebCore/page/MediaProducer.h:43 > +#if ENABLE(MEDIA_SESSION) > + IsSourcePlaying = 1 << 6, > +#endif I don't think you need to #if this flag. > Source/WebKit2/UIProcess/WebPageProxy.cpp:5947 > + if (focusManager->focusedMediaElementPage() == this && focusManager->focusedMediaElementID() == sourceElementID) > + focusManager->setFocusedMediaElementIsPlaying(state & MediaProducer::IsSourcePlaying); This seems like the wrong place for this logic. Why not pass the element ID into the focus manager so you can keep all of the focus logic there.

(In reply to comment #3) > Comment on attachment 258277 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=258277&action=review > > This looks good modulo my minor comments, but I am now a WK2 reviewer so > someone else will have to give the official r+. > > > Source/WebCore/Modules/webaudio/AudioContext.cpp:344 > > +#if ENABLE(MEDIA_SESSION) > > + document()->updateIsPlayingMedia(HTMLMediaElementUnknownID); > > +#else > > + document()->updateIsPlayingMedia(0); > > +#endif > > Nit: if you don't guard HTMLMediaElementUnknownID with > "ENABLE(MEDIA_SESSION)" you won't need the #if here. Removed it to simplify subsequent code (1). > > Also, the name "HTMLMediaElementUnknownID" is not quite correct because it > isn't an HTMLMediaElement at all. Maybe HTMLMediaElementInvalidID? Renamed. > > > Source/WebCore/Modules/webaudio/AudioContext.cpp:1085 > > +#if ENABLE(MEDIA_SESSION) > > + strongThis->document()->updateIsPlayingMedia(HTMLMediaElementUnknownID); > > +#else > > + strongThis->document()->updateIsPlayingMedia(0); > > +#endif > > Ditto. Simplified this after fixing (1). > > > Source/WebCore/dom/Document.cpp:3505 > > +#if ENABLE(MEDIA_SESSION) > > + updateIsPlayingMedia(HTMLMediaElementUnknownID); > > +#else > > + updateIsPlayingMedia(0); > > +#endif > > Ditto. Simplified this after fixing (1). > > > Source/WebCore/dom/Document.cpp:3516 > > +#if ENABLE(MEDIA_SESSION) > > + updateIsPlayingMedia(HTMLMediaElementUnknownID); > > +#else > > + updateIsPlayingMedia(0); > > +#endif > > Ditto. Simplified this after fixing (1). > > > Source/WebCore/dom/Document.cpp:3529 > > + if (HTMLMediaElement* sourceElement = HTMLMediaElement::elementWithID(sourceElementID)) { > > + if (sourceElement->isPlaying()) > > + state |= MediaProducer::IsSourcePlaying; > > + } > > Nit: you can prevent the failed lookup by checking for > HTMLMediaElementUnknownID (or HTMLMediaElementInvalidID, or whatever you > use). I prefer just checking if the pointer is null (even if it's logically equivalent to checking for an invalid ID). Feels safer. > > > Source/WebCore/page/MediaProducer.h:43 > > +#if ENABLE(MEDIA_SESSION) > > + IsSourcePlaying = 1 << 6, > > +#endif > > I don't think you need to #if this flag. Removed. > > > Source/WebKit2/UIProcess/WebPageProxy.cpp:5947 > > + if (focusManager->focusedMediaElementPage() == this && focusManager->focusedMediaElementID() == sourceElementID) > > + focusManager->setFocusedMediaElementIsPlaying(state & MediaProducer::IsSourcePlaying); > > This seems like the wrong place for this logic. Why not pass the element ID > into the focus manager so you can keep all of the focus logic there. Refactored.

Created attachment 258291 [details] Patch

Comment on attachment 258291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258291&action=review > Source/WebCore/dom/Document.h:1263 > + WEBCORE_EXPORT void updateIsPlayingMedia(uint64_t); How does this work if more than one media element is playing?

(In reply to comment #6) > Comment on attachment 258291 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=258291&action=review > > > Source/WebCore/dom/Document.h:1263 > > + WEBCORE_EXPORT void updateIsPlayingMedia(uint64_t); > > How does this work if more than one media element is playing? Every time any media element changes its 'playing' state, that information propagates to the UI process. The UI process keeps track of the currently focused media element belonging to a "Content" Media Session (there can only be one at a time). If the element IDs match, we can cache the 'playing' state as the focused media element's playing state.

Comment on attachment 258291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258291&action=review >>> Source/WebCore/dom/Document.h:1263 >>> + WEBCORE_EXPORT void updateIsPlayingMedia(uint64_t); >> >> How does this work if more than one media element is playing? > > Every time any media element changes its 'playing' state, that information propagates to the UI process. The UI process keeps track of the currently focused media element belonging to a "Content" Media Session (there can only be one at a time). If the element IDs match, we can cache the 'playing' state as the focused media element's playing state. Please make a typedef for this uint64_t (MediaElementID or whatever), and convert the current code to use it. You can do this in a separate patch. Give this a default argument, so you don't have to specify HTMLMediaElementInvalidID everywhere. > Source/WebCore/html/HTMLMediaElement.cpp:3177 > +#if ENABLE(MEDIA_SESSION) > + document().updateIsPlayingMedia(m_elementID); > +#else > + document().updateIsPlayingMedia(0); > +#endif This #ifdef is confusing. Why can't you always pass m_elementID? > Source/WebCore/html/HTMLMediaElement.cpp:4545 > +#if ENABLE(MEDIA_SESSION) > + document().updateIsPlayingMedia(m_elementID); > +#else > + document().updateIsPlayingMedia(0); > +#endif Ditto. > Source/WebCore/html/HTMLMediaElement.cpp:4810 > +#if ENABLE(MEDIA_SESSION) > + document().updateIsPlayingMedia(m_elementID); > +#else > + document().updateIsPlayingMedia(0); > +#endif Ditto.

(In reply to comment #8) > Comment on attachment 258291 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=258291&action=review > > >>> Source/WebCore/dom/Document.h:1263 > >>> + WEBCORE_EXPORT void updateIsPlayingMedia(uint64_t); > >> > >> How does this work if more than one media element is playing? > > > > Every time any media element changes its 'playing' state, that information propagates to the UI process. The UI process keeps track of the currently focused media element belonging to a "Content" Media Session (there can only be one at a time). If the element IDs match, we can cache the 'playing' state as the focused media element's playing state. > > Please make a typedef for this uint64_t (MediaElementID or whatever), and > convert the current code to use it. You can do this in a separate patch. Filed Bug 147709. > > Give this a default argument, so you don't have to specify > HTMLMediaElementInvalidID everywhere. Done. > > > Source/WebCore/html/HTMLMediaElement.cpp:3177 > > +#if ENABLE(MEDIA_SESSION) > > + document().updateIsPlayingMedia(m_elementID); > > +#else > > + document().updateIsPlayingMedia(0); > > +#endif > > This #ifdef is confusing. Why can't you always pass m_elementID? Element IDs are only assigned under the Media Session feature flag. With the default argument added, though, the confusing 0 argument is now gone. > > > Source/WebCore/html/HTMLMediaElement.cpp:4545 > > +#if ENABLE(MEDIA_SESSION) > > + document().updateIsPlayingMedia(m_elementID); > > +#else > > + document().updateIsPlayingMedia(0); > > +#endif > > Ditto. > > > Source/WebCore/html/HTMLMediaElement.cpp:4810 > > +#if ENABLE(MEDIA_SESSION) > > + document().updateIsPlayingMedia(m_elementID); > > +#else > > + document().updateIsPlayingMedia(0); > > +#endif > > Ditto.

Committed r188030: <http://trac.webkit.org/changeset/188030>

(In reply to comment #10) > Committed r188030: <http://trac.webkit.org/changeset/188030> It broke the WinCairo build: WebCore.lib(DOMAllInOne.cpp.obj) : error LNK2001: unresolved external symbol "unsigned __int64 const WebCore::HTMLMediaElementInvalidID" (?HTMLMediaElementInvalidID@WebCore@@3_KB)

HTMLMediaElementInvalidID is defined inside of ENABLE(VIDEO) in HTMLMediaElement.cpp and used in Document.h outside of ENABLE(VIDEO). Fixed build in http://trac.webkit.org/changeset/188041

(In reply to comment #12) > HTMLMediaElementInvalidID is defined inside of ENABLE(VIDEO) in > HTMLMediaElement.cpp and used in Document.h outside of ENABLE(VIDEO). > Fixed build in http://trac.webkit.org/changeset/188041 Thanks, I guess we have VIDEO enabled on all our build bots.