Change directory if the active list item on navigation list is changed.

Change directory if the active list item on navigation list is changed.

We want to change current directory if the active item on navigation list is changed.
But we don't want to change current directory when listed items are spliced or permuted.
In both cases, 'change' event is dispatched from selection model and we can't distinguish these cases from event itself.
currentActiveItem_ is introduced to detect change of actual active item.

BUG=375663
TEST=go through steps in Issue 375663 and 361047

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

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

[fukino, 2014-05-26 13:00:19]

@yoshiki - This is a plan which looks robust for me (among plans I can came up
with). What do you think?

[fukino, 2014-05-27 09:07:17]

yoshiki san, could you take a look?

I modified three functions.
onSelectionChange_() : Before returning, update the selected model item. If
actual selected model is changed and dontHandleSelectionEvent_ is false, we
change current directory to the root directory of selected model item.
selectByIndex() : Now this method is for handling request from outside to change
index. The only client is volume shortcuts(Ctrl+[0-9]). If this is called,
change selected index and activate the selected item.
activateModelItem() : Always opens the root directory of given model item.

[yoshiki, 2014-05-29 04:24:44]

https://codereview.chromium.org/303503004/diff/80001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js
(right):

https://codereview.chromium.org/303503004/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js:21:
* Entry set which is used for this test.
Please add @type annotation.

https://codereview.chromium.org/303503004/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js:33:
* Constants for each folders.
Please add @type annotation.

https://codereview.chromium.org/303503004/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js:35:
var DIR = {
nit: DIRECTORIES

https://codereview.chromium.org/303503004/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js:74:
* @return {Promise} Promise fulfilled with an array containing twi window IDs.
s/twi/two/

https://codereview.chromium.org/303503004/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js:191:
* @param {Promise} Promise fullfilled on success.
s/param/return/

https://codereview.chromium.org/303503004/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js:203:
* @param {Promise} Promise fullfilled with result of fakeMouseClick.
s/param/return/

https://codereview.chromium.org/303503004/diff/80001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/file_manager_browsertest/manifest.json
(right):

https://codereview.chromium.org/303503004/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/manifest.json:32:
"folder_shortcuts.js"
Please arrange the script files by alphabetical order.

[fukino, 2014-05-29 04:46:59]

Thank you for quick review!
I'm sorry for too many comment errors...

https://codereview.chromium.org/303503004/diff/80001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js
(right):

https://codereview.chromium.org/303503004/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js:21:
* Entry set which is used for this test.
On 2014/05/29 04:24:44, yoshiki wrote:
> Please add @type annotation.

Done.

https://codereview.chromium.org/303503004/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js:33:
* Constants for each folders.
On 2014/05/29 04:24:44, yoshiki wrote:
> Please add @type annotation.

Done.

https://codereview.chromium.org/303503004/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js:35:
var DIR = {
On 2014/05/29 04:24:44, yoshiki wrote:
> nit: DIRECTORIES

Done.

https://codereview.chromium.org/303503004/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js:74:
* @return {Promise} Promise fulfilled with an array containing twi window IDs.
On 2014/05/29 04:24:44, yoshiki wrote:
> s/twi/two/

Done.

https://codereview.chromium.org/303503004/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js:191:
* @param {Promise} Promise fullfilled on success.
On 2014/05/29 04:24:44, yoshiki wrote:
> s/param/return/

Done.

https://codereview.chromium.org/303503004/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js:203:
* @param {Promise} Promise fullfilled with result of fakeMouseClick.
On 2014/05/29 04:24:44, yoshiki wrote:
> s/param/return/

Done.

https://codereview.chromium.org/303503004/diff/80001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/file_manager_browsertest/manifest.json
(right):

https://codereview.chromium.org/303503004/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/manifest.json:32:
"folder_shortcuts.js"
On 2014/05/29 04:24:44, yoshiki wrote:
> Please arrange the script files by alphabetical order.

Done.

[yoshiki, 2014-05-29 11:03:05]

sorry for late. LTGM with nit

https://codereview.chromium.org/303503004/diff/100001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js
(right):

https://codereview.chromium.org/303503004/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js:153:
'fakeMouseRightClick', windowId, ['.table-row[selected]']);
nit: 4 space indent

https://codereview.chromium.org/303503004/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js:159:
'fakeMouseClick', windowId, ['[command="#create-folder-shortcut"]']);
ditto

