Remove DOMWindowProperty from ScreenOrientationCoontroller

Remove DOMWindowProperty from ScreenOrientationCoontroller

We're replacing DOMWindowProperty with ContextLifecycleObserver.
This CL does the replacement for ScreenOrientationCoontroller.

BUG=610176
Committed: https://crrev.com/596d1c908dbd53334f9f77162becc6b1b5c8dba8
Cr-Commit-Position: refs/heads/master@{#439421}

[haraken, 2016-12-13 08:11:54]

haraken@chromium.org changed reviewers:
+ sigbjornf@opera.com

[haraken, 2016-12-13 08:11:54]

PTAL

[haraken, 2016-12-13 08:13:28]

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

[haraken, 2016-12-13 08:13:28]

mlamouri@: I have a question.

https://codereview.chromium.org/2568913003/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationControllerImpl.cpp
(right):

https://codereview.chromium.org/2568913003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationControllerImpl.cpp:214:
m_activeLock = false;

I wonder why we don't need to call stopUpdating here.

[sof, 2016-12-13 08:46:46]

lgtm % clarification on issue raised.

[mlamouri (slow - plz ping), 2016-12-13 13:33:13]

lgtm

https://codereview.chromium.org/2568913003/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationControllerImpl.cpp
(right):

https://codereview.chromium.org/2568913003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationControllerImpl.cpp:214:
m_activeLock = false;
On 2016/12/13 at 08:13:28, haraken wrote:
> I wonder why we don't need to call stopUpdating here.

I don't see any reason not to :)

[haraken, 2016-12-14 02:02:58]

>
https://codereview.chromium.org/2568913003/diff/1/third_party/WebKit/Source/m...
> File
>
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationControllerImpl.cpp
> (right):
> 
>
https://codereview.chromium.org/2568913003/diff/1/third_party/WebKit/Source/m...
>
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationControllerImpl.cpp:214:
> m_activeLock = false;
> On 2016/12/13 at 08:13:28, haraken wrote:
> > I wonder why we don't need to call stopUpdating here.
> 
> I don't see any reason not to :)

Done.

[haraken, 2016-12-14 02:03:28]

The CQ bit was checked by haraken@chromium.org

[haraken, 2016-12-14 02:03:28]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com,
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2568913003/#ps20001
(title: "temp")

[commit-bot: I haz the power, 2016-12-14 02:03:57]

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, 2016-12-14 02:35:48]

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

[commit-bot: I haz the power, 2016-12-14 02:35:49]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[haraken, 2016-12-14 04:13:57]

The CQ bit was checked by haraken@chromium.org

[haraken, 2016-12-14 04:13:58]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com,
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2568913003/#ps40001
(title: "temp")

[commit-bot: I haz the power, 2016-12-14 04:14:18]

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, 2016-12-14 06:02:09]

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

[commit-bot: I haz the power, 2016-12-14 06:02:10]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[haraken, 2016-12-14 06:11:04]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-12-14 06:11:29]

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, 2016-12-14 07:46:57]

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

[commit-bot: I haz the power, 2016-12-14 07:46:58]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[haraken, 2016-12-14 09:48:26]

The CQ bit was checked by haraken@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 09:48:40]

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, 2016-12-14 11:17:04]

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

[commit-bot: I haz the power, 2016-12-14 11:17:05]

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

[sof, 2016-12-14 14:00:31]

https://codereview.chromium.org/2568913003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationControllerImpl.cpp
(right):

https://codereview.chromium.org/2568913003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationControllerImpl.cpp:217:
stopUpdating();
Seems like this is too strong a measure.

[haraken, 2016-12-14 16:07:24]

On 2016/12/14 14:00:31, sof wrote:
>
https://codereview.chromium.org/2568913003/diff/60001/third_party/WebKit/Sour...
> File
>
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationControllerImpl.cpp
> (right):
> 
>
https://codereview.chromium.org/2568913003/diff/60001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationControllerImpl.cpp:217:
> stopUpdating();
> Seems like this is too strong a measure.

Actually I'm hitting a bit more complicated issue on this CL.

I was assuming that ContextLifecycleObserver::contextDestroyed() is called at
the same timing as DOMWindowProperty::frameDestroyed(). This assumption is true
for LocalFrame::detach() but is not true for FrameLoader::prepareForCommit().

