Added pref for snapshot document collapsed state

Added pref for snapshot document collapsed state

* Added boolean preference to persist other devices snapshot document
  collapsed/expanded state.
* Added JNI bridge to set/expand state.

NOTRY=true
BUG=294401
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232874

[apiccion, 2013-10-21 22:22:50]

This CL is ready for review.

[newt (away), 2013-10-21 23:27:45]

Are you planning to use SnapshotDocumentHelper.java for more than just storing
the preference? If you just need a way to update a preference, you could use
chrome_native_preferences.cc (though I think there are plans to reduce usage of
that class), or perhaps the recommended upstream approach.... which Aurimas or
Yaron might be able to comment on.

Aurimas/Yaron, what's the recommendation these days for modifying preferences
from Java? Adding a Java/C++ bridge class for a single preference seems like
major overkill.

[Yaron, 2013-10-22 00:23:31]

On 2013/10/21 23:27:45, newt wrote:
> Are you planning to use SnapshotDocumentHelper.java for more than just storing
> the preference? If you just need a way to update a preference, you could use
> chrome_native_preferences.cc (though I think there are plans to reduce usage
of
> that class), or perhaps the recommended upstream approach.... which Aurimas or
> Yaron might be able to comment on.
> 
> Aurimas/Yaron, what's the recommendation these days for modifying preferences
> from Java? Adding a Java/C++ bridge class for a single preference seems like
> major overkill.

I agree that a separate class for each one is overkill. We've tried to place
them with relevant functionality. E.g. sync prefs in ProfileSyncService,
autofill prefs in PersonalDataManager, search engine ones in TemplateUrlService.
If we need to introduce a catch-all for others that don't fit (e.g.
chrome_native_preferences) we can do that, but I'd like to see this CL set the
stage for others to move in there. Essentially exposing a PrefService upstream
that others could move to. Alternatively, can this live in an NTP-related class
since it's focused on NTP presentation logic?

[aurimas (slooooooooow), 2013-10-22 00:32:12]

On 2013/10/22 00:23:31, Yaron wrote:
> On 2013/10/21 23:27:45, newt wrote:
> > Are you planning to use SnapshotDocumentHelper.java for more than just
storing
> > the preference? If you just need a way to update a preference, you could use
> > chrome_native_preferences.cc (though I think there are plans to reduce usage
> of
> > that class), or perhaps the recommended upstream approach.... which Aurimas
or
> > Yaron might be able to comment on.
> > 
> > Aurimas/Yaron, what's the recommendation these days for modifying
preferences
> > from Java? Adding a Java/C++ bridge class for a single preference seems like
> > major overkill.
> 
> I agree that a separate class for each one is overkill. We've tried to place
> them with relevant functionality. E.g. sync prefs in ProfileSyncService,
> autofill prefs in PersonalDataManager, search engine ones in
TemplateUrlService.
> If we need to introduce a catch-all for others that don't fit (e.g.
> chrome_native_preferences) we can do that, but I'd like to see this CL set the
> stage for others to move in there. Essentially exposing a PrefService upstream
> that others could move to. Alternatively, can this live in an NTP-related
class
> since it's focused on NTP presentation logic?

Ideally it could live in a place where it is used. But if it really is an
orphan, then we should maybe create an upstream class PrefService.java. What do
you guys think?

[apiccion, 2013-10-30 22:53:43]

Grouped other devices prefs into one class. Let me know what you think of this
approach.

I will need to break this into two upstream CLs (to do the foreign session
refactor without breaking downstream)

[newt (away), 2013-10-30 23:47:51]

aurimas or yfriedman, could you take a look too? I'm not very familiar with how
preferences are "supposed" to work.

https://codereview.chromium.org/33863002/diff/100001/chrome/android/java/src/...
File chrome/android/java/src/org/chromium/chrome/browser/NewTabPagePrefs.java
(right):

https://codereview.chromium.org/33863002/diff/100001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/NewTabPagePrefs.java:11: *
This class exposes to Java information about snapshot document preferences.
"allows Java code to read and modify preferences related to the NTP"

https://codereview.chromium.org/33863002/diff/100001/chrome/browser/android/n...
File chrome/browser/android/new_tab_page_prefs.cc (right):

