Comment on attachment 826479
0001-Bug-417952-Open-Containing-Folder-doesn-t-highlight-.patch

Thanks.  This is good.  Just some minor things:

>+  DBusGConnection* mDBusConnection = nullptr;
>+  DBusConnection* dbusConnection = nullptr;
>+  DBusGProxy* mDBusProxy = nullptr;

>+  gboolean rv_dbus_call;

Gecko uses the 'm' prefix on variables if they are member variables, but these
are local variables, so please rename mDBusConnection and mDBusProxy to 
something without the prefix, but still starting with a lower-case letter.

The initialization is unnecessary here, but this is C++ code, so these can be
declared when they are first set, and that is Gecko style. e.g.

DBusGConnection* mDBusConnection = dbus_g_bus_get(DBUS_BUS_SESSION,
&error);

Initializing out parameters, as you have with |error|, is often good
(and may be necessary even - I haven't checked).

>+  nsAutoCString uri("file://");
>+  uri.Append(aUri);

Please use g_filename_to_uri(aPath, nullptr, nullptr) to do any necessary
escaping.

>+  const char *uris[2] = { (const char*) uri.get(), NULL };

The (const char*) can be removed now, I assume.

Also, please use nullptr instead of NULL as nullptr provides more type safety
than a plain 0.

>+    } else if (giovfs &&
giovfs->OrgFreedesktopFileManager1ShowItems(mPath) == NS_OK) {

Gecko style is usually to write

   NS_SUCCEEDED(giovfs->OrgFreedesktopFileManager1ShowItems(mPath))

>   [noscript] void    showURIForInput(in ACString uri);
>+
>+  /* Open uri in file manager using org.freedesktop.FileManager1 interface */
>+  [noscript] void    orgFreedesktopFileManager1ShowItems(in ACString uri);

showURIForInput set a bad precedent here because the parameter is a path, not a
uri.

Please name the parameter for the new method "path", and update the comment.
I assume that is easier than changing the caller to pass a uri.

-- 
You received this bug notification because you are a member of Desktop
Packages, which is subscribed to firefox in Ubuntu.
https://bugs.launchpad.net/bugs/599846

Title:
  firefox right click open containing folder should highlight file

Status in The Mozilla Firefox Browser:
  Confirmed
Status in “firefox” package in Ubuntu:
  Confirmed

Bug description:
  Binary package hint: firefox

  The only reason I am assuming this is a bug is because the
  functionality is in Windows. Basically, if the user right clicks on a
  file in the Downloads of Firefox and selects "Open Containing Folder",
  Nautilus should open with the file highlighted to make the file easier
  to find.

  1) The release of Ubuntu you are using, via 'lsb_release -rd' or System -> 
About Ubuntu.
  Description:  Ubuntu 10.04 LTS
  Release:      10.04

  2) The version of the package you are using, via 'apt-cache policy 
packagename' or by checking in Synaptic.
    Installed: 3.6.3+nobinonly-0ubuntu4
    Candidate: 3.6.3+nobinonly-0ubuntu4
    Version table:
   *** 3.6.3+nobinonly-0ubuntu4 0
          500 http://us.archive.ubuntu.com/ubuntu/ lucid/main Packages
          100 /var/lib/dpkg/status

  
  3) What you expected to happen
  I expected my file to be highlighted.

  4) What happened instead
  Nautilus opened, but the file was not highlighted.

  ProblemType: Bug
  DistroRelease: Ubuntu 10.04
  Package: firefox 3.6.3+nobinonly-0ubuntu4
  ProcVersionSignature: Ubuntu 2.6.32-22.36-generic 2.6.32.11+drm33.2
  Uname: Linux 2.6.32-22-generic i686
  Architecture: i386
  Date: Tue Jun 29 11:15:58 2010
  FirefoxPackages:
   firefox 3.6.3+nobinonly-0ubuntu4
   firefox-gnome-support 3.6.3+nobinonly-0ubuntu4
   firefox-branding 3.6.3+nobinonly-0ubuntu4
   abroswer N/A
   abrowser-branding N/A
  InstallationMedia: Ubuntu 10.04 LTS "Lucid Lynx" - Release i386 (20100429)
  ProcEnviron:
   LANG=en_US.utf8
   SHELL=/bin/bash
  SourcePackage: firefox

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/599846/+subscriptions

-- 
Mailing list: https://launchpad.net/~desktop-packages
Post to     : [email protected]
Unsubscribe : https://launchpad.net/~desktop-packages
More help   : https://help.launchpad.net/ListHelp

Reply via email to