In the latter case, contextDestroyed() is called in Document::shutdown (which is
called in FrameLoader::prepareForCommit), but frameDestroyed() is not called
until LocalDOMWindow::reset is called for the window. If we simply replace
DOMWindowProperty with ContextLifecycleObserver, we end up with clearing frame()
a bit earlier. It looks like that this matters for a couple of objects, and
ScreenOrientation is one of them.

This is why I had to add line 85 - 88 to ScreenOrientationControllerImpl.cpp.

(In any case, I'll study a bit more -- I need to understand what can happen
between FrameLoader::prepareForCommit and LocalDOMWindow::reset.)

[haraken, 2016-12-19 05:32:56]

On 2016/12/14 16:07:24, haraken wrote:
> On 2016/12/14 14:00:31, sof wrote:
> >
>
https://codereview.chromium.org/2568913003/diff/60001/third_party/WebKit/Sour...
> > File
> >
>
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationControllerImpl.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2568913003/diff/60001/third_party/WebKit/Sour...
> >
>
third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationControllerImpl.cpp:217:
> > stopUpdating();
> > Seems like this is too strong a measure.
> 
> Actually I'm hitting a bit more complicated issue on this CL.
> 
> I was assuming that ContextLifecycleObserver::contextDestroyed() is called at
> the same timing as DOMWindowProperty::frameDestroyed(). This assumption is
true
> for LocalFrame::detach() but is not true for FrameLoader::prepareForCommit().
> 
> In the latter case, contextDestroyed() is called in Document::shutdown (which
is
> called in FrameLoader::prepareForCommit), but frameDestroyed() is not called
> until LocalDOMWindow::reset is called for the window. If we simply replace
> DOMWindowProperty with ContextLifecycleObserver, we end up with clearing
frame()
> a bit earlier. It looks like that this matters for a couple of objects, and
> ScreenOrientation is one of them.
> 
> This is why I had to add line 85 - 88 to ScreenOrientationControllerImpl.cpp.
> 
> (In any case, I'll study a bit more -- I need to understand what can happen
> between FrameLoader::prepareForCommit and LocalDOMWindow::reset.)

I've fixed the "subtle" issue in https://codereview.chromium.org/2580753004/.
Landing this CL.

[haraken, 2016-12-19 05:33:21]

The CQ bit was checked by haraken@chromium.org

[haraken, 2016-12-19 05:33:21]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com,
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2568913003/#ps120001
(title: "temp")

[commit-bot: I haz the power, 2016-12-19 05:33:31]

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, 2016-12-19 07:02:14]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1482125601113750,
"parent_rev": "7cd80a17a8a44c11e25f360a2459d13276e0e261", "commit_rev":
"6d6404c18a24a73a6e2fe7b68a8f170b25bf3178"}

[commit-bot: I haz the power, 2016-12-19 07:02:44]

Description was changed from

==========
Remove DOMWindowProperty from ScreenOrientationCoontroller

We're replacing DOMWindowProperty with ContextLifecycleObserver.
This CL does the replacement for ScreenOrientationCoontroller.

BUG=610176
==========

to

==========
Remove DOMWindowProperty from ScreenOrientationCoontroller

We're replacing DOMWindowProperty with ContextLifecycleObserver.
This CL does the replacement for ScreenOrientationCoontroller.

BUG=610176

Review-Url: https://codereview.chromium.org/2568913003
==========

[commit-bot: I haz the power, 2016-12-19 07:02:45]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-12-19 07:04:32]

Description was changed from

==========
Remove DOMWindowProperty from ScreenOrientationCoontroller

We're replacing DOMWindowProperty with ContextLifecycleObserver.
This CL does the replacement for ScreenOrientationCoontroller.

BUG=610176

Review-Url: https://codereview.chromium.org/2568913003
==========

to

==========
Remove DOMWindowProperty from ScreenOrientationCoontroller

We're replacing DOMWindowProperty with ContextLifecycleObserver.
This CL does the replacement for ScreenOrientationCoontroller.

BUG=610176

Committed: https://crrev.com/596d1c908dbd53334f9f77162becc6b1b5c8dba8
Cr-Commit-Position: refs/heads/master@{#439421}
==========

[commit-bot: I haz the power, 2016-12-19 07:04:33]

Patchset 7 (id:??) landed as
https://crrev.com/596d1c908dbd53334f9f77162becc6b1b5c8dba8
Cr-Commit-Position: refs/heads/master@{#439421}