https://codereview.chromium.org/33863002/diff/100001/chrome/browser/android/n...
chrome/browser/android/new_tab_page_prefs.cc:20: NewTabPagePrefs *
new_tab_page_prefs =
no space before *

https://codereview.chromium.org/33863002/diff/100001/chrome/browser/android/n...
chrome/browser/android/new_tab_page_prefs.cc:21: new
NewTabPagePrefs(ProfileAndroid::FromProfileAndroid(profile));
indent by 4 on wrapped lines

https://codereview.chromium.org/33863002/diff/100001/chrome/browser/android/n...
chrome/browser/android/new_tab_page_prefs.cc:26: : profile_(profile){
space before {

https://codereview.chromium.org/33863002/diff/100001/chrome/browser/android/n...
chrome/browser/android/new_tab_page_prefs.cc:98: registry->RegisterBooleanPref(
unindent

https://codereview.chromium.org/33863002/diff/100001/chrome/browser/android/n...
File chrome/browser/android/new_tab_page_prefs.h (right):

https://codereview.chromium.org/33863002/diff/100001/chrome/browser/android/n...
chrome/browser/android/new_tab_page_prefs.h:44: Profile* profile_;  // weak
could we store a pointer to PrefService instead of Profile? not sure which is
better.

https://codereview.chromium.org/33863002/diff/100001/chrome/browser/prefs/bro...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/33863002/diff/100001/chrome/browser/prefs/bro...
chrome/browser/prefs/browser_prefs.cc:13: #include
"chrome/browser/android/new_tab_page_prefs.h"
platform-specific defines should go at the end of the include list, I believe

[Yaron, 2013-10-31 20:57:34]

This seems fine and is consistent with elsewhere. I do worry if we have to do
this in general whether it's better to reduce the JNI barrier a little and not
introduce fn's for everything. For example, every pref currently needs 4
functions, 2 of which cross the barrier. Another option would be to expose the
public functions but internally make use of a common helper
Set{Boolean,String,...}Pref and Get{Boolean,String,...}Pref. I haven't though it
fully through but I'm sure we can code it up in a way that ensures type-safety
(we have the gcc macro generator) and actually makes it easier to add a new pref
anywhere in the code base. If you want to explore as a follow-up that's fine
with me.

https://codereview.chromium.org/33863002/diff/100001/chrome/browser/android/n...
File chrome/browser/android/new_tab_page_prefs.cc (right):

https://codereview.chromium.org/33863002/diff/100001/chrome/browser/android/n...
chrome/browser/android/new_tab_page_prefs.cc:101:
user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
Thinking out loud:I wonder whether we'll want these to be syncable. Currently we
don't sync prefs on Android but Miguel is adding that. These prefs don't make
sense on desktop but syncing them would mean that when you get a new phone (or
across phones) these preferences sync. Ok to leave for now if you want, but
throwing it out there.

[apiccion, 2013-11-01 23:30:31]

https://codereview.chromium.org/33863002/diff/100001/chrome/android/java/src/...
File chrome/android/java/src/org/chromium/chrome/browser/NewTabPagePrefs.java
(right):

https://codereview.chromium.org/33863002/diff/100001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/NewTabPagePrefs.java:11: *
This class exposes to Java information about snapshot document preferences.
On 2013/10/30 23:47:51, newt wrote:
> "allows Java code to read and modify preferences related to the NTP"

Done.

https://codereview.chromium.org/33863002/diff/100001/chrome/browser/android/n...
File chrome/browser/android/new_tab_page_prefs.cc (right):

https://codereview.chromium.org/33863002/diff/100001/chrome/browser/android/n...
chrome/browser/android/new_tab_page_prefs.cc:20: NewTabPagePrefs *
new_tab_page_prefs =
On 2013/10/30 23:47:51, newt wrote:
> no space before *

Done.

https://codereview.chromium.org/33863002/diff/100001/chrome/browser/android/n...
chrome/browser/android/new_tab_page_prefs.cc:21: new
NewTabPagePrefs(ProfileAndroid::FromProfileAndroid(profile));
On 2013/10/30 23:47:51, newt wrote:
> indent by 4 on wrapped lines

Done.

https://codereview.chromium.org/33863002/diff/100001/chrome/browser/android/n...
chrome/browser/android/new_tab_page_prefs.cc:26: : profile_(profile){
On 2013/10/30 23:47:51, newt wrote:
> space before {

Done.

https://codereview.chromium.org/33863002/diff/100001/chrome/browser/android/n...
chrome/browser/android/new_tab_page_prefs.cc:98: registry->RegisterBooleanPref(
On 2013/10/30 23:47:51, newt wrote:
> unindent

Done.

https://codereview.chromium.org/33863002/diff/100001/chrome/browser/android/n...
chrome/browser/android/new_tab_page_prefs.cc:101:
user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
On 2013/10/31 20:57:34, Yaron wrote:
> Thinking out loud:I wonder whether we'll want these to be syncable. Currently
we
> don't sync prefs on Android but Miguel is adding that. These prefs don't make
> sense on desktop but syncing them would mean that when you get a new phone (or
> across phones) these preferences sync. Ok to leave for now if you want, but
> throwing it out there.

These particular preferences are device specific (recently closed tabs, snapshot
documents and sync promo). 

However, we could consider syncing the foreign session collapsed (currently
registered in webui, I'll have to write a CL to move it here)

https://codereview.chromium.org/33863002/diff/100001/chrome/browser/android/n...
File chrome/browser/android/new_tab_page_prefs.h (right):

https://codereview.chromium.org/33863002/diff/100001/chrome/browser/android/n...
chrome/browser/android/new_tab_page_prefs.h:44: Profile* profile_;  // weak
On 2013/10/30 23:47:51, newt wrote:
> could we store a pointer to PrefService instead of Profile? not sure which is
> better.

I'm apprehensive because the preferences pointer may change (e.g. the underlying
object may be reconstructed) and we could end up holding onto a dead pointer.
I'm not sure if this actually happens, but it's not clear from the profile.h
interface that GetPrefs() will always return the same object.

https://codereview.chromium.org/33863002/diff/100001/chrome/browser/prefs/bro...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/33863002/diff/100001/chrome/browser/prefs/bro...
chrome/browser/prefs/browser_prefs.cc:13: #include
"chrome/browser/android/new_tab_page_prefs.h"
On 2013/10/30 23:47:51, newt wrote:
> platform-specific defines should go at the end of the include list, I believe

Done.

[newt (away), 2013-11-01 23:49:18]

lgtm

https://codereview.chromium.org/33863002/diff/100001/chrome/browser/android/n...
File chrome/browser/android/new_tab_page_prefs.h (right):

https://codereview.chromium.org/33863002/diff/100001/chrome/browser/android/n...
chrome/browser/android/new_tab_page_prefs.h:44: Profile* profile_;  // weak
On 2013/11/01 23:30:32, apiccion wrote:
> On 2013/10/30 23:47:51, newt wrote:
> > could we store a pointer to PrefService instead of Profile? not sure which
is
> > better.
> 
> I'm apprehensive because the preferences pointer may change (e.g. the
underlying
> object may be reconstructed) and we could end up holding onto a dead pointer.
> I'm not sure if this actually happens, but it's not clear from the profile.h
> interface that GetPrefs() will always return the same object.

sounds good to me.

[Yaron, 2013-11-02 01:04:33]

lgtm but I think you need a chrome/browser/prefs OWNER

[apiccion, 2013-11-02 01:12:11]

@pam
Seeking owners approval for:
chrome/browser/prefs/browser_prefs.cc

[Pam (message me for reviews), 2013-11-02 08:27:24]

LGTM once nits below are fixed.

- Pam

https://codereview.chromium.org/33863002/diff/200001/chrome/browser/prefs/bro...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/33863002/diff/200001/chrome/browser/prefs/bro...
chrome/browser/prefs/browser_prefs.cc:363: #if defined(OS_ANDROID)
Please put this in with the other OS_ANDROID section (with
PartnerBookmarksShim:: etc.) below.

https://codereview.chromium.org/33863002/diff/200001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/33863002/diff/200001/chrome/common/pref_names...
chrome/common/pref_names.cc:1714: // Keeps track of snapshot documents collapsed
state in the Other Devices menu.
nit: alphabetize to match newly alphabetized :) kNtp section of the .h file

https://codereview.chromium.org/33863002/diff/200001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/33863002/diff/200001/chrome/common/pref_names...
chrome/common/pref_names.h:573: extern const char
kNtpCollapsedSnapshotDocument[];
nit: alphabetize (the whole kNtp section, I notice)

[apiccion, 2013-11-03 17:52:58]

https://codereview.chromium.org/33863002/diff/200001/chrome/browser/prefs/bro...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/33863002/diff/200001/chrome/browser/prefs/bro...
chrome/browser/prefs/browser_prefs.cc:363: #if defined(OS_ANDROID)
On 2013/11/02 08:27:25, Pam (also PM for reviews) wrote:
> Please put this in with the other OS_ANDROID section (with
> PartnerBookmarksShim:: etc.) below.

Done.

https://codereview.chromium.org/33863002/diff/200001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/33863002/diff/200001/chrome/common/pref_names...
chrome/common/pref_names.cc:1714: // Keeps track of snapshot documents collapsed
state in the Other Devices menu.
On 2013/11/02 08:27:25, Pam (also PM for reviews) wrote:
> nit: alphabetize to match newly alphabetized :) kNtp section of the .h file

Done.

https://codereview.chromium.org/33863002/diff/200001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/33863002/diff/200001/chrome/common/pref_names...
chrome/common/pref_names.h:573: extern const char
kNtpCollapsedSnapshotDocument[];
On 2013/11/02 08:27:25, Pam (also PM for reviews) wrote:
> nit: alphabetize (the whole kNtp section, I notice)

Done.

[commit-bot: I haz the power, 2013-11-03 17:53:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apiccion@chromium.org/33863002/360001

[commit-bot: I haz the power, 2013-11-03 20:11:16]

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

[commit-bot: I haz the power, 2013-11-03 23:34:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apiccion@chromium.org/33863002/360001

[commit-bot: I haz the power, 2013-11-04 01:41:35]

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

[commit-bot: I haz the power, 2013-11-04 18:28:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apiccion@chromium.org/33863002/360001

[commit-bot: I haz the power, 2013-11-04 18:28:20]

Failed to apply patch for chrome/browser/prefs/browser_prefs.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/prefs/browser_prefs.cc
  Hunk #1 FAILED at 49.
  Hunk #2 FAILED at 170.
  Hunk #3 succeeded at 386 with fuzz 1 (offset -9 lines).
  2 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/prefs/browser_prefs.cc.rej

Patch:       chrome/browser/prefs/browser_prefs.cc
Index: chrome/browser/prefs/browser_prefs.cc
diff --git a/chrome/browser/prefs/browser_prefs.cc
b/chrome/browser/prefs/browser_prefs.cc
index
e59b3494be589e3bf3e4497e19832bd4a977a2c5..5c789e9628b0319c44b55256ebd3139d39ad71c5
100644
--- a/chrome/browser/prefs/browser_prefs.cc
+++ b/chrome/browser/prefs/browser_prefs.cc
@@ -49,9 +49,6 @@
 #include "chrome/browser/network_time/network_time_service.h"
 #include "chrome/browser/notifications/desktop_notification_service.h"
 #include "chrome/browser/notifications/notification_prefs_manager.h"
-#if !defined(OS_ANDROID)
-#include "chrome/browser/notifications/sync_notifier/chrome_notifier_service.h"
-#endif  // OS_ANDROID
 #include "chrome/browser/password_manager/password_generation_manager.h"
 #include "chrome/browser/password_manager/password_manager.h"
 #include "chrome/browser/pepper_flash_settings_manager.h"
@@ -170,9 +167,11 @@
 #endif
 
 #if defined(OS_ANDROID)
+#include "chrome/browser/android/new_tab_page_prefs.h"
 #include "chrome/browser/ui/webui/ntp/android/partner_bookmarks_shim.h"
 #include "chrome/browser/ui/webui/ntp/android/promo_handler.h"
 #else
+#include "chrome/browser/notifications/sync_notifier/chrome_notifier_service.h"
 #include "chrome/browser/profile_resetter/automatic_profile_resetter_factory.h"
 #include "chrome/browser/ui/autofill/generated_credit_card_bubble_controller.h"
 #endif
@@ -395,6 +394,7 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable*
registry) {
 #endif
 
 #if defined(OS_ANDROID)
+  NewTabPagePrefs::RegisterProfilePrefs(registry);
   PartnerBookmarksShim::RegisterProfilePrefs(registry);
   PromoHandler::RegisterProfilePrefs(registry);
 #endif

[commit-bot: I haz the power, 2013-11-04 18:35:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apiccion@chromium.org/33863002/700001

[commit-bot: I haz the power, 2013-11-04 18:35:47]

Failed to apply patch for chrome/browser/prefs/browser_prefs.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/prefs/browser_prefs.cc
  Hunk #1 FAILED at 49.
  Hunk #2 FAILED at 170.
  Hunk #3 succeeded at 386 with fuzz 1 (offset -13 lines).
  2 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/prefs/browser_prefs.cc.rej

Patch:       chrome/browser/prefs/browser_prefs.cc
Index: chrome/browser/prefs/browser_prefs.cc
diff --git a/chrome/browser/prefs/browser_prefs.cc
b/chrome/browser/prefs/browser_prefs.cc
index
58eb9c2215c77525d611403ff3e4eb8a241b101f..9d4baa6e3e3aa00823e4f32fef16d1dbd884a757
100644
--- a/chrome/browser/prefs/browser_prefs.cc
+++ b/chrome/browser/prefs/browser_prefs.cc
@@ -49,9 +49,6 @@
 #include "chrome/browser/network_time/network_time_service.h"
 #include "chrome/browser/notifications/desktop_notification_service.h"
 #include "chrome/browser/notifications/notification_prefs_manager.h"
-#if !defined(OS_ANDROID)
-#include "chrome/browser/notifications/sync_notifier/chrome_notifier_service.h"
-#endif  // OS_ANDROID
 #include "chrome/browser/password_manager/password_generation_manager.h"
 #include "chrome/browser/password_manager/password_manager.h"
 #include "chrome/browser/pepper_flash_settings_manager.h"
@@ -170,9 +167,11 @@
 #endif
 
 #if defined(OS_ANDROID)
+#include "chrome/browser/android/new_tab_page_prefs.h"
 #include "chrome/browser/ui/webui/ntp/android/partner_bookmarks_shim.h"
 #include "chrome/browser/ui/webui/ntp/android/promo_handler.h"
 #else
+#include "chrome/browser/notifications/sync_notifier/chrome_notifier_service.h"
 #include "chrome/browser/profile_resetter/automatic_profile_resetter_factory.h"
 #include "chrome/browser/ui/autofill/generated_credit_card_bubble_controller.h"
 #endif
@@ -399,6 +398,7 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable*
registry) {
 #endif
 
 #if defined(OS_ANDROID)
+  NewTabPagePrefs::RegisterProfilePrefs(registry);
   PartnerBookmarksShim::RegisterProfilePrefs(registry);
   PromoHandler::RegisterProfilePrefs(registry);
 #endif

[newt (away), 2013-11-04 18:47:40]

Looks like you need to rebase against origin/master.

 > git checkout your_branch
 > git branch --set-upstream-to=origin/master
 > git rebase
 > git cl upload

[newt (away), 2013-11-04 18:48:18]

On 2013/11/04 18:47:40, newt wrote:
> Looks like you need to rebase against origin/master.
> 
>  > git checkout your_branch
>  > git branch --set-upstream-to=origin/master
>  > git rebase
>  > git cl upload

doing a gclient sync in there would be a good idea too

[commit-bot: I haz the power, 2013-11-04 19:23:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apiccion@chromium.org/33863002/770001

[commit-bot: I haz the power, 2013-11-05 00:42:14]

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

[commit-bot: I haz the power, 2013-11-05 01:06:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apiccion@chromium.org/33863002/770001

[commit-bot: I haz the power, 2013-11-05 01:33:06]

Change committed as 232874