Add a NotificationTemplateBuilder for Action Center toasts

Add a NotificationTemplateBuilder for Action Center toasts

This adds a notification template builder that takes a notification_id
and a message_center::Notification object and builds the XML template
that can be displayed using the Windows Action Center.

It's functionality is incredibly limited -- the title, message and
origin attribution of a notification are included, everything else will
have to be added in the future.

BUG=734095
Review-Url: https://codereview.chromium.org/2920303002
Cr-Commit-Position: refs/heads/master@{#480097}
Committed: https://chromium.googlesource.com/chromium/src/+/5904da760ad0345a34a986601f6ae26fb78a4a2e

[Peter Beverloo, 2017-06-05 17:06:24]

peter@chromium.org changed reviewers:
+ dominicc@chromium.org, finnur@chromium.org

[Peter Beverloo, 2017-06-05 17:06:26]

+finnur for Windows notifications
+dominicc for libxml usage

I'll update the BUILD.gn files once you're happy - this just allows me to build
this part on Linux :).

https://codereview.chromium.org/2920303002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_template_builder.cc (right):

https://codereview.chromium.org/2920303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_template_builder.cc:60:
builder->EndToastElement();
An alternative would be to use comments:

builder->EndElement();  // </toast>

https://codereview.chromium.org/2920303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_template_builder.cc:79: return
template_xml.substr(sizeof(kXmlVersionHeader) - 1);
:(

https://codereview.chromium.org/2920303002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_template_builder.h (right):

https://codereview.chromium.org/2920303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_template_builder.h:40: 
Should this class be responsible for extracting the appropriate images too?
(Which we need the paths for.) In that case building the notification will have
to be asynchronous, as writing temporary files must happen on a different
thread.

https://codereview.chromium.org/2920303002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_template_builder_unittest.cc
(right):

https://codereview.chromium.org/2920303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_template_builder_unittest.cc:148:
TEST_F(NotificationTemplateBuilderTest, SimpleToastXml) {
I'm inclined to prefer this method of testing. It means that we'll have many
more string constants in the test suite, and it's also a little bit more fragile
(e.g. flipping around arguments means updating all expectations), but it's also
much clearer to see what's going on. The Action Center format won't frequently
change anyway.

SimpleToast does pretty much the same but in a DOM-walky way. What do you prefer
Finnur?

[Peter Beverloo, 2017-06-05 17:06:36]

The CQ bit was checked by peter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-05 17:06:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-05 18:51:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-05 18:51:04]

Dry run: This issue passed the CQ dry run.

[Finnur, 2017-06-06 11:00:10]

Yesterday was a holiday (Whit Monday). 

CL LGTM, with minor nits.

https://codereview.chromium.org/2920303002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_template_builder.cc (right):

https://codereview.chromium.org/2920303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_template_builder.cc:60:
builder->EndToastElement();
EndToastElement(), you mean?

I don't know... Seems simple enough without it.

https://codereview.chromium.org/2920303002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_template_builder.h (right):

https://codereview.chromium.org/2920303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_template_builder.h:40: 
Hmm... Not sure.

https://codereview.chromium.org/2920303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_template_builder.h:50: // Also closes
the |xml_writer_| for writing as the toast is now compelte.
s/compelte/complete/

https://codereview.chromium.org/2920303002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_template_builder_unittest.cc
(right):

https://codereview.chromium.org/2920303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_template_builder_unittest.cc:148:
TEST_F(NotificationTemplateBuilderTest, SimpleToastXml) {
Yeah, I think I agree with you...

[dominicc (has gone to gerrit), 2017-06-12 03:40:33]

LGTM

At some point someone will come to change or remove XmlReader and/or XmlWriter,
but for now this is fine.

XmlReader is fine in a test; for future reference don't use XmlReader in the
browser process.

Per our email thread, Nick and Daniel did not reply with objections and you're
converting the content to UTF8 here, so using XmlWriter is probably OK or at
least not known to be bad.

I recommend you add a libfuzzer test for the NotificationBuilder.

[Peter Beverloo, 2017-06-16 15:40:21]

Thanks both! I've applied the changes, made this Windows-only in BUILD.gn, so
let's land it.

On 2017/06/12 03:40:33, dominicc (has gone to gerrit) wrote:
> At some point someone will come to change or remove XmlReader and/or
XmlWriter,
> but for now this is fine.
> 
> XmlReader is fine in a test; for future reference don't use XmlReader in the
> browser process.

Good to know. We've now simplified the tests to just verify the generated XML
anyway - Microsoft's format is stable, so this should be sufficient until more
complicated cases come up.

> I recommend you add a libfuzzer test for the NotificationBuilder.

Good idea! I'll follow up.

https://codereview.chromium.org/2920303002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_template_builder.cc (right):

https://codereview.chromium.org/2920303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_template_builder.cc:60:
builder->EndToastElement();
On 2017/06/06 11:00:09, Finnur wrote:
> EndToastElement(), you mean?
> 
> I don't know... Seems simple enough without it. 

OK, we'll leave as is, this is super simple to change later on when we have a
better idea of how the builder is going to work for more complex layouts.

https://codereview.chromium.org/2920303002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_template_builder.h (right):

https://codereview.chromium.org/2920303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_template_builder.h:50: // Also closes
the |xml_writer_| for writing as the toast is now compelte.
On 2017/06/06 11:00:09, Finnur wrote:
> s/compelte/complete/

Done.

https://codereview.chromium.org/2920303002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_template_builder_unittest.cc
(right):

https://codereview.chromium.org/2920303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_template_builder_unittest.cc:148:
TEST_F(NotificationTemplateBuilderTest, SimpleToastXml) {
On 2017/06/06 11:00:09, Finnur wrote:
> Yeah, I think I agree with you...

Ok, done.

[Peter Beverloo, 2017-06-16 15:49:15]

Description was changed from

==========
Add a NotificationTemplateBuilder for Action Center toasts

This adds a notification template builder that takes a notification_id
and a message_center::Notification object and builds the XML template
that can be displayed using the Windows Action Center.

It's functionality is incredibly limited -- the title, message and
origin attribution of a notification are included, everything else will
have to be added in the future.

BUG=
==========

to

==========
Add a NotificationTemplateBuilder for Action Center toasts

This adds a notification template builder that takes a notification_id
and a message_center::Notification object and builds the XML template
that can be displayed using the Windows Action Center.

It's functionality is incredibly limited -- the title, message and
origin attribution of a notification are included, everything else will
have to be added in the future.

BUG=734095
==========

[Peter Beverloo, 2017-06-16 16:00:20]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2017-06-16 16:00:20]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org,
dominicc@chromium.org
Link to the patchset: https://codereview.chromium.org/2920303002/#ps40001
(title: "remove libxml usage from the test")

[commit-bot: I haz the power, 2017-06-16 16:00:37]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-16 17:57:55]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497628820463190,
"parent_rev": "20065cfbc168c8a6d919196499c7ac484351c5d8", "commit_rev":
"5904da760ad0345a34a986601f6ae26fb78a4a2e"}

[commit-bot: I haz the power, 2017-06-16 17:58:07]

Description was changed from

==========
Add a NotificationTemplateBuilder for Action Center toasts

This adds a notification template builder that takes a notification_id
and a message_center::Notification object and builds the XML template
that can be displayed using the Windows Action Center.

It's functionality is incredibly limited -- the title, message and
origin attribution of a notification are included, everything else will
have to be added in the future.

BUG=734095
==========

to

==========
Add a NotificationTemplateBuilder for Action Center toasts

This adds a notification template builder that takes a notification_id
and a message_center::Notification object and builds the XML template
that can be displayed using the Windows Action Center.

It's functionality is incredibly limited -- the title, message and
origin attribution of a notification are included, everything else will
have to be added in the future.

BUG=734095

Review-Url: https://codereview.chromium.org/2920303002
Cr-Commit-Position: refs/heads/master@{#480097}
Committed:
https://chromium.googlesource.com/chromium/src/+/5904da760ad0345a34a986601f6a...
==========

[commit-bot: I haz the power, 2017-06-16 17:58:08]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/5904da760ad0345a34a986601f6a...