https://codereview.chromium.org/303503004/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js:182:
'fakeMouseClick', windowId, ['[command="#remove-folder-shortcut"]']);
nit: 4 space indent

[yoshiki, 2014-05-29 11:07:58]

On 2014/05/29 11:03:05, yoshiki wrote:
> sorry for late. LTGM with nit
> 
>
https://codereview.chromium.org/303503004/diff/100001/chrome/test/data/extens...
> File
>
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js
> (right):
> 
>
https://codereview.chromium.org/303503004/diff/100001/chrome/test/data/extens...
>
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js:153:
> 'fakeMouseRightClick', windowId, ['.table-row[selected]']);
> nit: 4 space indent
> 
>
https://codereview.chromium.org/303503004/diff/100001/chrome/test/data/extens...
>
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js:159:
> 'fakeMouseClick', windowId, ['[command="#create-folder-shortcut"]']);
> ditto
> 
>
https://codereview.chromium.org/303503004/diff/100001/chrome/test/data/extens...
>
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js:182:
> 'fakeMouseClick', windowId, ['[command="#remove-folder-shortcut"]']);
> nit: 4 space indent

s/LTGM/LGTM/

[fukino, 2014-05-29 11:17:14]

Corrected indentation. Thanks!

https://codereview.chromium.org/303503004/diff/100001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js
(right):

https://codereview.chromium.org/303503004/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js:153:
'fakeMouseRightClick', windowId, ['.table-row[selected]']);
On 2014/05/29 11:03:05, yoshiki wrote:
> nit: 4 space indent

Done.

https://codereview.chromium.org/303503004/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js:159:
'fakeMouseClick', windowId, ['[command="#create-folder-shortcut"]']);
On 2014/05/29 11:03:05, yoshiki wrote:
> ditto

Done.

https://codereview.chromium.org/303503004/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_manager_browsertest/folder_shortcuts.js:182:
'fakeMouseClick', windowId, ['[command="#remove-folder-shortcut"]']);
On 2014/05/29 11:03:05, yoshiki wrote:
> nit: 4 space indent

Done.

[fukino, 2014-05-29 11:17:40]

The CQ bit was checked by fukino@chromium.org

