Add FeedbackReporter for reporting distillation quality.

Add feedback reporting for the DOM Distiller

This adds support for reporting whether the perceived quality of the
distillation of a web page is good. The user choice is recorded as
UMA stats.

This CL only implements a UI for Android.

BUG=319881
R=jar@chromium.org, newt@chromium.org, shashishekhar@chromium.org

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

[nyquist, 2014-03-12 17:50:37]

yfriedman: PTAL

[nyquist, 2014-03-14 19:05:27]

jar: PTAL tools/metrics/histograms/
dfalcantara: UI and usage of SwipableOverlayView
shashishekhar: *dom_distiller*

[shashi, 2014-03-14 20:33:24]

dom_distiller: lgtm with nits.

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReporter.java
(right):

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReporter.java:88:
* Dismiss the overlay which is currently being displayed.
remove this javadoc.

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReportingView.java
(right):

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReportingView.java:21:
* <p/>
nit: <p/> missing <p>.

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReportingView.java:53:
* Creates a BannerView and adds it to the given ContentView.
nit: BannerView or FeedbackReportingView?

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReportingView.java:111:
mNoButton.setClickable(false);
this seems to be really exhaustive :).

[jar (doing other things), 2014-03-14 23:54:46]

histograms.xml LGTM

[nyquist, 2014-03-15 00:09:40]

newt: PTAL UI

[newt (away), 2014-03-17 18:19:19]

