UMA for Windows 8 Secondary Tile pinning/unpinning user actions

win8/metro_driver/secondary_tile.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "stdafx.h"
 #include "secondary_tile.h"
 
 #include <windows.ui.startscreen.h>
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/utf_string_conversions.h"
 #include "googleurl/src/gurl.h"
 #include "win8/metro_driver/chrome_app_view.h"
 #include "win8/metro_driver/winrt_utils.h"
 
 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(
       RuntimeClass_Windows_UI_StartScreen_SecondaryTile,
       tile_factory.GetAddressOf());
   CheckHR(hr, "Failed to create instance of ISecondaryTileFactory");
 
   mswrw::HString id;
   id.Attach(MakeHString(tile_id));
 
   mswr::ComPtr<winui::StartScreen::ISecondaryTile> tile;
   hr = tile_factory->CreateWithId(id.Get(), tile.GetAddressOf());
   CheckHR(hr, "Failed to create tile");
 
   mswr::ComPtr<winfoundtn::IAsyncOperation<bool>> completion;
   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;
   HRESULT hr = winrt_utils::CreateActivationFactory(
       RuntimeClass_Windows_UI_StartScreen_SecondaryTile,
       tile_factory.GetAddressOf());
   CheckHR(hr, "Failed to create instance of ISecondaryTileFactory");
 
   winui::StartScreen::TileOptions options =
       winui::StartScreen::TileOptions_ShowNameOnLogo;
@@ skipping 32 matching lines @@
                                 tile.GetAddressOf());
   CheckHR(hr, "Failed to create tile");
 
   hr = tile->put_ForegroundText(winui::StartScreen::ForegroundText_Light);
   CheckHR(hr, "Failed to change foreground text color");
 
   mswr::ComPtr<winfoundtn::IAsyncOperation<bool>> completion;
   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
 
 BOOL MetroIsPinnedToStartScreen(const string16& tile_id) {
   mswr::ComPtr<winui::StartScreen::ISecondaryTileStatics> tile_statics;
   HRESULT hr = winrt_utils::CreateActivationFactory(
       RuntimeClass_Windows_UI_StartScreen_SecondaryTile,
       tile_statics.GetAddressOf());
   CheckHR(hr, "Failed to create instance of ISecondaryTileStatics");
 
   boolean exists;
   hr = tile_statics->Exists(MakeHString(tile_id), &exists);
   CheckHR(hr, "ISecondaryTileStatics.Exists failed");
   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));
 }