Add Prerender type to NavigationType for nav timing 2

Add Prerender type to NavigationType for nav timing 2

BUG=663217
Committed: https://crrev.com/a954beaadf5624ddf648b36cdf19561984d57c02
Cr-Commit-Position: refs/heads/master@{#435521}

[sunjian, 2016-11-16 23:07:09]

sunjian@chromium.org changed reviewers:
+ panicker@google.com

[panicker, 2016-11-17 22:23:57]

Description was changed from

==========
Add Prerender type to NavigationType for nav timing 2

BUG=663217
==========

to

==========
Add Prerender type to NavigationType for nav timing 2

BUG=663217
==========

[panicker, 2016-11-17 22:23:58]

panicker@chromium.org changed reviewers:
+ panicker@chromium.org
- panicker@google.com

[sunjian, 2016-11-18 20:34:56]

sunjian@chromium.org changed reviewers:
+ yoav@yoav.ws

[panicker, 2016-11-18 21:01:06]

lgtm

https://codereview.chromium.org/2509143002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right):

https://codereview.chromium.org/2509143002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:54: namespace {
nit: add blank line

[sunjian, 2016-11-18 21:17:58]

https://codereview.chromium.org/2509143002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right):

https://codereview.chromium.org/2509143002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:54: namespace {
On 2016/11/18 21:01:05, Shubhie wrote:
> nit: add blank line 

Done.

[panicker, 2016-11-22 02:19:43]

panicker@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2016-11-22 03:05:22]

https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right):

https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:67: default:
Prefer not using default but listing up actual enum names and have NOTREACHED
outside the switch (then we can be warned for non-used enum types when we add
any)

https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:71: }
nit: }  // namespace

https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h
(right):

