Make ScreenOrientationController a FrameDestructionObserver.

Make ScreenOrientationController a FrameDestructionObserver.

With LocalFrame now on the heap, switch this controller to be a frame
destruction observer so as to conventionally handle tracing of the
frame during GCs, and eventual frame detach.

This is a tidying change; what's currently provided isn't unsafe, as
the controller is also a page lifecycle observer and uses the lack of
a page to detect detached operation. But it is preferable overall
to handle frame access and lifetime using FrameDestructionObserver.

R=haraken,mlamouri
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182497

[sof, 2014-09-23 08:03:02]

sigbjornf@opera.com changed reviewers:
+ mlamouri@chromium.org, oilpan-reviews@chromium.org

[sof, 2014-09-23 08:03:03]

Please take a look.

[haraken, 2014-09-23 08:23:05]

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

[haraken, 2014-09-23 08:23:05]

What's a criteria where you want to use FrameDestructionObserver? If a class A
has 'LocalFrame& m_frame', does it mean that we want to make A a
FrameDestructionObserver?

I'm asking this because you're making ScreenOrientationController a
FrameDestructionObserver even though frame() will never become 0 in
ScreenOrientationController.

[sof, 2014-09-23 08:50:30]

On 2014/09/23 08:23:05, haraken wrote:
> What's a criteria where you want to use FrameDestructionObserver? If a class A
> has 'LocalFrame& m_frame', does it mean that we want to make A a
> FrameDestructionObserver?
> 
> I'm asking this because you're making ScreenOrientationController a
> FrameDestructionObserver even though frame() will never become 0 in
> ScreenOrientationController.

It isn't clear to me what provides that guarantee.

FrameDestructionObserver manages a LocalFrame member "for free". Given how few
there are of these controller objects, I'd argue it is preferable to reuse FDO
rather than keeping a Member<LocalFrame> on the controller. That was the
underlying reasoning used here.

[haraken, 2014-09-23 08:51:55]

On 2014/09/23 08:50:30, sof wrote:
> On 2014/09/23 08:23:05, haraken wrote:
> > What's a criteria where you want to use FrameDestructionObserver? If a class
A
> > has 'LocalFrame& m_frame', does it mean that we want to make A a
> > FrameDestructionObserver?
> > 
> > I'm asking this because you're making ScreenOrientationController a
> > FrameDestructionObserver even though frame() will never become 0 in
> > ScreenOrientationController.
> 
> It isn't clear to me what provides that guarantee.
> 
> FrameDestructionObserver manages a LocalFrame member "for free". Given how few
> there are of these controller objects, I'd argue it is preferable to reuse FDO
> rather than keeping a Member<LocalFrame> on the controller. That was the
> underlying reasoning used here.

Thanks, it makes sense to use a FrameDestructionObserver for controllers. LGTM.

[sof, 2014-09-23 09:01:02]

On 2014/09/23 08:51:55, haraken wrote:
> On 2014/09/23 08:50:30, sof wrote:
> > On 2014/09/23 08:23:05, haraken wrote:
> > > What's a criteria where you want to use FrameDestructionObserver? If a
class
> A
> > > has 'LocalFrame& m_frame', does it mean that we want to make A a
> > > FrameDestructionObserver?
> > > 
> > > I'm asking this because you're making ScreenOrientationController a
> > > FrameDestructionObserver even though frame() will never become 0 in
> > > ScreenOrientationController.
> > 
> > It isn't clear to me what provides that guarantee.
> > 
> > FrameDestructionObserver manages a LocalFrame member "for free". Given how
few
> > there are of these controller objects, I'd argue it is preferable to reuse
FDO
> > rather than keeping a Member<LocalFrame> on the controller. That was the
> > underlying reasoning used here.
> 
> Thanks, it makes sense to use a FrameDestructionObserver for controllers.
LGTM.

Thanks, will await module owner's feedback.

[mlamouri (slow - plz ping), 2014-09-23 10:29:49]

lgtm with the rename

https://codereview.chromium.org/595783003/diff/1/Source/modules/screen_orient...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/595783003/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientationController.cpp:94: bool
ScreenOrientationController::isActive() const
Could you rename this "isActiveAndVisible()"?

[sof, 2014-09-23 12:48:41]

Thanks for the quick reviews! :)

https://codereview.chromium.org/595783003/diff/1/Source/modules/screen_orient...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/595783003/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientationController.cpp:94: bool
ScreenOrientationController::isActive() const
On 2014/09/23 10:29:49, Mounir Lamouri wrote:
> Could you rename this "isActiveAndVisible()"?

Done.

[sof, 2014-09-23 12:48:45]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-09-23 12:49:07]

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

[commit-bot: I haz the power, 2014-09-23 13:51:36]

Committed patchset #2 (id:20001) as 182497