UMA for Windows 8 Secondary Tile pinning/unpinning user actions

win8/metro_driver/secondary_tile.cc

Index: win8/metro_driver/secondary_tile.cc
diff --git a/win8/metro_driver/secondary_tile.cc b/win8/metro_driver/secondary_tile.cc
index 97109c1924b5f79de574f1f0cdba58f3fef33e1f..c0e9010def0f9c5408855c36d2401173b738f6ac 100644
--- a/win8/metro_driver/secondary_tile.cc
+++ b/win8/metro_driver/secondary_tile.cc
@@ -16,7 +16,81 @@
 
 namespace {
 
-void DeleteTileFromStartScreen(const string16& tile_id) {
+// Callback for asynchronous pin requests.
+class TileRequestDone {
+ public:
+  enum PinType {
+    PIN,
+    UNPIN
+  };
+  TileRequestDone(mswr::ComPtr<winfoundtn::IAsyncOperation<bool>>& completion,
+                  PinType type,
+                  base::win::MetroPinResultCallback callback,
+                  void* callback_data)
+  : type_(type), callback_(callback), callback_data_(callback_data) {

[benwells, 2012/11/27 08:56:39]

Nit: indenting here is off. THe : should line up with the parameters to the
constructor.

[tapted, 2012/11/28 00:33:43]

Done.

+    typedef winfoundtn::IAsyncOperationCompletedHandler<bool> RequestDoneType;
+    mswr::ComPtr<RequestDoneType> handler(mswr::Callback<RequestDoneType>(

[benwells, 2012/11/27 08:56:39]

Can you move this logic into another function? It would be cleaner, remove any
question of weird problems, and would make the calling code more obvious.

It's also agains the google style guide to do 'work' in the constructor:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Doing_...

[tapted, 2012/11/28 00:33:43]

Done.

+        this, &TileRequestDone::respond));
+    DCHECK(handler.Get() != NULL);
+    HRESULT hr = completion->put_Completed(handler.Get());
+    CheckHR(hr, "Failed to put_Completed");
+  }
+
+ private:
+  HRESULT respond(winfoundtn::IAsyncOperation<bool>* async,
+                  AsyncStatus status);
+
+  PinType type_;
+  base::win::MetroPinResultCallback callback_;
+  void* callback_data_;
+};
+
+HRESULT TileRequestDone::respond(winfoundtn::IAsyncOperation<bool>* async,

[benwells, 2012/11/27 08:56:39]

Is there a reason to have the constructor inline and this function out of line?
Consistency would be good.

[tapted, 2012/11/28 00:33:43]

No particular reason. (It met the style guide :) -- 10 lines, no loops/switch
statements). I've moved it out. [Done.]

+                                 AsyncStatus status) {
+  PinType type = type_;
+  base::win::MetroPinResultCallback callback = callback_;
+  void* callback_data = callback_data_;
+  delete this;

[benwells, 2012/11/27 08:56:39]

Hmm. I assume there is some reason to copy the members, delete here and then
carry on, as it is a bit weird. Can you put a comment explaining why you're
doing this?

[tapted, 2012/11/28 00:33:43]

Just defensive programming - more from a mindset like ~"what if that callback
throws an exception". But while we're not making a generic callback API it's not
really an issue, so I've changed it to be less weird. [Done.]

+
+  base::win::MetroSecondaryTilePinState pin_state =
+      base::win::METRO_PIN_STATE_NONE;
+
+  if (status == Completed) {
+    unsigned char result;
+    CheckHR(async->GetResults(&result));
+    LOG(INFO) << __FUNCTION__ << " result " << static_cast<int>(result);
+    switch (result) {
+      case 0:
+        pin_state = type == PIN ?
+            base::win::METRO_PIN_RESULT_CANCEL :
+            base::win::METRO_UNPIN_RESULT_CANCEL;
+        break;
+      case 1:
+        pin_state = type == PIN ?
+            base::win::METRO_PIN_RESULT_OK :
+            base::win::METRO_UNPIN_RESULT_OK;
+        break;
+      default:
+        pin_state = type == PIN ?
+            base::win::METRO_PIN_RESULT_OTHER :
+            base::win::METRO_UNPIN_RESULT_OTHER;
+        break;
+    }
+  } else {
+    LOG(ERROR) << __FUNCTION__ << " Unexpected async status " << status;
+    pin_state = type == PIN ?
+        base::win::METRO_PIN_RESULT_ERROR :
+        base::win::METRO_UNPIN_RESULT_ERROR;
+  }
+  if (callback)
+    callback(callback_data, pin_state);
+
+  return S_OK;
+}
+
+void DeleteTileFromStartScreen(const string16& tile_id,
+                               base::win::MetroPinResultCallback callback,
+                               void* callback_data) {
   DVLOG(1) << __FUNCTION__;
   mswr::ComPtr<winui::StartScreen::ISecondaryTileFactory> tile_factory;
   HRESULT hr = winrt_utils::CreateActivationFactory(
@@ -35,18 +109,24 @@ void DeleteTileFromStartScreen(const string16& tile_id) {
   hr = tile->RequestDeleteAsync(completion.GetAddressOf());
   CheckHR(hr, "RequestDeleteAsync failed");
 
-  typedef winfoundtn::IAsyncOperationCompletedHandler<bool> RequestDoneType;
-  mswr::ComPtr<RequestDoneType> handler(mswr::Callback<RequestDoneType>(
-      globals.view, &ChromeAppView::TileRequestCreateDone));
-  DCHECK(handler.Get() != NULL);
-  hr = completion->put_Completed(handler.Get());
-  CheckHR(hr, "Failed to put_Completed");
+  if (FAILED(hr)) {
+    if (callback)
+      callback(callback_data, base::win::METRO_UNPIN_REQUEST_SHOW_ERROR);
+    return;
+  }
+
+  new TileRequestDone(completion,
+                      TileRequestDone::UNPIN,
+                      callback,
+                      callback_data);
 }
 
 void CreateTileOnStartScreen(const string16& tile_id,
                              const string16& title_str,
                              const string16& url_str,
-                             const FilePath& logo_path) {
+                             const FilePath& logo_path,
+                             base::win::MetroPinResultCallback callback,
+                             void* callback_data) {
   VLOG(1) << __FUNCTION__;
 
   mswr::ComPtr<winui::StartScreen::ISecondaryTileFactory> tile_factory;
@@ -99,12 +179,16 @@ void CreateTileOnStartScreen(const string16& tile_id,
   hr = tile->RequestCreateAsync(completion.GetAddressOf());
   CheckHR(hr, "RequestCreateAsync failed");
 
-  typedef winfoundtn::IAsyncOperationCompletedHandler<bool> RequestDoneType;
-  mswr::ComPtr<RequestDoneType> handler(mswr::Callback<RequestDoneType>(
-      globals.view, &ChromeAppView::TileRequestCreateDone));
-  DCHECK(handler.Get() != NULL);
-  hr = completion->put_Completed(handler.Get());
-  CheckHR(hr, "Failed to put_Completed");
+  if (FAILED(hr)) {
+    if (callback)
+      callback(callback_data, base::win::METRO_PIN_REQUEST_SHOW_ERROR);
+    return;
+  }
+
+  new TileRequestDone(completion,
+                      TileRequestDone::PIN,
+                      callback,
+                      callback_data);
 }
 
 }  // namespace
@@ -122,19 +206,35 @@ BOOL MetroIsPinnedToStartScreen(const string16& tile_id) {
   return exists;
 }
 
-void MetroUnPinFromStartScreen(const string16& tile_id) {
+void MetroUnPinFromStartScreen(const string16& tile_id,
+                               base::win::MetroPinResultCallback callback,
+                               void* callback_data) {
+  // Ensure function signature is correct.
+  base::win::MetroUnPinFromStartScreen check = &MetroUnPinFromStartScreen;

[benwells, 2012/11/27 08:56:39]

Um ... I haven't seen anything like this in chrome code. I can see what you are
doing, but maybe it would be better done in a unit test. This feels a little too
tricky.

[tapted, 2012/11/28 00:33:43]

In a unit test it would only check the prototype declaration. The actual
definition in the DLL could still be an override, resulting in GetProcAddress
failing at runtime. (Although if the unit test _called_ the function, the linker
should pick it up).

I actually hit this -- accidentally changed the signature IsPinned..(), and the
hotdog menu would no longer load. So it can be a subtle problem if someone
changes the typedef and misses one of the definitions.

But, you're right about nobody using the `(void)local_arg` technique in chrome
(I did a grep..). Maybe I just delete it [Done.].. but let me know if I should
think more.

+  (void)check;
+
   globals.appview_msg_loop->PostTask(
-      FROM_HERE, base::Bind(&DeleteTileFromStartScreen, tile_id));
+      FROM_HERE, base::Bind(&DeleteTileFromStartScreen,
+                            tile_id,
+                            callback,
+                            callback_data));
 }
 
 void MetroPinToStartScreen(const string16& tile_id,
                            const string16& title,
                            const string16& url,
-                           const FilePath& logo_path) {
+                           const FilePath& logo_path,
+                           base::win::MetroPinResultCallback callback,
+                           void* callback_data) {
+  base::win::MetroPinToStartScreen check = &MetroPinToStartScreen;
+  (void)check;
+
   globals.appview_msg_loop->PostTask(
     FROM_HERE, base::Bind(&CreateTileOnStartScreen,
                           tile_id,
                           title,
                           url,
-                          logo_path));
+                          logo_path,
+                          callback,
+                          callback_data));
 }