just nits

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/res/...
File chrome/android/java/res/layout/feedback_reporting_view.xml (right):

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/res/...
chrome/android/java/res/layout/feedback_reporting_view.xml:11:
android:id="@+id/feedback_reporting_view"
nit, put id before all other android: attributes

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/res/...
chrome/android/java/res/layout/feedback_reporting_view.xml:16:
android:layout_height="match_parent"
wrap_content

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/res/...
chrome/android/java/res/layout/feedback_reporting_view.xml:28:
android:layout_width="match_parent"
layout_width="0dp" for all four of these views

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/res/...
chrome/android/java/res/layout/feedback_reporting_view.xml:52:
android:layout_height="match_parent"
wrap_content

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReporter.java
(right):

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReporter.java:44:
* Created the FeedbackReporter, adds itself as a TabObserver, and ensures
Created -> Creates

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReporter.java:99:
if (mContentView != mTab.getContentView()) mContentView = mTab.getContentView();
What's the point of this if statement? Do you mean for
nativeReplaceWebContents() also to be inside of the if clause? If not, I'd just
remove the if statement.

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReportingView.java
(right):

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReportingView.java:46:
interface Observer {
The code below might be clearer if this name were a bit more specific. Maybe:
FeedbackObserver

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReportingView.java:69:
* Creates a SwipableOverlayView.
FeedbackReportingView

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReportingView.java:116:
* This is overridden since the method is protected in the parent {@link
SwipableOverlayView}.
Why is this needed?

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/stri...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/stri...
chrome/android/java/strings/android_chrome_strings.grd:286: <!-- DOM Distiller
strings -->
nit: add empty line above

https://codereview.chromium.org/197473006/diff/40001/chrome/browser/android/d...
File chrome/browser/android/dom_distiller/feedback_reporter_android.cc (right):

https://codereview.chromium.org/197473006/diff/40001/chrome/browser/android/d...
chrome/browser/android/dom_distiller/feedback_reporter_android.cc:52: void
FeedbackReporterAndroid::Destroy(JNIEnv* env, jobject obj) { delete this; }
nit: "delete this" should probably go on a new line

[nyquist, 2014-03-17 20:13:41]

newt: PTAL

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/res/...
File chrome/android/java/res/layout/feedback_reporting_view.xml (right):

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/res/...
chrome/android/java/res/layout/feedback_reporting_view.xml:11:
android:id="@+id/feedback_reporting_view"
On 2014/03/17 18:19:19, newt wrote:
> nit, put id before all other android: attributes

Done.

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/res/...
chrome/android/java/res/layout/feedback_reporting_view.xml:16:
android:layout_height="match_parent"
On 2014/03/17 18:19:19, newt wrote:
> wrap_content

Done.

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/res/...
chrome/android/java/res/layout/feedback_reporting_view.xml:28:
android:layout_width="match_parent"
On 2014/03/17 18:19:19, newt wrote:
> layout_width="0dp" for all four of these views

Done.

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/res/...
chrome/android/java/res/layout/feedback_reporting_view.xml:52:
android:layout_height="match_parent"
On 2014/03/17 18:19:19, newt wrote:
> wrap_content

Done.

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReporter.java
(right):

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReporter.java:44:
* Created the FeedbackReporter, adds itself as a TabObserver, and ensures
On 2014/03/17 18:19:19, newt wrote:
> Created -> Creates

Done.

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReporter.java:88:
* Dismiss the overlay which is currently being displayed.
On 2014/03/14 20:33:24, shashi wrote:
> remove this javadoc.

Done.

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReporter.java:99:
if (mContentView != mTab.getContentView()) mContentView = mTab.getContentView();
On 2014/03/17 18:19:19, newt wrote:
> What's the point of this if statement? Do you mean for
> nativeReplaceWebContents() also to be inside of the if clause? If not, I'd
just
> remove the if statement.

Done.

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReportingView.java
(right):

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReportingView.java:21:
* <p/>
On 2014/03/14 20:33:24, shashi wrote:
> nit: <p/> missing <p>.

As a standalone <p/> I think this is fine. It is at least what we do many other
places in the codebase.

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReportingView.java:46:
interface Observer {
On 2014/03/17 18:19:19, newt wrote:
> The code below might be clearer if this name were a bit more specific. Maybe:
> FeedbackObserver

Done.

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReportingView.java:53:
* Creates a BannerView and adds it to the given ContentView.
On 2014/03/14 20:33:24, shashi wrote:
> nit: BannerView or FeedbackReportingView?

Done.

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReportingView.java:69:
* Creates a SwipableOverlayView.
On 2014/03/17 18:19:19, newt wrote:
> FeedbackReportingView

Done.

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReportingView.java:111:
mNoButton.setClickable(false);
On 2014/03/14 20:33:24, shashi wrote:
> this seems to be really exhaustive :).

Done.

Kept setEnabled(false) and cleared OnClickListener to ensure we don't leak
anything, even though setEnabled(false) is enough to get the effect I want.

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReportingView.java:116:
* This is overridden since the method is protected in the parent {@link
SwipableOverlayView}.
On 2014/03/17 18:19:19, newt wrote:
> Why is this needed?

Added an exhaustive comment.

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/stri...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/197473006/diff/40001/chrome/android/java/stri...
chrome/android/java/strings/android_chrome_strings.grd:286: <!-- DOM Distiller
strings -->
On 2014/03/17 18:19:19, newt wrote:
> nit: add empty line above

Done.

https://codereview.chromium.org/197473006/diff/40001/chrome/browser/android/d...
File chrome/browser/android/dom_distiller/feedback_reporter_android.cc (right):

https://codereview.chromium.org/197473006/diff/40001/chrome/browser/android/d...
chrome/browser/android/dom_distiller/feedback_reporter_android.cc:52: void
FeedbackReporterAndroid::Destroy(JNIEnv* env, jobject obj) { delete this; }
On 2014/03/17 18:19:19, newt wrote:
> nit: "delete this" should probably go on a new line

Not according to clang format. Keeping as is since it's a nit, and I usually
just follow clang format for simplicity :-) If I move it to a new line an re-run
clang it would just move up again.

[newt (away), 2014-03-17 20:51:20]

lgtm

Looking forward to seeing this in action!

[nyquist, 2014-03-17 21:27:35]

The CQ bit was checked by nyquist@chromium.org

[commit-bot: I haz the power, 2014-03-17 21:28:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nyquist@chromium.org/197473006/80001

[commit-bot: I haz the power, 2014-03-17 21:35:39]

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

[commit-bot: I haz the power, 2014-03-17 21:35:40]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[nyquist, 2014-03-17 21:47:44]

The CQ bit was checked by nyquist@chromium.org

[commit-bot: I haz the power, 2014-03-17 21:53:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nyquist@chromium.org/197473006/80001

[nyquist, 2014-03-17 22:20:00]

The CQ bit was unchecked by nyquist@chromium.org

[nyquist, 2014-03-17 22:23:21]

The CQ bit was checked by nyquist@chromium.org

[nyquist, 2014-03-17 22:57:07]

Committed patchset #6 manually as r257506 (presubmit successful).