Simplify FullscreenElementStack::fullScreenChangeDelayTimerFired()

Simplify FullscreenElementStack::fullScreenChangeDelayTimerFired()

This function was needlessly complicated:

 1. A null pointer is never pushed to the queues outside of this
    function, so by adding a null check for the internal push, the
    initial null handling can be removed.

 2. !node->inDocument() implies !document()->contains(node.get()), so
    the latter can be dropped from the condition.

No observable change is expected from this simplication.

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

[philipj_slow, 2014-07-02 20:51:39]

PTAL

Falling back to the documentElement is still a bit weird IMHO, but removing that
changes observable behavior and fail existing tests.

[philipj_slow, 2014-07-02 21:03:37]

(Note that falling back to document instead of documentElement doesn't fail any
tests and makes a bit more sense since the document will always be there, but
it's probably not worth messing with.)

[falken, 2014-07-03 01:51:42]

Looks like a great simplification, I have a question about the "blown away"
comment.

https://codereview.chromium.org/365993002/diff/1/Source/core/dom/FullscreenEl...
File Source/core/dom/FullscreenElementStack.cpp (left):

https://codereview.chromium.org/365993002/diff/1/Source/core/dom/FullscreenEl...
Source/core/dom/FullscreenElementStack.cpp:501: // The dispatchEvent below may
have blown away our documentElement.
Hmm, why is this no longer possible? Can we add documentElement to the queue,
then call dispatchEvent and it gets removed/deleted/invalidated? Or is the idea
that adding it to the changeQueue protects it by a RefPtr so it won't be blown
away?

https://codereview.chromium.org/365993002/diff/1/Source/core/dom/FullscreenEl...
Source/core/dom/FullscreenElementStack.cpp:507: if
(!document()->contains(node.get()) && !node->inDocument())
nice catch!

[philipj_slow, 2014-07-03 13:53:00]

https://codereview.chromium.org/365993002/diff/1/Source/core/dom/FullscreenEl...
File Source/core/dom/FullscreenElementStack.cpp (left):

https://codereview.chromium.org/365993002/diff/1/Source/core/dom/FullscreenEl...
Source/core/dom/FullscreenElementStack.cpp:501: // The dispatchEvent below may
have blown away our documentElement.
On 2014/07/03 01:51:42, falken wrote:
> Hmm, why is this no longer possible? Can we add documentElement to the queue,
> then call dispatchEvent and it gets removed/deleted/invalidated? Or is the
idea
> that adding it to the changeQueue protects it by a RefPtr so it won't be blown
> away?

Since we never push null to the queue and the queue entries are refcounted, we
won't ever pop null from the queue, so neither of these if (!node) branches will
ever be taken.

However, it is possible that the documentElement is removed from the document in
the first dispatchEvent, and if a new documentElement was inserted, that one
would be added to the queue as well. This could continue in a loop forever, but
AFAICT that was already the case.

[falken, 2014-07-03 14:48:38]

https://codereview.chromium.org/365993002/diff/1/Source/core/dom/FullscreenEl...
File Source/core/dom/FullscreenElementStack.cpp (left):

https://codereview.chromium.org/365993002/diff/1/Source/core/dom/FullscreenEl...
Source/core/dom/FullscreenElementStack.cpp:501: // The dispatchEvent below may
have blown away our documentElement.
On 2014/07/03 13:53:00, philipj wrote:
> On 2014/07/03 01:51:42, falken wrote:
> > Hmm, why is this no longer possible? Can we add documentElement to the
queue,
> > then call dispatchEvent and it gets removed/deleted/invalidated? Or is the
> idea
> > that adding it to the changeQueue protects it by a RefPtr so it won't be
blown
> > away?
> 
> Since we never push null to the queue and the queue entries are refcounted, we
> won't ever pop null from the queue, so neither of these if (!node) branches
will
> ever be taken.
> 
> However, it is possible that the documentElement is removed from the document
in
> the first dispatchEvent, and if a new documentElement was inserted, that one
> would be added to the queue as well. This could continue in a loop forever,
but
> AFAICT that was already the case.

OK I see. lgtm

[philipj_slow, 2014-07-03 19:23:45]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-07-03 19:24:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/philipj@opera.com/365993002/1

[commit-bot: I haz the power, 2014-07-03 19:27:08]

Change committed as 177489