GetScaleFactorForNativeView should return scale factor in float

GetScaleFactorForNativeView should return scale factor in float

Step 1 to reduce the use of ui::ScaleFactor and
eliminate ui::GetImageScale

BUG=372212
R=ananta@chromium.org, sky@chromium.org
TBR=jam@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271587

[ananta, 2014-05-19 18:34:05]

lgtm

[oshima, 2014-05-19 19:41:52]

sky -> OWNERS

[sky, 2014-05-19 22:23:00]

https://codereview.chromium.org/293563002/diff/170001/ui/base/layout.h
File ui/base/layout.h (right):

https://codereview.chromium.org/293563002/diff/170001/ui/base/layout.h#newcode56
ui/base/layout.h:56: UI_BASE_EXPORT float
GetScaleFactorForNativeView(gfx::NativeView view);
Shouln't this be GetScaleForNativeView? This better matches other names here, eg
GetScaleForScaleFactor.

[oshima, 2014-05-19 22:45:11]

https://codereview.chromium.org/293563002/diff/170001/ui/base/layout.h
File ui/base/layout.h (right):

https://codereview.chromium.org/293563002/diff/170001/ui/base/layout.h#newcode56
ui/base/layout.h:56: UI_BASE_EXPORT float
GetScaleFactorForNativeView(gfx::NativeView view);
On 2014/05/19 22:23:01, sky wrote:
> Shouln't this be GetScaleForNativeView? This better matches other names here,
eg
> GetScaleForScaleFactor.

GetImageScale and GetScaleForScaleFactor will be removed, and
ui::ScaleFactor will be renamed to ui::ResourceScaleFactor as 
scale factor can be arbitrary now.

I'd like to keep the word "scale factor" to avoid confusion with other scales.

[sky, 2014-05-19 23:01:34]

On Mon, May 19, 2014 at 3:45 PM,  <oshima@chromium.org> wrote:
>
> https://codereview.chromium.org/293563002/diff/170001/ui/base/layout.h
> File ui/base/layout.h (right):
>
>
https://codereview.chromium.org/293563002/diff/170001/ui/base/layout.h#newcode56
> ui/base/layout.h:56: UI_BASE_EXPORT float
> GetScaleFactorForNativeView(gfx::NativeView view);
> On 2014/05/19 22:23:01, sky wrote:
>>
>> Shouln't this be GetScaleForNativeView? This better matches other
>
> names here, eg
>>
>> GetScaleForScaleFactor.
>
>
> GetImageScale and GetScaleForScaleFactor will be removed, and
> ui::ScaleFactor will be renamed to ui::ResourceScaleFactor as
> scale factor can be arbitrary now.
>
> I'd like to keep the word "scale factor" to avoid confusion with other
> scales.

Won't it be a scale you're returning though? And won't we still have
functions like IsScaleFactorySupported(ResourceScaleFactor)?


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

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[oshima, 2014-05-19 23:40:06]

On 2014/05/19 23:01:34, sky wrote:
> On Mon, May 19, 2014 at 3:45 PM,  <mailto:oshima@chromium.org> wrote:
> >
> > https://codereview.chromium.org/293563002/diff/170001/ui/base/layout.h
> > File ui/base/layout.h (right):
> >
> >
>
https://codereview.chromium.org/293563002/diff/170001/ui/base/layout.h#newcode56
> > ui/base/layout.h:56: UI_BASE_EXPORT float
> > GetScaleFactorForNativeView(gfx::NativeView view);
> > On 2014/05/19 22:23:01, sky wrote:
> >>
> >> Shouln't this be GetScaleForNativeView? This better matches other
> >
> > names here, eg
> >>
> >> GetScaleForScaleFactor.
> >
> >
> > GetImageScale and GetScaleForScaleFactor will be removed, and
> > ui::ScaleFactor will be renamed to ui::ResourceScaleFactor as
> > scale factor can be arbitrary now.
> >
> > I'd like to keep the word "scale factor" to avoid confusion with other
> > scales.
> 
> Won't it be a scale you're returning though?

Not sure if I understood your question. GetScaleFactorForNativeView returns
device scale factor, and that's why I'd like to keep "scale factor".


> And won't we still have
> functions like IsScaleFactorySupported(ResourceScaleFactor)?

IsScaleFactorSupported will also be removed from layout.h. (The code will be in
ui/base/resources,
but not as public API)

What I'm going to do is to eliminate most of functions that uses
ui::ScaleFactor,
and rename this enum to ui::ResourceScaleFactor, then move the rest of function
to ui/base/resources/
directory along with the enum definition.

> 
> >
> > https://codereview.chromium.org/293563002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-05-19 23:56:01]

LGTM - Oshima is going to try and find a better name.

[oshima, 2014-05-20 01:03:37]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-05-20 01:04:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/293563002/180001

[commit-bot: I haz the power, 2014-05-20 02:28:06]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-05-20 02:33:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-20 02:33:05]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[oshima, 2014-05-20 06:24:35]

tbr'ing jam for trivial return type change in content/public/browser/web_ui.h

[oshima, 2014-05-20 06:25:36]

Committed patchset #5 manually as r271587 (presubmit successful).