Bug 100378 – [BlackBerry] Implement MediaPlayerPrivate::didLoadingProgress for BlackBerry Media Player

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[John Griggs]

Bug 86113 replaced the progress mechanism based on MediaPlayerPrivate::bytesLoaded with one based on MediaPlayerPrivate::didLoadingProgress.  This bug is to provide an implementation for this new method for the BlackBerry platform.

[John Griggs]

Created attachment 170679 [details]
Patch

[John Griggs]

Created attachment 170695 [details]
Patch

[John Griggs]

Comment on attachment 170695 [details]
Patch

Updated ChangeLog to indicate why there are no new tests included.

[Rob Buis]

Comment on attachment 170695 [details]
Patch

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

Generally looks good, some details we need to sort out still.

> Source/WebCore/ChangeLog:3
> +        2012-10-25  John Griggs  <[e-mail]>

The ChangeLog needs some cleaning up. Basically it should match the layout of the other entries.

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.h:176
> +    mutable float m_lastLoadingTime;

I am not convinced mutalble is needed here, please try without it.

[John Griggs]

Created attachment 171251 [details]
Patch

[John Griggs]

mutable is required on the data member because it is altered in a const method.  The method (didLoadingProgress) is declared const in MediaPlayerPrivateInterface, so we can't re-declare it.

[Rob Buis]

Comment on attachment 171251 [details]
Patch

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

Looks good in general, some small issues to fix.

> Source/WebCore/ChangeLog:16
> +                (MediaPlayerPrivate):

Please fix indenting here, look at other entries.

> LayoutTests/ChangeLog:13
> +        * platform/blackberry/media/progress-events-generated-correctly.html: Added.

Ditto.

> LayoutTests/platform/blackberry/media/progress-events-generated-correctly.html:30
> +            //consoleWrite("EVENT(progress): " + progressCount);

We don't want commented code. If you don't want this, better remove the line.

> LayoutTests/platform/blackberry/media/progress-events-generated-correctly.html:35
> +            //consoleWrite("EVENT(ended): " + progressCount);

Ditto.

[John Griggs]

Created attachment 171294 [details]
Patch

[John Griggs]

Comment on attachment 171294 [details]
Patch

Fix Changelog formatting
Remove commented out lines from test case

[Antonio Gomes]

Comment on attachment 171294 [details]
Patch

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

> LayoutTests/ChangeLog:13
> +        * platform/blackberry/media/progress-events-generated-correctly-expected.txt: Added.
> +        * platform/blackberry/media/progress-events-generated-correctly.html: Added.

Is this test supposed to be supported in platform other than BlackBerry?

[John Griggs]

The test is specific to BlackBerry, at least for now, because the implementation of progress events lags on some platforms.  The platforms that have not yet implemented MediaPlayerPrivateInterface::didLoadingProgress will fail this test.

[Eric Carlson]

Comment on attachment 171294 [details]
Patch

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

> LayoutTests/ChangeLog:14
> +

Does this test do something more than the progress event part of LayoutTests/media/event-attributes.html?

[Eric Carlson]

