Move code about browsing data from PrefServiceBridge to BrowsingDataBridge.

Move code about browsing data from PrefServiceBridge to a new BrowsingDataBridge.

BUG=681523
Review-Url: https://codereview.chromium.org/2756173002
Cr-Commit-Position: refs/heads/master@{#460038}
Committed: https://chromium.googlesource.com/chromium/src/+/c454fad8104929e2624e221aeb2f7a27e9844671

[dullweber, 2017-03-17 14:32:55]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 14:33:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 15:30:50]

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

[commit-bot: I haz the power, 2017-03-17 15:30:51]

Dry run: This issue passed the CQ dry run.

[dullweber, 2017-03-23 14:38:08]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 14:38:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dullweber, 2017-03-23 14:41:00]

Description was changed from

==========
Move code about browsing data from PrefService Bridge to BrowsingDataBridge.

BUG=681523
==========

to

==========
Move code about browsing data from PrefServiceBridge to a new
BrowsingDataBridge.

BUG=681523
==========

[dullweber, 2017-03-23 14:51:06]

dullweber@chromium.org changed reviewers:
+ msramek@chromium.org

[dullweber, 2017-03-23 14:51:07]

Hi Martin,

I created a new bridge class and moved everything browsing data related, that
isn't a getter/setter for a preference, from PrefServiceBridge there. Please
take a look if it is ok this way.

[commit-bot: I haz the power, 2017-03-23 15:43:39]

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

[commit-bot: I haz the power, 2017-03-23 15:43:39]

Dry run: This issue passed the CQ dry run.

[msramek, 2017-03-23 16:02:04]

LGTM, thanks for doing this!

Please make sure to diligently test clear browsing data as well as important
sites, as the bridge had no automated tests.

https://codereview.chromium.org/2756173002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivitySessionTracker.java
(right):

https://codereview.chromium.org/2756173002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivitySessionTracker.java:190:
instance.resetAcceptLanguages(localeString);
nit: This should now be called differently, since it's not the only thing in
this method that is an "instance".

https://codereview.chromium.org/2756173002/diff/20001/chrome/browser/android/...
File chrome/browser/android/browsing_data/browsing_data_bridge.h (right):