https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:23:
KTypePrerender
'PerformanceNavigationType' sounds a little odd (we have 'navigation timing' in
performance API but don't have terms like 'performance navigation'), and we're
already inside the PerformanceNavigationTiming class, maybe the enum name could
be a little shorter

enum type names look weird, remove the leading K (like other enums in blink) or
use lower k (a la
https://google.github.io/styleguide/cppguide.html#Enumerator_Names)

Also let's use enum class so that we can use shorter enum names.  So it'd result
something like

enum class NavigationType {
  Navigate,
  Reload,
  BackForward,
  Prerender
};

[Yoav Weiss, 2016-11-22 10:46:32]

https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right):

https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:54: namespace {
Is there a reason you picked an extra namespace rather than making the function
static?

https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:62: switch (type) {
What's the reason for translating enums from NavigationType to
PerformanceNavigationType rather than adding prerender to NavigationType?

https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h
(right):

https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:24: };
A nearly identical enum is already defined (but not really used anywhere AFAICT)
in PerformanceNavigation.h
Can we at least get rid of one of them?

Also, is the "K" prefix part of the new style guide? Not an often used
convention in Blink code

[panicker, 2016-11-22 18:31:28]

https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right):

https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:54: namespace {
On 2016/11/22 10:46:32, Yoav Weiss wrote:
> Is there a reason you picked an extra namespace rather than making the
function
> static?

you mean a global static here? I believe anonymous namespace is considered a
superior alternative to static global, and is more versatile (works for types in
addition to functions, consistent way of controlling visibility etc.)

https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:62: switch (type) {
On 2016/11/22 10:46:32, Yoav Weiss wrote:
> What's the reason for translating enums from NavigationType to
> PerformanceNavigationType rather than adding prerender to NavigationType?

These are both defined in separate specs for NT1 and NT2 respectively:
https://www.w3.org/TR/navigation-timing/#performancenavigation
https://www.w3.org/TR/navigation-timing-2/#idl-def-navigationtype
So it seemed a bit weird to merge them, but you are likely more aware of
handling spec versions - WDYT?

[sunjian, 2016-11-22 22:04:42]

https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right):

https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:54: namespace {
On 2016/11/22 18:31:28, Shubhie wrote:
> On 2016/11/22 10:46:32, Yoav Weiss wrote:
> > Is there a reason you picked an extra namespace rather than making the
> function
> > static?
> 
> you mean a global static here? I believe anonymous namespace is considered a
> superior alternative to static global, and is more versatile (works for types
in
> addition to functions, consistent way of controlling visibility etc.)
> 

Acknowledged.

https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:62: switch (type) {
On 2016/11/22 10:46:32, Yoav Weiss wrote:
> What's the reason for translating enums from NavigationType to
> PerformanceNavigationType rather than adding prerender to NavigationType?

Agree. So i could have added prerender type at a lower level. Then it involves
handling this new type at other places that i am familiar with and confident
about. A few pointers:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr...
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebDataSou....

https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:67: default:
On 2016/11/22 03:05:22, kinuko wrote:
> Prefer not using default but listing up actual enum names and have NOTREACHED
> outside the switch (then we can be warned for non-used enum types when we add
> any)

Done.

https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:71: }
On 2016/11/22 03:05:22, kinuko wrote:
> nit: }  // namespace

Done.

https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h
(right):

https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:23:
KTypePrerender
On 2016/11/22 03:05:22, kinuko wrote:
> 'PerformanceNavigationType' sounds a little odd (we have 'navigation timing'
in
> performance API but don't have terms like 'performance navigation'), and we're
> already inside the PerformanceNavigationTiming class, maybe the enum name
could
> be a little shorter
> 
> enum type names look weird, remove the leading K (like other enums in blink)
or
> use lower k (a la
> https://google.github.io/styleguide/cppguide.html#Enumerator_Names)
> 
> Also let's use enum class so that we can use shorter enum names.  So it'd
result
> something like
> 
> enum class NavigationType {
>   Navigate,
>   Reload,
>   BackForward,
>   Prerender
> };

Done.

https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:24: };
On 2016/11/22 10:46:32, Yoav Weiss wrote:
> A nearly identical enum is already defined (but not really used anywhere
AFAICT)
> in PerformanceNavigation.h
> Can we at least get rid of one of them?
> 
> Also, is the "K" prefix part of the new style guide? Not an often used
> convention in Blink code

Agree. That's why i named this new enum differently than the existing one
and borrowed the old one's definition a little bit. I am not sure whether we
should
create a new one with the prerender type and then replace the old one with the
new one.
It sounds pretty messy to me cause nav timing 1 apparently doesn't have a
prerender type.

[panicker, 2016-11-22 22:20:50]

https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right):

https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:62: switch (type) {
On 2016/11/22 22:04:42, sunjian wrote:
> On 2016/11/22 10:46:32, Yoav Weiss wrote:
> > What's the reason for translating enums from NavigationType to
> > PerformanceNavigationType rather than adding prerender to NavigationType?
> 
> Agree. So i could have added prerender type at a lower level. Then it involves
> handling this new type at other places that i am familiar with and confident
> about. A few pointers:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr...
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebDataSou....

I may have misunderstood, Yoav I think you were referring to 
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr...
q=enum&l=46
as opposed to:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/timing/Pe...

[Yoav Weiss, 2016-11-23 06:56:59]

On 2016/11/22 22:20:50, Shubhie wrote:
>
https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right):
> 
>
https://codereview.chromium.org/2509143002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/timing/PerformanceBase.cpp:62: switch (type) {
> On 2016/11/22 22:04:42, sunjian wrote:
> > On 2016/11/22 10:46:32, Yoav Weiss wrote:
> > > What's the reason for translating enums from NavigationType to
> > > PerformanceNavigationType rather than adding prerender to NavigationType?
> > 
> > Agree. So i could have added prerender type at a lower level. Then it
involves
> > handling this new type at other places that i am familiar with and confident
> > about. A few pointers:
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr...
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebDataSou....
> 
> I may have misunderstood, Yoav I think you were referring to 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr...
> q=enum&l=46
> as opposed to:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/timing/Pe...

I was referring to
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/timing/Pe...
but now see that this enum is needed for the idl bindings of NT1.

[Yoav Weiss, 2016-11-23 07:00:39]

Code changes look good. Can you add tests as well?

In general (unrelated to this CL), I think it'd be a good idea to rename the old
NT1 implementation classes to reflect the fact that they are legacy support.

[Yoav Weiss, 2016-11-23 07:09:07]

On 2016/11/23 07:00:39, Yoav Weiss wrote:
> In general (unrelated to this CL), I think it'd be a good idea to rename the
old
> NT1 implementation classes to reflect the fact that they are legacy support.

Perhaps comments would suffice, as we don't want to file names to diverge from
the IDL class names.

[sunjian, 2016-11-23 22:42:10]

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

[commit-bot: I haz the power, 2016-11-23 22:43:10]

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-11-23 23:07:07]

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

[commit-bot: I haz the power, 2016-11-23 23:07:08]

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...)

