Add verbs API to file handlers. Modify the Chrome OS UI so that it displayes the internationalized …

Add verbs API to file handlers. Modify the Chrome OS UI so that it displayes the internationalized verb together with the extension name in the file manager.

BUG=578725
R=fukino@chromium.org

Committed: https://crrev.com/03a4c08c513f71d7bf2592bec43a33bfbf9290e9
Cr-Commit-Position: refs/heads/master@{#395782}

[fukino, 2016-04-11 09:19:43]

Thank you!

https://codereview.chromium.org/1872223002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/private_api_tasks.cc
(right):

https://codereview.chromium.org/1872223002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/private_api_tasks.cc:202:
converted.title = base::UTF16ToUTF8(task.task_title());
Just a question: Why should be convert task title from UTF8 to UTF16?
(I'm concerned about that it has been broken, or we need to do so from this
patch?)

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

https://codereview.chromium.org/1872223002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/file_tasks.cc:513: // TODO(cmihail): Should
we support verbs also for FileBrowserHandler?
I think FileBrowserHandler already has the similar functionality as verbs API.
(An app can provide multiple actions with free titles)

https://codereview.chromium.org/1872223002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/file_tasks.cc:515: handler->title(),
extensions::file_handler_verbs::kOpenWith),
We should not assume "Open with..." for FileBrowserHandler.
handler->title() can already have verb, like the example in
https://developer.chrome.com/extensions/fileBrowserHandler

https://codereview.chromium.org/1872223002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/file_tasks.h (right):

https://codereview.chromium.org/1872223002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/file_tasks.h:174: ~FullTaskDescriptor();
Unnecessary change?

https://codereview.chromium.org/1872223002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/file_tasks.js (right):

https://codereview.chromium.org/1872223002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/file_tasks.js:843: * @param
{Array<!Object>} tasks The list of tasks from where to choose the
{!Array<!Object>}

https://codereview.chromium.org/1872223002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/file_tasks.js:847: * @return
opt_taskToUseIfNoDefault or null in case opt_taskToUseIfNoDefault is
@return {Object} opt_task....

[cmihail, 2016-04-13 07:36:46]

https://codereview.chromium.org/1872223002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/private_api_tasks.cc
(right):

https://codereview.chromium.org/1872223002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/private_api_tasks.cc:202:
converted.title = base::UTF16ToUTF8(task.task_title());
On 2016/04/11 09:19:43, fukino wrote:
> Just a question: Why should be convert task title from UTF8 to UTF16?
> (I'm concerned about that it has been broken, or we need to do so from this
> patch?)

Unfortunately IDL doesn't allow me to pass a UTF16 string to JS, or at least I
don't know how to do it. This is what I left the TODO for.

The idea is that for simplicity I did the internationalization of the verbs in
C++ code (in the JS is a bit more complicated), but I can switch to that one if
better. Or use a proper technique to pass the C++ internationalization to JS if
possible, though unsure what is the correct way.

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

https://codereview.chromium.org/1872223002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/file_tasks.cc:513: // TODO(cmihail): Should
we support verbs also for FileBrowserHandler?
On 2016/04/11 09:19:43, fukino wrote:
> I think FileBrowserHandler already has the similar functionality as verbs API.
> (An app can provide multiple actions with free titles) 

Acknowledged.

https://codereview.chromium.org/1872223002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/file_tasks.h (right):

https://codereview.chromium.org/1872223002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/file_tasks.h:174: ~FullTaskDescriptor();
On 2016/04/11 09:19:43, fukino wrote:
> Unnecessary change?

Sorry, I forgot to remove it after adding and removing a field in
FullTaskDescriptor.
If having too many members the C++ compiler complains about the default inlined
constructor.

https://codereview.chromium.org/1872223002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/file_tasks.js (right):

https://codereview.chromium.org/1872223002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/file_tasks.js:843: * @param
{Array<!Object>} tasks The list of tasks from where to choose the
On 2016/04/11 09:19:43, fukino wrote:
> {!Array<!Object>}

Done.

https://codereview.chromium.org/1872223002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/file_tasks.js:847: * @return
opt_taskToUseIfNoDefault or null in case opt_taskToUseIfNoDefault is
On 2016/04/11 09:19:43, fukino wrote:
> @return {Object} opt_task....

Done.

[fukino, 2016-04-13 08:03:12]

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

[fukino, 2016-04-13 08:03:12]

https://codereview.chromium.org/1872223002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/private_api_tasks.cc
(right):

https://codereview.chromium.org/1872223002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/private_api_tasks.cc:202:
converted.title = base::UTF16ToUTF8(task.task_title());
On 2016/04/13 07:36:46, cmihail wrote:
> On 2016/04/11 09:19:43, fukino wrote:
> > Just a question: Why should be convert task title from UTF8 to UTF16?
> > (I'm concerned about that it has been broken, or we need to do so from this
> > patch?)
> 
> Unfortunately IDL doesn't allow me to pass a UTF16 string to JS, or at least I
> don't know how to do it. This is what I left the TODO for.
> 
> The idea is that for simplicity I did the internationalization of the verbs in
> C++ code (in the JS is a bit more complicated), but I can switch to that one
if
> better. Or use a proper technique to pass the C++ internationalization to JS
if
> possible, though unsure what is the correct way.

Sorry! I'm not familiar with internationalization in C++ side.
+mtomasz@chromium.org, is there a standard way to do the internationalization in
C++ side?

https://codereview.chromium.org/1872223002/diff/40001/extensions/common/manif...
File extensions/common/manifest_constants.cc (right):

