Chromium Code Reviews
|
|
|
Created:
4 years, 11 months ago by pfeldman Modified:
4 years, 10 months ago CC:
blink-reviews, caseq+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionDevTools: remove hand-written optimistic JSON parser introduced in r183821.
BUG=406900
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196688
Patch Set 1 #Patch Set 2 : with more test cases #
Total comments: 4
Patch Set 3 : for landing #Patch Set 4 : bringing relaxed parser back #
Messages
Total messages: 30 (12 generated)
pfeldman@chromium.org changed reviewers: + dgozman@chromium.org, eustas@chromium.org
PTAL
yurys@chromium.org changed reviewers: + yurys@chromium.org
I'm curious why this is being removed. Could you add this into the CL's description?
I was improving the json detection heuristics and could not figure out why we needed that code. The change that introduced it did not have bug associated, all tests that change it introduced pass :-)
I agree that we should put the reasoning into change description. Even bug does not mention anything. lgtm https://codereview.chromium.org/1163523002/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/network/RequestJSONView.js (right): https://codereview.chromium.org/1163523002/diff/20001/Source/devtools/front_e... Source/devtools/front_end/network/RequestJSONView.js:65: // Only process valid JSONP nit: full stop please. https://codereview.chromium.org/1163523002/diff/20001/Source/devtools/front_e... Source/devtools/front_end/network/RequestJSONView.js:66: if (suffix.length && !suffix.trim().startsWith(")")) && prefix.trim().endsWith("(")
https://codereview.chromium.org/1163523002/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/network/RequestJSONView.js (right): https://codereview.chromium.org/1163523002/diff/20001/Source/devtools/front_e... Source/devtools/front_end/network/RequestJSONView.js:66: if (suffix.length && !suffix.trim().startsWith(")")) "while (1) [1, 2]" is a valid json request.
https://codereview.chromium.org/1163523002/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/network/RequestJSONView.js (right): https://codereview.chromium.org/1163523002/diff/20001/Source/devtools/front_e... Source/devtools/front_end/network/RequestJSONView.js:66: if (suffix.length && !suffix.trim().startsWith(")")) On 2015/05/28 11:27:01, pfeldman wrote: > "while (1) [1, 2]" is a valid json request. But if we have suffix starting with ")", there must prefix ending with "(".
"while (1 [1,2,3,4]" would also work fine :-)
The CQ bit was checked by pfeldman@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/1163523002/#ps40001 (title: "for landing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1163523002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)
The CQ bit was checked by pfeldman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1163523002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)
The CQ bit was checked by pfeldman@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/1163523002/#ps60001 (title: "bringing relaxed parser back")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1163523002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/65478)
The CQ bit was checked by pfeldman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1163523002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196688
Message was sent while issue was closed.
I believe it was added in order to support the malformed JSON that GMail uses in its requests and responses.
Message was sent while issue was closed.
phistuck@gmail.com changed reviewers: + phistuck@gmail.com
Message was sent while issue was closed.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
