Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1337703002:
[Contextual Search] Add support for crushed sprites and animate the search provider icon (Closed)
|
Created:
5 years, 3 months ago by Theresa Modified:
5 years, 1 month ago Reviewers:
Dmitry Skiba, David Trainor- moved to gerrit, pedro (no code reviews), rmcilroy, newt (away) CC:
chromium-reviews, Donn Denman Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Contextual Search] Add support for crushed sprites and animate the search provider icon
Adds support for loading and drawing a crushed sprite resource.
Animates the search provider icon using a crushed sprite sheet every time
on long press if the user has never opened the panel before and
once a day on tap.
BUG=529915
Committed: https://crrev.com/af303ba5a35c1e608d6e92cc2fead270cf819274
Cr-Commit-Position: refs/heads/master@{#356977}
Patch Set 1 #
Total comments: 30
Patch Set 2 : Changes from reviews #
Total comments: 6
Patch Set 3 : Crushed sprites #Patch Set 4 : Changes from self-review #
Total comments: 6
Patch Set 5 : Changes from newt@ review #
Total comments: 28
Patch Set 6 : Changes from reviews, add integer-array for other resolutions #Patch Set 7 : Rebase #
Total comments: 9
Patch Set 8 : Rename xml files -> google_icon_sprite_frames, other small changes #Patch Set 9 : Manually scale crushed sprites #
Total comments: 1
Patch Set 10 : Adapt code to design doc #Patch Set 11 : Cleanup from self-review #Patch Set 12 : Fix findbugs & rebase #Patch Set 13 : Another findbugs fix #Patch Set 14 : Add unittest for new jni_array method #Patch Set 15 : Add TODO's for other planned tests #
Total comments: 88
Patch Set 16 : Changes from last dtrainor@ review #
Total comments: 22
Patch Set 17 : More changes from dtrainor@ review #
Total comments: 12
Patch Set 18 : Rebase #Patch Set 19 : Add back support for static icon #Patch Set 20 : Add resource_manager_impl_unittest #Patch Set 21 : Small changes from reviews #
Total comments: 16
Patch Set 22 : Changes from rmcilroy & pedrosimonetti reviews #Patch Set 23 : Update comment #Patch Set 24 : Rebase #Patch Set 25 : Fix clang error #Patch Set 26 : Fix CrushedSpriteTest (progaurd was removing a method) #
Total comments: 12
Patch Set 27 : Rebase #Patch Set 28 : Very small changes from last pedrosimonneti@ review #
Total comments: 6
Messages
Total messages: 65 (9 generated)
twellington@chromium.org changed reviewers: + changwan@chromium.org, newt@chromium.org, pedrosimonetti@chromium.org
ptal newt@ - chrome/android/java/res & ChromePreferenceManager.java pedrosimonetti@ - chrome/android/java/src/org/chromium/chrome/browser/* changwan@ - chrome/android/compositor/*
Let's wait for the new assets before submitting this change, since they are unnecessarily bigger than necessary, due to the presence of unneeded frames. I took a first look at your code and posted some comments to improve the code. Feel free to ping me offline if you have questions or want to discuss details. https://codereview.chromium.org/1337703002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java (right): https://codereview.chromium.org/1337703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:33: SEARCH_PROVIDER_ICON_SPRITE_FRAME SPRITE and FRAME and technical details we should care about. I'd name this SEARCH_PROVIDER_ICON_ANIMATION, which better describes its function (without mentioning implementation details). https://codereview.chromium.org/1337703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:232: SEARCH_PROVIDER_ICON_FIRST_SPRITE_FRAME, Similarly to the comment about the property name, I think we shouldn't care about frames in here, which is an implementation detail. Instead, we should provide a completion percentage (0..1), and then based on the completion, duration and total number of frames we should be able to determine which frame to render. https://codereview.chromium.org/1337703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:233: SEARCH_PROVIDER_ICON_LAST_SPRITE_FRAME, 500); Let's confirm the exact duration for the animation but I would think 500 it's too much. In either case, we should have a constant for it (if not reusing the existing duration constants). https://codereview.chromium.org/1337703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:390: setSearchProviderIconSpriteFrame((int) value); For the reasons explained above, I think we should call this: setSearchProviderIconAnimationCompletion(float percentage) https://codereview.chromium.org/1337703002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java (right): https://codereview.chromium.org/1337703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:767: protected static final int SEARCH_PROVIDER_ICON_FIRST_SPRITE_FRAME = 40; I would prefer not hardcoding the initial and final frame, because ideally we would not have extra unused sprites in the grid, which means the first and last frames would be the first and last available frames, respectively. https://codereview.chromium.org/1337703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:769: private static final int SEARCH_PROVIDER_ICON_SPRITES_PER_ROW = 10; Instead of hardcoding the number of sprites per row and column, can we deduct those values from the size of a single sprite and the size of the sprite grid? https://codereview.chromium.org/1337703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:837: public void setSearchProviderIconSpriteFrame(int spriteFrame) { As stated in the comment on the ContextualSearchPanelAnimation() file, I think we should name this setSearchProviderIconAnimationCompletion(float percentage). The idea is that the animation code should only care about completion (a number between 0 and 1). The ContextualSearchPanelBase class, on the other hand, knows how to render the details of things, so it should take the completion and deduct which frame to render based on a series of factors. We should only hardcode the size of the sprite, say 32dp. Then, with this information, and the completion percentage, we are able to tell which frame to render. Instead of hardcoding how many rows and columns are there, we can find that by comparing the sprite size with the grid size. We can also calculate the total number of frames using the same logic. Then, finally, we would pass to the native code (contextual_search_layer.cc) which exact frame to render, because this is the part of the code that render things but does not know the logic behind it (this class only "follow orders"). https://codereview.chromium.org/1337703002/diff/1/chrome/browser/android/comp... File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right): https://codereview.chromium.org/1337703002/diff/1/chrome/browser/android/comp... chrome/browser/android/compositor/layer/contextual_search_layer.cc:156: if (search_provider_icon_->parent() != layer_) { Nit: please rename search_provider_icon_ to search_provider_icon_sprite_ https://codereview.chromium.org/1337703002/diff/1/chrome/browser/android/comp... chrome/browser/android/compositor/layer/contextual_search_layer.cc:524: void ContextualSearchLayer::SetSearchProviderIconUV(int sprite_frame) { I think it would make more sense to call this SetSearchProviderIconSpriteFrame(), which is more meaningful.
Please run tools/resources/optimize-png-files.sh on the images, especially since they're so large. e.g. > tools/resources/optimize-png-files.sh path/to/image.png https://codereview.chromium.org/1337703002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java (right): https://codereview.chromium.org/1337703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:283: SimpleDateFormat dateFormat = new SimpleDateFormat("yyyy-MM-d"); Using System.currentTimeMillis() is much more efficient. It also has the advantage that night owls won't see the animation twice in a row if they start using Chrome just before midnight. long currentTimeMillis = System.currentTimeMillis(); long lastTappedTimeMillis = mPreferenceManager.getContextualSearchLastTapTime(); if (Math.abs(currentTimeMillis - lastTappedTimeMillis) > ONE_DAY_IN_MILLIS) { ... (I'd use Math.abs() to prevent weirdness in case the user sets their clock way into the future and then back again)
https://codereview.chromium.org/1337703002/diff/1/chrome/browser/android/comp... File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right): https://codereview.chromium.org/1337703002/diff/1/chrome/browser/android/comp... chrome/browser/android/compositor/layer/contextual_search_layer.cc:163: float search_provider_icon_sprite_width = Nit: How about just naming them icon_width and icon_height? They are scoped in the if statement anyways. https://codereview.chromium.org/1337703002/diff/1/chrome/browser/android/comp... chrome/browser/android/compositor/layer/contextual_search_layer.cc:171: float search_provider_icon_left; Nit: how about icon_x and icon_y for simplicity? https://codereview.chromium.org/1337703002/diff/1/chrome/browser/android/comp... chrome/browser/android/compositor/layer/contextual_search_layer.cc:186: search_provider_icon_sprite_height); gfx::Size size(search_provider_icon_sprite_width, search_provider_icon_sprite_height); Or could be inlined in SetBounds(), just as you did in the next line. It's currently leaking. https://codereview.chromium.org/1337703002/diff/1/chrome/browser/android/comp... chrome/browser/android/compositor/layer/contextual_search_layer.cc:525: float column = (float) (sprite_frame % search_provider_icon_sprites_per_row_); Please do not use C-style casting, and use static_cast<> when applicable. https://codereview.chromium.org/1337703002/diff/1/chrome/browser/android/comp... File chrome/browser/android/compositor/layer/contextual_search_layer.h (right): https://codereview.chromium.org/1337703002/diff/1/chrome/browser/android/comp... chrome/browser/android/compositor/layer/contextual_search_layer.h:83: // Uses the |sprite_frame| to limit the portion of search_provider_icon_ Nit: how about s/limit/determine/ ?
I ran tools/resources/optimize-png-files.sh on the sprite sheets - we got a 2% average reduction. https://codereview.chromium.org/1337703002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java (right): https://codereview.chromium.org/1337703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:33: SEARCH_PROVIDER_ICON_SPRITE_FRAME On 2015/09/11 22:49:42, pedrosimonetti wrote: > SPRITE and FRAME and technical details we should care about. I'd name this > SEARCH_PROVIDER_ICON_ANIMATION, which better describes its function (without > mentioning implementation details). Done. https://codereview.chromium.org/1337703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:232: SEARCH_PROVIDER_ICON_FIRST_SPRITE_FRAME, On 2015/09/11 22:49:42, pedrosimonetti wrote: > Similarly to the comment about the property name, I think we shouldn't care > about frames in here, which is an implementation detail. Instead, we should > provide a completion percentage (0..1), and then based on the completion, > duration and total number of frames we should be able to determine which frame > to render. Done. https://codereview.chromium.org/1337703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:233: SEARCH_PROVIDER_ICON_LAST_SPRITE_FRAME, 500); On 2015/09/11 22:49:42, pedrosimonetti wrote: > Let's confirm the exact duration for the animation but I would think 500 it's > too much. In either case, we should have a constant for it (if not reusing the > existing duration constants). Acknowledged. I'll bring it up at the meeting today. https://codereview.chromium.org/1337703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:390: setSearchProviderIconSpriteFrame((int) value); On 2015/09/11 22:49:42, pedrosimonetti wrote: > For the reasons explained above, I think we should call this: > setSearchProviderIconAnimationCompletion(float percentage) Done. https://codereview.chromium.org/1337703002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java (right): https://codereview.chromium.org/1337703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:767: protected static final int SEARCH_PROVIDER_ICON_FIRST_SPRITE_FRAME = 40; On 2015/09/11 22:49:42, pedrosimonetti wrote: > I would prefer not hardcoding the initial and final frame, because ideally we > would not have extra unused sprites in the grid, which means the first and last > frames would be the first and last available frames, respectively. The last frame still needs to be hard coded. The sprite sheet we have is a grid with a few blank frames on the end. We can reliably use 0 for the first frame, so I dropped that constant. https://codereview.chromium.org/1337703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:769: private static final int SEARCH_PROVIDER_ICON_SPRITES_PER_ROW = 10; On 2015/09/11 22:49:42, pedrosimonetti wrote: > Instead of hardcoding the number of sprites per row and column, can we deduct > those values from the size of a single sprite and the size of the sprite grid? Done. https://codereview.chromium.org/1337703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:837: public void setSearchProviderIconSpriteFrame(int spriteFrame) { On 2015/09/11 22:49:42, pedrosimonetti wrote: > As stated in the comment on the ContextualSearchPanelAnimation() file, I think > we should name this setSearchProviderIconAnimationCompletion(float percentage). > > The idea is that the animation code should only care about completion (a number > between 0 and 1). > > The ContextualSearchPanelBase class, on the other hand, knows how to render the > details of things, so it should take the completion and deduct which frame to > render based on a series of factors. > > We should only hardcode the size of the sprite, say 32dp. Then, with this > information, and the completion percentage, we are able to tell which frame to > render. > > Instead of hardcoding how many rows and columns are there, we can find that by > comparing the sprite size with the grid size. We can also calculate the total > number of frames using the same logic. > > Then, finally, we would pass to the native code (contextual_search_layer.cc) > which exact frame to render, because this is the part of the code that render > things but does not know the logic behind it (this class only "follow orders"). I changed this to a percentage and am passing the sprite frame and size of the sprite to contextual_search_layer.cc. That class still uses the size to calculate which part of the sprite sheet to show. That makes sense to me, because contextual_search_layer.cc is already using width/height to calculate placement, but I could also pass the x/y pairs that determine which part of the sprite sheet to draw from ContextualSearchPanelBase instead of calculating on the native side if that's preferable. https://codereview.chromium.org/1337703002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java (right): https://codereview.chromium.org/1337703002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:283: SimpleDateFormat dateFormat = new SimpleDateFormat("yyyy-MM-d"); On 2015/09/11 23:01:15, newt wrote: > Using System.currentTimeMillis() is much more efficient. It also has the > advantage that night owls won't see the animation twice in a row if they start > using Chrome just before midnight. > > long currentTimeMillis = System.currentTimeMillis(); > long lastTappedTimeMillis = > mPreferenceManager.getContextualSearchLastTapTime(); > if (Math.abs(currentTimeMillis - lastTappedTimeMillis) > ONE_DAY_IN_MILLIS) { > ... > > (I'd use Math.abs() to prevent weirdness in case the user sets their clock way > into the future and then back again) Done. https://codereview.chromium.org/1337703002/diff/1/chrome/browser/android/comp... File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right): https://codereview.chromium.org/1337703002/diff/1/chrome/browser/android/comp... chrome/browser/android/compositor/layer/contextual_search_layer.cc:156: if (search_provider_icon_->parent() != layer_) { On 2015/09/11 22:49:42, pedrosimonetti wrote: > Nit: please rename search_provider_icon_ to search_provider_icon_sprite_ Done. https://codereview.chromium.org/1337703002/diff/1/chrome/browser/android/comp... chrome/browser/android/compositor/layer/contextual_search_layer.cc:163: float search_provider_icon_sprite_width = On 2015/09/14 01:51:30, Changwan Ryu wrote: > Nit: How about just naming them icon_width and icon_height? They are scoped in > the if statement anyways. Acknowledged. This value is getting passed in now, so it needs a more descriptive name. https://codereview.chromium.org/1337703002/diff/1/chrome/browser/android/comp... chrome/browser/android/compositor/layer/contextual_search_layer.cc:171: float search_provider_icon_left; On 2015/09/14 01:51:30, Changwan Ryu wrote: > Nit: how about icon_x and icon_y for simplicity? Done. https://codereview.chromium.org/1337703002/diff/1/chrome/browser/android/comp... chrome/browser/android/compositor/layer/contextual_search_layer.cc:186: search_provider_icon_sprite_height); On 2015/09/14 01:51:29, Changwan Ryu wrote: > gfx::Size size(search_provider_icon_sprite_width, > search_provider_icon_sprite_height); > > Or could be inlined in SetBounds(), just as you did in the next line. > It's currently leaking. Done. https://codereview.chromium.org/1337703002/diff/1/chrome/browser/android/comp... chrome/browser/android/compositor/layer/contextual_search_layer.cc:524: void ContextualSearchLayer::SetSearchProviderIconUV(int sprite_frame) { On 2015/09/11 22:49:42, pedrosimonetti wrote: > I think it would make more sense to call this > SetSearchProviderIconSpriteFrame(), which is more meaningful. Done. https://codereview.chromium.org/1337703002/diff/1/chrome/browser/android/comp... chrome/browser/android/compositor/layer/contextual_search_layer.cc:525: float column = (float) (sprite_frame % search_provider_icon_sprites_per_row_); On 2015/09/14 01:51:30, Changwan Ryu wrote: > Please do not use C-style casting, and use static_cast<> when applicable. Done. https://codereview.chromium.org/1337703002/diff/1/chrome/browser/android/comp... File chrome/browser/android/compositor/layer/contextual_search_layer.h (right): https://codereview.chromium.org/1337703002/diff/1/chrome/browser/android/comp... chrome/browser/android/compositor/layer/contextual_search_layer.h:83: // Uses the |sprite_frame| to limit the portion of search_provider_icon_ On 2015/09/14 01:51:30, Changwan Ryu wrote: > Nit: how about s/limit/determine/ ? Done.
chrome/android/java/res and ChromePreferenceManager.java lgtm > I ran tools/resources/optimize-png-files.sh on the sprite sheets - we got a 2% average reduction. That's a lot less than usual. I guess the designer did a good job of optimizing these in the first place. https://codereview.chromium.org/1337703002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java (right): https://codereview.chromium.org/1337703002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java:237: public long getContextualSearchLastAnimationOnTapTime() { This name is very precise, but something of a mouthful. getContextualSearchLastAnimationTime() might be OK too.
https://codereview.chromium.org/1337703002/diff/20001/chrome/browser/android/... File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right): https://codereview.chromium.org/1337703002/diff/20001/chrome/browser/android/... chrome/browser/android/compositor/layer/contextual_search_layer.cc:153: search_provider_icon_sprites_per_row_ = Actually, you don't need to keep these as member variables, and they are only used in SetSearchProviderIconSpriteFrame(). How about passing search_provider_icon_sprite_resource->size and search_provider_icon_sprite_size to SetSearchProviderIconSpriteFrame() and calculate _per_row and _rows there? https://codereview.chromium.org/1337703002/diff/20001/chrome/browser/android/... chrome/browser/android/compositor/layer/contextual_search_layer.cc:522: gfx::PointF(bottom_right_x, bottom_right_y)); nit: indentation
ptal - this has changed significantly. The original sprite sheets would have increased APK size by more than 80kb, so we have new awesome "crushed" sprite sheets. There are notes through out the code explaining how the crushed sprite sheet works, but basically each frame is drawn on top of the last frame using a series of small rectangles that represent the pieces that have changed. This results in dramatically smaller files (and substantially more complex code). With this approach, our APK size increase is only going to be a few KB at most, partially because the files are a lot smaller and partially because we'll only deliver at a couple of dpi's. I have a few questions (left as TODO's in the code) - there are two in crushed_sprite_layer.cc about calculating unshared memory usage and one in ui_resource_anrdoid.cc Ping me if you want a link to the JavaScript demo this code is based off. https://codereview.chromium.org/1337703002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java (right): https://codereview.chromium.org/1337703002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java:237: public long getContextualSearchLastAnimationOnTapTime() { On 2015/09/14 19:59:29, newt wrote: > This name is very precise, but something of a mouthful. > getContextualSearchLastAnimationTime() might be OK too. Done. https://codereview.chromium.org/1337703002/diff/20001/chrome/browser/android/... File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right): https://codereview.chromium.org/1337703002/diff/20001/chrome/browser/android/... chrome/browser/android/compositor/layer/contextual_search_layer.cc:153: search_provider_icon_sprites_per_row_ = On 2015/09/17 06:10:08, Changwan Ryu wrote: > Actually, you don't need to keep these as member variables, and they are only > used in SetSearchProviderIconSpriteFrame(). How about passing > search_provider_icon_sprite_resource->size and search_provider_icon_sprite_size > to SetSearchProviderIconSpriteFrame() and calculate _per_row and _rows there? Done. https://codereview.chromium.org/1337703002/diff/20001/chrome/browser/android/... chrome/browser/android/compositor/layer/contextual_search_layer.cc:522: gfx::PointF(bottom_right_x, bottom_right_y)); On 2015/09/17 06:10:08, Changwan Ryu wrote: > nit: indentation Acknowledged.
Wow, the crushed sprites are... interesting looking :) I reviewed the resources, lint suppressions, and ChromePreferenceManager. https://codereview.chromium.org/1337703002/diff/60001/build/android/lint/supp... File build/android/lint/suppressions.xml (right): https://codereview.chromium.org/1337703002/diff/60001/build/android/lint/supp... build/android/lint/suppressions.xml:57: <ignore path="chrome/android/java/res/drawable-mdpi"/> nooooooooo :'( *tears* We don't want to ignore all IconDipSize warnings-- just the ones related to google_icon_sprite.png. The <ignore regexp="..."> line should be sufficient for that purpose. So, why do you need all the <ignore path="..."> lines? https://codereview.chromium.org/1337703002/diff/60001/chrome/android/java/res... File chrome/android/java/res/values/arrays.xml (right): https://codereview.chromium.org/1337703002/diff/60001/chrome/android/java/res... chrome/android/java/res/values/arrays.xml:36: <!-- Contextual Search Sprite Frame Rectangles --> I'd put this in a separate xml file (google_icon_sprite_gobbledygook.xml) so that arrays.xml remains human readable and uncluttered. (Note that Android doesn't care about the filename, putting arrays in arrays.xml is just a convention) https://codereview.chromium.org/1337703002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java (right): https://codereview.chromium.org/1337703002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java:36: private static final String CONTEXTUAL_SEARCH_LAST_ANIMATION_ON_TAP_TIME = I'd rename these to be consistent with the method name below
https://codereview.chromium.org/1337703002/diff/60001/build/android/lint/supp... File build/android/lint/suppressions.xml (right): https://codereview.chromium.org/1337703002/diff/60001/build/android/lint/supp... build/android/lint/suppressions.xml:57: <ignore path="chrome/android/java/res/drawable-mdpi"/> On 2015/09/25 01:21:27, newt wrote: > nooooooooo :'( *tears* > > We don't want to ignore all IconDipSize warnings-- just the ones related to > google_icon_sprite.png. The <ignore regexp="..."> line should be sufficient for > that purpose. So, why do you need all the <ignore path="..."> lines? I had assumed (based on nothing) it used both the path and regexp (which was incorrect anyway). Fixed the regexp and removed the paths. Is the new regexp specific enough? This is the warning message: chrome/android/java/res/drawable-mdpi/google_icon_sprite.png The image `google_icon_sprite.png` varies significantly in its density-independent (dip) size across the various density versions: drawable-xxhdpi/google_icon_sprite.png: 283x36 dp (849x108 px), drawable-xxxhdpi/google_icon_sprite.png: 281x36 dp (1124x144 px), drawable-xhdpi/google_icon_sprite.png: 90x102 dp (180x203 px), drawable-hdpi/google_icon_sprite.png: 85x100 dp (128x150 px), drawable-mdpi/google_icon_sprite.png: 77x94 dp (77x94 px): IconDipSize [warning] https://codereview.chromium.org/1337703002/diff/60001/chrome/android/java/res... File chrome/android/java/res/values/arrays.xml (right): https://codereview.chromium.org/1337703002/diff/60001/chrome/android/java/res... chrome/android/java/res/values/arrays.xml:36: <!-- Contextual Search Sprite Frame Rectangles --> On 2015/09/25 01:21:27, newt wrote: > I'd put this in a separate xml file (google_icon_sprite_gobbledygook.xml) so > that arrays.xml remains human readable and uncluttered. (Note that Android > doesn't care about the filename, putting arrays in arrays.xml is just a > convention) Acknowledged. I will move them when I go in and create the arrays for the other densities. https://codereview.chromium.org/1337703002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java (right): https://codereview.chromium.org/1337703002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceManager.java:36: private static final String CONTEXTUAL_SEARCH_LAST_ANIMATION_ON_TAP_TIME = On 2015/09/25 01:21:27, newt wrote: > I'd rename these to be consistent with the method name below Done.
https://codereview.chromium.org/1337703002/diff/80001/build/android/lint/supp... File build/android/lint/suppressions.xml (right): https://codereview.chromium.org/1337703002/diff/80001/build/android/lint/supp... build/android/lint/suppressions.xml:57: <ignore regexp=".*google_icon_sprite.png.*"/> Much better :D > Is the new regexp specific enough? This is the warning message: > > chrome/android/java/res/drawable-mdpi/google_icon_sprite.png The image > `google_icon_sprite.png` varies significantly in its density-independent (dip) > size across the various density versions: > drawable-xxhdpi/google_icon_sprite.png: 283x36 dp (849x108 px), > drawable-xxxhdpi/google_icon_sprite.png: 281x36 dp (1124x144 px), > drawable-xhdpi/google_icon_sprite.png: 90x102 dp (180x203 px), > drawable-hdpi/google_icon_sprite.png: 85x100 dp (128x150 px), > drawable-mdpi/google_icon_sprite.png: 77x94 dp (77x94 px): IconDipSize [warning] Make sure that this regex doesn't allow other mis-sized images to sneak past lint. I'd try messing up another image's dimensions and see what the error is in that case. For comparison, lint prints out a single IconDensities warning that contains a list of all images that are missing in some density, so the regex I added for IconDensities very specifically begins with ": " and ends with "$" to ensure that if other images are missing densities, they won't match that regex.
Hey Theresa, here are my comments. Overall, I think this CL is looking good, but as we discussed offline, it would be nice refactoring the code to make the Crushed Layer class more self-contained and easily reusable. Ideally the class would keep a cache of all the rects needed to paint each frame, and keep track of what was the last pained frame. So, instead of passing the rects needed for each frame, we would only pass what frame number we want to paint, and the class would do the rest. That being said, given that we are the only consumers of this class, and that the branch cut is around the corner, I would be okay with submitting this CL as I first step, and later working on a more robust design. https://codereview.chromium.org/1337703002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java (right): https://codereview.chromium.org/1337703002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:133: mIsAnimatingPanelOpen = true; In theory, we could call peekPanel() from any state, so this means that we're not necessarily animating the panel open at this point. See comment on onAnimationFinished() below. https://codereview.chromium.org/1337703002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:231: float duration = (getSearchProviderIconSpriteFrameCount() / framesPerSecond) * 1000; Is there a spec for the duration of the G animation? Currently we will end up having a 600ms duration, which seems too slow. I couldn't find the duration spec for the new G animation, but the old Kennedy spec [1] says that a "slow" animation should have 218ms. Maybe we should use 60fps instead? [1] https://spec.googleplex.com/kennedy#animations https://codereview.chromium.org/1337703002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:437: if (mIsAnimatingPanelOpen) { Instead of using a boolean to determine if we're animating the opening of the Panel, we could use a combination of mAnimatingState and getPanelState() to do that: PanelState state = getPanelState(); if (mAnimatingState == PanelState.PEEKED && (state == PanelState.CLOSED || state == PanelState.UNDEFINED)) { ... } https://codereview.chromium.org/1337703002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java (right): https://codereview.chromium.org/1337703002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:912: public boolean getSearchProviderIconSpritePaintPreviousFrames() { Nit: As we've discussed offline, we should move as much logic as possible regarding the Crushed Layer to the native side, so that the class is more self-contained and easy to reuse. That being said, it would be fine doing that in a separate CL given that we're the only consumers of this class so far. On the other hand, for now, we should rename this method to something else, to follow the pattern of using logical prefixes when naming methods that return boolean values. In this case it should've been called shouldPaintPreviousSearchProviderIconSpriteFrames(), or something similar. https://codereview.chromium.org/1337703002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java (right): https://codereview.chromium.org/1337703002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java:72: boolean searchProviderIconSpriteVisible = mSearchPanel.isSearchProviderIconSpriteVisible(); Nit: Move these a few lines above, right after the line: float searchBarShadowOpacity = mSearchPanel.getSearchBarShadowOpacity(); This way we keep the variables declarations in the same order as the arguments in the call below. https://codereview.chromium.org/1337703002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/1337703002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:499: !mSearchPanelDelegate.isShowing() Nit: Encapsulate the entire logic in the ContextualSearchPolicy#shouldAnimateSearchProviderIconSprite() method, that is, we should pass a boolean isPanelShowing to that method. https://codereview.chromium.org/1337703002/diff/80001/chrome/browser/android/... File chrome/browser/android/compositor/layer/crushed_sprite_layer.cc (right): https://codereview.chromium.org/1337703002/diff/80001/chrome/browser/android/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:91: // be included in the memory usage. Good question. I don't know the answer. Same applies for the question below. https://codereview.chromium.org/1337703002/diff/80001/ui/android/resources/ui... File ui/android/resources/ui_resource_android.cc (right): https://codereview.chromium.org/1337703002/diff/80001/ui/android/resources/ui... ui/android/resources/ui_resource_android.cc:28: // directly rather than exposing this method? I mostly did it this way because I don't know the answer to this question.
On 2015/09/25 02:18:44, newt wrote: > https://codereview.chromium.org/1337703002/diff/80001/build/android/lint/supp... > File build/android/lint/suppressions.xml (right): > > https://codereview.chromium.org/1337703002/diff/80001/build/android/lint/supp... > build/android/lint/suppressions.xml:57: <ignore > regexp=".*google_icon_sprite.png.*"/> > Much better :D > > > Is the new regexp specific enough? This is the warning message: > > > > chrome/android/java/res/drawable-mdpi/google_icon_sprite.png The image > > `google_icon_sprite.png` varies significantly in its density-independent (dip) > > size across the various density versions: > > drawable-xxhdpi/google_icon_sprite.png: 283x36 dp (849x108 px), > > drawable-xxxhdpi/google_icon_sprite.png: 281x36 dp (1124x144 px), > > drawable-xhdpi/google_icon_sprite.png: 90x102 dp (180x203 px), > > drawable-hdpi/google_icon_sprite.png: 85x100 dp (128x150 px), > > drawable-mdpi/google_icon_sprite.png: 77x94 dp (77x94 px): IconDipSize > [warning] > > Make sure that this regex doesn't allow other mis-sized images to sneak past > lint. I'd try messing up another image's dimensions and see what the error is in > that case. > > For comparison, lint prints out a single IconDensities warning that contains a > list of all images that are missing in some density, so the regex I added for > IconDensities very specifically begins with ": " and ends with "$" to ensure > that if other images are missing densities, they won't match that regex. It's a different line for each image, w/ the lint suppression, I still see errors for other images: chrome/android/java/res/drawable-mdpi/account_management_no_picture.png The image `account_management_no_picture.png` varies significantly in its density-independent (dip) size across the various density versions: drawable-hdpi/account_management_no_picture.png: 40x40 dp (60x60 px), drawable-xhdpi/account_management_no_picture.png: 40x40 dp (80x80 px), drawable-xxhdpi/account_management_no_picture.png: 40x40 dp (120x120 px), drawable-xxxhdpi/account_management_no_picture.png: 40x40 dp (160x160 px), drawable-mdpi/account_management_no_picture.png: 39x1000 dp (39x1000 px): IconDipSize [warning]
dtrainor@chromium.org changed reviewers: + dtrainor@chromium.org
Comments, but in general looks good to me. lgtm, but wait for Pedro before landing IMO. https://codereview.chromium.org/1337703002/diff/80001/chrome/android/java/res... File chrome/android/java/res/values/arrays.xml (right): https://codereview.chromium.org/1337703002/diff/80001/chrome/android/java/res... chrome/android/java/res/values/arrays.xml:37: <array name="contextual_search_sprite_frame_rects"> Can we move this to something like res/values/crushed_sprite_metadata.xml? https://codereview.chromium.org/1337703002/diff/80001/chrome/android/java/res... chrome/android/java/res/values/arrays.xml:65: <integer-array name="frame0"> Can we give these some kind of prefix so that they won't conflict with any newly added crushed sprites? https://codereview.chromium.org/1337703002/diff/80001/chrome/browser/android/... File chrome/browser/android/compositor/layer/crushed_sprite_layer.cc (right): https://codereview.chromium.org/1337703002/diff/80001/chrome/browser/android/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:91: // be included in the memory usage. On 2015/09/29 01:14:40, pedrosimonetti wrote: > Good question. I don't know the answer. Same applies for the question below. I would imagine it's asking memory that this class uses that isn't shared among other layers/display lists. ie: if this class died, would that lead to the memory being freed? I could be wrong though. https://codereview.chromium.org/1337703002/diff/80001/chrome/browser/android/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:113: previous_frame_ = NULL; I'm not sure this is necessary. It will just run the destructor on the temporarily created RefPtr, which will be the same as if this class died and the destructor ran on previous_frame_. https://codereview.chromium.org/1337703002/diff/80001/chrome/browser/android/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:115: src_bitmap_.reset(); Also might not be necessary? It looks like the underlying pixels will be freed either way.
Took another look and am including one more comment. https://codereview.chromium.org/1337703002/diff/80001/chrome/browser/android/... File chrome/browser/android/compositor/layer/crushed_sprite_layer.h (right): https://codereview.chromium.org/1337703002/diff/80001/chrome/browser/android/... chrome/browser/android/compositor/layer/crushed_sprite_layer.h:58: scoped_refptr<cc::PictureLayer> layer_; Sorry I didn't notice this before, but it feels weird that this is a Layer that returns another (picture) layer. Wouldn't it be possible to extend the PictureImageLayer instead and use it (which extends PictureLayer and implements the ContentLayerClient)? In either case, I not familiar with the implementation of those classes, so it would be good having someone more familiar with it reviewing this class and its usage in ContextualSearchLayer.
ptal - addressed or responded to all review comments. newt@ - there's a new lint suppression for InconsistentArrays kerz@ and I agreed in an offline conversation that it was okay to deliver assets at all densities since correctly scaling the int arrays was proving very difficult. We don't have assets for ldpi, so I'm disabled the animation on ldpi devices. The last frame of the mdpi resource is drawn for ldpi. https://chromiumcodereview.appspot.com/1337703002/diff/80001/chrome/android/j... File chrome/android/java/res/values/arrays.xml (right): https://chromiumcodereview.appspot.com/1337703002/diff/80001/chrome/android/j... chrome/android/java/res/values/arrays.xml:37: <array name="contextual_search_sprite_frame_rects"> On 2015/09/29 22:23:09, David Trainor wrote: > Can we move this to something like res/values/crushed_sprite_metadata.xml? Done. https://chromiumcodereview.appspot.com/1337703002/diff/80001/chrome/android/j... chrome/android/java/res/values/arrays.xml:65: <integer-array name="frame0"> On 2015/09/29 22:23:09, David Trainor wrote: > Can we give these some kind of prefix so that they won't conflict with any newly > added crushed sprites? Done. https://chromiumcodereview.appspot.com/1337703002/diff/80001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java (right): https://chromiumcodereview.appspot.com/1337703002/diff/80001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:133: mIsAnimatingPanelOpen = true; On 2015/09/29 01:14:39, pedrosimonetti wrote: > In theory, we could call peekPanel() from any state, so this means that we're > not necessarily animating the panel open at this point. See comment on > onAnimationFinished() below. Acknowledged. https://chromiumcodereview.appspot.com/1337703002/diff/80001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:231: float duration = (getSearchProviderIconSpriteFrameCount() / framesPerSecond) * 1000; On 2015/09/29 01:14:39, pedrosimonetti wrote: > Is there a spec for the duration of the G animation? Currently we will end up > having a 600ms duration, which seems too slow. > > I couldn't find the duration spec for the new G animation, but the old Kennedy > spec [1] says that a "slow" animation should have 218ms. > > Maybe we should use 60fps instead? > > [1] https://spec.googleplex.com/kennedy#animations > Per offline discussion, we're going with 350ms for now; we can easily speed it up if it still feels too slow. https://chromiumcodereview.appspot.com/1337703002/diff/80001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelAnimation.java:437: if (mIsAnimatingPanelOpen) { On 2015/09/29 01:14:39, pedrosimonetti wrote: > Instead of using a boolean to determine if we're animating the opening of the > Panel, we could use a combination of mAnimatingState and getPanelState() to do > that: > > PanelState state = getPanelState(); > > if (mAnimatingState == PanelState.PEEKED && (state == PanelState.CLOSED || state > == PanelState.UNDEFINED)) { > ... > } Done. https://chromiumcodereview.appspot.com/1337703002/diff/80001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java (right): https://chromiumcodereview.appspot.com/1337703002/diff/80001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:912: public boolean getSearchProviderIconSpritePaintPreviousFrames() { On 2015/09/29 01:14:39, pedrosimonetti wrote: > Nit: As we've discussed offline, we should move as much logic as possible > regarding the Crushed Layer to the native side, so that the class is more > self-contained and easy to reuse. That being said, it would be fine doing that > in a separate CL given that we're the only consumers of this class so far. > > On the other hand, for now, we should rename this method to something else, to > follow the pattern of using logical prefixes when naming methods that return > boolean values. In this case it should've been called > shouldPaintPreviousSearchProviderIconSpriteFrames(), or something similar. Done. https://chromiumcodereview.appspot.com/1337703002/diff/80001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java (right): https://chromiumcodereview.appspot.com/1337703002/diff/80001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java:72: boolean searchProviderIconSpriteVisible = mSearchPanel.isSearchProviderIconSpriteVisible(); On 2015/09/29 01:14:40, pedrosimonetti wrote: > Nit: Move these a few lines above, right after the line: > > float searchBarShadowOpacity = mSearchPanel.getSearchBarShadowOpacity(); > > This way we keep the variables declarations in the same order as the arguments > in the call below. Done. https://chromiumcodereview.appspot.com/1337703002/diff/80001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://chromiumcodereview.appspot.com/1337703002/diff/80001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:499: !mSearchPanelDelegate.isShowing() On 2015/09/29 01:14:40, pedrosimonetti wrote: > Nit: Encapsulate the entire logic in the > ContextualSearchPolicy#shouldAnimateSearchProviderIconSprite() method, that is, > we should pass a boolean isPanelShowing to that method. Done. https://chromiumcodereview.appspot.com/1337703002/diff/80001/chrome/browser/a... File chrome/browser/android/compositor/layer/crushed_sprite_layer.cc (right): https://chromiumcodereview.appspot.com/1337703002/diff/80001/chrome/browser/a... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:91: // be included in the memory usage. On 2015/09/29 22:23:09, David Trainor wrote: > On 2015/09/29 01:14:40, pedrosimonetti wrote: > > Good question. I don't know the answer. Same applies for the question below. > > I would imagine it's asking memory that this class uses that isn't shared among > other layers/display lists. ie: if this class died, would that lead to the > memory being freed? I could be wrong though. As discussed offline, I'm going to leave in the src_bitmap and previous_frame as part of the memory calculation. https://chromiumcodereview.appspot.com/1337703002/diff/80001/chrome/browser/a... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:113: previous_frame_ = NULL; On 2015/09/29 22:23:09, David Trainor wrote: > I'm not sure this is necessary. It will just run the destructor on the > temporarily created RefPtr, which will be the same as if this class died and the > destructor ran on previous_frame_. Done. https://chromiumcodereview.appspot.com/1337703002/diff/80001/chrome/browser/a... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:115: src_bitmap_.reset(); On 2015/09/29 22:23:09, David Trainor wrote: > Also might not be necessary? It looks like the underlying pixels will be freed > either way. I think you're right. I removed this and previous_frame_ = NULL; if I get memory leak errors, I'll add them back. https://chromiumcodereview.appspot.com/1337703002/diff/80001/chrome/browser/a... File chrome/browser/android/compositor/layer/crushed_sprite_layer.h (right): https://chromiumcodereview.appspot.com/1337703002/diff/80001/chrome/browser/a... chrome/browser/android/compositor/layer/crushed_sprite_layer.h:58: scoped_refptr<cc::PictureLayer> layer_; On 2015/09/30 16:02:46, pedrosimonetti wrote: > Sorry I didn't notice this before, but it feels weird that this is a Layer that > returns another (picture) layer. Wouldn't it be possible to extend the > PictureImageLayer instead and use it (which extends PictureLayer and implements > the ContentLayerClient)? > > In either case, I not familiar with the implementation of those classes, so it > would be good having someone more familiar with it reviewing this class and its > usage in ContextualSearchLayer. PictureImageLayer's constructor is private, so extending it would require changes to that class. For now, I changed CrushedSpriteLayer to extend PictureLayer and dropped this variable. There is some duplicate code between PictureImageLayer and this new class, but not a whole lot, so for this first pass I think it's okay. https://chromiumcodereview.appspot.com/1337703002/diff/80001/ui/android/resou... File ui/android/resources/ui_resource_android.cc (right): https://chromiumcodereview.appspot.com/1337703002/diff/80001/ui/android/resou... ui/android/resources/ui_resource_android.cc:28: // directly rather than exposing this method? I mostly did it this way because On 2015/09/29 01:14:40, pedrosimonetti wrote: > I don't know the answer to this question. Since this is going to undergo another major refactor anyway, I'm just going to leave it like this for now.
twellington@chromium.org changed reviewers: + enne@chromium.org
+enne@ for OWNERS on adding +cc/playback to chrome/browser/android/compositor/DEPS
lgtm Thanks Theresa! Just a couple more nits, but it looks good to me. https://codereview.chromium.org/1337703002/diff/120001/chrome/android/java/re... File chrome/android/java/res/values-ldpi/contextual_search_crushed_sprite_frames.xml (right): https://codereview.chromium.org/1337703002/diff/120001/chrome/android/java/re... chrome/android/java/res/values-ldpi/contextual_search_crushed_sprite_frames.xml:29: <integer-array name="contextual_search_sprite_frame_0"/> Just double checking that these are supposed to be empty. https://codereview.chromium.org/1337703002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java (right): https://codereview.chromium.org/1337703002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:283: float deviceDensity) { Nit: We could store the density from the context that gets passed to the constructor, right? This way we could simplify the method signature by removing the density.
https://codereview.chromium.org/1337703002/diff/120001/build/android/lint/sup... File build/android/lint/suppressions.xml (right): https://codereview.chromium.org/1337703002/diff/120001/build/android/lint/sup... build/android/lint/suppressions.xml:57: <ignore regexp=".*google_icon_sprite.png.*"/> Just confirming: this regex won't cause other IconDipSize warnings to be ignored? And same with the InconsistentArrays regex below. https://codereview.chromium.org/1337703002/diff/120001/chrome/android/java/re... File chrome/android/java/res/values-hdpi/contextual_search_crushed_sprite_frames.xml (right): https://codereview.chromium.org/1337703002/diff/120001/chrome/android/java/re... chrome/android/java/res/values-hdpi/contextual_search_crushed_sprite_frames.xml:8: <array name="contextual_search_sprite_frame_rects"> It would be nice to align the array and frame names with the png names. E.g. name them all google_icon_sprite* or name them all contextual_search_sprite*
https://codereview.chromium.org/1337703002/diff/120001/build/android/lint/sup... File build/android/lint/suppressions.xml (right): https://codereview.chromium.org/1337703002/diff/120001/build/android/lint/sup... build/android/lint/suppressions.xml:57: <ignore regexp=".*google_icon_sprite.png.*"/> On 2015/10/01 17:19:46, newt wrote: > Just confirming: this regex won't cause other IconDipSize warnings to be > ignored? And same with the InconsistentArrays regex below. Yes, it's a different line for each warning for both of these warnings. For IconDipSize, I tested by changing account_manager_on_picture.png and saw this warning with the lint suppression in place: chrome/android/java/res/drawable-mdpi/account_management_no_picture.png The image `account_management_no_picture.png` varies significantly in its density-independent (dip) size across the various density versions: drawable-hdpi/account_management_no_picture.png: 40x40 dp (60x60 px), drawable-xhdpi/account_management_no_picture.png: 40x40 dp (80x80 px), drawable-xxhdpi/account_management_no_picture.png: 40x40 dp (120x120 px), drawable-xxxhdpi/account_management_no_picture.png: 40x40 dp (160x160 px), drawable-mdpi/account_management_no_picture.png: 39x1000 dp (39x1000 px): IconDipSize [warning] For InconsistentArrays, if a second, differently named file has an array with the same name as one of the arrays in contextual_search_crushed_sprite_frames.xml and that array has a different # of items, the error will still be suppressed. If two differently named files have inconsistent arrays then the error will still be surfaced. The InconsistentArrays warnings look like this: chrome/android/java/res/values-hdpi/contextual_search_crushed_sprite_frames.xml:291 Array `contextual_search_sprite_frame_17` has an inconsistent number of items (0 in `values-ldpi/contextual_search_crushed_sprite_frames.xml`, 12 in `values-hdpi/contextual_search_crushed_sprite_frames.xml`): InconsistentArrays [warning] <integer-array name="contextual_search_sprite_frame_17"> ^ I could make the regexp more specific, but I personally don't think it's necessary. https://codereview.chromium.org/1337703002/diff/120001/chrome/android/java/re... File chrome/android/java/res/values-hdpi/contextual_search_crushed_sprite_frames.xml (right): https://codereview.chromium.org/1337703002/diff/120001/chrome/android/java/re... chrome/android/java/res/values-hdpi/contextual_search_crushed_sprite_frames.xml:8: <array name="contextual_search_sprite_frame_rects"> On 2015/10/01 17:19:46, newt wrote: > It would be nice to align the array and frame names with the png names. E.g. > name them all google_icon_sprite* or name them all contextual_search_sprite* Done. https://codereview.chromium.org/1337703002/diff/120001/chrome/android/java/re... File chrome/android/java/res/values-ldpi/contextual_search_crushed_sprite_frames.xml (right): https://codereview.chromium.org/1337703002/diff/120001/chrome/android/java/re... chrome/android/java/res/values-ldpi/contextual_search_crushed_sprite_frames.xml:29: <integer-array name="contextual_search_sprite_frame_0"/> On 2015/10/01 16:38:35, pedrosimonetti wrote: > Just double checking that these are supposed to be empty. Yes. For ldpi, I manually calculated the values for the last frame based on .75 * mdpi values. I didn't want to hand calculate all of the other frames (it likely wouldn't have looked right anyway), so for ldpi we never animate and always just show the last frame. https://codereview.chromium.org/1337703002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java (right): https://codereview.chromium.org/1337703002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:283: float deviceDensity) { On 2015/10/01 16:38:35, pedrosimonetti wrote: > Nit: We could store the density from the context that gets passed to the > constructor, right? This way we could simplify the method signature by removing > the density. Done.
Re: not animating ldpi. What will happen on tvdpi devices or future xxxxhdpi devices or even devices like Nexus 6 which have a density between xxhdpi and xxxhdpi? To handle these cases, the best approach might be to 1) ensure that Android doesn't scale the image when loading it, e.g. by loading the image manually via BitmapFactory, 2) animate the image at its full unscaled size, then 3) if necessary, scale the image when drawing it. https://codereview.chromium.org/1337703002/diff/120001/build/android/lint/sup... File build/android/lint/suppressions.xml (right): https://codereview.chromium.org/1337703002/diff/120001/build/android/lint/sup... build/android/lint/suppressions.xml:57: <ignore regexp=".*google_icon_sprite.png.*"/> On 2015/10/01 17:57:34, Theresa Wellington wrote: > On 2015/10/01 17:19:46, newt wrote: > > Just confirming: this regex won't cause other IconDipSize warnings to be > > ignored? And same with the InconsistentArrays regex below. > > Yes, it's a different line for each warning for both of these warnings. > > For IconDipSize, I tested by changing account_manager_on_picture.png and saw > this warning with the lint suppression in place: > chrome/android/java/res/drawable-mdpi/account_management_no_picture.png The > image `account_management_no_picture.png` varies significantly in its > density-independent (dip) size across the various density versions: > drawable-hdpi/account_management_no_picture.png: 40x40 dp (60x60 px), > drawable-xhdpi/account_management_no_picture.png: 40x40 dp (80x80 px), > drawable-xxhdpi/account_management_no_picture.png: 40x40 dp (120x120 px), > drawable-xxxhdpi/account_management_no_picture.png: 40x40 dp (160x160 px), > drawable-mdpi/account_management_no_picture.png: 39x1000 dp (39x1000 px): > IconDipSize [warning] > > > For InconsistentArrays, if a second, differently named file has an array with > the same name as one of the arrays in > contextual_search_crushed_sprite_frames.xml and that array has a different # of > items, the error will still be suppressed. If two differently named files have > inconsistent arrays then the error will still be surfaced. The > InconsistentArrays warnings look like this: > > chrome/android/java/res/values-hdpi/contextual_search_crushed_sprite_frames.xml:291 > Array `contextual_search_sprite_frame_17` has an inconsistent number of items (0 > in `values-ldpi/contextual_search_crushed_sprite_frames.xml`, 12 in > `values-hdpi/contextual_search_crushed_sprite_frames.xml`): InconsistentArrays > [warning] > <integer-array name="contextual_search_sprite_frame_17"> > ^ > > I could make the regexp more specific, but I personally don't think it's > necessary. Sounds good to me. Thanks for confirming.
On 2015/10/01 18:18:04, newt wrote: > Re: not animating ldpi. What will happen on tvdpi devices or future xxxxhdpi > devices or even devices like Nexus 6 which have a density between xxhdpi and > xxxhdpi? > > To handle these cases, the best approach might be to 1) ensure that Android > doesn't scale the image when loading it, e.g. by loading the image manually via > BitmapFactory, 2) animate the image at its full unscaled size, then 3) if > necessary, scale the image when drawing it. I didn't consider densities :( crap. It looks wrong on the Nexus7 because the px values in the rects that get drawn don't get scaled. Would the algorithm look something like: 1) manually load image at closest resolution 2) pass bitmap + rectangles + resolution for the image + device resolution to the cc layer 3) draw the rectangles unscaled, then scale the canvas based on deviceResolution/imageResolution (I played around with canvas->scale previously and this worked well for xxxhdpi -> xxhpdi so I think it would probably work for other scaling as well)
On 2015/10/01 19:01:51, Theresa Wellington wrote: > On 2015/10/01 18:18:04, newt wrote: > > Re: not animating ldpi. What will happen on tvdpi devices or future xxxxhdpi > > devices or even devices like Nexus 6 which have a density between xxhdpi and > > xxxhdpi? > > > > To handle these cases, the best approach might be to 1) ensure that Android > > doesn't scale the image when loading it, e.g. by loading the image manually > via > > BitmapFactory, 2) animate the image at its full unscaled size, then 3) if > > necessary, scale the image when drawing it. > > I didn't consider densities :( crap. It looks wrong on the Nexus7 because the px > values in the rects that get drawn don't get scaled. > Would the algorithm look something like: > > 1) manually load image at closest resolution > 2) pass bitmap + rectangles + resolution for the image + device resolution to > the cc layer > 3) draw the rectangles unscaled, then scale the canvas based on > deviceResolution/imageResolution (I played around with canvas->scale previously > and this worked well for xxxhdpi -> xxhpdi so I think it would probably work for > other scaling as well) *didn't consider those densities
newt@ / pedrosimonetti@ - ptal My efforts to avoid manual scaling were to no avail, so I'm now loading unscaled bitmaps. I got rid of the xml arrays (since I don't see a way to get a TypedArray at a specific density) and switched to static 2D arrays, which simplified that code a bit. I still need to test across multiple devices to see how the scaling looks (so I may change to mdpi + xxhdpi or to using 3 resource sizes), but I wanted to get some eyes on this overall approach.
Mmmm... this animation is really tricky. I would vote for putting it behind a flag, so we can remotely disable if necessary. Sending a bitmap on every frame doesn't seem right. Could you ask dtrainor@'s feedback about this approach? Please hold submitting this change until we address these issues. https://codereview.chromium.org/1337703002/diff/150001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java (right): https://codereview.chromium.org/1337703002/diff/150001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java:114: searchProviderIconSpriteBitmap, Passing a Bitmap here doesn't sound right, specially because we will call this on every frame. Please confirm with dtrainor@ if he's okay with this approach. I think using the Resource Loader system would be much better, although this might require tweaking the loader to use a custom resolution. https://code.google.com/p/chromium/codesearch#chromium/src/ui/android/java/sr... Another option would be passing the Bitmap only once, but using Resource Loader would be preferable.
Update: I'm not going to try to push this through before the branch (the manual scaling is proving to be complex). This gives me time to implement this more correctly (and move a lot of logic from Java -> native). I'll ping the CL when I'm ready for another review.
ptal - changed implementation to match design doc: https://docs.google.com/document/d/1HcIDfdcc8GPO29D6756eZ8N_hp4jxmQaCRYktJZUP... For the most part, everything was implemented to spec. If anything doesn't match what was agreed upon, please let me know. Things that still need to be done: - Add unit/instrumentation tests - Test animation/scaling on multiple devices and drop some of the assets if possible I'm going to be OOO from 10/14 - 10/21 (inclusive) for Grace Hopper and a recruiting trip . I don't expect to get this change through review and landed before then but I wanted to get eyes on what I have done so far prior to my trip.
On 2015/10/10 02:05:44, Theresa Wellington wrote: > ptal - changed implementation to match design doc: > https://docs.google.com/document/d/1HcIDfdcc8GPO29D6756eZ8N_hp4jxmQaCRYktJZUP... > > For the most part, everything was implemented to spec. If anything doesn't match > what was agreed upon, please let me know. > > Things that still need to be done: > - Add unit/instrumentation tests > - Test animation/scaling on multiple devices and drop some of the assets if > possible > > I'm going to be OOO from 10/14 - 10/21 (inclusive) for Grace Hopper and a > recruiting trip . I don't expect to get this change through review and landed > before then but I wanted to get eyes on what I have done so far prior to my > trip. Also, this CL is getting enormous. I'm amenable to splitting it up (e.g. add support for CrushedSpriteResources, add CrushedSpriteLayer, integrate with Contextual Search) if that's easier/makes sense to reviewers.
https://chromiumcodereview.appspot.com/1337703002/diff/270001/base/android/jn... File base/android/jni_array.cc (right): https://chromiumcodereview.appspot.com/1337703002/diff/270001/base/android/jn... base/android/jni_array.cc:249: jint* ints = env->GetIntArrayElements(int_array.obj(), NULL); nullptr https://chromiumcodereview.appspot.com/1337703002/diff/270001/base/android/jn... File base/android/jni_array_unittest.cc (right): https://chromiumcodereview.appspot.com/1337703002/diff/270001/base/android/jn... base/android/jni_array_unittest.cc:249: env, env->NewObjectArray(kNumItems, int_array_clazz.obj(), NULL)); nullptr https://chromiumcodereview.appspot.com/1337703002/diff/270001/base/android/jn... base/android/jni_array_unittest.cc:268: // Convert to std::vector<std::vector<int>>, check the content. Odd question, do we want to handle empty/null arrays within the 2D array? https://chromiumcodereview.appspot.com/1337703002/diff/270001/build/android/l... File build/android/lint/suppressions.xml (right): https://chromiumcodereview.appspot.com/1337703002/diff/270001/build/android/l... build/android/lint/suppressions.xml:54: <ignore regexp=".*: data_reduction_illustration.png, reader_mode_bar_background.9.png, tabs_moved_htc.png, tabs_moved_nexus.png, tabs_moved_samsung.png$"/> Should we join this line with the line below? It looks like some of these are duplicated. https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/android/... File chrome/android/java/res/raw/google_icon_sprite.json (right): https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/android/... chrome/android/java/res/raw/google_icon_sprite.json:1: { big data file is big. copyright at top? *shrug* https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java (right): https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:917: private ContextualSearchSearchProviderIconSpriteControl mSearchProviderIconSpriteControl; What a name! https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchSearchProviderIconSpriteControl.java (right): https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchSearchProviderIconSpriteControl.java:21: private static final int LAST_SPRITE_FRAME = 18; Can we pull this from the int arrays? Or can we use -1 or some constant to specify last frame? I'd need to see more about how we pass the frame value down, but maybe it would make sense to use a % complete until the last minute? https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/browser/... File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right): https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/browser/... chrome/browser/android/compositor/layer/contextual_search_layer.cc:288: search_bar_height / 2 - fix indenting. https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/browser/... File chrome/browser/android/compositor/layer/crushed_sprite_layer.cc (right): https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:30: resource_ = resource; Ideally we wouldn't hold onto the resource here. I think the general rule is the ResourceManager owns them. I see you got around it with struct wrapper, but I'm wondering if there's a better way to manage that data at a higher level. Ie: The caller of these methods drives whether or not this retains the resource. The tricky part would be dropping the resource from the manager at the right time (other people could want it). But for now you could add a method like releaseCrushedSpriteResource() or something... Not sure what's happening with the Java memory but I'll take a look in a bit :). ie in contextual_search_layer: if (layer_->IsLastFrame() && current_frame == layer_->GetFrame()) { resource_manager_->ReleaseCrushedSpriteResource(magicid); } else if (current_frame != layer_->GetFrame()) { resource = resource_manager_->GetCrushedSpriteResource(magicid); // This doesn't actually hold onto the data, but uses it during this draw. Can check some internal id or something to see if it's a totally different type of crushed sprite resource from last time this was called and reset internal state. layer_->SetFrame(resource, current_frame); } https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:33: void CrushedSpriteLayer::DrawSpriteFrame(int sprite_frame) { Should you clamp sprite_frame between 0 and the max frame? https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:43: !resource_->GetBitmapForLastFrame().empty()) { Do we need this? Can't we rely on the sprite_frame to always be the last one so we skip all of this code? https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:49: if (resource_->BitmapHasBeenEvictedFromMemory()) { See above. Can probably simplify this a lot and remove the inter-dependencies. Push control logic to the caller who knows about how this layer will be used? https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:58: SkCanvas* canvas = new SkCanvas(*bitmap); Leaking canvas and bitmap. Use: // Don't worry, it copies it but doesn't copy the pixel ref. SkBitmap bitmap; // We want canvas to go away when we're done painting. skia::RefPtr<SkCanvas> = skia::AdoptRef(new SkCanavs(bitmap)); https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:67: canvas->drawBitmap(previous_frame_bitmap_, 0, 0, nullptr); So... UIResourceBitmap expects an immututable bitmap. I forget why that is (probably due to assumptions that the UIResourceBitmap always represents the actual SkBitmap). If we can change that, the following makes sense. Otherwise, ignore (but it would be a good straight forward optimization): An interesting thing to try would be to back the canvas with the previous_frame_bitmap_'s canvas and see if you can just use that instead of redrawing it :). Actually, I would probably always back the frame with the previous frame's bitmap (if that works). Then in the cases where you can't use the previous frame I would clear it with canvas_->clear(SkColor). That will skip the bitmap allocation you do every frame. I would check that UIResourceLayer is okay with you changing the contents of the bitmap it holds... and still call SetBitmap() or something to let it know it's invalid. https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:83: if (sprite_frame == resource_->GetFrameCount() - 1) { See above, probably don't need. https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:93: void CrushedSpriteLayer::DrawRectanglesForFrame(int frame, SkCanvas* canvas) { skia::RefPtr<SkCanvas> https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:96: for (auto rect : src_dst_rects) { auto& so we don't copy the object? https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:108: previous_frame_(0) { -1 to say that we haven't drawn the first frame? https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/browser/... File chrome/browser/android/compositor/layer/crushed_sprite_layer.h (right): https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.h:52: scoped_refptr<ui::CrushedSpriteResource> resource_; I think nobody else holds onto resources. We set the relevant data then drop it? https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/java... File ui/android/java/src/org/chromium/ui/resources/ResourceManager.java (right): https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/java... ui/android/java/src/org/chromium/ui/resources/ResourceManager.java:121: if (resType == AndroidResourceType.CRUSHED_SPRITE) { Put this down below the if (mNativeResourceManagerPtr == 0) check. https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/java... ui/android/java/src/org/chromium/ui/resources/ResourceManager.java:175: private void reloadCrushedSpriteResource(int bitmapResId) { Can we just do crushedSpriteResourceRequested()? https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/java... File ui/android/java/src/org/chromium/ui/resources/sprites/CrushedSpriteResource.java (right): https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/java... ui/android/java/src/org/chromium/ui/resources/sprites/CrushedSpriteResource.java:41: public CrushedSpriteResource(int bitmapResId, int metadataResId, Resources resources) { Add trace events around the bitmap and json loading. https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/java... ui/android/java/src/org/chromium/ui/resources/sprites/CrushedSpriteResource.java:62: public static Bitmap loadBitmap(int bitmapResId, Resources resources) { If we still need this, can the constructor use this instead? https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/java... File ui/android/java/src/org/chromium/ui/resources/sprites/CrushedSpriteResourceLoader.java (right): https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/java... ui/android/java/src/org/chromium/ui/resources/sprites/CrushedSpriteResourceLoader.java:19: private ResourceLoaderCallback mCallback; final for both? https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/reso... File ui/android/resources/crushed_sprite_resource.cc (right): https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/reso... ui/android/resources/crushed_sprite_resource.cc:15: int sprite_size) { Should this be an int? Shouldn't it be a gfx::Size or something? https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/reso... File ui/android/resources/crushed_sprite_resource.h (right): https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/reso... ui/android/resources/crushed_sprite_resource.h:13: #include "ui/gfx/android/java_bitmap.h" Can we forward declare this and not include it in the header? https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/reso... ui/android/resources/crushed_sprite_resource.h:23: public base::RefCounted<CrushedSpriteResource> { Does it have to be ref counted? https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/reso... ui/android/resources/crushed_sprite_resource.h:34: const gfx::JavaBitmap& java_bitmap, Would it be better to hide the fact that this came from java from this class? https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/reso... ui/android/resources/crushed_sprite_resource.h:47: void SetBitmapForLastFrame(SkBitmap last_frame_bitmap); I would hide a lot of this and just expose something like the following to the ResourceManager. That could drop the internal resource here... but you could hold onto the metadata. releaseCrushedSprite(int id); Then on the next call to get the resource, reload it if necessary (maybe skip the metadata again like you do actually... I'm not sure what the cleanest way to do that is w.r.t. JNI and Java - store loaded ids in the Java resource loader, have the other method like you do, etc.?). https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/reso... ui/android/resources/crushed_sprite_resource.h:65: int GetBitmapResourceId(); Shouldn't be necessary I think? https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/reso... ui/android/resources/crushed_sprite_resource.h:77: SkBitmap last_frame_bitmap_; Shouldn't need? https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/reso... ui/android/resources/crushed_sprite_resource.h:80: int bitmap_res_id_; We should be able to rely on the res_id from all calls to ResourceManager. https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/reso... File ui/android/resources/resource_manager_impl.cc (right): https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/reso... ui/android/resources/resource_manager_impl.cc:174: for (size_t j = 0; j < frame_ints.size();) { ; j += 6 here instead? https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/reso... File ui/android/resources/resource_manager_impl.h (right): https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/reso... ui/android/resources/resource_manager_impl.h:63: struct CrushedSpriteResourceHolder { Hopefully we don't need this if we don't hold onto a strong reference to the resource in CrushedSpriteLayer.
twellington@chromium.org changed reviewers: - changwan@chromium.org, enne@chromium.org
twellington@chromium.org changed reviewers: - changwan@chromium.org, enne@chromium.org
Thank you for all of the commits David. Fixed some stuff, responded to some other stuff w/ explanations and/or questions. https://codereview.chromium.org/1337703002/diff/270001/base/android/jni_array.cc File base/android/jni_array.cc (right): https://codereview.chromium.org/1337703002/diff/270001/base/android/jni_array... base/android/jni_array.cc:249: jint* ints = env->GetIntArrayElements(int_array.obj(), NULL); On 2015/10/15 21:04:56, David Trainor wrote: > nullptr Done. https://codereview.chromium.org/1337703002/diff/270001/base/android/jni_array... File base/android/jni_array_unittest.cc (right): https://codereview.chromium.org/1337703002/diff/270001/base/android/jni_array... base/android/jni_array_unittest.cc:249: env, env->NewObjectArray(kNumItems, int_array_clazz.obj(), NULL)); On 2015/10/15 21:04:56, David Trainor wrote: > nullptr Done. https://codereview.chromium.org/1337703002/diff/270001/base/android/jni_array... base/android/jni_array_unittest.cc:268: // Convert to std::vector<std::vector<int>>, check the content. On 2015/10/15 21:04:56, David Trainor wrote: > Odd question, do we want to handle empty/null arrays within the 2D array? If we have an animation that displays the same image for a few frames then we could have empty array within the 2D array. It's not needed for this patch but wouldn't hurt to test that it works. I think null arrays within the 2D array shouldn't be supported. I added an entry for an empty array to this test and it still passes. I'll keep that case in mind while writing other tests (and probably manually test as well). https://codereview.chromium.org/1337703002/diff/270001/build/android/lint/sup... File build/android/lint/suppressions.xml (right): https://codereview.chromium.org/1337703002/diff/270001/build/android/lint/sup... build/android/lint/suppressions.xml:54: <ignore regexp=".*: data_reduction_illustration.png, reader_mode_bar_background.9.png, tabs_moved_htc.png, tabs_moved_nexus.png, tabs_moved_samsung.png$"/> On 2015/10/15 21:04:57, David Trainor wrote: > Should we join this line with the line below? It looks like some of these are > duplicated. They're duplicated because there's a different lint warning produced for each density that's missing a certain image(s); I don't remember exactly why I had to duplicate the lines.. currently it's not needed because the crushed sprite is in every density but I'll drop some of the densities before landing. I'll try to make the regexp smarter (make the expected inclusion of " google_icon_sprite.png," 0-1 rather than creating a completely separate list). I added a TODO note for myself (that will be done before the CL is landed). I also still need to figure out which resolutions we need to keep and which ones we can cut and still have the animation look good on various devices. https://codereview.chromium.org/1337703002/diff/270001/chrome/android/java/re... File chrome/android/java/res/raw/google_icon_sprite.json (right): https://codereview.chromium.org/1337703002/diff/270001/chrome/android/java/re... chrome/android/java/res/raw/google_icon_sprite.json:1: { On 2015/10/15 21:04:57, David Trainor wrote: > big data file is big. > > copyright at top? *shrug* JSON doesn't/shouldn't have comments explicitly: http://google-styleguide.googlecode.com/svn/trunk/jsoncstyleguide.xml?showone... I can add a "_comment" field above or below the version w/ the copyright. Since it's raw data (like an image or other... data things) it seemed less important to copyright than code. I updated a couple other things to better match the style guide https://codereview.chromium.org/1337703002/diff/270001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java (right): https://codereview.chromium.org/1337703002/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:917: private ContextualSearchSearchProviderIconSpriteControl mSearchProviderIconSpriteControl; On 2015/10/15 21:04:57, David Trainor wrote: > What a name! Acknowledged. I'm hoping somebody creates a Chromium badge similar to that camelCase badge. I think I'm winning ;) https://codereview.chromium.org/1337703002/diff/270001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchSearchProviderIconSpriteControl.java (right): https://codereview.chromium.org/1337703002/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchSearchProviderIconSpriteControl.java:21: private static final int LAST_SPRITE_FRAME = 18; On 2015/10/15 21:04:57, David Trainor wrote: > Can we pull this from the int arrays? Or can we use -1 or some constant to > specify last frame? I'd need to see more about how we pass the frame value > down, but maybe it would make sense to use a % complete until the last minute? We could pass the animation completion percent and let the CrushedSpriteLayer figure out which frame to display based on the %. I'd prefer not use -1 to indicate the last frame should be drawn, but I have no strong reasoning why so would be okay with that as well. Pedro, what do you think about passing the % to native instead of the frame to draw? https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right): https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... chrome/browser/android/compositor/layer/contextual_search_layer.cc:288: search_bar_height / 2 - On 2015/10/15 21:04:57, David Trainor wrote: > fix indenting. Done. https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... File chrome/browser/android/compositor/layer/crushed_sprite_layer.cc (right): https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:30: resource_ = resource; On 2015/10/15 21:04:57, David Trainor wrote: > Ideally we wouldn't hold onto the resource here. I think the general rule is > the ResourceManager owns them. I see you got around it with struct wrapper, but > I'm wondering if there's a better way to manage that data at a higher level. > Ie: The caller of these methods drives whether or not this retains the > resource. > > The tricky part would be dropping the resource from the manager at the right > time (other people could want it). But for now you could add a method like > releaseCrushedSpriteResource() or something... Not sure what's happening with > the Java memory but I'll take a look in a bit :). > > ie in contextual_search_layer: > if (layer_->IsLastFrame() && current_frame == layer_->GetFrame()) { > resource_manager_->ReleaseCrushedSpriteResource(magicid); > } else if (current_frame != layer_->GetFrame()) { > resource = resource_manager_->GetCrushedSpriteResource(magicid); > // This doesn't actually hold onto the data, but uses it during this draw. > Can check some internal id or something to see if it's a totally different type > of crushed sprite resource from last time this was called and reset internal > state. > layer_->SetFrame(resource, current_frame); > } I got rid of this method and just pass the resource in to DrawSpriteFrame. See response to other comment wrt to releasing the crushed sprite from memory in contextual_search_layer. https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:33: void CrushedSpriteLayer::DrawSpriteFrame(int sprite_frame) { On 2015/10/15 21:04:57, David Trainor wrote: > Should you clamp sprite_frame between 0 and the max frame? Done. https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:43: !resource_->GetBitmapForLastFrame().empty()) { On 2015/10/15 21:04:57, David Trainor wrote: > Do we need this? Can't we rely on the sprite_frame to always be the last one so > we skip all of this code? I think you mean rely on previous_frame_bitmap to be the last one? (sprite_frame is whatever frame we're currently drawing) I implemented it this way because I thought the resource could/would outlive the layer. Also, the resource could be shared between multiple layers, whereas the previous_frame_bitmap is not. I looked into this more and I think the contextual search layer only gets created once then lives in perpetuity until Chrome is closed, so for the current implementation, using previous_frame_bitmap instead of caching the bitmap for the last frame with the resource would work/perform equally well. https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:49: if (resource_->BitmapHasBeenEvictedFromMemory()) { On 2015/10/15 21:04:57, David Trainor wrote: > See above. Can probably simplify this a lot and remove the inter-dependencies. > Push control logic to the caller who knows about how this layer will be used? I put the logic for evicting the sprite sheet from memory/using the cached bitmap for the last frame/reloading the crushed sprite if necessary here because I think (hope) it will be useful for more than just the search provider icon animation. By default, I think we don't want crushed sprite sheets hanging around in memory after the animation is finished. But I would be okay with moving this logic and the above logic to contextual_search_layer.cc since nobody else uses this layer yet. https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:58: SkCanvas* canvas = new SkCanvas(*bitmap); On 2015/10/15 21:04:57, David Trainor wrote: > Leaking canvas and bitmap. Use: > > // Don't worry, it copies it but doesn't copy the pixel ref. > SkBitmap bitmap; > > // We want canvas to go away when we're done painting. > skia::RefPtr<SkCanvas> = skia::AdoptRef(new SkCanavs(bitmap)); Done. https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:67: canvas->drawBitmap(previous_frame_bitmap_, 0, 0, nullptr); On 2015/10/15 21:04:57, David Trainor wrote: > So... UIResourceBitmap expects an immututable bitmap. I forget why that is > (probably due to assumptions that the UIResourceBitmap always represents the > actual SkBitmap). If we can change that, the following makes sense. Otherwise, > ignore (but it would be a good straight forward optimization): > > An interesting thing to try would be to back the canvas with the > previous_frame_bitmap_'s canvas and see if you can just use that instead of > redrawing it :). > > Actually, I would probably always back the frame with the previous frame's > bitmap (if that works). Then in the cases where you can't use the previous > frame I would clear it with canvas_->clear(SkColor). That will skip the bitmap > allocation you do every frame. I would check that UIResourceLayer is okay with > you changing the contents of the bitmap it holds... and still call SetBitmap() > or something to let it know it's invalid. I'm not sure why it has to be immutable (it just has a comment saying it must be then does a bunch of DCHECK's to make sure it is). git blame says powei@chormium.org wrote that code.. any idea on who that is (ldap)? https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:93: void CrushedSpriteLayer::DrawRectanglesForFrame(int frame, SkCanvas* canvas) { On 2015/10/15 21:04:57, David Trainor wrote: > skia::RefPtr<SkCanvas> Done. https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:96: for (auto rect : src_dst_rects) { On 2015/10/15 21:04:57, David Trainor wrote: > auto& so we don't copy the object? Done. https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:108: previous_frame_(0) { On 2015/10/15 21:04:57, David Trainor wrote: > -1 to say that we haven't drawn the first frame? Done. https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... File chrome/browser/android/compositor/layer/crushed_sprite_layer.h (right): https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... chrome/browser/android/compositor/layer/crushed_sprite_layer.h:52: scoped_refptr<ui::CrushedSpriteResource> resource_; On 2015/10/15 21:04:57, David Trainor wrote: > I think nobody else holds onto resources. We set the relevant data then drop > it? Done. https://codereview.chromium.org/1337703002/diff/270001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/resources/ResourceManager.java (right): https://codereview.chromium.org/1337703002/diff/270001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/resources/ResourceManager.java:121: if (resType == AndroidResourceType.CRUSHED_SPRITE) { On 2015/10/15 21:04:57, David Trainor wrote: > Put this down below the if (mNativeResourceManagerPtr == 0) check. Done. https://codereview.chromium.org/1337703002/diff/270001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/resources/ResourceManager.java:175: private void reloadCrushedSpriteResource(int bitmapResId) { On 2015/10/15 21:04:57, David Trainor wrote: > Can we just do crushedSpriteResourceRequested()? The implementation is completely different. reloadResource() calls a static CrushedSpriteResource.loadBitmap without reloading any of the metadata, and it returns only the bitmap (so we're not passing the 2D array unnecessarily). https://codereview.chromium.org/1337703002/diff/270001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/resources/sprites/CrushedSpriteResource.java (right): https://codereview.chromium.org/1337703002/diff/270001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/resources/sprites/CrushedSpriteResource.java:41: public CrushedSpriteResource(int bitmapResId, int metadataResId, Resources resources) { On 2015/10/15 21:04:57, David Trainor wrote: > Add trace events around the bitmap and json loading. Done. https://codereview.chromium.org/1337703002/diff/270001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/resources/sprites/CrushedSpriteResource.java:62: public static Bitmap loadBitmap(int bitmapResId, Resources resources) { On 2015/10/15 21:04:57, David Trainor wrote: > If we still need this, can the constructor use this instead? Done. https://codereview.chromium.org/1337703002/diff/270001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/resources/sprites/CrushedSpriteResourceLoader.java (right): https://codereview.chromium.org/1337703002/diff/270001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/resources/sprites/CrushedSpriteResourceLoader.java:19: private ResourceLoaderCallback mCallback; On 2015/10/15 21:04:57, David Trainor wrote: > final for both? Done. https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/c... File ui/android/resources/crushed_sprite_resource.cc (right): https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/c... ui/android/resources/crushed_sprite_resource.cc:15: int sprite_size) { On 2015/10/15 21:04:57, David Trainor wrote: > Should this be an int? Shouldn't it be a gfx::Size or something? I changed it to gfx::Size. Currently the sprites are square but that's certainly not a requirement going forward. https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/c... File ui/android/resources/crushed_sprite_resource.h (right): https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/c... ui/android/resources/crushed_sprite_resource.h:13: #include "ui/gfx/android/java_bitmap.h" On 2015/10/15 21:04:57, David Trainor wrote: > Can we forward declare this and not include it in the header? I can; I would also need to forward declare or include SkBitmap. Are there guidelines for when things should be forward-declared vs included? https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/c... ui/android/resources/crushed_sprite_resource.h:23: public base::RefCounted<CrushedSpriteResource> { On 2015/10/15 21:04:58, David Trainor wrote: > Does it have to be ref counted? I wanted it to get automatically destroyed when it wasn't being used any longer, but I think it doesn't actually stop getting "used" until Chrome closes so, no, it doesn't. If I remove the RefCounted, do I need to explicitly destroy all of the CrushedSpriteResources being held in the ResourceManager when it gets destroyed? https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/c... ui/android/resources/crushed_sprite_resource.h:34: const gfx::JavaBitmap& java_bitmap, On 2015/10/15 21:04:58, David Trainor wrote: > Would it be better to hide the fact that this came from java from this class? I modeled it off the old UIResourceAndroid https://codereview.chromium.org/1371523003/patch/160001/170002) -- since it's in the ui/android/resources package I was expecting that all the bitmaps used to create the CrushedSpriteResource would come from Java. But I don't have a strong opinion on this and am okay with changing it. https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/c... ui/android/resources/crushed_sprite_resource.h:47: void SetBitmapForLastFrame(SkBitmap last_frame_bitmap); On 2015/10/15 21:04:58, David Trainor wrote: > I would hide a lot of this and just expose something like the following to the > ResourceManager. That could drop the internal resource here... but you could > hold onto the metadata. > > releaseCrushedSprite(int id); > > Then on the next call to get the resource, reload it if necessary (maybe skip > the metadata again like you do actually... I'm not sure what the cleanest way to > do that is w.r.t. JNI and Java - store loaded ids in the Java resource loader, > have the other method like you do, etc.?). contextual_search_layer will call getResource each time we update the layer, but we only need to reload the bitmap if something other than the last frame is being displayed or the animation is being run. If we rely on getResource to reload when necessary, I think that method will have to take a boolean to know when it's necessary. wrt to releasing in the ResourceManager; I could add a ReleaseCrushedSprite(int id) method that would do something like resource->GetBitmap()->release(). I don't really have a preference as to whether crushed_sprite_layer (or contextual_search_layer) calls into the resource_manager or the resource itself to release the bitmap. https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/c... ui/android/resources/crushed_sprite_resource.h:65: int GetBitmapResourceId(); On 2015/10/15 21:04:58, David Trainor wrote: > Shouldn't be necessary I think? This method is currently being used to reload the resource if necessary (in crushed_sprite_layer.cc). https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/c... ui/android/resources/crushed_sprite_resource.h:80: int bitmap_res_id_; On 2015/10/15 21:04:58, David Trainor wrote: > We should be able to rely on the res_id from all calls to ResourceManager. If the call to reload the crushed sprite resource is moved to contextual_search_layer.cc, that works. Otherwise, crushed_sprite_layer.cc needs someway to get the res_id for the bitmap. It could just be passed from contextual_search_layer.cc instead of stored here. https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/r... File ui/android/resources/resource_manager_impl.cc (right): https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/r... ui/android/resources/resource_manager_impl.cc:174: for (size_t j = 0; j < frame_ints.size();) { On 2015/10/15 21:04:58, David Trainor wrote: > ; j += 6 here instead? Done. Yep, the way I had it is just silly. https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/r... File ui/android/resources/resource_manager_impl.h (right): https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/r... ui/android/resources/resource_manager_impl.h:63: struct CrushedSpriteResourceHolder { On 2015/10/15 21:04:58, David Trainor wrote: > Hopefully we don't need this if we don't hold onto a strong reference to the > resource in CrushedSpriteLayer. That's correct. I only created the struct because the IDMap doesn't work with scoped_refptr's.
Thank you for all of the commits David. Fixed some stuff, responded to some other stuff w/ explanations and/or questions. https://codereview.chromium.org/1337703002/diff/270001/base/android/jni_array.cc File base/android/jni_array.cc (right): https://codereview.chromium.org/1337703002/diff/270001/base/android/jni_array... base/android/jni_array.cc:249: jint* ints = env->GetIntArrayElements(int_array.obj(), NULL); On 2015/10/15 21:04:56, David Trainor wrote: > nullptr Done. https://codereview.chromium.org/1337703002/diff/270001/base/android/jni_array... File base/android/jni_array_unittest.cc (right): https://codereview.chromium.org/1337703002/diff/270001/base/android/jni_array... base/android/jni_array_unittest.cc:249: env, env->NewObjectArray(kNumItems, int_array_clazz.obj(), NULL)); On 2015/10/15 21:04:56, David Trainor wrote: > nullptr Done. https://codereview.chromium.org/1337703002/diff/270001/base/android/jni_array... base/android/jni_array_unittest.cc:268: // Convert to std::vector<std::vector<int>>, check the content. On 2015/10/15 21:04:56, David Trainor wrote: > Odd question, do we want to handle empty/null arrays within the 2D array? If we have an animation that displays the same image for a few frames then we could have empty array within the 2D array. It's not needed for this patch but wouldn't hurt to test that it works. I think null arrays within the 2D array shouldn't be supported. I added an entry for an empty array to this test and it still passes. I'll keep that case in mind while writing other tests (and probably manually test as well). https://codereview.chromium.org/1337703002/diff/270001/build/android/lint/sup... File build/android/lint/suppressions.xml (right): https://codereview.chromium.org/1337703002/diff/270001/build/android/lint/sup... build/android/lint/suppressions.xml:54: <ignore regexp=".*: data_reduction_illustration.png, reader_mode_bar_background.9.png, tabs_moved_htc.png, tabs_moved_nexus.png, tabs_moved_samsung.png$"/> On 2015/10/15 21:04:57, David Trainor wrote: > Should we join this line with the line below? It looks like some of these are > duplicated. They're duplicated because there's a different lint warning produced for each density that's missing a certain image(s); I don't remember exactly why I had to duplicate the lines.. currently it's not needed because the crushed sprite is in every density but I'll drop some of the densities before landing. I'll try to make the regexp smarter (make the expected inclusion of " google_icon_sprite.png," 0-1 rather than creating a completely separate list). I added a TODO note for myself (that will be done before the CL is landed). I also still need to figure out which resolutions we need to keep and which ones we can cut and still have the animation look good on various devices. https://codereview.chromium.org/1337703002/diff/270001/chrome/android/java/re... File chrome/android/java/res/raw/google_icon_sprite.json (right): https://codereview.chromium.org/1337703002/diff/270001/chrome/android/java/re... chrome/android/java/res/raw/google_icon_sprite.json:1: { On 2015/10/15 21:04:57, David Trainor wrote: > big data file is big. > > copyright at top? *shrug* JSON doesn't/shouldn't have comments explicitly: http://google-styleguide.googlecode.com/svn/trunk/jsoncstyleguide.xml?showone... I can add a "_comment" field above or below the version w/ the copyright. Since it's raw data (like an image or other... data things) it seemed less important to copyright than code. I updated a couple other things to better match the style guide https://codereview.chromium.org/1337703002/diff/270001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java (right): https://codereview.chromium.org/1337703002/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:917: private ContextualSearchSearchProviderIconSpriteControl mSearchProviderIconSpriteControl; On 2015/10/15 21:04:57, David Trainor wrote: > What a name! Acknowledged. I'm hoping somebody creates a Chromium badge similar to that camelCase badge. I think I'm winning ;) https://codereview.chromium.org/1337703002/diff/270001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchSearchProviderIconSpriteControl.java (right): https://codereview.chromium.org/1337703002/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchSearchProviderIconSpriteControl.java:21: private static final int LAST_SPRITE_FRAME = 18; On 2015/10/15 21:04:57, David Trainor wrote: > Can we pull this from the int arrays? Or can we use -1 or some constant to > specify last frame? I'd need to see more about how we pass the frame value > down, but maybe it would make sense to use a % complete until the last minute? We could pass the animation completion percent and let the CrushedSpriteLayer figure out which frame to display based on the %. I'd prefer not use -1 to indicate the last frame should be drawn, but I have no strong reasoning why so would be okay with that as well. Pedro, what do you think about passing the % to native instead of the frame to draw? https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right): https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... chrome/browser/android/compositor/layer/contextual_search_layer.cc:288: search_bar_height / 2 - On 2015/10/15 21:04:57, David Trainor wrote: > fix indenting. Done. https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... File chrome/browser/android/compositor/layer/crushed_sprite_layer.cc (right): https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:30: resource_ = resource; On 2015/10/15 21:04:57, David Trainor wrote: > Ideally we wouldn't hold onto the resource here. I think the general rule is > the ResourceManager owns them. I see you got around it with struct wrapper, but > I'm wondering if there's a better way to manage that data at a higher level. > Ie: The caller of these methods drives whether or not this retains the > resource. > > The tricky part would be dropping the resource from the manager at the right > time (other people could want it). But for now you could add a method like > releaseCrushedSpriteResource() or something... Not sure what's happening with > the Java memory but I'll take a look in a bit :). > > ie in contextual_search_layer: > if (layer_->IsLastFrame() && current_frame == layer_->GetFrame()) { > resource_manager_->ReleaseCrushedSpriteResource(magicid); > } else if (current_frame != layer_->GetFrame()) { > resource = resource_manager_->GetCrushedSpriteResource(magicid); > // This doesn't actually hold onto the data, but uses it during this draw. > Can check some internal id or something to see if it's a totally different type > of crushed sprite resource from last time this was called and reset internal > state. > layer_->SetFrame(resource, current_frame); > } I got rid of this method and just pass the resource in to DrawSpriteFrame. See response to other comment wrt to releasing the crushed sprite from memory in contextual_search_layer. https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:33: void CrushedSpriteLayer::DrawSpriteFrame(int sprite_frame) { On 2015/10/15 21:04:57, David Trainor wrote: > Should you clamp sprite_frame between 0 and the max frame? Done. https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:43: !resource_->GetBitmapForLastFrame().empty()) { On 2015/10/15 21:04:57, David Trainor wrote: > Do we need this? Can't we rely on the sprite_frame to always be the last one so > we skip all of this code? I think you mean rely on previous_frame_bitmap to be the last one? (sprite_frame is whatever frame we're currently drawing) I implemented it this way because I thought the resource could/would outlive the layer. Also, the resource could be shared between multiple layers, whereas the previous_frame_bitmap is not. I looked into this more and I think the contextual search layer only gets created once then lives in perpetuity until Chrome is closed, so for the current implementation, using previous_frame_bitmap instead of caching the bitmap for the last frame with the resource would work/perform equally well. https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:49: if (resource_->BitmapHasBeenEvictedFromMemory()) { On 2015/10/15 21:04:57, David Trainor wrote: > See above. Can probably simplify this a lot and remove the inter-dependencies. > Push control logic to the caller who knows about how this layer will be used? I put the logic for evicting the sprite sheet from memory/using the cached bitmap for the last frame/reloading the crushed sprite if necessary here because I think (hope) it will be useful for more than just the search provider icon animation. By default, I think we don't want crushed sprite sheets hanging around in memory after the animation is finished. But I would be okay with moving this logic and the above logic to contextual_search_layer.cc since nobody else uses this layer yet. https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:58: SkCanvas* canvas = new SkCanvas(*bitmap); On 2015/10/15 21:04:57, David Trainor wrote: > Leaking canvas and bitmap. Use: > > // Don't worry, it copies it but doesn't copy the pixel ref. > SkBitmap bitmap; > > // We want canvas to go away when we're done painting. > skia::RefPtr<SkCanvas> = skia::AdoptRef(new SkCanavs(bitmap)); Done. https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:67: canvas->drawBitmap(previous_frame_bitmap_, 0, 0, nullptr); On 2015/10/15 21:04:57, David Trainor wrote: > So... UIResourceBitmap expects an immututable bitmap. I forget why that is > (probably due to assumptions that the UIResourceBitmap always represents the > actual SkBitmap). If we can change that, the following makes sense. Otherwise, > ignore (but it would be a good straight forward optimization): > > An interesting thing to try would be to back the canvas with the > previous_frame_bitmap_'s canvas and see if you can just use that instead of > redrawing it :). > > Actually, I would probably always back the frame with the previous frame's > bitmap (if that works). Then in the cases where you can't use the previous > frame I would clear it with canvas_->clear(SkColor). That will skip the bitmap > allocation you do every frame. I would check that UIResourceLayer is okay with > you changing the contents of the bitmap it holds... and still call SetBitmap() > or something to let it know it's invalid. I'm not sure why it has to be immutable (it just has a comment saying it must be then does a bunch of DCHECK's to make sure it is). git blame says powei@chormium.org wrote that code.. any idea on who that is (ldap)? https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:93: void CrushedSpriteLayer::DrawRectanglesForFrame(int frame, SkCanvas* canvas) { On 2015/10/15 21:04:57, David Trainor wrote: > skia::RefPtr<SkCanvas> Done. https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:96: for (auto rect : src_dst_rects) { On 2015/10/15 21:04:57, David Trainor wrote: > auto& so we don't copy the object? Done. https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:108: previous_frame_(0) { On 2015/10/15 21:04:57, David Trainor wrote: > -1 to say that we haven't drawn the first frame? Done. https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... File chrome/browser/android/compositor/layer/crushed_sprite_layer.h (right): https://codereview.chromium.org/1337703002/diff/270001/chrome/browser/android... chrome/browser/android/compositor/layer/crushed_sprite_layer.h:52: scoped_refptr<ui::CrushedSpriteResource> resource_; On 2015/10/15 21:04:57, David Trainor wrote: > I think nobody else holds onto resources. We set the relevant data then drop > it? Done. https://codereview.chromium.org/1337703002/diff/270001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/resources/ResourceManager.java (right): https://codereview.chromium.org/1337703002/diff/270001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/resources/ResourceManager.java:121: if (resType == AndroidResourceType.CRUSHED_SPRITE) { On 2015/10/15 21:04:57, David Trainor wrote: > Put this down below the if (mNativeResourceManagerPtr == 0) check. Done. https://codereview.chromium.org/1337703002/diff/270001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/resources/ResourceManager.java:175: private void reloadCrushedSpriteResource(int bitmapResId) { On 2015/10/15 21:04:57, David Trainor wrote: > Can we just do crushedSpriteResourceRequested()? The implementation is completely different. reloadResource() calls a static CrushedSpriteResource.loadBitmap without reloading any of the metadata, and it returns only the bitmap (so we're not passing the 2D array unnecessarily). https://codereview.chromium.org/1337703002/diff/270001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/resources/sprites/CrushedSpriteResource.java (right): https://codereview.chromium.org/1337703002/diff/270001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/resources/sprites/CrushedSpriteResource.java:41: public CrushedSpriteResource(int bitmapResId, int metadataResId, Resources resources) { On 2015/10/15 21:04:57, David Trainor wrote: > Add trace events around the bitmap and json loading. Done. https://codereview.chromium.org/1337703002/diff/270001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/resources/sprites/CrushedSpriteResource.java:62: public static Bitmap loadBitmap(int bitmapResId, Resources resources) { On 2015/10/15 21:04:57, David Trainor wrote: > If we still need this, can the constructor use this instead? Done. https://codereview.chromium.org/1337703002/diff/270001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/resources/sprites/CrushedSpriteResourceLoader.java (right): https://codereview.chromium.org/1337703002/diff/270001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/resources/sprites/CrushedSpriteResourceLoader.java:19: private ResourceLoaderCallback mCallback; On 2015/10/15 21:04:57, David Trainor wrote: > final for both? Done. https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/c... File ui/android/resources/crushed_sprite_resource.cc (right): https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/c... ui/android/resources/crushed_sprite_resource.cc:15: int sprite_size) { On 2015/10/15 21:04:57, David Trainor wrote: > Should this be an int? Shouldn't it be a gfx::Size or something? I changed it to gfx::Size. Currently the sprites are square but that's certainly not a requirement going forward. https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/c... File ui/android/resources/crushed_sprite_resource.h (right): https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/c... ui/android/resources/crushed_sprite_resource.h:13: #include "ui/gfx/android/java_bitmap.h" On 2015/10/15 21:04:57, David Trainor wrote: > Can we forward declare this and not include it in the header? I can; I would also need to forward declare or include SkBitmap. Are there guidelines for when things should be forward-declared vs included? https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/c... ui/android/resources/crushed_sprite_resource.h:23: public base::RefCounted<CrushedSpriteResource> { On 2015/10/15 21:04:58, David Trainor wrote: > Does it have to be ref counted? I wanted it to get automatically destroyed when it wasn't being used any longer, but I think it doesn't actually stop getting "used" until Chrome closes so, no, it doesn't. If I remove the RefCounted, do I need to explicitly destroy all of the CrushedSpriteResources being held in the ResourceManager when it gets destroyed? https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/c... ui/android/resources/crushed_sprite_resource.h:34: const gfx::JavaBitmap& java_bitmap, On 2015/10/15 21:04:58, David Trainor wrote: > Would it be better to hide the fact that this came from java from this class? I modeled it off the old UIResourceAndroid https://codereview.chromium.org/1371523003/patch/160001/170002) -- since it's in the ui/android/resources package I was expecting that all the bitmaps used to create the CrushedSpriteResource would come from Java. But I don't have a strong opinion on this and am okay with changing it. https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/c... ui/android/resources/crushed_sprite_resource.h:47: void SetBitmapForLastFrame(SkBitmap last_frame_bitmap); On 2015/10/15 21:04:58, David Trainor wrote: > I would hide a lot of this and just expose something like the following to the > ResourceManager. That could drop the internal resource here... but you could > hold onto the metadata. > > releaseCrushedSprite(int id); > > Then on the next call to get the resource, reload it if necessary (maybe skip > the metadata again like you do actually... I'm not sure what the cleanest way to > do that is w.r.t. JNI and Java - store loaded ids in the Java resource loader, > have the other method like you do, etc.?). contextual_search_layer will call getResource each time we update the layer, but we only need to reload the bitmap if something other than the last frame is being displayed or the animation is being run. If we rely on getResource to reload when necessary, I think that method will have to take a boolean to know when it's necessary. wrt to releasing in the ResourceManager; I could add a ReleaseCrushedSprite(int id) method that would do something like resource->GetBitmap()->release(). I don't really have a preference as to whether crushed_sprite_layer (or contextual_search_layer) calls into the resource_manager or the resource itself to release the bitmap. https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/c... ui/android/resources/crushed_sprite_resource.h:65: int GetBitmapResourceId(); On 2015/10/15 21:04:58, David Trainor wrote: > Shouldn't be necessary I think? This method is currently being used to reload the resource if necessary (in crushed_sprite_layer.cc). https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/c... ui/android/resources/crushed_sprite_resource.h:80: int bitmap_res_id_; On 2015/10/15 21:04:58, David Trainor wrote: > We should be able to rely on the res_id from all calls to ResourceManager. If the call to reload the crushed sprite resource is moved to contextual_search_layer.cc, that works. Otherwise, crushed_sprite_layer.cc needs someway to get the res_id for the bitmap. It could just be passed from contextual_search_layer.cc instead of stored here. https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/r... File ui/android/resources/resource_manager_impl.cc (right): https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/r... ui/android/resources/resource_manager_impl.cc:174: for (size_t j = 0; j < frame_ints.size();) { On 2015/10/15 21:04:58, David Trainor wrote: > ; j += 6 here instead? Done. Yep, the way I had it is just silly. https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/r... File ui/android/resources/resource_manager_impl.h (right): https://codereview.chromium.org/1337703002/diff/270001/ui/android/resources/r... ui/android/resources/resource_manager_impl.h:63: struct CrushedSpriteResourceHolder { On 2015/10/15 21:04:58, David Trainor wrote: > Hopefully we don't need this if we don't hold onto a strong reference to the > resource in CrushedSpriteLayer. That's correct. I only created the struct because the IDMap doesn't work with scoped_refptr's.
Thank you for all of the comments David. Fixed some stuff, responded to some other stuff w/ explanations and/or questions. Also uploaded a patch set with some of the changes (might make sense to look at those or you can just wait until I address all of the review comments).
https://codereview.chromium.org/1337703002/diff/270001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchSearchProviderIconSpriteControl.java (right): https://codereview.chromium.org/1337703002/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchSearchProviderIconSpriteControl.java:21: private static final int LAST_SPRITE_FRAME = 18; On 2015/10/24 00:06:44, Theresa Wellington wrote: > On 2015/10/15 21:04:57, David Trainor wrote: > > Can we pull this from the int arrays? Or can we use -1 or some constant to > > specify last frame? I'd need to see more about how we pass the frame value > > down, but maybe it would make sense to use a % complete until the last minute? > > We could pass the animation completion percent and let the CrushedSpriteLayer > figure out which frame to display based on the %. I'd prefer not use -1 to > indicate the last frame should be drawn, but I have no strong reasoning why so > would be okay with that as well. > Pedro, what do you think about passing the % to native instead of the frame to > draw? Per an offline discussion w/ Pedro, I'll change it to pass the completion % and let the crushed sprite layer figure out which frame to draw.
Just a couple comments regarding David's feedback on the previous patch. I still need to take another look though. https://codereview.chromium.org/1337703002/diff/270001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java (right): https://codereview.chromium.org/1337703002/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:917: private ContextualSearchSearchProviderIconSpriteControl mSearchProviderIconSpriteControl; Yeah, I agree with David. Double "Search" feels weird. I choosing a shorter name like ContextualSearchIconSpriteControl. Similarly, mSearchProviderIconSpriteControl, would be called mIconSpriteControl (without "Search"). https://codereview.chromium.org/1337703002/diff/270001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchSearchProviderIconSpriteControl.java (right): https://codereview.chromium.org/1337703002/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchSearchProviderIconSpriteControl.java:21: private static final int LAST_SPRITE_FRAME = 18; On 2015/10/24 00:06:44, Theresa Wellington wrote: > On 2015/10/15 21:04:57, David Trainor wrote: > > Can we pull this from the int arrays? Or can we use -1 or some constant to > > specify last frame? I'd need to see more about how we pass the frame value > > down, but maybe it would make sense to use a % complete until the last minute? > > We could pass the animation completion percent and let the CrushedSpriteLayer > figure out which frame to display based on the %. I'd prefer not use -1 to > indicate the last frame should be drawn, but I have no strong reasoning why so > would be okay with that as well. > Pedro, what do you think about passing the % to native instead of the frame to > draw? I agree with David here too. We shouldn't hardcode the number of frames here, passing a percentage would be better.
https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/browser/... File chrome/browser/android/compositor/layer/crushed_sprite_layer.cc (right): https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:43: !resource_->GetBitmapForLastFrame().empty()) { On 2015/10/24 00:06:44, Theresa Wellington wrote: > On 2015/10/15 21:04:57, David Trainor wrote: > > Do we need this? Can't we rely on the sprite_frame to always be the last one > so > > we skip all of this code? > > I think you mean rely on previous_frame_bitmap to be the last one? (sprite_frame > is whatever frame we're currently drawing) > > I implemented it this way because I thought the resource could/would outlive the > layer. Also, the resource could be shared between multiple layers, whereas the > previous_frame_bitmap is not. > > I looked into this more and I think the contextual search layer only gets > created once then lives in perpetuity until Chrome is closed, so for the current > implementation, using previous_frame_bitmap instead of caching the bitmap for > the last frame with the resource would work/perform equally well. I see your point. I'd be okay with sharing it. Something to fix though, calling layer_->SetBitmap() builds a new internal ScopedUIResource IIRC. That will build a new texture, which means we're not sharing the bitmap on the GPU. Can we save the last frame bitmap to a UI resource internally in CrushedSpriteResource? https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:49: if (resource_->BitmapHasBeenEvictedFromMemory()) { On 2015/10/24 00:06:45, Theresa Wellington wrote: > On 2015/10/15 21:04:57, David Trainor wrote: > > See above. Can probably simplify this a lot and remove the > inter-dependencies. > > Push control logic to the caller who knows about how this layer will be used? > > I put the logic for evicting the sprite sheet from memory/using the cached > bitmap for the last frame/reloading the crushed sprite if necessary here because > I think (hope) it will be useful for more than just the search provider icon > animation. > > By default, I think we don't want crushed sprite sheets hanging around in memory > after the animation is finished. But I would be okay with moving this logic and > the above logic to contextual_search_layer.cc since nobody else uses this layer > yet. Won't this cause reloads during animations if two crushed sprite layers are drawing? https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:67: canvas->drawBitmap(previous_frame_bitmap_, 0, 0, nullptr); On 2015/10/24 00:06:45, Theresa Wellington wrote: > On 2015/10/15 21:04:57, David Trainor wrote: > > So... UIResourceBitmap expects an immututable bitmap. I forget why that is > > (probably due to assumptions that the UIResourceBitmap always represents the > > actual SkBitmap). If we can change that, the following makes sense. > Otherwise, > > ignore (but it would be a good straight forward optimization): > > > > An interesting thing to try would be to back the canvas with the > > previous_frame_bitmap_'s canvas and see if you can just use that instead of > > redrawing it :). > > > > Actually, I would probably always back the frame with the previous frame's > > bitmap (if that works). Then in the cases where you can't use the previous > > frame I would clear it with canvas_->clear(SkColor). That will skip the > bitmap > > allocation you do every frame. I would check that UIResourceLayer is okay > with > > you changing the contents of the bitmap it holds... and still call SetBitmap() > > or something to let it know it's invalid. > > I'm not sure why it has to be immutable (it just has a comment saying it must be > then does a bunch of DCHECK's to make sure it is). git blame says > mailto:powei@chormium.org wrote that code.. any idea on who that is (ldap)? I'm not sure if he still works here. I would imagine it's because we want the GL texture to always represent the bitmap and we can't guarantee that if it's immutable. https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/java... File ui/android/java/src/org/chromium/ui/resources/ResourceManager.java (right): https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/java... ui/android/java/src/org/chromium/ui/resources/ResourceManager.java:175: private void reloadCrushedSpriteResource(int bitmapResId) { On 2015/10/24 00:06:45, Theresa Wellington wrote: > On 2015/10/15 21:04:57, David Trainor wrote: > > Can we just do crushedSpriteResourceRequested()? > > The implementation is completely different. reloadResource() calls a static > CrushedSpriteResource.loadBitmap without reloading any of the metadata, and it > returns only the bitmap (so we're not passing the 2D array unnecessarily). I was hoping we could hide the difference internally inside the load request. ie: The loader knows it already loaded the 2D array. But if that doesn't make sense we can add the APIs. https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/reso... File ui/android/resources/crushed_sprite_resource.h (right): https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/reso... ui/android/resources/crushed_sprite_resource.h:13: #include "ui/gfx/android/java_bitmap.h" On 2015/10/24 00:06:46, Theresa Wellington wrote: > On 2015/10/15 21:04:57, David Trainor wrote: > > Can we forward declare this and not include it in the header? > > I can; I would also need to forward declare or include SkBitmap. Are there > guidelines for when things should be forward-declared vs included? I would always forward declare if you can. https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/reso... ui/android/resources/crushed_sprite_resource.h:23: public base::RefCounted<CrushedSpriteResource> { On 2015/10/24 00:06:45, Theresa Wellington wrote: > On 2015/10/15 21:04:58, David Trainor wrote: > > Does it have to be ref counted? > > I wanted it to get automatically destroyed when it wasn't being used any longer, > but I think it doesn't actually stop getting "used" until Chrome closes so, no, > it doesn't. > > If I remove the RefCounted, do I need to explicitly destroy all of the > CrushedSpriteResources being held in the ResourceManager when it gets destroyed? IIRC the IDMap is set to IDMapOwnPointer so it'll auto-delete the object when it's removed from the map. https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/reso... ui/android/resources/crushed_sprite_resource.h:34: const gfx::JavaBitmap& java_bitmap, On 2015/10/24 00:06:45, Theresa Wellington wrote: > On 2015/10/15 21:04:58, David Trainor wrote: > > Would it be better to hide the fact that this came from java from this class? > > I modeled it off the old UIResourceAndroid > https://codereview.chromium.org/1371523003/patch/160001/170002) -- since it's in > the ui/android/resources package I was expecting that all the bitmaps used to > create the CrushedSpriteResource would come from Java. But I don't have a strong > opinion on this and am okay with changing it. Acknowledged. https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/reso... ui/android/resources/crushed_sprite_resource.h:47: void SetBitmapForLastFrame(SkBitmap last_frame_bitmap); On 2015/10/24 00:06:46, Theresa Wellington wrote: > On 2015/10/15 21:04:58, David Trainor wrote: > > I would hide a lot of this and just expose something like the following to the > > ResourceManager. That could drop the internal resource here... but you could > > hold onto the metadata. > > > > releaseCrushedSprite(int id); > > > > Then on the next call to get the resource, reload it if necessary (maybe skip > > the metadata again like you do actually... I'm not sure what the cleanest way > to > > do that is w.r.t. JNI and Java - store loaded ids in the Java resource loader, > > have the other method like you do, etc.?). > > contextual_search_layer will call getResource each time we update the layer, but > we only need to reload the bitmap if something other than the last frame is > being displayed or the animation is being run. If we rely on getResource to > reload when necessary, I think that method will have to take a boolean to know > when it's necessary. > > wrt to releasing in the ResourceManager; I could add a ReleaseCrushedSprite(int > id) method that would do something like resource->GetBitmap()->release(). I > don't really have a preference as to whether crushed_sprite_layer (or > contextual_search_layer) calls into the resource_manager or the resource itself > to release the bitmap. Yeah this is the downside of sharing the bitmap through here. We're sharing the crushed sprite layer bitmap inside the resource which we're trying to drop because we don't actually want the resource :(. So this creates a situation where we need to add these extra APIs to do this... it might be cleaner to have the crushed_sprite_layer have a static map of some ID -> ref counted last frame UIResource and rely on that instead or something. https://chromiumcodereview.appspot.com/1337703002/diff/290001/chrome/browser/... File chrome/browser/android/compositor/layer/crushed_sprite_layer.cc (right): https://chromiumcodereview.appspot.com/1337703002/diff/290001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:29: scoped_refptr<ui::CrushedSpriteResource> resource, Do we still need scoped_refptr<> if the resource isn't held internally? https://chromiumcodereview.appspot.com/1337703002/diff/290001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:97: for (auto& rect : src_dst_rects) { Actually... const auto& :). Sorry!
I think this most recent patchset addresses all of David's comments. As discussed with Pedro offline, I still need to add back in support for showing a static icon. I also still need to add the resource_manager_impl_unittest (I did add a Java CrushedSpriteResourceTest) https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java (right): https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:917: private ContextualSearchSearchProviderIconSpriteControl mSearchProviderIconSpriteControl; On 2015/10/24 00:29:00, pedrosimonetti wrote: > Yeah, I agree with David. Double "Search" feels weird. I choosing a shorter name > like ContextualSearchIconSpriteControl. > > Similarly, mSearchProviderIconSpriteControl, would be called mIconSpriteControl > (without "Search"). Done. Updated in other places as well. https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchSearchProviderIconSpriteControl.java (right): https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchSearchProviderIconSpriteControl.java:21: private static final int LAST_SPRITE_FRAME = 18; On 2015/10/24 00:29:00, pedrosimonetti wrote: > On 2015/10/24 00:06:44, Theresa Wellington wrote: > > On 2015/10/15 21:04:57, David Trainor wrote: > > > Can we pull this from the int arrays? Or can we use -1 or some constant to > > > specify last frame? I'd need to see more about how we pass the frame value > > > down, but maybe it would make sense to use a % complete until the last > minute? > > > > We could pass the animation completion percent and let the CrushedSpriteLayer > > figure out which frame to display based on the %. I'd prefer not use -1 to > > indicate the last frame should be drawn, but I have no strong reasoning why so > > would be okay with that as well. > > Pedro, what do you think about passing the % to native instead of the frame to > > draw? > > I agree with David here too. We shouldn't hardcode the number of frames here, > passing a percentage would be better. Done. https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/browser/... File chrome/browser/android/compositor/layer/crushed_sprite_layer.cc (right): https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:43: !resource_->GetBitmapForLastFrame().empty()) { On 2015/10/27 15:14:47, David Trainor wrote: > On 2015/10/24 00:06:44, Theresa Wellington wrote: > > On 2015/10/15 21:04:57, David Trainor wrote: > > > Do we need this? Can't we rely on the sprite_frame to always be the last > one > > so > > > we skip all of this code? > > > > I think you mean rely on previous_frame_bitmap to be the last one? > (sprite_frame > > is whatever frame we're currently drawing) > > > > I implemented it this way because I thought the resource could/would outlive > the > > layer. Also, the resource could be shared between multiple layers, whereas the > > previous_frame_bitmap is not. > > > > I looked into this more and I think the contextual search layer only gets > > created once then lives in perpetuity until Chrome is closed, so for the > current > > implementation, using previous_frame_bitmap instead of caching the bitmap for > > the last frame with the resource would work/perform equally well. > > I see your point. I'd be okay with sharing it. Something to fix though, > calling layer_->SetBitmap() builds a new internal ScopedUIResource IIRC. That > will build a new texture, which means we're not sharing the bitmap on the GPU. > Can we save the last frame bitmap to a UI resource internally in > CrushedSpriteResource? Done. https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:49: if (resource_->BitmapHasBeenEvictedFromMemory()) { On 2015/10/27 15:14:47, David Trainor wrote: > On 2015/10/24 00:06:45, Theresa Wellington wrote: > > On 2015/10/15 21:04:57, David Trainor wrote: > > > See above. Can probably simplify this a lot and remove the > > inter-dependencies. > > > Push control logic to the caller who knows about how this layer will be > used? > > > > I put the logic for evicting the sprite sheet from memory/using the cached > > bitmap for the last frame/reloading the crushed sprite if necessary here > because > > I think (hope) it will be useful for more than just the search provider icon > > animation. > > > > By default, I think we don't want crushed sprite sheets hanging around in > memory > > after the animation is finished. But I would be okay with moving this logic > and > > the above logic to contextual_search_layer.cc since nobody else uses this > layer > > yet. > > Won't this cause reloads during animations if two crushed sprite layers are > drawing? As discussed offline, yes, it could cause unnecessary reloads but it's unlikely that two layers will use the same crushed sprite resource. I moved the reloading logic to the ResourceManager. https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:67: canvas->drawBitmap(previous_frame_bitmap_, 0, 0, nullptr); On 2015/10/27 15:14:47, David Trainor wrote: > On 2015/10/24 00:06:45, Theresa Wellington wrote: > > On 2015/10/15 21:04:57, David Trainor wrote: > > > So... UIResourceBitmap expects an immututable bitmap. I forget why that is > > > (probably due to assumptions that the UIResourceBitmap always represents the > > > actual SkBitmap). If we can change that, the following makes sense. > > Otherwise, > > > ignore (but it would be a good straight forward optimization): > > > > > > An interesting thing to try would be to back the canvas with the > > > previous_frame_bitmap_'s canvas and see if you can just use that instead of > > > redrawing it :). > > > > > > Actually, I would probably always back the frame with the previous frame's > > > bitmap (if that works). Then in the cases where you can't use the previous > > > frame I would clear it with canvas_->clear(SkColor). That will skip the > > bitmap > > > allocation you do every frame. I would check that UIResourceLayer is okay > > with > > > you changing the contents of the bitmap it holds... and still call > SetBitmap() > > > or something to let it know it's invalid. > > > > I'm not sure why it has to be immutable (it just has a comment saying it must > be > > then does a bunch of DCHECK's to make sure it is). git blame says > > mailto:powei@chormium.org wrote that code.. any idea on who that is (ldap)? > > I'm not sure if he still works here. I would imagine it's because we want the > GL texture to always represent the bitmap and we can't guarantee that if it's > immutable. Acknowledged. https://chromiumcodereview.appspot.com/1337703002/diff/270001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:83: if (sprite_frame == resource_->GetFrameCount() - 1) { On 2015/10/15 21:04:57, David Trainor wrote: > See above, probably don't need. I left the bitmap memory ejection here but moved the reloading logic to ResourceManager->GetCrushedSpriteResource https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/java... File ui/android/java/src/org/chromium/ui/resources/ResourceManager.java (right): https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/java... ui/android/java/src/org/chromium/ui/resources/ResourceManager.java:175: private void reloadCrushedSpriteResource(int bitmapResId) { On 2015/10/27 15:14:47, David Trainor wrote: > On 2015/10/24 00:06:45, Theresa Wellington wrote: > > On 2015/10/15 21:04:57, David Trainor wrote: > > > Can we just do crushedSpriteResourceRequested()? > > > > The implementation is completely different. reloadResource() calls a static > > CrushedSpriteResource.loadBitmap without reloading any of the metadata, and it > > returns only the bitmap (so we're not passing the 2D array unnecessarily). > > I was hoping we could hide the difference internally inside the load request. > ie: The loader knows it already loaded the 2D array. But if that doesn't make > sense we can add the APIs. I dropped this method and added a boolean to crushedSpriteResourceRequested() https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/reso... File ui/android/resources/crushed_sprite_resource.h (right): https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/reso... ui/android/resources/crushed_sprite_resource.h:13: #include "ui/gfx/android/java_bitmap.h" On 2015/10/27 15:14:47, David Trainor wrote: > On 2015/10/24 00:06:46, Theresa Wellington wrote: > > On 2015/10/15 21:04:57, David Trainor wrote: > > > Can we forward declare this and not include it in the header? > > > > I can; I would also need to forward declare or include SkBitmap. Are there > > guidelines for when things should be forward-declared vs included? > > I would always forward declare if you can. Done. https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/reso... ui/android/resources/crushed_sprite_resource.h:23: public base::RefCounted<CrushedSpriteResource> { On 2015/10/27 15:14:47, David Trainor wrote: > On 2015/10/24 00:06:45, Theresa Wellington wrote: > > On 2015/10/15 21:04:58, David Trainor wrote: > > > Does it have to be ref counted? > > > > I wanted it to get automatically destroyed when it wasn't being used any > longer, > > but I think it doesn't actually stop getting "used" until Chrome closes so, > no, > > it doesn't. > > > > If I remove the RefCounted, do I need to explicitly destroy all of the > > CrushedSpriteResources being held in the ResourceManager when it gets > destroyed? > > IIRC the IDMap is set to IDMapOwnPointer so it'll auto-delete the object when > it's removed from the map. Done. https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/reso... ui/android/resources/crushed_sprite_resource.h:34: const gfx::JavaBitmap& java_bitmap, On 2015/10/27 15:14:47, David Trainor wrote: > On 2015/10/24 00:06:45, Theresa Wellington wrote: > > On 2015/10/15 21:04:58, David Trainor wrote: > > > Would it be better to hide the fact that this came from java from this > class? > > > > I modeled it off the old UIResourceAndroid > > https://codereview.chromium.org/1371523003/patch/160001/170002) -- since it's > in > > the ui/android/resources package I was expecting that all the bitmaps used to > > create the CrushedSpriteResource would come from Java. But I don't have a > strong > > opinion on this and am okay with changing it. > > Acknowledged. Changed to an SkBitmap https://chromiumcodereview.appspot.com/1337703002/diff/270001/ui/android/reso... ui/android/resources/crushed_sprite_resource.h:47: void SetBitmapForLastFrame(SkBitmap last_frame_bitmap); On 2015/10/27 15:14:47, David Trainor wrote: > On 2015/10/24 00:06:46, Theresa Wellington wrote: > > On 2015/10/15 21:04:58, David Trainor wrote: > > > I would hide a lot of this and just expose something like the following to > the > > > ResourceManager. That could drop the internal resource here... but you > could > > > hold onto the metadata. > > > > > > releaseCrushedSprite(int id); > > > > > > Then on the next call to get the resource, reload it if necessary (maybe > skip > > > the metadata again like you do actually... I'm not sure what the cleanest > way > > to > > > do that is w.r.t. JNI and Java - store loaded ids in the Java resource > loader, > > > have the other method like you do, etc.?). > > > > contextual_search_layer will call getResource each time we update the layer, > but > > we only need to reload the bitmap if something other than the last frame is > > being displayed or the animation is being run. If we rely on getResource to > > reload when necessary, I think that method will have to take a boolean to know > > when it's necessary. > > > > wrt to releasing in the ResourceManager; I could add a > ReleaseCrushedSprite(int > > id) method that would do something like resource->GetBitmap()->release(). I > > don't really have a preference as to whether crushed_sprite_layer (or > > contextual_search_layer) calls into the resource_manager or the resource > itself > > to release the bitmap. > > Yeah this is the downside of sharing the bitmap through here. We're sharing the > crushed sprite layer bitmap inside the resource which we're trying to drop > because we don't actually want the resource :(. So this creates a situation > where we need to add these extra APIs to do this... it might be cleaner to have > the crushed_sprite_layer have a static map of some ID -> ref counted last frame > UIResource and rely on that instead or something. As discussed offline,removed the logic for caching the last frame on the UIResource. I turns out to be totally unnecessary (at least for this patchset). The resource isn't shared (and it's unlikely that animation resources will be shared between layers in the future). If we're re-displaying the last_frame, then the CrushedSpriteLayer actually does nothing at all since the last bitmap set on the UIResourceLayer the CrushedSpriteLayer contains will be for the last frame. https://chromiumcodereview.appspot.com/1337703002/diff/290001/chrome/browser/... File chrome/browser/android/compositor/layer/crushed_sprite_layer.cc (right): https://chromiumcodereview.appspot.com/1337703002/diff/290001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:29: scoped_refptr<ui::CrushedSpriteResource> resource, On 2015/10/27 15:14:47, David Trainor wrote: > Do we still need scoped_refptr<> if the resource isn't held internally? Nope. Removed the ref counting. https://chromiumcodereview.appspot.com/1337703002/diff/290001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:97: for (auto& rect : src_dst_rects) { On 2015/10/27 15:14:47, David Trainor wrote: > Actually... const auto& :). Sorry! Done.
Here are some more comments. Will take a look at your most recent patch next. https://codereview.chromium.org/1337703002/diff/290001/chrome/android/java/re... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/1337703002/diff/290001/chrome/android/java/re... chrome/android/java/res/values/dimens.xml:80: <dimen name="contextual_search_sprite_size">36dp</dimen> Is this being used? I would prefer getting this dimension from the json file. https://codereview.chromium.org/1337703002/diff/290001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java (right): https://codereview.chromium.org/1337703002/diff/290001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:659: private ContextualSearchSearchProviderIconSpriteControl mSearchProviderIconSpriteControl; Nit: Can we name this class ContextualSearchIconSpriteControl? https://codereview.chromium.org/1337703002/diff/290001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:663: mSearchProviderIconSpriteControl = Nit: Similarly, can we name this mIconSpriteControl? https://codereview.chromium.org/1337703002/diff/290001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java (left): https://codereview.chromium.org/1337703002/diff/290001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java:88: R.drawable.google_icon, As we discussed offline, would it be possible to keep the old code for rendering a static logo? The reason is that, for now, Reader Mode will use our rendering code, at least until we find a better way to separate the rendering into reusable components. The crushed sprite icon and the static icon are mutually exclusive, which means you could check whether a crushed sprite resource was passed. If it was not, then the static icon should be used. https://codereview.chromium.org/1337703002/diff/290001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java (right): https://codereview.chromium.org/1337703002/diff/290001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java:127: searchProviderIconSpriteFrame, Shouldn't we pass the percentage instead? https://codereview.chromium.org/1337703002/diff/290001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/1337703002/diff/290001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:512: ContextualSearchPolicy.getInstance(mActivity).shouldAnimateSearchProviderIcon( ContextualSearchPolicy.getInstance(mActivity) --> mPolicy https://codereview.chromium.org/1337703002/diff/290001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java (right): https://codereview.chromium.org/1337703002/diff/290001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:332: return DisableablePromoTapCounter.getInstance(mPreferenceManager).isEnabled(); As we've discussed offline, this counter is for tap only, so we shouldn't be using that. I think we could use the getPromoOpenCount(), but that will only work if the user is undecided (we only update the count if the promo is showing, which happens when the user is undecided). If the logic is that we should show it for long press only if the user has never opened the panel before that would be OK since if the user has never opened the panel, then the user must be undecided (can only decide after opening the panel). In either case, please write down some comment explaining this logic and why we are reusing the PromoOpenCount, which is not immediately obvious to an uninformed reader. https://codereview.chromium.org/1337703002/diff/290001/chrome/browser/android... File chrome/browser/android/compositor/layer/contextual_search_layer.cc (left): https://codereview.chromium.org/1337703002/diff/290001/chrome/browser/android... chrome/browser/android/compositor/layer/contextual_search_layer.cc:269: // Centers the Search Provider Icon vertically in the Search Bar. As we discussed offline, please keep this code around for now, so that Reader Mode can reuse our rendering logic. https://codereview.chromium.org/1337703002/diff/290001/chrome/browser/android... File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right): https://codereview.chromium.org/1337703002/diff/290001/chrome/browser/android... chrome/browser/android/compositor/layer/contextual_search_layer.cc:111: add a new boolean here should_use_static_icon (or something similar) and check for whether the crushed sprite resource has been passed. Use this boolean to determine when to render the static version of the icon, or the crushed sprite version. https://codereview.chromium.org/1337703002/diff/290001/chrome/browser/android... File chrome/browser/android/compositor/layer/contextual_search_layer.h (left): https://codereview.chromium.org/1337703002/diff/290001/chrome/browser/android... chrome/browser/android/compositor/layer/contextual_search_layer.h:92: scoped_refptr<cc::UIResourceLayer> search_provider_icon_; As we discussed offline, keep this Layer and add a new one for the crushed sprite.
lgtm % pedro's comments https://chromiumcodereview.appspot.com/1337703002/diff/310001/chrome/browser/... File chrome/browser/android/compositor/layer/crushed_sprite_layer.cc (right): https://chromiumcodereview.appspot.com/1337703002/diff/310001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:36: if (frame_count_ == -1 || sprite_frame != previous_frame_) { Do we need frame_count_ == -1 if previous_frame will start at -1? https://chromiumcodereview.appspot.com/1337703002/diff/310001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:42: frame_count_ = resource->GetFrameCount(); Move up above the other sprite_frame https://chromiumcodereview.appspot.com/1337703002/diff/310001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:43: int sprite_frame = completion_percentage * (frame_count_ - 1); redundant? https://chromiumcodereview.appspot.com/1337703002/diff/310001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:81: resource->GetBitmap().empty(); Doesn't this just return if it's empty? Also do we want to drop the SkBitmap completely? https://chromiumcodereview.appspot.com/1337703002/diff/310001/ui/android/reso... File ui/android/resources/crushed_sprite_resource.cc (right): https://chromiumcodereview.appspot.com/1337703002/diff/310001/ui/android/reso... ui/android/resources/crushed_sprite_resource.cc:25: bitmap_ = bitmap; Can probably just leave it immutable at this point :).
Added a test to resource_manager_impl_unittests. I'll finish addressing comments this evening after I get back from teaching - looks like the CL is getting close! :) For tracking purposes, I still need to figure out which resolutions can be dropped, get newt@ to re-review specific Java files, and add a reviewer for the jni_array. https://chromiumcodereview.appspot.com/1337703002/diff/310001/chrome/browser/... File chrome/browser/android/compositor/layer/crushed_sprite_layer.cc (right): https://chromiumcodereview.appspot.com/1337703002/diff/310001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:36: if (frame_count_ == -1 || sprite_frame != previous_frame_) { On 2015/10/27 21:00:42, David Trainor wrote: > Do we need frame_count_ == -1 if previous_frame will start at -1? Maybe.. if the completion percentage starts at .5 then sprite_frame = .5 * (-1 - 1) = -1 That seems unlikely but better safe than sorry? https://chromiumcodereview.appspot.com/1337703002/diff/310001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:42: frame_count_ = resource->GetFrameCount(); On 2015/10/27 21:00:42, David Trainor wrote: > Move up above the other sprite_frame It's here on purpose. If we're redisplaying the last frame (and do nothing) we don't want to call GetCrushedSpriteResource or the bitmap will (possibly) be reloaded. But we need to at least try to calculate the sprite_frame to know if we're redisplaying. https://chromiumcodereview.appspot.com/1337703002/diff/310001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:43: int sprite_frame = completion_percentage * (frame_count_ - 1); On 2015/10/27 21:00:43, David Trainor wrote: > redundant? If frame_count_ != -1, yes. I'll surround this line and the one above it with that check before recalculating. https://chromiumcodereview.appspot.com/1337703002/diff/310001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:81: resource->GetBitmap().empty(); On 2015/10/27 21:00:42, David Trainor wrote: > Doesn't this just return if it's empty? Also do we want to drop the SkBitmap > completely? Discussed offline, I'll use .reset() and add a method to the resource itself to evict the spritesheet.
https://chromiumcodereview.appspot.com/1337703002/diff/290001/chrome/android/... File chrome/android/java/res/values/dimens.xml (right): https://chromiumcodereview.appspot.com/1337703002/diff/290001/chrome/android/... chrome/android/java/res/values/dimens.xml:80: <dimen name="contextual_search_sprite_size">36dp</dimen> On 2015/10/27 20:32:00, pedrosimonetti wrote: > Is this being used? I would prefer getting this dimension from the json file. Yes, it's being used to set the layer bounds and scale the sprite. I think it belongs here - the display size is unique to contextual search, not the search provider sprite itself. If the same animation is used somewhere else at a smaller (or larger) dp, this dp value will change but the JSON should not. Also, setting it here in the xml means that we can rely Android to automatically translate the dp to a px value for us rather than manually converting the value read from JSON. https://chromiumcodereview.appspot.com/1337703002/diff/290001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java (right): https://chromiumcodereview.appspot.com/1337703002/diff/290001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:659: private ContextualSearchSearchProviderIconSpriteControl mSearchProviderIconSpriteControl; On 2015/10/27 20:32:00, pedrosimonetti wrote: > Nit: Can we name this class ContextualSearchIconSpriteControl? Done a few patchsets ago. Sorry there's been so much churn on this CL. https://chromiumcodereview.appspot.com/1337703002/diff/290001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java (left): https://chromiumcodereview.appspot.com/1337703002/diff/290001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java:88: R.drawable.google_icon, On 2015/10/27 20:32:00, pedrosimonetti wrote: > As we discussed offline, would it be possible to keep the old code for rendering > a static logo? The reason is that, for now, Reader Mode will use our rendering > code, at least until we find a better way to separate the rendering into > reusable components. > > The crushed sprite icon and the static icon are mutually exclusive, which means > you could check whether a crushed sprite resource was passed. If it was not, > then the static icon should be used. Done. https://chromiumcodereview.appspot.com/1337703002/diff/290001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java (right): https://chromiumcodereview.appspot.com/1337703002/diff/290001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java:127: searchProviderIconSpriteFrame, On 2015/10/27 20:32:00, pedrosimonetti wrote: > Shouldn't we pass the percentage instead? Yes, done a few patchsets ago. https://chromiumcodereview.appspot.com/1337703002/diff/290001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java (right): https://chromiumcodereview.appspot.com/1337703002/diff/290001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:332: return DisableablePromoTapCounter.getInstance(mPreferenceManager).isEnabled(); On 2015/10/27 20:32:00, pedrosimonetti wrote: > As we've discussed offline, this counter is for tap only, so we shouldn't be > using that. > > I think we could use the getPromoOpenCount(), but that will only work if the > user is undecided (we only update the count if the promo is showing, which > happens when the user is undecided). > > If the logic is that we should show it for long press only if the user has never > opened the panel before that would be OK since if the user has never opened the > panel, then the user must be undecided (can only decide after opening the > panel). > > In either case, please write down some comment explaining this logic and why we > are reusing the PromoOpenCount, which is not immediately obvious to an > uninformed reader. Done. https://chromiumcodereview.appspot.com/1337703002/diff/290001/chrome/browser/... File chrome/browser/android/compositor/layer/contextual_search_layer.cc (left): https://chromiumcodereview.appspot.com/1337703002/diff/290001/chrome/browser/... chrome/browser/android/compositor/layer/contextual_search_layer.cc:269: // Centers the Search Provider Icon vertically in the Search Bar. On 2015/10/27 20:32:00, pedrosimonetti wrote: > As we discussed offline, please keep this code around for now, so that Reader > Mode can reuse our rendering logic. Done. https://chromiumcodereview.appspot.com/1337703002/diff/290001/chrome/browser/... File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right): https://chromiumcodereview.appspot.com/1337703002/diff/290001/chrome/browser/... chrome/browser/android/compositor/layer/contextual_search_layer.cc:111: On 2015/10/27 20:32:00, pedrosimonetti wrote: > add a new boolean here should_use_static_icon (or something similar) and check > for whether the crushed sprite resource has been passed. Use this boolean to > determine when to render the static version of the icon, or the crushed sprite > version. Done. I'm using a static resource id of 0 to indicate that the crushed sprite version should be used. I think that's safe and requires one less param to passed through JNI. https://chromiumcodereview.appspot.com/1337703002/diff/290001/chrome/browser/... File chrome/browser/android/compositor/layer/contextual_search_layer.h (left): https://chromiumcodereview.appspot.com/1337703002/diff/290001/chrome/browser/... chrome/browser/android/compositor/layer/contextual_search_layer.h:92: scoped_refptr<cc::UIResourceLayer> search_provider_icon_; On 2015/10/27 20:32:00, pedrosimonetti wrote: > As we discussed offline, keep this Layer and add a new one for the crushed > sprite. Done. https://chromiumcodereview.appspot.com/1337703002/diff/310001/chrome/browser/... File chrome/browser/android/compositor/layer/crushed_sprite_layer.cc (right): https://chromiumcodereview.appspot.com/1337703002/diff/310001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:43: int sprite_frame = completion_percentage * (frame_count_ - 1); On 2015/10/27 21:24:49, Theresa Wellington wrote: > On 2015/10/27 21:00:43, David Trainor wrote: > > redundant? > > If frame_count_ != -1, yes. I'll surround this line and the one above it with > that check before recalculating. Done. https://chromiumcodereview.appspot.com/1337703002/diff/310001/chrome/browser/... chrome/browser/android/compositor/layer/crushed_sprite_layer.cc:81: resource->GetBitmap().empty(); On 2015/10/27 21:24:49, Theresa Wellington wrote: > On 2015/10/27 21:00:42, David Trainor wrote: > > Doesn't this just return if it's empty? Also do we want to drop the SkBitmap > > completely? > > Discussed offline, I'll use .reset() and add a method to the resource itself to > evict the spritesheet. Done. https://chromiumcodereview.appspot.com/1337703002/diff/310001/ui/android/reso... File ui/android/resources/crushed_sprite_resource.cc (right): https://chromiumcodereview.appspot.com/1337703002/diff/310001/ui/android/reso... ui/android/resources/crushed_sprite_resource.cc:25: bitmap_ = bitmap; On 2015/10/27 21:00:43, David Trainor wrote: > Can probably just leave it immutable at this point :). Done.
twellington@chromium.org changed reviewers: + rmcilroy@chromium.org
+rmcilroy@ - ptal at base/android/* changes. I added a method (and a test) for converting a Java int[][] to std::vector<std::vector<int>>
https://chromiumcodereview.appspot.com/1337703002/diff/390001/base/android/jn... File base/android/jni_array.cc (right): https://chromiumcodereview.appspot.com/1337703002/diff/390001/base/android/jn... base/android/jni_array.cc:232: jbyte* bytes = env->GetByteArrayElements(bytes_array.obj(), NULL); nit - please make this nullptr too https://chromiumcodereview.appspot.com/1337703002/diff/390001/base/android/jn... base/android/jni_array.cc:251: vector_ints.resize(array_len); Is there a reason you can't use assign here, i.e: (*out)[i].assign(reinterpret_cast<const int*>(ints), array_len); ? https://chromiumcodereview.appspot.com/1337703002/diff/390001/base/android/jn... File base/android/jni_array.h (right): https://chromiumcodereview.appspot.com/1337703002/diff/390001/base/android/jn... base/android/jni_array.h:102: BASE_EXPORT void JavaArrayOfIntArrayToIntVector( Please add a comment, like JavaArrayOfByteArrayToStringVector above
A few more comments. https://codereview.chromium.org/1337703002/diff/390001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/1337703002/diff/390001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:512: ContextualSearchPolicy.getInstance(mActivity).shouldAnimateSearchProviderIcon( ContextualSearchPolicy.getInstance(mActivity) --> mPolicy https://codereview.chromium.org/1337703002/diff/390001/chrome/browser/android... File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right): https://codereview.chromium.org/1337703002/diff/390001/chrome/browser/android... chrome/browser/android/compositor/layer/contextual_search_layer.cc:112: I still think it's worth adding a new boolean: // Put comment here explaining that zero means static icon will be used. bool should_use_static_icon = panel_icon_resource_id != 0; This way it's easier to understand what "panel_icon_resource_id != 0" actually means. https://codereview.chromium.org/1337703002/diff/390001/chrome/browser/android... chrome/browser/android/compositor/layer/contextual_search_layer.cc:330: if (search_provider_icon_sprite_->layer().get() && We should only execute this when not rendering the static icon. The way it is written now, this will be executed every time a static icon is shown. This condition should be put inside a "if (!should_use_static_icon)" block, like this: if (!should_use_static_icon) { if (search_provider_icon_sprite_visible) { // add layer and render animated icon } else { // remove layer } } https://codereview.chromium.org/1337703002/diff/390001/chrome/browser/android... chrome/browser/android/compositor/layer/contextual_search_layer.cc:631: panel_icon_->SetIsDrawable(true); Don't we need to call SetIsDrawable() on the sprite layer as well? https://codereview.chromium.org/1337703002/diff/390001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/resources/sprites/CrushedSpriteResource.java (right): https://codereview.chromium.org/1337703002/diff/390001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/resources/sprites/CrushedSpriteResource.java:172: assert name.equals("apiVersion"); Nit: Shouldn't we check that the version number is supported?
rmcilroy@ & pedrosimonetti@ - addressed review comments, please take another look newt@ - ptal at build/android/lint/suppressions.xml, chrome/android/java/res/* & and other files you feel like checking. I uploaded screenshots of what the sprite looks like at various resolutions to the bug if anyone would like to see those. https://codereview.chromium.org/1337703002/diff/390001/base/android/jni_array.cc File base/android/jni_array.cc (right): https://codereview.chromium.org/1337703002/diff/390001/base/android/jni_array... base/android/jni_array.cc:232: jbyte* bytes = env->GetByteArrayElements(bytes_array.obj(), NULL); On 2015/10/28 11:22:11, rmcilroy wrote: > nit - please make this nullptr too Done. https://codereview.chromium.org/1337703002/diff/390001/base/android/jni_array... base/android/jni_array.cc:251: vector_ints.resize(array_len); On 2015/10/28 11:22:11, rmcilroy wrote: > Is there a reason you can't use assign here, i.e: > (*out)[i].assign(reinterpret_cast<const int*>(ints), array_len); > > ? I get a reinterpret cast error: ../../base/android/jni_array.cc:251:67: error: invalid conversion from 'const int*' to 'std::__1::vector<int>::size_type {aka unsigned int}' [-fpermissive] (*out)[i].assign(reinterpret_cast<const int*>(ints), array_len); I simplified this method to call JavaIntArrayToIntVector instead. https://codereview.chromium.org/1337703002/diff/390001/base/android/jni_array.h File base/android/jni_array.h (right): https://codereview.chromium.org/1337703002/diff/390001/base/android/jni_array... base/android/jni_array.h:102: BASE_EXPORT void JavaArrayOfIntArrayToIntVector( On 2015/10/28 11:22:11, rmcilroy wrote: > Please add a comment, like JavaArrayOfByteArrayToStringVector above Done. https://codereview.chromium.org/1337703002/diff/390001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/1337703002/diff/390001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:512: ContextualSearchPolicy.getInstance(mActivity).shouldAnimateSearchProviderIcon( On 2015/10/28 18:31:14, pedrosimonetti wrote: > ContextualSearchPolicy.getInstance(mActivity) --> mPolicy Done. https://codereview.chromium.org/1337703002/diff/390001/chrome/browser/android... File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right): https://codereview.chromium.org/1337703002/diff/390001/chrome/browser/android... chrome/browser/android/compositor/layer/contextual_search_layer.cc:112: On 2015/10/28 18:31:14, pedrosimonetti wrote: > I still think it's worth adding a new boolean: > > // Put comment here explaining that zero means static icon will be used. > bool should_use_static_icon = panel_icon_resource_id != 0; > > This way it's easier to understand what "panel_icon_resource_id != 0" actually > means. Done. https://codereview.chromium.org/1337703002/diff/390001/chrome/browser/android... chrome/browser/android/compositor/layer/contextual_search_layer.cc:330: if (search_provider_icon_sprite_->layer().get() && On 2015/10/28 18:31:14, pedrosimonetti wrote: > We should only execute this when not rendering the static icon. The way it is > written now, this will be executed every time a static icon is shown. This > condition should be put inside a "if (!should_use_static_icon)" block, like > this: > > if (!should_use_static_icon) { > if (search_provider_icon_sprite_visible) { > // add layer and render animated icon > } else { > // remove layer > } > } Done. https://codereview.chromium.org/1337703002/diff/390001/chrome/browser/android... chrome/browser/android/compositor/layer/contextual_search_layer.cc:631: panel_icon_->SetIsDrawable(true); On 2015/10/28 18:31:14, pedrosimonetti wrote: > Don't we need to call SetIsDrawable() on the sprite layer as well? I'm calling SetIsDrawable on the UIResourceLayer the CrushedSpriteLayer holds in the CrushedSpriteLayer constructor (since that UIResourceLayer will always need to be drawable for the crushed sprite to work). https://codereview.chromium.org/1337703002/diff/390001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/resources/sprites/CrushedSpriteResource.java (right): https://codereview.chromium.org/1337703002/diff/390001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/resources/sprites/CrushedSpriteResource.java:172: assert name.equals("apiVersion"); On 2015/10/28 18:31:14, pedrosimonetti wrote: > Nit: Shouldn't we check that the version number is supported? Done.
base/android LGTM, thanks.
lgtm Just a few more comments, but this change is looking good to me. Thanks for all the hard work! You put a lot of effort to make the crushed sprites reusable and to make it part of the resource loader system. Now all the complexity of the crushed sprite is well abstracted and using it is as easy as using any other resource. :) https://codereview.chromium.org/1337703002/diff/490001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchIconSpriteControl.java (right): https://codereview.chromium.org/1337703002/diff/490001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchIconSpriteControl.java:43: private float mSize; nit: For consistency, rename this to mSizePx. Currently, we assume in ContextualSearch that all measurements are in dp, and we include Px at the end of members to indicate a different unit is being used. https://codereview.chromium.org/1337703002/diff/490001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchIconSpriteControl.java:46: * The panel delegate. nit: Remove "delegate". We're now using the full panel. https://codereview.chromium.org/1337703002/diff/490001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchIconSpriteControl.java:86: public float getSize() { nit: Similarly, call this getSizePx() https://codereview.chromium.org/1337703002/diff/490001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java (right): https://codereview.chromium.org/1337703002/diff/490001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java:95: 0, Put a comment here explaining what 0 means (icon sprite will be used instead of static icon) https://codereview.chromium.org/1337703002/diff/490001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/1337703002/diff/490001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:511: mSearchPanel.setShouldAnimateIconSprite(mPolicy.shouldAnimateSearchProviderIcon( Nit: Put a comment here explaining why we are setting this earlier (if the animation will happen, the icon will start hidden). I'm asking to include a comment because it's not obvious for those not familiar with the icon sprite code, and moving this call to a different place might brake the beginning of the animation. https://codereview.chromium.org/1337703002/diff/490001/chrome/browser/android... File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right): https://codereview.chromium.org/1337703002/diff/490001/chrome/browser/android... chrome/browser/android/compositor/layer/contextual_search_layer.cc:296: if (panel_icon_.get() && panel_icon_->parent()) { Since we are assuming that only one type of icon will be used, I think we can remove this part that removes the static icon from the parent. This way, when the icon sprite is used we won't have to unnecessarily execute this on every frame.
https://codereview.chromium.org/1337703002/diff/490001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchIconSpriteControl.java (right): https://codereview.chromium.org/1337703002/diff/490001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchIconSpriteControl.java:43: private float mSize; On 2015/10/29 17:01:42, pedrosimonetti wrote: > nit: For consistency, rename this to mSizePx. Currently, we assume in > ContextualSearch that all measurements are in dp, and we include Px at the end > of members to indicate a different unit is being used. Done. https://codereview.chromium.org/1337703002/diff/490001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchIconSpriteControl.java:46: * The panel delegate. On 2015/10/29 17:01:42, pedrosimonetti wrote: > nit: Remove "delegate". We're now using the full panel. Done. https://codereview.chromium.org/1337703002/diff/490001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchIconSpriteControl.java:86: public float getSize() { On 2015/10/29 17:01:42, pedrosimonetti wrote: > nit: Similarly, call this getSizePx() Done. https://codereview.chromium.org/1337703002/diff/490001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java (right): https://codereview.chromium.org/1337703002/diff/490001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ContextualSearchSceneLayer.java:95: 0, On 2015/10/29 17:01:42, pedrosimonetti wrote: > Put a comment here explaining what 0 means (icon sprite will be used instead of > static icon) Done. https://codereview.chromium.org/1337703002/diff/490001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/1337703002/diff/490001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:511: mSearchPanel.setShouldAnimateIconSprite(mPolicy.shouldAnimateSearchProviderIcon( On 2015/10/29 17:01:42, pedrosimonetti wrote: > Nit: Put a comment here explaining why we are setting this earlier (if the > animation will happen, the icon will start hidden). I'm asking to include a > comment because it's not obvious for those not familiar with the icon sprite > code, and moving this call to a different place might brake the beginning of the > animation. Done. https://codereview.chromium.org/1337703002/diff/490001/chrome/browser/android... File chrome/browser/android/compositor/layer/contextual_search_layer.cc (right): https://codereview.chromium.org/1337703002/diff/490001/chrome/browser/android... chrome/browser/android/compositor/layer/contextual_search_layer.cc:296: if (panel_icon_.get() && panel_icon_->parent()) { On 2015/10/29 17:01:42, pedrosimonetti wrote: > Since we are assuming that only one type of icon will be used, I think we can > remove this part that removes the static icon from the parent. This way, when > the icon sprite is used we won't have to unnecessarily execute this on every > frame. Done.
still lgtm https://codereview.chromium.org/1337703002/diff/530001/chrome/android/java/re... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/1337703002/diff/530001/chrome/android/java/re... chrome/android/java/res/values/dimens.xml:80: <!-- TODO(twellington): Consider moving this to the JSON sprite metadata and I'm not sure how aspirational or realistic this is, but considering how much time you've already put into this, I'd guess it's mostly aspirational. If so, I wouldn't make it a TODO in the code; instead file a bug, or put it on your personal TODO list.
The CQ bit was checked by twellington@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, pedrosimonetti@chromium.org, rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1337703002/#ps530001 (title: "Very small changes from last pedrosimonneti@ review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1337703002/530001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1337703002/530001
Message was sent while issue was closed.
Committed patchset #28 (id:530001)
Message was sent while issue was closed.
Patchset 28 (id:??) landed as https://crrev.com/af303ba5a35c1e608d6e92cc2fead270cf819274 Cr-Commit-Position: refs/heads/master@{#356977}
Message was sent while issue was closed.
dskiba@google.com changed reviewers: + dskiba@google.com
Message was sent while issue was closed.
https://codereview.chromium.org/1337703002/diff/530001/base/android/jni_array.cc File base/android/jni_array.cc (right): https://codereview.chromium.org/1337703002/diff/530001/base/android/jni_array... base/android/jni_array.cc:248: jint* ints = env->GetIntArrayElements(int_array.obj(), nullptr); Hmm, do we really need to get 'ints' here? It doesn't seem to be used.
Message was sent while issue was closed.
https://codereview.chromium.org/1337703002/diff/530001/base/android/jni_array.cc File base/android/jni_array.cc (right): https://codereview.chromium.org/1337703002/diff/530001/base/android/jni_array... base/android/jni_array.cc:248: jint* ints = env->GetIntArrayElements(int_array.obj(), nullptr); On 2015/10/30 16:46:19, Dmitry Skiba wrote: > Hmm, do we really need to get 'ints' here? It doesn't seem to be used. It's used to release the int array elements on line 250.
Message was sent while issue was closed.
https://codereview.chromium.org/1337703002/diff/530001/base/android/jni_array.cc File base/android/jni_array.cc (right): https://codereview.chromium.org/1337703002/diff/530001/base/android/jni_array... base/android/jni_array.cc:248: jint* ints = env->GetIntArrayElements(int_array.obj(), nullptr); On 2015/10/30 17:01:15, Theresa Wellington wrote: > On 2015/10/30 16:46:19, Dmitry Skiba wrote: > > Hmm, do we really need to get 'ints' here? It doesn't seem to be used. > > It's used to release the int array elements on line 250. Yes, but 'ints' is not read from. For example in the function above, JavaArrayOfByteArrayToStringVector(), 'bytes' variable (similar to our 'ints') is used to initialize 'out[i]'. In our case JavaIntArrayToIntVector() retrieves elements by itself, without using 'ints'.
Message was sent while issue was closed.
https://codereview.chromium.org/1337703002/diff/530001/base/android/jni_array.cc File base/android/jni_array.cc (right): https://codereview.chromium.org/1337703002/diff/530001/base/android/jni_array... base/android/jni_array.cc:248: jint* ints = env->GetIntArrayElements(int_array.obj(), nullptr); On 2015/10/30 17:17:32, Dmitry Skiba wrote: > On 2015/10/30 17:01:15, Theresa Wellington wrote: > > On 2015/10/30 16:46:19, Dmitry Skiba wrote: > > > Hmm, do we really need to get 'ints' here? It doesn't seem to be used. > > > > It's used to release the int array elements on line 250. > > Yes, but 'ints' is not read from. For example in the function above, > JavaArrayOfByteArrayToStringVector(), 'bytes' variable (similar to our 'ints') > is used to initialize 'out[i]'. In our case JavaIntArrayToIntVector() retrieves > elements by itself, without using 'ints'. I included this code because a new jintArray is created on line 246. As I understand it, calling ReleaseIntArrayElements on the jint* derived from that jintArray indicates that the native code no longer needs access to those elements. If that's not necessary, I can remove these lines in a new CL.
Message was sent while issue was closed.
https://codereview.chromium.org/1337703002/diff/530001/base/android/jni_array.cc File base/android/jni_array.cc (right): https://codereview.chromium.org/1337703002/diff/530001/base/android/jni_array... base/android/jni_array.cc:248: jint* ints = env->GetIntArrayElements(int_array.obj(), nullptr); On 2015/10/30 17:25:28, Theresa Wellington wrote: > On 2015/10/30 17:17:32, Dmitry Skiba wrote: > > On 2015/10/30 17:01:15, Theresa Wellington wrote: > > > On 2015/10/30 16:46:19, Dmitry Skiba wrote: > > > > Hmm, do we really need to get 'ints' here? It doesn't seem to be used. > > > > > > It's used to release the int array elements on line 250. > > > > Yes, but 'ints' is not read from. For example in the function above, > > JavaArrayOfByteArrayToStringVector(), 'bytes' variable (similar to our 'ints') > > is used to initialize 'out[i]'. In our case JavaIntArrayToIntVector() > retrieves > > elements by itself, without using 'ints'. > > I included this code because a new jintArray is created on line 246. As I > understand it, calling ReleaseIntArrayElements on the jint* derived from that > jintArray indicates that the native code no longer needs access to those > elements. If that's not necessary, I can remove these lines in a new CL. There are two ways of accessing array elements: GetIntArrayRegion() which just copies range into pre-allocated buffer and GetIntArrayElements / GetPrimitiveArrayCritical which can sometimes return internal representation of the array (pin the array), but are also allowed to return a copy of array's elements (hence isCopy argument which is nullptr in our case). Since we already using GetIntArrayRegion(), we don't really need GetIntArrayElements / ReleaseIntArrayElements. And I believe GetIntArrayRegion() is more efficient in our case as it avoids possible intermediate copy. |
