Adapting the UI to bring it closer to the spec, and fixing image fetching.

Adapting the UI to bring it closer to the spec, and fixing image fetching.

Firstly, this uses more correct fields for displaying synced notifications.
The Annotation field is better than the description or text fields in most
cases.

Secondly, this fixes some image fetch problems.  I was using images from the
expanded_notification section instead of the collapsed_notification section,
and the schemaless URLs were being interpreted by the chrome code as UNC paths!
I added a function to add a default schema of https: if none was provided to
prevent the GURL code from thinking "//" meant a UNC path on windows.

Thirdly, I noticed some changes to the server side protobuf that we were not in
sync with, so I copied the new fields into the protobuf that we use.  (This
includes the annotation field).

BUG=262303, 247668
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213288

[Pete Williamson, 2013-07-19 22:10:09]

zea: Please review the protobuf changes
dewittj: Please review everything else

[dewittj, 2013-07-19 22:37:18]

https://codereview.chromium.org/19771013/diff/1/chrome/browser/notifications/...
File chrome/browser/notifications/sync_notifier/synced_notification.cc (right):

https://codereview.chromium.org/19771013/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/sync_notifier/synced_notification.cc:37:
std::string AddDefaultSchemaIfNeeded(std::string& url_spec) {
This function makes me sad. Is there anything else we can do within GURL?  like
has_scheme? Or if that doesn't work, just disallow file:// images in
notifications entirely and do a match using SchemeIsFile or SchemeIsFileSystem?

Worst case I'd prefer you to use the string_util StartsWithASCII function
instead of this hand-rolled one.

[Pete Williamson, 2013-07-19 22:49:07]

Answer for DewittJ

https://codereview.chromium.org/19771013/diff/1/chrome/browser/notifications/...
File chrome/browser/notifications/sync_notifier/synced_notification.cc (right):

https://codereview.chromium.org/19771013/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/sync_notifier/synced_notification.cc:37:
std::string AddDefaultSchemaIfNeeded(std::string& url_spec) {
On 2013/07/19 22:37:19, dewittj wrote:
> This function makes me sad. Is there anything else we can do within GURL? 
like
> has_scheme? Or if that doesn't work, just disallow file:// images in
> notifications entirely and do a match using SchemeIsFile or
SchemeIsFileSystem?
> 
> Worst case I'd prefer you to use the string_util StartsWithASCII function
> instead of this hand-rolled one.

Yes, I share your sadness for this function.  My first plan is to get the server
side to send URLs with a full scheme instead of schemeless.

I could definitely convert this into a GURL, then use GURL functions to find out
if it has file scheme.  The File scheme is being added by the GURL constructor
itself, so if I did that, I would have to parse the URL once to see that it had
a file: scheme, and then I would fixup the URL, and later parse it again.  I
think that URL parsing is a bit expensive, which is why I opted for a cheaper
fix of just checking the first two characters. 

I did look for some functions that could easily tell me this was a schemeless
URL, but I did not find anything, and I've very open if any of the reviewers
have one to suggest.  If not, I plan to go with StartsWithAscii as you suggest.

[Pete Williamson, 2013-07-19 23:16:06]

Fixes per DewittJ@

[dewittj, 2013-07-22 14:48:16]

https://codereview.chromium.org/19771013/diff/6001/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/synced_notification.cc (right):

https://codereview.chromium.org/19771013/diff/6001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/synced_notification.cc:44: return
url_spec;
On other platforms, how is a url starting with 2 slashes interpreted?

[Pete Williamson, 2013-07-22 22:25:29]

https://codereview.chromium.org/19771013/diff/6001/chrome/browser/notificatio...
File chrome/browser/notifications/sync_notifier/synced_notification.cc (right):

https://codereview.chromium.org/19771013/diff/6001/chrome/browser/notificatio...
chrome/browser/notifications/sync_notifier/synced_notification.cc:44: return
url_spec;
On 2013/07/22 14:48:16, dewittj wrote:
> On other platforms, how is a url starting with 2 slashes interpreted?

It is interpreted as a scheme-less URL. (which is how it was intended here in
the server protobuf).  The special casing for the 2 slashes is all in ifdef
OS_WINDOWS or ifdef WIN232 guards.

[dewittj, 2013-07-22 23:24:01]

lgtm but I don't have enough context to determine if the actual data you're
pulling out is correct, I defer to someone with better knowledge of the
protobuf.

[dewittj, 2013-07-22 23:29:36]

I would also prefer to set any urls to https that have no scheme.  Perhaps your
helper function can return a GURL instead of a replaced string?

[Pete Williamson, 2013-07-23 18:31:11]

ping Zea@ - Please look at the protobuf changes

Also, I thought I had run my unit tests, but I forgot one.  Fixed the failures
that this change caused in the unit test.

[Nicolas Zea, 2013-07-23 20:23:31]

sync LGTM

[commit-bot: I haz the power, 2013-07-23 20:26:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/petewil@chromium.org/19771013/16001

[commit-bot: I haz the power, 2013-07-23 20:33:59]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2013-07-23 22:00:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/petewil@chromium.org/19771013/16001

[commit-bot: I haz the power, 2013-07-24 00:15:48]

Change committed as 213288