Issue 144783002: Simplify directory initialization in Files app.

Issue 144783002: Simplify directory initialization in Files app. (Closed)

Created:
6 years, 11 months ago by mtomasz

Modified:
6 years, 10 months ago

Reviewers:
hirono, Peter Kasting, sky, kinaba

CC:
chromium-reviews, extensions-reviews_chromium.org, nkostylev+watch_chromium.org, tfarina, rginda+watch_chromium.org, mtomasz+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Simplify directory initialization in Files app. This patch cleans up the initialization code, by separating the current directory from selection. Before, there was only one variable holding either current directory or the selection, and heuristics were used to determine how to interpred the path there. The new code is also a preparation for migrating to URLs, since there are no more operations on the passed paths on the JS side. Note, that before we were fetching parent of a non existing path, which would be very tricky after migrating to URLs. And finally, this patch fixes wrong behaviour of the select directory dialog. Since now, the selected directory is really selected, and not opened. TEST=Tested manually several scenarios. All dialogs + recovering from crash. Drive and Downloads. BUG=328767, 321962 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247556

Patch Set 1 #

Total comments: 10

Patch Set 2 : Fixed. #

Total comments: 23

Patch Set 3 : Addressed comments. #

Patch Set 4 : Rebased. #

Patch Set 5 : Fixed tests. #

Patch Set 6 : Fixed tests. #

Patch Set 7 : Cleaned up. #

Created: 6 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+272 lines, -239 lines)			Patch
M	chrome/browser/chromeos/file_manager/url_util.h	View		1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/browser/chromeos/file_manager/url_util.cc	View		1 chunk	+6 lines, -2 lines	0 comments	Download
M	chrome/browser/chromeos/file_manager/url_util_unittest.cc	View	1 2 3 4	4 chunks	+10 lines, -4 lines	0 comments	Download
M	chrome/browser/resources/file_manager/background/js/background.js	View		3 chunks	+29 lines, -11 lines	0 comments	Download
M	chrome/browser/resources/file_manager/common/js/util.js	View	1 2	2 chunks	+17 lines, -15 lines	0 comments	Download
M	chrome/browser/resources/file_manager/foreground/js/file_manager.js	View	1 2 3	12 chunks	+104 lines, -136 lines	0 comments	Download
M	chrome/browser/resources/file_manager/foreground/js/file_manager_commands.js	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/file_manager/foreground/js/file_tasks.js	View	1 2	1 chunk	+6 lines, -2 lines	0 comments	Download
M	chrome/browser/resources/file_manager/foreground/js/photo/gallery.js	View		1 chunk	+3 lines, -1 line	0 comments	Download
M	chrome/browser/ui/views/select_file_dialog_extension.cc	View	1 2 3 4 5	3 chunks	+65 lines, -41 lines	0 comments	Download
M	chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc	View	1 2 3 4 5 6	3 chunks	+4 lines, -0 lines	0 comments	Download
M	chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js	View	1 2 3	25 chunks	+25 lines, -25 lines	0 comments	Download

Messages

Total messages: 21 (0 generated)

Expand Messages | Collapse Messages

mtomasz

@kinaba: PTAL at C++ @hirono: PTAL at JS. This is the second approach. The next ...

6 years, 11 months ago (2014-01-22 06:45:44 UTC) #1

kinaba

C++ lgtm https://codereview.chromium.org/144783002/diff/1/chrome/browser/resources/file_manager/foreground/js/file_manager.js File chrome/browser/resources/file_manager/foreground/js/file_manager.js (right): https://codereview.chromium.org/144783002/diff/1/chrome/browser/resources/file_manager/foreground/js/file_manager.js#newcode1411 chrome/browser/resources/file_manager/foreground/js/file_manager.js:1411: console.log(nextCurrentDirEntry, selectionEntry); Don't forget to remove these ...

6 years, 11 months ago (2014-01-23 04:58:02 UTC) #2

hirono

https://codereview.chromium.org/144783002/diff/1/chrome/browser/resources/file_manager/background/js/background.js File chrome/browser/resources/file_manager/background/js/background.js (right): https://codereview.chromium.org/144783002/diff/1/chrome/browser/resources/file_manager/background/js/background.js#newcode535 chrome/browser/resources/file_manager/background/js/background.js:535: if (opt_appState.currentDirectoryPath !== Please also update appStates in chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js. ...

6 years, 11 months ago (2014-01-23 05:35:56 UTC) #3

