Chrome OS "open with" picker allowing Web Intents

Updates the Chrome OS file browser to support opening files with Web Intent handlers. Adds a basic test.

This is added to the list of 'normal' file handlers in the same way that Drive tasks are currently added. (although I feel like the whole flow needs to be changed a bit, since we don't store defaults for anything but the original file browser tasks).

BUG=138664
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157693

[thorogood, 2012-08-25 04:25:07]

Hey Ben, PTAL.

[benwells, 2012-08-27 08:02:59]

Initial comments...

https://chromiumcodereview.appspot.com/10834383/diff/3006/chrome/browser/chro...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/3006/chrome/browser/chro...
chrome/browser/chromeos/extensions/file_handler_util.cc:215: if (list->size() <=
1) {
Could you add a comment to explain the logic used and why it's important? That
is, why we sometimes just sort by name and why we sometimes also sort using the
timestamp.

https://chromiumcodereview.appspot.com/10834383/diff/3006/chrome/browser/chro...
chrome/browser/chromeos/extensions/file_handler_util.cc:836: if
(IsWebIntentAction(action_id_)) {
This is a bit of a monster function now. This block should come out into another
function.

https://chromiumcodereview.appspot.com/10834383/diff/3006/chrome/browser/exte...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/3006/chrome/browser/exte...
chrome/browser/extensions/extension_special_storage_policy.cc:86:
extension->intents_services().empty()) {
We need to do this only if the service is for the view action. This code
currently does it for all intent actions.

https://chromiumcodereview.appspot.com/10834383/diff/3006/net/base/mime_util_...
File net/base/mime_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/3006/net/base/mime_util_...
net/base/mime_util_unittest.cc:83: EXPECT_TRUE(MatchesMimeType("*", ""));
Is there a reason to add this test?

https://chromiumcodereview.appspot.com/10834383/diff/15001/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/15001/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:722: if
(!extension->is_platform_app() || !extension->has_background_page())
Don't need to check for background page here, all platform apps have them.

https://chromiumcodereview.appspot.com/10834383/diff/15001/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:730:
ActionsForExtension(extension, mime_types);
I should have mentioned this, but we only care about the view action, other
actions should be ignored.

https://chromiumcodereview.appspot.com/10834383/diff/15001/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_handler_util.h (right):

https://chromiumcodereview.appspot.com/10834383/diff/15001/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.h:30: // extension. This is
a Web Intent if |action_id| is prefixed with "http".
Using a prefix here feels pretty hacky. Can we pass in an enum saying what type
of task it is?

[thorogood, 2012-08-28 00:04:03]

Some replies (not all).

https://chromiumcodereview.appspot.com/10834383/diff/3006/chrome/browser/chro...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/3006/chrome/browser/chro...
chrome/browser/chromeos/extensions/file_handler_util.cc:215: if (list->size() <=
1) {
On 2012/08/27 08:02:59, benwells wrote:
> Could you add a comment to explain the logic used and why it's important? That
> is, why we sometimes just sort by name and why we sometimes also sort using
the
> timestamp.

I've put the comment in front of the if statement, and removed the inline ones.

https://chromiumcodereview.appspot.com/10834383/diff/3006/net/base/mime_util_...
File net/base/mime_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/3006/net/base/mime_util_...
net/base/mime_util_unittest.cc:83: EXPECT_TRUE(MatchesMimeType("*", ""));
On 2012/08/27 08:02:59, benwells wrote:
> Is there a reason to add this test?

The reason for this is that it demonstrates that if a Web Intent handler
supports "*", it can accept all files, even those without extensions.

We weren't showing that this was the case through a test, and I added this
mostly to confirm this behaviour (as opposed to the alternative, which was that
a blank mime-type could not be matched by "*").

https://chromiumcodereview.appspot.com/10834383/diff/15001/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/15001/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:722: if
(!extension->is_platform_app() || !extension->has_background_page())
On 2012/08/27 08:02:59, benwells wrote:
> Don't need to check for background page here, all platform apps have them.

Done.

https://chromiumcodereview.appspot.com/10834383/diff/15001/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:730:
ActionsForExtension(extension, mime_types);
On 2012/08/27 08:02:59, benwells wrote:
> I should have mentioned this, but we only care about the view action, other
> actions should be ignored.

Before I make this change, can I ask why? In my mind, it makes sense to support
all possible actions on files.

[thorogood, 2012-08-29 06:34:46]

https://chromiumcodereview.appspot.com/10834383/diff/3006/chrome/browser/chro...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/3006/chrome/browser/chro...
chrome/browser/chromeos/extensions/file_handler_util.cc:836: if
(IsWebIntentAction(action_id_)) {
On 2012/08/27 08:02:59, benwells wrote:
> This is a bit of a monster function now. This block should come out into
another
> function.

I've modified platform_app_launcher to allow launching an app with a path - PTAL
- so this change is now tiny.

https://chromiumcodereview.appspot.com/10834383/diff/3006/chrome/browser/exte...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/3006/chrome/browser/exte...
chrome/browser/extensions/extension_special_storage_policy.cc:86:
extension->intents_services().empty()) {
On 2012/08/27 08:02:59, benwells wrote:
> We need to do this only if the service is for the view action. This code
> currently does it for all intent actions.

Done.

https://chromiumcodereview.appspot.com/10834383/diff/15001/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/15001/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:730:
ActionsForExtension(extension, mime_types);
On 2012/08/28 00:04:04, thorogood wrote:
> On 2012/08/27 08:02:59, benwells wrote:
> > I should have mentioned this, but we only care about the view action, other
> > actions should be ignored.
> 
> Before I make this change, can I ask why? In my mind, it makes sense to
support
> all possible actions on files.

I've updated this file to work this way now. PTAL!

https://chromiumcodereview.appspot.com/10834383/diff/15001/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_handler_util.h (right):

https://chromiumcodereview.appspot.com/10834383/diff/15001/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.h:30: // extension. This is
a Web Intent if |action_id| is prefixed with "http".
On 2012/08/27 08:02:59, benwells wrote:
> Using a prefix here feels pretty hacky. Can we pass in an enum saying what
type
> of task it is?

Yes, I guess so, although I'd probably be keen to add a third parameter then: it
becomes a bit kludgy since we have DriveTasks, WebIntentTasks, and then
"regular" FileActionTasks. This way we have a prefix for all three.

How does that sound?

[thorogood, 2012-08-29 08:58:07]

https://chromiumcodereview.appspot.com/10834383/diff/15001/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_handler_util.h (right):

https://chromiumcodereview.appspot.com/10834383/diff/15001/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.h:30: // extension. This is
a Web Intent if |action_id| is prefixed with "http".
On 2012/08/29 06:34:46, thorogood wrote:
> On 2012/08/27 08:02:59, benwells wrote:
> > Using a prefix here feels pretty hacky. Can we pass in an enum saying what
> type
> > of task it is?
> 
> Yes, I guess so, although I'd probably be keen to add a third parameter then:
it
> becomes a bit kludgy since we have DriveTasks, WebIntentTasks, and then
> "regular" FileActionTasks. This way we have a prefix for all three.
> 
> How does that sound?

I've taken on your first suggestion now - like I should have to start with :)

There's now an enum describing the type of task, and that's used in MakeTaskID
and CrackTaskID. There's also some relevant JS changes in the file browser code
since the TaskID now has three fields instead of just two.

[benwells, 2012-08-30 08:09:03]

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:299: const
std::string& action, std::string* title) {
I think you should pass in the mime types here and make sure the service you get
the title from matches at least one of the mime types.

Perhaps the function ExtensionSupportsAction could output a title.

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:697: // matching
apps and add them. These tasks will be identified by the "http"
Is this comment still correct?

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:719:
file_extension.substr(1), &mime_type))
Should this be if (file_extension.empty || !net::.....)?

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:730: // We can't
use hosted apps to open files.
Hmm ... apart from it not being implemented, is there a reason why not? Maybe
this should be a TODO instead, or the comment make it clearer that this isn't a
design choice but a limitation.

As an aside, after you download a file if you have an extension installed
(hosted app or platform app) that can view the file, it should show the picker
and let you open the file in the chosen app.

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_handler_util.cc:193: bool
HasCommonTimestamp(LastUsedHandlerList* list) {
Now I understand. Can you rename this to AllHaveSameTimestamp (or something), I
previously assumed from the name it was returning true if any of the list shared
a common timestamp.

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_handler_util.cc:253: LOG(INFO) << "Made
task_id=" << out
Make this VLOG(1) (then it will only show up with verbose logging enabled). Same
in CrackTaskID below.

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/extensions/...
chrome/browser/extensions/extension_special_storage_policy.cc:25: const char
kWebIntentViewAction[] = "http://webintents.org/view";
I think I saw this constant get declared in one of the content/ web intent files
recently. Can you use that?

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_launcher.cc (right):

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:295: // Optionally resolve
the file_path from the first command-line argument.
I really like these refactorings as the role of the PlatformAppPathLauncher
seems really clean: it launches apps with paths.

Let's keep it nice and clean by making it not handle the simple case of
launching with no data. That can just become a function in the anonymous
namespace used by LaunchPlatformApp if there is no command line data, or by the
PlatformAppPathLauncher if there is some problem.

Also, creating a function GetAbsolutePathFromCommandLine  would make this really
nice, it can then be something like:

if (GetAbsolutePathFromCommandLine(command_line, cur_dir, &path) {
  LaunchPlatformAppWIthPath(...)
  return;
}

LaunchPlatformAppWithNoData(...)

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_launcher.h (right):

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.h:30: // path may be empty, in
which case the app is launched without launch data.
Let's change this so it needs a path, that feels a lot better.

[thorogood, 2012-08-31 01:27:32]

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_handler_util.cc:193: bool
HasCommonTimestamp(LastUsedHandlerList* list) {
On 2012/08/30 08:09:03, benwells wrote:
> Now I understand. Can you rename this to AllHaveSameTimestamp (or something),
I
> previously assumed from the name it was returning true if any of the list
shared
> a common timestamp.

Done.

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_handler_util.cc:253: LOG(INFO) << "Made
task_id=" << out
On 2012/08/30 08:09:03, benwells wrote:
> Make this VLOG(1) (then it will only show up with verbose logging enabled).
Same
> in CrackTaskID below.

Removed actually. Was just for testing.

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/extensions/...
chrome/browser/extensions/extension_special_storage_policy.cc:25: const char
kWebIntentViewAction[] = "http://webintents.org/view";
On 2012/08/30 08:09:03, benwells wrote:
> I think I saw this constant get declared in one of the content/ web intent
files
> recently. Can you use that?

Done.

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_launcher.cc (right):

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:295: // Optionally resolve
the file_path from the first command-line argument.
On 2012/08/30 08:09:03, benwells wrote:
> I really like these refactorings as the role of the PlatformAppPathLauncher
> seems really clean: it launches apps with paths.
> 
> Let's keep it nice and clean by making it not handle the simple case of
> launching with no data. That can just become a function in the anonymous
> namespace used by LaunchPlatformApp if there is no command line data, or by
the
> PlatformAppPathLauncher if there is some problem.
> 
> Also, creating a function GetAbsolutePathFromCommandLine  would make this
really
> nice, it can then be something like:
> 
> if (GetAbsolutePathFromCommandLine(command_line, cur_dir, &path) {
>   LaunchPlatformAppWIthPath(...)
>   return;
> }
> 
> LaunchPlatformAppWithNoData(...)

These are great suggestions! The code feels much neater now. 
The only thing I'm hesitant about is that we still have the method
"LaunchPlatformApp", when we could instead call it
"LaunchPlatformAppWithCommandLine". However perhaps it's not required...

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_launcher.h (right):

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.h:30: // path may be empty, in
which case the app is launched without launch data.
On 2012/08/30 08:09:03, benwells wrote:
> Let's change this so it needs a path, that feels a lot better.

Done.

[benwells, 2012-08-31 05:40:15]

lgtm with nits

http://codereview.chromium.org/10834383/diff/35002/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/10834383/diff/35002/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:86: const char
kWebIntentViewAction[] = "http://webintents.org/view";
Nit: Use constant from other file.

http://codereview.chromium.org/10834383/diff/35002/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_launcher.cc (right):

http://codereview.chromium.org/10834383/diff/35002/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:342:
LaunchPlatformAppWithNoData(profile, extension);
Nit: Use an early return here:

if (...) {
  Launch...
  return;
}

Launch...

[thorogood, 2012-09-01 03:02:10]

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:299: const
std::string& action, std::string* title) {
On 2012/08/30 08:09:03, benwells wrote:
> I think you should pass in the mime types here and make sure the service you
get
> the title from matches at least one of the mime types.
> 
> Perhaps the function ExtensionSupportsAction could output a title.

Sounds good. My original methods made more sense when this was more open-ended
to multiple intent types.

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:697: // matching
apps and add them. These tasks will be identified by the "http"
On 2012/08/30 08:09:03, benwells wrote:
> Is this comment still correct?

Nope, clarified.

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:719:
file_extension.substr(1), &mime_type))
On 2012/08/30 08:09:03, benwells wrote:
> Should this be if (file_extension.empty || !net::.....)?

Well, the second command should only run if the file_extension is _not_ empty.
The mime_type is empty already due to the constructor above.

The condition in here is basically just for safety: if
GetWellKnownMimeTypeFromExtension returns false, but modifies mime_type, that we
reset it.

Perhaps I'm thinking about this too much :)

http://codereview.chromium.org/10834383/diff/31001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:730: // We can't
use hosted apps to open files.
On 2012/08/30 08:09:03, benwells wrote:
> Hmm ... apart from it not being implemented, is there a reason why not? Maybe
> this should be a TODO instead, or the comment make it clearer that this isn't
a
> design choice but a limitation.

I've now said "We don't support using hosted apps to open files.".

> 
> As an aside, after you download a file if you have an extension installed
> (hosted app or platform app) that can view the file, it should show the picker
> and let you open the file in the chosen app.

Is there a bug to track this? :)

http://codereview.chromium.org/10834383/diff/35002/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/10834383/diff/35002/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:86: const char
kWebIntentViewAction[] = "http://webintents.org/view";
On 2012/08/31 05:40:15, benwells wrote:
> Nit: Use constant from other file.

Done.

http://codereview.chromium.org/10834383/diff/35002/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_launcher.cc (right):

http://codereview.chromium.org/10834383/diff/35002/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_launcher.cc:342:
LaunchPlatformAppWithNoData(profile, extension);
On 2012/08/31 05:40:15, benwells wrote:
> Nit: Use an early return here:
> 
> if (...) {
>   Launch...
>   return;
> }
> 
> Launch...

Done.

[thorogood, 2012-09-06 08:41:57]

Hi Sergey/Toni,

I'm mostly sending this to both of you because: Sergey has his name on this
TODO, and Toni last reviewed a big change to this area. Please consider it a WIP
since I'm not sure if this is the best way to approach this change.

[tbarzic, 2012-09-11 02:01:40]

sorry for the delay,
I forgot to publish comments on Friday..

https://chromiumcodereview.appspot.com/10834383/diff/38026/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/38026/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.cc:241: return
base::StringPrintf("%s|%d|%s",
I'd rather make task_type string (or, if you keep integers, add a comment in
TaskType enum that order of enum values cannot change)

https://chromiumcodereview.appspot.com/10834383/diff/38026/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.cc:256: if (count != 3)
Since task ids are persisted in prefs, we should probably keep supporting old
style task ids (at least for a while)

https://chromiumcodereview.appspot.com/10834383/diff/38026/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.cc:847: for
(FileDefinitionList::const_iterator iter = file_list.begin();
why don't you create a separate executor for intents.. looks like you don't need
the most of the stuff handled in ExtensionTaskExecutor (opening external file
system, granting file access permissions and waking up lazy background page,
dispatching task event).
afaik, LaunchPlatformAppWihtPath does all of this for the web intent (correct me
if I'm wrong :) )

(the extension only needs access to isolated file system, right?)

https://chromiumcodereview.appspot.com/10834383/diff/38026/chrome/test/data/e...
File chrome/test/data/extensions/api_test/filebrowser_component/main.js (right):

https://chromiumcodereview.appspot.com/10834383/diff/38026/chrome/test/data/e...
chrome/test/data/extensions/api_test/filebrowser_component/main.js:90:
errorCallback({message: 'Wrong task from getFileTasks(): ' + taskName});
this function has been removed when "patterns" were removed from task passed to
the file manager extension..

https://chromiumcodereview.appspot.com/10834383/diff/38026/chrome/test/data/e...
File chrome/test/data/extensions/api_test/webintent_handler/background.js
(right):

https://chromiumcodereview.appspot.com/10834383/diff/38026/chrome/test/data/e...
chrome/test/data/extensions/api_test/webintent_handler/background.js:14: }
can you test that the extension can read the file for which intent task was
executed?

[SeRya, 2012-09-11 16:57:43]

lgtm

[thorogood, 2012-09-13 02:34:49]

https://chromiumcodereview.appspot.com/10834383/diff/38026/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/38026/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.cc:241: return
base::StringPrintf("%s|%d|%s",
On 2012/09/11 02:01:40, tbarzic wrote:
> I'd rather make task_type string (or, if you keep integers, add a comment in
> TaskType enum that order of enum values cannot change)

I agree with you. I've updated this CL with all the other suggestions, and I'll
ping it again when I've converted this to a string.

https://chromiumcodereview.appspot.com/10834383/diff/38026/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.cc:256: if (count != 3)
On 2012/09/11 02:01:40, tbarzic wrote:
> Since task ids are persisted in prefs, we should probably keep supporting old
> style task ids (at least for a while)

Sounds good. If we see two components I'll try to parse out the Drive/File
intent that was previously contained.

https://chromiumcodereview.appspot.com/10834383/diff/38026/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.cc:847: for
(FileDefinitionList::const_iterator iter = file_list.begin();
On 2012/09/11 02:01:40, tbarzic wrote:
> why don't you create a separate executor for intents.. looks like you don't
need
> the most of the stuff handled in ExtensionTaskExecutor (opening external file
> system, granting file access permissions and waking up lazy background page,
> dispatching task event).
> afaik, LaunchPlatformAppWihtPath does all of this for the web intent (correct
me
> if I'm wrong :) )
> 
> (the extension only needs access to isolated file system, right?)

Yup. You're totally right. This is much more concise now, thanks for the
suggestion.

https://chromiumcodereview.appspot.com/10834383/diff/38026/chrome/test/data/e...
File chrome/test/data/extensions/api_test/filebrowser_component/main.js (right):

https://chromiumcodereview.appspot.com/10834383/diff/38026/chrome/test/data/e...
chrome/test/data/extensions/api_test/filebrowser_component/main.js:90:
errorCallback({message: 'Wrong task from getFileTasks(): ' + taskName});
On 2012/09/11 02:01:40, tbarzic wrote:
> this function has been removed when "patterns" were removed from task passed
to
> the file manager extension..
> 

Cool, that makes sense. Yeah - I wondered about "patterns", since we were only
returning a list of things for your selected files already.

this was just left over from the big merge. Reverted this whole file.

[tbarzic, 2012-09-13 04:29:16]

http://codereview.chromium.org/10834383/diff/38026/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/filebrowser_component/main.js (right):

http://codereview.chromium.org/10834383/diff/38026/chrome/test/data/extension...
chrome/test/data/extensions/api_test/filebrowser_component/main.js:113:
this.fileCreator_ = new TestFileCreator("tmp", true /* shouldRandomize */);
Do
if (expectations.fileVerifierFunction_)
  this.listener_ = ..

http://codereview.chromium.org/10834383/diff/38026/chrome/test/data/extension...
chrome/test/data/extensions/api_test/filebrowser_component/main.js:120:
console.log('Requesting local file system...');
if (this.listener_)

http://codereview.chromium.org/10834383/diff/38026/chrome/test/data/extension...
chrome/test/data/extensions/api_test/filebrowser_component/main.js:175: if
(!this.expectations_.hasVerifyStep()) {
I think you still need something like this (TestRunner will wait for the
response which will not be sent by the intent handler)

if (!this.listener_)
  this.reportSuccess_();

http://codereview.chromium.org/10834383/diff/41022/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/10834383/diff/41022/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:248: // If this
method returns true, also returns the |title| of this action.
why not add function std::string GetTitleForAction()
that will return the title of the first intents_service (or first non empty
title,, in case it can be '').

http://codereview.chromium.org/10834383/diff/41022/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:257: for
(std::vector<webkit_glue::WebIntentServiceData>::const_iterator i =
since you have nested loops, it would probably be better if iterator names are
more descriptive.

http://codereview.chromium.org/10834383/diff/41022/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:267: while (pi !=
pending.end()) {
how about (I think it is nicer, but your call):

std::set<std::string>::iterator current = pi++;
if (net::MatchesMime...)
  pending.erase(current);

http://codereview.chromium.org/10834383/diff/41022/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:708:
DCHECK_EQ(task_type, file_handler_util::TASK_DRIVE)
I don't think the log adds extra value to the check..

http://codereview.chromium.org/10834383/diff/41022/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:710: if (!result
|| task_type != file_handler_util::TASK_DRIVE)
you have a dcheck for (task_type == TASK_DRIVE), so you shouldn't handle the
case task_type != TASK_DRIVE...

http://codereview.chromium.org/10834383/diff/41022/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:803: mime_type =
std::string();  // There was no mime-type available from this
does this mean that std::string() is used to represent "*" matcher?

http://codereview.chromium.org/10834383/diff/41022/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

http://codereview.chromium.org/10834383/diff/41022/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_handler_util.cc:529: //
you lost 'static' in the comment

http://codereview.chromium.org/10834383/diff/41022/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_handler_util.cc:997: // If we're a Web
Intent action (to an extension), short-circuit and deliver
probably don't need the comment anymore.

http://codereview.chromium.org/10834383/diff/41022/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_handler_util.cc:1029: bool is_drive_file
= url.type() == fileapi::kFileSystemTypeDrive;
I don't think this is needed.

[thorogood, 2012-09-13 08:14:04]

https://chromiumcodereview.appspot.com/10834383/diff/38026/chrome/test/data/e...
File chrome/test/data/extensions/api_test/filebrowser_component/main.js (right):

https://chromiumcodereview.appspot.com/10834383/diff/38026/chrome/test/data/e...
chrome/test/data/extensions/api_test/filebrowser_component/main.js:113:
this.fileCreator_ = new TestFileCreator("tmp", true /* shouldRandomize */);
On 2012/09/13 04:29:16, tbarzic wrote:
> Do
> if (expectations.fileVerifierFunction_)
>   this.listener_ = ..

Done, although I never make a request like this from my packaged app, as
"chrome.extension.sendRequest" is not available in v2 packaged apps. I just call
success from the app.

https://chromiumcodereview.appspot.com/10834383/diff/38026/chrome/test/data/e...
chrome/test/data/extensions/api_test/filebrowser_component/main.js:120:
console.log('Requesting local file system...');
On 2012/09/13 04:29:16, tbarzic wrote:
> if (this.listener_)

Done.

https://chromiumcodereview.appspot.com/10834383/diff/38026/chrome/test/data/e...
chrome/test/data/extensions/api_test/filebrowser_component/main.js:175: if
(!this.expectations_.hasVerifyStep()) {
On 2012/09/13 04:29:16, tbarzic wrote:
> I think you still need something like this (TestRunner will wait for the
> response which will not be sent by the intent handler)
> 
> if (!this.listener_)
>   this.reportSuccess_();

I'm not sure - my test intent handler does in fact call "chrome.test.succeed",
which works fine. So I've left this out.

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:248: // If this
method returns true, also returns the |title| of this action.
On 2012/09/13 04:29:17, tbarzic wrote:
> why not add function std::string GetTitleForAction()
> that will return the title of the first intents_service (or first non empty
> title,, in case it can be ''). 

I actually started with that, and benwells@ suggested rolling it into this
method. I feel like this approach is fine.

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:257: for
(std::vector<webkit_glue::WebIntentServiceData>::const_iterator i =
On 2012/09/13 04:29:17, tbarzic wrote:
> since you have nested loops, it would probably be better if iterator names are
> more descriptive.

Done.

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:267: while (pi !=
pending.end()) {
On 2012/09/13 04:29:17, tbarzic wrote:
> how about (I think it is nicer, but your call):
> 
> std::set<std::string>::iterator current = pi++;
> if (net::MatchesMime...)
>   pending.erase(current);

I like it. I'm hardly a C++ guru :)

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:708:
DCHECK_EQ(task_type, file_handler_util::TASK_DRIVE)
On 2012/09/13 04:29:17, tbarzic wrote:
> I don't think the log adds extra value to the check..

Done.

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:710: if (!result
|| task_type != file_handler_util::TASK_DRIVE)
On 2012/09/13 04:29:17, tbarzic wrote:
> you have a dcheck for (task_type == TASK_DRIVE), so you shouldn't handle the
> case task_type != TASK_DRIVE...

Well, before I got here, there was also a DCHECK for calling CrackDriveTaskID.
This if statement was also here before, and is just sanity for production code.

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:803: mime_type =
std::string();  // There was no mime-type available from this
On 2012/09/13 04:29:17, tbarzic wrote:
> does this mean that std::string() is used to represent "*" matcher?

Almost. You're the wrong way around; this says that if the file doesn't have an
extension (or we can't find it's mime-type) then we say the file has a mime type
of "", which is only supported if the Web Intents handler is "*" or "*/*".

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.cc:529: //
On 2012/09/13 04:29:17, tbarzic wrote:
> you lost 'static' in the comment

Done.

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.cc:997: // If we're a Web
Intent action (to an extension), short-circuit and deliver
On 2012/09/13 04:29:17, tbarzic wrote:
> probably don't need the comment anymore.

Done.

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.cc:1029: bool is_drive_file
= url.type() == fileapi::kFileSystemTypeDrive;
On 2012/09/13 04:29:17, tbarzic wrote:
> I don't think this is needed.

Done.

[benwells, 2012-09-13 09:52:29]

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:248: // If this
method returns true, also returns the |title| of this action.
On 2012/09/13 08:14:04, thorogood wrote:
> On 2012/09/13 04:29:17, tbarzic wrote:
> > why not add function std::string GetTitleForAction()
> > that will return the title of the first intents_service (or first non empty
> > title,, in case it can be ''). 
> 
> I actually started with that, and benwells@ suggested rolling it into this
> method. I feel like this approach is fine.

Either way you have to handle the case where an extension has multiple view
actions for different mime types, with different titles. In this case you need
to look at the mime types you're handling to pick the right title. If there are
multiple mime types being handled there might be multiple possible titles, but
you should always pick a valid one.

[tbarzic, 2012-09-13 18:44:45]

https://chromiumcodereview.appspot.com/10834383/diff/38026/chrome/test/data/e...
File chrome/test/data/extensions/api_test/filebrowser_component/main.js (right):

https://chromiumcodereview.appspot.com/10834383/diff/38026/chrome/test/data/e...
chrome/test/data/extensions/api_test/filebrowser_component/main.js:175: if
(!this.expectations_.hasVerifyStep()) {
On 2012/09/13 08:14:04, thorogood wrote:
> On 2012/09/13 04:29:16, tbarzic wrote:
> > I think you still need something like this (TestRunner will wait for the
> > response which will not be sent by the intent handler)
> > 
> > if (!this.listener_)
> >   this.reportSuccess_();
> 
> I'm not sure - my test intent handler does in fact call "chrome.test.succeed",
> which works fine. So I've left this out.

yeah, but the problem is files created during the test are cleaned up in
reportSuccess_/reportFail_ (but again, cleaning up the test dirs in the
extension could be racy in your case, especially if you add file reading in your
test intent handler)

FWIW, it think you would benefit from writing your own test behaviour rather
than using this, see remote.js for example (you would need initTest(),
readFile() and simplified executeReadFile() part). you should create/delete the
test file in C++ code (you could find ScopedTempDir usefull for cleaning up the
file. you can see  SetUp and AddMountPoint in
chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc for
example).
I plan to refactor the rest of the tests to use something like this soon.

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:248: // If this
method returns true, also returns the |title| of this action.
On 2012/09/13 09:52:30, benwells wrote:
> On 2012/09/13 08:14:04, thorogood wrote:
> > On 2012/09/13 04:29:17, tbarzic wrote:
> > > why not add function std::string GetTitleForAction()
> > > that will return the title of the first intents_service (or first non
empty
> > > title,, in case it can be ''). 
> > 
> > I actually started with that, and benwells@ suggested rolling it into this
> > method. I feel like this approach is fine.
> 
> Either way you have to handle the case where an extension has multiple view
> actions for different mime types, with different titles. In this case you need
> to look at the mime types you're handling to pick the right title. If there
are
> multiple mime types being handled there might be multiple possible titles, but
> you should always pick a valid one.

yeah, makes sense..
I missed continue part in the for loop when I was writing the comment.

Still, I think it would be clearer if we avoided having function that does two
things.. but, do as you like :)

You should probably reflect in the function name that it also returns action
title..

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:710: if (!result
|| task_type != file_handler_util::TASK_DRIVE)
On 2012/09/13 08:14:04, thorogood wrote:
> On 2012/09/13 04:29:17, tbarzic wrote:
> > you have a dcheck for (task_type == TASK_DRIVE), so you shouldn't handle the
> > case task_type != TASK_DRIVE...
> 
> Well, before I got here, there was also a DCHECK for calling CrackDriveTaskID.
> This if statement was also here before, and is just sanity for production
code.

yeah, but the thing is DCHECK contradicts the need to handle failure case. So
you should either remove if() or DCHECK.
(in this case, tasks in available_tasks are created by MakeDriveTaskID (see
IntersectAvaliableDriveTasks) just before calling this function, so I would go
with keeping DCHECKs)

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:803: mime_type =
std::string();  // There was no mime-type available from this
On 2012/09/13 08:14:04, thorogood wrote:
> On 2012/09/13 04:29:17, tbarzic wrote:
> > does this mean that std::string() is used to represent "*" matcher?
> 
> Almost. You're the wrong way around; this says that if the file doesn't have
an
> extension (or we can't find it's mime-type) then we say the file has a mime
type
> of "", which is only supported if the Web Intents handler is "*" or "*/*".

oh, could you please clarify this in the comment.

also nits:
could you put the comment in the line before |mime_type = std::string()|

please add { } for the if block

https://chromiumcodereview.appspot.com/10834383/diff/46003/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/46003/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:249: bool
ExtensionSupportsAction(const Extension* extension,
nit:
const Extension* extension should got to the new line
(args should be vertically aligned)

[thorogood, 2012-09-16 03:41:18]

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:248: // If this
method returns true, also returns the |title| of this action.
On 2012/09/13 18:44:45, tbarzic wrote:
> On 2012/09/13 09:52:30, benwells wrote:
> > On 2012/09/13 08:14:04, thorogood wrote:
> > > On 2012/09/13 04:29:17, tbarzic wrote:
> > > > why not add function std::string GetTitleForAction()
> > > > that will return the title of the first intents_service (or first non
> empty
> > > > title,, in case it can be ''). 
> > > 
> > > I actually started with that, and benwells@ suggested rolling it into this
> > > method. I feel like this approach is fine.
> > 
> > Either way you have to handle the case where an extension has multiple view
> > actions for different mime types, with different titles. In this case you
need
> > to look at the mime types you're handling to pick the right title. If there
> are
> > multiple mime types being handled there might be multiple possible titles,
but
> > you should always pick a valid one.
> 
> yeah, makes sense..
> I missed continue part in the for loop when I was writing the comment.
> 
> Still, I think it would be clearer if we avoided having function that does two
> things.. but, do as you like :)
> 
> You should probably reflect in the function name that it also returns action
> title..

I've changed this method to be named "FindTitleForActionWithTypes", which I feel
is also accurate, and updated the comment. Basically you can legitimately think
of this method as finding the title of the specified action that supports these
mime-types.

Btw, interestingly enough, since Web Intents are specified as a action/spec
mapping, then there is a direct mapping between action and title. (It's just
that if an action supports multiple mime types, this is flattened in the
Extension initialisation code - so finding the "first" match is fine, since
they'll all be the same). Ben's comment is thus invalid.

I know this probably feels like bikeshedding from all directions, but given the
above paragraph, I'm happy to break this apart into two methods again if you
think it's cleaner.

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:710: if (!result
|| task_type != file_handler_util::TASK_DRIVE)
On 2012/09/13 18:44:45, tbarzic wrote:
> On 2012/09/13 08:14:04, thorogood wrote:
> > On 2012/09/13 04:29:17, tbarzic wrote:
> > > you have a dcheck for (task_type == TASK_DRIVE), so you shouldn't handle
the
> > > case task_type != TASK_DRIVE...
> > 
> > Well, before I got here, there was also a DCHECK for calling
CrackDriveTaskID.
> > This if statement was also here before, and is just sanity for production
> code.
> 
> yeah, but the thing is DCHECK contradicts the need to handle failure case. So
> you should either remove if() or DCHECK.
> (in this case, tasks in available_tasks are created by MakeDriveTaskID (see
> IntersectAvaliableDriveTasks) just before calling this function, so I would go
> with keeping DCHECKs)

Fair enough. I think I assumed that this might be somehow unsanitised data, but
you're right here. I'll remove the if in favour of DCHECKs.

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:803: mime_type =
std::string();  // There was no mime-type available from this
On 2012/09/13 18:44:45, tbarzic wrote:
> On 2012/09/13 08:14:04, thorogood wrote:
> > On 2012/09/13 04:29:17, tbarzic wrote:
> > > does this mean that std::string() is used to represent "*" matcher?
> > 
> > Almost. You're the wrong way around; this says that if the file doesn't have
> an
> > extension (or we can't find it's mime-type) then we say the file has a mime
> type
> > of "", which is only supported if the Web Intents handler is "*" or "*/*".
> 
> oh, could you please clarify this in the comment.
> 
> also nits:
> could you put the comment in the line before |mime_type = std::string()|
> 
> please add { } for the if block

I've made these changes and made this a bit clearer with an if/else block.

https://chromiumcodereview.appspot.com/10834383/diff/46003/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/46003/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:249: bool
ExtensionSupportsAction(const Extension* extension,
On 2012/09/13 18:44:45, tbarzic wrote:
> nit:
> const Extension* extension should got to the new line
> (args should be vertically aligned)

Done.

[tbarzic, 2012-09-16 04:26:11]

non test code looks good (except for few nits)

I'm still concerned about test not cleaning up created files (in
filebrowser_component extension)

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:248: // If this
method returns true, also returns the |title| of this action.
On 2012/09/16 03:41:18, thorogood wrote:
> On 2012/09/13 18:44:45, tbarzic wrote:
> > On 2012/09/13 09:52:30, benwells wrote:
> > > On 2012/09/13 08:14:04, thorogood wrote:
> > > > On 2012/09/13 04:29:17, tbarzic wrote:
> > > > > why not add function std::string GetTitleForAction()
> > > > > that will return the title of the first intents_service (or first non
> > empty
> > > > > title,, in case it can be ''). 
> > > > 
> > > > I actually started with that, and benwells@ suggested rolling it into
this
> > > > method. I feel like this approach is fine.
> > > 
> > > Either way you have to handle the case where an extension has multiple
view
> > > actions for different mime types, with different titles. In this case you
> need
> > > to look at the mime types you're handling to pick the right title. If
there
> > are
> > > multiple mime types being handled there might be multiple possible titles,
> but
> > > you should always pick a valid one.
> > 
> > yeah, makes sense..
> > I missed continue part in the for loop when I was writing the comment.
> > 
> > Still, I think it would be clearer if we avoided having function that does
two
> > things.. but, do as you like :)
> > 
> > You should probably reflect in the function name that it also returns action
> > title..
> 
> I've changed this method to be named "FindTitleForActionWithTypes", which I
feel
> is also accurate, and updated the comment. Basically you can legitimately
think
> of this method as finding the title of the specified action that supports
these
> mime-types.
> 
> Btw, interestingly enough, since Web Intents are specified as a action/spec
> mapping, then there is a direct mapping between action and title. (It's just
> that if an action supports multiple mime types, this is flattened in the
> Extension initialisation code - so finding the "first" match is fine, since
> they'll all be the same). Ben's comment is thus invalid.
> 
> I know this probably feels like bikeshedding from all directions, but given
the
> above paragraph, I'm happy to break this apart into two methods again if you
> think it's cleaner.

I'm ok with this..

https://chromiumcodereview.appspot.com/10834383/diff/46022/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/46022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:277: return
false;   // we haven't found all mime-types
nit:
I'm more used to seeing comments before if line in these cases..
And comments should end with .

https://chromiumcodereview.appspot.com/10834383/diff/46022/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/46022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.cc:248: || task_type ==
TASK_DRIVE
nit:
|| should be at the end of the previous line.

https://chromiumcodereview.appspot.com/10834383/diff/46022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.cc:257: const size_t
kDriveTaskExtensionPrefixLength =
should these go in anonymous namespace?

https://chromiumcodereview.appspot.com/10834383/diff/46022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.cc:302: || *task_type ==
TASK_DRIVE
nit:
|| should go at the end of previous line

https://chromiumcodereview.appspot.com/10834383/diff/46022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.cc:565: extension_id, //
really app_id
nit:
two spaces before //

https://chromiumcodereview.appspot.com/10834383/diff/46022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.cc:580: // TODO: bigger
explosion than this
nit: '.' at the end.

[benwells, 2012-09-17 01:37:23]

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/41022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:248: // If this
method returns true, also returns the |title| of this action.
On 2012/09/16 04:26:11, tbarzic wrote:
> On 2012/09/16 03:41:18, thorogood wrote:
> > On 2012/09/13 18:44:45, tbarzic wrote:
> > > On 2012/09/13 09:52:30, benwells wrote:
> > > > On 2012/09/13 08:14:04, thorogood wrote:
> > > > > On 2012/09/13 04:29:17, tbarzic wrote:
> > > > > > why not add function std::string GetTitleForAction()
> > > > > > that will return the title of the first intents_service (or first
non
> > > empty
> > > > > > title,, in case it can be ''). 
> > > > > 
> > > > > I actually started with that, and benwells@ suggested rolling it into
> this
> > > > > method. I feel like this approach is fine.
> > > > 
> > > > Either way you have to handle the case where an extension has multiple
> view
> > > > actions for different mime types, with different titles. In this case
you
> > need
> > > > to look at the mime types you're handling to pick the right title. If
> there
> > > are
> > > > multiple mime types being handled there might be multiple possible
titles,
> > but
> > > > you should always pick a valid one.
> > > 
> > > yeah, makes sense..
> > > I missed continue part in the for loop when I was writing the comment.
> > > 
> > > Still, I think it would be clearer if we avoided having function that does
> two
> > > things.. but, do as you like :)
> > > 
> > > You should probably reflect in the function name that it also returns
action
> > > title..
> > 
> > I've changed this method to be named "FindTitleForActionWithTypes", which I
> feel
> > is also accurate, and updated the comment. Basically you can legitimately
> think
> > of this method as finding the title of the specified action that supports
> these
> > mime-types.
> > 
> > Btw, interestingly enough, since Web Intents are specified as a action/spec
> > mapping, then there is a direct mapping between action and title. (It's just
> > that if an action supports multiple mime types, this is flattened in the
> > Extension initialisation code - so finding the "first" match is fine, since
> > they'll all be the same). Ben's comment is thus invalid.
> > 
> > I know this probably feels like bikeshedding from all directions, but given
> the
> > above paragraph, I'm happy to break this apart into two methods again if you
> > think it's cleaner.
> 
> I'm ok with this..

Ah, my bad. I'm OK with either one or two functions since the titles will all be
the same.

[thorogood, 2012-09-18 02:13:02]

Still working on the test fix.

https://chromiumcodereview.appspot.com/10834383/diff/46022/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/46022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:277: return
false;   // we haven't found all mime-types
On 2012/09/16 04:26:11, tbarzic wrote:
> nit:
> I'm more used to seeing comments before if line in these cases..
> And comments should end with .

Done (and I've removed the "we" form, since apparently that's discouraged on
Chrome too).

https://chromiumcodereview.appspot.com/10834383/diff/46022/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/46022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.cc:248: || task_type ==
TASK_DRIVE
On 2012/09/16 04:26:11, tbarzic wrote:
> nit:
> || should be at the end of the previous line.

Done.

https://chromiumcodereview.appspot.com/10834383/diff/46022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.cc:257: const size_t
kDriveTaskExtensionPrefixLength =
On 2012/09/16 04:26:11, tbarzic wrote:
> should these go in anonymous namespace?

Done.

https://chromiumcodereview.appspot.com/10834383/diff/46022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.cc:302: || *task_type ==
TASK_DRIVE
On 2012/09/16 04:26:11, tbarzic wrote:
> nit:
> || should go at the end of previous line

Done.

https://chromiumcodereview.appspot.com/10834383/diff/46022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.cc:565: extension_id, //
really app_id
On 2012/09/16 04:26:11, tbarzic wrote:
> nit:
> two spaces before //

Done.

https://chromiumcodereview.appspot.com/10834383/diff/46022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.cc:580: // TODO: bigger
explosion than this
On 2012/09/16 04:26:11, tbarzic wrote:
> nit: '.' at the end.

I've removed this TODO. It was just a note for me, mostly. I think the
NOTREACHED check is fine since the data should be sanitised at this point.

The alternative check is to handle NULL where we use this call, and make the API
call fail.

[tbarzic, 2012-09-18 03:21:56]

https://chromiumcodereview.appspot.com/10834383/diff/46022/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/46022/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.cc:580: // TODO: bigger
explosion than this
On 2012/09/18 02:13:02, thorogood wrote:
> On 2012/09/16 04:26:11, tbarzic wrote:
> > nit: '.' at the end.
> 
> I've removed this TODO. It was just a note for me, mostly. I think the
> NOTREACHED check is fine since the data should be sanitised at this point.
> 
> The alternative check is to handle NULL where we use this call, and make the
API
> call fail.

this looks ok

[thorogood, 2012-09-19 07:01:58]

On 2012/09/18 03:21:56, tbarzic wrote:
>
https://chromiumcodereview.appspot.com/10834383/diff/46022/chrome/browser/chr...
> File chrome/browser/chromeos/extensions/file_handler_util.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10834383/diff/46022/chrome/browser/chr...
> chrome/browser/chromeos/extensions/file_handler_util.cc:580: // TODO: bigger
> explosion than this
> On 2012/09/18 02:13:02, thorogood wrote:
> > On 2012/09/16 04:26:11, tbarzic wrote:
> > > nit: '.' at the end.
> > 
> > I've removed this TODO. It was just a note for me, mostly. I think the
> > NOTREACHED check is fine since the data should be sanitised at this point.
> > 
> > The alternative check is to handle NULL where we use this call, and make the
> API
> > call fail.
> 
> this looks ok

Hey Toni - I've refactored the test and migrated the TaskType enum to now be
string-based, which is probably easier to grok. I consider this good to go - let
me know!

[thorogood, 2012-09-19 07:07:17]

https://chromiumcodereview.appspot.com/10834383/diff/38026/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://chromiumcodereview.appspot.com/10834383/diff/38026/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_handler_util.cc:241: return
base::StringPrintf("%s|%d|%s",
On 2012/09/13 02:34:49, thorogood wrote:
> On 2012/09/11 02:01:40, tbarzic wrote:
> > I'd rather make task_type string (or, if you keep integers, add a comment in
> > TaskType enum that order of enum values cannot change)
> 
> I agree with you. I've updated this CL with all the other suggestions, and
I'll
> ping it again when I've converted this to a string.

just updating this as Done :)

[tbarzic, 2012-09-19 15:45:13]

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:23: #include
"base/utf_string_conversions.h"
this goes before base/values.h

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:40: #include
"chrome/browser/google_apis/operation_registry.h"
nit:
not in alphabetical order

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:802:
file_extension.substr(1), &mime_type)) {
indent +=2

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:827: extension,
web_intents::kActionView, mime_types, &title))
nit:
indent

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_handler_util.cc:174: const size_t
kDriveTaskExtensionPrefixLength =
nit:
I'd expect these to be at the beginning of the namespace

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_handler_util.cc:1014: :
FileTaskExecutor(profile, extension_id),
nit:
indent += 2 (from line 1014)

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_handler_util.h (right):

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_handler_util.h:28: const char
kTaskFile[] = "file";
extern const char kTaskFile[];

const char kTaskFile[] = "file"; should go into .cc

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_handler_util.h:56: const std::string&
task_type,
optional:
you could have TaskType enum passed to MakeTaksID/CreateTaskID and have utility
functions in .cc that convert the enum from/to string. That way it would be
clearer that |task_type| is restricted to certain values. Just looking at this
function one could think that |task_type| could be an arbitrary string.

I'm ok with the current state too, just add a comment about permitted values for
|task_type|.

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_handler_util.h:132: const std::string
extension_id_;
Lint complains about how there's no #include <string>

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/extensions...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/extensions...
chrome/browser/extensions/extension_special_storage_policy.cc:27: const
std::string& action) {
nit:
args should be vertically aligned

https://codereview.chromium.org/10834383/diff/54001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/filebrowser_component/main.js (right):

https://codereview.chromium.org/10834383/diff/54001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/filebrowser_component/main.js:21: //
|fileVerifierFunction| optional method that will verify test results received
you can revert changes in this file now

https://codereview.chromium.org/10834383/diff/54001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/webintent_handler/background.js
(right):

https://codereview.chromium.org/10834383/diff/54001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/webintent_handler/background.js:16:
chrome.test.assertEq('http://webintents.org/view', intent['action']);
optionally, you can verify the intent file is readable, and that it contains
"hello, world"

https://codereview.chromium.org/10834383/diff/54001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/webintent_handler/background.js:19: // Note
that we're not using chrome.extension.sendRequest here to call back
nit: you probably don't need this comment anymore

[thorogood, 2012-09-20 00:48:17]

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:23: #include
"base/utf_string_conversions.h"
On 2012/09/19 15:45:13, tbarzic wrote:
> this goes before base/values.h

Done, I think this was me being lazy with a merge.

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:40: #include
"chrome/browser/google_apis/operation_registry.h"
On 2012/09/19 15:45:13, tbarzic wrote:
> nit:
> not in alphabetical order

ditto, merge.

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:802:
file_extension.substr(1), &mime_type)) {
On 2012/09/19 15:45:13, tbarzic wrote:
> indent +=2

Done.

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:827: extension,
web_intents::kActionView, mime_types, &title))
On 2012/09/19 15:45:13, tbarzic wrote:
> nit:
> indent

Done.

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_handler_util.cc:174: const size_t
kDriveTaskExtensionPrefixLength =
On 2012/09/19 15:45:13, tbarzic wrote:
> nit:
> I'd expect these to be at the beginning of the namespace

Done.

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_handler_util.cc:1014: :
FileTaskExecutor(profile, extension_id),
On 2012/09/19 15:45:13, tbarzic wrote:
> nit:
> indent += 2 (from line 1014)

Done here and on line 782.

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_handler_util.h (right):

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_handler_util.h:28: const char
kTaskFile[] = "file";
On 2012/09/19 15:45:13, tbarzic wrote:
> extern const char kTaskFile[];
> 
> const char kTaskFile[] = "file"; should go into .cc

Done.

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_handler_util.h:56: const std::string&
task_type,
On 2012/09/19 15:45:13, tbarzic wrote:
> optional:
> you could have TaskType enum passed to MakeTaksID/CreateTaskID and have
utility
> functions in .cc that convert the enum from/to string. That way it would be
> clearer that |task_type| is restricted to certain values. Just looking at this
> function one could think that |task_type| could be an arbitrary string.
> 
> I'm ok with the current state too, just add a comment about permitted values
for
> |task_type|.

I've left it as-is and added a strict comment.

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_handler_util.h:132: const std::string
extension_id_;
On 2012/09/19 15:45:13, tbarzic wrote:
> Lint complains about how there's no #include <string> 

Done.

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/extensions...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/extensions...
chrome/browser/extensions/extension_special_storage_policy.cc:27: const
std::string& action) {
On 2012/09/19 15:45:13, tbarzic wrote:
> nit:
> args should be vertically aligned

Done.

https://codereview.chromium.org/10834383/diff/54001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/filebrowser_component/main.js (right):

https://codereview.chromium.org/10834383/diff/54001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/filebrowser_component/main.js:21: //
|fileVerifierFunction| optional method that will verify test results received
On 2012/09/19 15:45:13, tbarzic wrote:
> you can revert changes in this file now

Done, thanks.

[tbarzic, 2012-09-20 00:52:24]

lgtm

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

https://codereview.chromium.org/10834383/diff/54001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:23: #include
"base/utf_string_conversions.h"
On 2012/09/20 00:48:17, thorogood wrote:
> On 2012/09/19 15:45:13, tbarzic wrote:
> > this goes before base/values.h
> 
> Done, I think this was me being lazy with a merge.

yeah, I know the feeling :)

[commit-bot: I haz the power, 2012-09-20 01:05:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thorogood@chromium.org/10834383/64001

[commit-bot: I haz the power, 2012-09-20 03:29:24]

Change committed as 157693

[memyy, 2012-09-20 03:58:56]

I like good tunk'y.  memyi04@ovi.com

[memyy, 2012-09-20 08:32:46]

2012/9/20, metanata4@gmail.com <metanata4@gmail.com>:
> http://codereview.chromium.org/10834383/
>