Adjust ScreenOrientation unit test to handle changed detachment timing.

Adjust ScreenOrientation unit test to handle changed detachment timing.

The observer notification of frame detachment is being changed via
http://crrev.com/881683003. Adjust a ScreenOrientationBrowserTest
test to also expect this new behavior.

R=mlamouri
BUG=446452
Committed: https://crrev.com/219ad0dfedd72afac601871dfc1ea1626982e2d7
Cr-Commit-Position: refs/heads/master@{#315564}

[sof, 2015-02-10 07:36:39]

sigbjornf@opera.com changed reviewers:
+ jonross@chromium.org, mlamouri@chromium.org

[sof, 2015-02-10 07:36:40]

Please take a look.

(If there are others even better placed to look this over, please let me know.
tia.)

[mlamouri (slow - plz ping), 2015-02-10 11:32:05]

On 2015/02/10 at 07:36:40, sigbjornf wrote:
> Please take a look.
> 
> (If there are others even better placed to look this over, please let me know.
tia.)

I'm not sure I understand the intent. If I understand correctly
https://codereview.chromium.org/881683003 was reverted because of a crash in
that test. How is that fixing the crash?

[sof, 2015-02-10 11:36:17]

On 2015/02/10 11:32:05, Mounir Lamouri wrote:
> On 2015/02/10 at 07:36:40, sigbjornf wrote:
> > Please take a look.
> > 
> > (If there are others even better placed to look this over, please let me
know.
> tia.)
> 
> I'm not sure I understand the intent. If I understand correctly
> https://codereview.chromium.org/881683003 was reverted because of a crash in
> that test. How is that fixing the crash?

It was asserting, yes. That has now been addressed wrt
https://code.google.com/p/chromium/issues/detail?id=455161#c17

[mlamouri (slow - plz ping), 2015-02-10 12:33:23]

On 2015/02/10 at 11:36:17, sigbjornf wrote:
> On 2015/02/10 11:32:05, Mounir Lamouri wrote:
> > On 2015/02/10 at 07:36:40, sigbjornf wrote:
> > > Please take a look.
> > > 
> > > (If there are others even better placed to look this over, please let me
know.
> > tia.)
> > 
> > I'm not sure I understand the intent. If I understand correctly
> > https://codereview.chromium.org/881683003 was reverted because of a crash in
> > that test. How is that fixing the crash?
> 
> It was asserting, yes. That has now been addressed wrt
https://code.google.com/p/chromium/issues/detail?id=455161#c17

Do you mean that you need that CL to prevent the crash? Isn't the fact that this
code would crash a bug somewhere else?

[sof, 2015-02-10 12:36:19]

On 2015/02/10 12:33:23, Mounir Lamouri wrote:
> On 2015/02/10 at 11:36:17, sigbjornf wrote:
> > On 2015/02/10 11:32:05, Mounir Lamouri wrote:
> > > On 2015/02/10 at 07:36:40, sigbjornf wrote:
> > > > Please take a look.
> > > > 
> > > > (If there are others even better placed to look this over, please let me
> know.
> > > tia.)
> > > 
> > > I'm not sure I understand the intent. If I understand correctly
> > > https://codereview.chromium.org/881683003 was reverted because of a crash
in
> > > that test. How is that fixing the crash?
> > 
> > It was asserting, yes. That has now been addressed wrt
> https://code.google.com/p/chromium/issues/detail?id=455161#c17
> 
> Do you mean that you need that CL to prevent the crash? Isn't the fact that
this
> code would crash a bug somewhere else?

With that CL on board, this test _as is_ will throw an exception & fail as
screen.orientation returns null. Leading to a timeout. Hence the change is
required to accommodate both the behavior seen on trunk _and_ what it will be
once that CL lands.

[mlamouri (slow - plz ping), 2015-02-10 13:37:01]

On 2015/02/10 at 12:36:19, sigbjornf wrote:
> On 2015/02/10 12:33:23, Mounir Lamouri wrote:
> > On 2015/02/10 at 11:36:17, sigbjornf wrote:
> > > On 2015/02/10 11:32:05, Mounir Lamouri wrote:
> > > > On 2015/02/10 at 07:36:40, sigbjornf wrote:
> > > > > Please take a look.
> > > > > 
> > > > > (If there are others even better placed to look this over, please let
me
> > know.
> > > > tia.)
> > > > 
> > > > I'm not sure I understand the intent. If I understand correctly
> > > > https://codereview.chromium.org/881683003 was reverted because of a
crash in
> > > > that test. How is that fixing the crash?
> > > 
> > > It was asserting, yes. That has now been addressed wrt
> > https://code.google.com/p/chromium/issues/detail?id=455161#c17
> > 
> > Do you mean that you need that CL to prevent the crash? Isn't the fact that
this
> > code would crash a bug somewhere else?
> 
> With that CL on board, this test _as is_ will throw an exception & fail as
screen.orientation returns null. Leading to a timeout. Hence the change is
required to accommodate both the behavior seen on trunk _and_ what it will be
once that CL lands.

Ok. I guess that's where the misunderstanding was: it wasn't clear that the test
was failing because of a timeout. I looked at the results but not carefully
enough and I assume the stack was a crash. Thanks for the explanations.

lgtm

[sof, 2015-02-10 14:20:49]

On 2015/02/10 13:37:01, Mounir Lamouri wrote:
> On 2015/02/10 at 12:36:19, sigbjornf wrote:
> > On 2015/02/10 12:33:23, Mounir Lamouri wrote:
> > > On 2015/02/10 at 11:36:17, sigbjornf wrote:
> > > > On 2015/02/10 11:32:05, Mounir Lamouri wrote:
> > > > > On 2015/02/10 at 07:36:40, sigbjornf wrote:
> > > > > > Please take a look.
> > > > > > 
> > > > > > (If there are others even better placed to look this over, please
let
> me
> > > know.
> > > > > tia.)
> > > > > 
> > > > > I'm not sure I understand the intent. If I understand correctly
> > > > > https://codereview.chromium.org/881683003 was reverted because of a
> crash in
> > > > > that test. How is that fixing the crash?
> > > > 
> > > > It was asserting, yes. That has now been addressed wrt
> > > https://code.google.com/p/chromium/issues/detail?id=455161#c17
> > > 
> > > Do you mean that you need that CL to prevent the crash? Isn't the fact
that
> this
> > > code would crash a bug somewhere else?
> > 
> > With that CL on board, this test _as is_ will throw an exception & fail as
> screen.orientation returns null. Leading to a timeout. Hence the change is
> required to accommodate both the behavior seen on trunk _and_ what it will be
> once that CL lands.
> 
> Ok. I guess that's where the misunderstanding was: it wasn't clear that the
test
> was failing because of a timeout. I looked at the results but not carefully
> enough and I assume the stack was a crash. Thanks for the explanations.
> 
> lgtm

Thanks; yes, quite so - the unit test dumps a stack trace when it is interrupted
due to timer expiry, which gave me a fright also.

[sof, 2015-02-10 14:21:15]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-02-10 14:21:51]

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

[commit-bot: I haz the power, 2015-02-10 15:17:17]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-02-10 15:17:54]

Patchset 1 (id:??) landed as
https://crrev.com/219ad0dfedd72afac601871dfc1ea1626982e2d7
Cr-Commit-Position: refs/heads/master@{#315564}