[screen-orientation] Expose orientation info in OrientationInformation interface.

[screen-orientation] Expose orientation info in OrientationInformation interface.

BUG=162827
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177486

[abarth-chromium, 2014-07-02 17:50:04]

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

https://codereview.chromium.org/366853008/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientationController.h:52:
Persistent<OrientationInformation> m_orientation;
Persistent -> Member?

[mlamouri (slow - plz ping), 2014-07-02 17:51:52]

On 2014/07/02 at 17:50:04, abarth wrote:
>
https://codereview.chromium.org/366853008/diff/1/Source/modules/screen_orient...
> File Source/modules/screen_orientation/ScreenOrientationController.h (right):
> 
>
https://codereview.chromium.org/366853008/diff/1/Source/modules/screen_orient...
> Source/modules/screen_orientation/ScreenOrientationController.h:52:
Persistent<OrientationInformation> m_orientation;
> Persistent -> Member?

Member<> is only usable if the owner is GarbageCollected. Persistent<> should be
used otherwise.

[abarth-chromium, 2014-07-02 18:01:40]

On 2014/07/02 at 17:51:52, mlamouri wrote:
> On 2014/07/02 at 17:50:04, abarth wrote:
> >
https://codereview.chromium.org/366853008/diff/1/Source/modules/screen_orient...
> > File Source/modules/screen_orientation/ScreenOrientationController.h
(right):
> > 
> >
https://codereview.chromium.org/366853008/diff/1/Source/modules/screen_orient...
> > Source/modules/screen_orientation/ScreenOrientationController.h:52:
Persistent<OrientationInformation> m_orientation;
> > Persistent -> Member?
> 
> Member<> is only usable if the owner is GarbageCollected. Persistent<> should
be used otherwise.

public NoBaseWillBeGarbageCollectedFinalized<ScreenOrientationController> ... I
guess PersistentWillBeMember?

[abarth-chromium, 2014-07-02 18:01:40]

On 2014/07/02 at 17:51:52, mlamouri wrote:
> On 2014/07/02 at 17:50:04, abarth wrote:
> >
https://codereview.chromium.org/366853008/diff/1/Source/modules/screen_orient...
> > File Source/modules/screen_orientation/ScreenOrientationController.h
(right):
> > 
> >
https://codereview.chromium.org/366853008/diff/1/Source/modules/screen_orient...
> > Source/modules/screen_orientation/ScreenOrientationController.h:52:
Persistent<OrientationInformation> m_orientation;
> > Persistent -> Member?
> 
> Member<> is only usable if the owner is GarbageCollected. Persistent<> should
be used otherwise.

public NoBaseWillBeGarbageCollectedFinalized<ScreenOrientationController> ... I
guess PersistentWillBeMember?

[mlamouri (slow - plz ping), 2014-07-02 18:06:21]

On 2014/07/02 at 18:01:40, abarth wrote:
> On 2014/07/02 at 17:51:52, mlamouri wrote:
> > On 2014/07/02 at 17:50:04, abarth wrote:
> > >
https://codereview.chromium.org/366853008/diff/1/Source/modules/screen_orient...
> > > File Source/modules/screen_orientation/ScreenOrientationController.h
(right):
> > > 
> > >
https://codereview.chromium.org/366853008/diff/1/Source/modules/screen_orient...
> > > Source/modules/screen_orientation/ScreenOrientationController.h:52:
Persistent<OrientationInformation> m_orientation;
> > > Persistent -> Member?
> > 
> > Member<> is only usable if the owner is GarbageCollected. Persistent<>
should be used otherwise.
> 
> public NoBaseWillBeGarbageCollectedFinalized<ScreenOrientationController> ...
I guess PersistentWillBeMember?

Eh... My source (http://www.chromium.org/blink/blink-gc) wasn't mentioning that.
Updated.

[abarth-chromium, 2014-07-02 18:25:10]

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
File Source/modules/screen_orientation/OrientationInformation.cpp (right):

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/OrientationInformation.cpp:9: #include
"wtf/text/WTFString.h"
You should move this include to the header because you're using |String| in the
header.

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/OrientationInformation.cpp:18:
orientation->m_angle = 0;
Totally an unimportant nit, but we usually would call the setters in this case
given that they exist.  Even though C++ is a class-private language, we often
treat it as if it were instance-private.

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/OrientationInformation.cpp:39: }
Why do you need a destructor at all?  You can use just GarbageCollected rather
than GarbageCollectedFinalized

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
File Source/modules/screen_orientation/OrientationInformation.h (right):

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/OrientationInformation.h:18: static
OrientationInformation* createFake();
I wonder if we should pick a slightly different name for this function. 
|createFake| makes me think that this is a something for testing.  Maybe
|createPlaceholder| ?

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/OrientationInformation.h:20: virtual
~OrientationInformation();
Why does this destructor need to be virtual?

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/OrientationInformation.h:30: friend class
ScreenOrientationController;
Please don't use friends.  Just expose the functions you need to expose.

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/OrientationInformation.h:33:
OrientationInformation(const OrientationInformation&);
It's unusual to have a copy constructor for a DOM object.  Maybe use a static
|clone| function instead?

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/OrientationInformation.h:35: bool
operator!=(const OrientationInformation&);
These are also unusual.  Maybe an |equals| function?

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/OrientationInformation.h:38: void
setAngle(unsigned short);
You can just make these public.

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:124: return
OrientationInformation::createFake();
Why not just return 0?  That's what we usually do when we're disconnected from
the frame.

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientationController.cpp:19: 
This blank line looks spurious

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientationController.cpp:101:
OrientationInformation previousOrientation = *m_orientation;
Hum..  We shouldn't keep DOM object as values on the stack.  What happens if
someone ends up with a pointer to them?  Instead, I'd recommend something like:

OrientationInformation* previousOrientation = m_orientation->clone();

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientationController.cpp:109: if
(m_frame == m_frame.localFrameRoot() && previousOrientation != *m_orientation)
Here I'd just call !previousOrientation->equals(m_orientation)

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientationController.cpp:110:
ScreenOrientationController::from(m_frame).notifyOrientationChanged();
Can ScreenOrientationController::from(m_frame) return something other than
|this| ?

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientationController.cpp:121:
updateOrientation();
Can we end up calling this function again when notifyOrientationChanged() is
called from ScreenOrientationController::pageVisibilityChanged?

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientationController.cpp:126:
window->dispatchEvent(Event::create(EventTypeNames::orientationchange));
What happens if script below this point destroys |this| ?

[mlamouri (slow - plz ping), 2014-07-02 19:18:13]

PTAL

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
File Source/modules/screen_orientation/OrientationInformation.cpp (right):

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/OrientationInformation.cpp:9: #include
"wtf/text/WTFString.h"
On 2014/07/02 18:25:09, abarth wrote:
> You should move this include to the header because you're using |String| in
the
> header.

Done.

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/OrientationInformation.cpp:18:
orientation->m_angle = 0;
On 2014/07/02 18:25:09, abarth wrote:
> Totally an unimportant nit, but we usually would call the setters in this case
> given that they exist.  Even though C++ is a class-private language, we often
> treat it as if it were instance-private.

Done.

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/OrientationInformation.cpp:39: }
On 2014/07/02 18:25:09, abarth wrote:
> Why do you need a destructor at all?  You can use just GarbageCollected rather
> than GarbageCollectedFinalized

Done.

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
File Source/modules/screen_orientation/OrientationInformation.h (right):

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/OrientationInformation.h:18: static
OrientationInformation* createFake();
On 2014/07/02 18:25:09, abarth wrote:
> I wonder if we should pick a slightly different name for this function. 
> |createFake| makes me think that this is a something for testing.  Maybe
> |createPlaceholder| ?

Removed createFake() entirely.

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/OrientationInformation.h:20: virtual
~OrientationInformation();
On 2014/07/02 18:25:09, abarth wrote:
> Why does this destructor need to be virtual?

Removed destructor.

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/OrientationInformation.h:30: friend class
ScreenOrientationController;
On 2014/07/02 18:25:10, abarth wrote:
> Please don't use friends.  Just expose the functions you need to expose.

Done.

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/OrientationInformation.h:33:
OrientationInformation(const OrientationInformation&);
On 2014/07/02 18:25:09, abarth wrote:
> It's unusual to have a copy constructor for a DOM object.  Maybe use a static
> |clone| function instead?

Done.

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/OrientationInformation.h:35: bool
operator!=(const OrientationInformation&);
On 2014/07/02 18:25:09, abarth wrote:
> These are also unusual.  Maybe an |equals| function?

Done.

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/OrientationInformation.h:38: void
setAngle(unsigned short);
On 2014/07/02 18:25:09, abarth wrote:
> You can just make these public.

Done.

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:124: return
OrientationInformation::createFake();
On 2014/07/02 18:25:10, abarth wrote:
> Why not just return 0?  That's what we usually do when we're disconnected from
> the frame.

Because we are usually not throwing and if we return 0, screen.orientation.angle
would throw. Though, I will return 0 if you prefer.

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientationController.cpp:19: 
On 2014/07/02 18:25:10, abarth wrote:
> This blank line looks spurious

Done.

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientationController.cpp:101:
OrientationInformation previousOrientation = *m_orientation;
On 2014/07/02 18:25:10, abarth wrote:
> Hum..  We shouldn't keep DOM object as values on the stack.  What happens if
> someone ends up with a pointer to them?  Instead, I'd recommend something
like:
> 
> OrientationInformation* previousOrientation = m_orientation->clone();