https://codereview.chromium.org/1872223002/diff/40001/extensions/common/manif...
extensions/common/manifest_constants.cc:411: "Invalid value for 'verb'.
Supported values are OPEN_WITH, ADD_TO and "
It will be tough to maintain the list [OPEN_WITH,ADD_TO,PACK_WITH] here
considering future expansions.
How about "Invalid value for 'file_handlers[*].verb'."?

[mtomasz, 2016-04-13 09:17:07]

https://codereview.chromium.org/1872223002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/private_api_tasks.cc
(right):

https://codereview.chromium.org/1872223002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/private_api_tasks.cc:202:
converted.title = base::UTF16ToUTF8(task.task_title());
On 2016/04/13 08:03:12, fukino wrote:
> On 2016/04/13 07:36:46, cmihail wrote:
> > On 2016/04/11 09:19:43, fukino wrote:
> > > Just a question: Why should be convert task title from UTF8 to UTF16?
> > > (I'm concerned about that it has been broken, or we need to do so from
this
> > > patch?)
> > 
> > Unfortunately IDL doesn't allow me to pass a UTF16 string to JS, or at least
I
> > don't know how to do it. This is what I left the TODO for.
> > 
> > The idea is that for simplicity I did the internationalization of the verbs
in
> > C++ code (in the JS is a bit more complicated), but I can switch to that one
> if
> > better. Or use a proper technique to pass the C++ internationalization to JS
> if
> > possible, though unsure what is the correct way.
> 
> Sorry! I'm not familiar with internationalization in C++ side.
> mailto:+mtomasz@chromium.org, is there a standard way to do the
internationalization in
> C++ side?

I think internationalization in C++ is OK.

https://codereview.chromium.org/1872223002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/private_api_tasks.cc
(right):

https://codereview.chromium.org/1872223002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/private_api_tasks.cc:201: //
TODO(cmihail): Does this conversion really work?
Please confirm before committing :)

https://codereview.chromium.org/1872223002/diff/40001/extensions/common/manif...
File extensions/common/manifest_handlers/file_handler_info.cc (right):

https://codereview.chromium.org/1872223002/diff/40001/extensions/common/manif...
extensions/common/manifest_handlers/file_handler_info.cc:215: base::string16
GetTitleForExtensionAndVerb(const std::string& extension_name,
I'm OK to translate in C++, but I'm not sure if this is the proper place for
this code. Title is not part of file handler manifest section. Maybe we can move
to file_tasks.cc? Or moving it entirely to JavaScript may be simpler. Up to you.

https://codereview.chromium.org/1872223002/diff/40001/extensions/common/manif...
extensions/common/manifest_handlers/file_handler_info.cc:226: DCHECK(false);
nit: DCHECK(false) -> NOTREACHED().

[cmihail, 2016-04-18 01:28:10]

https://codereview.chromium.org/1872223002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/private_api_tasks.cc
(right):

https://codereview.chromium.org/1872223002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/private_api_tasks.cc:201: //
TODO(cmihail): Does this conversion really work?
On 2016/04/13 09:17:06, mtomasz wrote:
> Please confirm before committing :)

N/A as internationalization moved to JS code.

https://codereview.chromium.org/1872223002/diff/40001/extensions/common/manif...
File extensions/common/manifest_constants.cc (right):

https://codereview.chromium.org/1872223002/diff/40001/extensions/common/manif...
extensions/common/manifest_constants.cc:411: "Invalid value for 'verb'.
Supported values are OPEN_WITH, ADD_TO and "
On 2016/04/13 08:03:12, fukino wrote:
> It will be tough to maintain the list [OPEN_WITH,ADD_TO,PACK_WITH] here
> considering future expansions.
> How about "Invalid value for 'file_handlers[*].verb'."?

Done.

https://codereview.chromium.org/1872223002/diff/40001/extensions/common/manif...
File extensions/common/manifest_handlers/file_handler_info.cc (right):

