Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2180743002:
[Devtools] Fixed regex filtering in the Network panel for Negative lookaheads (Closed)
|
Created:
4 years, 5 months ago by gjpyzer Modified:
4 years, 4 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, pfeldman, sergeyv+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Devtools] Fixed regex filtering in the Network panel for Negative lookaheads
Fixed regex filtering in the Network panel as ORing two regex tests breaks
in some situatons, such as negative lookaheads. For example, if you are to
filter out "foo", and you have the request Name property as "bob.js" and the
Path as "/path/foo", then the regex is true for Name and false for Path.
This results in: true || false === true. It would only filter out if both
contained "foo" in them.
We now concatenate the Path and Name together, with a "/" to join. This is
also more intuitive, as it will allow first character matching on Path.
BUG=630852
Committed: https://crrev.com/c002b266f83798e8c652a641f74b381f7fb82171
Cr-Commit-Position: refs/heads/master@{#411252}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove redundant brackets and avoided variable use for test function #
Total comments: 1
Patch Set 3 : Fixed function name to reflect functional changes #
Total comments: 1
Patch Set 4 : Made function name shorter #Messages
Total messages: 31 (11 generated)
gjpyzer@gmail.com changed reviewers: + paulirish@chromium.com, pfeldman@chromium.com
The CQ bit was checked by gjpyzer@gmail.com
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Description was changed from ========== Fixed regex filtering in the Network panel as ORing two regex tests breaks in some situatons, such as negative lookaheads. For example, if you are to filter out "foo", and you have the request Name property as "bob.js" and the Path as "/path/foo", then the regex is true for Name and false for Path. This results in: true || false === true. It would only filter out if both contained "foo" in them. We now concatenate the Path and Name together, with a "/" to join. This is also more intuitive, as it will allow first character matching on Path. BUG=630852 ========== to ========== Fixed regex filtering in the Network panel as ORing two regex tests breaks in some situatons, such as negative lookaheads. For example, if you are to filter out "foo", and you have the request Name property as "bob.js" and the Path as "/path/foo", then the regex is true for Name and false for Path. This results in: true || false === true. It would only filter out if both contained "foo" in them. We now concatenate the Path and Name together, with a "/" to join. This is also more intuitive, as it will allow first character matching on Path. BUG=630852 ==========
allada@chromium.org changed reviewers: + allada@chromium.org, lushnikov@chromium.org - paulirish@chromium.com, pfeldman@chromium.com
Thanks very much, it looks good, just a few things. https://codereview.chromium.org/2180743002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): https://codereview.chromium.org/2180743002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:999: var text = (this._networkLogLargeRowsSetting.get() ? request.path() + "/" + request.name() : request.name()); nit: remove parenthesis wrap. https://codereview.chromium.org/2180743002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1483: var text = request.path() + "/" + request.name(); nit: lets put this into the regex.test() method.
Description was changed from ========== Fixed regex filtering in the Network panel as ORing two regex tests breaks in some situatons, such as negative lookaheads. For example, if you are to filter out "foo", and you have the request Name property as "bob.js" and the Path as "/path/foo", then the regex is true for Name and false for Path. This results in: true || false === true. It would only filter out if both contained "foo" in them. We now concatenate the Path and Name together, with a "/" to join. This is also more intuitive, as it will allow first character matching on Path. BUG=630852 ========== to ========== [Devtools] Filtering in the Network panel as ORing two regex tests breaks Fixed regex filtering in the Network panel as ORing two regex tests breaks in some situatons, such as negative lookaheads. For example, if you are to filter out "foo", and you have the request Name property as "bob.js" and the Path as "/path/foo", then the regex is true for Name and false for Path. This results in: true || false === true. It would only filter out if both contained "foo" in them. We now concatenate the Path and Name together, with a "/" to join. This is also more intuitive, as it will allow first character matching on Path. BUG=630852 ==========
I will write a test for this and commit it after yours is published. (per: dgozman@)
On 2016/08/08 at 18:00:34, allada wrote: > I will write a test for this and commit it after yours is published. (per: dgozman@) Hi, the second nit you mentioned "lets put this into the regex.test() method.". What do you mean by that? There is only one parameter for the `test` function - the string you are testing.
On 2016/08/08 at 18:07:12, gjpyzer wrote: > On 2016/08/08 at 18:00:34, allada wrote: > > I will write a test for this and commit it after yours is published. (per: dgozman@) > Hi, the second nit you mentioned "lets put this into the regex.test() method.". What do you mean by that? There is only one parameter for the `test` function - the string you are testing. I have now assumed you meant I should avoid creating the `text` variable to pass into the `test` function and use the concatenated string directly, since it's pretty small. I have submitted the new patch.
lgtm bringing lushnikov@ for final review.
let's add a test https://codereview.chromium.org/2180743002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): https://codereview.chromium.org/2180743002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1478: WebInspector.NetworkLogView._requestNameOrPathFilter = function(regex, request) _requestPathFilter = function...
as per offline discussion, Blaise is going to write a test for this in a follow-up. Let's fix function name and this is good to go
On 2016/08/08 at 21:16:29, lushnikov wrote: > as per offline discussion, Blaise is going to write a test for this in a follow-up. > > Let's fix function name and this is good to go I have fixed the function name. Thanks.
https://codereview.chromium.org/2180743002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): https://codereview.chromium.org/2180743002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1478: WebInspector.NetworkLogView._requestNameAndPathFilter = function(regex, request) Can we name it _requestPathFilter? It would be shorter & more clear
On 2016/08/08 at 23:13:20, lushnikov wrote: > https://codereview.chromium.org/2180743002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): > > https://codereview.chromium.org/2180743002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1478: WebInspector.NetworkLogView._requestNameAndPathFilter = function(regex, request) > Can we name it _requestPathFilter? It would be shorter & more clear Sure, that works too. Done.
thank you, lgtm
Just as an FYI you need to click "commit" checkbox in order for the changes to take affect.
The CQ bit was checked by gjpyzer@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from allada@chromium.org Link to the patchset: https://codereview.chromium.org/2180743002/#ps60001 (title: "Made function name shorter")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/10 at 22:14:06, allada wrote: > Just as an FYI you need to click "commit" checkbox in order for the changes to take affect. Haha thanks.
Message was sent while issue was closed.
Description was changed from ========== [Devtools] Filtering in the Network panel as ORing two regex tests breaks Fixed regex filtering in the Network panel as ORing two regex tests breaks in some situatons, such as negative lookaheads. For example, if you are to filter out "foo", and you have the request Name property as "bob.js" and the Path as "/path/foo", then the regex is true for Name and false for Path. This results in: true || false === true. It would only filter out if both contained "foo" in them. We now concatenate the Path and Name together, with a "/" to join. This is also more intuitive, as it will allow first character matching on Path. BUG=630852 ========== to ========== [Devtools] Filtering in the Network panel as ORing two regex tests breaks Fixed regex filtering in the Network panel as ORing two regex tests breaks in some situatons, such as negative lookaheads. For example, if you are to filter out "foo", and you have the request Name property as "bob.js" and the Path as "/path/foo", then the regex is true for Name and false for Path. This results in: true || false === true. It would only filter out if both contained "foo" in them. We now concatenate the Path and Name together, with a "/" to join. This is also more intuitive, as it will allow first character matching on Path. BUG=630852 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Devtools] Filtering in the Network panel as ORing two regex tests breaks Fixed regex filtering in the Network panel as ORing two regex tests breaks in some situatons, such as negative lookaheads. For example, if you are to filter out "foo", and you have the request Name property as "bob.js" and the Path as "/path/foo", then the regex is true for Name and false for Path. This results in: true || false === true. It would only filter out if both contained "foo" in them. We now concatenate the Path and Name together, with a "/" to join. This is also more intuitive, as it will allow first character matching on Path. BUG=630852 ========== to ========== [Devtools] Filtering in the Network panel as ORing two regex tests breaks Fixed regex filtering in the Network panel as ORing two regex tests breaks in some situatons, such as negative lookaheads. For example, if you are to filter out "foo", and you have the request Name property as "bob.js" and the Path as "/path/foo", then the regex is true for Name and false for Path. This results in: true || false === true. It would only filter out if both contained "foo" in them. We now concatenate the Path and Name together, with a "/" to join. This is also more intuitive, as it will allow first character matching on Path. BUG=630852 Committed: https://crrev.com/c002b266f83798e8c652a641f74b381f7fb82171 Cr-Commit-Position: refs/heads/master@{#411252} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c002b266f83798e8c652a641f74b381f7fb82171 Cr-Commit-Position: refs/heads/master@{#411252}
Message was sent while issue was closed.
Description was changed from ========== [Devtools] Filtering in the Network panel as ORing two regex tests breaks Fixed regex filtering in the Network panel as ORing two regex tests breaks in some situatons, such as negative lookaheads. For example, if you are to filter out "foo", and you have the request Name property as "bob.js" and the Path as "/path/foo", then the regex is true for Name and false for Path. This results in: true || false === true. It would only filter out if both contained "foo" in them. We now concatenate the Path and Name together, with a "/" to join. This is also more intuitive, as it will allow first character matching on Path. BUG=630852 Committed: https://crrev.com/c002b266f83798e8c652a641f74b381f7fb82171 Cr-Commit-Position: refs/heads/master@{#411252} ========== to ========== [Devtools] Fixed regex filtering in the Network panel for Negative lookaheads Fixed regex filtering in the Network panel as ORing two regex tests breaks in some situatons, such as negative lookaheads. For example, if you are to filter out "foo", and you have the request Name property as "bob.js" and the Path as "/path/foo", then the regex is true for Name and false for Path. This results in: true || false === true. It would only filter out if both contained "foo" in them. We now concatenate the Path and Name together, with a "/" to join. This is also more intuitive, as it will allow first character matching on Path. BUG=630852 Committed: https://crrev.com/c002b266f83798e8c652a641f74b381f7fb82171 Cr-Commit-Position: refs/heads/master@{#411252} ========== |
