[Webkit-unassigned] [Bug 82930] [BlackBerry] Tab awareness for HTML5 concurrent audio

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=82930


Eric Carlson <eric.carlson at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |abarth at webkit.org




--- Comment #22 from Eric Carlson <eric.carlson at apple.com>  2012-04-16 21:29:42 PST ---
(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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.