[sunjian, 2016-11-28 23:04:08]

Added unit test.

[Yoav Weiss, 2016-11-29 06:45:26]

On 2016/11/29 00:36:23, sunjian wrote:

LGTM

[sunjian, 2016-11-29 19:06:12]

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

[commit-bot: I haz the power, 2016-11-29 19:06:36]

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-11-29 19:25:48]

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

[commit-bot: I haz the power, 2016-11-29 19:25:49]

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

[kinuko, 2016-11-30 05:29:29]

lgtm

https://codereview.chromium.org/2509143002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp
(right):

https://codereview.chromium.org/2509143002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:134:
NOTREACHED();
nit- still need to return something here to get it compiling

[sunjian, 2016-11-30 19:15:47]

https://codereview.chromium.org/2509143002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp
(right):

https://codereview.chromium.org/2509143002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:134:
NOTREACHED();
On 2016/11/30 05:29:29, kinuko wrote:
> nit- still need to return something here to get it compiling

Done.

[sunjian, 2016-11-30 19:15:55]

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

[commit-bot: I haz the power, 2016-11-30 19:16:22]

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-11-30 20:43:31]

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

[commit-bot: I haz the power, 2016-11-30 20:43:32]

Dry run: This issue passed the CQ dry run.

[sunjian, 2016-11-30 23:19:23]

The CQ bit was checked by sunjian@chromium.org

[sunjian, 2016-11-30 23:19:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
panicker@chromium.org, kinuko@chromium.org, yoav@yoav.ws
Link to the patchset: https://codereview.chromium.org/2509143002/#ps120001
(title: "addressed comments")

[commit-bot: I haz the power, 2016-11-30 23:19:48]

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-11-30 23:23:23]

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

[commit-bot: I haz the power, 2016-11-30 23:23:25]

Failed to apply patch for
third_party/WebKit/Source/core/timing/PerformanceBaseTest.cpp:
While running git apply --index -p1;
  error: patch failed:
third_party/WebKit/Source/core/timing/PerformanceBaseTest.cpp:109
  error: third_party/WebKit/Source/core/timing/PerformanceBaseTest.cpp: patch
does not apply

Patch:       third_party/WebKit/Source/core/timing/PerformanceBaseTest.cpp
Index: third_party/WebKit/Source/core/timing/PerformanceBaseTest.cpp
diff --git a/third_party/WebKit/Source/core/timing/PerformanceBaseTest.cpp
b/third_party/WebKit/Source/core/timing/PerformanceBaseTest.cpp
index
2ea1b664788ccbc49b6f1201a02c665833156a4e..8e427fb96f51fb063a73b5f977b084bea0189803
100644
--- a/third_party/WebKit/Source/core/timing/PerformanceBaseTest.cpp
+++ b/third_party/WebKit/Source/core/timing/PerformanceBaseTest.cpp
@@ -6,6 +6,7 @@
 
 #include "bindings/core/v8/PerformanceObserverCallback.h"
 #include "bindings/core/v8/V8BindingForTesting.h"