mtomasz

6 years, 11 months ago (2014-01-23 06:27:04 UTC) #4

hirono

On 2014/01/23 06:27:04, mtomasz wrote: > https://codereview.chromium.org/144783002/diff/1/chrome/browser/resources/file_manager/background/js/background.js > File chrome/browser/resources/file_manager/background/js/background.js (right): > > https://codereview.chromium.org/144783002/diff/1/chrome/browser/resources/file_manager/background/js/background.js#newcode535 > ...

6 years, 11 months ago (2014-01-23 06:43:06 UTC) #5

On 2014/01/23 06:27:04, mtomasz wrote:
>
https://codereview.chromium.org/144783002/diff/1/chrome/browser/resources/fil...
> File chrome/browser/resources/file_manager/background/js/background.js
(right):
> 
>
https://codereview.chromium.org/144783002/diff/1/chrome/browser/resources/fil...
> chrome/browser/resources/file_manager/background/js/background.js:535: if
> (opt_appState.currentDirectoryPath !==
> On 2014/01/23 05:35:57, hirono wrote:
> > Please also update appStates in
> > chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js.
> 
> Done.
> 
>
https://codereview.chromium.org/144783002/diff/1/chrome/browser/resources/fil...
> File chrome/browser/resources/file_manager/common/js/util.js (right):
> 
>
https://codereview.chromium.org/144783002/diff/1/chrome/browser/resources/fil...
> chrome/browser/resources/file_manager/common/js/util.js:685:
util.updateAppState
> = function(currentDirectoryPath, selectionPath, opt_param) {
> On 2014/01/23 05:35:57, hirono wrote:
> > How about creating AppState class and making the method a member of the
class?
> 
> Sounds good, but let me limit scope of this CL to as narrow as possible, since
> it is already too big, and work on it separately.
> 
>
https://codereview.chromium.org/144783002/diff/1/chrome/browser/resources/fil...
> File chrome/browser/resources/file_manager/foreground/js/file_manager.js
> (right):
> 
>
https://codereview.chromium.org/144783002/diff/1/chrome/browser/resources/fil...
> chrome/browser/resources/file_manager/foreground/js/file_manager.js:1408: //
> Resolve the selectionPath to selectionEntry or to currentDirectoryEntry
> On 2014/01/23 05:35:57, hirono wrote:
> > How about using Promise here? There are many fallback lines in
> > setupCurrentDirectory_ and Promise makes it easy to handle fallback.
> 
> I think using Promise here would make the code much more complicated than
using
> a simple Queue. How about trying it in a separate CL? I'm concerned, this CL
is
> already too big, so I would like to keep it as simple as possible at this
> moment.
> 
>
https://codereview.chromium.org/144783002/diff/1/chrome/browser/resources/fil...
> chrome/browser/resources/file_manager/foreground/js/file_manager.js:1411:
> console.log(nextCurrentDirEntry, selectionEntry);
> On 2014/01/23 04:58:02, kinaba wrote:
> > Don't forget to remove these debug logs.
> 
> Done.

Thanks, lgtm!

hirono

https://codereview.chromium.org/144783002/diff/1/chrome/browser/resources/file_manager/common/js/util.js File chrome/browser/resources/file_manager/common/js/util.js (right): https://codereview.chromium.org/144783002/diff/1/chrome/browser/resources/file_manager/common/js/util.js#newcode685 chrome/browser/resources/file_manager/common/js/util.js:685: util.updateAppState = function(currentDirectoryPath, selectionPath, opt_param) { On 2014/01/23 06:27:04, ...

6 years, 11 months ago (2014-01-23 06:43:17 UTC) #6

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/144783002/80001

6 years, 11 months ago (2014-01-23 08:06:12 UTC) #7

commit-bot: I haz the power

Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=46039

6 years, 11 months ago (2014-01-23 08:20:36 UTC) #8

mtomasz

On 2014/01/23 08:20:36, I haz the power (commit-bot) wrote: > Retried try job too often ...

6 years, 11 months ago (2014-01-23 08:27:08 UTC) #9

Peter Kasting

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/select_file_dialog_extension.cc File chrome/browser/ui/views/select_file_dialog_extension.cc (right): https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/select_file_dialog_extension.cc#newcode299 chrome/browser/ui/views/select_file_dialog_extension.cc:299: NOTREACHED() << "File dialog opened by panel."; Nit: While ...

6 years, 11 months ago (2014-01-24 19:17:19 UTC) #10

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/select_file_dialog_extension.cc (right):

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/select_file_dialog_extension.cc:299: NOTREACHED() <<
"File dialog opened by panel.";
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());

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/select_file_dialog_extension.cc:312: if (!owner_browser)
{
Nit: Similarly, can you remove this whole conditional block?

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/select_file_dialog_extension.cc:335: LOG(ERROR) <<
"Could not find the pref service.";
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)

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/select_file_dialog_extension.cc:348: base::FilePath
selection_virtual_path;
Nit: Declare these as close to their initializations as possible.

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/select_file_dialog_extension.cc:350: base::FilePath
fallback_path = !profile_->last_selected_directory().empty() ?
Nit: Remove "!" and reverse arms

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/select_file_dialog_extension.cc:355: using
file_manager::kFileManagerAppId;
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.

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/select_file_dialog_extension.cc:357: profile_,
Nit: Indent 4, not 8

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

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/select_file_dialog_extension.cc:364: //
crbug.com/110119.
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: ...?

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/select_file_dialog_extension.cc:374:
selection_virtual_path = base::FilePath("/").Append(selection_virtual_path);
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)

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/select_file_dialog_extension.cc:396: file_types ?
file_types->extensions.size() > 1 : true;
Nit: Slightly simpler:

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

