Update size of Google Logo on Android NTP.

Update size of Google Logo on Android NTP.

The logo is now 64dp high on <600dp screens, and 96dp on bigger screens.

Because the default logo is never upscaled, using smaller assets while keeping the size of the view the same works, and ensures Doodles will not get downscaled.

Screenshots on tablet/phone: https://imgur.com/a/ntrQj

BUG=612156
Committed: https://crrev.com/fb7d5e9e3633a8df4fca421c1d0ddefa6d1c2a4e
Cr-Commit-Position: refs/heads/master@{#400769}

[Bernhard Bauer, 2016-06-15 17:30:02]

Description was changed from

==========
Update size of Google Logo on Android NTP.

Also, replace the logo bitmap with a vector drawable.

BUG=612156
==========

to

==========
Update size of Google Logo on Android NTP.

Also, replace the logo bitmap with a vector drawable.

Screenshots on tablet/phone: https://imgur.com/a/K1CJ0

BUG=612156
==========

[Bernhard Bauer, 2016-06-15 17:30:22]

bauerb@chromium.org changed reviewers:
+ dgn@chromium.org, tedchoc@chromium.org

[Bernhard Bauer, 2016-06-15 17:30:23]

Please review. Thanks!

[Ted C, 2016-06-15 20:05:57]

On 2016/06/15 17:30:23, Bernhard Bauer wrote:
> Please review. Thanks!

Vector drawables are not supported pre-L.

ianwen@ is working on adding support, but it requires the latest
support library.  Rolling the support library is blocked on adding
toolchain support for the AAR format they've switched to

I'm not terribly familiar with LogoView either, might want to rope
Ian into this.  Does it handle size changes automatically?  Can
animated doodles grow out of their bounds?  From the bug, it says
we shouldn't be changing animated doodles, but to me it looks like
we are shrinking those as well and scaling them down more.

[Bernhard Bauer, 2016-06-15 20:27:29]

On Wed, Jun 15, 2016, 21:05 <tedchoc@chromium.org> wrote:

> On 2016/06/15 17:30:23, Bernhard Bauer wrote:
> > Please review. Thanks!
>
> Vector drawables are not supported pre-L.
>
> ianwen@ is working on adding support, but it requires the latest
> support library. Rolling the support library is blocked on adding
> toolchain support for the AAR format they've switched to
>

I don't know about that, but we do use the support library, and I've tested
this successfully on JB-MR1.

I'm not terribly familiar with LogoView either, might want to rope
> Ian into this. Does it handle size changes automatically? Can
> animated doodles grow out of their bounds? From the bug, it says
> we shouldn't be changing animated doodles, but to me it looks like
> we are shrinking those as well and scaling them down more.
>

Hm, I think that will depend on the size of the doodle. I'll check tomorrow
whether we can have doodles bigger than this size of logo.


> https://codereview.chromium.org/2068883003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ian Wen, 2016-06-15 20:39:50]

ianwen@chromium.org changed reviewers:
+ ianwen@chromium.org

[Ian Wen, 2016-06-15 20:39:51]

If you do a quick test for vector drawable compat, it might work. Yet no
guarantee, unless we roll our support library to the newest version.

The support library team is constantly patching this feature in appcompat.
According to their release blog, 23.4.0 is the version that they are comfortable
with the performance of the vector drawable compat[1]. At the same time, Chrome
has not rolled support library for two months (we are stuck on 23.3.2), due to
the lack of AAR format support[2]. We expect to start rolling android_tools
folder in late M53 development cycle.

[1]. https://plus.sandbox.google.com/+AndroidDevelopers/posts/B7QhFkWZ6YX
[2]. http://crbug.com/611171

[Bernhard Bauer, 2016-06-15 21:22:53]

On 2016/06/15 20:39:51, Ian Wen wrote:
> If you do a quick test for vector drawable compat, it might work. Yet no
> guarantee, unless we roll our support library to the newest version.

What does that mean? Under which circumstances would it not work? Or are we just
guessing?

> The support library team is constantly patching this feature in appcompat.
> According to their release blog, 23.4.0 is the version that they are
comfortable
> with the performance of the vector drawable compat[1]. At the same time,
Chrome
> has not rolled support library for two months (we are stuck on 23.3.2), due to
> the lack of AAR format support[2]. We expect to start rolling android_tools
> folder in late M53 development cycle.
> 
> [1]. https://plus.sandbox.google.com/+AndroidDevelopers/posts/B7QhFkWZ6YX
> [2]. http://crbug.com/611171

[Ian Wen, 2016-06-15 21:36:33]

On 2016/06/15 21:22:53, Bernhard Bauer wrote:
> On 2016/06/15 20:39:51, Ian Wen wrote:
> > If you do a quick test for vector drawable compat, it might work. Yet no
> > guarantee, unless we roll our support library to the newest version.
> 
> What does that mean? Under which circumstances would it not work? Or are we
just
> guessing?
> 
> > The support library team is constantly patching this feature in appcompat.
> > According to their release blog, 23.4.0 is the version that they are
> comfortable
> > with the performance of the vector drawable compat[1]. At the same time,
> Chrome
> > has not rolled support library for two months (we are stuck on 23.3.2), due
to
> > the lack of AAR format support[2]. We expect to start rolling android_tools
> > folder in late M53 development cycle.
> > 
> > [1]. https://plus.sandbox.google.com/+AndroidDevelopers/posts/B7QhFkWZ6YX
> > [2]. http://crbug.com/611171

In 23.3, which is the version we currently use, setting
app:srcCompat="@drawable/ic_vector" might not work as they promoted originally,
according to [1]. Between 23.3 and 23.4 there has been active development to
improve the feature. For things that might not work, [2][3] are examples found
by developers.

[1]. https://plus.sandbox.google.com/+AndroidDevelopers/posts/iTDmFiGrVne
[2]. https://code.google.com/p/android/issues/detail?id=205236
[3]. https://code.google.com/p/android/issues/detail?id=204708

[Bernhard Bauer, 2016-06-16 14:26:47]

On 2016/06/15 21:36:33, Ian Wen wrote:
> On 2016/06/15 21:22:53, Bernhard Bauer wrote:
> > On 2016/06/15 20:39:51, Ian Wen wrote:
> > > If you do a quick test for vector drawable compat, it might work. Yet no
> > > guarantee, unless we roll our support library to the newest version.
> > 
> > What does that mean? Under which circumstances would it not work? Or are we
> just
> > guessing?
> > 
> > > The support library team is constantly patching this feature in appcompat.
> > > According to their release blog, 23.4.0 is the version that they are
> > comfortable
> > > with the performance of the vector drawable compat[1]. At the same time,
> > Chrome
> > > has not rolled support library for two months (we are stuck on 23.3.2),
due
> to
> > > the lack of AAR format support[2]. We expect to start rolling
android_tools
> > > folder in late M53 development cycle.
> > > 
> > > [1]. https://plus.sandbox.google.com/+AndroidDevelopers/posts/B7QhFkWZ6YX
> > > [2]. http://crbug.com/611171
> 
> In 23.3, which is the version we currently use, setting
> app:srcCompat="@drawable/ic_vector" might not work as they promoted
originally,
> according to [1]. Between 23.3 and 23.4 there has been active development to
> improve the feature. For things that might not work, [2][3] are examples found
> by developers.

Hm, according to the G+ post, app:srcCompat and manually inflating at runtime
still works, and I'm doing the latter here.

Or is the issue that we consider vector support overall to be too unstable for
our purposes, and therefore are banning it everywhere (even in cases where it
would work), similar to how we ban C++11 features that don't work everywhere? Is
there a place where decisions like this are documented?

> [1]. https://plus.sandbox.google.com/+AndroidDevelopers/posts/iTDmFiGrVne
> [2]. https://code.google.com/p/android/issues/detail?id=205236
> [3]. https://code.google.com/p/android/issues/detail?id=204708

[Ted C, 2016-06-16 17:08:32]

On 2016/06/16 14:26:47, Bernhard Bauer wrote:
> On 2016/06/15 21:36:33, Ian Wen wrote:
> > On 2016/06/15 21:22:53, Bernhard Bauer wrote:
> > > On 2016/06/15 20:39:51, Ian Wen wrote:
> > > > If you do a quick test for vector drawable compat, it might work. Yet no
> > > > guarantee, unless we roll our support library to the newest version.
> > > 
> > > What does that mean? Under which circumstances would it not work? Or are
we
> > just
> > > guessing?
> > > 
> > > > The support library team is constantly patching this feature in
appcompat.
> > > > According to their release blog, 23.4.0 is the version that they are
> > > comfortable
> > > > with the performance of the vector drawable compat[1]. At the same time,
> > > Chrome
> > > > has not rolled support library for two months (we are stuck on 23.3.2),
> due
> > to
> > > > the lack of AAR format support[2]. We expect to start rolling
> android_tools
> > > > folder in late M53 development cycle.
> > > > 
> > > > [1].
https://plus.sandbox.google.com/+AndroidDevelopers/posts/B7QhFkWZ6YX
> > > > [2]. http://crbug.com/611171
> > 
> > In 23.3, which is the version we currently use, setting
> > app:srcCompat="@drawable/ic_vector" might not work as they promoted
> originally,
> > according to [1]. Between 23.3 and 23.4 there has been active development to
> > improve the feature. For things that might not work, [2][3] are examples
found
> > by developers.
> 
> Hm, according to the G+ post, app:srcCompat and manually inflating at runtime
> still works, and I'm doing the latter here.
> 
> Or is the issue that we consider vector support overall to be too unstable for
> our purposes, and therefore are banning it everywhere (even in cases where it
> would work), similar to how we ban C++11 features that don't work everywhere?
Is
> there a place where decisions like this are documented?

I think the main concern was around performance.  We can measure this
with science!

Ok...maybe not science, but simple logging!  Ok...no more exclamations.

!!!!!!!

Our initial thinking was that we'd replace larger single use icons (think
promo dialogs that get only shown once) because the developers did seem
to imply a potential performance regression in using vectors.

