sync: Track validity of about:sync fields

sync: Track validity of about:sync fields

This commit changes the way we initialize the fields which get exported
to about:sync's about tab.  The goal was to pre-declare all the data
which would be exported in a complete snapshot, and to track which of
these fields were actually set.  Unset fields will have a default value
and be specially highlighted on about:sync.  This commit includes the
changes necessary to perform this highlighting.

Also in this commit are a few backend changes that are used to determine
from sync_ui_util which fields are properly initialized and which fields
have been unset by anything other than a default constructor.

The signature for ProfileSyncService::QueryDetailedSyncStatus() has been
modified to return an additional parameter indicating whether or not the
returned status actually came from the backend.

The SyncSesionSnapshot takes a different approach. The object is
immutable and has two constructors. If it is constructed with the
zero-argument constructor it is clearly uninitialized, and will remain
so. This change adds a flag which is set or unset based on which
constructor is used.

Finally, the ConstructAboutInforation function and all its helpers have
been moved out of sync_ui_util.cc and into their own file.

BUG=134927
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147531

[rlarocque, 2012-07-04 00:27:47]

Here's the follow-up from http://codereview.chromium.org/10694013/.  This diff
is big and experimental so I didn't want to append it to that review.  

The goal was to pre-declare the structure of the 'snapshot' which gets exported
to about:sync, so that we would know when a field was left unset.  I wanted to
reach that goal without violating the following constraints:
- Never specify the same string twice.
- No templates.
- No iterating over fields.
- Require as few references to each field as possible.

This is the result.  I had to create some small classes so I could reference the
fields I had declared earlier.  It's ugly, and the lifetime semantics don't make
a lot of sense, but it works fairly well.  

