Adding parameter to GetStatusLabels to indicate if links are acceptable.

Adding parameter to GetStatusLabels to indicate if links are acceptable, as some platforms cannot handle links in all the places these messages are shown.

Also adding unit tests for other functionality of GetStatusLabels.

BUG=none, discussed beforehand with akalin
TEST=HtmlNotIncludedInStatusIfNotRequested


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

[jimblackler, 2011-10-25 11:14:54]

Richard, please re-review this and if possible run the presubmit checks. I will
send it to Fred after that.

[stuartmorgan, 2011-10-27 11:51:20]

Quick style drive-by while you are waiting for the real review:

http://codereview.chromium.org/8383036/diff/2001/chrome/browser/sync/sync_ui_...
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/8383036/diff/2001/chrome/browser/sync/sync_ui_...
chrome/browser/sync/sync_ui_util.cc:142: if (html_links)
These are multi-line, so need to have braces.

http://codereview.chromium.org/8383036/diff/2001/chrome/browser/sync/sync_ui_...
File chrome/browser/sync/sync_ui_util_unittest.cc (right):

http://codereview.chromium.org/8383036/diff/2001/chrome/browser/sync/sync_ui_...
chrome/browser/sync/sync_ui_util_unittest.cc:297: }
Chromium style discourages braces for one-liners (here and below)

[jimblackler, 2011-10-27 14:53:52]

Tx. Also fixed a build warning.

http://codereview.chromium.org/8383036/diff/2001/chrome/browser/sync/sync_ui_...
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/8383036/diff/2001/chrome/browser/sync/sync_ui_...
chrome/browser/sync/sync_ui_util.cc:142: if (html_links)
On 2011/10/27 11:51:20, stuartmorgan wrote:
> These are multi-line, so need to have braces.

Done.

http://codereview.chromium.org/8383036/diff/2001/chrome/browser/sync/sync_ui_...
File chrome/browser/sync/sync_ui_util_unittest.cc (right):

http://codereview.chromium.org/8383036/diff/2001/chrome/browser/sync/sync_ui_...
chrome/browser/sync/sync_ui_util_unittest.cc:297: }
On 2011/10/27 11:51:20, stuartmorgan wrote:
> Chromium style discourages braces for one-liners (here and below)

Done.

[akalin, 2011-10-28 19:10:22]

Some comments.

Sorry for the long turnaround, I missed your e-mail.  I think bumping the code
review issue is a more reliable ping, as it pushes it to the top of my list in
Reitveld and I scan for e-mails from codereview.