(In reply to comment #11)
> The test is specific to BlackBerry, at least for now, because the implementation of progress events lags on some platforms.  The platforms that have not yet implemented MediaPlayerPrivateInterface::didLoadingProgress will fail this test.

See the above comment. If this test *is* only supported on BlackBerry but will eventually be supported on other platforms, it should go in LayoutTests/media/ and skipped on other platforms with bugs.

[John Griggs]

If you look at the html file for LayoutTests/media/event-attributes, you will see that there is a comment at the top of the eventHandler that states that progress messages are not logged, since the order and number are platform specific, so that test does not cover progress events.

I can move the test to a more general location, but I am not sure which platforms other than BlackBerry generate progress events during load and which ones would need to be disabled - perhaps enable it for BlackBerry only initially?

[Eric Carlson]

(In reply to comment #14)
> If you look at the html file for LayoutTests/media/event-attributes, you will see that there is a comment at the top of the eventHandler that states that progress messages are not logged, since the order and number are platform specific, so that test does not cover progress events.
> 
No, the comment says not to *log* progress events because the number and order are platform specific. The test for the 'canplaythrough' event fails unless at least one 'progress' event has been fired: 

    case "canplaythrough":
        testExpected('progressEventCount', 1, '>=')

> I can move the test to a more general location, but I am not sure which platforms other than BlackBerry generate progress events during load and which ones would need to be disabled - perhaps enable it for BlackBerry only initially?

Unless other platforms skip event-attributes.html or have failing results saved, all platforms generate progress events.

[John Griggs]

Sorry - I guess my point about the existing test was not clear.  The user agent fires a progress event when the source for a media element is resolved and then is supposed to generate another progress event every 300ms or for every byte loaded, which ever is less frequent.  The existing test covers the event for resolving the source (which works on every platform), but not the ones for loading data, which varies by platform.  

My new test covers the progress events generated for loading data.

[Eric Carlson]

(In reply to comment #16)
> Sorry - I guess my point about the existing test was not clear.  The user agent fires a progress event when the source for a media element is resolved and then is supposed to generate another progress event every 300ms or for every byte loaded, which ever is less frequent.  The existing test covers the event for resolving the source (which works on every platform), but not the ones for loading data, which varies by platform.  
> 
> My new test covers the progress events generated for loading data.

OK, I see that now. Please add something to the test so someone reading the results can easily see that.

[John Griggs]

Created attachment 171469 [details]
Patch

[John Griggs]

Comment on attachment 171469 [details]
Patch

Update test title to: "Test that progress events are generated during loading of media resource."  This appears in the test output.

[Eric Carlson]

(In reply to comment #17)
> (In reply to comment #16)
> > Sorry - I guess my point about the existing test was not clear.  The user agent fires a progress event when the source for a media element is resolved and then is supposed to generate another progress event every 300ms or for every byte loaded, which ever is less frequent.  The existing test covers the event for resolving the source (which works on every platform), but not the ones for loading data, which varies by platform.  
> > 
> > My new test covers the progress events generated for loading data.
> 
> OK, I see that now. Please add something to the test so someone reading the results can easily see that.

Hang on, this test *should* pass on other ports as well because r119187 added didLoadingProgress for OSX, Windows, GTK, Qt, and Chromium at least.

[John Griggs]

I have to admit to trusting comments in tests and other bugs rather than checking all of the other platform's MediaPlayerPrivate implementations.  It looks like many platforms do implement didLoadingProgress - I am still relatively new at this, is there a way to run the tests on other platforms to make sure I'm not missing any that should skip this test?  Or should I simply move the test into LayoutTests/media and disable the failing platforms later?

[Antonio Gomes]

You can CC people that might know for each port: OSSY (qt), martin robinson (GTK), etc.

[John Griggs]

It seems to me that the chances of missing a person responsible for a port is about equal to the chance of missing the port itself.  But if someone can supply a list, I am happy to add folks to the CC list...

[Eric Carlson]

(In reply to comment #23)
> It seems to me that the chances of missing a person responsible for a port is about equal to the chance of missing the port itself.  But if someone can supply a list, I am happy to add folks to the CC list...

If the patch has the test in LayoutTests/media/, the bots will tell you if a port fails to run it. Skip the test on the ports that fail and regenerate your patch, rinse, lather, and repeat.

[John Griggs]

Created attachment 171502 [details]
Patch

[John Griggs]

Comment on attachment 171502 [details]
Patch

Moved test to LayoutTests/media so that it applies across platforms.

[WebKit Review Bot]

Comment on attachment 171502 [details]
Patch

Attachment 171502 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14662026

New failing tests:
media/progress-events-generated-correctly.html

[WebKit Review Bot]

Comment on attachment 171502 [details]
Patch

Attachment 171502 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14662047

New failing tests:
media/progress-events-generated-correctly.html

[Build Bot]

Comment on attachment 171502 [details]
Patch

Attachment 171502 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14685006

New failing tests:
media/progress-events-generated-correctly.html

[Eric Carlson]

If you are not able to figure out why the new test fails on some ports, you can skip the test and write bugs on those ports.

[John Griggs]

So the new test appears to fail for mac and chromium-xvfb - which TestExpectations files should I be modifying to skip the test for these platforms?  There appear to be several TestExpectations files for each of these platform, depending on the version and hardware platform...

[John Griggs]

Created attachment 171892 [details]
Patch

[John Griggs]

Comment on attachment 171892 [details]
Patch

No need to review, just testing TestExpectations modifications to make sure they disable the new test on the correct platforms - I still have to add bug numbers, etc.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/medi..." exit_code: 1
LayoutTests/platform/chromium/TestExpectations:182:  Test lacks BUG modifier.  [test/expectations] [5]
Total errors found: 1 in 8 files


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

[John Griggs]

Created attachment 171922 [details]
Patch

[John Griggs]

Comment on attachment 171922 [details]
Patch

Disabled new test on mac and chrome-linux platforms because the bots were failing.

[Eric Carlson]

Comment on attachment 171922 [details]
Patch

This looks good to me. I am not setting the cq+ flag yet as the bots have not all processed the patch.

Thanks for keeping at this John!

[John Griggs]

Eric - Thanks for all your help!

All green now, BTW 8^)

[John Griggs]

ping!  Any chance of getting this added to the commit queue now that it has passed all the bots and been reviewed?

[Rob Buis]

(In reply to comment #39)
> ping!  Any chance of getting this added to the commit queue now that it has passed all the bots and been reviewed?

Done!

[Eric Carlson]

(In reply to comment #39)
> ping!  Any chance of getting this added to the commit queue now that it has passed all the bots and been reviewed?

Oops, sorry I forget to come back to this!

[WebKit Review Bot]

Comment on attachment 171922 [details]
Patch

Clearing flags on attachment: 171922

Committed r133660: <http://trac.webkit.org/changeset/133660>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.