Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2806443002:
arc: Add IsArcAllowedInAppListForProfile. (Closed)
|
Created:
3 years, 8 months ago by kinaba Modified:
3 years, 8 months ago Reviewers:
hidehiko CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, oshima+watch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: Add IsArcAllowedInAppListForProfile.
This will be used to keep app list even when ARC is disabled due to
filesystem incompatibility. This CL just splits IsArcAllowedForProfile
for use in subsequent changes.
BUG=699850
TEST=ChromeArcUtilTest
Review-Url: https://codereview.chromium.org/2806443002
Cr-Commit-Position: refs/heads/master@{#463255}
Committed: https://chromium.googlesource.com/chromium/src/+/21c492878576af55fb89bf56efd9cbed20ef4eaa
Patch Set 1 #Patch Set 2 : Test #
Total comments: 2
Patch Set 3 : Rename #
Messages
Total messages: 29 (17 generated)
The CQ bit was checked by kinaba@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...
kinaba@chromium.org changed reviewers: + hidehiko@chromium.org
As we've discussed today. PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The code itself looks good. Could you add tests?
On 2017/04/06 12:31:57, hidehiko wrote:
> The code itself looks good. Could you add tests?
Hm, how the tests should look like?
All EXPECT_{TRUE,FALSE}(IsArcAllowedForProfile()) lines replicated for the new
function?
On 2017/04/06 12:45:39, kinaba wrote:
> On 2017/04/06 12:31:57, hidehiko wrote:
> > The code itself looks good. Could you add tests?
>
> Hm, how the tests should look like?
> All EXPECT_{TRUE,FALSE}(IsArcAllowedForProfile()) lines replicated for the new
> function?
Good point. Maybe;
- add a test of FS check for IsArcAllowedForProfile(), and
- a few dup (e.g only success case) + FS check won't work for new function,
which is the key diff from IsArcAllowedForProfile()?
On 2017/04/06 13:05:04, hidehiko wrote:
> On 2017/04/06 12:45:39, kinaba wrote:
> > On 2017/04/06 12:31:57, hidehiko wrote:
> > > The code itself looks good. Could you add tests?
> >
> > Hm, how the tests should look like?
> > All EXPECT_{TRUE,FALSE}(IsArcAllowedForProfile()) lines replicated for the
new
> > function?
>
> Good point. Maybe;
> - add a test of FS check for IsArcAllowedForProfile(), and
> - a few dup (e.g only success case) + FS check won't work for new function,
> which is the key diff from IsArcAllowedForProfile()?
OK, let me think a bit.
"FS check won't work for new function," is not much meaningful unless we move
the check out of IsRunningOnChromeOS(),
but that's a bit tough because I don't want the test behavior to change
depending on the workstations filesystem or lsb-release content.
I'll at least do the rest (by pulling the FS check part inside
IsRunningOnChromeOS out as a separate function; prefs and lsb-release are
modifiable for testing it.)
Give me some time for the last point (which I agree that it makes sense.)
On 2017/04/06 13:24:59, kinaba wrote:
> On 2017/04/06 13:05:04, hidehiko wrote:
> > On 2017/04/06 12:45:39, kinaba wrote:
> > > On 2017/04/06 12:31:57, hidehiko wrote:
> > > > The code itself looks good. Could you add tests?
> > >
> > > Hm, how the tests should look like?
> > > All EXPECT_{TRUE,FALSE}(IsArcAllowedForProfile()) lines replicated for the
> new
> > > function?
> >
> > Good point. Maybe;
> > - add a test of FS check for IsArcAllowedForProfile(), and
> > - a few dup (e.g only success case) + FS check won't work for new function,
> > which is the key diff from IsArcAllowedForProfile()?
>
> OK, let me think a bit.
> "FS check won't work for new function," is not much meaningful unless we move
> the check out of IsRunningOnChromeOS(),
> but that's a bit tough because I don't want the test behavior to change
> depending on the workstations filesystem or lsb-release content.
> I'll at least do the rest (by pulling the FS check part inside
> IsRunningOnChromeOS out as a separate function; prefs and lsb-release are
> modifiable for testing it.)
> Give me some time for the last point (which I agree that it makes sense.)
SG. Please keep TODO then. Thank you in advance!
The CQ bit was checked by kinaba@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.
Added tests. PTanotherL.
Thank you for adding precise tests! One more minor comment. https://codereview.chromium.org/2806443002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_util.h (right): https://codereview.chromium.org/2806443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.h:40: bool IsProfileVerifiedToBeOnArcCompatibleFilesystem(const Profile* profile); Functions here is started with "Be-verb + Arc", how about following it? E.g. IsArcCompatibleFileSystemUsedForProfile()?
The CQ bit was checked by kinaba@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...
https://codereview.chromium.org/2806443002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_util.h (right): https://codereview.chromium.org/2806443002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.h:40: bool IsProfileVerifiedToBeOnArcCompatibleFilesystem(const Profile* profile); On 2017/04/10 08:50:55, hidehiko wrote: > Functions here is started with "Be-verb + Arc", how about following it? > E.g. IsArcCompatibleFileSystemUsedForProfile()? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== arc: Add IsArcAllowedInAppListForProfile. This will be used to keep app list even when ARC is disabled due to filesystem incompatibility. This CL just splits IsArcAllowedForProfile for use in subsequent changes. BUG=699850 TEST=(existing ones. no behavior change intended.) ========== to ========== arc: Add IsArcAllowedInAppListForProfile. This will be used to keep app list even when ARC is disabled due to filesystem incompatibility. This CL just splits IsArcAllowedForProfile for use in subsequent changes. BUG=699850 TEST=ChromeArcUtilTest ==========
lgtm
The CQ bit was checked by kinaba@chromium.org
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": 40001, "attempt_start_ts": 1491833621969470,
"parent_rev": "e6974751597de203bb5a78dc82fa253517338f0e", "commit_rev":
"21c492878576af55fb89bf56efd9cbed20ef4eaa"}
Message was sent while issue was closed.
Description was changed from ========== arc: Add IsArcAllowedInAppListForProfile. This will be used to keep app list even when ARC is disabled due to filesystem incompatibility. This CL just splits IsArcAllowedForProfile for use in subsequent changes. BUG=699850 TEST=ChromeArcUtilTest ========== to ========== arc: Add IsArcAllowedInAppListForProfile. This will be used to keep app list even when ARC is disabled due to filesystem incompatibility. This CL just splits IsArcAllowedForProfile for use in subsequent changes. BUG=699850 TEST=ChromeArcUtilTest Review-Url: https://codereview.chromium.org/2806443002 Cr-Commit-Position: refs/heads/master@{#463255} Committed: https://chromium.googlesource.com/chromium/src/+/21c492878576af55fb89bf56efd9... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/21c492878576af55fb89bf56efd9... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
