Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2550213002:
[DevTools] Fix Cookie Sort (Closed)
|
Created:
4 years ago by phulce Modified:
4 years ago Reviewers:
dgozman CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] Fix Cookie Sort
Adjust CookiesTable comparator to get the correct property for string comparisons.
BUG=671251
Committed: https://crrev.com/6cbb9199efb2e4996c77e67982cf8a55bed4df3d
Cr-Commit-Position: refs/heads/master@{#437605}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add tests and remove comment #
Total comments: 2
Patch Set 3 : test feedback #
Messages
Total messages: 24 (15 generated)
Description was changed from ========== [DevTools] Fix Cookie Sort Adjust CookiesTable comparator to get the correct property for string comparisons. BUG=671251 ========== to ========== [DevTools] Fix Cookie Sort Adjust CookiesTable comparator to get the correct property for string comparisons. BUG=671251 ==========
phulce@chromium.org changed reviewers: + dgozman@chromium.org
I think this was regressed recently. Could you please find the offending patch? We might need to merge this to last branch. Let's also add a test. https://codereview.chromium.org/2550213002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js (right): https://codereview.chromium.org/2550213002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:216: * @return {string} Value of the property or `name` if no such property exists We don't describe the return value, just mention the result type. The reason for that is written comments get outdated quickly, while compilation-enforced types do not.
On 2016/12/06 01:35:00, dgozman wrote: > I think this was regressed recently. Could you please find the offending patch? > We might need to merge this to last branch. > > Let's also add a test. > > https://codereview.chromium.org/2550213002/diff/1/third_party/WebKit/Source/d... > File > third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js > (right): > > https://codereview.chromium.org/2550213002/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/components_lazy/CookiesTable.js:216: > * @return {string} Value of the property or `name` if no such property exists > We don't describe the return value, just mention the result type. > The reason for that is written comments get outdated quickly, while > compilation-enforced types do not. Yeah was introduced by https://codereview.chromium.org/2466123002. Tests are on the way and I'll remove the description.
PTAL :)
lgtm, thanks! https://codereview.chromium.org/2550213002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector/components/cookies-table.html (right): https://codereview.chromium.org/2550213002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/components/cookies-table.html:6: function initialize_CookiesTableTests() { Don't put initialize_XXX functions in a test - it's only designed for common helper files. Put helper functions inside the test() function instead, and don't prefix them with InspectorTest.
https://codereview.chromium.org/2550213002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector/components/cookies-table.html (right): https://codereview.chromium.org/2550213002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/components/cookies-table.html:6: function initialize_CookiesTableTests() { On 2016/12/08 19:41:20, dgozman wrote: > Don't put initialize_XXX functions in a test - it's only designed for common > helper files. > Put helper functions inside the test() function instead, and don't prefix them > with InspectorTest. Done.
The CQ bit was checked by phulce@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_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by phulce@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.
The CQ bit was checked by phulce@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2550213002/#ps40001 (title: "test feedback")
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": 40001, "attempt_start_ts": 1481309396748430,
"parent_rev": "1ec0af92b43a6f4a431b1ee23f816750affd5179", "commit_rev":
"bed488272de5c6cec5abf58002a2a9b38c612012"}
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Fix Cookie Sort Adjust CookiesTable comparator to get the correct property for string comparisons. BUG=671251 ========== to ========== [DevTools] Fix Cookie Sort Adjust CookiesTable comparator to get the correct property for string comparisons. BUG=671251 Review-Url: https://codereview.chromium.org/2550213002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Fix Cookie Sort Adjust CookiesTable comparator to get the correct property for string comparisons. BUG=671251 Review-Url: https://codereview.chromium.org/2550213002 ========== to ========== [DevTools] Fix Cookie Sort Adjust CookiesTable comparator to get the correct property for string comparisons. BUG=671251 Committed: https://crrev.com/6cbb9199efb2e4996c77e67982cf8a55bed4df3d Cr-Commit-Position: refs/heads/master@{#437605} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/6cbb9199efb2e4996c77e67982cf8a55bed4df3d Cr-Commit-Position: refs/heads/master@{#437605} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
