|
|
Descriptionnav timing 2 redirect allow opt-in
BUG=665160
Committed: https://crrev.com/d3ac1c7a7ff4222e700720b5d93de532acd48b64
Cr-Commit-Position: refs/heads/master@{#438052}
Patch Set 1 #
Total comments: 6
Patch Set 2 : addressed comments and added unit tests #
Total comments: 4
Patch Set 3 : addressed comments #Patch Set 4 : sync #
Depends on Patchset: Messages
Total messages: 47 (35 generated)
sunjian@chromium.org changed reviewers: + panicker@chromium.org
https://codereview.chromium.org/2550883003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2550883003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:370: return; Factor code above into a helper method getSecurityOrigin(..), to avoid DRY issue: same lines repeated for ResourceTiming. https://codereview.chromium.org/2550883003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:372: const Vector<ResourceResponse>& redirectChain = no need for temp variable? https://codereview.chromium.org/2550883003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:374: bool allowRedirectDetails = allowsTimingRedirect(redirectChain, finalResponse, let's add a unittest for allowTimingRedirect(..)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/2550883003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2550883003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:370: return; On 2016/12/05 19:11:52, panicker wrote: > Factor code above into a helper method getSecurityOrigin(..), to avoid DRY > issue: same lines repeated for ResourceTiming. Done. https://codereview.chromium.org/2550883003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:372: const Vector<ResourceResponse>& redirectChain = On 2016/12/05 19:11:52, panicker wrote: > no need for temp variable? Done. https://codereview.chromium.org/2550883003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:374: bool allowRedirectDetails = allowsTimingRedirect(redirectChain, finalResponse, On 2016/12/05 19:11:52, panicker wrote: > let's add a unittest for allowTimingRedirect(..) Done.
panicker@chromium.org changed reviewers: + yoav@yoav.ws
LGTM https://codereview.chromium.org/2550883003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2550883003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:284: SecurityOrigin* PerformanceBase::getSecurityOrigin() const { Nit: prefer moving to anonymous namespace here as a helper and pass in getExecutionContext() as an arg https://codereview.chromium.org/2550883003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/timing/PerformanceBaseTest.cpp (right): https://codereview.chromium.org/2550883003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceBaseTest.cpp:173: NullExecutionContext nullExecutionContext; Nit: Make this a member in PerformanceBaseTest? In setUp you can: m_executionContext = new NullExecutionContext(); also avoid the const_casts.
All in all, looks good. Can you please run the trybots on this?
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
https://codereview.chromium.org/2550883003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2550883003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:284: SecurityOrigin* PerformanceBase::getSecurityOrigin() const { On 2016/12/07 00:40:52, panicker wrote: > Nit: prefer moving to anonymous namespace here as a helper and pass in > getExecutionContext() as an arg Done. https://codereview.chromium.org/2550883003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/timing/PerformanceBaseTest.cpp (right): https://codereview.chromium.org/2550883003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceBaseTest.cpp:173: NullExecutionContext nullExecutionContext; On 2016/12/07 00:40:52, panicker wrote: > Nit: Make this a member in PerformanceBaseTest? > In setUp you can: m_executionContext = new NullExecutionContext(); > also avoid the const_casts. Done.
The CQ bit was checked by sunjian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by sunjian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by panicker@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Patchset #3 (id:120001) has been deleted
The CQ bit was checked by sunjian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sunjian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/07 03:03:05, Yoav Weiss wrote: > All in all, looks good. Can you please run the trybots on this? Friendly ping. (the failing tests are unrelated)
LGTM
The CQ bit was checked by sunjian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from panicker@chromium.org Link to the patchset: https://codereview.chromium.org/2550883003/#ps140001 (title: "addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by sunjian@chromium.org
The CQ bit was checked by sunjian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yoav@yoav.ws, panicker@chromium.org Link to the patchset: https://codereview.chromium.org/2550883003/#ps160001 (title: "sync")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481592346590040, "parent_rev": "a6c34805d914be6629bab2d553be67133b60773c", "commit_rev": "b7c98aa554e9e1e0c9b9a1ac70f77133ec4d5916"}
Message was sent while issue was closed.
Description was changed from ========== nav timing 2 redirect allow opt-in BUG=665160 ========== to ========== nav timing 2 redirect allow opt-in BUG=665160 Review-Url: https://codereview.chromium.org/2550883003 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== nav timing 2 redirect allow opt-in BUG=665160 Review-Url: https://codereview.chromium.org/2550883003 ========== to ========== nav timing 2 redirect allow opt-in BUG=665160 Committed: https://crrev.com/d3ac1c7a7ff4222e700720b5d93de532acd48b64 Cr-Commit-Position: refs/heads/master@{#438052} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d3ac1c7a7ff4222e700720b5d93de532acd48b64 Cr-Commit-Position: refs/heads/master@{#438052} |