https://codereview.chromium.org/1872223002/diff/40001/extensions/common/manif...
extensions/common/manifest_handlers/file_handler_info.cc:215: base::string16
GetTitleForExtensionAndVerb(const std::string& extension_name,
On 2016/04/13 09:17:06, mtomasz wrote:
> I'm OK to translate in C++, but I'm not sure if this is the proper place for
> this code. Title is not part of file handler manifest section. Maybe we can
move
> to file_tasks.cc? Or moving it entirely to JavaScript may be simpler. Up to
you.

The reason I kept it here was due to make it easier for future extension as both
IsSupportedVerb and it's usage were in the same place. Otherwise I don't see any
reason to keep it in C++. I wrote the JS code for comparison.

https://codereview.chromium.org/1872223002/diff/40001/extensions/common/manif...
extensions/common/manifest_handlers/file_handler_info.cc:226: DCHECK(false);
On 2016/04/13 09:17:06, mtomasz wrote:
> nit: DCHECK(false) -> NOTREACHED().

ACK. N/A as moved this logic to JS.

[cmihail, 2016-04-18 01:30:46]

https://codereview.chromium.org/1872223002/diff/80001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/file_tasks.js (right):

https://codereview.chromium.org/1872223002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/file_tasks.js:357: } else if
(taskParts[2] === 'view-pdf') {
Should we change this so it has also a verb?
I.E. For PDF we show "View" now, but we could show "Open with Viewer". This is
how I mentioned in the API, but I see that we use TASK_VIEW also for 'generic',
and I have no idea how to handle that case.

WDYT?

[fukino, 2016-04-18 08:06:06]

https://codereview.chromium.org/1872223002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/private_api_tasks.cc
(right):

https://codereview.chromium.org/1872223002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/private_api_tasks.cc:13:
#include "base/strings/utf_string_conversions.h"
Not necessary

https://codereview.chromium.org/1872223002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/file_tasks.cc (right):

https://codereview.chromium.org/1872223002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/file_tasks.cc:153:
FullTaskDescriptor::~FullTaskDescriptor() {}
Looks unnecessary :)

https://codereview.chromium.org/1872223002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/file_tasks.h (right):

https://codereview.chromium.org/1872223002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/file_tasks.h:176: ~FullTaskDescriptor();
not necessary

https://codereview.chromium.org/1872223002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/file_tasks_unittest.cc (right):

https://codereview.chromium.org/1872223002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/file_tasks_unittest.cc:12: #include
"base/strings/utf_string_conversions.h"
looks unnecessary

https://codereview.chromium.org/1872223002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/file_tasks_unittest.cc:995:
EXPECT_EQ("Foo", txt_result[0].task_title());
Is this an intended modification?

https://codereview.chromium.org/1872223002/diff/80001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/file_tasks.js (right):

https://codereview.chromium.org/1872223002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/file_tasks.js:357: } else if
(taskParts[2] === 'view-pdf') {
On 2016/04/18 01:30:46, cmihail wrote:
> Should we change this so it has also a verb?
> I.E. For PDF we show "View" now, but we could show "Open with Viewer". This is
> how I mentioned in the API, but I see that we use TASK_VIEW also for
'generic',
> and I have no idea how to handle that case.
> 
> WDYT?

Let's keep the current labels if the task.verb is not specified for now.

https://codereview.chromium.org/1872223002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/file_tasks.js:374: task.title =
loadTimeData.getString(
You can use getStringF or strf()
https://code.google.com/p/chromium/codesearch#chromium/src/ui/file_manager/fi...

https://codereview.chromium.org/1872223002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/file_tasks.js:805: bold: opt_bold ||
false,
wrap this with parenthesis, to be consistent with the value of 'label'

https://codereview.chromium.org/1872223002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/file_tasks.js:806: isDefault:
opt_isDefault || false,
ditto

https://codereview.chromium.org/1872223002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/file_tasks.js:870: return
!!opt_taskToUseIfNoDefault ? opt_taskToUseIfNoDefault : null;
nit: You don't need to convert 'opt_taskToUseIfNoDefault' to boolean.

[cmihail, 2016-04-21 09:36:44]

https://codereview.chromium.org/1872223002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/private_api_tasks.cc
(right):

https://codereview.chromium.org/1872223002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/private_api_tasks.cc:13:
#include "base/strings/utf_string_conversions.h"
On 2016/04/18 08:06:05, fukino wrote:
> Not necessary

Done.

https://codereview.chromium.org/1872223002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/file_tasks.cc (right):

https://codereview.chromium.org/1872223002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/file_tasks.cc:153:
FullTaskDescriptor::~FullTaskDescriptor() {}
On 2016/04/18 08:06:05, fukino wrote:
> Looks unnecessary :)

See header comment.

https://codereview.chromium.org/1872223002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/file_tasks.h (right):

https://codereview.chromium.org/1872223002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/file_tasks.h:176: ~FullTaskDescriptor();
On 2016/04/18 08:06:05, fukino wrote:
> not necessary

Actually necessary due to the extra task_verb field.
I get the next compiler error:
"Complex class/struct needs an explicit out-of-line destructor."

https://codereview.chromium.org/1872223002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/file_tasks_unittest.cc (right):

https://codereview.chromium.org/1872223002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/file_tasks_unittest.cc:12: #include
"base/strings/utf_string_conversions.h"
On 2016/04/18 08:06:05, fukino wrote:
> looks unnecessary

Done.

https://codereview.chromium.org/1872223002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/file_tasks_unittest.cc:995:
EXPECT_EQ("Foo", txt_result[0].task_title());
On 2016/04/18 08:06:06, fukino wrote:
> Is this an intended modification?

Thx for the catch, didn't observe it.

https://codereview.chromium.org/1872223002/diff/80001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/file_tasks.js (right):

https://codereview.chromium.org/1872223002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/file_tasks.js:357: } else if
(taskParts[2] === 'view-pdf') {
On 2016/04/18 08:06:06, fukino wrote:
> On 2016/04/18 01:30:46, cmihail wrote:
> > Should we change this so it has also a verb?
> > I.E. For PDF we show "View" now, but we could show "Open with Viewer". This
is
> > how I mentioned in the API, but I see that we use TASK_VIEW also for
> 'generic',
> > and I have no idea how to handle that case.
> > 
> > WDYT?
> 
> Let's keep the current labels if the task.verb is not specified for now.

Acknowledged.

https://codereview.chromium.org/1872223002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/file_tasks.js:374: task.title =
loadTimeData.getString(
On 2016/04/18 08:06:06, fukino wrote:
> You can use getStringF or strf()
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/file_manager/fi...

Done.

https://codereview.chromium.org/1872223002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/file_tasks.js:805: bold: opt_bold ||
false,
On 2016/04/18 08:06:06, fukino wrote:
> wrap this with parenthesis, to be consistent with the value of 'label'

Actually I removed "()" from label as I added them during a time when I was
figuring out when file_tasks.js works.

https://codereview.chromium.org/1872223002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/file_tasks.js:806: isDefault:
opt_isDefault || false,
On 2016/04/18 08:06:06, fukino wrote:
> ditto

Acknowledged.

https://codereview.chromium.org/1872223002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/file_tasks.js:870: return
!!opt_taskToUseIfNoDefault ? opt_taskToUseIfNoDefault : null;
On 2016/04/18 08:06:06, fukino wrote:
> nit: You don't need to convert 'opt_taskToUseIfNoDefault' to boolean.

Done.

[fukino, 2016-04-25 04:26:16]

LGTM. Thank you!

Please find an reviewer for extensions, and please check the closure compilation
before submitting (if you haven't) by following command in src/

GYP_GENERATORS=ninja tools/gyp/gyp --depth .
ui/file_manager/file_manager/foreground/js/compiled_resources.gyp && ninja -C
out/Default

[cmihail, 2016-05-08 22:52:37]

Description was changed from

==========
Add verbs API to file handlers. Modify the Chrome OS UI so that it displayes the
internationalized verb together with the extension name in the file manager.

BUG=578725
R=fukino@chromium.org
==========

to

==========
Add verbs API to file handlers. Modify the Chrome OS UI so that it displayes the
internationalized verb together with the extension name in the file manager.

BUG=578725
R=fukino@chromium.org
==========

[cmihail, 2016-05-08 22:52:37]

cmihail@chromium.org changed reviewers:
+ finnur@chromium.org

[cmihail, 2016-05-08 22:53:06]

On 2016/04/25 04:26:16, fukino (OOO til May 8) wrote:
> LGTM. Thank you!
> 
> Please find an reviewer for extensions, and please check the closure
compilation
> before submitting (if you haven't) by following command in src/
> 
> GYP_GENERATORS=ninja tools/gyp/gyp --depth .
> ui/file_manager/file_manager/foreground/js/compiled_resources.gyp && ninja -C
> out/Default

+ finnur@ for review according to git cl owners.

[Finnur, 2016-05-09 09:38:16]

finnur@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org
- finnur@chromium.org

[Finnur, 2016-05-09 09:38:49]

This is probably more appropriate for Devlin to approve. I haven't done any
extensions work in a long time.

[Devlin, 2016-05-09 14:28:41]

I've added a couple questions to the doc (that seemed like a better place to ask
them).  Once those are answered, I'll circle back here.

[cmihail, 2016-05-16 07:50:09]

On 2016/05/09 14:28:41, Devlin wrote:
> I've added a couple questions to the doc (that seemed like a better place to
ask
> them).  Once those are answered, I'll circle back here.

Hey,

Sorry for the long reply time, forgot to sent an email.
I've removed the uppercase requirement of verbs title from the manifest file.
PTAL

[Devlin, 2016-05-16 16:17:32]

https://codereview.chromium.org/1872223002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/file_tasks.cc (right):

https://codereview.chromium.org/1872223002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/file_tasks.cc:186: task_verb_(task_verb),
Should we DCHECK that task_verb_ is a valid verb?

Or, looking at this more, maybe we should introduce an enum on the
fileManagerPrivate API (which would give us compile errors)?

https://codereview.chromium.org/1872223002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/file_tasks.cc:495: for (const auto handler
: file_handlers) {
nit: I personally think auto here reduces readability (partially because it
makes me have to think "Does this need to be const auto&?")

https://codereview.chromium.org/1872223002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/file_tasks.cc:506: for (const auto entry :
handlers_for_entries) {
this results in a copy - please make it const auto&

https://codereview.chromium.org/1872223002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/file_tasks_unittest.cc (right):

https://codereview.chromium.org/1872223002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/file_tasks_unittest.cc:68: "task title",
"OPEN_WITH", GURL("http://example.com/icon.png"),
OPEN_WITH isn't a valid verb anymore (the suggestion in the previous file would
catch this).

https://codereview.chromium.org/1872223002/diff/120001/extensions/common/mani...
File extensions/common/manifest_handlers/file_handler_info.cc (right):

https://codereview.chromium.org/1872223002/diff/120001/extensions/common/mani...
extensions/common/manifest_handlers/file_handler_info.cc:26: const char
kOpenWith[] = "Open with";
manifest constants are almost always all_lowercase_with_underscores.  Let's try
to be consistent (even though we're not always).

https://codereview.chromium.org/1872223002/diff/120001/extensions/common/mani...
extensions/common/manifest_handlers/file_handler_info.cc:42: return
supported_verbs.find(verb) != supported_verbs.end();
This results in the construction of a set and three extra strings, but doesn't
save anything over a simpler comparison:
return verb == OpenWith || verb == AddTo || verb == PackWith;

https://codereview.chromium.org/1872223002/diff/120001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/file_tasks.js (right):

https://codereview.chromium.org/1872223002/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/file_tasks.js:375: if (task.verb ===
'Add to') {
here, too, this looks like it would benefit from an enum.

[cmihail, 2016-05-18 02:35:31]

https://codereview.chromium.org/1872223002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/file_tasks.cc (right):

https://codereview.chromium.org/1872223002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/file_tasks.cc:186: task_verb_(task_verb),
On 2016/05/16 16:17:32, Devlin wrote:
> Should we DCHECK that task_verb_ is a valid verb?
> 
> Or, looking at this more, maybe we should introduce an enum on the
> fileManagerPrivate API (which would give us compile errors)?

Done. Thanks, I didn't really know about enum's support from idl files, so good
to keep it in mind for the future :).

https://codereview.chromium.org/1872223002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/file_tasks.cc:495: for (const auto handler
: file_handlers) {
On 2016/05/16 16:17:32, Devlin wrote:
> nit: I personally think auto here reduces readability (partially because it
> makes me have to think "Does this need to be const auto&?")

Done.

https://codereview.chromium.org/1872223002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/file_tasks.cc:506: for (const auto entry :
handlers_for_entries) {
On 2016/05/16 16:17:32, Devlin wrote:
> this results in a copy - please make it const auto&

Done.

https://codereview.chromium.org/1872223002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/file_tasks_unittest.cc (right):

https://codereview.chromium.org/1872223002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/file_tasks_unittest.cc:68: "task title",
"OPEN_WITH", GURL("http://example.com/icon.png"),
On 2016/05/16 16:17:32, Devlin wrote:
> OPEN_WITH isn't a valid verb anymore (the suggestion in the previous file
would
> catch this).

Done.

https://codereview.chromium.org/1872223002/diff/120001/extensions/common/mani...
File extensions/common/manifest_handlers/file_handler_info.cc (right):

https://codereview.chromium.org/1872223002/diff/120001/extensions/common/mani...
extensions/common/manifest_handlers/file_handler_info.cc:26: const char
kOpenWith[] = "Open with";
On 2016/05/16 16:17:32, Devlin wrote:
> manifest constants are almost always all_lowercase_with_underscores.  Let's
try
> to be consistent (even though we're not always).

Done.

https://codereview.chromium.org/1872223002/diff/120001/extensions/common/mani...
extensions/common/manifest_handlers/file_handler_info.cc:42: return
supported_verbs.find(verb) != supported_verbs.end();
On 2016/05/16 16:17:32, Devlin wrote:
> This results in the construction of a set and three extra strings, but doesn't
> save anything over a simpler comparison:
> return verb == OpenWith || verb == AddTo || verb == PackWith;

Done.

https://codereview.chromium.org/1872223002/diff/120001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/file_tasks.js (right):

https://codereview.chromium.org/1872223002/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/file_tasks.js:375: if (task.verb ===
'Add to') {
On 2016/05/16 16:17:32, Devlin wrote:
> here, too, this looks like it would benefit from an enum.

Done.

https://codereview.chromium.org/1872223002/diff/140001/third_party/closure_co...
File third_party/closure_compiler/externs/file_manager_private.js (right):

https://codereview.chromium.org/1872223002/diff/140001/third_party/closure_co...
third_party/closure_compiler/externs/file_manager_private.js:5: // This file was
generated by:
git cl upload asked me to run next command fro this file:
python tools/json_schema_compiler/compiler.py
/usr/local/google/home/cmihail/chrome/src/chrome/common/extensions/api/file_manager_private.idl
--root=. --generator=externs >
/usr/local/google/home/cmihail/chrome/src/third_party/closure_compiler/externs/file_manager_private.js
Though I ended up with more changes, plus it asks me to do some extra changes to
README.chromium (I've ignore that).

Should I just revert this file or go forward with the change (I can ignore it).

[Devlin, 2016-05-18 18:49:41]

https://codereview.chromium.org/1872223002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/file_tasks.h (right):

https://codereview.chromium.org/1872223002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/file_tasks.h:175: const
extensions::api::file_manager_private::Verb& task_verb,
const enum& is a little excessive - just pass by value :)

https://codereview.chromium.org/1872223002/diff/140001/chrome/common/extensio...
File chrome/common/extensions/api/file_manager_private.idl (right):

https://codereview.chromium.org/1872223002/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/file_manager_private.idl:187: // File handler
verbs.
This should have a better description.

https://codereview.chromium.org/1872223002/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/file_manager_private.idl:207: Verb verb;
why not just make it an optional field?

https://codereview.chromium.org/1872223002/diff/140001/third_party/closure_co...
File third_party/closure_compiler/externs/file_manager_private.js (right):

https://codereview.chromium.org/1872223002/diff/140001/third_party/closure_co...
third_party/closure_compiler/externs/file_manager_private.js:5: // This file was
generated by:
On 2016/05/18 02:35:31, cmihail wrote:
> git cl upload asked me to run next command fro this file:
> python tools/json_schema_compiler/compiler.py
>
/usr/local/google/home/cmihail/chrome/src/chrome/common/extensions/api/file_manager_private.idl
> --root=. --generator=externs >
>
/usr/local/google/home/cmihail/chrome/src/third_party/closure_compiler/externs/file_manager_private.js
> Though I ended up with more changes, plus it asks me to do some extra changes
to
> README.chromium (I've ignore that).
> 
> Should I just revert this file or go forward with the change (I can ignore
it).

This is correct.  The reason there are so many changes is because other people
didn't do it. :)

https://codereview.chromium.org/1872223002/diff/140001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/file_tasks.js (right):

https://codereview.chromium.org/1872223002/diff/140001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/file_tasks.js:375: if (task.verb ===
'add_to') {
use the enums on the API object.

[cmihail, 2016-05-18 22:53:36]

https://codereview.chromium.org/1872223002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/file_tasks.h (right):

https://codereview.chromium.org/1872223002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/file_tasks.h:175: const
extensions::api::file_manager_private::Verb& task_verb,
On 2016/05/18 18:49:40, Devlin wrote:
> const enum& is a little excessive - just pass by value :)

Done.

https://codereview.chromium.org/1872223002/diff/140001/chrome/common/extensio...
File chrome/common/extensions/api/file_manager_private.idl (right):

https://codereview.chromium.org/1872223002/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/file_manager_private.idl:187: // File handler
verbs.
On 2016/05/18 18:49:41, Devlin wrote:
> This should have a better description.

Done.

https://codereview.chromium.org/1872223002/diff/140001/chrome/common/extensio...
chrome/common/extensions/api/file_manager_private.idl:207: Verb verb;
On 2016/05/18 18:49:40, Devlin wrote:
> why not just make it an optional field?

Done.

https://codereview.chromium.org/1872223002/diff/140001/third_party/closure_co...
File third_party/closure_compiler/externs/file_manager_private.js (right):

https://codereview.chromium.org/1872223002/diff/140001/third_party/closure_co...
third_party/closure_compiler/externs/file_manager_private.js:5: // This file was
generated by:
On 2016/05/18 18:49:41, Devlin wrote:
> On 2016/05/18 02:35:31, cmihail wrote:
> > git cl upload asked me to run next command fro this file:
> > python tools/json_schema_compiler/compiler.py
> >
>
/usr/local/google/home/cmihail/chrome/src/chrome/common/extensions/api/file_manager_private.idl
> > --root=. --generator=externs >
> >
>
/usr/local/google/home/cmihail/chrome/src/third_party/closure_compiler/externs/file_manager_private.js
> > Though I ended up with more changes, plus it asks me to do some extra
changes
> to
> > README.chromium (I've ignore that).
> > 
> > Should I just revert this file or go forward with the change (I can ignore
> it).
> 
> This is correct.  The reason there are so many changes is because other people
> didn't do it. :)

Hmm, I tried running third_party/closure_compiler/run_compiler as mentioned at
https://chromium.googlesource.com/chromium/src/+/master/docs/closure_compilat...,
but it fails with errors unrelated to the verbs change.

Any recommendations? I don't want to mix up changes that fix things unrelated to
a CL that is already big enough, especially if ending up in a need to revert the
change.

Also, not sure what exactly to write in README.chromium as mostly they contain
license information unrelated to private APIs.

https://codereview.chromium.org/1872223002/diff/140001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/file_tasks.js (right):

https://codereview.chromium.org/1872223002/diff/140001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/file_tasks.js:375: if (task.verb ===
'add_to') {
On 2016/05/18 18:49:41, Devlin wrote:
> use the enums on the API object.

Done.

[fukino, 2016-05-19 07:53:01]

On 2016/05/18 22:53:36, cmihail wrote:
> 
> Hmm, I tried running third_party/closure_compiler/run_compiler as mentioned at
>
https://chromium.googlesource.com/chromium/src/+/master/docs/closure_compilat...,
> but it fails with errors unrelated to the verbs change.
> 
> Any recommendations? I don't want to mix up changes that fix things unrelated
to
> a CL that is already big enough, especially if ending up in a need to revert
the
> change.
> 

IIRC, we manually maintain file_manager_private's externs file since we ran into
some issues with generated one.
We might be able to use generated one now, but I think it should be done by
separated CL (and by us, Files app team)

I'd recommend to manually update the
third_party/closure_compiler/externs/file_manager_private.js partially to
compile this change.
I filed issue 613096 to use the generated one in the future.

[cmihail, 2016-05-20 02:45:43]

On 2016/05/19 07:53:01, fukino wrote:
> On 2016/05/18 22:53:36, cmihail wrote:
> > 
> > Hmm, I tried running third_party/closure_compiler/run_compiler as mentioned
at
> >
>
https://chromium.googlesource.com/chromium/src/+/master/docs/closure_compilat...,
> > but it fails with errors unrelated to the verbs change.
> > 
> > Any recommendations? I don't want to mix up changes that fix things
unrelated
> to
> > a CL that is already big enough, especially if ending up in a need to revert
> the
> > change.
> > 
> 
> IIRC, we manually maintain file_manager_private's externs file since we ran
into
> some issues with generated one.
> We might be able to use generated one now, but I think it should be done by
> separated CL (and by us, Files app team)
> 
> I'd recommend to manually update the
> third_party/closure_compiler/externs/file_manager_private.js partially to
> compile this change.
> I filed issue 613096 to use the generated one in the future.

Done.

[Devlin, 2016-05-20 21:08:16]

Getting close!

https://codereview.chromium.org/1872223002/diff/180001/chrome/common/extensio...
File chrome/common/extensions/api/file_manager_private.idl (right):

https://codereview.chromium.org/1872223002/diff/180001/chrome/common/extensio...
chrome/common/extensions/api/file_manager_private.idl:206: // Task verb (ex.
Open With). Can be empty string in case of no verb
It can't be empty anymore, right?  It could just be omitted (which is already
indicated by the '?').  Also, the first part doesn't really tell much about how
it will be used.  Maybe add something like "The verb that will be used to
indicate what action the app will take" or something?

https://codereview.chromium.org/1872223002/diff/180001/extensions/common/mani...
File extensions/common/manifest_handlers/file_handler_info.cc (right):

https://codereview.chromium.org/1872223002/diff/180001/extensions/common/mani...
extensions/common/manifest_handlers/file_handler_info.cc:26: const char
kOpenWith[] = "open with";
https://codereview.chromium.org/1872223002/diff/120001/extensions/common/mani...

You missed the underscores part. :)

https://codereview.chromium.org/1872223002/diff/180001/extensions/common/mani...
extensions/common/manifest_handlers/file_handler_info.cc:104:
!handler_info.GetString(keys::kFileHandlerVerb, &handler.verb) &&
This is wrong - it should be an || on this line, right?

if (has_key && (!GetKey || KeyInvalid)) {
}

Otherwise we'll never validate.

This also implies there should be a test for this. :)

