Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(19)

Issue 294163010: Refactor "IsUnderDriveMountPoint" in v2 app code for generalization to non-Drive volumes. (Closed)

Created:
6 years, 7 months ago by kinaba
Modified:
6 years, 6 months ago
Reviewers:
mtomasz, benwells
CC:
chromium-reviews, extensions-reviews_chromium.org, tzik, tfarina, nhiroki, chromium-apps-reviews_chromium.org, kinuko+watch, hashimoto
Visibility:
Public.

Description

Refactor "IsUnderDriveMountPoint" in v2 app code for generalization to non-Drive volumes. Chrome OS file manager is going to support more types of volumes in addition to Google Drive that does not mapped to the local filesystem. (E.g., volumes provided by extensions via fileSystemProvider API, cloud devices, MTP, ...) This CL extracts handling of such non-local files in apps fileSystem API code and removes direct dependence to Google Drive code. BUG=367028 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273434

Patch Set 1 : #

Total comments: 6

Patch Set 2 : Review fix #

Patch Set 3 : Fix test failure #

Patch Set 4 : Rename #

Unified diffs Side-by-side diffs Delta from patch set Stats (+212 lines, -82 lines) Patch
M apps/DEPS View 1 chunk +1 line, -1 line 0 comments Download
M apps/launcher.cc View 1 2 3 3 chunks +9 lines, -31 lines 0 comments Download
A chrome/browser/chromeos/file_manager/filesystem_api_util.h View 1 2 3 1 chunk +53 lines, -0 lines 0 comments Download
A chrome/browser/chromeos/file_manager/filesystem_api_util.cc View 1 2 3 1 chunk +109 lines, -0 lines 0 comments Download
M chrome/browser/extensions/api/file_handlers/app_file_handler_util.cc View 1 2 3 4 chunks +16 lines, -34 lines 0 comments Download
M chrome/browser/extensions/api/file_system/file_system_api.h View 1 2 1 chunk +6 lines, -5 lines 0 comments Download
M chrome/browser/extensions/api/file_system/file_system_api.cc View 1 2 3 4 chunks +16 lines, -11 lines 0 comments Download
M chrome/chrome_browser_chromeos.gypi View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 18 (0 generated)
kinaba
@benwells, @mtomasz: PTAL. In next patch(es), I'll add handling for other types of file systems ...
6 years, 7 months ago (2014-05-26 02:21:59 UTC) #1
mtomasz
Basically lgtm, but one concern about the code location. https://codereview.chromium.org/294163010/diff/40001/apps/DEPS File apps/DEPS (right): https://codereview.chromium.org/294163010/diff/40001/apps/DEPS#newcode20 apps/DEPS:20: ...
6 years, 7 months ago (2014-05-26 05:54:14 UTC) #2
benwells
apps/ lgtm with the code #if'd out. https://codereview.chromium.org/294163010/diff/40001/apps/launcher.cc File apps/launcher.cc (right): https://codereview.chromium.org/294163010/diff/40001/apps/launcher.cc#newcode215 apps/launcher.cc:215: void OnGotMimeType(bool ...
6 years, 7 months ago (2014-05-27 03:21:19 UTC) #3
kinaba
https://codereview.chromium.org/294163010/diff/40001/apps/DEPS File apps/DEPS (right): https://codereview.chromium.org/294163010/diff/40001/apps/DEPS#newcode20 apps/DEPS:20: "+chrome/browser/chromeos/file_manager/filesystem_api_util.h", On 2014/05/26 05:54:15, mtomasz wrote: > I'm not ...
6 years, 7 months ago (2014-05-27 05:39:27 UTC) #4
mtomasz
lgtm lgtm! https://codereview.chromium.org/294163010/diff/40001/apps/DEPS File apps/DEPS (right): https://codereview.chromium.org/294163010/diff/40001/apps/DEPS#newcode20 apps/DEPS:20: "+chrome/browser/chromeos/file_manager/filesystem_api_util.h", On 2014/05/27 05:39:28, kinaba wrote: > ...
6 years, 7 months ago (2014-05-27 05:40:18 UTC) #5
kinaba
The CQ bit was checked by kinaba@chromium.org
6 years, 7 months ago (2014-05-27 05:43:17 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kinaba@chromium.org/294163010/60001
6 years, 7 months ago (2014-05-27 05:43:32 UTC) #7
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are ...
6 years, 7 months ago (2014-05-27 07:29:24 UTC) #8
kinaba
The CQ bit was unchecked by kinaba@chromium.org
6 years, 7 months ago (2014-05-27 07:31:37 UTC) #9
kinaba
On 2014/05/27 07:29:24, I haz the power (commit-bot) wrote: > FYI, CQ is re-trying this ...
6 years, 7 months ago (2014-05-27 07:31:53 UTC) #10
kinaba
On 2014/05/27 07:31:53, kinaba wrote: > On 2014/05/27 07:29:24, I haz the power (commit-bot) wrote: ...
6 years, 7 months ago (2014-05-27 07:55:29 UTC) #11
kinaba
CC: hashimoto
6 years, 7 months ago (2014-05-27 08:40:08 UTC) #12
kinaba
Updated the function names per the review in other CL (304503007). @benwells, could you mind ...
6 years, 6 months ago (2014-05-28 22:54:10 UTC) #13
benwells
slgtm
6 years, 6 months ago (2014-05-29 01:32:03 UTC) #14
kinaba
On 2014/05/29 01:32:03, benwells wrote: > slgtm Thanks!
6 years, 6 months ago (2014-05-29 01:33:57 UTC) #15
kinaba
The CQ bit was checked by kinaba@chromium.org
6 years, 6 months ago (2014-05-29 01:34:02 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kinaba@chromium.org/294163010/90001
6 years, 6 months ago (2014-05-29 01:35:05 UTC) #17
commit-bot: I haz the power
6 years, 6 months ago (2014-05-29 01:50:31 UTC) #18
Message was sent while issue was closed.
Change committed as 273434

Powered by Google App Engine
This is Rietveld 408576698