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

 // Copyright 2015 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.
 
 #ifndef CHROME_BROWSER_ANDROID_WEBAPPS_ADD_TO_HOMESCREEN_DIALOG_HELPER_H_
 #define CHROME_BROWSER_ANDROID_WEBAPPS_ADD_TO_HOMESCREEN_DIALOG_HELPER_H_
 
 #include "base/android/jni_android.h"
 #include "base/android/scoped_java_ref.h"
 #include "base/macros.h"
@@ skipping 15 matching lines @@
 // org.chromium.chrome.browser's AddToHomescreenDialogHelper in Java. The object
 // is owned by the Java object. It is created from there via a JNI (Initialize)
 // call and MUST BE DESTROYED via Destroy().
 class AddToHomescreenDialogHelper :
     public AddToHomescreenDataFetcher::Observer {
  public:
   AddToHomescreenDialogHelper(JNIEnv* env,
                               jobject obj,
                               content::WebContents* web_contents);
 
+  // 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);
 
-  // Registers JNI hooks.
-  static bool RegisterAddToHomescreenDialogHelper(JNIEnv* env);
-
   // 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. :)

 
   // Called only when the AddToHomescreenDataFetcher has retrieved all of the
   // data needed to add the shortcut.
   void AddShortcut(const ShortcutInfo& info, const SkBitmap& icon);
 
   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_;
 
   // Whether the user has requested that a shortcut be added while a fetch was
   // in progress.
   bool add_shortcut_pending_;
 
   // Fetches data required to add a shortcut.
   scoped_refptr<AddToHomescreenDataFetcher> data_fetcher_;
 
   DISALLOW_COPY_AND_ASSIGN(AddToHomescreenDialogHelper);
 };
 
 #endif  // CHROME_BROWSER_ANDROID_WEBAPPS_ADD_TO_HOMESCREEN_DIALOG_HELPER_H_