Done.

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientationController.cpp:109: if
(m_frame == m_frame.localFrameRoot() && previousOrientation != *m_orientation)
On 2014/07/02 18:25:10, abarth wrote:
> Here I'd just call !previousOrientation->equals(m_orientation)

Done.

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientationController.cpp:110:
ScreenOrientationController::from(m_frame).notifyOrientationChanged();
On 2014/07/02 18:25:10, abarth wrote:
> Can ScreenOrientationController::from(m_frame) return something other than
> |this| ?

Done.

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientationController.cpp:121:
updateOrientation();
On 2014/07/02 18:25:10, abarth wrote:
> Can we end up calling this function again when notifyOrientationChanged() is
> called from ScreenOrientationController::pageVisibilityChanged?

Done. I check changes in pageVisibilityChanged using the angle so we don't call
this method twice.

https://codereview.chromium.org/366853008/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientationController.cpp:126:
window->dispatchEvent(Event::create(EventTypeNames::orientationchange));
On 2014/07/02 18:25:10, abarth wrote:
> What happens if script below this point destroys |this| ?

I created the |childFrames| list before notifying the current frame. We use a
RefPtr<> for LocalFrame so we should keep those children alive regardless of
what happens during the event handling, right?

[abarth-chromium, 2014-07-02 19:32:11]

LGTM

https://codereview.chromium.org/366853008/diff/60001/Source/modules/screen_or...
File Source/modules/screen_orientation/OrientationInformation.h (right):

https://codereview.chromium.org/366853008/diff/60001/Source/modules/screen_or...
Source/modules/screen_orientation/OrientationInformation.h:18: : public
GarbageCollectedFinalized<OrientationInformation>
GarbageCollectedFinalized -> GarbageCollected, right?

[sof, 2014-07-02 20:50:30]

No need to wait on ScriptWrappable allowing non-Finalized uses, but
https://codereview.chromium.org/369633003/ (or some variation thereof) will
enable it.

https://codereview.chromium.org/366853008/diff/60001/Source/modules/screen_or...
File Source/modules/screen_orientation/OrientationInformation.h (right):

https://codereview.chromium.org/366853008/diff/60001/Source/modules/screen_or...
Source/modules/screen_orientation/OrientationInformation.h:18: : public
GarbageCollectedFinalized<OrientationInformation>
On 2014/07/02 19:32:11, abarth wrote:
> GarbageCollectedFinalized -> GarbageCollected, right?

Not quite yet, but close - ScriptWrappable still has a dtor (albeit empty with
r177389.)

[abarth-chromium, 2014-07-02 20:56:40]

Ah!

[sof, 2014-07-03 06:10:20]

https://codereview.chromium.org/366853008/diff/60001/Source/modules/screen_or...
File Source/modules/screen_orientation/OrientationInformation.h (right):

https://codereview.chromium.org/366853008/diff/60001/Source/modules/screen_or...
Source/modules/screen_orientation/OrientationInformation.h:18: : public
GarbageCollectedFinalized<OrientationInformation>
On 2014/07/02 20:50:30, sof wrote:
> On 2014/07/02 19:32:11, abarth wrote:
> > GarbageCollectedFinalized -> GarbageCollected, right?
> 
> Not quite yet, but close - ScriptWrappable still has a dtor (albeit empty with
> r177389.)

With r177436, making OrientationInformation derive from GarbageCollected<> is
possible and preferable. But Oilpan'ers will sweep out such now-redundant uses
of Finalized in any case.

[Michael van Ouwerkerk, 2014-07-03 10:09:36]

lgtm

[mlamouri (slow - plz ping), 2014-07-03 13:39:14]

blink-gc still complains if I try to build with GarbageCollected instead of
GarbageCollectedFinalized. I will land with the later then.

[mlamouri (slow - plz ping), 2014-07-03 13:41:11]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2014-07-03 13:41:15]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-07-03 13:41:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlamouri@chromium.org/366853008/80001

[commit-bot: I haz the power, 2014-07-03 14:46:14]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/14134)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/15295)

[mlamouri (slow - plz ping), 2014-07-03 15:05:43]

The CQ bit was unchecked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2014-07-03 16:30:24]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-07-03 16:31:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlamouri@chromium.org/366853008/100001

[commit-bot: I haz the power, 2014-07-03 17:34:51]

Change committed as 177486

[lushnikov, 2014-07-04 15:50:13]

A revert of this CL has been created in
https://codereview.chromium.org/364253007/ by lushnikov@chromium.org.

The reason for reverting is: Speculative revert: seems like a cause of
Linux_ASAN memory leak after blink roll.

http://build.chromium.org/p/chromium.memory/buildstatus?builder=Linux%20ASan%....