Implement feedback in Chromoting Help & Feedback screen

Implement feedback in Chromoting Help & Feedback screen

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

[Lambros, 2014-03-06 00:38:29]

Note that taking a screenshot doesn't currently work for the Desktop activity.
Let me know if you think I should disable taking screenshots from this activity.

https://codereview.chromium.org/187663005/diff/1/remoting/android/java/src/or...
File remoting/android/java/src/org/chromium/chromoting/HelpActivity.java
(right):

https://codereview.chromium.org/187663005/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/HelpActivity.java:42: private
static Bitmap mScreenshot;
Global variables are evil, but there doesn't seem to be any other simple way of
passing the screenshot from the main activity to the Help activity. We do it the
same way in Chrome's Feedback implementation.

[Jamie, 2014-03-06 01:52:01]

lgtm

https://codereview.chromium.org/187663005/diff/1/remoting/android/java/src/or...
File remoting/android/java/src/org/chromium/chromoting/HelpActivity.java
(right):

https://codereview.chromium.org/187663005/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/HelpActivity.java:40: private
static final int MAX_FEEDBACK_SCREENSHOT_DIMENSION = 600;
Is this assuming a square screenshot?

https://codereview.chromium.org/187663005/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/HelpActivity.java:42: private
static Bitmap mScreenshot;
On 2014/03/06 00:38:30, Lambros wrote:
> Global variables are evil, but there doesn't seem to be any other simple way
of
> passing the screenshot from the main activity to the Help activity. We do it
the
> same way in Chrome's Feedback implementation.

I think this warrants a comment on the member itself, not just in the code
review.

https://codereview.chromium.org/187663005/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/HelpActivity.java:66:
service.transact(Binder.FIRST_CALL_TRANSACTION, parcel, null, 0);
I had to look up FIRST_CALL_TRANSATION. As far as self-documenting code goes,
it's not much better than hard-coding a constant value. Is it worth defining a
constant, named appropriately for what the recipient of this transaction will do
when it processes it?

https://codereview.chromium.org/187663005/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/HelpActivity.java:69:
Log.d("help", "Unexpected error sending feedback: ", ex);
Should this be Log.e?

[Lambros, 2014-03-06 02:31:30]

https://codereview.chromium.org/187663005/diff/1/remoting/android/java/src/or...
File remoting/android/java/src/org/chromium/chromoting/HelpActivity.java
(right):

https://codereview.chromium.org/187663005/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/HelpActivity.java:40: private
static final int MAX_FEEDBACK_SCREENSHOT_DIMENSION = 600;
On 2014/03/06 01:52:01, Jamie wrote:
> Is this assuming a square screenshot?

I don't know :) Chrome uses this same value.

https://codereview.chromium.org/187663005/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/HelpActivity.java:42: private
static Bitmap mScreenshot;
On 2014/03/06 00:38:30, Lambros wrote:
> Global variables are evil, but there doesn't seem to be any other simple way
of
> passing the screenshot from the main activity to the Help activity. We do it
the
> same way in Chrome's Feedback implementation.

Done.

https://codereview.chromium.org/187663005/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/HelpActivity.java:66:
service.transact(Binder.FIRST_CALL_TRANSACTION, parcel, null, 0);
On 2014/03/06 01:52:01, Jamie wrote:
> I had to look up FIRST_CALL_TRANSATION. As far as self-documenting code goes,
> it's not much better than hard-coding a constant value. Is it worth defining a
> constant, named appropriately for what the recipient of this transaction will
do
> when it processes it?

Done, though I'm not sure I've thought of a good name for it.

https://codereview.chromium.org/187663005/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/HelpActivity.java:69:
Log.d("help", "Unexpected error sending feedback: ", ex);
On 2014/03/06 01:52:01, Jamie wrote:
> Should this be Log.e?

Done.

[Lambros, 2014-03-06 02:33:57]

The CQ bit was checked by lambroslambrou@chromium.org

[commit-bot: I haz the power, 2014-03-06 03:22:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/187663005/...

[commit-bot: I haz the power, 2014-03-06 04:07:29]

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

[commit-bot: I haz the power, 2014-03-06 04:07:31]

Retried try job too often on win_rel for step(s) check_deps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Lambros, 2014-03-06 06:03:46]

The CQ bit was checked by lambroslambrou@chromium.org

[commit-bot: I haz the power, 2014-03-06 06:04:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/187663005/...

[commit-bot: I haz the power, 2014-03-06 07:04:43]

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

[commit-bot: I haz the power, 2014-03-06 07:04:44]

Retried try job too often on win_rel for step(s) check_deps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Lambros, 2014-03-06 19:51:10]

yfriedman: Please could you approve DEPS?

[Yaron, 2014-03-06 21:18:12]

lgtm

[Lambros, 2014-03-06 22:02:08]

The CQ bit was checked by lambroslambrou@chromium.org

[commit-bot: I haz the power, 2014-03-06 22:30:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/187663005/...

[commit-bot: I haz the power, 2014-03-07 04:18:47]

Change committed as 255523