Don't return unsupported scales frrom GetScaleFactorFromScale()

Don't return unsupported scales frrom GetScaleFactorFromScale()

BUG=148841
TEST=LayoutTest.GetScaleFactorFromScale

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157906

[Xianzhu, 2012-09-19 01:04:12]

Sorry there are some unexpected changes in Patch Set 1. Cleaning up.

[Xianzhu, 2012-09-19 01:10:31]

On 2012/09/19 01:04:12, Xianzhu wrote:
> Sorry there are some unexpected changes in Patch Set 1. Cleaning up.

Uploaded Patch Set 2.

[pkotwicz, 2012-09-19 02:02:17]

To help me look at this patch, is this something which needs to be in Chromium
for M23 or is this patch for M24?

[Xianzhu, 2012-09-19 02:33:04]

On 2012/09/19 02:02:17, pkotwicz wrote:
> To help me look at this patch, is this something which needs to be in Chromium
> for M23 or is this patch for M24?

This patch will be in trunk (for M24).

[oshima, 2012-09-19 07:06:35]

https://chromiumcodereview.appspot.com/10946019/diff/9/ui/base/layout.h
File ui/base/layout.h (right):

https://chromiumcodereview.appspot.com/10946019/diff/9/ui/base/layout.h#newco...
ui/base/layout.h:54: UI_EXPORT ScaleFactor GetScaleFactorFromScale(float scale);
This shouldn't return unsupported scale factor, should it? -> peter

[pkotwicz, 2012-09-19 14:22:01]

Right now GetScaleFactorFromScale(float scale) may return an unsupported scale
factor if scale == 1.4f.
From having inspected the Chromium code, it does not look as if there are any
places where we want GetScaleFactorFromScale(float scale) to return an
unsupported scale factor. I can see us wanting to return an unsupported scale
factor in the future, but it might be worth changing the behavior of
GetScaleFactorFromScale() from now and adding a separate function when needed in
the future

[pkotwicz, 2012-09-19 14:22:10]

http://codereview.chromium.org/10946019/diff/9/ui/base/layout.cc
File ui/base/layout.cc (right):

