Bug 87551 - [BlackBerry] Conditionally enlarge HTML5 video controls in fullscreen mode
Summary: [BlackBerry] Conditionally enlarge HTML5 video controls in fullscreen mode
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Max Feil
URL:
Keywords:
Depends on: 87817
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-25 16:53 PDT by Max Feil
Modified: 2012-11-02 12:42 PDT (History)
17 users (show)

See Also:


Attachments
Patch (23.89 KB, patch)
2012-05-25 17:06 PDT, Max Feil
no flags Details | Formatted Diff | Diff
Patch (23.79 KB, patch)
2012-05-25 17:17 PDT, Max Feil
no flags Details | Formatted Diff | Diff
Patch (45.12 KB, patch)
2012-05-25 19:53 PDT, Max Feil
no flags Details | Formatted Diff | Diff
Patch (45.10 KB, patch)
2012-05-25 20:06 PDT, Max Feil
no flags Details | Formatted Diff | Diff
Patch (45.20 KB, patch)
2012-05-25 20:41 PDT, Max Feil
no flags Details | Formatted Diff | Diff
Patch (45.92 KB, patch)
2012-05-25 21:39 PDT, Max Feil
no flags Details | Formatted Diff | Diff
Patch (43.45 KB, patch)
2012-05-29 21:47 PDT, Max Feil
no flags Details | Formatted Diff | Diff
Patch (53.18 KB, patch)
2012-06-02 19:18 PDT, Max Feil
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (759.00 KB, application/zip)
2012-06-02 19:49 PDT, WebKit Review Bot
no flags Details
Patch (54.06 KB, patch)
2012-06-04 18:54 PDT, Max Feil
no flags Details | Formatted Diff | Diff
Patch (54.10 KB, patch)
2012-06-05 09:01 PDT, Max Feil
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Max Feil 2012-05-25 16:53:28 PDT
The BlackBerry media controls need to be larger in fullscreen mode so that they are usable on small screens. Until now the BlackBerry media controls have been sized by a combination of CSS styles specified in mediaControlsBlackBerry.css and hard coded constants in RenderThemeBlackBerry.cpp. This patch moves all the dimensional sizes to a common location, i.e.  RenderThemeBlackBerry.cpp. Having all the logic in C++ code allows more complex decision making for when and how much to enlarge the media controls. This patch currently enlarges the controls by a factor of 2 if the document is in fullscreen mode, the video element is the current fullscreen element, and the deviceScaleFactor of the page is less than 2.0. This avoids overly large controls on pages which are already at least doubling the size of the controls via viewport meta tag device-width settings. In other words, if the CSS pixels are already enlarged compared to device pixels by a factor of at least 2 in length and width.

In order to accomplish this, I had to add a new Element* parameter to RenderTheme::adjustSliderThumbSize(). This is needed so that fullscreen mode can be checked and the deviceScaleFactor retrieved. I feel that this does not have a big impact on the code and the other ports, for which I will simply be adding the additional parameter in their platform code.
Comment 1 Max Feil 2012-05-25 17:06:49 PDT
Created attachment 144172 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-25 17:08:57 PDT
Attachment 144172 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/blackberry/RenderThemeBlackBerry.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/blackberry/RenderThemeBlackBerry.cpp:156:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/platform/blackberry/RenderThemeBlackBerry.cpp:760:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/WebCore/platform/blackberry/RenderThemeBlackBerry.cpp:789:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/WebCore/ChangeLog:31:  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: 5 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Build Bot 2012-05-25 17:14:49 PDT
Comment on attachment 144172 [details]
Patch

Attachment 144172 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12800634
Comment 4 Max Feil 2012-05-25 17:17:31 PDT
Created attachment 144175 [details]
Patch
Comment 5 Max Feil 2012-05-25 17:18:20 PDT
Even though this patch is not 100% complete, I would like to get an initial review. Mainly I would like to be sure that my approach of adding a new parameter to adjustSliderThumbSize() is acceptable. If so, I will update the patch with code changes for the other ports, and tests.
Comment 6 WebKit Review Bot 2012-05-25 17:20:48 PDT
Attachment 144175 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:31:  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 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Build Bot 2012-05-25 17:45:03 PDT
Comment on attachment 144175 [details]
Patch

Attachment 144175 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12801616
Comment 8 Early Warning System Bot 2012-05-25 18:05:08 PDT
Comment on attachment 144175 [details]
Patch

Attachment 144175 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12808542
Comment 9 Early Warning System Bot 2012-05-25 18:05:48 PDT
Comment on attachment 144175 [details]
Patch