[commit-bot: I haz the power, 2014-05-29 11:18:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/fukino@chromium.org/303503004/120001

[commit-bot: I haz the power, 2014-05-29 21:21:02]

Change committed as 273601

[fukino, 2014-05-30 05:18:11]

A revert of this CL has been created in
https://codereview.chromium.org/307053002/ by fukino@chromium.org.

The reason for reverting is: Added browser test broke Linux ChromiumOS Tests
(dbg). This may be flakiness and I should investigate the reason before
relanding.

[fukino, 2014-06-05 11:27:38]

yoshiki-san, could you take a look?

Only test code changed.
I divide the too slow browser test into two tests. Now both of them run in 5 or
6 seconds in release build.
There still is a risk of time out in debug build and these test is basically for
improve coverage of JS code, so I disabled these 2 tests in debug build.

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

https://codereview.chromium.org/303503004/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/file_manager_browsertest.cc:905: #define
WRAPPED_INSTANTIATE_TEST_CASE_P(prefix, test_case_name, generator) \
MAYBE_FolderShortcuts was not expanded because INSTANTIATE_TEST_CASE_P macro
concatenates prefix(MAYBE_FolderShortcuts) and
test_case_name(FileManagerBrowserTest) first.

This macro works around for this.

[yoshiki, 2014-06-05 15:11:23]

lgtm with nits

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

https://codereview.chromium.org/303503004/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/file_manager_browsertest.cc:905: #define
WRAPPED_INSTANTIATE_TEST_CASE_P(prefix, test_case_name, generator) \
On 2014/06/05 11:27:38, fukino wrote:
> MAYBE_FolderShortcuts was not expanded because INSTANTIATE_TEST_CASE_P macro
> concatenates prefix(MAYBE_FolderShortcuts) and
> test_case_name(FileManagerBrowserTest) first.
> 
> This macro works around for this.

nit: Please write it in the comment. The detailed comment will help a code
reader in near feature.

https://codereview.chromium.org/303503004/diff/160001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/file_manager/folder_shortcuts.js
(right):

https://codereview.chromium.org/303503004/diff/160001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_manager_browsertest/file_manager/folder_shortcuts.js:1:
// Copyright (c) 2014 The Chromium Authors. All rights reserved.
nitL we don't need "(c)" in copyright header in new file.

https://codereview.chromium.org/303503004/diff/160001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_manager_browsertest/file_manager/folder_shortcuts.js:83:
openNewWindow(null, RootPath.DRIVE),
nit: 4 space indent.

https://codereview.chromium.org/303503004/diff/160001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_manager_browsertest/file_manager/folder_shortcuts.js:87:
waitForElement(windowIds[0], '#detail-table'),
nit: 4 space indent.

https://codereview.chromium.org/303503004/diff/160001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/ui/navigation_list.js (right):

https://codereview.chromium.org/303503004/diff/160001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/ui/navigation_list.js:306: // Change
directory to the model item's root directory if needed.
nit: Changes

https://codereview.chromium.org/303503004/diff/160001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/ui/navigation_list.js:362: //
Remember the selected model item.
nit: remembers

[fukino, 2014-06-06 02:15:25]

Thank you for comments!

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

https://codereview.chromium.org/303503004/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/file_manager_browsertest.cc:905: #define
WRAPPED_INSTANTIATE_TEST_CASE_P(prefix, test_case_name, generator) \
On 2014/06/05 15:11:23, yoshiki wrote:
> On 2014/06/05 11:27:38, fukino wrote:
> > MAYBE_FolderShortcuts was not expanded because INSTANTIATE_TEST_CASE_P macro
> > concatenates prefix(MAYBE_FolderShortcuts) and
> > test_case_name(FileManagerBrowserTest) first.
> > 
> > This macro works around for this.
> 
> nit: Please write it in the comment. The detailed comment will help a code
> reader in near feature.
> 

I agree! It wasn't good idea to make the comment one line for tricky code like
this.

I copied comment from following code because the explanation is more precise,
detailed, and general than my review comment.
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/cert_veri...

https://codereview.chromium.org/303503004/diff/160001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/file_manager/folder_shortcuts.js
(right):

https://codereview.chromium.org/303503004/diff/160001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_manager_browsertest/file_manager/folder_shortcuts.js:1:
// Copyright (c) 2014 The Chromium Authors. All rights reserved.
On 2014/06/05 15:11:23, yoshiki wrote:
> nitL we don't need "(c)" in copyright header in new file.

Done.

https://codereview.chromium.org/303503004/diff/160001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_manager_browsertest/file_manager/folder_shortcuts.js:83:
openNewWindow(null, RootPath.DRIVE),
On 2014/06/05 15:11:23, yoshiki wrote:
> nit: 4 space indent.

'gjslint --strict' told me that it should be 2 space indent.
It's because these lines are inside an array, I think.

https://codereview.chromium.org/303503004/diff/160001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_manager_browsertest/file_manager/folder_shortcuts.js:87:
waitForElement(windowIds[0], '#detail-table'),
On 2014/06/05 15:11:23, yoshiki wrote:
> nit: 4 space indent.

same as above.

https://codereview.chromium.org/303503004/diff/160001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/ui/navigation_list.js (right):

https://codereview.chromium.org/303503004/diff/160001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/ui/navigation_list.js:306: // Change
directory to the model item's root directory if needed.
On 2014/06/05 15:11:23, yoshiki wrote:
> nit: Changes

Done.

https://codereview.chromium.org/303503004/diff/160001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/ui/navigation_list.js:362: //
Remember the selected model item.
On 2014/06/05 15:11:23, yoshiki wrote:
> nit: remembers

Done.

[yoshiki, 2014-06-06 02:53:10]

Still lgtm

https://codereview.chromium.org/303503004/diff/160001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/file_manager/folder_shortcuts.js
(right):

https://codereview.chromium.org/303503004/diff/160001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_manager_browsertest/file_manager/folder_shortcuts.js:83:
openNewWindow(null, RootPath.DRIVE),
On 2014/06/06 02:15:24, fukino wrote:
> On 2014/06/05 15:11:23, yoshiki wrote:
> > nit: 4 space indent.
> 
> 'gjslint --strict' told me that it should be 2 space indent.
> It's because these lines are inside an array, I think.

Got it.

[fukino, 2014-06-06 04:55:32]

The CQ bit was checked by fukino@chromium.org

[commit-bot: I haz the power, 2014-06-06 04:55:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/fukino@chromium.org/303503004/180001

[commit-bot: I haz the power, 2014-06-06 09:13:08]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[commit-bot: I haz the power, 2014-06-06 09:54:19]

Change committed as 275359