http://codereview.chromium.org/10946019/diff/9/ui/base/layout.cc#newcode92
ui/base/layout.cc:92: bool includeUnsupported) {
Nit: use camel case for variable include_unsupported

[oshima, 2012-09-19 16:03:32]

On 2012/09/19 14:22:01, pkotwicz wrote:
> Right now GetScaleFactorFromScale(float scale) may return an unsupported scale
> factor if scale == 1.4f.
> From having inspected the Chromium code, it does not look as if there are any
> places where we want GetScaleFactorFromScale(float scale) to return an
> unsupported scale factor. I can see us wanting to return an unsupported scale
> factor in the future,

In what scenario we want this behavior?

> but it might be worth changing the behavior of
> GetScaleFactorFromScale() from now and adding a separate function when needed
in
> the future

Yes. I like to avoid extra functions that looks very similar unless it's
absolutely necessary,
and changing GetScaleFactorFromScale to return only supported scale factor looks
right things to me.

[Xianzhu, 2012-09-19 17:13:21]

Thanks all for comments. Just uploaded Patch Set 3 that let
GetScaleFactorFromScale return only supported scales.

[pkotwicz, 2012-09-19 21:05:16]

LGTM with nits

http://codereview.chromium.org/10946019/diff/7001/ui/base/layout.cc
File ui/base/layout.cc (right):

http://codereview.chromium.org/10946019/diff/7001/ui/base/layout.cc#newcode116
ui/base/layout.cc:116: if (!IsScaleFactorSupported(static_cast<ScaleFactor>(i)))
Nit: iterate through the scale factors you get from GetSupportedScaleFactors()
rather than doing this, it is more efficient.

http://codereview.chromium.org/10946019/diff/7001/ui/base/layout_unittest.cc
File ui/base/layout_unittest.cc (right):

http://codereview.chromium.org/10946019/diff/7001/ui/base/layout_unittest.cc#...
ui/base/layout_unittest.cc:24: for (float scale = 0.1; scale < 3; scale += 0.1)
Nit: set your bounds between 1 and 2.

http://codereview.chromium.org/10946019/diff/7001/ui/base/layout_unittest.cc#...
ui/base/layout_unittest.cc:40: supported_factors.push_back(SCALE_FACTOR_200P);
Nit: Add either SCALE_FACTOR_140P or SCALE_FACTOR_200P.
I do not think that your test is much stronger if you add both. But its ok if
you leave it as is. (And I don't mind being proven wrong)

[Xianzhu, 2012-09-19 21:38:23]

http://codereview.chromium.org/10946019/diff/7001/ui/base/layout.cc
File ui/base/layout.cc (right):

http://codereview.chromium.org/10946019/diff/7001/ui/base/layout.cc#newcode116
ui/base/layout.cc:116: if (!IsScaleFactorSupported(static_cast<ScaleFactor>(i)))
On 2012/09/19 21:05:17, pkotwicz wrote:
> Nit: iterate through the scale factors you get from GetSupportedScaleFactors()
> rather than doing this, it is more efficient.

Oops I should have made that kind of change :)

http://codereview.chromium.org/10946019/diff/7001/ui/base/layout_unittest.cc
File ui/base/layout_unittest.cc (right):

http://codereview.chromium.org/10946019/diff/7001/ui/base/layout_unittest.cc#...
ui/base/layout_unittest.cc:24: for (float scale = 0.1; scale < 3; scale += 0.1)
On 2012/09/19 21:05:17, pkotwicz wrote:
> Nit: set your bounds between 1 and 2.

We do have scales smaller than 1 or bigger than 2 on Android.

http://codereview.chromium.org/10946019/diff/7001/ui/base/layout_unittest.cc#...
ui/base/layout_unittest.cc:40: supported_factors.push_back(SCALE_FACTOR_200P);
On 2012/09/19 21:05:17, pkotwicz wrote:
> Nit: Add either SCALE_FACTOR_140P or SCALE_FACTOR_200P.
> I do not think that your test is much stronger if you add both. But its ok if
> you leave it as is. (And I don't mind being proven wrong)

Actually this is testing the behavior of the original GetScaleFactorFromScale(),
that is, all of the factors are supported. The case that part of the factors are
supported has been tested above.

Separated the test case into 3 trying to make it clearer.

[Xianzhu, 2012-09-20 16:53:38]

sky, could you also review this CL? Thanks.

[oshima, 2012-09-20 18:10:55]

lgtm with one nit.

thank you for adding tests!

http://codereview.chromium.org/10946019/diff/11004/ui/base/layout_unittest.cc
File ui/base/layout_unittest.cc (right):

http://codereview.chromium.org/10946019/diff/11004/ui/base/layout_unittest.cc...
ui/base/layout_unittest.cc:7: #include "ui/base/layout.h"
nit: swap include order. (test file should have the header of the code it is
testing first. check out other *test.cc)

[Xianzhu, 2012-09-20 18:51:33]

http://codereview.chromium.org/10946019/diff/11004/ui/base/layout_unittest.cc
File ui/base/layout_unittest.cc (right):

http://codereview.chromium.org/10946019/diff/11004/ui/base/layout_unittest.cc...
ui/base/layout_unittest.cc:7: #include "ui/base/layout.h"
On 2012/09/20 18:10:55, oshima wrote:
> nit: swap include order. (test file should have the header of the code it is
> testing first. check out other *test.cc)

Done.

[sky, 2012-09-20 19:45:12]

LGTM

[commit-bot: I haz the power, 2012-09-20 20:05:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/10946019/19001

[commit-bot: I haz the power, 2012-09-20 21:44:11]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-20 22:54:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/10946019/20006

[commit-bot: I haz the power, 2012-09-21 02:38:12]

Change committed as 157906