Wired preference that hides hosted documents for the directory view.

Wired preference that hides hosted documents for the directory view.

BUG=chromium-os:28907
TEST=open file maneger, flip hosted documents option back and forth

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

[achuithb, 2012-04-04 19:14:11]

https://chromiumcodereview.appspot.com/9947002/diff/1001/chrome/browser/chrom...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

https://chromiumcodereview.appspot.com/9947002/diff/1001/chrome/browser/chrom...
chrome/browser/chromeos/gdata/gdata_file_system.cc:3820:
pref_observer_.reset(new PrefSetObserver(profile_->GetPrefs(), this));
pref_observer_ doesn't seem to do anything? What am I missing?

https://chromiumcodereview.appspot.com/9947002/diff/1001/chrome/browser/chrom...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

https://chromiumcodereview.appspot.com/9947002/diff/1001/chrome/browser/chrom...
chrome/browser/chromeos/gdata/gdata_file_system.h:402: virtual bool
hide_hosted_documents() = 0;
Shouldn't this be HideHostedDocuments?

[zel, 2012-04-04 20:16:57]

https://chromiumcodereview.appspot.com/9947002/diff/1001/chrome/browser/chrom...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

https://chromiumcodereview.appspot.com/9947002/diff/1001/chrome/browser/chrom...
chrome/browser/chromeos/gdata/gdata_file_system.cc:3820:
pref_observer_.reset(new PrefSetObserver(profile_->GetPrefs(), this));
On 2012/04/04 19:14:11, achuith.bhandarkar wrote:
> pref_observer_ doesn't seem to do anything? What am I missing?

it does. it's a helper class for registering for monitoring changed in a
specific set of preferences

https://chromiumcodereview.appspot.com/9947002/diff/1001/chrome/browser/chrom...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

https://chromiumcodereview.appspot.com/9947002/diff/1001/chrome/browser/chrom...
chrome/browser/chromeos/gdata/gdata_file_system.h:402: virtual bool
hide_hosted_documents() = 0;
On 2012/04/04 19:14:11, achuith.bhandarkar wrote:
> Shouldn't this be HideHostedDocuments?

it's a simple getter, unlike its setter method that does way more when we flip
that state

[achuithb, 2012-04-04 20:46:19]

LGTM

http://codereview.chromium.org/9947002/diff/1001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/9947002/diff/1001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:3820:
pref_observer_.reset(new PrefSetObserver(profile_->GetPrefs(), this));
On 2012/04/04 20:16:57, zel wrote:
> On 2012/04/04 19:14:11, achuith.bhandarkar wrote:
> > pref_observer_ doesn't seem to do anything? What am I missing?
> 
> it does. it's a helper class for registering for monitoring changed in a
> specific set of preferences

Ah, got it. It takes care of the NotificationRegistrar business, and forwards
Observe notifications.

http://codereview.chromium.org/9947002/diff/1001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

http://codereview.chromium.org/9947002/diff/1001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.h:402: virtual bool
hide_hosted_documents() = 0;
On 2012/04/04 20:16:57, zel wrote:
> On 2012/04/04 19:14:11, achuith.bhandarkar wrote:
> > Shouldn't this be HideHostedDocuments?
> 
> it's a simple getter, unlike its setter method that does way more when we flip
> that state

I'm not sure what the rule is with getters when they are pure virtual, but this
is a nit anyway.

[satorux1, 2012-04-04 21:23:16]

http://codereview.chromium.org/9947002/diff/1001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/9947002/diff/1001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2707: }
Please remove TODOs. My part won't need code in GDataFileSystem.

http://codereview.chromium.org/9968105/

[satorux1, 2012-04-04 21:47:11]

http://codereview.chromium.org/9947002/diff/1001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/9947002/diff/1001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:3820:
pref_observer_.reset(new PrefSetObserver(profile_->GetPrefs(), this));
On 2012/04/04 20:46:19, achuith.bhandarkar wrote:
> On 2012/04/04 20:16:57, zel wrote:
> > On 2012/04/04 19:14:11, achuith.bhandarkar wrote:
> > > pref_observer_ doesn't seem to do anything? What am I missing?
> > 
> > it does. it's a helper class for registering for monitoring changed in a
> > specific set of preferences
> 
> Ah, got it. It takes care of the NotificationRegistrar business, and forwards
> Observe notifications.

BTW, PrefChangeRegistar solves the same problem.

PrefSetObserver is implemented on top of PrefChangeRegistar, but it doesn't seem
to be adding values. I'd prefer PrefChangeRegistar in favor of less indirection.

[zel, 2012-04-04 22:14:18]

http://codereview.chromium.org/9947002/diff/1001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/9947002/diff/1001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2707: }
On 2012/04/04 21:23:16, satorux1 wrote:
> Please remove TODOs. My part won't need code in GDataFileSystem.
> 
> http://codereview.chromium.org/9968105/

Done.

http://codereview.chromium.org/9947002/diff/1001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:3820:
pref_observer_.reset(new PrefSetObserver(profile_->GetPrefs(), this));
On 2012/04/04 21:47:11, satorux1 wrote:
> On 2012/04/04 20:46:19, achuith.bhandarkar wrote:
> > On 2012/04/04 20:16:57, zel wrote:
> > > On 2012/04/04 19:14:11, achuith.bhandarkar wrote:
> > > > pref_observer_ doesn't seem to do anything? What am I missing?
> > > 
> > > it does. it's a helper class for registering for monitoring changed in a
> > > specific set of preferences
> > 
> > Ah, got it. It takes care of the NotificationRegistrar business, and
forwards
> > Observe notifications.
> 
> BTW, PrefChangeRegistar solves the same problem.
> 
> PrefSetObserver is implemented on top of PrefChangeRegistar, but it doesn't
seem
> to be adding values. I'd prefer PrefChangeRegistar in favor of less
indirection.

Done.

[satorux1, 2012-04-04 22:16:33]

LGTM