Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
581273002:
Shadows: crop corner tiles instead of hiding. (Closed)
|
Created:
6 years, 3 months ago by hshi1 Modified:
5 years, 11 months ago CC:
chromium-reviews, Ian Vollick, sievers+watch_chromium.org, jbauman+watch_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, cc-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionShadows: crop corner tiles instead of hiding.
When the shadow layer bounds is smaller than the full shadow image size,
we should crop the corner tiles by reducing aperture values instead of making
the shadow layer invisible.
This allows shadow to draw correctly when window size is very small.
BUG=415514
TEST=visually verify that window shadows are drawn correctly with very small windows.
Committed: https://crrev.com/d89c83ac82fdd3ce5d66cdda341662098fe39e98
Cr-Commit-Position: refs/heads/master@{#295728}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address nits from danakj. #
Total comments: 5
Patch Set 3 : Remove useless comments. #Patch Set 4 : Adding shadow_unittest #
Total comments: 17
Patch Set 5 : derat's comments. #Patch Set 6 : Fix CHECK failure for shared instance init twice. #
Total comments: 14
Patch Set 7 : oshima #
Total comments: 4
Patch Set 8 : nits #
Total comments: 2
Patch Set 9 : nits from sky #
Total comments: 2
Messages
Total messages: 30 (4 generated)
hshi@chromium.org changed reviewers: + derat@chromium.org, oshima@chromium.org, sky@chromium.org
PTAL, thanks!
danakj@chromium.org changed reviewers: + danakj@chromium.org
ui/compositor LGTM https://codereview.chromium.org/581273002/diff/1/ui/wm/core/shadow.cc File ui/wm/core/shadow.cc (right): https://codereview.chromium.org/581273002/diff/1/ui/wm/core/shadow.cc#newcode175 ui/wm/core/shadow.cc:175: // Update nine-patch layer with new bitmap and aperture. this isn't setting the aperture anymore is it? https://codereview.chromium.org/581273002/diff/1/ui/wm/core/shadow.cc#newcode179 ui/wm/core/shadow.cc:179: image_size_ = gfx::Size(image.Width(), image.Height()); image_size_ = image.Size(); ?
https://codereview.chromium.org/581273002/diff/1/ui/wm/core/shadow.cc File ui/wm/core/shadow.cc (right): https://codereview.chromium.org/581273002/diff/1/ui/wm/core/shadow.cc#newcode175 ui/wm/core/shadow.cc:175: // Update nine-patch layer with new bitmap and aperture. On 2014/09/18 21:28:18, danakj wrote: > this isn't setting the aperture anymore is it? Done. https://codereview.chromium.org/581273002/diff/1/ui/wm/core/shadow.cc#newcode179 ui/wm/core/shadow.cc:179: image_size_ = gfx::Size(image.Width(), image.Height()); On 2014/09/18 21:28:18, danakj wrote: > image_size_ = image.Size(); ? Done.
LGTM https://codereview.chromium.org/581273002/diff/20001/ui/wm/core/shadow.cc File ui/wm/core/shadow.cc (right): https://codereview.chromium.org/581273002/diff/20001/ui/wm/core/shadow.cc#new... ui/wm/core/shadow.cc:175: // Update nine-patch layer with new bitmap. Thanks, this comment now just says the function name in a longer way so can remove? https://codereview.chromium.org/581273002/diff/20001/ui/wm/core/shadow.cc#new... ui/wm/core/shadow.cc:178: // Update shadow image size. Similar comment about this comment, wdyt?
https://codereview.chromium.org/581273002/diff/20001/ui/wm/core/shadow.cc File ui/wm/core/shadow.cc (right): https://codereview.chromium.org/581273002/diff/20001/ui/wm/core/shadow.cc#new... ui/wm/core/shadow.cc:175: // Update nine-patch layer with new bitmap. On 2014/09/18 21:35:29, danakj wrote: > Thanks, this comment now just says the function name in a longer way so can > remove? Done. https://codereview.chromium.org/581273002/diff/20001/ui/wm/core/shadow.cc#new... ui/wm/core/shadow.cc:178: // Update shadow image size. On 2014/09/18 21:35:29, danakj wrote: > Similar comment about this comment, wdyt? Done. https://codereview.chromium.org/581273002/diff/20001/ui/wm/core/shadow.cc#new... ui/wm/core/shadow.cc:181: // Update interior inset for style. Lemme remove this line too (the function name is already self-documenting).
i'll defer to others, as i don't have any experience with the compositor nine-patch code that this interacts with. would it be feasible to start a shadow_unittest.cc file?
On 2014/09/18 21:47:17, Daniel Erat wrote: > would it be feasible to start a shadow_unittest.cc file? I thought about it but one hurdle for a shadow_unittest.cc is to abstract ResourceBundle (to return a fake image resource with hard-coded size, for example). It would require some refactoring. Also I can't seem to find any existing unit tests written for classes that depend on ResourceBundle.
On 2014/09/18 21:53:50, hshi1 wrote: > On 2014/09/18 21:47:17, Daniel Erat wrote: > > would it be feasible to start a shadow_unittest.cc file? > > I thought about it but one hurdle for a shadow_unittest.cc is to abstract > ResourceBundle (to return a fake image resource with hard-coded size, for > example). It would require some refactoring. Also I can't seem to find any > existing unit tests written for classes that depend on ResourceBundle. You should be able to create your own using ResourceBundle::InitSharedInstance + Delegate::GetImageNamed impl. Make sure you delete it (CleanupSharedInstance) when existting the test.
On 2014/09/18 22:11:33, oshima wrote: > On 2014/09/18 21:53:50, hshi1 wrote: > > On 2014/09/18 21:47:17, Daniel Erat wrote: > > > would it be feasible to start a shadow_unittest.cc file? > > > > I thought about it but one hurdle for a shadow_unittest.cc is to abstract > > ResourceBundle (to return a fake image resource with hard-coded size, for > > example). It would require some refactoring. Also I can't seem to find any > > existing unit tests written for classes that depend on ResourceBundle. > > You should be able to create your own using ResourceBundle::InitSharedInstance + > Delegate::GetImageNamed impl. > Make sure you delete it (CleanupSharedInstance) when existting the test. re:#10 oshima: would this interfere with the existing InitSharedInstance and CleanupSharedInstance calls in AuraShellTestSuite?
On 2014/09/18 22:20:13, hshi1 wrote:
> On 2014/09/18 22:11:33, oshima wrote:
> > On 2014/09/18 21:53:50, hshi1 wrote:
> > > On 2014/09/18 21:47:17, Daniel Erat wrote:
> > > > would it be feasible to start a shadow_unittest.cc file?
> > >
> > > I thought about it but one hurdle for a shadow_unittest.cc is to abstract
> > > ResourceBundle (to return a fake image resource with hard-coded size, for
> > > example). It would require some refactoring. Also I can't seem to find any
> > > existing unit tests written for classes that depend on ResourceBundle.
> >
> > You should be able to create your own using
ResourceBundle::InitSharedInstance
> +
> > Delegate::GetImageNamed impl.
> > Make sure you delete it (CleanupSharedInstance) when existting the test.
>
> re:#10 oshima: would this interfere with the existing InitSharedInstance and
> CleanupSharedInstance calls in AuraShellTestSuite?
You just need to reset the resource bundle at the end of your test like below
so that the rest of the test will use the same resource bundle.
ui::ResourceBundle::CleanupShardInstance();
ui::ResourceBundle::InitSharedInstanceWithLocale(
"en-US", NULL, ui::ResourceBundle::LOAD_COMMON_RESOURCES);
oshima@, derat@: I added shadow_unittest, PTAL, thanks!
thanks! https://codereview.chromium.org/581273002/diff/60001/ui/wm/core/shadow_unitte... File ui/wm/core/shadow_unittest.cc (right): https://codereview.chromium.org/581273002/diff/60001/ui/wm/core/shadow_unitte... ui/wm/core/shadow_unittest.cc:26: bitmap_small.allocPixels(SkImageInfo::MakeN32Premul(129, 129)); nit: mind moving these numbers into constants? https://codereview.chromium.org/581273002/diff/60001/ui/wm/core/shadow_unitte... ui/wm/core/shadow_unittest.cc:90: new MockResourceBundleDelegate()); mind making a ShadowTest class and moving this code to its c'tor or SetUp() method and the CleanupSharedInstance() call to its d'tor or TearDown() method so it doesn't need to be duplicated across tests? https://codereview.chromium.org/581273002/diff/60001/ui/wm/core/shadow_unitte... ui/wm/core/shadow_unittest.cc:92: "en-US", delegate.get(), ui::ResourceBundle::LOAD_COMMON_RESOURCES); nit: indent four spaces; same below https://codereview.chromium.org/581273002/diff/60001/ui/wm/core/shadow_unitte... ui/wm/core/shadow_unittest.cc:97: EXPECT_EQ(delegate->last_resource_id(), IDR_WINDOW_BUBBLE_SHADOW_SMALL); i was going to suggest that it'd be better to check that the correct image was actually applied, but it seems like it'd be a lot of work (e.g. you'd need to eventually get the bitmap out of cc/layers/ui_resource_layer.h, i think), so never mind. :-( https://codereview.chromium.org/581273002/diff/60001/ui/wm/core/shadow_unitte... ui/wm/core/shadow_unittest.cc:115: // verify that layer bounds are inset from content bounds nit: capitalize "Verify" and add a trailing period https://codereview.chromium.org/581273002/diff/60001/ui/wm/core/shadow_unitte... ui/wm/core/shadow_unittest.cc:120: EXPECT_EQ(shadow->layer()->bounds(), gfx::Rect(36, 36, 428, 428)); could you make these numbers be a function of the numbers from MockResourceBundleDelegate() so it's clearer how they relate to them? https://codereview.chromium.org/581273002/diff/60001/ui/wm/core/shadow_unitte... ui/wm/core/shadow_unittest.cc:124: EXPECT_EQ(shadow->layer()->bounds(), gfx::Rect(96, 96, 308, 308)); does this exercise the change that you're making, or do you need a smaller size for that?
https://codereview.chromium.org/581273002/diff/60001/ui/wm/core/shadow_unitte... File ui/wm/core/shadow_unittest.cc (right): https://codereview.chromium.org/581273002/diff/60001/ui/wm/core/shadow_unitte... ui/wm/core/shadow_unittest.cc:26: bitmap_small.allocPixels(SkImageInfo::MakeN32Premul(129, 129)); On 2014/09/19 00:28:58, Daniel Erat wrote: > nit: mind moving these numbers into constants? Done. https://codereview.chromium.org/581273002/diff/60001/ui/wm/core/shadow_unitte... ui/wm/core/shadow_unittest.cc:90: new MockResourceBundleDelegate()); On 2014/09/19 00:28:58, Daniel Erat wrote: > mind making a ShadowTest class and moving this code to its c'tor or SetUp() > method and the CleanupSharedInstance() call to its d'tor or TearDown() method so > it doesn't need to be duplicated across tests? Done. https://codereview.chromium.org/581273002/diff/60001/ui/wm/core/shadow_unitte... ui/wm/core/shadow_unittest.cc:92: "en-US", delegate.get(), ui::ResourceBundle::LOAD_COMMON_RESOURCES); On 2014/09/19 00:28:58, Daniel Erat wrote: > nit: indent four spaces; same below Done. https://codereview.chromium.org/581273002/diff/60001/ui/wm/core/shadow_unitte... ui/wm/core/shadow_unittest.cc:97: EXPECT_EQ(delegate->last_resource_id(), IDR_WINDOW_BUBBLE_SHADOW_SMALL); On 2014/09/19 00:28:58, Daniel Erat wrote: > i was going to suggest that it'd be better to check that the correct image was > actually applied, but it seems like it'd be a lot of work (e.g. you'd need to > eventually get the bitmap out of cc/layers/ui_resource_layer.h, i think), so > never mind. :-( Acknowledged. https://codereview.chromium.org/581273002/diff/60001/ui/wm/core/shadow_unitte... ui/wm/core/shadow_unittest.cc:115: // verify that layer bounds are inset from content bounds On 2014/09/19 00:28:58, Daniel Erat wrote: > nit: capitalize "Verify" and add a trailing period Done. https://codereview.chromium.org/581273002/diff/60001/ui/wm/core/shadow_unitte... ui/wm/core/shadow_unittest.cc:120: EXPECT_EQ(shadow->layer()->bounds(), gfx::Rect(36, 36, 428, 428)); On 2014/09/19 00:28:58, Daniel Erat wrote: > could you make these numbers be a function of the numbers from > MockResourceBundleDelegate() so it's clearer how they relate to them? actually the layer bounds are only related to the content bounds by inset values (which are constants as defined in shadow.cc). they are not in any way related to the bitmap size. https://codereview.chromium.org/581273002/diff/60001/ui/wm/core/shadow_unitte... ui/wm/core/shadow_unittest.cc:124: EXPECT_EQ(shadow->layer()->bounds(), gfx::Rect(96, 96, 308, 308)); On 2014/09/19 00:28:58, Daniel Erat wrote: > does this exercise the change that you're making, or do you need a smaller size > for that? No unfortunately it doesn't exercise the current change. A smaller size won't help either. To verify that corner tiles are cropped, I need to access NinePatchLayer's private members (to compare the aperture and border values).
lgtm for tests, but please wait for oshima and sky to look at the shadow.cc changes. https://codereview.chromium.org/581273002/diff/60001/ui/wm/core/shadow_unitte... File ui/wm/core/shadow_unittest.cc (right): https://codereview.chromium.org/581273002/diff/60001/ui/wm/core/shadow_unitte... ui/wm/core/shadow_unittest.cc:124: EXPECT_EQ(shadow->layer()->bounds(), gfx::Rect(96, 96, 308, 308)); On 2014/09/19 00:41:13, hshi1 wrote: > On 2014/09/19 00:28:58, Daniel Erat wrote: > > does this exercise the change that you're making, or do you need a smaller > size > > for that? > > No unfortunately it doesn't exercise the current change. A smaller size won't > help either. To verify that corner tiles are cropped, I need to access > NinePatchLayer's private members (to compare the aperture and border values). got it. :-/ thanks for the tests, in any case.
https://codereview.chromium.org/581273002/diff/60001/ui/wm/core/shadow_unitte... File ui/wm/core/shadow_unittest.cc (right): https://codereview.chromium.org/581273002/diff/60001/ui/wm/core/shadow_unitte... ui/wm/core/shadow_unittest.cc:124: EXPECT_EQ(shadow->layer()->bounds(), gfx::Rect(96, 96, 308, 308)); On 2014/09/19 00:41:13, hshi1 wrote: > On 2014/09/19 00:28:58, Daniel Erat wrote: > > does this exercise the change that you're making, or do you need a smaller > size > > for that? > > No unfortunately it doesn't exercise the current change. A smaller size won't > help either. To verify that corner tiles are cropped, I need to access > NinePatchLayer's private members (to compare the aperture and border values). You can add an accessor for those if you want.
https://codereview.chromium.org/581273002/diff/60001/ui/wm/core/shadow_unitte... File ui/wm/core/shadow_unittest.cc (right): https://codereview.chromium.org/581273002/diff/60001/ui/wm/core/shadow_unitte... ui/wm/core/shadow_unittest.cc:124: EXPECT_EQ(shadow->layer()->bounds(), gfx::Rect(96, 96, 308, 308)); On 2014/09/19 00:44:38, danakj wrote: > On 2014/09/19 00:41:13, hshi1 wrote: > > On 2014/09/19 00:28:58, Daniel Erat wrote: > > > does this exercise the change that you're making, or do you need a smaller > > size > > > for that? > > > > No unfortunately it doesn't exercise the current change. A smaller size won't > > help either. To verify that corner tiles are cropped, I need to access > > NinePatchLayer's private members (to compare the aperture and border values). > > You can add an accessor for those if you want. I think it's worth it. https://codereview.chromium.org/581273002/diff/100001/ui/wm/core/shadow_unitt... File ui/wm/core/shadow_unittest.cc (right): https://codereview.chromium.org/581273002/diff/100001/ui/wm/core/shadow_unitt... ui/wm/core/shadow_unittest.cc:20: static const int kLargeBitmapSize = 269; nit: put them in anonymous namespace and remove static https://codereview.chromium.org/581273002/diff/100001/ui/wm/core/shadow_unitt... ui/wm/core/shadow_unittest.cc:36: // ResourceBundle::Delegate overrides. just // ResourceBundle::Delegate: (overrdies. is no longer necessary) https://codereview.chromium.org/581273002/diff/100001/ui/wm/core/shadow_unitt... ui/wm/core/shadow_unittest.cc:82: int last_resource_id() const { return last_resource_id_; } nit: indent https://codereview.chromium.org/581273002/diff/100001/ui/wm/core/shadow_unitt... ui/wm/core/shadow_unittest.cc:88: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/581273002/diff/100001/ui/wm/core/shadow_unitt... ui/wm/core/shadow_unittest.cc:96: nit: // aura::test:AuraTestBase: https://codereview.chromium.org/581273002/diff/100001/ui/wm/core/shadow_unitt... ui/wm/core/shadow_unittest.cc:106: ui::ResourceBundle::CleanupSharedInstance(); You need base::FilePath ui_test_pak_path; ASSERT_TRUE(PathService::Get(ui::UI_TEST_PAK, &ui_test_pak_path)); ui::ResourceBundle::InitSharedInstanceWithPakPath(ui_test_pak_path); as in ui/wm/test/run_all_unittest.cc. It's probably passing now but, it's just because next tests are not using resources. https://codereview.chromium.org/581273002/diff/100001/ui/wm/core/shadow_unitt... ui/wm/core/shadow_unittest.cc:111: }; private: DISALLOW_COPY_AND_ASSIGN
https://codereview.chromium.org/581273002/diff/100001/ui/wm/core/shadow_unitt... File ui/wm/core/shadow_unittest.cc (right): https://codereview.chromium.org/581273002/diff/100001/ui/wm/core/shadow_unitt... ui/wm/core/shadow_unittest.cc:20: static const int kLargeBitmapSize = 269; On 2014/09/19 14:24:02, oshima wrote: > nit: put them in anonymous namespace and remove static Done. https://codereview.chromium.org/581273002/diff/100001/ui/wm/core/shadow_unitt... ui/wm/core/shadow_unittest.cc:36: // ResourceBundle::Delegate overrides. On 2014/09/19 14:24:02, oshima wrote: > just // ResourceBundle::Delegate: > > (overrdies. is no longer necessary) Done. https://codereview.chromium.org/581273002/diff/100001/ui/wm/core/shadow_unitt... ui/wm/core/shadow_unittest.cc:82: int last_resource_id() const { return last_resource_id_; } On 2014/09/19 14:24:02, oshima wrote: > nit: indent Done. https://codereview.chromium.org/581273002/diff/100001/ui/wm/core/shadow_unitt... ui/wm/core/shadow_unittest.cc:88: }; On 2014/09/19 14:24:02, oshima wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/581273002/diff/100001/ui/wm/core/shadow_unitt... ui/wm/core/shadow_unittest.cc:96: On 2014/09/19 14:24:01, oshima wrote: > nit: // aura::test:AuraTestBase: Done. https://codereview.chromium.org/581273002/diff/100001/ui/wm/core/shadow_unitt... ui/wm/core/shadow_unittest.cc:106: ui::ResourceBundle::CleanupSharedInstance(); On 2014/09/19 14:24:01, oshima wrote: > You need > > base::FilePath ui_test_pak_path; > ASSERT_TRUE(PathService::Get(ui::UI_TEST_PAK, &ui_test_pak_path)); > ui::ResourceBundle::InitSharedInstanceWithPakPath(ui_test_pak_path); > > as in ui/wm/test/run_all_unittest.cc. > > It's probably passing now but, it's just because next tests are not using > resources. Ok but it requires me to add "+ui/base/ui_base_paths.h" to DEPS https://codereview.chromium.org/581273002/diff/100001/ui/wm/core/shadow_unitt... ui/wm/core/shadow_unittest.cc:111: }; On 2014/09/19 14:24:01, oshima wrote: > private: DISALLOW_COPY_AND_ASSIGN Done.
lgtm I'll leave accessor changes to you and danakj. https://codereview.chromium.org/581273002/diff/120001/ui/wm/core/shadow_unitt... File ui/wm/core/shadow_unittest.cc (right): https://codereview.chromium.org/581273002/diff/120001/ui/wm/core/shadow_unitt... ui/wm/core/shadow_unittest.cc:97: nit: remove extra new line https://codereview.chromium.org/581273002/diff/120001/ui/wm/core/shadow_unitt... ui/wm/core/shadow_unittest.cc:104: MockResourceBundleDelegate* delegate() const { return delegate_.get(); } nit: remove const (when returning a non const internal state)
https://codereview.chromium.org/581273002/diff/120001/ui/wm/core/shadow_unitt... File ui/wm/core/shadow_unittest.cc (right): https://codereview.chromium.org/581273002/diff/120001/ui/wm/core/shadow_unitt... ui/wm/core/shadow_unittest.cc:97: On 2014/09/19 15:09:59, oshima wrote: > nit: remove extra new line Done. https://codereview.chromium.org/581273002/diff/120001/ui/wm/core/shadow_unitt... ui/wm/core/shadow_unittest.cc:104: MockResourceBundleDelegate* delegate() const { return delegate_.get(); } On 2014/09/19 15:09:59, oshima wrote: > nit: remove const (when returning a non const internal state) Done.
ping sky@ (still need OWNER approval for ui/wm/core) thanks!
LGTM https://codereview.chromium.org/581273002/diff/140001/ui/wm/core/shadow_unitt... File ui/wm/core/shadow_unittest.cc (right): https://codereview.chromium.org/581273002/diff/140001/ui/wm/core/shadow_unitt... ui/wm/core/shadow_unittest.cc:128: scoped_ptr<Shadow> shadow(new Shadow()); Is there a reason you don't declare these on the stack?
https://codereview.chromium.org/581273002/diff/140001/ui/wm/core/shadow_unitt... File ui/wm/core/shadow_unittest.cc (right): https://codereview.chromium.org/581273002/diff/140001/ui/wm/core/shadow_unitt... ui/wm/core/shadow_unittest.cc:128: scoped_ptr<Shadow> shadow(new Shadow()); On 2014/09/19 16:11:31, sky wrote: > Is there a reason you don't declare these on the stack? Done.
The CQ bit was checked by hshi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/581273002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as 7eb2c0b04b3016868bb7c8f81048ba6cdf112592
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/d89c83ac82fdd3ce5d66cdda341662098fe39e98 Cr-Commit-Position: refs/heads/master@{#295728}
Message was sent while issue was closed.
tfarina@chromium.org changed reviewers: + tfarina@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/581273002/diff/160001/ui/wm/core/shadow_unitt... File ui/wm/core/shadow_unittest.cc (right): https://codereview.chromium.org/581273002/diff/160001/ui/wm/core/shadow_unitt... ui/wm/core/shadow_unittest.cc:111: ui::ResourceBundle::InitSharedInstanceWithLocale( Why do you need to initialize ResourceBundle here? If we are initializing it for all tests in https://chromium.googlesource.com/chromium/src/+/master/ui/wm/test/run_all_un... https://codereview.chromium.org/581273002/diff/160001/ui/wm/core/shadow_unitt... ui/wm/core/shadow_unittest.cc:118: ui::ResourceBundle::InitSharedInstanceWithPakPath(ui_test_pak_path); Why are you initializing (again) ResourceBundle in the teardown pass of the test setup? Sorry, without knowing the context that does not look right. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
