Files.app: Fix selection on creating new folder

Files.app: Fix selection on creating new folder

This fixes the focus glitch in creating new folder in Files.app. The patch fixes the following bugs:
- Inconsistency of timings between redrawing of the list and the culculation of selection.
  => Add start/endBatchUpdate in order to adjust timings. (file_manager.js & directory_model.js)
- The unselect in the list is not reflected in redrawing.
  => Check not only selecting but also unselecting in redraw  (list.js)

BUG=366506
TEST=manually tested

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270094

[yoshiki, 2014-05-09 06:46:27]

@hirono, could you take a look at the filemanager-related files?

[hirono, 2014-05-09 07:29:38]

https://codereview.chromium.org/275493006/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_manager/file_manager_browsertest.cc (right):

https://codereview.chromium.org/275493006/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/file_manager_browsertest.cc:759:
"createNewFolderAfterSelectFile"),
Why don't you run the test with NOT_IN_GUEST_MODE?

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js
(right):

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:8:
* Select the first item in the file list.
Selects

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:20:
// Wait until rename completes.
The comment is not correct.

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:22:
'queryAllElements',
The number of indent is 4.

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:26:
chrome.test.assertEq(1, elements.length);
Can we ensure the selected attribute is added here? Or should we use
waitElement?

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:51:
// Wait until rename completes.
The comment is incorrect?

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:53:
'queryAllElements',
Indent is 4.

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:55:
['div.detail-table > list > li[selected]']);
How about li[selected][renaming]? instead of checking it below.

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:59:
return Promise.resolve();
This is not needed.

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:79:
// Wait until rename completes.
The comment is incorrect?

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:86:
return Promise.resolve();
This is not needed.

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:101:
checkIfNoErrorsOccured.bind(null, chrome.test.callbackPass()),
How about using testPromise?
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/e...

https://codereview.chromium.org/275493006/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/directory_model.js (right):

https://codereview.chromium.org/275493006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/directory_model.js:586: * @param
{function()} abortCallback Callback on abort (cancelled by user).
We sometimes use ABORT_ERR to notify aborting. How about keeping consistency?

https://codereview.chromium.org/275493006/diff/20001/ui/webui/resources/js/cr...
File ui/webui/resources/js/cr/ui/list.js (right):

https://codereview.chromium.org/275493006/diff/20001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/list.js:1128: this.cachedItems_[y].selected =
!this.cachedItems_[y].selected;
nit: How about this.cachedItems_[y].selected = sm.getIndexSelected(y)
straightforwardly?

[yoshiki, 2014-05-09 13:54:16]

@hirono, PTAL? Thanks.

https://codereview.chromium.org/275493006/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_manager/file_manager_browsertest.cc (right):

https://codereview.chromium.org/275493006/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/file_manager_browsertest.cc:759:
"createNewFolderAfterSelectFile"),
On 2014/05/09 07:29:38, hirono wrote:
> Why don't you run the test with NOT_IN_GUEST_MODE?

I think, it should be enough only in NOT_IN_GUEST_MODE.

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js
(right):

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:8:
* Select the first item in the file list.
On 2014/05/09 07:29:38, hirono wrote:
> Selects

Done.

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:20:
// Wait until rename completes.
On 2014/05/09 07:29:38, hirono wrote:
> The comment is not correct.

Done.

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:22:
'queryAllElements',
On 2014/05/09 07:29:38, hirono wrote:
> The number of indent is 4.

Done.

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:26:
chrome.test.assertEq(1, elements.length);
On 2014/05/09 07:29:38, hirono wrote:
> Can we ensure the selected attribute is added here? Or should we use
> waitElement?

the selected attribute is checked in the query. And I'd like to check the number
of selected element.

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:51:
// Wait until rename completes.
On 2014/05/09 07:29:38, hirono wrote:
> The comment is incorrect?

Done.

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:53:
'queryAllElements',
On 2014/05/09 07:29:38, hirono wrote:
> Indent is 4.

Done.

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:55:
['div.detail-table > list > li[selected]']);
On 2014/05/09 07:29:38, hirono wrote:
> How about li[selected][renaming]? instead of checking it below.

I'd like to ensure that the number of selected item is 1.

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:59:
return Promise.resolve();
On 2014/05/09 07:29:38, hirono wrote:
> This is not needed.