Attachment 144175 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12804644
Comment 10 Build Bot 2012-05-25 18:06:10 PDT
Comment on attachment 144175 [details]
Patch

Attachment 144175 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12804635
Comment 11 WebKit Review Bot 2012-05-25 18:29:50 PDT
Comment on attachment 144175 [details]
Patch

Attachment 144175 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12794649
Comment 12 Gyuyoung Kim 2012-05-25 19:08:53 PDT
Comment on attachment 144175 [details]
Patch

Attachment 144175 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12795686
Comment 13 Max Feil 2012-05-25 19:53:07 PDT
Created attachment 144182 [details]
Patch
Comment 14 WebKit Review Bot 2012-05-25 19:56:11 PDT
Attachment 144182 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:31:  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 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Early Warning System Bot 2012-05-25 20:03:31 PDT
Comment on attachment 144182 [details]
Patch

Attachment 144182 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12800653
Comment 16 Early Warning System Bot 2012-05-25 20:06:28 PDT
Comment on attachment 144182 [details]
Patch

Attachment 144182 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12802614
Comment 17 Gyuyoung Kim 2012-05-25 20:06:34 PDT
Comment on attachment 144182 [details]
Patch

Attachment 144182 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12794665
Comment 18 Max Feil 2012-05-25 20:06:54 PDT
Created attachment 144184 [details]
Patch
Comment 19 WebKit Review Bot 2012-05-25 20:10:41 PDT
Attachment 144184 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:31:  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 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Gustavo Noronha (kov) 2012-05-25 20:30:31 PDT
Comment on attachment 144184 [details]
Patch

Attachment 144184 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12793650
Comment 21 Build Bot 2012-05-25 20:33:48 PDT
Comment on attachment 144184 [details]
Patch

Attachment 144184 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12807634
Comment 22 Early Warning System Bot 2012-05-25 20:34:55 PDT
Comment on attachment 144184 [details]
Patch

Attachment 144184 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12800660
Comment 23 Build Bot 2012-05-25 20:36:10 PDT
Comment on attachment 144184 [details]
Patch

Attachment 144184 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12804670
Comment 24 Early Warning System Bot 2012-05-25 20:37:32 PDT
Comment on attachment 144184 [details]
Patch

Attachment 144184 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12794669
Comment 25 WebKit Review Bot 2012-05-25 20:39:46 PDT
Comment on attachment 144184 [details]
Patch

Attachment 144184 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12799733
Comment 26 Max Feil 2012-05-25 20:41:50 PDT
Created attachment 144188 [details]
Patch
Comment 27 WebKit Review Bot 2012-05-25 20:43:53 PDT
Attachment 144188 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:31:  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 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 28 Build Bot 2012-05-25 21:21:15 PDT
Comment on attachment 144188 [details]
Patch

Attachment 144188 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12802625
Comment 29 Max Feil 2012-05-25 21:39:19 PDT
Created attachment 144191 [details]
Patch
Comment 30 WebKit Review Bot 2012-05-25 21:41:39 PDT
Attachment 144191 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:31:  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 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 31 Antonio Gomes 2012-05-26 20:25:40 PDT
Comment on attachment 144191 [details]
Patch

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

I think the change to viewport should be of its own. I did not look at the rest deeply, but that would be enough for an r- ... leaving r? so other can walk-in and review.

+Jacky and Konrad who last touched it.

> Source/WebCore/platform/blackberry/RenderThemeBlackBerry.cpp:154
> +    for (int i = 1; object && i <= 3; i++)

why 3?

