Migrate PingLoader to be a LocalFrameLifecycleObserver.

Migrate PingLoader to be a LocalFrameLifecycleObserver.

BUG=526743, 522622
TEST=webkit_unit_tests

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

[ncarter (slow), 2015-08-31 23:37:38]

nick@chromium.org changed reviewers:
+ dgozman@chromium.org, japhet@chromium.org

[ncarter (slow), 2015-08-31 23:37:38]

Please review. This definitely fixes the crash (confirmed by a content
browsertest), but I'm not sure if it will negatively impact the devtools tracing
behavior (since there are many cases where frame might live for a shorter time
than the page does).

https://codereview.chromium.org/1311433008/diff/1/Source/core/loader/PingLoad...
File Source/core/loader/PingLoader.cpp (right):

https://codereview.chromium.org/1311433008/diff/1/Source/core/loader/PingLoad...
Source/core/loader/PingLoader.cpp:137: : LocalFrameLifecycleObserver(frame)
Note that we could potentially use frame->localFrameRoot() here, if we wanted to
extend the lifetime as much as possible. That feels like a bit of a hack to me,
but so does the original behavior of tying this to the page lifetime.

[ncarter (slow), 2015-08-31 23:38:11]

nick@chromium.org changed reviewers:
+ dcheng@chromium.org

[Nate Chapin, 2015-08-31 23:50:16]

Code looks reasonable, but the test should really be in blink if at all
possible.

https://codereview.chromium.org/1311433008/diff/1/Source/core/html/HTMLAnchor...
File Source/core/html/HTMLAnchorElement.cpp (right):

https://codereview.chromium.org/1311433008/diff/1/Source/core/html/HTMLAnchor...
Source/core/html/HTMLAnchorElement.cpp:32: #include "core/frame/UseCounter.h"
Why?

[dcheng, 2015-08-31 23:54:18]

https://codereview.chromium.org/1311433008/diff/1/Source/core/loader/PingLoad...
File Source/core/loader/PingLoader.h (right):

https://codereview.chromium.org/1311433008/diff/1/Source/core/loader/PingLoad...
Source/core/loader/PingLoader.h:86: void didReceiveResponse(WebURLLoader*, const
WebURLResponse&) final;
Maybe just mark the class final.

[dgozman, 2015-09-01 00:56:16]

Looks good from devtools side.

[ncarter (slow), 2015-09-01 21:09:47]

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

[commit-bot: I haz the power, 2015-09-01 21:10:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1311433008/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1311433008/20001

[ncarter (slow), 2015-09-01 21:10:56]

I added a webkit_unit_test that hits the crash (which I verified both with and
without the fix).

https://codereview.chromium.org/1311433008/diff/1/Source/core/html/HTMLAnchor...
File Source/core/html/HTMLAnchorElement.cpp (right):

https://codereview.chromium.org/1311433008/diff/1/Source/core/html/HTMLAnchor...
Source/core/html/HTMLAnchorElement.cpp:32: #include "core/frame/UseCounter.h"
On 2015/08/31 23:50:15, Nate Chapin wrote:
> Why?

Without the new #include we'd get a compile error on line 326.

It used to be pulled in via core/loader/PingLoader.h ->
core/page/PageLifecycleObserver.h -> core/page/Page.h -> core/frame/UseCounter.h

Now PingLoader.h no longer pulls in PageLifecycleObserver.h, and UseCounter.h is
not pulled in by any of the other transitive includes of this translation unit,

https://codereview.chromium.org/1311433008/diff/1/Source/core/loader/PingLoad...
File Source/core/loader/PingLoader.h (right):

https://codereview.chromium.org/1311433008/diff/1/Source/core/loader/PingLoad...
Source/core/loader/PingLoader.h:86: void didReceiveResponse(WebURLLoader*, const
WebURLResponse&) final;
On 2015/08/31 23:54:18, dcheng wrote:
> Maybe just mark the class final.

Can't. BeaconLoader derives from PingLoader (which is what motivated me to
protect these methods)

[Nate Chapin, 2015-09-01 21:26:50]

LGTM. Thanks for adding a test!

[ncarter (slow), 2015-09-01 22:15:01]

The CQ bit was unchecked by nick@chromium.org

[ncarter (slow), 2015-09-01 22:15:01]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2015-09-01 22:15:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1311433008/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1311433008/20001

[commit-bot: I haz the power, 2015-09-01 22:57:10]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201580