Create a ContentVideoViewDelegate.java

Create a ContentVideoViewDelegate.java

BUG=6295515
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170565

[acleung, 2012-11-19 19:00:50]

Tried it with the ToT. Fullscreen video still works after this.

[qinmin, 2012-11-20 21:15:39]

lgtm. One thing still haven't been addressed by this change is the
initResources() call. 
Since we get some resources from the manifest file in ContentVideoView(), you
probably will have to manually set those values in your delegate class.

https://codereview.chromium.org/11412076/diff/1/content/shell/android/java/sr...
File
content/shell/android/java/src/org/chromium/content_shell/ContentShellActivity.java
(right):

https://codereview.chromium.org/11412076/diff/1/content/shell/android/java/sr...
content/shell/android/java/src/org/chromium/content_shell/ContentShellActivity.java:63:
new AndroidContentVideoViewDelegate(this));
fix indent

[acleung, 2012-11-23 22:56:58]

Good point. I have included the resource in the delegate as well.

@yaron: I need OWNER approval for this as well.

https://codereview.chromium.org/11412076/diff/1/content/shell/android/java/sr...
File
content/shell/android/java/src/org/chromium/content_shell/ContentShellActivity.java
(right):

https://codereview.chromium.org/11412076/diff/1/content/shell/android/java/sr...
content/shell/android/java/src/org/chromium/content_shell/ContentShellActivity.java:63:
new AndroidContentVideoViewDelegate(this));
On 2012/11/20 21:15:40, qinmin wrote:
> fix indent

Done.

[Yaron, 2012-11-27 02:36:51]

https://codereview.chromium.org/11412076/diff/2002/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/AndroidContentVideoViewDelegate.java
(right):

https://codereview.chromium.org/11412076/diff/2002/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/AndroidContentVideoViewDelegate.java:17:
public class AndroidContentVideoViewDelegate implements ContentVideoViewDelegate
{
Android* isn't that useful a name. What about Activity* since the idea is that
this is providing the Activity-backed implementation

https://codereview.chromium.org/11412076/diff/2002/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoViewDelegate.java
(right):

https://codereview.chromium.org/11412076/diff/2002/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentVideoViewDelegate.java:10:
public interface ContentVideoViewDelegate {
Please add a description of this interface. Also, this isn't really a helpful
name. Maybe something like ContentVideoViewContextDelegate?

[acleung, 2012-11-27 08:20:49]

https://codereview.chromium.org/11412076/diff/2002/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/AndroidContentVideoViewDelegate.java
(right):

https://codereview.chromium.org/11412076/diff/2002/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/AndroidContentVideoViewDelegate.java:17:
public class AndroidContentVideoViewDelegate implements ContentVideoViewDelegate
{
On 2012/11/27 02:36:51, Yaron wrote:
> Android* isn't that useful a name. What about Activity* since the idea is that
> this is providing the Activity-backed implementation

Done.

https://codereview.chromium.org/11412076/diff/2002/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoViewDelegate.java
(right):

https://codereview.chromium.org/11412076/diff/2002/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentVideoViewDelegate.java:10:
public interface ContentVideoViewDelegate {
On 2012/11/27 02:36:51, Yaron wrote:
> Please add a description of this interface. Also, this isn't really a helpful
> name. Maybe something like ContentVideoViewContextDelegate?

Done.  What does the word "Context" imply in this case?

[Yaron, 2012-11-27 17:50:56]

lgtm

Heh, "context" can be a loaded term. In this case though I was referring to the
Android Context class
(http://developer.android.com/reference/android/content/Context.html). I thought
the intent of your CL was to factor out the dependency on Activity and use the
parent context class for the purposes of webview

[joth, 2012-11-28 00:17:30]

just wondering how much we really need ActivityContentVideoViewDelegate in the
content public API?

otherwise lgtm, thanks.

https://codereview.chromium.org/11412076/diff/10001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ActivityContentVideoViewDelegate.java
(right):

https://codereview.chromium.org/11412076/diff/10001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ActivityContentVideoViewDelegate.java:14:
import org.chromium.content.app.AppResource;
nit: \n before org section

https://codereview.chromium.org/11412076/diff/10001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ActivityContentVideoViewDelegate.java:21:
private Activity activity;
activity => mActivity

https://codereview.chromium.org/11412076/diff/10001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ActivityContentVideoViewDelegate.java:36:
Gravity.CENTER));
I'm in two minds whether the amount of resuse here really makes this class worth
while - I can easily imagine someday we'd want to keep this simple handling for
content shell, but do something more elaborate in the browser proper, which
would be easier done if each embedder just provided these few methods directly.

The 5 resource string getters below are another question. Seems they could all
be avoided by having the caller just do
sDelegate.getContext().getString(AppResource.STRING_XXX) and then you don't need
any here.

https://codereview.chromium.org/11412076/diff/10001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ActivityContentVideoViewDelegate.java:48:
return
activity.getString(AppResource.STRING_MEDIA_PLAYER_MESSAGE_PLAYBACK_ERROR);
note also that https://codereview.chromium.org/11377117 deleted AppResource, so
you probably need to rebase :)

[acleung, 2012-11-29 20:11:28]

https://codereview.chromium.org/11412076/diff/10001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ActivityContentVideoViewDelegate.java
(right):

https://codereview.chromium.org/11412076/diff/10001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ActivityContentVideoViewDelegate.java:14:
import org.chromium.content.app.AppResource;
On 2012/11/28 00:17:30, joth wrote:
> nit: \n before org section

Done.

https://codereview.chromium.org/11412076/diff/10001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ActivityContentVideoViewDelegate.java:21:
private Activity activity;
On 2012/11/28 00:17:30, joth wrote:
> activity => mActivity

Done.

https://codereview.chromium.org/11412076/diff/10001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ActivityContentVideoViewDelegate.java:36:
Gravity.CENTER));
On 2012/11/28 00:17:30, joth wrote:
> I'm in two minds whether the amount of resuse here really makes this class
worth
> while - I can easily imagine someday we'd want to keep this simple handling
for
> content shell, but do something more elaborate in the browser proper, which
> would be easier done if each embedder just provided these few methods
directly.
> 
> The 5 resource string getters below are another question. Seems they could all
> be avoided by having the caller just do
> sDelegate.getContext().getString(AppResource.STRING_XXX) and then you don't
need
> any here.
> 

It really depends how much ContentShell and the browser differs in how they
handle full screen videos.

I am going to guess that it isn't going to be that different down the road?

https://codereview.chromium.org/11412076/diff/10001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ActivityContentVideoViewDelegate.java:48:
return
activity.getString(AppResource.STRING_MEDIA_PLAYER_MESSAGE_PLAYBACK_ERROR);
On 2012/11/28 00:17:30, joth wrote:
> note also that https://codereview.chromium.org/11377117 deleted AppResource,
so
> you probably need to rebase :)

Done.

[commit-bot: I haz the power, 2012-11-29 20:57:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acleung@google.com/11412076/9002

[commit-bot: I haz the power, 2012-11-29 21:21:55]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-29 21:27:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acleung@google.com/11412076/14001

[commit-bot: I haz the power, 2012-11-29 21:55:41]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-30 20:07:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acleung@google.com/11412076/14001

[commit-bot: I haz the power, 2012-11-30 20:12:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acleung@google.com/11412076/8005

[commit-bot: I haz the power, 2012-11-30 22:01:22]

Change committed as 170565