+#include "core/testing/DummyPageHolder.h"
 #include "core/timing/PerformanceBase.h"
 #include "core/timing/PerformanceLongTaskTiming.h"
 #include "core/timing/PerformanceObserver.h"
@@ -42,12 +43,23 @@ class PerformanceBaseTest : public ::testing::Test {
     m_observer = PerformanceObserver::create(scriptState, m_base, m_cb);
   }
 
+  void SetUp() override {
+    m_pageHolder = DummyPageHolder::create(IntSize(800, 600));
+  }
+
   int numPerformanceEntriesInObserver() {
     return m_observer->m_performanceEntries.size();
   }
 
+  PerformanceNavigationTiming::NavigationType getNavigationType(
+      NavigationType type,
+      Document* document) {
+    return PerformanceBase::getNavigationType(type, document);
+  }
+
   Persistent<TestPerformanceBase> m_base;
   Persistent<PerformanceObserver> m_observer;
+  std::unique_ptr<DummyPageHolder> m_pageHolder;
   Persistent<PerformanceObserverCallback> m_cb;
 };
 
@@ -109,4 +121,24 @@ TEST_F(PerformanceBaseTest, AddLongTaskTiming) {
   m_base->addLongTaskTiming(1234, 5678, "www.foo.com/bar", nullptr);
   EXPECT_EQ(1, numPerformanceEntriesInObserver());  // added an entry
 }
+
+TEST_F(PerformanceBaseTest, GetNavigationType) {
+  m_pageHolder->page().setVisibilityState(PageVisibilityStatePrerender, false);
+  PerformanceNavigationTiming::NavigationType returnedType =
+      getNavigationType(NavigationTypeBackForward, &m_pageHolder->document());
+  EXPECT_EQ(returnedType,
+            PerformanceNavigationTiming::NavigationType::Prerender);
+
+  m_pageHolder->page().setVisibilityState(PageVisibilityStateHidden, false);
+  returnedType =
+      getNavigationType(NavigationTypeBackForward, &m_pageHolder->document());
+  EXPECT_EQ(returnedType,
+            PerformanceNavigationTiming::NavigationType::BackForward);
+
+  m_pageHolder->page().setVisibilityState(PageVisibilityStateVisible, false);
+  returnedType = getNavigationType(NavigationTypeFormResubmitted,
+                                   &m_pageHolder->document());
+  EXPECT_EQ(returnedType,
+            PerformanceNavigationTiming::NavigationType::Navigate);
+}
 }  // namespace blink

[sunjian, 2016-12-01 00:02:11]

The CQ bit was checked by sunjian@chromium.org

[sunjian, 2016-12-01 00:02:12]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
yoav@yoav.ws, panicker@chromium.org
Link to the patchset: https://codereview.chromium.org/2509143002/#ps140001
(title: "synced client")

[commit-bot: I haz the power, 2016-12-01 00:02:28]

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-12-01 01:37:57]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1480550531554880,
"parent_rev": "dd1d836c267cdc41cc50663ea2fe71f35aae2530", "commit_rev":
"b08725d47ae8b92f7375854f13e8a7f7f7589197"}

[commit-bot: I haz the power, 2016-12-01 01:38:28]

Description was changed from

==========
Add Prerender type to NavigationType for nav timing 2

BUG=663217
==========

to

==========
Add Prerender type to NavigationType for nav timing 2

BUG=663217
==========

[commit-bot: I haz the power, 2016-12-01 01:38:29]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-12-01 01:40:56]

Description was changed from

==========
Add Prerender type to NavigationType for nav timing 2

BUG=663217
==========

to

==========
Add Prerender type to NavigationType for nav timing 2

BUG=663217

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

[commit-bot: I haz the power, 2016-12-01 01:40:57]

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