Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10869067:
Deliver attach intent to packaged apps (Closed)
|
Created:
8 years, 4 months ago by thorogood Modified:
8 years, 3 months ago Reviewers:
vandebo (ex-Chrome), benwells, kmadhusu, kinuko, James Hawkins, not at google - send to devlin CC:
chromium-reviews, mihaip-chromium-reviews_chromium.org, Aaron Boodman, groby+watch_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org, gbillock+watch_chromium.org, smckay+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionThis forwards through a DOMFileSystem of the attached media device to interested packaged apps, via Web Intent data.
BUG=144520
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154112
Patch Set 1 #Patch Set 2 : Delivers DOMFileSystem to packaged apps #Patch Set 3 : Reverted some extraneous stuff #Patch Set 4 : r #
Total comments: 3
Patch Set 5 : sync to head #
Total comments: 6
Patch Set 6 : comment #
Total comments: 2
Patch Set 7 : DCHECK #Patch Set 8 : Allowed file contents to actually be read #
Total comments: 12
Patch Set 9 : Sync to head, jhawkins comments #Patch Set 10 : Fixes build error -- oops #
Messages
Total messages: 28 (0 generated)
https://chromiumcodereview.appspot.com/10869067/diff/5001/chrome/browser/exte... File chrome/browser/extensions/api/app_runtime/app_runtime_api.cc (right): https://chromiumcodereview.appspot.com/10869067/diff/5001/chrome/browser/exte... chrome/browser/extensions/api/app_runtime/app_runtime_api.cc:94: intent_data->SetString("baseName", web_intent_data.root_name); I don't understand why we need to send the base name here as well ... we don't for the FileEntry case.
chrome/browser/resources LGTM
https://chromiumcodereview.appspot.com/10869067/diff/5001/chrome/browser/exte... File chrome/browser/extensions/api/app_runtime/app_runtime_api.cc (right): https://chromiumcodereview.appspot.com/10869067/diff/5001/chrome/browser/exte... chrome/browser/extensions/api/app_runtime/app_runtime_api.cc:94: intent_data->SetString("baseName", web_intent_data.root_name); On 2012/08/27 08:25:51, benwells wrote: > I don't understand why we need to send the base name here as well ... we don't > for the FileEntry case. We also send it for fileEntry? The baseName is the name that we're trying to serve from within the isolated filesystem. (Notably, in both cases below, the baseName and fileSystemId are stripped before sending to the client). In the fileEntry case, it is the _only_ file that exists in the FS, and we want to deliver it directly. In the filesystem (this) case, it is the only _directory_ that exists in the FS, and we want to create another fake FS that exists at that directory.
This is still ready for review. PTAL. https://chromiumcodereview.appspot.com/10869067/diff/5001/chrome/browser/exte... File chrome/browser/extensions/api/app_runtime/app_runtime_api.cc (right): https://chromiumcodereview.appspot.com/10869067/diff/5001/chrome/browser/exte... chrome/browser/extensions/api/app_runtime/app_runtime_api.cc:94: intent_data->SetString("baseName", web_intent_data.root_name); On 2012/08/27 22:45:54, thorogood wrote: > On 2012/08/27 08:25:51, benwells wrote: > > I don't understand why we need to send the base name here as well ... we don't > > for the FileEntry case. > > We also send it for fileEntry? The baseName is the name that we're trying to > serve from within the isolated filesystem. (Notably, in both cases below, the > baseName and fileSystemId are stripped before sending to the client). > > In the fileEntry case, it is the _only_ file that exists in the FS, and we want > to deliver it directly. > > In the filesystem (this) case, it is the only _directory_ that exists in the FS, > and we want to create another fake FS that exists at that directory. I've discussed this with Ben, and confirmed that delivering filesystem intents to hosted apps work in the same way as I'm building for packaged apps. As discussed, the way an attached intent works is: + create virtual filesystem (empty) + add 'link' to the media device, as a single folder within the vfs + create a DOMFileSystem object (from app_runtime_custom_bindings.js) that refers to the folder *within* the vfs + send that DOMFileSystem to the client
lgtm, but remove the commented out security code if it isn't needed (or uncomment it if it is). https://chromiumcodereview.appspot.com/10869067/diff/10001/chrome/browser/ext... File chrome/browser/extensions/platform_app_launcher.cc (right): https://chromiumcodereview.appspot.com/10869067/diff/10001/chrome/browser/ext... chrome/browser/extensions/platform_app_launcher.cc:296: // TODO(thorogood): We may need to revisit this. Kinuko: is this security stuff needed still? https://chromiumcodereview.appspot.com/10869067/diff/10001/chrome/browser/med... File chrome/browser/media_gallery/media_storage_util.cc (right): https://chromiumcodereview.appspot.com/10869067/diff/10001/chrome/browser/med... chrome/browser/media_gallery/media_storage_util.cc:106: if (prefix_length != std::string::npos) { Are there instances where there are no colons in the device ID? Could you add a comment about why this happens?
https://chromiumcodereview.appspot.com/10869067/diff/10001/chrome/browser/ext... File chrome/browser/extensions/platform_app_launcher.cc (right): https://chromiumcodereview.appspot.com/10869067/diff/10001/chrome/browser/ext... chrome/browser/extensions/platform_app_launcher.cc:296: // TODO(thorogood): We may need to revisit this. On 2012/08/28 08:50:51, benwells wrote: > Kinuko: is this security stuff needed still? Yes.. unfortunately we still need them.
On 2012/08/28 10:58:33, kinuko wrote: > https://chromiumcodereview.appspot.com/10869067/diff/10001/chrome/browser/ext... > File chrome/browser/extensions/platform_app_launcher.cc (right): > > https://chromiumcodereview.appspot.com/10869067/diff/10001/chrome/browser/ext... > chrome/browser/extensions/platform_app_launcher.cc:296: // TODO(thorogood): We > may need to revisit this. > On 2012/08/28 08:50:51, benwells wrote: > > Kinuko: is this security stuff needed still? > > Yes.. unfortunately we still need them. web intents stuff looks good lgtm
https://chromiumcodereview.appspot.com/10869067/diff/10001/chrome/browser/ext... File chrome/browser/extensions/platform_app_launcher.cc (right): https://chromiumcodereview.appspot.com/10869067/diff/10001/chrome/browser/ext... chrome/browser/extensions/platform_app_launcher.cc:296: // TODO(thorogood): We may need to revisit this. On 2012/08/28 10:58:33, kinuko wrote: > On 2012/08/28 08:50:51, benwells wrote: > > Kinuko: is this security stuff needed still? > > Yes.. unfortunately we still need them. Hi Kinuko, I seem to still be able to read the contents of the virtual file system despite not granting directory permissions. I'll look again tomorrow to try a few different approaches though to see if I'm actually doing something wrong. Do you have any comments on the other parts of this patch? Ben tells me you're quite authoritative in this area. :) https://chromiumcodereview.appspot.com/10869067/diff/10001/chrome/browser/med... File chrome/browser/media_gallery/media_storage_util.cc (right): https://chromiumcodereview.appspot.com/10869067/diff/10001/chrome/browser/med... chrome/browser/media_gallery/media_storage_util.cc:106: if (prefix_length != std::string::npos) { On 2012/08/28 08:50:51, benwells wrote: > Are there instances where there are no colons in the device ID? Could you add a > comment about why this happens? I don't believe so. The device_id is only something we build internally (where we guarantee it contains a colon), pass around, and then want to crack apart again - it's not persisted or anything. Note that this was previously a bug - all the prefix types (e.g., kUsbMassStorageNoDCIMPrefix) contain the colon, so the prefix we were obtaining didn't contain that, and we'd always hit NOTREACHED below. The check (the if statement) is mostly because we search for the colon, but grab a character past that, so that would be bad if we were at std::string::npos... I've added a short comment for clarification.
https://chromiumcodereview.appspot.com/10869067/diff/15001/chrome/browser/med... File chrome/browser/media_gallery/media_storage_util.cc (right): https://chromiumcodereview.appspot.com/10869067/diff/15001/chrome/browser/med... chrome/browser/media_gallery/media_storage_util.cc:108: if (prefix_length != std::string::npos) { Nit: This should be a DCHECK then. It will be more obvious to fail here than in the not_reached below, if there is no colon.
https://chromiumcodereview.appspot.com/10869067/diff/15001/chrome/browser/med... File chrome/browser/media_gallery/media_storage_util.cc (right): https://chromiumcodereview.appspot.com/10869067/diff/15001/chrome/browser/med... chrome/browser/media_gallery/media_storage_util.cc:108: if (prefix_length != std::string::npos) { On 2012/08/29 00:00:14, benwells wrote: > Nit: This should be a DCHECK then. It will be more obvious to fail here than in > the not_reached below, if there is no colon. Done.
https://chromiumcodereview.appspot.com/10869067/diff/10001/chrome/browser/ext... File chrome/browser/extensions/platform_app_launcher.cc (right): https://chromiumcodereview.appspot.com/10869067/diff/10001/chrome/browser/ext... chrome/browser/extensions/platform_app_launcher.cc:296: // TODO(thorogood): We may need to revisit this. On 2012/08/28 23:48:11, thorogood wrote: > On 2012/08/28 10:58:33, kinuko wrote: > > On 2012/08/28 08:50:51, benwells wrote: > > > Kinuko: is this security stuff needed still? > > > > Yes.. unfortunately we still need them. > > Hi Kinuko, I seem to still be able to read the contents of the virtual file > system despite not granting directory permissions. I'll look again tomorrow to > try a few different approaches though to see if I'm actually doing something > wrong. > > Do you have any comments on the other parts of this patch? Ben tells me you're > quite authoritative in this area. :) Sorry - you were right. I was able to iterate through the top-level directories in the filesystem, but nothing else. Fixed now.
https://chromiumcodereview.appspot.com/10869067/diff/16002/chrome/browser/ext... File chrome/browser/extensions/platform_app_launcher.cc (right): https://chromiumcodereview.appspot.com/10869067/diff/16002/chrome/browser/ext... chrome/browser/extensions/platform_app_launcher.cc:234: class PlatformAppGrantIntentLauncher I don't understand the name of this class, specifically the word 'grant' here. It's a launcher that launches a GrantIntent? https://chromiumcodereview.appspot.com/10869067/diff/16002/chrome/browser/med... File chrome/browser/media_gallery/media_storage_util.cc (right): https://chromiumcodereview.appspot.com/10869067/diff/16002/chrome/browser/med... chrome/browser/media_gallery/media_storage_util.cc:104: // The device ID is always generated by MakeDeviceId, so we should always nit: Don't use pronouns (we) in comments; pronouns are ambiguous. https://chromiumcodereview.appspot.com/10869067/diff/16002/chrome/browser/med... chrome/browser/media_gallery/media_storage_util.cc:108: std::string prefix = device_id.substr(0, prefix_length + 1); What this a bug previously? https://chromiumcodereview.appspot.com/10869067/diff/16002/chrome/browser/med... File chrome/browser/media_gallery/media_storage_util_unittest.cc (right): https://chromiumcodereview.appspot.com/10869067/diff/16002/chrome/browser/med... chrome/browser/media_gallery/media_storage_util_unittest.cc:17: TEST_F(MediaStorageUtilTest, DeviceId) { nit: Document what this is testing. https://chromiumcodereview.appspot.com/10869067/diff/16002/chrome/browser/med... chrome/browser/media_gallery/media_storage_util_unittest.cc:22: ASSERT_TRUE(MediaStorageUtil::IsMediaDevice(device_id)); All of these should be EXPECT_TRUE. https://chromiumcodereview.appspot.com/10869067/diff/16002/chrome/renderer/ex... File chrome/renderer/extensions/file_system_natives.cc (right): https://chromiumcodereview.appspot.com/10869067/diff/16002/chrome/renderer/ex... chrome/renderer/extensions/file_system_natives.cc:42: // Optionally create the DOMFileSystem rooted at a subdirectory. Optionally?
+vandebo for media gallery.
Do you need to rebase again? The MediaStorageUtil bug has been fixed and a unit test file already added.
+kmadhusu@ as she's most familiar with the device attach intent.
https://chromiumcodereview.appspot.com/10869067/diff/16002/chrome/browser/ext... File chrome/browser/extensions/platform_app_launcher.cc (right): https://chromiumcodereview.appspot.com/10869067/diff/16002/chrome/browser/ext... chrome/browser/extensions/platform_app_launcher.cc:234: class PlatformAppGrantIntentLauncher On 2012/08/29 22:46:51, James Hawkins wrote: > I don't understand the name of this class, specifically the word 'grant' here. > It's a launcher that launches a GrantIntent? After some discussion here in Sydney, we've changed this to PlatformAppWebIntentLauncher, and ensured that all launch actions with intents pass through this class, even if some short-circuit and deliver immediately. So the name should now be clearer. Grant was confusing :) https://chromiumcodereview.appspot.com/10869067/diff/16002/chrome/browser/med... File chrome/browser/media_gallery/media_storage_util.cc (right): https://chromiumcodereview.appspot.com/10869067/diff/16002/chrome/browser/med... chrome/browser/media_gallery/media_storage_util.cc:104: // The device ID is always generated by MakeDeviceId, so we should always On 2012/08/29 22:46:51, James Hawkins wrote: > nit: Don't use pronouns (we) in comments; pronouns are ambiguous. Thanks, although this has been fixed by another patch (which has no comment), so it's gone now. https://chromiumcodereview.appspot.com/10869067/diff/16002/chrome/browser/med... chrome/browser/media_gallery/media_storage_util.cc:108: std::string prefix = device_id.substr(0, prefix_length + 1); On 2012/08/29 22:46:51, James Hawkins wrote: > What this a bug previously? Yes. It's been fixed by another patch, so you can ignore this. https://chromiumcodereview.appspot.com/10869067/diff/16002/chrome/browser/med... File chrome/browser/media_gallery/media_storage_util_unittest.cc (right): https://chromiumcodereview.appspot.com/10869067/diff/16002/chrome/browser/med... chrome/browser/media_gallery/media_storage_util_unittest.cc:17: TEST_F(MediaStorageUtilTest, DeviceId) { On 2012/08/29 22:46:51, James Hawkins wrote: > nit: Document what this is testing. Thanks, although someone else went and wrote this under me, so ignore this file. https://chromiumcodereview.appspot.com/10869067/diff/16002/chrome/browser/med... chrome/browser/media_gallery/media_storage_util_unittest.cc:22: ASSERT_TRUE(MediaStorageUtil::IsMediaDevice(device_id)); On 2012/08/29 22:46:51, James Hawkins wrote: > All of these should be EXPECT_TRUE. I'm no longer touching this file. https://chromiumcodereview.appspot.com/10869067/diff/16002/chrome/renderer/ex... File chrome/renderer/extensions/file_system_natives.cc (right): https://chromiumcodereview.appspot.com/10869067/diff/16002/chrome/renderer/ex... chrome/renderer/extensions/file_system_natives.cc:42: // Optionally create the DOMFileSystem rooted at a subdirectory. On 2012/08/29 22:46:51, James Hawkins wrote: > Optionally? I've expanded this comment slightly.
lgtm
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thorogood@chromium.org/10869067/14011
Try job failure for 10869067-14011 (retry) on mac_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thorogood@chromium.org/10869067/13004
Try job failure for 10869067-13004 (retry) (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thorogood@chromium.org/10869067/13004
Try job failure for 10869067-13004 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thorogood@chromium.org/10869067/13004
Change committed as 154112 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
