Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2559643002:
ARC Media View: Mojo definitions. (Closed)
|
Created:
4 years ago by Shuhei Takahashi Modified:
4 years ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, viettrungluu+watch_chromium.org, hidehiko+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, lhchavez+watch_chromium.org, darin (slow to review), qsr+mojo_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionARC Media View: Mojo definitions.
BUG=chromium:671511
TEST=trybot
Committed: https://crrev.com/c216c500aceb0054b09365f914187594b4bcd7c5
Cr-Commit-Position: refs/heads/master@{#437511}
Patch Set 1 #Patch Set 2 : . #
Total comments: 4
Patch Set 3 : Update comments. #
Total comments: 12
Patch Set 4 : Addressed comments. #Patch Set 5 : Add TODO. #Patch Set 6 : More detailed comments about SAF. #
Messages
Total messages: 35 (16 generated)
The CQ bit was checked by nya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nya@chromium.org changed reviewers: + dcheng@chromium.org, yusukes@chromium.org
yusukes: PTAL dcheng: PTAL for IPC security
nya@chromium.org changed reviewers: + hashimoto@chromium.org
+hashimoto: FYI
https://codereview.chromium.org/2559643002/diff/20001/components/arc/common/f... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2559643002/diff/20001/components/arc/common/f... components/arc/common/file_system.mojom:37: string parent_document_id) => qq #1: Existing methods like GetFileSize() and OpenFileToRead() take URLs (content: and file:) while these new methods take URL components. Isn't it going to be confusing? qq #2: Android's documentation[1] says a content URL consists of an authority, a path, and an ID, while these new methods don't take paths. Why can we ignore paths here? It'd be nice to put the reasoning as comments. [1]: https://developer.android.com/reference/android/content/ContentUris.html
https://codereview.chromium.org/2559643002/diff/20001/components/arc/common/f... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2559643002/diff/20001/components/arc/common/f... components/arc/common/file_system.mojom:37: string parent_document_id) => On 2016/12/07 10:02:22, hashimoto wrote: > qq #1: Existing methods like GetFileSize() and OpenFileToRead() take URLs > (content: and file:) while these new methods take URL components. > Isn't it going to be confusing? We can make IPC interface to solely use content URLs instead of document ID, but I want to use knowledge about authority/documentID in Chrome anyway to write authority-specific logic etc. For example we are discussing to hide "Download" directory in media views because files there are duplicated with ones in Chrome OS Downloads directory. > qq #2: Android's documentation[1] says a content URL consists of an authority, a > path, and an ID, while these new methods don't take paths. Why can we ignore > paths here? It'd be nice to put the reasoning as comments. > > [1]: https://developer.android.com/reference/android/content/ContentUris.html Document ID is a concept of Documents Provider. I added mentions of Documents Provider. I'm not sure if it is clear enough, so I welcome suggestion to update the comment.
Thank you for the explanation. That makes sense. https://codereview.chromium.org/2559643002/diff/20001/components/arc/common/f... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2559643002/diff/20001/components/arc/common/f... components/arc/common/file_system.mojom:37: string parent_document_id) => On 2016/12/07 10:35:32, Shuhei Takahashi wrote: > On 2016/12/07 10:02:22, hashimoto wrote: > > qq #1: Existing methods like GetFileSize() and OpenFileToRead() take URLs > > (content: and file:) while these new methods take URL components. > > Isn't it going to be confusing? > > We can make IPC interface to solely use content URLs instead of document ID, but > I want to use knowledge about authority/documentID in Chrome anyway to write > authority-specific logic etc. For example we are discussing to hide "Download" > directory in media views because files there are duplicated with ones in Chrome > OS Downloads directory. > > > qq #2: Android's documentation[1] says a content URL consists of an authority, > a > > path, and an ID, while these new methods don't take paths. Why can we ignore > > paths here? It'd be nice to put the reasoning as comments. > > > > [1]: https://developer.android.com/reference/android/content/ContentUris.html > > Document ID is a concept of Documents Provider. I added mentions of Documents > Provider. I'm not sure if it is clear enough, so I welcome suggestion to update > the comment. How about putting an Android doc URL? https://developer.android.com/reference/android/provider/DocumentsContract.html
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
drive-by https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... File components/arc/common/file_system.mojom (left): https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... components/arc/common/file_system.mojom:9: // Next method ID: 3 Next method ID: 5 https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... components/arc/common/file_system.mojom:11: [Extensible] nit: only enums need [Extensible].
https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... components/arc/common/file_system.mojom:13: // ID of the document. Does this have any structure, or is it just an opaque identifier? https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... components/arc/common/file_system.mojom:28: uint64 last_modified; Can we use Time from common_custom_types.mojom?
https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... components/arc/common/file_system.mojom:23: // Size of the document in bytes. If the size is unknown, -1 is set. When is the size unknown typically? Example?
PTAL https://codereview.chromium.org/2559643002/diff/20001/components/arc/common/f... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2559643002/diff/20001/components/arc/common/f... components/arc/common/file_system.mojom:37: string parent_document_id) => On 2016/12/07 10:51:00, hashimoto wrote: > On 2016/12/07 10:35:32, Shuhei Takahashi wrote: > > On 2016/12/07 10:02:22, hashimoto wrote: > > > qq #1: Existing methods like GetFileSize() and OpenFileToRead() take URLs > > > (content: and file:) while these new methods take URL components. > > > Isn't it going to be confusing? > > > > We can make IPC interface to solely use content URLs instead of document ID, > but > > I want to use knowledge about authority/documentID in Chrome anyway to write > > authority-specific logic etc. For example we are discussing to hide "Download" > > directory in media views because files there are duplicated with ones in > Chrome > > OS Downloads directory. > > > > > qq #2: Android's documentation[1] says a content URL consists of an > authority, > > a > > > path, and an ID, while these new methods don't take paths. Why can we ignore > > > paths here? It'd be nice to put the reasoning as comments. > > > > > > [1]: > https://developer.android.com/reference/android/content/ContentUris.html > > > > Document ID is a concept of Documents Provider. I added mentions of Documents > > Provider. I'm not sure if it is clear enough, so I welcome suggestion to > update > > the comment. > How about putting an Android doc URL? > https://developer.android.com/reference/android/provider/DocumentsContract.html Thanks, added a link. https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... File components/arc/common/file_system.mojom (left): https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... components/arc/common/file_system.mojom:9: // Next method ID: 3 On 2016/12/07 15:59:05, Luis Héctor Chávez wrote: > Next method ID: 5 Oops, done. https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... components/arc/common/file_system.mojom:11: [Extensible] On 2016/12/07 15:59:05, Luis Héctor Chávez wrote: > nit: only enums need [Extensible]. Done. https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... components/arc/common/file_system.mojom:13: // ID of the document. On 2016/12/07 18:30:48, dcheng wrote: > Does this have any structure, or is it just an opaque identifier? It's an opaque string, described in DocumentsContract.Document reference. https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... components/arc/common/file_system.mojom:23: // Size of the document in bytes. If the size is unknown, -1 is set. On 2016/12/07 18:50:27, Yusuke Sato wrote: > When is the size unknown typically? Example? Added examples (directories and streams). https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... components/arc/common/file_system.mojom:28: uint64 last_modified; On 2016/12/07 18:30:48, dcheng wrote: > Can we use Time from common_custom_types.mojom? Having a typed value sounds pretty, but Time is marked as [Native] and it can be used only from C++. https://www.chromium.org/developers/design-documents/mojo/type-mapping#TOC-Us...
c/arc/test/ lgtm
Just to confirm: this is to allow ARC++ to provide a DocumentsProvider to the Android container? https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... components/arc/common/file_system.mojom:28: uint64 last_modified; On 2016/12/08 05:38:03, Shuhei Takahashi wrote: > On 2016/12/07 18:30:48, dcheng wrote: > > Can we use Time from common_custom_types.mojom? > > Having a typed value sounds pretty, but Time is marked as [Native] and it can be > used only from C++. > https://www.chromium.org/developers/design-documents/mojo/type-mapping#TOC-Us... Would you be willing to file a bug for this and add a TODO here? I'll convert the struct to not be native and fix this in a followup.
On 2016/12/09 08:00:18, dcheng wrote: > Just to confirm: this is to allow ARC++ to provide a DocumentsProvider to the > Android container? > > https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... > File components/arc/common/file_system.mojom (right): > > https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... > components/arc/common/file_system.mojom:28: uint64 last_modified; > On 2016/12/08 05:38:03, Shuhei Takahashi wrote: > > On 2016/12/07 18:30:48, dcheng wrote: > > > Can we use Time from common_custom_types.mojom? > > > > Having a typed value sounds pretty, but Time is marked as [Native] and it can > be > > used only from C++. > > > https://www.chromium.org/developers/design-documents/mojo/type-mapping#TOC-Us... > > Would you be willing to file a bug for this and add a TODO here? I'll convert > the struct to not be native and fix this in a followup. Oh, I should have read the bug, it's the other way. From what I can tell, document IDs are opaque identifiers that shouldn't be parsed: what about authority?
> Oh, I should have read the bug, it's the other way. From what I can tell, > document IDs are opaque identifiers that shouldn't be parsed: what about > authority? You're right, these methods are for querying DocumentsProviders in Android from Chrome. "authority" is the origin part of content:// URL, which is served by DocumentsProvider. It is something like "com.android.providers.media.documents". FYI, Android side change is ag/1690906. https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... components/arc/common/file_system.mojom:28: uint64 last_modified; On 2016/12/09 08:00:18, dcheng wrote: > On 2016/12/08 05:38:03, Shuhei Takahashi wrote: > > On 2016/12/07 18:30:48, dcheng wrote: > > > Can we use Time from common_custom_types.mojom? > > > > Having a typed value sounds pretty, but Time is marked as [Native] and it can > be > > used only from C++. > > > https://www.chromium.org/developers/design-documents/mojo/type-mapping#TOC-Us... > > Would you be willing to file a bug for this and add a TODO here? I'll convert > the struct to not be native and fix this in a followup. Sure, filed a bug: http://crbug.com/672737
On 2016/12/09 08:17:54, Shuhei Takahashi wrote: > > Oh, I should have read the bug, it's the other way. From what I can tell, > > document IDs are opaque identifiers that shouldn't be parsed: what about > > authority? > > You're right, these methods are for querying DocumentsProviders in Android from > Chrome. > > "authority" is the origin part of content:// URL, which is served by > DocumentsProvider. It is something like "com.android.providers.media.documents". > It might be helpful to include this information in the mojom (I couldn't find it in the linked docs, maybe I'm not looking in the right place) Just to confirm... we won't be doing manual parsing of this? > FYI, Android side change is ag/1690906. > > https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... > File components/arc/common/file_system.mojom (right): > > https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... > components/arc/common/file_system.mojom:28: uint64 last_modified; > On 2016/12/09 08:00:18, dcheng wrote: > > On 2016/12/08 05:38:03, Shuhei Takahashi wrote: > > > On 2016/12/07 18:30:48, dcheng wrote: > > > > Can we use Time from common_custom_types.mojom? > > > > > > Having a typed value sounds pretty, but Time is marked as [Native] and it > can > > be > > > used only from C++. > > > > > > https://www.chromium.org/developers/design-documents/mojo/type-mapping#TOC-Us... > > > > Would you be willing to file a bug for this and add a TODO here? I'll convert > > the struct to not be native and fix this in a followup. > > Sure, filed a bug: http://crbug.com/672737
On 2016/12/09 08:19:53, dcheng wrote: > On 2016/12/09 08:17:54, Shuhei Takahashi wrote: > > > Oh, I should have read the bug, it's the other way. From what I can tell, > > > document IDs are opaque identifiers that shouldn't be parsed: what about > > > authority? > > > > You're right, these methods are for querying DocumentsProviders in Android > from > > Chrome. > > > > "authority" is the origin part of content:// URL, which is served by > > DocumentsProvider. It is something like > "com.android.providers.media.documents". > > > > It might be helpful to include this information in the mojom (I couldn't find it > in the linked docs, maybe I'm not looking in the right place) > Just to confirm... we won't be doing manual parsing of this? In Chrome side we treat |authority| and |document_id| as opaque strings and never try to parse them. In Android side, they are used to construct a content URI like content://<authority>/document/<document_id>/children. > > FYI, Android side change is ag/1690906. > > > > > https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... > > File components/arc/common/file_system.mojom (right): > > > > > https://codereview.chromium.org/2559643002/diff/40001/components/arc/common/f... > > components/arc/common/file_system.mojom:28: uint64 last_modified; > > On 2016/12/09 08:00:18, dcheng wrote: > > > On 2016/12/08 05:38:03, Shuhei Takahashi wrote: > > > > On 2016/12/07 18:30:48, dcheng wrote: > > > > > Can we use Time from common_custom_types.mojom? > > > > > > > > Having a typed value sounds pretty, but Time is marked as [Native] and it > > can > > > be > > > > used only from C++. > > > > > > > > > > https://www.chromium.org/developers/design-documents/mojo/type-mapping#TOC-Us... > > > > > > Would you be willing to file a bug for this and add a TODO here? I'll > convert > > > the struct to not be native and fix this in a followup. > > > > Sure, filed a bug: http://crbug.com/672737
mojo lgtm
The CQ bit was checked by nya@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusukes@chromium.org Link to the patchset: https://codereview.chromium.org/2559643002/#ps100001 (title: "More detailed comments about SAF.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481276976184160,
"parent_rev": "f2eb151d07d91d2458219a3f66df0b138d1643e9", "commit_rev":
"a6e159ebb48efbf82ddc26f85b7bacb88934be01"}
Message was sent while issue was closed.
Description was changed from ========== ARC Media View: Mojo definitions. BUG=chromium:671511 TEST=trybot ========== to ========== ARC Media View: Mojo definitions. BUG=chromium:671511 TEST=trybot Review-Url: https://codereview.chromium.org/2559643002 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== ARC Media View: Mojo definitions. BUG=chromium:671511 TEST=trybot Review-Url: https://codereview.chromium.org/2559643002 ========== to ========== ARC Media View: Mojo definitions. BUG=chromium:671511 TEST=trybot Committed: https://crrev.com/c216c500aceb0054b09365f914187594b4bcd7c5 Cr-Commit-Position: refs/heads/master@{#437511} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c216c500aceb0054b09365f914187594b4bcd7c5 Cr-Commit-Position: refs/heads/master@{#437511} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