mtomasz

6 years, 11 months ago (2014-01-27 02:38:43 UTC) #11

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/select_file_dialog_extension.cc (right):

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/select_file_dialog_extension.cc:299: NOTREACHED() <<
"File dialog opened by panel.";
On 2014/01/24 19:17:25, Peter Kasting wrote:
> 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());

Done.

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/select_file_dialog_extension.cc:312: if (!owner_browser)
{
On 2014/01/24 19:17:25, Peter Kasting wrote:
> Nit: Similarly, can you remove this whole conditional block?

Done.

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/select_file_dialog_extension.cc:335: LOG(ERROR) <<
"Could not find the pref service.";
On 2014/01/24 19:17:25, Peter Kasting wrote:
> 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)

Done.

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/select_file_dialog_extension.cc:348: base::FilePath
selection_virtual_path;
On 2014/01/24 19:17:25, Peter Kasting wrote:
> Nit: Declare these as close to their initializations as possible.

Done.

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/select_file_dialog_extension.cc:350: base::FilePath
fallback_path = !profile_->last_selected_directory().empty() ?
On 2014/01/24 19:17:25, Peter Kasting wrote:
> Nit: Remove "!" and reverse arms

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

And I feel that this is often done this way.

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/select_file_dialog_extension.cc:355: using
file_manager::kFileManagerAppId;
On 2014/01/24 19:17:25, Peter Kasting wrote:
> 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.

Done.

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/select_file_dialog_extension.cc:357: profile_,
On 2014/01/24 19:17:25, Peter Kasting wrote:
> Nit: Indent 4, not 8
> 
> You're welcome to put more than one arg on a line if you wish.

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.

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/select_file_dialog_extension.cc:364: //
crbug.com/110119.
On 2014/01/24 19:17:25, Peter Kasting wrote:
> 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: ...?

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.

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/select_file_dialog_extension.cc:374:
selection_virtual_path = base::FilePath("/").Append(selection_virtual_path);
On 2014/01/24 19:17:25, Peter Kasting wrote:
> 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)

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.

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/select_file_dialog_extension.cc:396: file_types ?
file_types->extensions.size() > 1 : true;
On 2014/01/24 19:17:25, Peter Kasting wrote:
> Nit: Slightly simpler:
> 
>       !file_types || (file_types->extensions.size() > 1);

Done.

mtomasz

On 2014/01/27 02:38:43, mtomasz wrote: > https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/select_file_dialog_extension.cc > File chrome/browser/ui/views/select_file_dialog_extension.cc (right): > > https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/select_file_dialog_extension.cc#newcode299 > ...

6 years, 10 months ago (2014-01-28 00:11:26 UTC) #12

