Simplify directory initialization in Files app.

chrome/browser/ui/views/select_file_dialog_extension.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 "chrome/browser/ui/views/select_file_dialog_extension.h"
 
 #include "apps/shell_window.h"
 #include "apps/shell_window_registry.h"
 #include "apps/ui/native_app_window.h"
 #include "base/bind.h"
@@ skipping 278 matching lines @@
     if (!owner_browser) {
       // If an owner_window was supplied but we couldn't find a browser, this
       // could be for a shell window.
       shell_window = apps::ShellWindowRegistry::
           GetShellWindowForNativeWindowAnyProfile(owner_window);
     }
   }
 
   if (shell_window) {
     if (shell_window->window_type_is_panel()) {
       NOTREACHED() << "File dialog opened by panel.";

[Peter Kasting, 2014/01/24 19:17:25]

Nit: While here, can you fix this style guide violation?  We shouldn't be
handling DCHECK failure.  This should just be:

    DCHECK(!shell_window->window_type_is_panel());

[mtomasz, 2014/01/27 02:38:44]

Done.

       return;
     }
     base_window = shell_window->GetBaseWindow();
     web_contents = shell_window->web_contents();
   } else {
     // If the owning window is still unknown, this could be a background page or
     // and extension popup. Use the last active browser.
     if (!owner_browser) {
       owner_browser =
           chrome::FindLastActiveWithHostDesktopType(chrome::GetActiveDesktop());
     }
     DCHECK(owner_browser);
     if (!owner_browser) {

[Peter Kasting, 2014/01/24 19:17:25]

Nit: Similarly, can you remove this whole conditional block?

[mtomasz, 2014/01/27 02:38:44]

Done.

       LOG(ERROR) << "Could not find browser or shell window for popup.";
       return;
     }
     base_window = owner_browser->window();
     web_contents = owner_browser->tab_strip_model()->GetActiveWebContents();
   }
 
   DCHECK(base_window);
   DCHECK(web_contents);
   profile_ = Profile::FromBrowserContext(web_contents->GetBrowserContext());
   DCHECK(profile_);
 
   // Check if we have another dialog opened for the contents. It's unlikely, but
   // possible.
   RoutingID routing_id = GetRoutingIDFromWebContents(web_contents);
   if (PendingExists(routing_id)) {
     DLOG(WARNING) << "Pending dialog exists with id " << routing_id;
     return;
   }
 
-  base::FilePath default_dialog_path;
-
   const PrefService* pref_service = profile_->GetPrefs();