> Source/WebKit/blackberry/Api/WebPage.cpp:3340
>  IntSize WebPagePrivate::recomputeVirtualViewportFromViewportArguments()
>  {
>      static const ViewportArguments defaultViewportArguments;
> -    if (m_viewportArguments == defaultViewportArguments)
> +    if (m_viewportArguments == defaultViewportArguments) {
> +        m_page->setDeviceScaleFactor(1.0);
>          return IntSize();
> +    }
>  

Jacky, Konrad?

> Source/WebKit/blackberry/Api/WebPage.cpp:3378
>  void WebPagePrivate::dispatchViewportPropertiesDidChange(const ViewportArguments& arguments)
>  {
> -    static ViewportArguments defaultViewportArguments;
> -    if (arguments == defaultViewportArguments)
> +    if (arguments == m_viewportArguments)
>          return;

Jacky, Konrad?
Comment 32 Konrad Piascik 2012-05-28 10:02:29 PDT
Comment on attachment 144191 [details]
Patch

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

> Source/WebKit/blackberry/Api/WebPage.cpp:3337
> +        m_page->setDeviceScaleFactor(1.0);

I don't think we currently reset the deviceScaleFactor, so this is a good change r+ for this method from me.

> Source/WebKit/blackberry/Api/WebPage.cpp:3377
> +    if (arguments == m_viewportArguments)

This change looks good to me too.  I'm not sure why we check against default ags here since we don't really care about default, but rather if the new args are different from m_viewportArguments.
Comment 33 Konrad Piascik 2012-05-28 10:10:40 PDT
(In reply to comment #32)
> (From update of attachment 144191 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144191&action=review
> 
> > Source/WebKit/blackberry/Api/WebPage.cpp:3337
> > +        m_page->setDeviceScaleFactor(1.0);
> 
> I don't think we currently reset the deviceScaleFactor, so this is a good change r+ for this method from me.
> 
> > Source/WebKit/blackberry/Api/WebPage.cpp:3377
> > +    if (arguments == m_viewportArguments)
> 
> This change looks good to me too.  I'm not sure why we check against default ags here since we don't really care about default, but rather if the new args are different from m_viewportArguments.


I agree with Antonio that the above changes could/should be in their own patch.  Then you could make this bug depend on the other patch.
Comment 34 Max Feil 2012-05-29 21:38:48 PDT
(In reply to comment #31)
> > Source/WebCore/platform/blackberry/RenderThemeBlackBerry.cpp:154
> > +    for (int i = 1; object && i <= 3; i++)
> 
> why 3?

This comes from the nested node structure of the media controls. I did not change the functionality of this part of the code, just created a more efficient convenience function.
Comment 35 Max Feil 2012-05-29 21:47:28 PDT
Created attachment 144697 [details]
Patch
Comment 36 WebKit Review Bot 2012-05-29 21:50:46 PDT
Attachment 144697 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:31:  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 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 37 Max Feil 2012-05-29 21:56:52 PDT
I have separated the Source/WebKit/blackberry into its own bug and made this bug depend on it. 

I am still hoping for feedback on the addition of a new Element* parameter to RenderTheme::adjustSliderThumbSize(), as mentioned in Comment #0. If I don't hear anything I will assume that all are in agreement.

And yes, I know I still need tests...
Comment 38 Rob Buis 2012-06-01 07:52:42 PDT
Comment on attachment 144697 [details]
Patch

R- because of the style queue thing. Also once you do a new patch (I assume you are adding tests) can you add the PR number, if any?
Comment 39 Max Feil 2012-06-02 19:18:00 PDT
Created attachment 145468 [details]
Patch
Comment 40 Max Feil 2012-06-02 19:20:27 PDT
This is part of PR140677.
Comment 41 WebKit Review Bot 2012-06-02 19:48:55 PDT
Comment on attachment 145468 [details]
Patch

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

New failing tests:
platform/blackberry/media/video-controls-enlarged-fullscreen-nometa.html
platform/blackberry/media/video-controls-enlarged-fullscreen-meta.html
Comment 42 WebKit Review Bot 2012-06-02 19:49:24 PDT
Created attachment 145472 [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
Comment 43 Max Feil 2012-06-03 02:53:54 PDT
(In reply to comment #41)
> (From update of attachment 145468 [details])
> Attachment 145468 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12869889
> 
> New failing tests:
> platform/blackberry/media/video-controls-enlarged-fullscreen-nometa.html
> platform/blackberry/media/video-controls-enlarged-fullscreen-meta.html

Why are other ports running BlackBerry platform specific tests? Am I missing something??
Comment 44 Antonio Gomes 2012-06-03 21:23:47 PDT
(In reply to comment #43)
> (In reply to comment #41)
> > (From update of attachment 145468 [details] [details])
> > Attachment 145468 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/12869889
> > 
> > New failing tests:
> > platform/blackberry/media/video-controls-enlarged-fullscreen-nometa.html
> > platform/blackberry/media/video-controls-enlarged-fullscreen-meta.html
> 
> Why are other ports running BlackBerry platform specific tests? Am I missing something?

Dirk, would you know?
Comment 45 Antonio Gomes 2012-06-04 11:33:18 PDT
Comment on attachment 145468 [details]
Patch

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

The approach look sane to me, but I still have some questions/requests.

does your patch fix the problem that HTML5 controls (with the new fullscreen API) are getting differently scaled according to the webpage scale?

> Source/WebCore/platform/blackberry/RenderThemeBlackBerry.cpp:154
> +    for (int i = 1; object && i <= 3; i++)

please add a comment about these values.

> Source/WebCore/platform/blackberry/RenderThemeBlackBerry.cpp:167
> +    int fullScreenMultiplier = 1;
> +#if ENABLE(FULLSCREEN_API) && ENABLE(VIDEO)
> +    if (element && element->document()->webkitIsFullScreen() && element->document()->webkitCurrentFullScreenElement() == toParentMediaElement(element)) {
> +        if (element->document()->page()->deviceScaleFactor() < scaleFactorThreshold)
> +            fullScreenMultiplier = fullScreenEnlargementFactor;
> +    }
> +#endif

webkit has a tendency of early return instead of nesting if checks.

> Source/WebCore/platform/blackberry/RenderThemeBlackBerry.cpp:757
> +    Length controlsHeight(mediaControlsHeight * fullScreenMultiplier, Fixed);
> +    Length timeWidth(mediaControlsHeight * 3 / 2 * fullScreenMultiplier, Fixed);
> +    Length volumeHeight(mediaControlsHeight * 4 * fullScreenMultiplier, Fixed);
> +    Length padding(mediaControlsHeight / 8 * fullScreenMultiplier, Fixed);
> +    int fontSize = mediaControlsHeight / 2 * fullScreenMultiplier;

could you explain where these literals come from (3, 2, 4, 8)?

> Source/WebCore/platform/blackberry/RenderThemeBlackBerry.cpp:787
> +    Length volumeHeight(mediaControlsHeight * 4 * fullScreenMultiplier, Fixed);

what is 4 herE?
Comment 46 Max Feil 2012-06-04 13:47:28 PDT
(In reply to comment #45)
> please add a comment about these values.
> could you explain where these literals come from (3, 2, 4, 8)?
> what is 4 herE?

The items you are asking me to comment are all from pre-existing code that I did not write. I will gladly add comments, but I don't think this should hold up landing of my patch.
Comment 47 Rob Buis 2012-06-04 14:19:21 PDT
(In reply to comment #43)
> (In reply to comment #41)
> > (From update of attachment 145468 [details] [details])
> > Attachment 145468 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/12869889
> > 
> > New failing tests:
> > platform/blackberry/media/video-controls-enlarged-fullscreen-nometa.html
> > platform/blackberry/media/video-controls-enlarged-fullscreen-meta.html
> 
> Why are other ports running BlackBerry platform specific tests? Am I missing something??

That does seem strange, I also thought having it in platform/port made it visible for the port only. Maybe we need to skip it for chromium these days?
Comment 48 Max Feil 2012-06-04 15:05:25 PDT
(In reply to comment #47)
> That does seem strange, I also thought having it in platform/port made it visible for the port only. Maybe we need to skip it for chromium these days?

Of the 7 ports in the EWS, only cr-linux runs tests. The others are build-only. I'm not sure why this is, but it does mean we have to assume all ports will attempt to run my new BlackBerry-only tests (and fail).

I will investigate what is being done for the other ports and implement platform/blackberry the same way
Comment 49 Max Feil 2012-06-04 18:54:59 PDT
Created attachment 145675 [details]
Patch
Comment 50 Max Feil 2012-06-04 18:56:41 PDT
I have resolved the test failure the same way the existing platform/blackberry test was being skipped.

I also believe I have addressed all comments made about this patch so far, and look forward to a swift conclusion :)
Comment 51 Antonio Gomes 2012-06-04 21:22:57 PDT
Comment on attachment 145675 [details]
Patch

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

r=me

one minor request bellow to address before it lands..

Test could get improved but I think it is ok.

> Source/WebCore/platform/blackberry/RenderThemeBlackBerry.cpp:154
> +    // The RenderSlider is 3 parent levels about the slider thumb

nit: misses a . at the end (coding style).

I would add an ASSERT here to indicate what your RenderObject is/will be.
Comment 52 Max Feil 2012-06-05 09:01:59 PDT
Created attachment 145810 [details]
Patch
Comment 53 WebKit Review Bot 2012-06-05 18:40:55 PDT
Comment on attachment 145810 [details]
Patch

Clearing flags on attachment: 145810

Committed r119547: <http://trac.webkit.org/changeset/119547>
Comment 54 WebKit Review Bot 2012-06-05 18:41:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 55 Max Feil 2012-11-02 12:42:00 PDT
Closing bug for patch that landed a long time ago.