Merge willEnter/willExitFullScreen into didEnter/didExitFullScreen

Merge willEnter/willExitFullScreen into didEnter/didExitFullScreen

The will/did callback pairs came into being in 2011:
https://bugs.webkit.org/show_bug.cgi?id=70477
https://code.google.com/p/chromium/issues/detail?id=100264

The original problem was that the webkitfullscreenchange was
dispatched too early, but this is not affected since the did*
callbacks are the ones left in use.

The risk is that something done in the will* callbacks is too late if
done moved to the did* callbacks. That amounts to:

 - Saving placeholder style and unwrapping/wrapping the renderer in
   Fullscreen::willEnterFullScreenForElement().

 - The overlayFullscreenVideoEnabled() bits in
   HTMLMediaElement::willStopBeingFullscreenElement() and
   FullscreenController::willExitFullScreen().

Since the only difference between will* and did* is that the new size
is known and events aren't fired synchronously, this seems likely to
be safe.

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

[philipj_slow, 2014-08-25 04:01:26]

PTAL. I'm on my phone and didn't finish the commit message, but the two links
explain how the code ended up like this. I've merged more than strictly
necessary, to implement the APEC what is important is that FullscreenController
only calls back to Fullscreen once, since any request callback could fail and
reversing the effects of all the will* would be tricky.

[falken, 2014-08-25 05:07:54]

lgtm. if it doesn't break the layout tests introduced by the bugs listed in the
CL description, looks good.

> I've merged more than strictly necessary, to implement the APEC

Sorry, what is APEC?

https://codereview.chromium.org/497723002/diff/1/Source/core/dom/Fullscreen.cpp
File Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/497723002/diff/1/Source/core/dom/Fullscreen.c...
Source/core/dom/Fullscreen.cpp:448: void
Fullscreen::didExitFullScreenForElement(Element*)
Should we remove the unused parameter?

[philipj_slow, 2014-08-25 08:06:23]

On 2014/08/25 05:07:54, falken wrote:
> lgtm. if it doesn't break the layout tests introduced by the bugs listed in
the
> CL description, looks good.
> 
> > I've merged more than strictly necessary, to implement the APEC
> 
> Sorry, what is APEC?

Oops, my phone autocorrected "spec"

[philipj_slow, 2014-08-25 08:06:30]

https://codereview.chromium.org/497723002/diff/1/Source/core/dom/Fullscreen.cpp
File Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/497723002/diff/1/Source/core/dom/Fullscreen.c...
Source/core/dom/Fullscreen.cpp:448: void
Fullscreen::didExitFullScreenForElement(Element*)
On 2014/08/25 05:07:54, falken wrote:
> Should we remove the unused parameter?

Definitely, but I thought it might be cleaner to do it in a CL that moves all
knowledge about pending fullscreen elements from FullscreenController to
Fullscreen. (Need to do that because there can be several pending requests.)

[philipj_slow, 2014-08-25 08:58:26]

esprehn, mind giving a second opinion? I've updated the description to point out
where things go wrong.

I tested the result in a Chrome debug build and YouTube's fullscreen worked.

I'm a bit mystified why the change doesn't break anything, looking back at 2011
I don't see why the same thing wouldn't have worked then, and yet two callbacks
were added.

[philipj_slow, 2014-08-25 09:07:41]

I wrote a bit in https://code.google.com/p/chromium/issues/detail?id=407002
about the motivation as well.

[philipj_slow, 2014-08-26 21:20:02]

Ping esprehn.

[philipj_slow, 2014-09-02 08:24:39]

philipj@opera.com changed reviewers:
+ darin@chromium.org

[philipj_slow, 2014-09-02 08:24:40]

I asked Darin Fisher, who wrote the original patch, to take a look and he said
"If will* methods are no longer needed, then feel free to remove them."

With that, I'll try landing this.

[philipj_slow, 2014-09-02 08:24:43]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-09-02 08:25:28]

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

[commit-bot: I haz the power, 2014-09-02 08:33:45]

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

[philipj_slow, 2014-09-02 08:37:38]

philipj@opera.com changed reviewers:
+ tkent@chromium.org

[philipj_slow, 2014-09-02 08:37:39]

tkent, can you please review Source/web?

[commit-bot: I haz the power, 2014-09-02 08:37:40]

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

[commit-bot: I haz the power, 2014-09-02 08:37:41]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[tkent, 2014-09-02 08:47:12]

The CQ bit was checked by tkent@chromium.org

[tkent, 2014-09-02 08:47:13]

lgtm

[commit-bot: I haz the power, 2014-09-02 08:47:36]

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

[commit-bot: I haz the power, 2014-09-02 08:52:49]

Committed patchset #1 (id:1) as 181218