Surface pushState event

Surface pushState event

BUG=685729
Review-Url: https://codereview.chromium.org/2652213005
Cr-Commit-Position: refs/heads/master@{#446755}
Committed: https://chromium.googlesource.com/chromium/src/+/26ddde696e8a0076fed1b3f402d5f20009be16b2

[sunjian, 2017-01-26 19:49:19]

Description was changed from

==========
surface pushState event

BUG=685729
==========

to

==========
Surface pushState event

BUG=685729
==========

[sunjian, 2017-01-26 19:50:59]

sunjian@chromium.org changed reviewers:
+ panicker@chromium.org, tdresser@chromium.org

[sunjian, 2017-01-26 19:50:59]

Surfaced History::pushState. Manually tested it locally.

[tdresser, 2017-01-26 20:01:53]

LGTM

https://codereview.chromium.org/2652213005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/History.h (right):

https://codereview.chromium.org/2652213005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/History.h:67:
TRACE_EVENT0("spa:navigation", "History::pushState");
I think we can just use "blink" as the category.
Let's move this method to the cpp.

[sunjian, 2017-01-26 20:21:53]

Patchset #2 (id:20001) has been deleted

[sunjian, 2017-01-26 20:32:52]

Comments addressed.

[tdresser, 2017-01-26 21:30:52]

https://codereview.chromium.org/2652213005/diff/30001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/History.h (right):

https://codereview.chromium.org/2652213005/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/History.h:62: void
pushState(PassRefPtr<SerializedScriptValue>,
We do need parameter names here:

https://www.chromium.org/blink/coding-style

Leave meaningless variable names out of function declarations. A good rule of
thumb is if the parameter type name contains the parameter name (without
trailing numbers or pluralization), then the parameter name isn't needed.
Usually, there should be a parameter name for bools, strings, and numerical
types.

[sunjian, 2017-01-26 22:50:08]

https://codereview.chromium.org/2652213005/diff/30001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/History.h (right):

https://codereview.chromium.org/2652213005/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/History.h:62: void
pushState(PassRefPtr<SerializedScriptValue>,
On 2017/01/26 21:30:52, tdresser wrote:
> We do need parameter names here:
> 
> https://www.chromium.org/blink/coding-style
> 
> Leave meaningless variable names out of function declarations. A good rule of
> thumb is if the parameter type name contains the parameter name (without
> trailing numbers or pluralization), then the parameter name isn't needed.
> Usually, there should be a parameter name for bools, strings, and numerical
> types.

Done.

[tdresser, 2017-01-27 13:44:34]

Thanks, still LGTM.

Shubie, want to take a look?

[panicker, 2017-01-27 17:05:32]

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

[panicker, 2017-01-27 17:05:33]

LGTM
Steve could you review for owners ?

[skobes, 2017-01-27 17:58:08]

lgtm

[sunjian, 2017-01-27 18:22:06]

The CQ bit was checked by sunjian@chromium.org

[commit-bot: I haz the power, 2017-01-27 18:22:36]

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, 2017-01-27 20:39:50]

CQ is committing da patch.

Bot data: {"patchset_id": 50001, "attempt_start_ts": 1485541326169620,
"parent_rev": "25a6b95c34d5318dbb2cf46545a6f0bf07d2a6ef", "commit_rev":
"26ddde696e8a0076fed1b3f402d5f20009be16b2"}

[commit-bot: I haz the power, 2017-01-27 20:40:27]

Description was changed from

==========
Surface pushState event

BUG=685729
==========

to

==========
Surface pushState event

BUG=685729

Review-Url: https://codereview.chromium.org/2652213005
Cr-Commit-Position: refs/heads/master@{#446755}
Committed:
https://chromium.googlesource.com/chromium/src/+/26ddde696e8a0076fed1b3f402d5...
==========

[commit-bot: I haz the power, 2017-01-27 20:40:29]

Committed patchset #3 (id:50001) as
https://chromium.googlesource.com/chromium/src/+/26ddde696e8a0076fed1b3f402d5...