Reorder functions in add_to_homescreen_dialog_helper.cc to match order in .h file

chrome/browser/android/webapps/add_to_homescreen_dialog_helper.h

Index: chrome/browser/android/webapps/add_to_homescreen_dialog_helper.h
diff --git a/chrome/browser/android/webapps/add_to_homescreen_dialog_helper.h b/chrome/browser/android/webapps/add_to_homescreen_dialog_helper.h
index 296011b65785f4a9bf31a87b6829b72e01154a70..66899b2809b138746f38e8db35ad4935125679f8 100644
--- a/chrome/browser/android/webapps/add_to_homescreen_dialog_helper.h
+++ b/chrome/browser/android/webapps/add_to_homescreen_dialog_helper.h
@@ -33,24 +33,17 @@ class AddToHomescreenDialogHelper :
                               jobject obj,
                               content::WebContents* web_contents);
 
-  // Called by the Java counterpart to destroy its native half.
-  void Destroy(JNIEnv* env, const base::android::JavaParamRef<jobject>& obj);
-
   // Registers JNI hooks.

[dominickn, 2016/08/24 22:58:16]

Nit: I would order the static method above the constructor.

[pkotwicz, 2016/08/24 23:14:50]

Done.

   static bool RegisterAddToHomescreenDialogHelper(JNIEnv* env);
 
+  // Called by the Java counterpart to destroy its native half.
+  void Destroy(JNIEnv* env, const base::android::JavaParamRef<jobject>& obj);
+
   // Adds a shortcut to the current URL to the Android home screen.
   void AddShortcut(JNIEnv* env,
                    const base::android::JavaParamRef<jobject>& obj,
                    const base::android::JavaParamRef<jstring>& title);
 
-  // AddToHomescreenDataFetcher::Observer
-  void OnUserTitleAvailable(const base::string16& user_title) override;
-  void OnDataAvailable(const ShortcutInfo& info, const SkBitmap& icon) override;
-  SkBitmap FinalizeLauncherIconInBackground(const SkBitmap& icon,
-                                            const GURL& url,
-                                            bool* is_generated) override;
-
  private:
   virtual ~AddToHomescreenDialogHelper();

[dominickn, 2016/08/24 22:58:16]

Just realised that this has a private virtual destructor. Not sure if that makes
sense - it should probably be public and virtual or private and non-virtual.

[pkotwicz, 2016/08/24 23:14:50]

Making the destructor private seems like a common pattern for C++ code owned by
Java. For instance:
https://cs.chromium.org/chromium/src/chrome/browser/android/recently_closed_t...

[dominickn, 2016/08/25 00:16:04]

I'm not fussed about the destructor being private - that's appropriate here. I
don't think the destructor should be private and virtual - that doesn't make
sense. :)

 
@@ -60,6 +53,13 @@ class AddToHomescreenDialogHelper :
 
   void RecordAddToHomescreen();
 
+  // AddToHomescreenDataFetcher::Observer:
+  void OnUserTitleAvailable(const base::string16& user_title) override;
+  void OnDataAvailable(const ShortcutInfo& info, const SkBitmap& icon) override;
+  SkBitmap FinalizeLauncherIconInBackground(const SkBitmap& icon,
+                                            const GURL& url,
+                                            bool* is_generated) override;
+
   // Points to the Java object.
   base::android::ScopedJavaGlobalRef<jobject> java_ref_;