Done.

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:79:
// Wait until rename completes.
On 2014/05/09 07:29:38, hirono wrote:
> The comment is incorrect?

Done.

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:86:
return Promise.resolve();
On 2014/05/09 07:29:38, hirono wrote:
> This is not needed.

Done.

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:101:
checkIfNoErrorsOccured.bind(null, chrome.test.callbackPass()),
On 2014/05/09 07:29:38, hirono wrote:
> How about using testPromise?
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/e...
> 

Done.

https://codereview.chromium.org/275493006/diff/20001/ui/webui/resources/js/cr...
File ui/webui/resources/js/cr/ui/list.js (right):

https://codereview.chromium.org/275493006/diff/20001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/list.js:1128: this.cachedItems_[y].selected =
!this.cachedItems_[y].selected;
On 2014/05/09 07:29:38, hirono wrote:
> nit: How about this.cachedItems_[y].selected = sm.getIndexSelected(y)
> straightforwardly?

getIndexSelected() may be heavy if it's overridden. I think just flipping the
boolean is enough.

[hirono, 2014-05-12 02:28:27]

Thank you!

https://codereview.chromium.org/275493006/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_manager/file_manager_browsertest.cc (right):

https://codereview.chromium.org/275493006/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/file_manager_browsertest.cc:759:
"createNewFolderAfterSelectFile"),
On 2014/05/09 13:54:17, yoshiki wrote:
> On 2014/05/09 07:29:38, hirono wrote:
> > Why don't you run the test with NOT_IN_GUEST_MODE?
> 
> I think, it should be enough only in NOT_IN_GUEST_MODE.

Yes, but line 758 is IN_GUEST_MODE. Is it intentional?

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js
(right):

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:26:
chrome.test.assertEq(1, elements.length);
On 2014/05/09 13:54:17, yoshiki wrote:
> On 2014/05/09 07:29:38, hirono wrote:
> > Can we ensure the selected attribute is added here? Or should we use
> > waitElement?
> 
> the selected attribute is checked in the query. And I'd like to check the
number
> of selected element.

I think it can cause a flakiness.
If detail-table-1 is already selected, it can success but may fail.

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:55:
['div.detail-table > list > li[selected]']);
On 2014/05/09 13:54:17, yoshiki wrote:
> On 2014/05/09 07:29:38, hirono wrote:
> > How about li[selected][renaming]? instead of checking it below.
> 
> I'd like to ensure that the number of selected item is 1.

SGTM.

https://codereview.chromium.org/275493006/diff/40001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js
(right):