On 2014/01/27 02:38:43, mtomasz wrote:
>
https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/select_file_dialog_extension.cc (right):
> 
>
https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
> chrome/browser/ui/views/select_file_dialog_extension.cc:299: NOTREACHED() <<
> "File dialog opened by panel.";
> On 2014/01/24 19:17:25, Peter Kasting wrote:
> > 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());
> 
> Done.
> 
>
https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
> chrome/browser/ui/views/select_file_dialog_extension.cc:312: if
(!owner_browser)
> {
> On 2014/01/24 19:17:25, Peter Kasting wrote:
> > Nit: Similarly, can you remove this whole conditional block?
> 
> Done.
> 
>
https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
> chrome/browser/ui/views/select_file_dialog_extension.cc:335: LOG(ERROR) <<
> "Could not find the pref service.";
> On 2014/01/24 19:17:25, Peter Kasting wrote:
> > 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)
> 
> Done.
> 
>
https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
> chrome/browser/ui/views/select_file_dialog_extension.cc:348: base::FilePath
> selection_virtual_path;
> On 2014/01/24 19:17:25, Peter Kasting wrote:
> > Nit: Declare these as close to their initializations as possible.
> 
> Done.
> 
>
https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
> chrome/browser/ui/views/select_file_dialog_extension.cc:350: base::FilePath
> fallback_path = !profile_->last_selected_directory().empty() ?
> On 2014/01/24 19:17:25, Peter Kasting wrote:
> > Nit: Remove "!" and reverse arms
> 
> Done, but is it better? I usually do this:
> condition ? more_likely : less_likely.
> 
> And I feel that this is often done this way.
> 
>
https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
> chrome/browser/ui/views/select_file_dialog_extension.cc:355: using
> file_manager::kFileManagerAppId;
> On 2014/01/24 19:17:25, Peter Kasting wrote:
> > 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.
> 
> Done.
> 
>
https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
> chrome/browser/ui/views/select_file_dialog_extension.cc:357: profile_,
> On 2014/01/24 19:17:25, Peter Kasting wrote:
> > Nit: Indent 4, not 8
> > 
> > You're welcome to put more than one arg on a line if you wish.
> 
> 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.
> 
>
https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
> chrome/browser/ui/views/select_file_dialog_extension.cc:364: //
> crbug.com/110119.
> On 2014/01/24 19:17:25, Peter Kasting wrote:
> > 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: ...?
> 
> 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.
> 
>
https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
> chrome/browser/ui/views/select_file_dialog_extension.cc:374:
> selection_virtual_path = base::FilePath("/").Append(selection_virtual_path);
> On 2014/01/24 19:17:25, Peter Kasting wrote:
> > 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)
> 
> 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.
> 
>
https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
> chrome/browser/ui/views/select_file_dialog_extension.cc:396: file_types ?
> file_types->extensions.size() > 1 : true;
> On 2014/01/24 19:17:25, Peter Kasting wrote:
> > Nit: Slightly simpler:
> > 
> >       !file_types || (file_types->extensions.size() > 1);
> 
> Done.

@msw: Can you please take a look since @pkasting is OOO entire week? This CL is
blocking two other CLs. Thanks!

mtomasz

On 2014/01/28 00:11:26, mtomasz wrote: > On 2014/01/27 02:38:43, mtomasz wrote: > > > https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/select_file_dialog_extension.cc ...

6 years, 10 months ago (2014-01-28 00:22:52 UTC) #13