For something as visible and used as the NTP, I do have a bit of trepidation
saying that we should just use them without a bit more understanding of
the impact.

Can we measure the difference in inflation time for vector vs bitmap (on
older platforms, I'd say KitKat is representative enough)?  Then measure
the difference in draw time?

I think if we have both of those then we'd be able to make a much more
informed and less speculative decision.  I just want to make sure we're
not paying some hidden cost on "hopefully" one of the most visible
screens in Chrome.

> 
> > [1]. https://plus.sandbox.google.com/+AndroidDevelopers/posts/iTDmFiGrVne
> > [2]. https://code.google.com/p/android/issues/detail?id=205236
> > [3]. https://code.google.com/p/android/issues/detail?id=204708

[Bernhard Bauer, 2016-06-16 20:11:09]

On 2016/06/16 17:08:32, Ted C wrote:
> On 2016/06/16 14:26:47, Bernhard Bauer wrote:
> > On 2016/06/15 21:36:33, Ian Wen wrote:
> > > On 2016/06/15 21:22:53, Bernhard Bauer wrote:
> > > > On 2016/06/15 20:39:51, Ian Wen wrote:
> > > > > If you do a quick test for vector drawable compat, it might work. Yet
no
> > > > > guarantee, unless we roll our support library to the newest version.
> > > > 
> > > > What does that mean? Under which circumstances would it not work? Or are
> we
> > > just
> > > > guessing?
> > > > 
> > > > > The support library team is constantly patching this feature in
> appcompat.
> > > > > According to their release blog, 23.4.0 is the version that they are
> > > > comfortable
> > > > > with the performance of the vector drawable compat[1]. At the same
time,
> > > > Chrome
> > > > > has not rolled support library for two months (we are stuck on
23.3.2),
> > due
> > > to
> > > > > the lack of AAR format support[2]. We expect to start rolling
> > android_tools
> > > > > folder in late M53 development cycle.
> > > > > 
> > > > > [1].
> https://plus.sandbox.google.com/+AndroidDevelopers/posts/B7QhFkWZ6YX
> > > > > [2]. http://crbug.com/611171
> > > 
> > > In 23.3, which is the version we currently use, setting
> > > app:srcCompat="@drawable/ic_vector" might not work as they promoted
> > originally,
> > > according to [1]. Between 23.3 and 23.4 there has been active development
to
> > > improve the feature. For things that might not work, [2][3] are examples
> found
> > > by developers.
> > 
> > Hm, according to the G+ post, app:srcCompat and manually inflating at
runtime
> > still works, and I'm doing the latter here.
> > 
> > Or is the issue that we consider vector support overall to be too unstable
for
> > our purposes, and therefore are banning it everywhere (even in cases where
it
> > would work), similar to how we ban C++11 features that don't work
everywhere?
> Is
> > there a place where decisions like this are documented?
> 
> I think the main concern was around performance.  We can measure this
> with science!
> 
> Ok...maybe not science, but simple logging!  Ok...no more exclamations.
> 
> !!!!!!!
> 
> Our initial thinking was that we'd replace larger single use icons (think
> promo dialogs that get only shown once) because the developers did seem
> to imply a potential performance regression in using vectors.
> 
> For something as visible and used as the NTP, I do have a bit of trepidation
> saying that we should just use them without a bit more understanding of
> the impact.
> 
> Can we measure the difference in inflation time for vector vs bitmap (on
> older platforms, I'd say KitKat is representative enough)?  Then measure
> the difference in draw time?
> 
> I think if we have both of those then we'd be able to make a much more
> informed and less speculative decision.  I just want to make sure we're
> not paying some hidden cost on "hopefully" one of the most visible
> screens in Chrome.

Ok, that sounds good. Hooray for science!

> > > [1]. https://plus.sandbox.google.com/+AndroidDevelopers/posts/iTDmFiGrVne
> > > [2]. https://code.google.com/p/android/issues/detail?id=205236
> > > [3]. https://code.google.com/p/android/issues/detail?id=204708

[Bernhard Bauer, 2016-06-17 12:40:48]

OK, I've reverted back to bitmap drawables for this CL while I investigate the
performance impact of vector drawables.

In order not to downscale Doodles, the CL now keeps the height dimen the same
while changing the default logo assets to be smaller. The default logo will not
get upscaled, so it will effectively be smaller, while Doodles will stay the
same size as before.

[Bernhard Bauer, 2016-06-17 12:42:45]

Description was changed from

==========
Update size of Google Logo on Android NTP.

Also, replace the logo bitmap with a vector drawable.

Screenshots on tablet/phone: https://imgur.com/a/K1CJ0

BUG=612156
==========

to

==========
Update size of Google Logo on Android NTP.

Also, replace the logo bitmap with a vector drawable.

Screenshots on tablet/phone: https://imgur.com/a/ntrQj

BUG=612156
==========

[Bernhard Bauer, 2016-06-20 17:35:48]

On 2016/06/17 12:40:48, Bernhard Bauer wrote:
> OK, I've reverted back to bitmap drawables for this CL while I investigate the
> performance impact of vector drawables.
> 
> In order not to downscale Doodles, the CL now keeps the height dimen the same
> while changing the default logo assets to be smaller. The default logo will
not
> get upscaled, so it will effectively be smaller, while Doodles will stay the
> same size as before.

Ping?

[Ted C, 2016-06-20 18:10:04]

lgtm

Sorry about that!  Did the tablet logos get slightly bigger with this change? 
Also, update the commit message?

[Bernhard Bauer, 2016-06-20 19:39:30]

Description was changed from

==========
Update size of Google Logo on Android NTP.

Also, replace the logo bitmap with a vector drawable.

Screenshots on tablet/phone: https://imgur.com/a/ntrQj

BUG=612156
==========

to

==========
Update size of Google Logo on Android NTP.

Because the default logo is never upscaled, using smaller assets while keeping
the size of the view the same works, and ensures Doodles will not get
downscaled.

Screenshots on tablet/phone: https://imgur.com/a/ntrQj

BUG=612156
==========

[Bernhard Bauer, 2016-06-20 19:48:13]

Description was changed from

==========
Update size of Google Logo on Android NTP.

Because the default logo is never upscaled, using smaller assets while keeping
the size of the view the same works, and ensures Doodles will not get
downscaled.

Screenshots on tablet/phone: https://imgur.com/a/ntrQj

BUG=612156
==========

to

==========
Update size of Google Logo on Android NTP.

The logo is now 64dp high on <600dp screens, and 96dp on bigger screens.

Because the default logo is never upscaled, using smaller assets while keeping
the size of the view the same works, and ensures Doodles will not get
downscaled.

Screenshots on tablet/phone: https://imgur.com/a/ntrQj

BUG=612156
==========

[Bernhard Bauer, 2016-06-20 19:51:01]

On 2016/06/20 18:10:04, Ted C wrote:
> lgtm
> 
> Sorry about that!  Did the tablet logos get slightly bigger with this change? 

Yes, a little bit. I think what's happening here is that the height of the logo
view for >600dp was already bigger than the default logo assets, and I eyeballed
the size of the >600dp logo to have approximately the same ratio to the <600dp
one as the corresponding height values in dimens.xml have (rounded to the height
values that we have official logo bitmaps for). It ended up changing the height
from 94dp to 96dp.

> Also, update the commit message?

Done!

[Bernhard Bauer, 2016-06-20 19:51:17]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2016-06-20 19:51:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2068883003/20001

[commit-bot: I haz the power, 2016-06-20 20:34:23]

Description was changed from

==========
Update size of Google Logo on Android NTP.

The logo is now 64dp high on <600dp screens, and 96dp on bigger screens.

Because the default logo is never upscaled, using smaller assets while keeping
the size of the view the same works, and ensures Doodles will not get
downscaled.

Screenshots on tablet/phone: https://imgur.com/a/ntrQj

BUG=612156
==========

to

==========
Update size of Google Logo on Android NTP.

The logo is now 64dp high on <600dp screens, and 96dp on bigger screens.

Because the default logo is never upscaled, using smaller assets while keeping
the size of the view the same works, and ensures Doodles will not get
downscaled.

Screenshots on tablet/phone: https://imgur.com/a/ntrQj

BUG=612156
==========

[commit-bot: I haz the power, 2016-06-20 20:34:26]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-20 20:41:13]

Description was changed from

==========
Update size of Google Logo on Android NTP.

The logo is now 64dp high on <600dp screens, and 96dp on bigger screens.

Because the default logo is never upscaled, using smaller assets while keeping
the size of the view the same works, and ensures Doodles will not get
downscaled.

Screenshots on tablet/phone: https://imgur.com/a/ntrQj

BUG=612156
==========

to

==========
Update size of Google Logo on Android NTP.

The logo is now 64dp high on <600dp screens, and 96dp on bigger screens.

Because the default logo is never upscaled, using smaller assets while keeping
the size of the view the same works, and ensures Doodles will not get
downscaled.

Screenshots on tablet/phone: https://imgur.com/a/ntrQj

BUG=612156

Committed: https://crrev.com/fb7d5e9e3633a8df4fca421c1d0ddefa6d1c2a4e
Cr-Commit-Position: refs/heads/master@{#400769}
==========

[commit-bot: I haz the power, 2016-06-20 20:41:14]

Patchset 2 (id:??) landed as
https://crrev.com/fb7d5e9e3633a8df4fca421c1d0ddefa6d1c2a4e
Cr-Commit-Position: refs/heads/master@{#400769}