-
-  if (default_path.empty() && pref_service) {
-    default_dialog_path =
-        pref_service->GetFilePath(prefs::kDownloadDefaultDirectory);
-  } else {
-    default_dialog_path = default_path;
+  if (!pref_service) {
+    LOG(ERROR) << "Could not find the pref service.";

[Peter Kasting, 2014/01/24 19:17:25]

In general, don't use logging statements.  They bloat the binary but are very
little use to us because normally we can't collect their data.  They also
obscure the question of whether some codepath can actually occur or not.

If something really can't happen, and you want to assert that to the reader,
DCHECK it and then move on without handling it.  Otherwise, just handle it
silently, with a comment as necessary to explain under what conditions it can
occur.

If you don't know whether or not something can happen, find out before checking
in.

I realize the existing code here also used logging statements, which it
shouldn't have; you don't need to clean this up before landing (although you're
welcome to do so).

(Several places)

[mtomasz, 2014/01/27 02:38:44]

Done.

+    return;
   }
 
-  base::FilePath virtual_path;
-  base::FilePath fallback_path = profile_->last_selected_directory().Append(
-      default_dialog_path.BaseName());
-  // If an absolute path is specified as the default path, convert it to the
-  // virtual path in the file browser extension. Due to the current design,
-  // an invalid temporal cache file path may passed as |default_dialog_path|
-  // (crbug.com/178013 #9-#11). In such a case, we use the last selected
-  // directory as a workaround. Real fix is tracked at crbug.com/110119.
+  // All of the paths below are absolute.
+  base::FilePath download_default_path(
+      pref_service->GetFilePath(prefs::kDownloadDefaultDirectory));
+
+  base::FilePath selection_path = default_path.IsAbsolute() ?
+      default_path : download_default_path.Append(default_path.BaseName());
+  base::FilePath current_directory_path = selection_path.DirName();
+
+  base::FilePath current_directory_virtual_path;
+  base::FilePath selection_virtual_path;

[Peter Kasting, 2014/01/24 19:17:25]

Nit: Declare these as close to their initializations as possible.

[mtomasz, 2014/01/27 02:38:44]

Done.

+
+  base::FilePath fallback_path = !profile_->last_selected_directory().empty() ?

[Peter Kasting, 2014/01/24 19:17:25]

Nit: Remove "!" and reverse arms

[mtomasz, 2014/01/27 02:38:44]

Done, but is it better? I usually do this:
condition ? more_likely : less_likely.

And I feel that this is often done this way.

[Peter Kasting, 2014/01/28 03:22:04]

I find it easier to read conditionals when they're expressed as simply as
possible.  In this case, given that the negated term is "directory empty", it's
still pretty readable when negated, but in general, I find my brain getting a
bit hung up on the "else" side of such conditionals, because they amount to a
double negative.

+      profile_->last_selected_directory() : download_default_path;
+
+  // Convert the above absolute paths to virtual paths.
+  // TODO(mtomasz): Use URLs instead of paths.
   using file_manager::kFileManagerAppId;

[Peter Kasting, 2014/01/24 19:17:25]

Nit: This using statement doesn't buy us much; all uses of this symbol have
plenty of space for the qualifier, and the qualifier isn't hugely long and
unreadable.  I'd just nuke this.

[mtomasz, 2014/01/27 02:38:44]

Done.

-  if (default_dialog_path.IsAbsolute() &&
-      (file_manager::util::ConvertAbsoluteFilePathToRelativeFileSystemPath(
-           profile_, kFileManagerAppId, default_dialog_path, &virtual_path) ||
-       file_manager::util::ConvertAbsoluteFilePathToRelativeFileSystemPath(
-           profile_, kFileManagerAppId, fallback_path, &virtual_path))) {
-    virtual_path = base::FilePath("/").Append(virtual_path);
-  } else {
-    // If the path was relative, or failed to convert, just use the base name,
-    virtual_path = default_dialog_path.BaseName();
+  if (!file_manager::util::ConvertAbsoluteFilePathToRelativeFileSystemPath(
+          profile_,

[Peter Kasting, 2014/01/24 19:17:25]

Nit: Indent 4, not 8

You're welcome to put more than one arg on a line if you wish.

[mtomasz, 2014/01/27 02:38:44]

Over and over again I receive contrasting comments during code reviews about
this. Some engineers want absolute 4 from the block beginning, other 4 starting
from `(`. What's the correct way?

Done.

[Peter Kasting, 2014/01/28 03:22:04]

The Google style guide is ambiguous.  My memory of C-style discussions is that
the intent is that a subsequent line that needs indenting be indented 4 space
from the previous line's indent level, i.e. "the former".

+          kFileManagerAppId,
+          selection_path,
+          &selection_virtual_path)) {
+    // Due to the current design, an invalid temporal cache file path may passed
+    // as |default_path| (crbug.com/178013 #9-#11). In such a case, we use the
+    // last selected directory as a workaround. Real fix is tracked at
+    // crbug.com/110119.

[Peter Kasting, 2014/01/24 19:17:25]

Nit: Normally I discourage bug references in comments; it's nicer to just put
all the necessary context in the code.  In this case perhaps the first reference
could be expanded upon (or just eliminated), and the second converted to a
TODO(...): crbug.com/110119: ...?

[mtomasz, 2014/01/27 02:38:44]

I'm not sure if I understand. I think the context is well described. Also, note
that this is not a new comment, but a comment moved from another place, which
existed before.

[Peter Kasting, 2014/01/28 03:22:04]

I mostly meant, if "an invalid temporal cache file path may passed as
|default_path|" is sufficiently descriptive for any reader on its own, the first
bug reference is unnecessary; if not, it'd be better to summarize the additional
information directly here instead of referring readers to somewhere else.  So
either way the bug reference is suboptimal.

The second reference looks like a TODO, hence the suggestion of writing it as a
standard TODO.  Otherwise, it doesn't say much to the reader, so if a
description of the real fix is useful, that would serve better here than a
reference to something external.

+    if (!file_manager::util::ConvertAbsoluteFilePathToRelativeFileSystemPath(
+            profile_,
+            kFileManagerAppId,
+            fallback_path.Append(selection_path.BaseName()),
+            &selection_virtual_path)) {
+      LOG(ERROR) << "Unable to resolve the selection path.";
+      return;
+    }
   }
+  selection_virtual_path = base::FilePath("/").Append(selection_virtual_path);

[Peter Kasting, 2014/01/24 19:17:25]

I don't work with FilePaths enough to know the answer to this, but using "/"
directly here seems worrisome.  A quick codesearch shows that we don't really do
this anywhere else.  Should we be using base::FilePath::kSeparators[0] here?

(2 places)

[mtomasz, 2014/01/27 02:38:44]

This works, because select_file_dialog_extension.cc is used by Chrome OS only.
For Windows adding a leading "\" wouldn't make any sense.

Note, that (1) this is not a new code, (2) this trick will be removed by an
upcoming patch which replaces paths with URLs. See line #354 for a TODO.

I added an extra comment.

+
+  if (!file_manager::util::ConvertAbsoluteFilePathToRelativeFileSystemPath(
+          profile_,
+          kFileManagerAppId,
+          current_directory_path,
+          &current_directory_virtual_path)) {
+    // Fallback if necessary, see the comment above.
+    if (!file_manager::util::ConvertAbsoluteFilePathToRelativeFileSystemPath(
+            profile_,
+            kFileManagerAppId,
+            fallback_path,
+            &current_directory_virtual_path)) {
+      LOG(ERROR) << "Unable to resolve the current directory path: "
+                 << fallback_path.value();
+      return;
+    }
+  }
+  current_directory_virtual_path = base::FilePath("/").Append(
+      current_directory_virtual_path);
 
   has_multiple_file_type_choices_ =
       file_types ? file_types->extensions.size() > 1 : true;

[Peter Kasting, 2014/01/24 19:17:25]

Nit: Slightly simpler:

      !file_types || (file_types->extensions.size() > 1);

[mtomasz, 2014/01/27 02:38:44]

Done.

 
   GURL file_manager_url =
       file_manager::util::GetFileManagerMainPageUrlWithParams(
-          type, title, virtual_path, file_types, file_type_index,
+          type,
+          title,
+          current_directory_virtual_path,
+          selection_virtual_path,
+          file_types,
+          file_type_index,
           default_extension);
 
   ExtensionDialog* dialog = ExtensionDialog::Show(file_manager_url,
       base_window, profile_, web_contents,
       kFileManagerWidth, kFileManagerHeight,
       kFileManagerMinimumWidth,
       kFileManagerMinimumHeight,
 #if defined(USE_AURA)
       file_manager::util::GetSelectFileDialogTitle(type),
 #else
       // HTML-based header used.
       base::string16(),
 #endif
       this /* ExtensionDialog::Observer */);
   if (!dialog) {
     LOG(ERROR) << "Unable to create extension dialog";
     return;
   }
 
   // Connect our listener to FileDialogFunction's per-tab callbacks.
   AddPending(routing_id);
 
   extension_dialog_ = dialog;
   params_ = params;
   routing_id_ = routing_id;
   owner_window_ = owner_window;
 }