https://codereview.chromium.org/1872223002/diff/180001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/file_tasks.js (right):

https://codereview.chromium.org/1872223002/diff/180001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/file_tasks.js:375: if (task.verb ===
chrome.fileManagerPrivate.Verb.ADD_TO) {
use a switch()?

[cmihail, 2016-05-23 02:46:13]

https://codereview.chromium.org/1872223002/diff/180001/chrome/common/extensio...
File chrome/common/extensions/api/file_manager_private.idl (right):

https://codereview.chromium.org/1872223002/diff/180001/chrome/common/extensio...
chrome/common/extensions/api/file_manager_private.idl:206: // Task verb (ex.
Open With). Can be empty string in case of no verb
On 2016/05/20 21:08:16, Devlin wrote:
> It can't be empty anymore, right?  It could just be omitted (which is already
> indicated by the '?').  Also, the first part doesn't really tell much about
how
> it will be used.  Maybe add something like "The verb that will be used to
> indicate what action the app will take" or something?

Done. Though instead of empy, I did use Verb::VERB_NONE for the
FileBrowserHandler. Seems this is a default created value in the enum.

https://codereview.chromium.org/1872223002/diff/180001/extensions/common/mani...
File extensions/common/manifest_handlers/file_handler_info.cc (right):

https://codereview.chromium.org/1872223002/diff/180001/extensions/common/mani...
extensions/common/manifest_handlers/file_handler_info.cc:26: const char
kOpenWith[] = "open with";
On 2016/05/20 21:08:16, Devlin wrote:
>
https://codereview.chromium.org/1872223002/diff/120001/extensions/common/mani...
> 
> You missed the underscores part. :)

