Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2160013002:
Add device format filter in DisableIf() annotation (Closed)
|
Created:
4 years, 5 months ago by shenghuazhang Modified:
4 years, 4 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd device format filter in DisableIf() annotation
Add device_is_tablet() in DisableIf.Build
Add device() to check format in DisableIfSkipCheck.java
BUG=620138
Committed: https://crrev.com/9eb2957737fed96485850e6b1692ee334b6ee31a
Cr-Commit-Position: refs/heads/master@{#408240}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Move DeviceFormFactor.java from src/ui to src/base #Patch Set 3 : Move DeviceFormFactor.java from src/ui to src/base #
Total comments: 6
Patch Set 4 : ChromeDisableIf.java in chrome/ layer. #
Total comments: 8
Patch Set 5 : change mTargetContext to ChromeInstrumentationTestRunner.java. Change type name to be simpler. #
Total comments: 5
Messages
Total messages: 24 (7 generated)
shenghuazhang@chromium.org changed reviewers: + jbudorick@chromium.org
https://codereview.chromium.org/2160013002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/util/DisableIfSkipCheck.java (right): https://codereview.chromium.org/2160013002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/DisableIfSkipCheck.java:71: private boolean device(DisableIf.Build v) { device() checks the device format. Returns true if v.device_is_tablet() is not assigned or same with device type. https://codereview.chromium.org/2160013002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/DisableIfSkipCheck.java:73: ContextUtils.getApplicationContext()) == v.device_is_tablet(); But I wonder if DeviceFormFactor.isTablet() works here as you mentioned the check implementation can't be in //base/.
https://codereview.chromium.org/2160013002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/util/DisableIfSkipCheck.java (right): https://codereview.chromium.org/2160013002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/DisableIfSkipCheck.java:73: ContextUtils.getApplicationContext()) == v.device_is_tablet(); On 2016/07/18 21:11:40, shenghuazhang1 wrote: > But I wonder if DeviceFormFactor.isTablet() works here as you mentioned the > check implementation can't be in //base/. Yeah, I don't think you can do this. Did you try building? Did this successfully build?
This CL refactors the DeviceFormFactor.java from src/ui to src/base. If this way sounds good, I will delete the one in src/ui and refactor all codes using it to redirect to the one under src/base in next patch. I also came out another way which is trying to create an annotation under src/ui e.g. DisableIf.Tablet & TabletSkipCheck.java and merge it in addTestHooks() under ChromeInstrumentationTestRunner. Do you think this works and better?
Description was changed from ========== Add device format filter in DisableIf() annotation Add device_is_tablet() in DisableIf.Build Add device() to check format in DisableIfSkipCheck.java BUG=620138 ========== to ========== Add device format filter in DisableIf() annotation Add device_is_tablet() in DisableIf.Build Add device() to check format in DisableIfSkipCheck.java BUG=620138 ==========
jbudorick@chromium.org changed reviewers: + tedchoc@chromium.org
On 2016/07/20 17:57:18, shenghuazhang1 wrote: > This CL refactors the DeviceFormFactor.java from src/ui to src/base. If this way > sounds good, I will delete the one in src/ui and refactor all codes using it to > redirect to the one under src/base in next patch. I didn't realize that DeviceFormFactor had no dependencies in chromium outside of //base/. Moving it is ok from a dependency perspective, but I have no idea if doing so is a good idea. +tedchoc? > > I also came out another way which is trying to create an annotation under src/ui > e.g. DisableIf.Tablet & TabletSkipCheck.java and merge it in addTestHooks() > under ChromeInstrumentationTestRunner. Do you think this works and better? If moving DeviceFormFactor isn't ok, then yeah, we'll have to do something like this, but let's talk if it gets to that point.
tedchoc@chromium.org changed reviewers: + yfriedman@chromium.org
I'm not an owner of base, so yfriedman@ is probably better to comment on moving it. While I don't have real objections with moving the file, it does seem to belong more nicely in ui/ (at least conceptually). https://codereview.chromium.org/2160013002/diff/40001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2160013002/diff/40001/base/BUILD.gn#newcode2290 base/BUILD.gn:2290: "android/java/src/org/chromium/base/util/base/DeviceFormFactor.java", util/base is odd...I don't see why this wouldn't be directly under base https://codereview.chromium.org/2160013002/diff/40001/base/android/java/src/o... File base/android/java/src/org/chromium/base/util/base/DeviceFormFactor.java (right): https://codereview.chromium.org/2160013002/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/util/base/DeviceFormFactor.java:5: package org.chromium.ui.base; package needs to be updated (as well as all callers) this change also isn't removing the entry from ui, so now we'd have duplicate ones https://codereview.chromium.org/2160013002/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/DisableIfSkipCheck.java (right): https://codereview.chromium.org/2160013002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/DisableIfSkipCheck.java:79: && DeviceFormFactor.isTablet(mTargetContext)) || (v.device_format().equals("phone") indent 8
sorry, somehow didn't hit send https://codereview.chromium.org/2160013002/diff/40001/base/android/java/src/o... File base/android/java/src/org/chromium/base/util/base/DeviceFormFactor.java (right): https://codereview.chromium.org/2160013002/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/util/base/DeviceFormFactor.java:36: public static boolean isTablet(Context context) { isTablet and isLargeTablet don't seem like "base"-level concepts to me. The other functions are less troubling to me but UI still seems like a better home and I'm assuming that would've address your use case.
can you elaborate on where you plan to use these (i.e. include an example). Perhaps we can generalize DisableIf (like we did with Restrictions) for layering
Thank you both for the feedback. On 2016/07/21 15:59:28, Yaron wrote: > can you elaborate on where you plan to use these (i.e. include an example). https://codesearch.chromium.org/chromium/src/chrome/android/javatests/src/org... > Perhaps we can generalize DisableIf (like we did with Restrictions) for layering Yeah, I think that's probably where we need to go with this.
Add interface DisableIf.Device in DisableIf.java. In chrome/ layer, generate the ChromeDisableIf.java identifying device form factors enum, and extends DisableIfSkipCheck. https://codereview.chromium.org/2160013002/diff/40001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2160013002/diff/40001/base/BUILD.gn#newcode2290 base/BUILD.gn:2290: "android/java/src/org/chromium/base/util/base/DeviceFormFactor.java", On 2016/07/20 18:45:35, Ted C wrote: > util/base is odd...I don't see why this wouldn't be directly under base Done. https://codereview.chromium.org/2160013002/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/DisableIfSkipCheck.java (right): https://codereview.chromium.org/2160013002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/DisableIfSkipCheck.java:79: && DeviceFormFactor.isTablet(mTargetContext)) || (v.device_format().equals("phone") On 2016/07/20 18:45:35, Ted C wrote: > indent 8 Done.
Mostly nits, the overall structure here is good. https://codereview.chromium.org/2160013002/diff/60001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/DisableIfSkipCheck.java (right): https://codereview.chromium.org/2160013002/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/DisableIfSkipCheck.java:26: private final Context mTargetContext; This doesn't appear to be used? https://codereview.chromium.org/2160013002/diff/60001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java (right): https://codereview.chromium.org/2160013002/diff/60001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:126: mTargetContext should be a member here. https://codereview.chromium.org/2160013002/diff/60001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeDisableIf.java (right): https://codereview.chromium.org/2160013002/diff/60001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeDisableIf.java:14: public static final String CHROMEDISABLEIF_TYPE_PHONE = "Phone"; nit: Can these just be PHONE, TABLET, and LARGETABLET (or maybe DEVICE_IS_PHONE etc) s.t. usage is simply ChromeDisableIf.PHONE (or ChromeDisableIf.DEVICE_IS_PHONE, though I think I like the former better) https://codereview.chromium.org/2160013002/diff/60001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeDisableIf.java:18: public static final String CHROMEDISABLEIF_TYPE_LARGETABLET = "LargeTablet"; I didn't even realize we made a distinction for large tablets.
https://codereview.chromium.org/2160013002/diff/60001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/DisableIfSkipCheck.java (right): https://codereview.chromium.org/2160013002/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/DisableIfSkipCheck.java:26: private final Context mTargetContext; On 2016/07/21 22:54:53, jbudorick wrote: > This doesn't appear to be used? Done. https://codereview.chromium.org/2160013002/diff/60001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java (right): https://codereview.chromium.org/2160013002/diff/60001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:126: On 2016/07/21 22:54:53, jbudorick wrote: > mTargetContext should be a member here. Done. https://codereview.chromium.org/2160013002/diff/60001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeDisableIf.java (right): https://codereview.chromium.org/2160013002/diff/60001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeDisableIf.java:14: public static final String CHROMEDISABLEIF_TYPE_PHONE = "Phone"; On 2016/07/21 22:54:53, jbudorick wrote: > nit: Can these just be PHONE, TABLET, and LARGETABLET (or maybe DEVICE_IS_PHONE > etc) s.t. usage is simply > > ChromeDisableIf.PHONE > > (or ChromeDisableIf.DEVICE_IS_PHONE, though I think I like the former better) Done. Using PHONE, TABLET, and LARGETABLET. https://codereview.chromium.org/2160013002/diff/60001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeDisableIf.java:18: public static final String CHROMEDISABLEIF_TYPE_LARGETABLET = "LargeTablet"; On 2016/07/21 22:54:53, jbudorick wrote: > I didn't even realize we made a distinction for large tablets. LOL. I think I need to keep it in case being used since the large tablet type already exists.
lgtm https://codereview.chromium.org/2160013002/diff/80001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/DisableIfSkipCheck.java (right): https://codereview.chromium.org/2160013002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/DisableIfSkipCheck.java:39: for (DisableIf.Device d : getAnnotations(method, DisableIf.Device.class)) { Do you need any modifications in base? Couldn't you move this to ChromeDisableIfSkipCheck.shouldSkip? Oh but then your annotation would be ChromeDisableIf.Device(..) :( I wonder if there's a cleaner way of specifying the annotations but I don't know annotation processing well enough to recommend it. https://codereview.chromium.org/2160013002/diff/80001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeDisableIf.java (right): https://codereview.chromium.org/2160013002/diff/80001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeDisableIf.java:18: public static final String LARGETABLET = "LargeTablet"; LARGE_TABLET
@jbudorick Not sure about which layer the annotation should be at. https://codereview.chromium.org/2160013002/diff/80001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/DisableIfSkipCheck.java (right): https://codereview.chromium.org/2160013002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/DisableIfSkipCheck.java:39: for (DisableIf.Device d : getAnnotations(method, DisableIf.Device.class)) { On 2016/07/22 15:43:05, Yaron wrote: > Do you need any modifications in base? Couldn't you move this to > ChromeDisableIfSkipCheck.shouldSkip? > > Oh but then your annotation would be ChromeDisableIf.Device(..) :( > > I wonder if there's a cleaner way of specifying the annotations but I don't know > annotation processing well enough to recommend it. Yeah. I am not sure which is better, to keep the Device filter annotation under base/ @DisableIf, or create it in chrome/ layer just as a new one. @jbudorick What do you think about this? https://codereview.chromium.org/2160013002/diff/80001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeDisableIf.java (right): https://codereview.chromium.org/2160013002/diff/80001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeDisableIf.java:18: public static final String LARGETABLET = "LargeTablet"; On 2016/07/22 15:43:05, Yaron wrote: > LARGE_TABLET Done.
lgtm https://codereview.chromium.org/2160013002/diff/80001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/DisableIfSkipCheck.java (right): https://codereview.chromium.org/2160013002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/DisableIfSkipCheck.java:39: for (DisableIf.Device d : getAnnotations(method, DisableIf.Device.class)) { On 2016/07/22 20:37:32, shenghuazhang1 wrote: > On 2016/07/22 15:43:05, Yaron wrote: > > Do you need any modifications in base? Couldn't you move this to > > ChromeDisableIfSkipCheck.shouldSkip? > > > > Oh but then your annotation would be ChromeDisableIf.Device(..) :( > > > > I wonder if there's a cleaner way of specifying the annotations but I don't > know > > annotation processing well enough to recommend it. > > Yeah. I am not sure which is better, to keep the Device filter annotation under > base/ @DisableIf, or create it in chrome/ layer just as a new one. > @jbudorick What do you think about this? This way works, and I think I'm ok with it for now. I'd prefer that we handle DisableIf similarly to how we handle Restriction -- top-level annotation defined in base/ along with some basic values, then chrome/-specific values in chrome/ -- but I haven't been able to come up with a clean way to do so yet. (I'm also no longer convinced that @DisableIf should be separate from @Restriction; the distinction between the two is likely too nuanced for most people to care, and it functionally shouldn't make a difference.)
The CQ bit was checked by shenghuazhang@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.
Description was changed from ========== Add device format filter in DisableIf() annotation Add device_is_tablet() in DisableIf.Build Add device() to check format in DisableIfSkipCheck.java BUG=620138 ========== to ========== Add device format filter in DisableIf() annotation Add device_is_tablet() in DisableIf.Build Add device() to check format in DisableIfSkipCheck.java BUG=620138 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add device format filter in DisableIf() annotation Add device_is_tablet() in DisableIf.Build Add device() to check format in DisableIfSkipCheck.java BUG=620138 ========== to ========== Add device format filter in DisableIf() annotation Add device_is_tablet() in DisableIf.Build Add device() to check format in DisableIfSkipCheck.java BUG=620138 Committed: https://crrev.com/9eb2957737fed96485850e6b1692ee334b6ee31a Cr-Commit-Position: refs/heads/master@{#408240} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/9eb2957737fed96485850e6b1692ee334b6ee31a Cr-Commit-Position: refs/heads/master@{#408240} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
