Bug 82930

For more information on the BlackBerry side of things, see PR96004.

Created attachment 135166 [details] Patch

I'm wondering if a test is needed for this patch. Since it involves tabs and tab visibility, I'm assuming that any test would need to be a BlackBerry-only manual test.

could not the PAGE_VISIBILITY stuff we just enabled to BlackBerry port help somehow here?

(In reply to comment #4) > could not the PAGE_VISIBILITY stuff we just enabled to BlackBerry port help somehow here? Yeah the WebPagePrivate::setVisible property shows whether the tab is the selected one.

(In reply to comment #5) > (In reply to comment #4) > > could not the PAGE_VISIBILITY stuff we just enabled to BlackBerry port help somehow here? > > Yeah the WebPagePrivate::setVisible property shows whether the tab is the selected one. Note that, as described in comment #0, the intention of this patch is to let the platform media code know whether the tab it is on is the selected one. So I believe that PAGE_VISIBILITY does not apply here because it also seems to tie into the the activation state. Responding to the App's activation state is already handled for HTML5 media using a separate mechanism, and is not within the scope of this bug.

I will write a manual test in ManualTests/blackberry and I'm hoping that will be enough to give this bug an r+

(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > could not the PAGE_VISIBILITY stuff we just enabled to BlackBerry port help somehow here? > > > > Yeah the WebPagePrivate::setVisible property shows whether the tab is the selected one. > > Note that, as described in comment #0, the intention of this patch is to let the platform media code know whether the tab it is on is the selected one. So I believe that PAGE_VISIBILITY does not apply here because it also seems to tie into the the activation state. Responding to the App's activation state is already handled for HTML5 media using a separate mechanism, and is not within the scope of this bug. So the difference here is that from your "tab awareness" perspective, a tab can has to be the current selected tab, no matter if it is in thumbnail view mode, or the app itself is in background, etc. Whereas from a "page visibility" perspective, the tab (page) has to be the current selected one, and in active state, i.e. has focus and user can interact with it. Does it sound correct?

(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > could not the PAGE_VISIBILITY stuff we just enabled to BlackBerry port help somehow here? > > > > Yeah the WebPagePrivate::setVisible property shows whether the tab is the selected one. > > Note that, as described in comment #0, the intention of this patch is to let the platform media code know whether the tab it is on is the selected one. So I believe that PAGE_VISIBILITY does not apply here because it also seems to tie into the the activation state. Responding to the App's activation state is already handled for HTML5 media using a separate mechanism, and is not within the scope of this bug. PAGE_VISIBILITY takes both tab selection and application activation state into account. However WebPage's visible property only takes tab selection into account, not activation state. Therefore it seems like you can use the latter.

(In reply to comment #8) > So the difference here is that from your "tab awareness" perspective, a tab can has to be the current selected tab, no matter if it is in thumbnail view mode, or the app itself is in background, etc. Whereas from a "page visibility" perspective, the tab (page) has to be the current selected one, and in active state, i.e. has focus and user can interact with it. > > > Does it sound correct? Yes, that is what I'm trying to say. Only the tab selection matters.

Created attachment 135697 [details] Patch

My new manual test is a modification of an existing one which has not been upstreamed yet, but you can see it here: r? webkit/001b6c8

Comment on attachment 135697 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135697&action=review > Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:118 > + void* tabId = 0; > + if (frameView() && frameView()->hostWindow()) > + tabId = frameView()->hostWindow()->platformPageClient(); > +#if USE(ACCELERATED_COMPOSITING) > + m_platformPlayer = new MMRPlayer(this, tabId, true); > +#else > + m_platformPlayer = new MMRPlayer(this, tabId, false); > +#endif could you help me to understand how the tabId is used here?

(In reply to comment #13) > could you help me to understand how the tabId is used here? The tabId is just a unique identifier that the MMRPlayer BlackBerryPlatform class uses to group media elements (aka players) by tab. It does not ever dereference the pointer. Media elements on the same tab will have the same tabId, media elements on different tabs will have different tabId's. That's all. For details on how the MMRPlayer class does this grouping see my already reviewed patch: platform/982d560

Created attachment 137402 [details] Patch

Comment on attachment 137402 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137402&action=review > Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:728 > + if (frameView() && frameView()->hostWindow()) > + return frameView()->hostWindow()->platformPageClient()->isVisible(); It's a layering violation for code in platform/ to know about code in page/

Comment on attachment 137402 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137402&action=review > Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:115 > + tabId = frameView()->hostWindow()->platformPageClient(); This is a layering violation, code in WebCore/platform should know about code in /page. > Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:697 > + rc = frameView()->hostWindow()->platformPageClient()->showAlertDialog(atype); Ditto. >> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:728 >> + return frameView()->hostWindow()->platformPageClient()->isVisible(); > > It's a layering violation for code in platform/ to know about code in page/ Ditto. > LayoutTests/ChangeLog:11 > + * media/audio-concurrent-supported.html: Added. This test looks platform specific. Why is it in the common media test directory?

Comment on attachment 137402 [details] Patch Attachment 137402 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12417257 New failing tests: media/audio-concurrent-supported.html

Created attachment 137414 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 137429 [details] Patch

(In reply to comment #17) > > Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:115 > > + tabId = frameView()->hostWindow()->platformPageClient(); > > This is a layering violation, code in WebCore/platform should know about code in /page. Can you please be more specific about the layering violation? Accessing the platformPageClient() via hostWindow() seems to be fairly common in several of the major ports. > > LayoutTests/ChangeLog:11 > > + * media/audio-concurrent-supported.html: Added. > > This test looks platform specific. Why is it in the common media test directory? Can you please be more specific about which part of the test is platform specific? It passes for all major ports.

(In reply to comment #21) > (In reply to comment #17) > > > Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:115 > > > + tabId = frameView()->hostWindow()->platformPageClient(); > > > > This is a layering violation, code in WebCore/platform should know about code in /page. > > Can you please be more specific about the layering violation? > Files in WebCore/platform should not reference WebCore files outside of platform. You can see a discussion in this email thread from earlier this year: https://lists.webkit.org/pipermail/webkit-dev/2012-January/019033.html Adam Barth summed it up nicely I think: Something should go into Platform if it's an abstraction of a facility provided by the underlying platform. For example, ScrollableArea is an abstraction of a concept provided by most platforms. Something should not go in Platform if it refers to web concepts (e.g., a Frame, which belongs in WebCore proper) or if it's something that might be used by every program (e.g., RefPtr, which belongs in WTF). [ SNIP ] Platform can depend on third-party libraries (e.g., SQLite) or on operating system facilities. Platform can depend on WTF, but no other code in the WebKit project. > Accessing the platformPageClient() via hostWindow() seems to be fairly common in several of the major ports. I see now that it is used by BlackBerry, Cairo, EFL, and Qt: [dev:WebCore/platform/graphics] eric% find . -name "*.cpp" | xargs grep -r "platformPageClient" ./blackberry/MediaPlayerPrivateBlackBerry.cpp: rc = topdoc->view()->hostWindow()->platformPageClient()->showAlertDialog(atype); ./blackberry/MediaPlayerPrivateBlackBerry.cpp: return frameView()->hostWindow()->platformPageClient()->platformWindow(); ./cairo/GraphicsContext3DPrivate.cpp: , m_glContext(GLContext::createOffscreenContext(GLContext::getContextForWidget(m_window->platformPageClient()))) ./efl/GraphicsContext3DPrivate.cpp: PageClientEfl* pageClient = static_cast<PageClientEfl*>(hostWindow->platformPageClient()); ./qt/GraphicsContext3DQt.cpp: QWebPageClient* webPageClient = m_hostWindow->platformPageClient(); This doesn't change the fact that this is a layering violation, which we strive to avoid in WebKit. > > > > LayoutTests/ChangeLog:11 > > > + * media/audio-concurrent-supported.html: Added. > > > > This test looks platform specific. Why is it in the common media test directory? > > Can you please be more specific about which part of the test is platform specific? It passes for all major ports. My mistake. I assumed that a test for a BlackBerry specific feature would be platform specific. I should have asked - is the test useful for any other port? What does it test in a port that does not restrict the number of audio elements that can play concurrently?

(In reply to comment #22) > Files in WebCore/platform should not reference WebCore files outside of platform. That clarifies it partially. But the question remains, which WebCore files am I referencing outside of platform that I'm not supposed to be? The platformPageClient() is in WebKit/blackberry/WebCoreSupport/, not WebCore/. > I should have asked - is the test useful for any other port? What does it test in a port that does not restrict the number of audio elements that can play concurrently? That makes sense. I don't think the test is very useful even for Blackberry. However patches are not passing review without associated automated layout tests, so I'm just doing what I can to try and shorten the review process.

(In reply to comment #23) > (In reply to comment #22) > > Files in WebCore/platform should not reference WebCore files outside of platform. > > That clarifies it partially. But the question remains, which WebCore files am I referencing outside of platform that I'm not supposed to be? The platformPageClient() is in WebKit/blackberry/WebCoreSupport/, not WebCore/. > > > > I should have asked - is the test useful for any other port? What does it test in a port that does not restrict the number of audio elements that can play concurrently? > > That makes sense. I don't think the test is very useful even for Blackberry. However patches are not passing review without associated automated layout tests, so I'm just doing what I can to try and shorten the review process. I don't think this is any worse than the previous code here which was already r+. However we probably need to think about how to abstract the platform/ code to not call to WebKit/ here. All that said, this is pretty app-specific for BlackBerry.

Comment on attachment 137429 [details] Patch Clearing flags on attachment: 137429 Committed r114493: <http://trac.webkit.org/changeset/114493>

All reviewed patches have been landed. Closing bug.

(In reply to comment #24) > > I don't think this is any worse than the previous code here which was already r+. However we probably need to think about how to abstract the platform/ code to not call to WebKit/ here. All that said, this is pretty app-specific for BlackBerry. I agree that it would be good to figure out how to change the existing code to not assume/know so much about code outside of WebCore/platform. Particularly unfortunate, IMO, is the code that casts MediaPlayer::mediaPlayerClient() to HTMLMediaElement to make calls on the media element directly. The engine constructor is passed a MediaPlayer* so it has an opaque interface to make calls to WebCore without having to do this. The MediaPlayerClient interface can be extended if you need something from the media element or something it knows about. Adding the necessary plumbing isn't a lot of work, and once it is there other ports can use it too. For example, to allow the AVFoundation based media engine to pass the correct UA string (https://bugs.webkit.org/show_bug.cgi?id=46241) we added mediaPlayerUserAgent to MediaPlayerClient so the engine can get the media element to fetch the UA string from the loader. MediaPlayerBlackBerry apparently needed the same functionality as well, but instead casts to an HTMLMediaElement so it can get a pointer to the frame loader to call userAgent() directly. Bug 46241 would have been much easier to fix if the MediaPlayerClient plumbing had already been there :-).

(In reply to comment #27) Thanks for the clear description, Eric. I've created Bug 84291 to track your concerns about HTMLMediaElement being referenced directly. I may need your help on that one because there are certain specific reasons we are using HTMLMediaElement. For example, taking the FrameView object from MediaPlayer is not possible during crucial phases of MediaPlayerPrivate setup/teardown because it is set too late and cleared too early.

(In reply to comment #28) > (In reply to comment #27) > > Thanks for the clear description, Eric. I've created Bug 84291 to track your concerns about HTMLMediaElement being referenced directly. I may need your help on that one because there are certain specific reasons we are using HTMLMediaElement. For example, taking the FrameView object from MediaPlayer is not possible during crucial phases of MediaPlayerPrivate setup/teardown because it is set too late and cleared too early. Thanks for filing the bug! I will be happy to help you in any way I can. Feel free to track me down on irc, send me email with questions, or just cc me on bugs.

Closing bug for patch that landed a long time ago.