Done.

https://codereview.chromium.org/1872223002/diff/180001/extensions/common/mani...
extensions/common/manifest_handlers/file_handler_info.cc:104:
!handler_info.GetString(keys::kFileHandlerVerb, &handler.verb) &&
On 2016/05/20 21:08:16, Devlin wrote:
> This is wrong - it should be an || on this line, right?
> 
> if (has_key && (!GetKey || KeyInvalid)) {
> }
> 
> Otherwise we'll never validate.
> 
> This also implies there should be a test for this. :)

Done.

https://codereview.chromium.org/1872223002/diff/180001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/file_tasks.js (right):

https://codereview.chromium.org/1872223002/diff/180001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/file_tasks.js:375: if (task.verb ===
chrome.fileManagerPrivate.Verb.ADD_TO) {
On 2016/05/20 21:08:16, Devlin wrote:
> use a switch()?

Done.

[cmihail, 2016-05-23 09:26:22]

cmihail@chromium.org changed reviewers:
+ kinaba@chromium.org

[kinaba, 2016-05-23 09:51:37]

arc_file_tasks lgtm

https://codereview.chromium.org/1872223002/diff/220001/chrome/app/chromeos_st...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/1872223002/diff/220001/chrome/app/chromeos_st...
chrome/app/chromeos_strings.grdp:475: <message
name="IDS_FILE_BROWSER_SHARE_WITH_ACTION_LABEL" desc="Menu item label,
describing the action of sharing/attaching/sending files with the given
application.">
could you remove this one? (assuming it's not used elsewhere.)

[Devlin, 2016-05-23 16:35:06]

lgtm

https://codereview.chromium.org/1872223002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/file_tasks.h (right):

https://codereview.chromium.org/1872223002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/file_tasks.h:188: const
extensions::api::file_manager_private::Verb& task_verb() const {
nit: again, const & unnecessary.

https://codereview.chromium.org/1872223002/diff/220001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/file_browser/app_file_handler_multi/manifest.json
(right):

https://codereview.chromium.org/1872223002/diff/220001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/file_browser/app_file_handler_multi/manifest.json:16:
"verb": "add to",
How does this work?

https://codereview.chromium.org/1872223002/diff/220001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/file_tasks.js (right):

https://codereview.chromium.org/1872223002/diff/220001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/file_tasks.js:886: return
opt_taskToUseIfNoDefault ? opt_taskToUseIfNoDefault : null;
nit:
return opt_taskToUseIfNoDefault || null;
(or maybe just "return opt_taskToUseIfNoDefault" since it says it has to be
valid or null)

[cmihail, 2016-05-24 05:43:37]

https://codereview.chromium.org/1872223002/diff/220001/chrome/app/chromeos_st...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/1872223002/diff/220001/chrome/app/chromeos_st...
chrome/app/chromeos_strings.grdp:475: <message
name="IDS_FILE_BROWSER_SHARE_WITH_ACTION_LABEL" desc="Menu item label,
describing the action of sharing/attaching/sending files with the given
application.">
On 2016/05/23 09:51:37, kinaba wrote:
> could you remove this one? (assuming it's not used elsewhere.)

Done.

https://codereview.chromium.org/1872223002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/file_tasks.h (right):

https://codereview.chromium.org/1872223002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/file_tasks.h:188: const
extensions::api::file_manager_private::Verb& task_verb() const {
On 2016/05/23 16:35:06, Devlin wrote:
> nit: again, const & unnecessary.

Done.

https://codereview.chromium.org/1872223002/diff/220001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/file_browser/app_file_handler_multi/manifest.json
(right):

https://codereview.chromium.org/1872223002/diff/220001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/file_browser/app_file_handler_multi/manifest.json:16:
"verb": "add to",
On 2016/05/23 16:35:06, Devlin wrote:
> How does this work?

Sorry, I forgot to update the browser tests and run them and I only checked unit
tests. Fixed.
I will run a CQ dry to see if I missed any tests.

[cmihail, 2016-05-24 05:43:45]

The CQ bit was checked by cmihail@chromium.org

[cmihail, 2016-05-24 05:43:46]

The patchset sent to the CQ was uploaded after l-g-t-m from fukino@chromium.org,
rdevlin.cronin@chromium.org, kinaba@chromium.org
Link to the patchset: https://codereview.chromium.org/1872223002/#ps240001
(title: "Fix last nits.")

[cmihail, 2016-05-24 05:43:48]

The CQ bit was unchecked by cmihail@chromium.org

[cmihail, 2016-05-24 05:43:53]

The CQ bit was checked by cmihail@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-24 05:43:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1872223002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1872223002/240001

[cmihail, 2016-05-24 05:44:00]

The CQ bit was unchecked by cmihail@chromium.org

[cmihail, 2016-05-24 05:44:04]

The CQ bit was unchecked by cmihail@chromium.org

[cmihail, 2016-05-24 05:44:11]

The CQ bit was checked by cmihail@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-24 05:44:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1872223002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1872223002/240001

[commit-bot: I haz the power, 2016-05-24 06:58:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-24 06:58:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[cmihail, 2016-05-24 08:37:20]

https://codereview.chromium.org/1872223002/diff/220001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/file_tasks.js (right):

https://codereview.chromium.org/1872223002/diff/220001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/file_tasks.js:886: return
opt_taskToUseIfNoDefault ? opt_taskToUseIfNoDefault : null;
On 2016/05/23 16:35:06, Devlin wrote:
> nit:
> return opt_taskToUseIfNoDefault || null;
> (or maybe just "return opt_taskToUseIfNoDefault" since it says it has to be
> valid or null)

Just as mentioned in the last patch. In case opt_taskToUseIfNoDefault is
undefined, we need to return null due to check against null in other functions,
or otherwise code will break (see
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...)

[cmihail, 2016-05-24 09:01:03]

The CQ bit was checked by cmihail@chromium.org

[cmihail, 2016-05-24 09:01:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, fukino@chromium.org, kinaba@chromium.org
Link to the patchset: https://codereview.chromium.org/1872223002/#ps280001
(title: "Fix another of nits and add unit tests for SHARE_WITH after reading
whole CL from scratch.")

[commit-bot: I haz the power, 2016-05-24 09:01:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1872223002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1872223002/280001

[commit-bot: I haz the power, 2016-05-24 10:15:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-24 10:15:15]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[cmihail, 2016-05-25 00:01:55]

The CQ bit was checked by cmihail@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-25 00:02:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1872223002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1872223002/300001

[cmihail, 2016-05-25 00:03:10]

fukino@ can you take another look for the files app command id? I renamed it
back to open-with as it seems there is a higher risk of breaking other parts.
See
https://code.google.com/p/chromium/codesearch#search/&q=open-with&p=2&sq=pack...
for possible places were open-with is used.

[commit-bot: I haz the power, 2016-05-25 01:54:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-25 01:54:17]

Dry run: This issue passed the CQ dry run.

[commit-bot: I haz the power, 2016-05-25 01:54:29]

Dry run: None

[cmihail, 2016-05-25 02:12:59]

On 2016/05/25 01:54:29, commit-bot: I haz the power wrote:
> Dry run: None

Last dry run was successful, so if moving back to "open-with" for the id is ok,
I'll submit the CL.

[fukino, 2016-05-25 02:24:30]

On 2016/05/25 02:12:59, cmihail wrote:
> On 2016/05/25 01:54:29, commit-bot: I haz the power wrote:
> > Dry run: None
> 
> Last dry run was successful, so if moving back to "open-with" for the id is
ok,
> I'll submit the CL.

It is OK to keep using 'open-with' for the id of more actions.
We should use more-actions consistently by fixing tests, but it can be done by a
separated CL.

[cmihail, 2016-05-25 02:25:24]

On 2016/05/25 02:24:30, fukino wrote:
> On 2016/05/25 02:12:59, cmihail wrote:
> > On 2016/05/25 01:54:29, commit-bot: I haz the power wrote:
> > > Dry run: None
> > 
> > Last dry run was successful, so if moving back to "open-with" for the id is
> ok,
> > I'll submit the CL.
> 
> It is OK to keep using 'open-with' for the id of more actions.
> We should use more-actions consistently by fixing tests, but it can be done by
a
> separated CL.

ACK. Submitting

[cmihail, 2016-05-25 02:25:36]

The CQ bit was checked by cmihail@chromium.org

[cmihail, 2016-05-25 02:25:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, fukino@chromium.org, kinaba@chromium.org
Link to the patchset: https://codereview.chromium.org/1872223002/#ps300001
(title: "Move back to "open-with" for the id of the more actions dialog, as it
breaks some browsers tests, a…")

[commit-bot: I haz the power, 2016-05-25 02:26:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1872223002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1872223002/300001

[commit-bot: I haz the power, 2016-05-25 02:30:54]

Description was changed from

==========
Add verbs API to file handlers. Modify the Chrome OS UI so that it displayes the
internationalized verb together with the extension name in the file manager.

BUG=578725
R=fukino@chromium.org
==========

to

==========
Add verbs API to file handlers. Modify the Chrome OS UI so that it displayes the
internationalized verb together with the extension name in the file manager.

BUG=578725
R=fukino@chromium.org
==========

[commit-bot: I haz the power, 2016-05-25 02:30:56]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-05-25 02:32:52]

Description was changed from

==========
Add verbs API to file handlers. Modify the Chrome OS UI so that it displayes the
internationalized verb together with the extension name in the file manager.

BUG=578725
R=fukino@chromium.org
==========

to

==========
Add verbs API to file handlers. Modify the Chrome OS UI so that it displayes the
internationalized verb together with the extension name in the file manager.

BUG=578725
R=fukino@chromium.org

Committed: https://crrev.com/03a4c08c513f71d7bf2592bec43a33bfbf9290e9
Cr-Commit-Position: refs/heads/master@{#395782}
==========

[commit-bot: I haz the power, 2016-05-25 02:32:53]

Patchset 16 (id:??) landed as
https://crrev.com/03a4c08c513f71d7bf2592bec43a33bfbf9290e9
Cr-Commit-Position: refs/heads/master@{#395782}