Bug 87551 – [BlackBerry] Conditionally enlarge HTML5 video controls in fullscreen mode

• Component: WebKit BlackBerry
• Status: CLOSED FIXED

[Max Feil]

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.

[Max Feil]

Created attachment 144172 [details]
Patch

[WebKit Review Bot]

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.

[Build Bot]

Comment on attachment 144172 [details]
Patch

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

[Max Feil]

Created attachment 144175 [details]
Patch

[Max Feil]

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.

[WebKit Review Bot]

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.

[Build Bot]

Comment on attachment 144175 [details]
Patch

Attachment 144175 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12801616

[Early Warning System Bot]

Comment on attachment 144175 [details]
Patch

Attachment 144175 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12808542

[Early Warning System Bot]

Comment on attachment 144175 [details]
Patch

Attachment 144175 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12804644

[Build Bot]

Comment on attachment 144175 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 144175 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 144175 [details]
Patch

Attachment 144175 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12795686

[Max Feil]

Created attachment 144182 [details]
Patch

[WebKit Review Bot]

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.

[Early Warning System Bot]

Comment on attachment 144182 [details]
Patch

Attachment 144182 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12800653

[Early Warning System Bot]

Comment on attachment 144182 [details]
Patch

Attachment 144182 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12802614

[Gyuyoung Kim]

Comment on attachment 144182 [details]
Patch

Attachment 144182 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12794665

[Max Feil]

Created attachment 144184 [details]
Patch

[WebKit Review Bot]

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.

[Gustavo Noronha (kov)]

Comment on attachment 144184 [details]
Patch

Attachment 144184 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12793650

[Build Bot]

Comment on attachment 144184 [details]
Patch

Attachment 144184 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12807634

[Early Warning System Bot]

Comment on attachment 144184 [details]
Patch

Attachment 144184 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12800660

[Build Bot]

Comment on attachment 144184 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 144184 [details]
Patch

Attachment 144184 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12794669

[WebKit Review Bot]

Comment on attachment 144184 [details]
Patch

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

[Max Feil]

Created attachment 144188 [details]
Patch

[WebKit Review Bot]

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.

[Build Bot]

Comment on attachment 144188 [details]
Patch

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

[Max Feil]

Created attachment 144191 [details]
Patch

[WebKit Review Bot]

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.

[Antonio Gomes]

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?

[Konrad Piascik]

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.

[Konrad Piascik]

(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.

[Max Feil]

(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.

[Max Feil]

Created attachment 144697 [details]
Patch

[WebKit Review Bot]

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.

[Max Feil]

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...

[Rob Buis]

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?

[Max Feil]

Created attachment 145468 [details]
Patch

[Max Feil]

This is part of PR140677.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Max Feil]

(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??

[Antonio Gomes]

(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?

[Antonio Gomes]

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?

[Max Feil]

(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.

[Rob Buis]

(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?

[Max Feil]

(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

[Max Feil]

Created attachment 145675 [details]
Patch

[Max Feil]

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 :)

[Antonio Gomes]

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.

[Max Feil]

Created attachment 145810 [details]
Patch

[WebKit Review Bot]

Comment on attachment 145810 [details]
Patch

Clearing flags on attachment: 145810

Committed r119547: <http://trac.webkit.org/changeset/119547>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Max Feil]

Closing bug for patch that landed a long time ago.