[FileBrowser] Added DefaultAction dialog to choose default action.

[FileBrowser] Added DefaultAction dialog to choose default action. 

BUG=126927
TEST=

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

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

[Dmitry Zvorygin, 2012-05-18 13:00:38]

Please take a look.

[dgozman, 2012-05-18 14:50:06]

http://codereview.chromium.org/10411018/diff/1/chrome/browser/chromeos/extens...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/10411018/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:641:
file_handler_util::UpdateFileHandlerUsageStats(profile_, task_id);
This probably should be called on UI thread?

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
File chrome/browser/resources/file_manager/css/file_manager.css (right):

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/css/file_manager.css:569:
#default-actions-list > * {
You should use 'li' instead of * here and below.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
File chrome/browser/resources/file_manager/js/default_action_dialog.js (right):

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/default_action_dialog.js:7: *
PromptDialog contains a message, a text input, an ok button, and a
Copypasted comment.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/default_action_dialog.js:11: function
DefaultActionDialog(parentNode) {
No JSDoc.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/default_action_dialog.js:12:
cr.ui.dialogs.BaseDialog.apply(this, [parentNode]);
If you have explicit arguments, better use |call|.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/default_action_dialog.js:18:
cr.ui.List.decorate(this.list_);
I believe, you can just |this.list_ = new cr.ui.List()|.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/default_action_dialog.js:22:
this.list_.autoExpands = true;
This really means that dialog is not constrained in height, which is a bad idea.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/default_action_dialog.js:29:
this.list_.itemConstructor = function(item) {
this.list_.itemConstructor = this.renderItem.bind(this)

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/default_action_dialog.js:75:
this.dataModel_.push(tasksList[i]);
Instead of pushing n times, use one splice?

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/default_action_dialog.js:84: return
this.list_.value;
Wow, list has |value| method?

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/default_action_dialog.js:97:
DefaultActionDialog.prototype.onContainerKeyDown_ = function(event) {
Where do you add this listener?

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/default_action_dialog.js:102: } else if
(!this.okButton_.disabled &&
How can it be disabled?

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/file_manager.js:2459: if
(tasksList.length > 1) {
use tasksCount, which is not the same as tasksList.length

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/file_manager.js:2492: } else {
We should have a comment that item without task must be a 'choose default' one.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/file_manager.js:2496: var ext =
/\.(\w+)$/g.exec(this.selection.urls[i])[1].toUpperCase();
There may be no extension.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/file_manager.js:2504:
strf('CHANGE_DEFAULT_CAPTION',
We don't want to show all extensions in the caption.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/file_manager.js:2505:
extensions.join(', ')),
indentation

http://codereview.chromium.org/10411018/diff/1/chrome/common/extensions/api/f...
File chrome/common/extensions/api/file_browser_private.json (right):

http://codereview.chromium.org/10411018/diff/1/chrome/common/extensions/api/f...
chrome/common/extensions/api/file_browser_private.json:392: "description": "Sets
default task for passed patterns, to specified task.",
Wrong description. No patterns are passed.

http://codereview.chromium.org/10411018/diff/1/chrome/common/extensions/api/f...
chrome/common/extensions/api/file_browser_private.json:397: "description": "The
unique identifier of task to save for this patterns."
Wrong description.

http://codereview.chromium.org/10411018/diff/1/chrome/common/extensions/api/f...
chrome/common/extensions/api/file_browser_private.json:408: "description": "True
of default task successfully assigned to passed patterns."
Wrong description. Typo: of -> if.

[Dmitry Zvorygin, 2012-05-22 14:32:59]

Fixed comments. PTAL.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/chromeos/extens...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/10411018/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:641:
file_handler_util::UpdateFileHandlerUsageStats(profile_, task_id);
On 2012/05/18 14:50:06, dgozman wrote:
> This probably should be called on UI thread?

Done.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
File chrome/browser/resources/file_manager/css/file_manager.css (right):

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/css/file_manager.css:569:
#default-actions-list > * {
On 2012/05/18 14:50:06, dgozman wrote:
> You should use 'li' instead of * here and below.

Done.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
File chrome/browser/resources/file_manager/js/default_action_dialog.js (right):

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/default_action_dialog.js:7: *
PromptDialog contains a message, a text input, an ok button, and a
On 2012/05/18 14:50:06, dgozman wrote:
> Copypasted comment.

Done.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/default_action_dialog.js:11: function
DefaultActionDialog(parentNode) {
On 2012/05/18 14:50:06, dgozman wrote:
> No JSDoc.

Done.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/default_action_dialog.js:12:
cr.ui.dialogs.BaseDialog.apply(this, [parentNode]);
On 2012/05/18 14:50:06, dgozman wrote:
> If you have explicit arguments, better use |call|.

Done.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/default_action_dialog.js:18:
cr.ui.List.decorate(this.list_);
On 2012/05/18 14:50:06, dgozman wrote:
> I believe, you can just |this.list_ = new cr.ui.List()|.

Done.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/default_action_dialog.js:22:
this.list_.autoExpands = true;
On 2012/05/18 14:50:06, dgozman wrote:
> This really means that dialog is not constrained in height, which is a bad
idea.

Added comment.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/default_action_dialog.js:29:
this.list_.itemConstructor = function(item) {
On 2012/05/18 14:50:06, dgozman wrote:
> this.list_.itemConstructor = this.renderItem.bind(this)

Binding stuff doesn't work with constructors. See
file_manager.js::itemConstructor hacks.

Added comment.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/default_action_dialog.js:75:
this.dataModel_.push(tasksList[i]);
On 2012/05/18 14:50:06, dgozman wrote:
> Instead of pushing n times, use one splice?

Splice requires items passed as parameters, not as an array.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/default_action_dialog.js:84: return
this.list_.value;
On 2012/05/18 14:50:06, dgozman wrote:
> Wow, list has |value| method?

Done.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/default_action_dialog.js:97:
DefaultActionDialog.prototype.onContainerKeyDown_ = function(event) {
On 2012/05/18 14:50:06, dgozman wrote:
> Where do you add this listener?
Added comment.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/default_action_dialog.js:102: } else if
(!this.okButton_.disabled &&
On 2012/05/18 14:50:06, dgozman wrote:
> How can it be disabled?

Done.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/file_manager.js:2459: if
(tasksList.length > 1) {
On 2012/05/18 14:50:06, dgozman wrote:
> use tasksCount, which is not the same as tasksList.length

Done.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/file_manager.js:2492: } else {
On 2012/05/18 14:50:06, dgozman wrote:
> We should have a comment that item without task must be a 'choose default'
one.

Done.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/file_manager.js:2496: var ext =
/\.(\w+)$/g.exec(this.selection.urls[i])[1].toUpperCase();
On 2012/05/18 14:50:06, dgozman wrote:
> There may be no extension.

Done.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/file_manager.js:2504:
strf('CHANGE_DEFAULT_CAPTION',
On 2012/05/18 14:50:06, dgozman wrote:
> We don't want to show all extensions in the caption.

Done.

http://codereview.chromium.org/10411018/diff/1/chrome/browser/resources/file_...
chrome/browser/resources/file_manager/js/file_manager.js:2505:
extensions.join(', ')),
On 2012/05/18 14:50:06, dgozman wrote:
> indentation

Done.

http://codereview.chromium.org/10411018/diff/1/chrome/common/extensions/api/f...
File chrome/common/extensions/api/file_browser_private.json (right):

http://codereview.chromium.org/10411018/diff/1/chrome/common/extensions/api/f...
chrome/common/extensions/api/file_browser_private.json:392: "description": "Sets
default task for passed patterns, to specified task.",
On 2012/05/18 14:50:06, dgozman wrote:
> Wrong description. No patterns are passed.

Done.

http://codereview.chromium.org/10411018/diff/1/chrome/common/extensions/api/f...
chrome/common/extensions/api/file_browser_private.json:397: "description": "The
unique identifier of task to save for this patterns."
On 2012/05/18 14:50:06, dgozman wrote:
> Wrong description.

Done.

http://codereview.chromium.org/10411018/diff/1/chrome/common/extensions/api/f...
chrome/common/extensions/api/file_browser_private.json:408: "description": "True
of default task successfully assigned to passed patterns."
On 2012/05/18 14:50:06, dgozman wrote:
> Wrong description. Typo: of -> if.

Done.

[dgozman, 2012-05-22 14:55:27]

http://codereview.chromium.org/10411018/diff/7001/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/10411018/diff/7001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:646:
BrowserThread::FILE, FROM_HERE,
FILE -> UI

http://codereview.chromium.org/10411018/diff/7001/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/js/default_action_dialog.js (right):

http://codereview.chromium.org/10411018/diff/7001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/default_action_dialog.js:7: *
DefaultActionDialog contains a message, a list box, an ok button, and a
Better describe what is this dialog for, not the elements.

http://codereview.chromium.org/10411018/diff/7001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/default_action_dialog.js:75:
DefaultActionDialog.prototype.show = function(message, items, defaultIdx,
idx -> index

http://codereview.chromium.org/10411018/diff/7001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/default_action_dialog.js:104: //
Overrides BaseDialog::onContainerKeyDown_;
Call the basic implementation somewhere?

http://codereview.chromium.org/10411018/diff/12002/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_private_api.h (right):

http://codereview.chromium.org/10411018/diff/12002/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.h:117: class
SetDefaultTaskFileBrowserFunction : public AsyncExtensionFunction {
This function is actually sync one.

[Dmitry Zvorygin, 2012-05-22 15:11:10]

Fixed.

http://codereview.chromium.org/10411018/diff/7001/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/10411018/diff/7001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:646:
BrowserThread::FILE, FROM_HERE,
On 2012/05/22 14:55:27, dgozman wrote:
> FILE -> UI

Done.

http://codereview.chromium.org/10411018/diff/7001/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/js/default_action_dialog.js (right):

http://codereview.chromium.org/10411018/diff/7001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/default_action_dialog.js:7: *
DefaultActionDialog contains a message, a list box, an ok button, and a
On 2012/05/22 14:55:27, dgozman wrote:
> Better describe what is this dialog for, not the elements.

Done.

http://codereview.chromium.org/10411018/diff/7001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/default_action_dialog.js:75:
DefaultActionDialog.prototype.show = function(message, items, defaultIdx,
On 2012/05/22 14:55:27, dgozman wrote:
> idx -> index

Done.

http://codereview.chromium.org/10411018/diff/7001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/default_action_dialog.js:104: //
Overrides BaseDialog::onContainerKeyDown_;
On 2012/05/22 14:55:27, dgozman wrote:
> Call the basic implementation somewhere?

Not needed.

[dgozman, 2012-05-22 15:12:30]

LGTM

[asargent_no_longer_on_chrome, 2012-05-22 16:46:23]

LGTM

http://codereview.chromium.org/10411018/diff/9013/chrome/common/extensions/ap...
File chrome/common/extensions/api/file_browser_private.json (right):

http://codereview.chromium.org/10411018/diff/9013/chrome/common/extensions/ap...
chrome/common/extensions/api/file_browser_private.json:407: "optional": true,
nit: I'm not sure of your use cases, but it seems like you'd want this boolean
to be non-optional, since making the task default either succeeded or it didn't.
"Do or do not, there is no try" as Yoda says.

Also, if you wanted you could forgo having the explicit boolean for success
altogether, and instead just set chrome.extension.lastError (by returning false
instead of true from your ExtensionFunction subclass in the browser process) if
the call failed for some reason.

[commit-bot: I haz the power, 2012-05-23 13:35:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zvorygin@chromium.org/10411018/17001