http://codereview.chromium.org/8383036/diff/10013/chrome/browser/sync/sync_ui...
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/8383036/diff/10013/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.cc:135: bool html_links) {
indent with (

http://codereview.chromium.org/8383036/diff/10013/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.cc:352: html_links);
indent with (

http://codereview.chromium.org/8383036/diff/10013/chrome/browser/sync/sync_ui...
File chrome/browser/sync/sync_ui_util.h (right):

http://codereview.chromium.org/8383036/diff/10013/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.h:44: bool html_links);
Chromium style is to avoid booleans in favor of enums, and I think it'll help
readability in this case.

How about:

enum StatusLabelStyle { PLAIN_TEXT, WITH_HTML };

Also, input args go before mixed/output args, so:

MessageType GetStatusLabels(StatusLabelStyle ...,
                            ProfileSyncService* ...,
                            string16* ...);

http://codereview.chromium.org/8383036/diff/10013/chrome/browser/sync/sync_ui...
File chrome/browser/sync/sync_ui_util_unittest.cc (right):

http://codereview.chromium.org/8383036/diff/10013/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util_unittest.cc:22: STATUS_CASE__SETUP_IN_PROGRESS,
why two underscores?  don't care much either way, but the usual style is just
one

http://codereview.chromium.org/8383036/diff/10013/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/personal_options_handler.cc (right):

http://codereview.chromium.org/8383036/diff/10013/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/personal_options_handler.cc:250: service,
&status_label, &link_label, true) == sync_ui_util::SYNC_ERROR;
what about "true /* has_links */"

[jimblackler, 2011-11-01 16:15:30]

http://codereview.chromium.org/8383036/diff/10013/chrome/browser/sync/sync_ui...
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/8383036/diff/10013/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.cc:135: bool html_links) {
On 2011/10/28 19:10:22, akalin wrote:
> indent with (

Done.

http://codereview.chromium.org/8383036/diff/10013/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.cc:135: bool html_links) {
On 2011/10/28 19:10:22, akalin wrote:
> indent with (

Done.

http://codereview.chromium.org/8383036/diff/10013/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.cc:352: html_links);
On 2011/10/28 19:10:22, akalin wrote:
> indent with (

Done.

http://codereview.chromium.org/8383036/diff/10013/chrome/browser/sync/sync_ui...
File chrome/browser/sync/sync_ui_util.h (right):

http://codereview.chromium.org/8383036/diff/10013/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.h:44: bool html_links);
On 2011/10/28 19:10:22, akalin wrote:
> Chromium style is to avoid booleans in favor of enums, and I think it'll help
> readability in this case.
> 
> How about:
> 
> enum StatusLabelStyle { PLAIN_TEXT, WITH_HTML };
> 
> Also, input args go before mixed/output args, so:
> 
> MessageType GetStatusLabels(StatusLabelStyle ...,
>                             ProfileSyncService* ...,
>                             string16* ...);

Done.

http://codereview.chromium.org/8383036/diff/10013/chrome/browser/sync/sync_ui...
File chrome/browser/sync/sync_ui_util_unittest.cc (right):

http://codereview.chromium.org/8383036/diff/10013/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util_unittest.cc:22: STATUS_CASE__SETUP_IN_PROGRESS,
On 2011/10/28 19:10:22, akalin wrote:
> why two underscores?  don't care much either way, but the usual style is just
> one

Done.

http://codereview.chromium.org/8383036/diff/10013/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/personal_options_handler.cc (right):

http://codereview.chromium.org/8383036/diff/10013/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/personal_options_handler.cc:250: service,
&status_label, &link_label, true) == sync_ui_util::SYNC_ERROR;
Done.

[akalin, 2011-11-02 23:36:28]

LGTM

http://codereview.chromium.org/8383036/diff/18002/chrome/browser/sync/sync_ui...
File chrome/browser/sync/sync_ui_util.h (right):

http://codereview.chromium.org/8383036/diff/18002/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.h:45: // If |html_links| is true,
|status_label| may contain an HTML-formatted link.
update comment

[jimblackler, 2011-11-04 13:11:49]

http://codereview.chromium.org/8383036/diff/18002/chrome/browser/sync/sync_ui...
File chrome/browser/sync/sync_ui_util.h (right):

http://codereview.chromium.org/8383036/diff/18002/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.h:45: // If |html_links| is true,
|status_label| may contain an HTML-formatted link.
On 2011/11/02 23:36:28, akalin wrote:
> update comment

Done.

[commit-bot: I haz the power, 2011-11-04 13:12:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jimblackler@google.com/8383036/19001

[commit-bot: I haz the power, 2011-11-04 13:38:19]

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

[commit-bot: I haz the power, 2011-11-04 13:47:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jimblackler@google.com/8383036/23002

[commit-bot: I haz the power, 2011-11-04 14:21:56]

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

[commit-bot: I haz the power, 2011-11-04 16:03:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jimblackler@google.com/8383036/27001

[commit-bot: I haz the power, 2011-11-04 17:03:16]

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

[commit-bot: I haz the power, 2011-11-04 17:55:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jimblackler@google.com/8383036/27003

[commit-bot: I haz the power, 2011-11-04 19:03:31]

Try job failure for 8383036-27003 (retry) on linux_rel for step "unit_tests".
It's a second try, previously, steps "unit_tests, browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-11-15 13:06:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jimblackler@google.com/8383036/27003

[commit-bot: I haz the power, 2011-11-15 14:19:26]

Try job failure for 8383036-27003 (retry) (retry) (retry) on linux_rel for step
"unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-11-18 15:15:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jimblackler@google.com/8383036/33001

[commit-bot: I haz the power, 2011-11-18 15:59:49]

Try job failure for 8383036-33001 (retry) on linux_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-11-18 18:25:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jimblackler@google.com/8383036/37001

[commit-bot: I haz the power, 2011-11-18 20:23:08]

Try job failure for 8383036-37001 (retry) on win_rel for step "update".
It's a second try, previously, step "ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2011-11-21 10:18:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jimblackler@google.com/8383036/37001

[commit-bot: I haz the power, 2011-11-21 13:12:06]

Change committed as 110905