However, I'm not sure if this is better than the alternatives, which include:
- Pre-declaring most of the strings in HTML, rather than creating a massive
dictionary-like object in JSON then using jstemplate to dynamically convert it
into HTML.
- Not explicitly pre-declaring all fields (ie., the approach used in
http://codereview.chromium.org/10694013/)

[rlarocque, 2012-07-04 00:29:34]

By the way, this is still draft quality.  It's not ready full review, I just
wanted to hear your thoughts on the general direction of this patch.

[akalin, 2012-07-11 01:50:36]

sorry for late reply.

this doesn't seem too big.  I think it'll end up being cleaner than your first
version.  wdyt?

[rlarocque, 2012-07-11 17:32:36]

On 2012/07/11 01:50:36, akalin wrote:
> sorry for late reply.
> 
> this doesn't seem too big.  I think it'll end up being cleaner than your first
> version.  wdyt?

I don't like it, but it may be the least bad of all available options.  I'll
clean up the style violations, move some classes around, and upload a new diff
later today.

[rlarocque, 2012-07-11 23:01:58]

Please review.

I decided sync_ui_util::ConstructAboutInformation() and all its helpers were big
enough that they now deserve their own file separate from sync_ui_util.cc.  This
made the diff bigger, but the patch hasn't changed in any significant way.

[akalin, 2012-07-12 00:01:55]

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
File chrome/browser/sync/about_sync_util.cc (right):

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util.cc:18: // Creates a 'section' for discplay
on about:sync, consisting of a title
discplay -> display

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util.cc:20: ListValue* AddSection(ListValue*
parent_list, const std::string& title) {
scoped_ptr<>?

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util.cc:39: void SetValue(std::string value);
const ref?

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util.cc:40: void SetValue(string16 value);
const ref?

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util.cc:43: DictionaryValue* stat_;
specify that this is owned by |section|

(Value* const?)

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util.cc:146: DictionaryValue* strings = new
DictionaryValue(); // The value to return.
scoped_ptr?

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util.cc:252:
client_version.SetValue(GetVersionString());
move client_version line before !service check?

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util.cc:346:
strings->Set("actionable_error_detected",
SetString?

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util.cc:353: strings->Set("actionable_error",
actionable_error);
shouldn't this go in the if (actionable_error_detected) block?

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util.cc:369:
strings->Set("unrecoverable_error_detected",
SetString?

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
File chrome/browser/sync/about_sync_util.h (right):

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util.h:18: // Note that |service| may be NULL.
specify that caller owns returned object
(or maybe returned a scoped_ptr<>?)

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
File chrome/browser/sync/about_sync_util_unittest.cc (right):

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util_unittest.cc:13: using ::testing::NiceMock;
namespace sync_ui_util { namespace { } } ?

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util_unittest.cc:25: // Will be released when the
dictionary is destroyed
hunh?

[rlarocque, 2012-07-12 01:02:20]

Updated.  PTAL.

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
File chrome/browser/sync/about_sync_util.cc (right):

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util.cc:18: // Creates a 'section' for discplay
on about:sync, consisting of a title
On 2012/07/12 00:01:55, akalin wrote:
> discplay -> display

Done.

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util.cc:20: ListValue* AddSection(ListValue*
parent_list, const std::string& title) {
On 2012/07/12 00:01:55, akalin wrote:
> scoped_ptr<>?

That would be wrong in this case.  The pointer is owned by 'parent_list' by the
time it is returned.  I've added a comment explaining the unusual ownership
rules of this function.

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util.cc:39: void SetValue(std::string value);
On 2012/07/12 00:01:55, akalin wrote:
> const ref?

Done.

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util.cc:40: void SetValue(string16 value);
On 2012/07/12 00:01:55, akalin wrote:
> const ref?

Done.

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util.cc:43: DictionaryValue* stat_;
On 2012/07/12 00:01:55, akalin wrote:
> specify that this is owned by |section|
> 
> (Value* const?)

Done.

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util.cc:146: DictionaryValue* strings = new
DictionaryValue(); // The value to return.
On 2012/07/12 00:01:55, akalin wrote:
> scoped_ptr?

Done.  I also renamed it because 'strings' is a bad name in general, and mostly
inaccurate in this case.

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util.cc:252:
client_version.SetValue(GetVersionString());
On 2012/07/12 00:01:55, akalin wrote:
> move client_version line before !service check?

Done.

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util.cc:346:
strings->Set("actionable_error_detected",
On 2012/07/12 00:01:55, akalin wrote:
> SetString?

SetBoolean.

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util.cc:353: strings->Set("actionable_error",
actionable_error);
On 2012/07/12 00:01:55, akalin wrote:
> shouldn't this go in the if (actionable_error_detected) block?

That's debatable.  If !actionable_error_detected, then we won't display any of
these values.  The only effect of putting lines 353-358 into the if statement
would be to conditionally remove these values from the JSON dictionary.  

My love of static typing makes me cringe at the thought of having the members
appear or disappear based on the program's current state.  I'd prefer to always
have these members defined.

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util.cc:369:
strings->Set("unrecoverable_error_detected",
On 2012/07/12 00:01:55, akalin wrote:
> SetString?

I changed this to SetBoolean, too.

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
File chrome/browser/sync/about_sync_util.h (right):

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util.h:18: // Note that |service| may be NULL.
On 2012/07/12 00:01:55, akalin wrote:
> specify that caller owns returned object
> (or maybe returned a scoped_ptr<>?)

Done.

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
File chrome/browser/sync/about_sync_util_unittest.cc (right):

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util_unittest.cc:13: using ::testing::NiceMock;
On 2012/07/12 00:01:55, akalin wrote:
> namespace sync_ui_util { namespace { } } ?

Done, mostly.  I left the using declarations out of the namespace.

http://codereview.chromium.org/10696107/diff/24/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util_unittest.cc:25: // Will be released when the
dictionary is destroyed
On 2012/07/12 00:01:55, akalin wrote:
> hunh?

That was there when I copied this test out of sync_ui_util_unittest.cc.  I don't
know what it's referring to, either.

I'll remove it.

[akalin, 2012-07-12 22:51:19]

LGTM

http://codereview.chromium.org/10696107/diff/25/chrome/browser/sync/about_syn...
File chrome/browser/sync/about_sync_util.cc (right):

http://codereview.chromium.org/10696107/diff/25/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util.cc:155: ListValue* stats_list = new
ListValue(); // 'details': A list of sections.
two spaces before //

[rlarocque, 2012-07-13 18:41:55]

+csilv for webui OWNERS review.

[csilv, 2012-07-16 18:42:07]

On 2012/07/13 18:41:55, rlarocque wrote:
> +csilv for webui OWNERS review.

webui/sync_internals_ui.cc owners LGTM

[commit-bot: I haz the power, 2012-07-16 18:43:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/10696107/27

[commit-bot: I haz the power, 2012-07-16 19:08:21]

Try job failure for 10696107-27 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-16 19:59:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/10696107/27

[commit-bot: I haz the power, 2012-07-16 20:22:39]

Try job failure for 10696107-27 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-19 17:58:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/10696107/26003

[commit-bot: I haz the power, 2012-07-19 17:58:34]

Presubmit check for 10696107-26003 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/cocoa

Presubmit checks took 2.0s to calculate.

[rlarocque, 2012-07-19 18:04:36]

On 2012/07/19 17:58:34, I haz the power (commit-bot) wrote:
> Presubmit check for 10696107-26003 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit Messages **
> If this change requires manual test instructions to QA team, add
> TEST=[instructions].
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     chrome/browser/ui/cocoa
> 
> Presubmit checks took 2.0s to calculate.

I had already unchecked the commit box before this went through.  I just
remembered that I need to look at crbug.com/137720 before committing this.

[rlarocque, 2012-07-19 19:08:50]

+sail for chrome/browser/ui/cocoa OWNERS review.

[sail, 2012-07-19 19:13:40]

cocoa/* LGTM!

[commit-bot: I haz the power, 2012-07-19 19:35:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/10696107/31001

[commit-bot: I haz the power, 2012-07-19 21:23:51]

Change committed as 147531