[Notification] Make HTML5 Notification use ActionCenter on Windows 10, behind Flags.

[Notification] Make HTML5 Notification use ActionCenter on Windows 10, behind Flags.

Notes:
- This CL only considers HTML5 Notifications, not Rich Notifications in Chrome Extensions.
- Targetted for Windows 10 2016/07 update AKA Anniversary Update.
- Feature is behind flags: --enable-features=NativeNotifications
- Resurrecting deleted metro_driver code for this:
  https://src.chromium.org/svn/trunk/src/win8/metro_driver/

(Temporary) Test instructions:
- Server testing/notification/index.html on local server, e.g.:
cd testing/notification
python -m SimpleHTTPServer

- Run Chrome:
chrome.exe --enable-features=NativeNotifications --user-data-dir=C:\temp_delete_me_after

- Navigate to test page (http://localhost:8000). Message boxes are displayed for debugging.

BUG=516147

[huangs, 2016-06-02 20:31:58]

huangs@chromium.org changed reviewers:
+ chrisha@chromium.org, peter@chromium.org

[huangs, 2016-06-02 20:31:59]

Not ready for review yet, but want to keep you updated on progress. TODO:
- Add image.
- Implement Close() and GetDisplayed().
- Clean up code.

Question: I noticed that notification_platform_bridge.h is only in GYP, but not
in BUILD.gn.  Is this deliberate?

[chrisha, 2016-06-02 21:14:10]

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

https://codereview.chromium.org/2033093003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_win.cc:56: void
CheckHR(HRESULT hr, const char* message = nullptr) {
Worth documenting that this is non-returning and causes a crash in the case of a
failing hr?

https://codereview.chromium.org/2033093003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_win.cc:192: hr =
CreateTextNode(toast_xml.Get(), 0, notification.title());
Where does the constant '0' come from?

https://codereview.chromium.org/2033093003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_win.cc:195: hr =
CreateTextNode(toast_xml.Get(), 1, notification.message());
Ditto '1'? Maybe make these constants describing what they are? Something like?

TOAST_TITLE_NODE_INDEX
TOAST_MESSAGE_NODE_INDEX

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

https://codereview.chromium.org/2033093003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_win.h:38: HRESULT
OnActivate(winui::Notifications::IToastNotification* notification,
No comment for this function?

https://codereview.chromium.org/2033093003/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/2033093003/diff/1/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:3925: 'runtimeobject.lib',
Is this safe for all versions of Windows that we still support? Will it
introduce a dependency on a DLL that doesn't exist on all platforms? etc

[huangs, 2016-06-06 04:51:57]

Updates:
- I'm trying to replace ToastTemplateType_ToastText02 with
ToastTemplateType_ToastTexAndImaget02 but something breaks; have to figure out.
- Per email thread, have to figure out how to pass images.  Awaiting feedback
re. passing data://base64 passing, but meanwhile I'll look into temp-file
method.

[Peter Beverloo, 2016-06-07 15:48:29]

This is really exciting to see, thanks :) I didn't review your CL in great
detail, but I was curious enough to want to understand where you are, hope you
don't mind!

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

https://codereview.chromium.org/2033093003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_win.cc:56: void
CheckHR(HRESULT hr, const char* message = nullptr) {
I don't know if this is common for Windows code, but because we use DFATAL it
will crash in debug, but only ERROR in normal mode.

That means that release builds would continue under the assumption the previous
operation was successful, even if it wasn't. Seems dangerous?

https://codereview.chromium.org/2033093003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_win.cc:71: HSTRING
MakeHString(const base::string16& str) {
Dito here, although as I understand it a NULL HSTRING is equal to the empty
string, so that'd be less impactful. This should only happen in case of OOM,
right?

https://codereview.chromium.org/2033093003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_win.cc:104: if (index
> count) {
It sounds like the index to IXmlNode::Item() is zero-based, in which case we
need ">=" here.

(Is there public documentation for these classes somewhere?)

https://codereview.chromium.org/2033093003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_win.cc:138: hr =
GetTextNodeRoot(index, xml_doc, &text_node_root);
nit: CreateTextNode takes |index| as an (int), but GetTextNodeRoot takes |index|
as an (unsigned int), so this is an implicit conversion. Could we use the same
signedness in both places?

https://codereview.chromium.org/2033093003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_win.cc:178:
winui::Notifications::ToastTemplateType_ToastText02,
I agree with starting with ToastText02, but could you perhaps share some ideas
on where we'd like to go in the long term? Given that images are optional, we
may have buttons and other rich features, and displaying the origin on the toast
is important (for which ToastText04 may be more appropriate), it's not a
straightforward matrix at all :/.

https://codereview.chromium.org/2033093003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_win.cc:266: //
TODO(huangs): Implement.
Do you expect any issues in supporting these two TODOs? (I.e. should we change
the NPB API?)

[Peter Beverloo, 2016-06-07 15:50:59]

Also, feel free to iterate on this in-tree if you prefer, that'll allow the
patches to become much smaller and gives people a chance to try it out as you
progress :-).

[huangs, 2016-06-07 21:59:53]

Quick update (will pore through comments after):
- The Windows-specific code is mostly copied from the now-deleted Metro Driver;
I didn't come up with it. :)
https://src.chromium.org/svn/trunk/src/win8/metro_driver/

- The main problem I'm dealing now is figuring out why replacing
  ToastTemplateType_ToastText02
with
  ToastTemplateType_ToastImageAndText02
does not work.  Once that's figured out I'll address comments and upload new
version.

[huangs, 2016-06-07 23:27:47]

Figured out images: Turns out if <image> tag has src="" then weird things happen
to text (but it points to nonexisting image then that's fine; image will just be
empty).  Uploaded for progress.  Refactoring will come next.

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

https://codereview.chromium.org/2033093003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_win.cc:56: void
CheckHR(HRESULT hr, const char* message = nullptr) {
On 2016/06/02 21:14:10, chrisha (slow) wrote:
> Worth documenting that this is non-returning and causes a crash in the case of
a
> failing hr?

Done.

https://codereview.chromium.org/2033093003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_win.cc:56: void
CheckHR(HRESULT hr, const char* message = nullptr) {
From sample code, looks the style should be a chain of
hr = ...
if (SUCCEEDED(hr)) {
  hr = ...
  if (SUCCEEDED(hr)) {
    ...
  }
}

Will refactor this to use a helper class that also wraps the verbose COM usage. 
Getting functionality to work first though.

[huangs, 2016-06-08 22:12:14]

Updated snapshot and addressed *some* comments.  The remaining I'll still need
to deal with.  Not ready for real review.  Goal for the next update:
- ">" to ">=" change.
- Experiment with saving image to temp file, displaying it, then deleting file
after delay (1 min), or process exit?
- Pick template differently depending on whether image is present.

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

https://codereview.chromium.org/2033093003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_win.cc:71: HSTRING
MakeHString(const base::string16& str) {
Done. Made the entire thing defensive in Patch #3; so on failure we'd just skip.

https://codereview.chromium.org/2033093003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_win.cc:138: hr =
GetTextNodeRoot(index, xml_doc, &text_node_root);
Refactoring removed this difference.  Now using unsigned int.

https://codereview.chromium.org/2033093003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_win.cc:178:
winui::Notifications::ToastTemplateType_ToastText02,
Refactored code to shuffle ugliness in helper class (in local namespace {} , but
we can shuffle it around).  Now the high-level logic should be clearer than
before.

https://codereview.chromium.org/2033093003/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_platform_bridge_win.cc:192: hr =
CreateTextNode(toast_xml.Get(), 0, notification.title());
First <text> element in the template, which looks like:

<toast>
  <visual>
    <binding template="ToastImageAndText02">
      <image id="1" src=""/>
      <text id="1"></text>
      <text id="2"></text>
    </binding>
  </visual>
</toast>

Code is still in flux; will consider using constants later.

[huangs, 2016-06-14 01:12:35]

Update:
- Can display custom images now! Looks a bit off though (tiny and not cropped).
- Added NotificationToastSession, indexed by notification_id and tracked in
|session_map_|.
- Did temp file management, with proper threading.  Turns out file extension is
important (used ".png").
- Callback: The "Action" callback is invoked only after user clicks on
notification (not when it's shown).
- Doesn't seem we can bind object with callback.  Ended up embedding session id
into XML, and we have data back.
- Couple of things are not robust:
  1. Have to rename temp file to .png, and there's a small chance of collision /
failure.
    - Maybe augment the Chrome's random temp file API so we can set file
extension?
  2. Toast may be clicked after program exits, and that's fine; but what if
callback happens as Chrome is exiting, and NotificationPlatformBridgeWin got
deallocated??
    - See if we can cancel notifications?

TODO:
- Experiment: See how early can we delete image files.
- Delete temp files in NotificationPlatformBridgeWin destructor, so we won't
have dangling temp files.
- Handle Close() and GetDisplayed().
- Look into alternative to "file://"; may be use ms-appx:// ?? This will involve
Win10 API though.
- See if executable is compatible with older versions of Windows.

https://chromiumcodereview.appspot.com/2033093003/diff/1/chrome/browser/notif...
File chrome/browser/notifications/notification_platform_bridge_win.cc (right):

https://chromiumcodereview.appspot.com/2033093003/diff/1/chrome/browser/notif...
chrome/browser/notifications/notification_platform_bridge_win.cc:104: if (index
> count) {
On 2016/06/07 15:48:29, Peter Beverloo - OOO June 8-16 wrote:
> It sounds like the index to IXmlNode::Item() is zero-based, in which case we
> need ">=" here.
> 
> (Is there public documentation for these classes somewhere?)

Done.

https://chromiumcodereview.appspot.com/2033093003/diff/1/chrome/browser/notif...
File chrome/browser/notifications/notification_platform_bridge_win.h (right):

https://chromiumcodereview.appspot.com/2033093003/diff/1/chrome/browser/notif...
chrome/browser/notifications/notification_platform_bridge_win.h:38: HRESULT
OnActivate(winui::Notifications::IToastNotification* notification,
Added.  This is callback when notification gets clicked.  Haven't found one when
notification is shown.

[huangs, 2016-06-21 22:33:59]

We can clicking/closing events of the toast can propagate to JS! Caveats:
- For Windows 10 toast, clicking also closes...
- ...but, there are inactive areas on the toast (e.g., left of "Settings..."
button for Patch Set 7 prototype.

More notes:
- We're using our own custom XML template now.

TODO:
- Resize icon to 32 x 32.
- Get "Settings..." link to work.
- Clicking on area left of "Settings..." does not close toast; see if we can do
something??
- Handle Close() and GetDisplayed().
- Maybe sessions should be keyed by profile ID?
- Incognito?
- Maybe use temp directory to store temp images?

[huangs, 2016-10-27 18:23:06]

Description was changed from

==========
[Notification] Make HTML5 Notification use ActionCenter on Windows 10, behind
Flags.

Notes:
- This CL only considers HTML5 Notifications, not Rich Notifications in
  Chrome Extensions.
- Feature is behind flags: --enable-native-notifications
- Resurrecting deleted metro_driver code for this:
  https://src.chromium.org/svn/trunk/src/win8/metro_driver/

BUG=516147
==========

to

==========
[Notification] Make HTML5 Notification use ActionCenter on Windows 10, behind
Flags.

Notes:
- This CL only considers HTML5 Notifications, not Rich Notifications in Chrome
Extensions.
- Targetted for Windows 10 2016/07 update AKA Anniversary Update.
- Feature is behind flags: --enable-features=NativeNotifications
- Resurrecting deleted metro_driver code for this:
  https://src.chromium.org/svn/trunk/src/win8/metro_driver/

BUG=516147
==========

[huangs, 2017-04-03 17:54:53]

Description was changed from

==========
[Notification] Make HTML5 Notification use ActionCenter on Windows 10, behind
Flags.

Notes:
- This CL only considers HTML5 Notifications, not Rich Notifications in Chrome
Extensions.
- Targetted for Windows 10 2016/07 update AKA Anniversary Update.
- Feature is behind flags: --enable-features=NativeNotifications
- Resurrecting deleted metro_driver code for this:
  https://src.chromium.org/svn/trunk/src/win8/metro_driver/

BUG=516147
==========

to

==========
[Notification] Make HTML5 Notification use ActionCenter on Windows 10, behind
Flags.

Notes:
- This CL only considers HTML5 Notifications, not Rich Notifications in Chrome
Extensions.
- Targetted for Windows 10 2016/07 update AKA Anniversary Update.
- Feature is behind flags: --enable-features=NativeNotifications
- Resurrecting deleted metro_driver code for this:
  https://src.chromium.org/svn/trunk/src/win8/metro_driver/

(Temporary) Test instructions:
- Server testing/notification/index.html on local server, e.g.:
cd testing/notification
python -m SimpleHTTPServer

- Run Chrome:
chrome.exe --enable-features=NativeNotifications
--user-data-dir=C:\temp_delete_me_after

- Navigate to test page (http://localhost:8000). Message boxes are displayed for
debugging.

BUG=516147
==========

[huangs, 2017-04-03 18:11:14]

Status:
- We can handle images: Have code to fetch, resize, copy-to-temp, and cleanup.
- Main challenge now: Respond to events, without making Chrome UWP.
  - Info suggested by robliao@:
https://blogs.msdn.microsoft.com/tiles_and_toasts/2015/10/16/quickstart-handl...
  - http://crbug.com/516147#c57 suggests we look at how Opera does this:
https://github.com/operasoftware/desktop/tree/master/lgpl/sources