PlzNavigate: update navigation only if it took place.

PlzNavigate: update navigation only if it took place.

This CL prevents timing attributes to be set if a navigation has not
navigated yet. There is such a navigation, because the FrameLoader
create at least 2 DocumentLoader :
- init()      -> A fake navigation to an empty url.
- startLoad() -> The real navigation.

TEST=TracingBrowserTest.TestBackgroundMemoryInfra
     with --enable-browser-side-navigation

BUG=576261
Committed: https://crrev.com/a29a332cf32f52fc4936029e120b67a1cbaa10d6
Cr-Commit-Position: refs/heads/master@{#421160}

[arthursonzogni, 2016-09-23 13:17:34]

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

[commit-bot: I haz the power, 2016-09-23 13:17:50]

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-09-23 13:34:53]

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

[commit-bot: I haz the power, 2016-09-23 13:34:54]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[arthursonzogni, 2016-09-23 14:46:34]

arthursonzogni@chromium.org changed reviewers:
+ clamy@chromium.org, japhet@chromium.org

[arthursonzogni, 2016-09-23 14:46:35]

Hi Camille and Nate,

This is a small bugfix that ensures that navigation timings are updated if and
only if a real navigation took place.
Please, could you take a look?

clamy:
  content/renderer/render_frame_impl.cc
japhet:
  third_party/WebKit/Source/web/WebDataSourceImpl.cpp


Thanks!

note: Windows buildbot seems to be broken actually, that's why the dry run fails
partially for the moment.

[clamy, 2016-09-23 15:35:39]

Thanks! A few comments below.

https://codereview.chromium.org/2359723007/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2359723007/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:1: #include <iostream>
Wrong place for include. Please put it just above #include <map> on line 8.

https://codereview.chromium.org/2359723007/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:3188: if (!timing.fetch_start.is_null()) {
I would find it a bit more readable if you just did:
// PlzNavigate: if an actual navigation took place, inform the datasource of
what happened in the browser.
if (IsBrowserSideNavigationEnabled() && !timing.fetch_start.is_null()) {
...
}

(Note: I don't see a particular reason to modify what's inside - it makes the
change harder to read).

https://codereview.chromium.org/2359723007/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebDataSourceImpl.cpp (right):

