Make DeviceDisplayInfo a Singleton and have ScreenOrientationListener update it.

Make DeviceDisplayInfo a Singleton and have ScreenOrientationListener update it.

This is not the best design. Ideally, we might want to create a new
listener specific for Display changes (onConfigurationChange or
DisplayListener). It would be created at startup and
ScreenOrientationListener would simply listen to it while
DeviceDisplayInfo would be updated when a Display change would be
detected.

BUG=354275

[mlamouri (slow - plz ping), 2014-03-27 11:27:38]

aelias@chromium.org: Please review changes in:
ui/

benm@chromium.org: Please review changes in:
everything else

+CC bulach@ for information

[mlamouri (slow - plz ping), 2014-03-31 10:05:27]

aelias@ and benm@, I forgot to point that this CL need to land for M35. We
already missed the branching point. It would be great if we could get it in
sooner than later :)

[boliu, 2014-03-31 15:49:17]

Drive by...

It's wrong for DeviceDisplayInfo to grab the application context. This makes the
assumption that there is only one display with one density etc, which in general
is not true with attaching an hdmi cable to a big screen. I've been meaning to
fix that but haven't had time. But this is going more into the wrong direction.

Other than not creating a new DeviceDisplayInfo every time it's needed and
leaving garbage behind, is this CL trying to solve anything else?

[mlamouri (slow - plz ping), 2014-03-31 16:25:14]

On 2014/03/31 15:49:17, boliu wrote:
> Drive by...
> 
> It's wrong for DeviceDisplayInfo to grab the application context. This makes
the
> assumption that there is only one display with one density etc, which in
general
> is not true with attaching an hdmi cable to a big screen. I've been meaning to
> fix that but haven't had time. But this is going more into the wrong
direction.

Why is that going in the "wrong direction". It doesn't seem to make the two
screens scenario any worse or any better as far as I can tell. What makes you
think that?

> Other than not creating a new DeviceDisplayInfo every time it's needed and
> leaving garbage behind, is this CL trying to solve anything else?

The idea is to fix this:
http://code.google.com/p/chromium/issues/detail?id=354275

[boliu, 2014-03-31 16:39:30]

On 2014/03/31 16:25:14, Mounir Lamouri wrote:
> On 2014/03/31 15:49:17, boliu wrote:
> > Drive by...
> > 
> > It's wrong for DeviceDisplayInfo to grab the application context. This makes
> the
> > assumption that there is only one display with one density etc, which in
> general
> > is not true with attaching an hdmi cable to a big screen. I've been meaning
to
> > fix that but haven't had time. But this is going more into the wrong
> direction.
> 
> Why is that going in the "wrong direction". It doesn't seem to make the two
> screens scenario any worse or any better as far as I can tell. What makes you
> think that?

Because after this, code using DeviceDisplayInfo can safely assume it's a
singleton when it really should be per context. So fixing that will be harder in
the future.

I do like getting rid of registerComponentCallbacks though. Looks like it never
unregisters the callback, which means it was leaking before :/

> 
> > Other than not creating a new DeviceDisplayInfo every time it's needed and
> > leaving garbage behind, is this CL trying to solve anything else?
> 
> The idea is to fix this:
> http://code.google.com/p/chromium/issues/detail?id=354275

So the only call to getInstance that you added (rather than replacing create) is
in ScreenOrientationListener. Is that code called per frame or very frequently?
If yes, can it hold on to a reference to an instance of DeviceDisplayInfo? If
not, is it so bad to just create a new DeviceDisplayInfo instance and let gc
take care of it?

[mlamouri (slow - plz ping), 2014-03-31 19:50:43]

On 2014/03/31 16:39:30, boliu wrote:
> On 2014/03/31 16:25:14, Mounir Lamouri wrote:
> > On 2014/03/31 15:49:17, boliu wrote:
> > > Drive by...
> > > 
> > > It's wrong for DeviceDisplayInfo to grab the application context. This
makes
> > the
> > > assumption that there is only one display with one density etc, which in
> > general
> > > is not true with attaching an hdmi cable to a big screen. I've been
meaning
> to
> > > fix that but haven't had time. But this is going more into the wrong
> > direction.
> > 
> > Why is that going in the "wrong direction". It doesn't seem to make the two
> > screens scenario any worse or any better as far as I can tell. What makes
you
> > think that?
> 
> Because after this, code using DeviceDisplayInfo can safely assume it's a
> singleton when it really should be per context. So fixing that will be harder
in
> the future.
> 
> I do like getting rid of registerComponentCallbacks though. Looks like it
never
> unregisters the callback, which means it was leaking before :/

getInstance() takes a Context especially so we could return an instance that
depends of the Context easily by having a Hashtable that associates a Context
with an instance. I could rename this get() if the term |instance| is
over-bloated.

> > > Other than not creating a new DeviceDisplayInfo every time it's needed and
> > > leaving garbage behind, is this CL trying to solve anything else?
> > 
> > The idea is to fix this:
> > http://code.google.com/p/chromium/issues/detail?id=354275
> 
> So the only call to getInstance that you added (rather than replacing create)
is
> in ScreenOrientationListener. Is that code called per frame or very
frequently?
> If yes, can it hold on to a reference to an instance of DeviceDisplayInfo? If
> not, is it so bad to just create a new DeviceDisplayInfo instance and let gc
> take care of it?

That should be possible, indeed.

[boliu, 2014-03-31 20:01:07]

On 2014/03/31 19:50:43, Mounir Lamouri wrote:
> getInstance() takes a Context especially so we could return an instance that
> depends of the Context easily by having a Hashtable that associates a Context
> with an instance. I could rename this get() if the term |instance| is
> over-bloated.

But who's responsible then for cleaning up the hash table then?

> 
> > > > Other than not creating a new DeviceDisplayInfo every time it's needed
and
> > > > leaving garbage behind, is this CL trying to solve anything else?
> > > 
> > > The idea is to fix this:
> > > http://code.google.com/p/chromium/issues/detail?id=354275
> > 
> > So the only call to getInstance that you added (rather than replacing
create)
> is
> > in ScreenOrientationListener. Is that code called per frame or very
> frequently?
> > If yes, can it hold on to a reference to an instance of DeviceDisplayInfo?
If
> > not, is it so bad to just create a new DeviceDisplayInfo instance and let gc
> > take care of it?
> 
> That should be possible, indeed.

You should just do that then. The singleton change can happen in another CL.
Keep this CL small and focused to it's safer to merge to a release branch.

[mlamouri (slow - plz ping), 2014-03-31 20:44:11]

Things moved there: https://codereview.chromium.org/219963002/

Closing this CL.