Fix a problem that FeedbackReportingView is stacked infinitely whenever navigating to chrome-distil…

Fix a problem that FeedbackReportingView is stacked infinitely whenever navigating to chrome-distiller://.

FeedbackReporter creates FeedbackReportingView when a given url is reportable and otherwise delete it.
Everything is fine in normal flows, but if we navigate to a reportable url continuously,
FeedbackReportingView is created and stacked as much as we did.
The current call flows are as follows.

A-1. navigating to http://
FeedbackReporter.TabObserver.onUpdateUrl()
FeedbackReporter.dismissOverlay()
FeedbackReporterAndroid::DidNavigateMainFrame()
FeedbackReporter.dismissOverlay()

B-1. navigating to chrome-distiller://
FeedbackReporter.TabObserver.onUpdateUrl()
FeedbackReporter.showOverlay()
FeedbackReporterAndroid::DidNavigateMainFrame()

As we can see, there is no FeedbackReporter.dismissOverlay() call in the 2nd case.
This patch changes the above call flows as follows.

A-2. navigating to http://
FeedbackReporterAndroid::DidNavigateMainFrame()
FeedbackReporter.dismissOverlay()

B-2. navigating to chrome-distiller://
FeedbackReporterAndroid::DidNavigateMainFrame()
FeedbackReporter.dismissOverlay()
FeedbackReporter.showOverlay()

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

[Sungmann Cho, 2014-03-23 23:21:48]

Please take a look. Thanks.

[Sungmann Cho, 2014-03-25 05:28:59]

+shashi
Please take a look. Thanks.

[Sungmann Cho, 2014-03-26 20:59:40]

+Yaron
Please take a look. Thanks.

[nyquist, 2014-03-27 20:16:22]

https://codereview.chromium.org/208263010/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReporter.java
(left):

https://codereview.chromium.org/208263010/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReporter.java:140:
private static native boolean nativeIsReportableUrl(String url);
Instead of removing this, could you move it to
org.chromium.components.dom_distiller.core.DomDistillerUrlUtils and make it
publicly available there? And then use it in
org.chromium.chrome.shell.ChromeShellActivity#prepareMenu?

https://codereview.chromium.org/208263010/diff/1/chrome/browser/android/dom_d...
File chrome/browser/android/dom_distiller/feedback_reporter_android.cc (left):

https://codereview.chromium.org/208263010/diff/1/chrome/browser/android/dom_d...
chrome/browser/android/dom_distiller/feedback_reporter_android.cc:39: jboolean
IsReportableUrl(JNIEnv* env, jclass clazz, jstring j_url_str) {
Move to //components/dom_distiller/core/url_utils_android.[cc|h]. It would
require also passing in the scheme.

[Sungmann Cho, 2014-03-28 01:28:45]

Thank you for your comments.
I submit a revised patch.
Please take a look.

https://codereview.chromium.org/208263010/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReporter.java
(left):

https://codereview.chromium.org/208263010/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/FeedbackReporter.java:140:
private static native boolean nativeIsReportableUrl(String url);
On 2014/03/27 20:16:22, nyquist wrote:
> Instead of removing this, could you move it to
> org.chromium.components.dom_distiller.core.DomDistillerUrlUtils and make it
> publicly available there? And then use it in
> org.chromium.chrome.shell.ChromeShellActivity#prepareMenu?

Done.

https://codereview.chromium.org/208263010/diff/1/chrome/browser/android/dom_d...
File chrome/browser/android/dom_distiller/feedback_reporter_android.cc (left):

https://codereview.chromium.org/208263010/diff/1/chrome/browser/android/dom_d...
chrome/browser/android/dom_distiller/feedback_reporter_android.cc:39: jboolean
IsReportableUrl(JNIEnv* env, jclass clazz, jstring j_url_str) {
On 2014/03/27 20:16:22, nyquist wrote:
> Move to //components/dom_distiller/core/url_utils_android.[cc|h]. It would
> require also passing in the scheme.

Done.

[Sungmann Cho, 2014-03-31 23:03:00]

ping...

[nyquist, 2014-03-31 23:08:25]

mostly lg

https://codereview.chromium.org/208263010/diff/20001/chrome/android/shell/jav...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java
(right):

https://codereview.chromium.org/208263010/diff/20001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java:351:
CHROME_DISTILLER_SCHEME, activeTab.getUrl()));
Use |url| here.

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