https://codereview.chromium.org/275493006/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:13:
return Promise.resolve().then(function() {
nit: We can start with callRemoteTestUtil instead of Promise.resolve();

callRemoteTestUtil('fakeKeyDown', windowId, [...]).then(function() {
  ...
}).then(function()) {
  ...
});

https://codereview.chromium.org/275493006/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:39:
return Promise.resolve(
ditto.

https://codereview.chromium.org/275493006/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:84:
chrome.test.assertEq(1, elements.length);
Could you check the text to ensure the selected item is really the newly added
folder?

https://codereview.chromium.org/275493006/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:100:
testPromise(promise);
Could you move the testPromise function from keyboard_operations.js to
background.js? We start to use the function in other than
keyboard_operations.js.

[yoshiki, 2014-05-13 09:40:57]

@hirono PTAL

https://codereview.chromium.org/275493006/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_manager/file_manager_browsertest.cc (right):

https://codereview.chromium.org/275493006/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/file_manager_browsertest.cc:759:
"createNewFolderAfterSelectFile"),
On 2014/05/12 02:28:27, hirono wrote:
> On 2014/05/09 13:54:17, yoshiki wrote:
> > On 2014/05/09 07:29:38, hirono wrote:
> > > Why don't you run the test with NOT_IN_GUEST_MODE?
> > 
> > I think, it should be enough only in NOT_IN_GUEST_MODE.
> 
> Yes, but line 758 is IN_GUEST_MODE. Is it intentional?

Sorry, IN_GUEST_MODE is correct. I think running in only IN_GUEST_MODE is
enough.

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js
(right):

https://codereview.chromium.org/275493006/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:26:
chrome.test.assertEq(1, elements.length);
On 2014/05/12 02:28:27, hirono wrote:
> On 2014/05/09 13:54:17, yoshiki wrote:
> > On 2014/05/09 07:29:38, hirono wrote:
> > > Can we ensure the selected attribute is added here? Or should we use
> > > waitElement?
> > 
> > the selected attribute is checked in the query. And I'd like to check the
> number
> > of selected element.
> 
> I think it can cause a flakiness.
> If detail-table-1 is already selected, it can success but may fail.

Done.

https://codereview.chromium.org/275493006/diff/40001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js
(right):

https://codereview.chromium.org/275493006/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:13:
return Promise.resolve().then(function() {
On 2014/05/12 02:28:28, hirono wrote:
> nit: We can start with callRemoteTestUtil instead of Promise.resolve();
> 
> callRemoteTestUtil('fakeKeyDown', windowId, [...]).then(function() {
>   ...
> }).then(function()) {
>   ...
> });

In my opinion, it is better to use the same style as the succeeding steps for
readability. How do you think?

[hirono, 2014-05-13 10:16:28]

lgtm. Thank you very much!

https://codereview.chromium.org/275493006/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_manager/file_manager_browsertest.cc (right):

https://codereview.chromium.org/275493006/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/file_manager_browsertest.cc:759:
"createNewFolderAfterSelectFile"),
On 2014/05/13 09:40:57, yoshiki wrote:
> On 2014/05/12 02:28:27, hirono wrote:
> > On 2014/05/09 13:54:17, yoshiki wrote:
> > > On 2014/05/09 07:29:38, hirono wrote:
> > > > Why don't you run the test with NOT_IN_GUEST_MODE?
> > > 
> > > I think, it should be enough only in NOT_IN_GUEST_MODE.
> > 
> > Yes, but line 758 is IN_GUEST_MODE. Is it intentional?
> 
> Sorry, IN_GUEST_MODE is correct. I think running in only IN_GUEST_MODE is
> enough.

I agree with that one mode is enough here. And many users actually use
NOT_IN_GUEST_MODE, so I'm thinking the mode is better to test. WDYT?

https://codereview.chromium.org/275493006/diff/40001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js
(right):

https://codereview.chromium.org/275493006/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/create_new_folder.js:13:
return Promise.resolve().then(function() {
On 2014/05/13 09:40:57, yoshiki wrote:
> On 2014/05/12 02:28:28, hirono wrote:
> > nit: We can start with callRemoteTestUtil instead of Promise.resolve();
> > 
> > callRemoteTestUtil('fakeKeyDown', windowId, [...]).then(function() {
> >   ...
> > }).then(function()) {
> >   ...
> > });
> 
> In my opinion, it is better to use the same style as the succeeding steps for
> readability. How do you think?

I don't have a strong opinion. Please keep it as it is.

[yoshiki, 2014-05-13 13:21:38]

https://codereview.chromium.org/275493006/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_manager/file_manager_browsertest.cc (right):

https://codereview.chromium.org/275493006/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/file_manager_browsertest.cc:759:
"createNewFolderAfterSelectFile"),
On 2014/05/13 10:16:29, hirono wrote:
> On 2014/05/13 09:40:57, yoshiki wrote:
> > On 2014/05/12 02:28:27, hirono wrote:
> > > On 2014/05/09 13:54:17, yoshiki wrote:
> > > > On 2014/05/09 07:29:38, hirono wrote:
> > > > > Why don't you run the test with NOT_IN_GUEST_MODE?
> > > > 
> > > > I think, it should be enough only in NOT_IN_GUEST_MODE.
> > > 
> > > Yes, but line 758 is IN_GUEST_MODE. Is it intentional?
> > 
> > Sorry, IN_GUEST_MODE is correct. I think running in only IN_GUEST_MODE is
> > enough.
> 
> I agree with that one mode is enough here. And many users actually use
> NOT_IN_GUEST_MODE, so I'm thinking the mode is better to test. WDYT?

I don't have a strong opinion and changes it into NOT_IN_GUEST_MODE as you
guided. Thanks.

[yoshiki, 2014-05-13 13:21:43]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2014-05-13 13:21:50]

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

[commit-bot: I haz the power, 2014-05-13 14:57:38]

Change committed as 270094