[Device Service] Eliminate content::ScreenOrientationObserver.

[Device Service] Eliminate content::ScreenOrientationObserver.

This CL lets Blink talk directly with Device Service to consume the
mojo interface device.mojom.ScreenOrientationListener, then removes
content::ScreenOrientationObserver completely.

BUG=678545
Review-Url: https://codereview.chromium.org/2751513007
Cr-Commit-Position: refs/heads/master@{#457713}
Committed: https://chromium.googlesource.com/chromium/src/+/b7d0a0d196d6b45cb3c4acad994ce98ec05cb044

[leonhsl(Using Gerrit), 2017-03-15 07:41:48]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 07:42:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 07:45:38]

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

[commit-bot: I haz the power, 2017-03-15 07:45:39]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[leonhsl(Using Gerrit), 2017-03-15 07:51:37]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[leonhsl(Using Gerrit), 2017-03-15 07:51:44]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-03-15 07:51:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 07:57:58]

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

[commit-bot: I haz the power, 2017-03-15 07:57:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[leonhsl(Using Gerrit), 2017-03-15 08:27:34]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 08:27:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-03-15 08:28:05]

Patchset #1 (id:20001) has been deleted

[leonhsl(Using Gerrit), 2017-03-15 10:34:38]

leon.han@intel.com changed reviewers:
+ blundell@chromium.org, mlamouri@chromium.org

[leonhsl(Using Gerrit), 2017-03-15 10:34:39]

Hi, Colin, Mounir, PTAL, Thanks!

https://codereview.chromium.org/2751513007/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationDispatcher.cpp
(right):

https://codereview.chromium.org/2751513007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationDispatcher.cpp:31:
DCHECK(!m_listener);
This is a little different behavior with before, but I think it makes more
sense, before we've been keeping alive one ScreenOrientationListener impl in
browser side until its corresponding renderer process ends, but now we can just
keep it on need.

Now the lifecycle of the mojo connection is between 
  startListening() --> When the first one window.screen.orientation is created.
and
  stopListening() --> When all window.screen.orientation disappear.

[commit-bot: I haz the power, 2017-03-15 10:59:14]

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

[commit-bot: I haz the power, 2017-03-15 10:59:15]

Dry run: This issue passed the CQ dry run.

[blundell, 2017-03-16 10:25:54]

lgtm with question

This is great to see, thanks!

https://codereview.chromium.org/2751513007/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationDispatcher.cpp
(right):

https://codereview.chromium.org/2751513007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationDispatcher.cpp:22:
if (m_listener)
Do you know if this can occur? Otherwise, it would be nice to
DCHECK(!m_listener) instead.

[leonhsl(Using Gerrit), 2017-03-16 10:37:32]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 10:37:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-03-16 10:43:51]

Hi, Colin, Thanks a lot for kindly review! Uploaded ps#2, PTAnL.

https://codereview.chromium.org/2751513007/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationDispatcher.cpp
(right):

https://codereview.chromium.org/2751513007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationDispatcher.cpp:22:
if (m_listener)
On 2017/03/16 10:25:54, blundell wrote:
> Do you know if this can occur? Otherwise, it would be nice to
> DCHECK(!m_listener) instead.

Yeah I also think DCHECK(!m_listener) is much better. I was playing for safety
:)

[blundell, 2017-03-16 10:47:52]

still lgtm, thanks

[leonhsl(Using Gerrit), 2017-03-16 10:48:49]

Hi, Mounir, as you own almost all files of this CL, definitely PTAL or RS it:)
Thanks.

[mlamouri (slow - plz ping), 2017-03-16 12:11:47]

lgtm

https://codereview.chromium.org/2751513007/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationDispatcher.h
(right):

https://codereview.chromium.org/2751513007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationDispatcher.h:23:
: public GarbageCollectedFinalized<ScreenOrientationDispatcher>,
Is it worth making the object Finalized just for a DCHECK()? Finalized comes
with a cost.

[commit-bot: I haz the power, 2017-03-16 12:35:11]

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

[commit-bot: I haz the power, 2017-03-16 12:35:12]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2017-03-17 03:01:12]

Thank you Mounir!

https://codereview.chromium.org/2751513007/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationDispatcher.h
(right):

https://codereview.chromium.org/2751513007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationDispatcher.h:23:
: public GarbageCollectedFinalized<ScreenOrientationDispatcher>,
On 2017/03/16 12:11:47, mlamouri wrote:
> Is it worth making the object Finalized just for a DCHECK()? Finalized comes
> with a cost.

Seems similar codes are already existing inside Blink codebase,, so for our
case, for now I'd like to keep this DCHECK() for safety, we could remove this
later at any time when we're sure it's unnecessary :)

[leonhsl(Using Gerrit), 2017-03-17 03:12:41]

leon.han@intel.com changed reviewers:
+ kinuko@chromium.org

[leonhsl(Using Gerrit), 2017-03-17 03:12:42]

+kinuko@, would you PTAL for OWNER review of
   content/renderer/BUILD.gn
   content/renderer/renderer_blink_platform_impl.cc
   third_party/WebKit/public/platform/WebPlatformEventType.h

Thanks!

[kinuko, 2017-03-17 04:38:10]

The content and public changes look good, I do have one question though.

https://codereview.chromium.org/2751513007/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationDispatcher.h
(right):

https://codereview.chromium.org/2751513007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationDispatcher.h:23:
: public GarbageCollectedFinalized<ScreenOrientationDispatcher>,
On 2017/03/17 03:01:12, leonhsl wrote:
> On 2017/03/16 12:11:47, mlamouri wrote:
> > Is it worth making the object Finalized just for a DCHECK()? Finalized comes
> > with a cost.
> 
> Seems similar codes are already existing inside Blink codebase,, so for our
> case, for now I'd like to keep this DCHECK() for safety, we could remove this
> later at any time when we're sure it's unnecessary :)

