Expose text track settings in Content and implement for Android

Expose text track settings in Content and implement for Android

WebKit CL : https://codereview.chromium.org/921833003/

This commit exposes the settings introduced in the above WebKit CL:
Text color, background color, text shadow, font size, font family, font style,
and font variant.

These settings are implemented and used for Android with this commit.

BUG=457850
TEST=content/public/android/javatests/src/org/chromium/content/browser/CaptioningChangeDelegateTest.java

Committed: https://crrev.com/e736c9db485fc59e61a7c81e5ec59301e083aad0
Cr-Commit-Position: refs/heads/master@{#323579}

[dspell, 2015-03-11 22:50:51]

dspell@amazon.com changed reviewers:
+ nasko@chromium.org

[dspell, 2015-03-11 22:54:24]

Hi nasko,

This is the Chromium side of Closed Caption Settings with an Android
implementation.

Here is the corresponding WebKit CL: https://codereview.chromium.org/921833003/

PTAL

Thanks,
Dspell

[nasko, 2015-03-11 23:58:09]

Couple of things:
* We are moving away from RenderView(Host) objects in favor of RenderFrame(Host)
ones. Please use those to send IPCs, as you can always get the main
RenderFrameHost for a WebContents.
* Have someone familiar with the Android code review it first, as am not as
familiar with it.

Thanks for your contributions!

[philipj_slow, 2015-03-12 04:56:10]

I don't know who would be a good person to review the Android bits, but if you
can't find anyone, maybe ask Laura Palmaro who's on the accessibility team.

[dmazzoni, 2015-03-12 05:05:42]

dmazzoni@chromium.org changed reviewers:
+ dmazzoni@chromium.org, dtrainor@chromium.org

[dmazzoni, 2015-03-12 05:05:43]

+dtrainor

David, could you review the Android parts of the code and help find other owners
on your team as necessary?

[dspell, 2015-03-12 18:01:14]

On 2015/03/11 23:58:09, nasko wrote:
> Couple of things:
> * We are moving away from RenderView(Host) objects in favor of
RenderFrame(Host)
> ones. Please use those to send IPCs, as you can always get the main
> RenderFrameHost for a WebContents.
> * Have someone familiar with the Android code review it first, as am not as
> familiar with it.
> 
> Thanks for your contributions!

Thank you for your quick feedback. I have uploaded a second patch set that moves
the IPC communication
from RenderView(Host) to RenderFrame(Host).

[dspell, 2015-03-12 18:03:41]

On 2015/03/11 23:58:09, nasko wrote:
> Couple of things:
> * We are moving away from RenderView(Host) objects in favor of
RenderFrame(Host)
> ones. Please use those to send IPCs, as you can always get the main
> RenderFrameHost for a WebContents.
> * Have someone familiar with the Android code review it first, as am not as
> familiar with it.
> 
> Thanks for your contributions!

Thank you for your quick feedback. I have uploaded a second patch set that moves
the IPC communication
from RenderView(Host) to RenderFrame(Host).

[David Trainor- moved to gerrit, 2015-03-12 18:10:17]

Android bits lgtm.  Thanks!

https://chromiumcodereview.appspot.com/1002473003/diff/1/content/public/andro...
File
content/public/android/java/src/org/chromium/content/browser/accessibility/captioning/CaptioningChangeDelegate.java
(right):

https://chromiumcodereview.appspot.com/1002473003/diff/1/content/public/andro...
content/public/android/java/src/org/chromium/content/browser/accessibility/captioning/CaptioningChangeDelegate.java:147:
default: // CaptionStyle.EDGE_TPYE_NONE
TPYE -> TYPE, this is also for EDGE_TYPE_UNSPECIFIED too I assume?

https://chromiumcodereview.appspot.com/1002473003/diff/1/content/public/andro...
content/public/android/java/src/org/chromium/content/browser/accessibility/captioning/CaptioningChangeDelegate.java:164:
* Default color ot use if no color is specified when
ot -> to

[nasko, 2015-03-12 20:53:53]

Thanks for moving the code over! This will save us unnecessary work.

Just a few minor comments.

https://codereview.chromium.org/1002473003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1002473003/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1831: const
FrameMsg_TextTrackSettings_Params& params) {
Let's add a DCHECK to verify the assumptions that this is only called for the
main frame.

https://codereview.chromium.org/1002473003/diff/20001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1002473003/diff/20001/content/common/frame_me...
content/common/frame_messages.h:318: // Font family of the text track text.
nit: Put empty lines between comments and the preceding struct member. It is the
style used in most code in this file.

https://codereview.chromium.org/1002473003/diff/20001/content/common/frame_me...
content/common/frame_messages.h:534: // Send to the render view to set text
track style settings.
s/render view/RenderFrame/

Also, if it is only meant to be sent to the top-level frame, it will be useful
constraint to put in comment.

https://codereview.chromium.org/1002473003/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1002473003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:1552: const
FrameMsg_TextTrackSettings_Params& params) {
If this is only expected for the top-level frame, let's put a (D)CHECK here to
ensure that is the case.

[dspell, 2015-03-12 21:52:25]

I addressed all the comments. PTAL.

Thanks,
Dspell

[nasko, 2015-03-12 22:07:40]

LGTM

[dspell, 2015-03-18 19:15:29]

On 2015/03/12 22:07:40, nasko wrote:
> LGTM

I added another patchset due to feedback from the corresponding WebKit CL. This
new patch ensures that author-specified closed caption style settings are
respected if top-level closed caption state is disabled.

PTAL

Thanks!

[David Trainor- moved to gerrit, 2015-03-18 19:36:05]

android bits still LGTM

[philipj_slow, 2015-03-19 13:54:00]

I would like it very much if the CaptioningManager.isEnabled() is made into an
explicit setting in Blink. Even though one can check all the settings
individually, it's a bit more clear with a top-level setting and this setting
would come to good use for solving another bug, where I would use it to
determine if captions should be enabled by default or not:
https://code.google.com/p/chromium/issues/detail?id=388588

Currently we just always enable captions and using this API would match user
expectations instead.

[dspell, 2015-03-20 16:59:00]

On 2015/03/19 13:54:00, philipj_UTC7 wrote:
> I would like it very much if the CaptioningManager.isEnabled() is made into an
> explicit setting in Blink. Even though one can check all the settings
> individually, it's a bit more clear with a top-level setting and this setting
> would come to good use for solving another bug, where I would use it to
> determine if captions should be enabled by default or not:
> https://code.google.com/p/chromium/issues/detail?id=388588
> 
> Currently we just always enable captions and using this API would match user
> expectations instead.

Thanks for the information Philip. Can we track the fix for that bug in a
follow-up CL to minimize the complexity of this commit?

[dspell, 2015-03-27 18:01:05]

The corresponding WebKit CL has landed. This is a rebase that fixed merge
conflicts in two of the header files.

Regarding Philip's comment, since the WebKit change landed without exposing the
"enabled" setting, I will expose that in a separate CL set.

[dspell, 2015-03-27 18:01:45]

The CQ bit was checked by dspell@amazon.com

[dspell, 2015-03-27 18:01:46]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org,
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/1002473003/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2015-03-27 18:02:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1002473003/80001

[commit-bot: I haz the power, 2015-03-27 18:12:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-27 18:12:24]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dspell, 2015-03-27 23:49:09]

