Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2464333004:
arc: Convert content: and file: URLs from ARC to externalfile: URLs (Closed)
Descriptionarc: Convert content: and file: URLs from ARC to externalfile: URLs
BUG=654684
Committed: https://crrev.com/4fd6d1e812d0848c8cf817da160664e548ba3a87
Cr-Commit-Position: refs/heads/master@{#432415}
Patch Set 1 #
Total comments: 2
Depends on Patchset: Messages
Total messages: 21 (11 generated)
The CQ bit was checked by hashimoto@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...
hashimoto@chromium.org changed reviewers: + hidehiko@chromium.org
Sorry for spamming you with CLs. Please review these CLs at your convenience. This is part of a series of CLs: 1. https://codereview.chromium.org/2469963003 Add methods to IntentHelperInstance to read ARC contents 2. https://codereview.chromium.org/2464333003 Add utility functions to implement ARC content file system 3. https://codereview.chromium.org/2469243002 Implement ArcContentFileSystemAsyncFileUtil::GetFileInfo 4. https://codereview.chromium.org/2470843004 Implement ArcContentFileSystemFileStreamReader::Read/GetLength 5. https://codereview.chromium.org/2464333004 Convert content: and file: URLs from ARC to externalfile: URLs <- THIS
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM, but you'll need review by an OWNER.
hashimoto@chromium.org changed reviewers: + oshima@chromium.org
oshima, Could you review this change as an owner of this file?
lgtm https://codereview.chromium.org/2464333004/diff/1/chrome/browser/ui/ash/chrom... File chrome/browser/ui/ash/chrome_shell_delegate.cc (right): https://codereview.chromium.org/2464333004/diff/1/chrome/browser/ui/ash/chrom... chrome/browser/ui/ash/chrome_shell_delegate.cc:410: if (!url.is_valid()) Just curious. Is there any reason why we don't show any UI when the invalid url is passed here?
https://codereview.chromium.org/2464333004/diff/1/chrome/browser/ui/ash/chrom... File chrome/browser/ui/ash/chrome_shell_delegate.cc (right): https://codereview.chromium.org/2464333004/diff/1/chrome/browser/ui/ash/chrom... chrome/browser/ui/ash/chrome_shell_delegate.cc:410: if (!url.is_valid()) On 2016/11/11 01:13:25, oshima wrote: > Just curious. Is there any reason why we don't show any UI when the invalid url > is passed here? This code was introduced by yusukes@, I have no idea. https://codereview.chromium.org/1590723003/diff/1/chrome/browser/chromeos/arc... However, in practice the URL here is the one given as part of an Intent so it should be always valid. So I don't see any problems in silently ignoring invalid ones. (maybe some logging might be helpful?)
The CQ bit was checked by hashimoto@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== arc: Convert content: and file: URLs from ARC to externalfile: URLs BUG=654684 ========== to ========== arc: Convert content: and file: URLs from ARC to externalfile: URLs BUG=654684 Committed: https://crrev.com/4fd6d1e812d0848c8cf817da160664e548ba3a87 Cr-Commit-Position: refs/heads/master@{#432415} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/4fd6d1e812d0848c8cf817da160664e548ba3a87 Cr-Commit-Position: refs/heads/master@{#432415}
Message was sent while issue was closed.
vkuzkokov@chromium.org changed reviewers: + vkuzkokov@chromium.org
Message was sent while issue was closed.
This breaks ARC printing. https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/print/arc_pr... Is there still a way to use actual file: from print service?
Message was sent while issue was closed.
Description was changed from ========== arc: Convert content: and file: URLs from ARC to externalfile: URLs BUG=654684 Committed: https://crrev.com/4fd6d1e812d0848c8cf817da160664e548ba3a87 Cr-Commit-Position: refs/heads/master@{#432415} ========== to ========== arc: Convert content: and file: URLs from ARC to externalfile: URLs BUG=654684 Committed: https://crrev.com/4fd6d1e812d0848c8cf817da160664e548ba3a87 Cr-Commit-Position: refs/heads/master@{#432415} ==========
Message was sent while issue was closed.
vkuzkokov@chromium.org changed reviewers: + poromov@chromium.org
Message was sent while issue was closed.
On 2017/08/03 09:28:06, vkuzkokov wrote: > This breaks ARC printing. > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/print/arc_pr... > Is there still a way to use actual file: from print service? Sorry for causing you trouble. This OpenUrlFromArc() implementation handles the given file: URL as one in the ARC container which chrome cannot access directly, so it converts the ARC container file: URL to an externalfile: URL which will be handled by ExternalFileURLRequestJob & ArcContentFileSystemFileStreamReader. Instead of OpenUrlFromArc(), how about using platform_util::OpenItem() to open the PDF file? https://chromium.googlesource.com/chromium/src/+/8a5b373dc09c2d8667a744df0dea... Because your ArcPrintService code is located under chrome/browser, you don't necessarily have to use ShellDelegate to open a new tab. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