https://codereview.chromium.org/2359723007/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebDataSourceImpl.cpp:80: if (redirectChain.size()
>= 2) {
This does not seem related to the other issue and the CL description. Please put
it in another CL if this relates to another issue, or remove it if it is no
longer needed.

[arthursonzogni, 2016-09-23 16:44:51]

Patchset #2 (id:20001) has been deleted

[arthursonzogni, 2016-09-23 16:52:08]

Patchset #2 (id:40001) has been deleted

[arthursonzogni, 2016-09-23 17:08:41]

Patchset #2 (id:60001) has been deleted

[arthursonzogni, 2016-09-26 08:02:24]

Patchset #2 (id:80001) has been deleted

[arthursonzogni, 2016-09-26 08:04:05]

Description was changed from

==========
PlzNavigate: fix remaining TracingBrowserTest.*

This CL prevents timing attributes to be set if a navigation has not
navigated yet. There is such a navigation, because the FrameLoader
create at least 2 DocumentLoader :
- init()      -> A fake navigation to an empty url.
- startLoad() -> The real navigation.

TEST=TracingBrowserTest.TestBackgroundMemoryInfra
     TracingBrowserTest.TestMemoryInfra
     with --enable-browser-side-navigation

BUG=576261
==========

to

==========
PlzNavigate: update navigation only if it took place.

This CL prevents timing attributes to be set if a navigation has not
navigated yet. There is such a navigation, because the FrameLoader
create at least 2 DocumentLoader :
- init()      -> A fake navigation to an empty url.
- startLoad() -> The real navigation.

TEST=TracingBrowserTest.TestBackgroundMemoryInfra
     with --enable-browser-side-navigation

BUG=576261
==========

[arthursonzogni, 2016-09-26 08:15:57]

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

[commit-bot: I haz the power, 2016-09-26 08:16:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[arthursonzogni, 2016-09-26 08:30:31]

arthursonzogni@chromium.org changed reviewers:
- japhet@chromium.org

[arthursonzogni, 2016-09-26 08:30:32]

Thanks Camille.

I broke this patch into two parts.
There is now only one reviewer for each patch, Camille on this one and Nate on
the other.

https://codereview.chromium.org/2359723007/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2359723007/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:1: #include <iostream>
On 2016/09/23 15:35:38, clamy wrote:
> Wrong place for include. Please put it just above #include <map> on line 8.

Oops, I didn't have seen I left this debug-line here.

https://codereview.chromium.org/2359723007/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:3188: if (!timing.fetch_start.is_null()) {
On 2016/09/23 15:35:38, clamy wrote:
> I would find it a bit more readable if you just did:
> // PlzNavigate: if an actual navigation took place, inform the datasource of
> what happened in the browser.
> if (IsBrowserSideNavigationEnabled() && !timing.fetch_start.is_null()) {
> ...
> }
> 
> (Note: I don't see a particular reason to modify what's inside - it makes the
> change harder to read).

Done.

https://codereview.chromium.org/2359723007/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebDataSourceImpl.cpp (right):

https://codereview.chromium.org/2359723007/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebDataSourceImpl.cpp:80: if (redirectChain.size()
>= 2) {
On 2016/09/23 15:35:38, clamy wrote:
> This does not seem related to the other issue and the CL description. Please
put
> it in another CL if this relates to another issue, or remove it if it is no
> longer needed.

There are related because they cause the same assertion violation.
Without it, only one of TracingBrowserTest still pass and the renderer crash.

Nevertheless, I think it may be better to do two different patchs.

[commit-bot: I haz the power, 2016-09-26 10:47:44]

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

[commit-bot: I haz the power, 2016-09-26 10:47:45]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[clamy, 2016-09-26 10:57:00]

Thanks! Lgtm

[arthursonzogni, 2016-09-27 09:03:35]

The CQ bit was checked by arthursonzogni@chromium.org

[commit-bot: I haz the power, 2016-09-27 09:04:00]

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-09-27 10:02:29]

Description was changed from

==========
PlzNavigate: update navigation only if it took place.

This CL prevents timing attributes to be set if a navigation has not
navigated yet. There is such a navigation, because the FrameLoader
create at least 2 DocumentLoader :
- init()      -> A fake navigation to an empty url.
- startLoad() -> The real navigation.

TEST=TracingBrowserTest.TestBackgroundMemoryInfra
     with --enable-browser-side-navigation

BUG=576261
==========

to

==========
PlzNavigate: update navigation only if it took place.

This CL prevents timing attributes to be set if a navigation has not
navigated yet. There is such a navigation, because the FrameLoader
create at least 2 DocumentLoader :
- init()      -> A fake navigation to an empty url.
- startLoad() -> The real navigation.

TEST=TracingBrowserTest.TestBackgroundMemoryInfra
     with --enable-browser-side-navigation

BUG=576261
==========

[commit-bot: I haz the power, 2016-09-27 10:02:30]

Committed patchset #2 (id:100001)

[commit-bot: I haz the power, 2016-09-27 10:04:05]

Description was changed from

==========
PlzNavigate: update navigation only if it took place.

This CL prevents timing attributes to be set if a navigation has not
navigated yet. There is such a navigation, because the FrameLoader
create at least 2 DocumentLoader :
- init()      -> A fake navigation to an empty url.
- startLoad() -> The real navigation.

TEST=TracingBrowserTest.TestBackgroundMemoryInfra
     with --enable-browser-side-navigation

BUG=576261
==========

to

==========
PlzNavigate: update navigation only if it took place.

This CL prevents timing attributes to be set if a navigation has not
navigated yet. There is such a navigation, because the FrameLoader
create at least 2 DocumentLoader :
- init()      -> A fake navigation to an empty url.
- startLoad() -> The real navigation.

TEST=TracingBrowserTest.TestBackgroundMemoryInfra
     with --enable-browser-side-navigation

BUG=576261

Committed: https://crrev.com/a29a332cf32f52fc4936029e120b67a1cbaa10d6
Cr-Commit-Position: refs/heads/master@{#421160}
==========

[commit-bot: I haz the power, 2016-09-27 10:04:06]

Patchset 2 (id:??) landed as
https://crrev.com/a29a332cf32f52fc4936029e120b67a1cbaa10d6
Cr-Commit-Position: refs/heads/master@{#421160}

[jam, 2016-09-27 19:24:23]

TracingBrowserTest.TestMemoryInfra is hanging on some tryjobs, not sure why?
i.e. see
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

[arthursonzogni, 2016-09-28 07:47:45]

Sorry, I think it is my fault, I will disable the test until the second part of
this patch is landed:
https://codereview.chromium.org/2368883003/