On 2014/01/28 00:11:26, mtomasz wrote:
> On 2014/01/27 02:38:43, mtomasz wrote:
> >
>
https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
> > File chrome/browser/ui/views/select_file_dialog_extension.cc (right):
> > 
> >
>
https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
> > chrome/browser/ui/views/select_file_dialog_extension.cc:299: NOTREACHED() <<
> > "File dialog opened by panel.";
> > On 2014/01/24 19:17:25, Peter Kasting wrote:
> > > 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());
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
> > chrome/browser/ui/views/select_file_dialog_extension.cc:312: if
> (!owner_browser)
> > {
> > On 2014/01/24 19:17:25, Peter Kasting wrote:
> > > Nit: Similarly, can you remove this whole conditional block?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
> > chrome/browser/ui/views/select_file_dialog_extension.cc:335: LOG(ERROR) <<
> > "Could not find the pref service.";
> > On 2014/01/24 19:17:25, Peter Kasting wrote:
> > > 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)
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
> > chrome/browser/ui/views/select_file_dialog_extension.cc:348: base::FilePath
> > selection_virtual_path;
> > On 2014/01/24 19:17:25, Peter Kasting wrote:
> > > Nit: Declare these as close to their initializations as possible.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
> > chrome/browser/ui/views/select_file_dialog_extension.cc:350: base::FilePath
> > fallback_path = !profile_->last_selected_directory().empty() ?
> > On 2014/01/24 19:17:25, Peter Kasting wrote:
> > > Nit: Remove "!" and reverse arms
> > 
> > Done, but is it better? I usually do this:
> > condition ? more_likely : less_likely.
> > 
> > And I feel that this is often done this way.
> > 
> >
>
https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
> > chrome/browser/ui/views/select_file_dialog_extension.cc:355: using
> > file_manager::kFileManagerAppId;
> > On 2014/01/24 19:17:25, Peter Kasting wrote:
> > > 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.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
> > chrome/browser/ui/views/select_file_dialog_extension.cc:357: profile_,
> > On 2014/01/24 19:17:25, Peter Kasting wrote:
> > > Nit: Indent 4, not 8
> > > 
> > > You're welcome to put more than one arg on a line if you wish.
> > 
> > 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.
> > 
> >
>
https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
> > chrome/browser/ui/views/select_file_dialog_extension.cc:364: //
> > crbug.com/110119.
> > On 2014/01/24 19:17:25, Peter Kasting wrote:
> > > 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: ...?
> > 
> > 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.
> > 
> >
>
https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
> > chrome/browser/ui/views/select_file_dialog_extension.cc:374:
> > selection_virtual_path = base::FilePath("/").Append(selection_virtual_path);
> > On 2014/01/24 19:17:25, Peter Kasting wrote:
> > > 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)
> > 
> > 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.
> > 
> >
>
https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
> > chrome/browser/ui/views/select_file_dialog_extension.cc:396: file_types ?
> > file_types->extensions.size() > 1 : true;
> > On 2014/01/24 19:17:25, Peter Kasting wrote:
> > > Nit: Slightly simpler:
> > > 
> > >       !file_types || (file_types->extensions.size() > 1);
> > 
> > Done.
> 
> @msw: Can you please take a look since @pkasting is OOO entire week? This CL
is
> blocking two other CLs. Thanks!

@sky: Can you please take a look since @pkasting is OOO until the end of this
week? This CL is
 blocking two other CLs. Thanks!

mtomasz

On 2014/01/28 00:45:56, sky wrote: > LGTM Thanks for the fast reply!

6 years, 10 months ago (2014-01-28 00:46:36 UTC) #15

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/144783002/480001

6 years, 10 months ago (2014-01-28 00:48:39 UTC) #16

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/144783002/560002

6 years, 10 months ago (2014-01-28 02:07:34 UTC) #17

Peter Kasting

(Just FYI, since you asked some questions) https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/select_file_dialog_extension.cc File chrome/browser/ui/views/select_file_dialog_extension.cc (right): https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/select_file_dialog_extension.cc#newcode350 chrome/browser/ui/views/select_file_dialog_extension.cc:350: base::FilePath fallback_path ...

6 years, 10 months ago (2014-01-28 03:22:03 UTC) #18

(Just FYI, since you asked some questions)

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/select_file_dialog_extension.cc (right):

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/select_file_dialog_extension.cc:350: base::FilePath
fallback_path = !profile_->last_selected_directory().empty() ?
On 2014/01/27 02:38:44, mtomasz wrote:
> On 2014/01/24 19:17:25, Peter Kasting wrote:
> > Nit: Remove "!" and reverse arms
> 
> Done, but is it better? I usually do this:
> condition ? more_likely : less_likely.
> 
> And I feel that this is often done this way.

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.

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/select_file_dialog_extension.cc:357: profile_,
On 2014/01/27 02:38:44, mtomasz wrote:
> On 2014/01/24 19:17:25, Peter Kasting wrote:
> > Nit: Indent 4, not 8
> > 
> > You're welcome to put more than one arg on a line if you wish.
> 
> 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?

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".

https://codereview.chromium.org/144783002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/select_file_dialog_extension.cc:364: //
crbug.com/110119.
On 2014/01/27 02:38:44, mtomasz wrote:
> On 2014/01/24 19:17:25, Peter Kasting wrote:
> > 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: ...?
> 
> I'm not sure if I understand. I think the context is well described.

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.

commit-bot: I haz the power

Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=252622

6 years, 10 months ago (2014-01-28 03:34:23 UTC) #19

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/144783002/480002

6 years, 10 months ago (2014-01-28 18:24:26 UTC) #20

Message was sent while issue was closed.

Change committed as 247556

Expand Messages | Collapse Messages