This patch set fixes the issues found in CQ. 

PTAL

[qinmin, 2015-04-01 18:26:52]

qinmin@chromium.org changed reviewers:
+ qinmin@chromium.org

[qinmin, 2015-04-01 19:08:20]

https://codereview.chromium.org/1002473003/diff/100001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/accessibility/captioning/CaptioningChangeDelegate.java
(right):

https://codereview.chromium.org/1002473003/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/accessibility/captioning/CaptioningChangeDelegate.java:89:
if (typeFace != null && typeFace.isItalic()) {
Not sure whether system captioning fonts can be italic as I don't see italic
fonts in android captioning settings.
But if we support this, should we also support isBold() by adding font-weight
css? 
This could be in a separate CL though

[dspell, 2015-04-01 20:15:44]

Hi David,

Would you like to review the changes I made to fix the CQ, or am I free to
submit it to the CQ again?

Thanks.

[dspell, 2015-04-01 21:52:43]

On 2015/04/01 19:08:20, qinmin wrote:
>
https://codereview.chromium.org/1002473003/diff/100001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/accessibility/captioning/CaptioningChangeDelegate.java
> (right):
> 
>
https://codereview.chromium.org/1002473003/diff/100001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/accessibility/captioning/CaptioningChangeDelegate.java:89:
> if (typeFace != null && typeFace.isItalic()) {
> Not sure whether system captioning fonts can be italic as I don't see italic
> fonts in android captioning settings.
> But if we support this, should we also support isBold() by adding font-weight
> css? 
> This could be in a separate CL though

Adding CSS font-weight should be a separate CL related to BUG=457850 since the
initial WebKit Captioning Settings for this CL have landed.

[David Trainor- moved to gerrit, 2015-04-01 22:16:37]

lgtm

[dspell, 2015-04-02 18:40:05]

On 2015/04/01 19:08:20, qinmin wrote:
>
https://codereview.chromium.org/1002473003/diff/100001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/accessibility/captioning/CaptioningChangeDelegate.java
> (right):
> 
>
https://codereview.chromium.org/1002473003/diff/100001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/accessibility/captioning/CaptioningChangeDelegate.java:89:
> if (typeFace != null && typeFace.isItalic()) {
> Not sure whether system captioning fonts can be italic as I don't see italic
> fonts in android captioning settings.
> But if we support this, should we also support isBold() by adding font-weight
> css? 
> This could be in a separate CL though

Thanks. I added your comment to the bug.

[dspell, 2015-04-02 18:41:59]

The CQ bit was checked by dspell@amazon.com

[dspell, 2015-04-02 18:42:00]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1002473003/#ps100001
(title: "Fix CQ Issues")

[commit-bot: I haz the power, 2015-04-02 18:45:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1002473003/100001

[commit-bot: I haz the power, 2015-04-03 04:53:32]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-04-03 20:29:54]

Patchset 6 (id:??) landed as
https://crrev.com/e736c9db485fc59e61a7c81e5ec59301e083aad0
Cr-Commit-Position: refs/heads/master@{#323579}