https://codereview.chromium.org/2756173002/diff/20001/chrome/browser/android/...
chrome/browser/android/browsing_data/browsing_data_bridge.h:10: class
BrowsingDataBridge {
style: Pure static classes are discouraged.

If you look at other bridges, it seems to be customary the customary approach
for library-like bridge files is:

bool RegisterBrowsingDataBridge(JNIEnv* env);

[dullweber, 2017-03-24 09:19:43]

Thanks, I fixed the issues and tested CBD and Important Sites, everything seems
to work fine

https://codereview.chromium.org/2756173002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivitySessionTracker.java
(right):

https://codereview.chromium.org/2756173002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivitySessionTracker.java:190:
instance.resetAcceptLanguages(localeString);
On 2017/03/23 16:02:04, msramek wrote:
> nit: This should now be called differently, since it's not the only thing in
> this method that is an "instance".

I removed the local variable because now it is only used once.

https://codereview.chromium.org/2756173002/diff/20001/chrome/browser/android/...
File chrome/browser/android/browsing_data/browsing_data_bridge.h (right):

https://codereview.chromium.org/2756173002/diff/20001/chrome/browser/android/...
chrome/browser/android/browsing_data/browsing_data_bridge.h:10: class
BrowsingDataBridge {
On 2017/03/23 16:02:04, msramek wrote:
> style: Pure static classes are discouraged.
> 
> If you look at other bridges, it seems to be customary the customary approach
> for library-like bridge files is:
> 
> bool RegisterBrowsingDataBridge(JNIEnv* env);

I removed the class and renamed the function. Thanks

[dullweber, 2017-03-24 09:51:45]

dullweber@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[dullweber, 2017-03-24 09:51:46]

dfalcantara@chromium.org: Please review these changes. I moved code about
browsing data that was in PrefServiceBridge but has nothing to do with
preferences to a new bridge.

[gone, 2017-03-24 21:07:04]

lgtm

https://codereview.chromium.org/2756173002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataBridge.java
(right):

https://codereview.chromium.org/2756173002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataBridge.java:24:
public interface OnClearBrowsingDataListener { public abstract void
onBrowsingDataCleared(); }
git cl format makes some weird choices, I take it?

https://codereview.chromium.org/2756173002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataBridge.java:82:
return sInstance != null;
Should this be on the UI thread, too?

https://codereview.chromium.org/2756173002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataBridge.java:100:
*      the shared enum {@link
org.chromium.chrome.browser.browsing_data.BrowsingDataType}.
Fix indentation here and onwards? e.g. line up "the" under "An"

[dullweber, 2017-03-27 09:30:00]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 09:30:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dullweber, 2017-03-27 09:43:30]

Thanks, I fixed your comments. Is there a reason that interface methods are
sometimes declared "public abstract" in chromium code? That is the default for
methods inside an interface anyway and Android Studio complained about it.

https://codereview.chromium.org/2756173002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataBridge.java
(right):

https://codereview.chromium.org/2756173002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataBridge.java:24:
public interface OnClearBrowsingDataListener { public abstract void
onBrowsingDataCleared(); }
On 2017/03/24 21:07:04, dfalcantara (load balance plz) wrote:
> git cl format makes some weird choices, I take it?

I just tried git cl format again and it doesn't change anything here. The
"public abstract" isn't neccessary because it is an interface, I removed it but
git cl format still doesn't change anything here.

There are only two formatting changes in
chrome/android/javatests/src/org/chromium/chrome/browser/browsing_data/BrowsingDataRemoverIntegrationTest.java
but that is in code I didn't touch and I'm not sure if it is actually an
improvement so didn't keep it.

https://codereview.chromium.org/2756173002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataBridge.java:82:
return sInstance != null;
On 2017/03/24 21:07:04, dfalcantara (load balance plz) wrote:
> Should this be on the UI thread, too?

I noticed that this method isn't called at all, so I removed it.

https://codereview.chromium.org/2756173002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataBridge.java:100:
*      the shared enum {@link
org.chromium.chrome.browser.browsing_data.BrowsingDataType}.
On 2017/03/24 21:07:04, dfalcantara (load balance plz) wrote:
> Fix indentation here and onwards? e.g. line up "the" under "An"

Done.

[commit-bot: I haz the power, 2017-03-27 10:19:46]

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

[commit-bot: I haz the power, 2017-03-27 10:19:47]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[gone, 2017-03-27 21:29:56]

On 2017/03/27 09:43:30, dullweber wrote:
> Thanks, I fixed your comments. Is there a reason that interface methods are
> sometimes declared "public abstract" in chromium code? That is the default for
> methods inside an interface anyway and Android Studio complained about it.

Eh, likely accidental.  I've accidentally done it in the past when I converted
a class to an interface.

> 
>
https://codereview.chromium.org/2756173002/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataBridge.java
> (right):
> 
>
https://codereview.chromium.org/2756173002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataBridge.java:24:
> public interface OnClearBrowsingDataListener { public abstract void
> onBrowsingDataCleared(); }
> On 2017/03/24 21:07:04, dfalcantara (load balance plz) wrote:
> > git cl format makes some weird choices, I take it?
> 
> I just tried git cl format again and it doesn't change anything here. The
> "public abstract" isn't neccessary because it is an interface, I removed it
but
> git cl format still doesn't change anything here.
> 
> There are only two formatting changes in
>
chrome/android/javatests/src/org/chromium/chrome/browser/browsing_data/BrowsingDataRemoverIntegrationTest.java
> but that is in code I didn't touch and I'm not sure if it is actually an
> improvement so didn't keep it.
> 
>
https://codereview.chromium.org/2756173002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataBridge.java:82:
> return sInstance != null;
> On 2017/03/24 21:07:04, dfalcantara (load balance plz) wrote:
> > Should this be on the UI thread, too?
> 
> I noticed that this method isn't called at all, so I removed it.
> 
>
https://codereview.chromium.org/2756173002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/BrowsingDataBridge.java:100:
> *      the shared enum {@link
> org.chromium.chrome.browser.browsing_data.BrowsingDataType}.
> On 2017/03/24 21:07:04, dfalcantara (load balance plz) wrote:
> > Fix indentation here and onwards? e.g. line up "the" under "An"
> 
> Done.

[dullweber, 2017-03-28 07:51:58]

The CQ bit was checked by dullweber@chromium.org

[dullweber, 2017-03-28 07:51:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2756173002/#ps60001
(title: "fix comments")

[commit-bot: I haz the power, 2017-03-28 07:52:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 07:56:43]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490687518411380,
"parent_rev": "0a7406f924b6c1b1d9f26ffdb1ee23596fb452ef", "commit_rev":
"c454fad8104929e2624e221aeb2f7a27e9844671"}

[commit-bot: I haz the power, 2017-03-28 07:57:34]

Description was changed from

==========
Move code about browsing data from PrefServiceBridge to a new
BrowsingDataBridge.

BUG=681523
==========

to

==========
Move code about browsing data from PrefServiceBridge to a new
BrowsingDataBridge.

BUG=681523

Review-Url: https://codereview.chromium.org/2756173002
Cr-Commit-Position: refs/heads/master@{#460038}
Committed:
https://chromium.googlesource.com/chromium/src/+/c454fad8104929e2624e221aeb2f...
==========

[commit-bot: I haz the power, 2017-03-28 07:57:37]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/c454fad8104929e2624e221aeb2f...