Show generic file handler more modestly.

Show generic file handler more modestly.

- Do not show generic file handler (file handlers which can take any type of file such as types:["*"]) as a default task.
- All file handlers are shown in context menu and menu of the combobottuon.

BUG=414566
TEST=out/Release/browser_tests --gtest_filter=GenericTask/FileManagerBrowserTest.Test/*

Committed: https://crrev.com/0b49eba721c71610781a80d243184dbf9a273aea
Cr-Commit-Position: refs/heads/master@{#302777}

[yawano, 2014-11-05 00:42:28]

yawano@chromium.org changed reviewers:
+ mtomasz@chromium.org

[yawano, 2014-11-05 00:42:29]

@mtomasz PTAL. Thanks!

[mtomasz, 2014-11-05 01:11:12]

https://codereview.chromium.org/700023002/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/file_manager/tasks.js
(right):

https://codereview.chromium.org/700023002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/file_manager/tasks.js:13:
* @param {boolean=} opt_isGenericFileHandler Whether the task is generic file
nit: generic -> a generic

https://codereview.chromium.org/700023002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/file_manager/tasks.js:233:
return executeDefaultTask(runtimeId + '|file|view-in-browser',
Could you explain briefly what this test actually does? Why are we trying to
execute 'id|file|view-in-browser| here?

https://codereview.chromium.org/700023002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/test_util.js (right):

https://codereview.chromium.org/700023002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/test_util.js:748:
test.util.sync.getChromeRuntimeId = function() {
This seems to be an extension id. How about renaming to simply getExtensionId?
On the other hand extension id of Files app is already known in tests, so maybe
we don't need it?

See:
FILE_MANAGER_EXTENSIONS_ID

https://codereview.chromium.org/700023002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/test_util.js:750: }
nit: ;.

https://codereview.chromium.org/700023002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/file_manager.js (right):

https://codereview.chromium.org/700023002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:3678: if
(items.length === 1) {
Will it show a generic task as a default item here if it's the only one item?
What does 'default' mean here? Will it execute after a double click?

https://codereview.chromium.org/700023002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/file_tasks.js (right):

https://codereview.chromium.org/700023002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_tasks.js:283: for(var i = 0; i <
this.tasks_.length; i++) {
nit: for\s(

https://codereview.chromium.org/700023002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_tasks.js:621:
if(this.defaultTask_) {
nit: if\s

https://codereview.chromium.org/700023002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_tasks.js:742: if
(hideGenericFileHandler) {
nit optional: {} not necessary here, since one line body.

[yawano, 2014-11-05 01:50:53]

Thank you for the review. PTAL.

https://codereview.chromium.org/700023002/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/file_manager/tasks.js
(right):

https://codereview.chromium.org/700023002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/file_manager/tasks.js:13:
* @param {boolean=} opt_isGenericFileHandler Whether the task is generic file
On 2014/11/05 01:11:12, mtomasz wrote:
> nit: generic -> a generic

Done.

https://codereview.chromium.org/700023002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/file_manager/tasks.js:233:
return executeDefaultTask(runtimeId + '|file|view-in-browser',
On 2014/11/05 01:11:12, mtomasz wrote:
> Could you explain briefly what this test actually does? Why are we trying to
> execute 'id|file|view-in-browser| here?

Done.

https://codereview.chromium.org/700023002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/test_util.js (right):

https://codereview.chromium.org/700023002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/test_util.js:748:
test.util.sync.getChromeRuntimeId = function() {
On 2014/11/05 01:11:12, mtomasz wrote:
> This seems to be an extension id. How about renaming to simply getExtensionId?
> On the other hand extension id of Files app is already known in tests, so
maybe
> we don't need it?
> 
> See:
> FILE_MANAGER_EXTENSIONS_ID

Done. I removed this method, and changed to use FILE_MANAGER_EXTENSIONS_ID.

https://codereview.chromium.org/700023002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/test_util.js:750: }
On 2014/11/05 01:11:12, mtomasz wrote:
> nit: ;.

Acknowledged.

https://codereview.chromium.org/700023002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/file_manager.js (right):

https://codereview.chromium.org/700023002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:3678: if
(items.length === 1) {
On 2014/11/05 01:11:12, mtomasz wrote:
> Will it show a generic task as a default item here if it's the only one item?
> What does 'default' mean here? Will it execute after a double click?

Yes, if we have only one generic task, we show it as a "defaultItem". "default"
here have different meaning from "default" in "defaultTask". "default" here
means that it is a default action item "in the context menu". When user double
clicks the item, while "defaultTask" is executed, "defaultItem" is not executed.

https://codereview.chromium.org/700023002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/file_tasks.js (right):

https://codereview.chromium.org/700023002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_tasks.js:283: for(var i = 0; i <
this.tasks_.length; i++) {
On 2014/11/05 01:11:12, mtomasz wrote:
> nit: for\s(

Done.

https://codereview.chromium.org/700023002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_tasks.js:621:
if(this.defaultTask_) {
On 2014/11/05 01:11:12, mtomasz wrote:
> nit: if\s

Done.

https://codereview.chromium.org/700023002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_tasks.js:742: if
(hideGenericFileHandler) {
On 2014/11/05 01:11:12, mtomasz wrote:
> nit optional: {} not necessary here, since one line body.

Done.

[mtomasz, 2014-11-05 01:59:34]

https://codereview.chromium.org/700023002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/file_manager.js (right):

https://codereview.chromium.org/700023002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:3678: if
(items.length === 1) {
On 2014/11/05 01:50:53, yawano wrote:
> On 2014/11/05 01:11:12, mtomasz wrote:
> > Will it show a generic task as a default item here if it's the only one
item?
> > What does 'default' mean here? Will it execute after a double click?
> 
> Yes, if we have only one generic task, we show it as a "defaultItem".
"default"
> here have different meaning from "default" in "defaultTask". "default" here
> means that it is a default action item "in the context menu". When user double
> clicks the item, while "defaultTask" is executed, "defaultItem" is not
executed.

Can we rename defaultItem then? It's quite confusing. How about eg. "mainItem"?

[yawano, 2014-11-05 03:20:48]

Renamed defaultItem to actionItem. I found a bug that the previous patch didn't
handle activate event of actionMenuItem properly. I fixed it with this patch.
PTAL. Thanks.

https://codereview.chromium.org/700023002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/file_manager.js (right):

https://codereview.chromium.org/700023002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:3678: if
(items.length === 1) {
On 2014/11/05 01:59:34, mtomasz wrote:
> On 2014/11/05 01:50:53, yawano wrote:
> > On 2014/11/05 01:11:12, mtomasz wrote:
> > > Will it show a generic task as a default item here if it's the only one
> item?
> > > What does 'default' mean here? Will it execute after a double click?
> > 
> > Yes, if we have only one generic task, we show it as a "defaultItem".
> "default"
> > here have different meaning from "default" in "defaultTask". "default" here
> > means that it is a default action item "in the context menu". When user
double
> > clicks the item, while "defaultTask" is executed, "defaultItem" is not
> executed.
> 
> Can we rename defaultItem then? It's quite confusing. How about eg.
"mainItem"?

Done.

[mtomasz, 2014-11-05 08:55:13]

https://codereview.chromium.org/700023002/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/file_manager.js (right):

https://codereview.chromium.org/700023002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/file_manager.js:2541: * @private
nit: Please add some short description to the method.

https://codereview.chromium.org/700023002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/file_manager.js:2547: }
nit: ; missing

https://codereview.chromium.org/700023002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/file_manager.js:3692:
defaultItem.iconType);
nit: Indent is wrong.
nit: defaultItem -> actionItem

[mtomasz, 2014-11-05 08:55:23]

lgtm with nits

[yawano, 2014-11-05 09:12:06]

Thank you for the review!

https://codereview.chromium.org/700023002/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/file_manager.js (right):

https://codereview.chromium.org/700023002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/file_manager.js:2541: * @private
On 2014/11/05 08:55:13, mtomasz wrote:
> nit: Please add some short description to the method.

Done.

https://codereview.chromium.org/700023002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/file_manager.js:2547: }
On 2014/11/05 08:55:13, mtomasz wrote:
> nit: ; missing

Done.

https://codereview.chromium.org/700023002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/file_manager.js:3692:
defaultItem.iconType);
On 2014/11/05 08:55:13, mtomasz wrote:
> nit: Indent is wrong.
> nit: defaultItem -> actionItem

Done.

[yawano, 2014-11-05 09:12:26]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2014-11-05 09:13:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/700023002/60001

[commit-bot: I haz the power, 2014-11-05 11:07:51]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-05 11:08:34]

Patchset 4 (id:??) landed as
https://crrev.com/0b49eba721c71610781a80d243184dbf9a273aea
Cr-Commit-Position: refs/heads/master@{#302777}