https://codereview.chromium.org/208263010/diff/20001/chrome/browser/android/d...
chrome/browser/android/dom_distiller/feedback_reporter_android.cc:58: if
(url.is_valid() && url.scheme() == chrome::kDomDistillerScheme)
Could you merge this with the other check int
components/dom_distiller/core/url_utils_android.cc into url_utils.[cc|h]?
That way the logic only lives one place if we change it in the future.

https://codereview.chromium.org/208263010/diff/20001/components/dom_distiller...
File components/dom_distiller/core/url_utils_android.cc (right):

https://codereview.chromium.org/208263010/diff/20001/components/dom_distiller...
components/dom_distiller/core/url_utils_android.cc:43: if (!url.is_valid()) {
Could you move this last part to the normal url utils as a static, and call it
from here?

[Sungmann Cho, 2014-04-01 07:51:38]

Please take a look. Thanks.

https://codereview.chromium.org/208263010/diff/20001/chrome/android/shell/jav...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java
(right):

https://codereview.chromium.org/208263010/diff/20001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java:351:
CHROME_DISTILLER_SCHEME, activeTab.getUrl()));
On 2014/03/31 23:08:25, nyquist wrote:
> Use |url| here.

Done.

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

https://codereview.chromium.org/208263010/diff/20001/chrome/browser/android/d...
chrome/browser/android/dom_distiller/feedback_reporter_android.cc:58: if
(url.is_valid() && url.scheme() == chrome::kDomDistillerScheme)
On 2014/03/31 23:08:25, nyquist wrote:
> Could you merge this with the other check int
> components/dom_distiller/core/url_utils_android.cc into url_utils.[cc|h]?
> That way the logic only lives one place if we change it in the future.

Done.

https://codereview.chromium.org/208263010/diff/20001/components/dom_distiller...
File components/dom_distiller/core/url_utils_android.cc (right):

https://codereview.chromium.org/208263010/diff/20001/components/dom_distiller...
components/dom_distiller/core/url_utils_android.cc:43: if (!url.is_valid()) {
On 2014/03/31 23:08:25, nyquist wrote:
> Could you move this last part to the normal url utils as a static, and call it
> from here?

Done.

[nyquist, 2014-04-01 20:09:13]

lgtm with comment. thanks.

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

https://codereview.chromium.org/208263010/diff/40002/chrome/browser/android/d...
chrome/browser/android/dom_distiller/feedback_reporter_android.cc:60:
chrome::kDomDistillerScheme, url))
Since the conditional is two lines now, could you please add curly braces?

[Sungmann Cho, 2014-04-01 22:11:59]

I fixed above nits. Thank you for your guidance.

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

https://codereview.chromium.org/208263010/diff/40002/chrome/browser/android/d...
chrome/browser/android/dom_distiller/feedback_reporter_android.cc:60:
chrome::kDomDistillerScheme, url))
On 2014/04/01 20:09:14, nyquist wrote:
> Since the conditional is two lines now, could you please add curly braces?

Done.

[Sungmann Cho, 2014-04-01 22:12:14]

The CQ bit was checked by sungmann.cho@navercorp.com

[commit-bot: I haz the power, 2014-04-01 22:54:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sungmann.cho@navercorp.com/208263010/7...

[commit-bot: I haz the power, 2014-04-02 02:07:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sungmann.cho@navercorp.com/208263010/7...

[commit-bot: I haz the power, 2014-04-02 03:50:48]

Change committed as 261053