I also wonder if we want to do this.  Actually it looks this dispatcher is
DEFINE_STATIC_LOCAL'ed instance, don't we just leak it then?

[leonhsl(Using Gerrit), 2017-03-17 06:14:06]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 06:14:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-03-17 06:21:38]

Hi, kinuko@, Thanks for reminding about DEFINE_STATIC_LOCAL. Uploaded ps#3,
PTAnL.

https://codereview.chromium.org/2751513007/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationDispatcher.h
(right):

https://codereview.chromium.org/2751513007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationDispatcher.h:23:
: public GarbageCollectedFinalized<ScreenOrientationDispatcher>,
On 2017/03/17 04:38:10, kinuko wrote:
> On 2017/03/17 03:01:12, leonhsl wrote:
> > On 2017/03/16 12:11:47, mlamouri wrote:
> > > Is it worth making the object Finalized just for a DCHECK()? Finalized
comes
> > > with a cost.
> > 
> > Seems similar codes are already existing inside Blink codebase,, so for our
> > case, for now I'd like to keep this DCHECK() for safety, we could remove
this
> > later at any time when we're sure it's unnecessary :)
> 
> I also wonder if we want to do this.  Actually it looks this dispatcher is
> DEFINE_STATIC_LOCAL'ed instance, don't we just leak it then?

Oh I checked around DEFINE_STATIC_LOCAL, and got something new to me:
"
// Use |DEFINE_STATIC_LOCAL()| to declare and define a static local variable
// (|static T;|) so that it is leaked and its destructors are not called at
// exit. 
"
So it means that our destructor will never get called, means that DCHECK will
never be reached. 
And, to think further, this no destruction behavior should have no bad effect to
us, even if m_listener is alive, when renderer process exits, the mojo
connection peer side can still get notified that underlying message pipe broke.

I was taking BatteryDispatcher at [1] as an example, so we could also avoid
GarbageCollectedFinalized there and maybe many other places..
[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/batter...

[commit-bot: I haz the power, 2017-03-17 06:26:08]

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

[commit-bot: I haz the power, 2017-03-17 06:26:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[leonhsl(Using Gerrit), 2017-03-17 06:37:45]

Seems trybots complain that ScreenOrientationDispatcher requires finalization,,
so now we're in a dilemma:
DEFINE_STATIC_LOCAL does not call destructor,
and
PlatformEventDispatcher enforces ScreenOrientationDispatcher to be garbage
collectable while this further requires a finalization because of the new member
m_listener.

../../third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationDispatcher.h:22:1:
error: [blink-gc] Class 'ScreenOrientationDispatcher' requires finalization.
class ScreenOrientationDispatcher final
^
../../third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationDispatcher.h:40:3:
note: [blink-gc] Field 'm_listener' requiring finalization declared here:
  device::mojom::blink::ScreenOrientationListenerPtr m_listener;
  ^
1 error generated.

[kinuko, 2017-03-17 07:26:29]

I see, we have a field that needs finalization and there's an external reason
that needs to be GC'ed class. Ok I think PS2 makes sense. LGTM for PS2 (and
cc-ing oilpan-review in case they may have comments)

[sof, 2017-03-17 07:38:39]

On 2017/03/17 07:26:29, kinuko wrote:
> I see, we have a field that needs finalization and there's an external reason
> that needs to be GC'ed class. Ok I think PS2 makes sense. LGTM for PS2 (and
> cc-ing oilpan-review in case they may have comments)

ps#2 looks fine; i don't think eager finalization (or not) needs to be
considered for this singleton dispatcher.

[leonhsl(Using Gerrit), 2017-03-17 08:05:48]

OK now I suppose ps#2 makes everyone happy :) Let me delete ps#3 and send ps#2
to CQ now. Thanks all!

[leonhsl(Using Gerrit), 2017-03-17 08:06:13]

Patchset #3 (id:80001) has been deleted

[leonhsl(Using Gerrit), 2017-03-17 08:07:10]

The CQ bit was checked by leon.han@intel.com

[commit-bot: I haz the power, 2017-03-17 08:07:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 08:12:00]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489738030523580,
"parent_rev": "6e4c507dc612077518d242eabcfae9622601d7e6", "commit_rev":
"b7d0a0d196d6b45cb3c4acad994ce98ec05cb044"}

[commit-bot: I haz the power, 2017-03-17 08:12:42]

Description was changed from

==========
[Device Service] Eliminate content::ScreenOrientationObserver.

This CL lets Blink talk directly with Device Service to consume the
mojo interface device.mojom.ScreenOrientationListener, then removes
content::ScreenOrientationObserver completely.

BUG=678545
==========

to

==========
[Device Service] Eliminate content::ScreenOrientationObserver.

This CL lets Blink talk directly with Device Service to consume the
mojo interface device.mojom.ScreenOrientationListener, then removes
content::ScreenOrientationObserver completely.

BUG=678545

Review-Url: https://codereview.chromium.org/2751513007
Cr-Commit-Position: refs/heads/master@{#457713}
Committed:
https://chromium.googlesource.com/chromium/src/+/b7d0a0d196d6b45cb3c4acad994c...
==========

[commit-bot: I haz the power, 2017-03-17 08:12:43]

Committed patchset #2 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b7d0a0d196d6b45cb3c4acad994c...

[haraken, 2017-03-17 10:06:42]

PS2 LGTM