Closed
Bug 1364096
Opened 8 years ago
Closed 7 years ago
Autocomplete flag values
Categories
(DevTools :: Netmonitor, enhancement, P3)
DevTools
Netmonitor
Tracking
(firefox56 fixed)
RESOLVED
FIXED
Firefox 56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: ntim, Assigned: ruturaj)
References
Details
Attachments
(4 files, 8 obsolete files)
(deleted),
image/gif
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
ruturaj
:
review+
|
Details | Diff | Splinter Review |
After typing "status-code:", autocomplete should show all possible values (in this case all the status codes from the request list).
Similar thing for "is:", it should show "cached"/"from-cache" and "running".
Possibly other flags could benefit as well, I haven't checked.
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Comment 1•7 years ago
|
||
Atleast from https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor, the pre-defined list should be
is:
- cached
- from-cache
- running
cause:
- js
- stylesheet
- img
Rest of the values are free-form, unless those too should be auto-completed like "status-code", etc...
Reporter | ||
Comment 2•7 years ago
|
||
(In reply to Ruturaj Vartak from comment #1)
> Rest of the values are free-form, unless those too should be auto-completed
> like "status-code", etc...
Well, we could for example list all status codes found in the request list. Similarly for method, domain, remote ip, mime types, schemes, set-cookie-*, has-response-header.
Feel free to split this in multiple bugs though, it seems like a task big enough to have smaller parts.
Reporter | ||
Comment 3•7 years ago
|
||
You can see what chrome does for autocompletion, I think it really makes sense here.
Assignee | ||
Comment 4•7 years ago
|
||
Chrome seems to be limiting the so-called autocomplete free-form values to what is available in the requests, so if only 200, 302 and 304 type of status-codes are available, it'll show only those in Autocomplete.
I think we need to break this bug into 2 high-level parts atleast
- a sub-autocomplete list feature support in the existing component, and then list work on the react-component, probably it could get an object of sub-autocompletes ie. { "status-code": [200, 302, 304], ... }
- A server-values reducer/distinct list creator. (i'm not sure what the non-UI JS is called in mozilla, I guess server). Perhaps these could be individual tasks.
Once that binding is done, everything should just fall in place, with more reducers/distinct value bindings to attributes can be added.
Please share your thoughts as well.
Reporter | ||
Comment 5•7 years ago
|
||
(In reply to Ruturaj Vartak from comment #4)
> Chrome seems to be limiting the so-called autocomplete free-form values to
> what is available in the requests, so if only 200, 302 and 304 type of
> status-codes are available, it'll show only those in Autocomplete.
Yes, this makes sense. We don't want to show status code that aren't in the requests do we ?
> I think we need to break this bug into 2 high-level parts atleast
> - a sub-autocomplete list feature support in the existing component, and
> then list work on the react-component, probably it could get an object of
> sub-autocompletes ie. { "status-code": [200, 302, 304], ... }
A function makes more sense in this case, since the available status codes may not be constant.
So something like this:
function getAutocompleteValues(flag) {}
As for the react component work, accepting a function that takes in the value and returns a list of items makes sense.
autocompleteList(filter) {
return ...;
}
You might want to ask Julian for his thoughts here.
> - A server-values reducer/distinct list creator. (i'm not sure what the
> non-UI JS is called in mozilla, I guess server). Perhaps these could be
> individual tasks.
You should be able to get the data directly from the client, notably from the redux store.
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → ntim.bugs
Reporter | ||
Updated•7 years ago
|
Assignee: ntim.bugs → nobody
Assignee | ||
Comment 6•7 years ago
|
||
Hi Tim,
I've got netmonitor/src/components/toolbar.js to connect with the redux store and fetch `displayedRequests` which could now be passed on as Toolbar -> SearchBox -> AutocompletePop.
Now I can mix filter and displayedRequests to build the output. If the flow of data is correct, I'll move ahead. Also what kind of object is `displayedRequests` ? something like list / array? the iterated value of it seems to be some sort of a map ie. obj.status, obj.isXHR - I asked this question on irc as well. I got lost during `getUniqueDisplayRequestValues` while working on the object!!
Is the displayedRequest flow correct? or something else should be structured ?
Attachment #8881244 -
Flags: review?(ntim.bugs)
Reporter | ||
Comment 7•7 years ago
|
||
Comment on attachment 8881244 [details] [diff] [review]
WIP-broken-1364096-1.patch
Review of attachment 8881244 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks for working on this!
The displayedRequests flow seems correct, except for the extra prop in the shared components (search-box and autocomplete-popup) that can be solved by my suggestion below.
> Also what kind of object is `displayedRequests` ? something like list / array?
I think it's an object that's created using the Immutable library, they are iterable so you can try logging [...object] to see what kind of object it is (I'm really not sure on top of my head).
::: devtools/client/shared/components/autocomplete-popup.js
@@ +41,5 @@
> }
> },
>
> + computeState({ autocompleteProvider, filter, displayedRequests }) {
> + let list = autocompleteProvider(filter, displayedRequests);
You can avoid this change by setting autocompleteProvider to:
SearchBox({
...
autocompleteProvider: (filter) => autocompleteProvider(filter, displayedRequests);
});
in netmonitor/src/components/toolbar.js
Attachment #8881244 -
Flags: review?(ntim.bugs) → feedback+
Assignee | ||
Comment 8•7 years ago
|
||
- Got status-code working (should work for filter "st")
- Please check if the idea of passing filters / aliases is right, I'll add more flags.
Assignee: nobody → ruturaj
Attachment #8881244 -
Attachment is obsolete: true
Attachment #8881673 -
Flags: review?(ntim.bugs)
Reporter | ||
Comment 9•7 years ago
|
||
Comment on attachment 8881673 [details] [diff] [review]
WIP-1364096-2.patch
Review of attachment 8881673 [details] [diff] [review]:
-----------------------------------------------------------------
::: devtools/client/shared/components/search-box.js
@@ +22,5 @@
> onChange: PropTypes.func,
> placeholder: PropTypes.string,
> type: PropTypes.string,
> autocompleteProvider: PropTypes.func,
> + displayedRequests: PropTypes.object,
nit: remove this leftover
Attachment #8881673 -
Flags: review?(ntim.bugs) → review?(jdescottes)
Assignee | ||
Comment 10•7 years ago
|
||
I've found sometimes the status-code is "undefined" what should I do with it? In such cases the auto list is
- status-code:200
- status-code:304
- status-code:undefined
Should I filter it?
Flags: needinfo?(ntim.bugs)
Reporter | ||
Comment 11•7 years ago
|
||
(In reply to Ruturaj Vartak from comment #10)
> I've found sometimes the status-code is "undefined" what should I do with
> it? In such cases the auto list is
>
> - status-code:200
> - status-code:304
> - status-code:undefined
>
> Should I filter it?
It would make sense to remove/filter out the undefined status-code yes.
Flags: needinfo?(ntim.bugs)
Assignee | ||
Comment 12•7 years ago
|
||
- Generates the autocomplete of flag only when "flag:" is the last token of search string (Initial patch generated the autocomplete when only single flag was listed as the output of autocomplete)
- Integrated all the flags
- Ensured when "flag:" all the requests are listed - isFlagFilterMatch - some flags didn't show this behavior.
Attachment #8881673 -
Attachment is obsolete: true
Attachment #8881673 -
Flags: review?(jdescottes)
Attachment #8882853 -
Flags: review?(ntim.bugs)
Assignee | ||
Comment 13•7 years ago
|
||
Currently the autocompleteProvider uses `displayedRequests` to generate flag values. Sometimes it happens that the displayedValues isn't generated, and flag values aren't found. Need to ensure displayedValues are generated only then flag values are computed.
In the example, status-code: was able to generate the flag values (perhaps typing was faster than rendering of the component). However, when I typed "method:" - autoProvider delivered before displayedResults I guess. Hence nothing. When I focused out and focused in - the values popped up correctly.
I ensured I waited 200ms for flag values (await new Promise(r => setTimeout(r, 200));) by making autocompleteProvider async and await-ing it in SearchBox component, but I goofed up. Not sure how to do it.
Flags: needinfo?(ntim.bugs)
Reporter | ||
Comment 14•7 years ago
|
||
Comment on attachment 8882853 [details] [diff] [review]
WIP-1364096-3.patch
The general approach looks fine, however I don't have time to look at this in detail. Maybe Julian or Honza could look at this.
Flags: needinfo?(ntim.bugs)
Attachment #8882853 -
Flags: review?(ntim.bugs) → review?(jdescottes)
Comment 15•7 years ago
|
||
Comment on attachment 8882853 [details] [diff] [review]
WIP-1364096-3.patch
Review of attachment 8882853 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks for the patch! In the good direction but a few things to improve.
1- you should not rely on displayed requests. If I type, letter by letter, "domain:", when I press ":", the only displayed requests are the ones luckily matching "domain". So the autocomplete list is empty. Maybe use getSortedRequests or another selector that gives you all the requests.
2- I am slightly concerned about performance. On a netmonitor with ~1000 requests, I spend between 10 and 50ms in getFilterFlagValues. That said filtering is already pretty slow, so that's maybe not the most urgent. Would be interested to get performance figures on a regular (ie not 15inches macbook pro) machine.
3- when values are too long they wrap and the autocomplete becomes hard to understand. A single item spans on several lines, but there is no margin between the items. We should add an ellipsis if the text is too long but we shouldn't wrap. Will attach a screenshot.
4- (related to 3-) for the flag has-response-header, we get items such as
"Accept-Ranges,Access-Control-Allow-Headers,Access-Control-Allow-Methods,Access-Control-Allow-Origin,Access-Control-Expose-Headers,Access-Control-Max-Age,Content-Type,Cache-Control,Date,Content-Range,Content-Length,Connection" (this is a single autocomplete item!)
Instead, what should be done here is to split on "," and consider each subvalue as an item. From a user perspective that would be much more useful.
5- autocomplete only shows all the values, but if you start typing a value it won't show anything. For instance, on cnn.com, I have a lot of suggestions for "set-cookie-domain:". I want to start refining them but as soon as I type any letter after ":" all the suggestions disappear. (can be a follow up)
::: devtools/client/netmonitor/src/assets/styles/netmonitor.css
@@ +738,5 @@
>
> /* Network details panel toggle */
>
> .network-details-panel-toggle[disabled] {
> + opacity: 0;
So we forgot to land Bug 1367338 which was supposed to address this issue. I just pushed it to autoland. The button will now simply look disabled.
Please remove this change to avoid merge conflicts.
::: devtools/client/netmonitor/src/utils/filter-text-utils.js
@@ +102,5 @@
> }
> }
>
> function isFlagFilterMatch(item, { type, value, negative }) {
> + // Ensures just flags show all the requests
I don't really understand this comment. What does just flags mean? If I read the code I guess this means that the empty string should be a valid value for any filter.
@@ +197,5 @@
> match = false;
> }
> break;
> case "set-cookie-name":
> + match = value.length > 0 ?
I don't think value is reassigned anywhere in the function. So the first if at the beginning of the method already ensures `value.length > 0`
@@ +203,3 @@
> break;
> case "set-cookie-value":
> + match = value.length > 0 ?
same comment.
@@ +248,5 @@
>
> return match;
> }
>
> +function getRequestFlagValue(flag, request) {
JSDoc
@@ +272,5 @@
> + value = request.mimeType;
> + break;
> + case "set-cookie-name":
> + // Sometimes responseCookies is an object instead of an array
> + value = request.responseCookies instanceof Array ?
I don't know much about the netmonitor but I found the following code in many places:
requestCookies = requestCookies.cookies || requestCookies;
responseCookies = responseCookies.cookies || responseCookies;
You should do the same here and assume that responseCookies will be an array.
@@ +285,5 @@
> + request.responseCookies.map(c => c.hasOwnProperty("domain") ?
> + c.domain : request.urlDetails.host) :
> + "";
> + break;
> + case "is":
For "is" we only have three possible values: "running, "cached", "from-cache".
I think that for discovery purposes and usability if these 3 values were always proposed to the user rather than making them dynamically optional depending on the currently visible requests.
case "is":
value = ["cached", "from-cache", "running"]
(of course then it doesn't make much sense to loop on requests for this case, but extracting it would just make the whole thing even more complicated)
@@ +298,5 @@
> + value = request.responseHeaders &&
> + request.responseHeaders.headers.map(h => h.name);
> + break;
> + case "method":
> + case "protocol":
I don't think request.protocol exists, see the corresponding entry in isFlagFilterMatch()
@@ +306,5 @@
> +
> + return value;
> +}
> +
> +function getFilterFlagValues(filterFlag, displayedRequests) {
Missing JSDoc. I think we are passing a "token" which needs to be parsed to find a flag? filterFlag seems misleading here.
@@ +310,5 @@
> +function getFilterFlagValues(filterFlag, displayedRequests) {
> + let uniqueValues = new Set();
> + for (let request of displayedRequests) {
> + // strip out "-" and ":" from flags ie. "-method:" and pass as flag
> + let value = getRequestFlagValue(filterFlag.replace(/^(:?-)?(.*?):$/, "$2"), request);
#1: moving the filterFlag.replace(re, "$2") on its own line would make this easier to read.
#2: Not sure about this regexp group: `(:?-)?` at first I thought it was a typo and you wanted a non capturing group. This would be `(?:-)?`, but it should actually be equivalent to `-?` (so full re would be `^-?(.*?):$/`). Am I missing something. If yes, probably worth adding a comment.
@@ +311,5 @@
> + let uniqueValues = new Set();
> + for (let request of displayedRequests) {
> + // strip out "-" and ":" from flags ie. "-method:" and pass as flag
> + let value = getRequestFlagValue(filterFlag.replace(/^(:?-)?(.*?):$/, "$2"), request);
> + if (value instanceof Array) {
use Array.isArray(value) (instanceof Array has many pitfalls and is generally a bad practice, https://stackoverflow.com/questions/22289727/difference-between-using-array-isarray-and-instanceof-array)
@@ +321,5 @@
> + }
> + }
> +
> + return Array.from(uniqueValues)
> + .filter(value =>
extra space here
@@ +322,5 @@
> + }
> +
> + return Array.from(uniqueValues)
> + .filter(value =>
> + typeof value !== "undefined" && value !== "" && value !== "undefined")
Can we really have value === "undefined" ? is this coming from the status-code case with 'value = request.status + "";'?
Wondering why you need to stringify the value in the other method, as it would be automatically stringified when passed to `${filterFlag}${value}`.
If it's more complex than that please add a comment clarifying why this is needed.
@@ +335,5 @@
> * The string is then tokenized into "is:cached" and "pr"
> *
> * @param {string} filter - The entire search string of the search box
> + * @param {object} displayedRequests - Iteratable object of requests displayed
> + *
nit: remove empty line here.
@@ +367,5 @@
> && item.toLowerCase() !== lastToken.toLowerCase();
> + });
> +
> + let filledInFlags = [];
> + if (lastToken.endsWith(":")) {
I think this should be checked by getFilterFlagValues (it should actually even check that it )
@@ +368,5 @@
> + });
> +
> + let filledInFlags = [];
> + if (lastToken.endsWith(":")) {
> + // debugger;
remove
@@ +371,5 @@
> + if (lastToken.endsWith(":")) {
> + // debugger;
> + filledInFlags = getFilterFlagValues(lastToken, displayedRequests);
> + }
> + let finalList = filledInFlags.length > 0 ? filledInFlags : interimList;
We should not build the interimList at all if filledInFlags.length > 0
let flags;
if (filledInFlags.length > 0) { flags = filledInFlags}
else { flags = baseList.filter(...) }
Attachment #8882853 -
Flags: review?(jdescottes) → feedback+
Comment 16•7 years ago
|
||
screenshot to illustrate my point #3
Assignee | ||
Comment 17•7 years ago
|
||
Thanks Julian,
I'll work on the suggestions, however regarding the following comment -
> 1- you should not rely on displayed requests. If I type, letter by letter, "domain:", when I press ":", the only displayed requests are the ones luckily matching "domain". So the autocomplete list is empty. Maybe use getSortedRequests or another selector that gives you all the requests.
Yes, I too had this specific concern, however, If a user has pressed any Netmon button like "HTML", 'XHR', etc.. in such a case we might end up showing him requests outside the button-based filtered list - hence I thought `displayedRequests` to be a good list - some other selector (as you mentioned) that uses button filters but NOT the search string filters ? - thoughts ?
- Rutu
Flags: needinfo?(jdescottes)
Comment 18•7 years ago
|
||
I think you could handle this as a followup. If you show values for filtered out requests, it's honestly not that bad.
Then as you said, you can also create a new selector, based on getDisplayedRequests/getFilterFn in devtools/client/netmonitor/src/selectors/requests.js but that only takes the type filtering into account.
Up to you!
Flags: needinfo?(jdescottes)
Assignee | ||
Comment 19•7 years ago
|
||
Worked on following
- created a selector - getButtonFilteredRequests
- Specified network monitor's ellipsis for autocomplete values
- set-cookie* flags match includes instead of exact match
- added JsDoc for getRequestFlagValue
- used suggested responseCookies lookup
- is: is hardcoded autocomplete
- fixed protocol:
- renamed getFilterFlagValues to getLastTokenFlagValues
- added jsDoc for getLastTokenFlagValues
- used suggested Array.isArray()
- creating interimList only if lastTokenFlagValues are absent
I haven't yet worked on "has-response-header:" splitting of ",". Since I thought isn't it wrong to have a comma separated single Header? Could you please give me the URL that lead to that kind of response?
While you review this, I'll work on #5 of comment #15.
I have a older Pentium Laptop, I'll try running some benchmarks on it.
Attachment #8882853 -
Attachment is obsolete: true
Attachment #8884515 -
Flags: review?(jdescottes)
Reporter | ||
Comment 20•7 years ago
|
||
Comment on attachment 8884515 [details] [diff] [review]
WIP-1364096-4.patch
Review of attachment 8884515 [details] [diff] [review]:
-----------------------------------------------------------------
I don't have time to fully review this, but here are some things I've noticed:
- The filter-text-utils.js function is starting to get very large, I would split all the autocomplete code into a new file.
::: devtools/client/netmonitor/src/assets/styles/netmonitor.css
@@ +106,5 @@
> text-decoration: underline;
> }
>
> +/* Special direction change and ellipsis for netmon autocomplete */
> +.network-monitor .autocomplete-item {
This is something that makes sense for all autocomplete items. It should probably be in common.css
::: devtools/client/netmonitor/src/utils/filter-text-utils.js
@@ +258,5 @@
> + * @param {string} flag - flag specified in filter, ie. "status-code"
> + * @param {object} request - Network request item
> + * @return {string|Array} - The output is a string or an array based on the request
> + */
> +function getRequestFlagValue(flag, request) {
The function name doesn't reflect what the function is actually doing IMO. Especially with "is" returning a hardcoded array. The function returns all autocomplete values for a request.
@@ +266,5 @@
> +
> + switch (flag) {
> + case "status-code":
> + // Sometimes status comes as Number
> + value = request.status + "";
nit: value = String(request.status)
Comment 21•7 years ago
|
||
(In reply to Ruturaj Vartak from comment #19)
> Created attachment 8884515 [details] [diff] [review]
> WIP-1364096-4.patch
>
> I haven't yet worked on "has-response-header:" splitting of ",". Since I
> thought isn't it wrong to have a comma separated single Header? Could you
> please give me the URL that lead to that kind of response?
>
Hmm so I can reproduce this issue on virtually any website with your old patch, but the new one seems to fix it. I seemed to have missed the implementation issue that lead to this issue but it seems you fixed it anyway :)
For reference two sites were I reproduced the problem right now with your old patch:
- https://www.mozilla.org/en-US/
- http://us.cnn.com/
Comment 22•7 years ago
|
||
Comment on attachment 8884515 [details] [diff] [review]
WIP-1364096-4.patch
Review of attachment 8884515 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good! Let's see a new patch with both ntim's and my comments addressed.
Can we also add a test in the same vein as devtools/client/netmonitor/test/browser_net_filter-autocomplete.js ?
::: devtools/client/netmonitor/src/selectors/requests.js
@@ +44,5 @@
> return matchesType && isFreetextMatch(r, filters.requestFilterText);
> }
> );
>
> +const getButtonFilterFn = createSelector(
I think it's more relevant to talk about "type" filters than "button" filters here.
@@ +78,5 @@
> (requests, filterFn, sortFn) => requests.valueSeq()
> .filter(filterFn).sort(sortFn).toList()
> );
>
> +const getButtonFilteredRequests = createSelector(
Same comment about naming.
@@ +129,5 @@
>
> module.exports = {
> getDisplayedRequestById,
> getDisplayedRequests,
> + getButtonFilteredRequests,
Preserve alphabetical sorting of this exports object.
Attachment #8884515 -
Flags: review?(jdescottes) → feedback+
Assignee | ||
Comment 23•7 years ago
|
||
Thanks Tim, Julian,
I'll work on your suggestions - Also Tim, could suggest a name for `getRequestFlagValue` - `getValue` would be adequate ?
Flags: needinfo?(ntim.bugs)
Reporter | ||
Comment 24•7 years ago
|
||
How about getAutocompleteValuesForFlag ? Julian might have better suggestions though :)
Flags: needinfo?(ntim.bugs)
Assignee | ||
Comment 25•7 years ago
|
||
- Implemented autocomplete for value
- Implemented suggestions, nits
- renamed getRequestFlagValue to getAutocompleteValuesForFlag
- Working on test case[s]
Attachment #8884515 -
Attachment is obsolete: true
Attachment #8886201 -
Flags: review?(jdescottes)
Reporter | ||
Comment 26•7 years ago
|
||
To answer your question on IRC (sorry I've just seen it now), does document.querySelector(".devtools-filterinput").focus() work ?
Assignee | ||
Comment 27•7 years ago
|
||
yes, that did work thanks ! :)
Assignee | ||
Comment 28•7 years ago
|
||
- Fixed tokenization for negative filter flags
- updated test cases with flag value autocompletion
Attachment #8886201 -
Attachment is obsolete: true
Attachment #8886201 -
Flags: review?(jdescottes)
Attachment #8886773 -
Flags: review?(jdescottes)
Reporter | ||
Comment 29•7 years ago
|
||
Comment on attachment 8886773 [details] [diff] [review]
fix-1364096-1.patch
Review of attachment 8886773 [details] [diff] [review]:
-----------------------------------------------------------------
A few comments.
::: devtools/client/netmonitor/src/utils/filter-autocomplete-provider.js
@@ +79,5 @@
> + * @param {object} requests - List of requests from which values are generated
> + * @return {Array} - array of autocomplete values
> + */
> +function getLastTokenFlagValues(lastToken, requests) {
> + let flag, tokenFlag, typedFlagValue, isNegativeFlag = false;
I usually don't like defining variables until they are used. Also, I would prefer less verbose variable names like `flag` and `flagValue`.
@@ +80,5 @@
> + * @return {Array} - array of autocomplete values
> + */
> +function getLastTokenFlagValues(lastToken, requests) {
> + let flag, tokenFlag, typedFlagValue, isNegativeFlag = false;
> + if (!lastToken.includes(":")) {
Add a comment describing what it does:
// Flag name hasn't been typed, no values are available
@@ +87,5 @@
> +
> + [tokenFlag, typedFlagValue] = lastToken.split(":");
> + flag = tokenFlag;
> + if (tokenFlag.startsWith("-")) {
> + flag = tokenFlag.slice(1);
I think it's easier to read if you remove the `tokenFlag` variable and replace it with a simple `flag` variable. With my suggestion above, this would become:
let [flag, flagValue] = lastToken.split(":");
let isNegative = false;
if (flag.startsWith("-")) {
flag = flag.slice(1);
isNegativeFlag = true;
}
@@ +91,5 @@
> + flag = tokenFlag.slice(1);
> + isNegativeFlag = true;
> + }
> + if (!FILTER_FLAGS.includes(flag)) {
> + // Flag is some random string, return
nit: move the comment above the `if` condition, also add a new line above it.
@@ +105,5 @@
> + uniqueValues.add(v);
> + }
> + } else {
> + uniqueValues.add(value);
> + }
This loop could be simplified:
let values = [];
for (let request of requests) {
values.concat(getAutocompleteValuesForFlag(flag, request));
}
values = [...new Set(values)];
(It would be easier if getAutocompleteValuesForFlag always returned an array btw).
Assignee | ||
Comment 30•7 years ago
|
||
- getAutocompleteValuesForFlag always returns array
- worked on suggestions by :ntim
Attachment #8886773 -
Attachment is obsolete: true
Attachment #8886773 -
Flags: review?(jdescottes)
Attachment #8886835 -
Flags: review?(jdescottes)
Comment 31•7 years ago
|
||
Comment on attachment 8886835 [details] [diff] [review]
fix-1364096-2.patch
Review of attachment 8886835 [details] [diff] [review]:
-----------------------------------------------------------------
Great work! Thanks a lot for adding a test. A few remaining nits from me, but you can forward my r+ to the next versions of your patch after addressing my comments.
Flagging ntim for review since he is tagged in the changeset summary.
::: devtools/client/netmonitor/src/components/toolbar.js
@@ +13,5 @@
> const { connect } = require("devtools/client/shared/vendor/react-redux");
> const Actions = require("../actions/index");
> const { FILTER_SEARCH_DELAY } = require("../constants");
> const {
> + getTypeFilteredRequests,
keep the list alphabetically sorted.
::: devtools/client/netmonitor/src/selectors/requests.js
@@ +79,5 @@
> .filter(filterFn).sort(sortFn).toList()
> );
>
> +const getTypeFilteredRequests = createSelector(
> + state =>
I find the indentation confusing here. Please make sure all arguments have the same indent.
createSelector(
state => state.requests.requests,
getTypeFilterFn,
(requests, filterFn) => requests.valueSeq().filter(filterFn).toList()
);
::: devtools/client/netmonitor/test/browser_net_filter-autocomplete.js
@@ +38,5 @@
> let { monitor } = await initNetMonitor(FILTERING_URL);
> + let { document, store, windowRequire } = monitor.panelWin;
> + let Actions = windowRequire("devtools/client/netmonitor/src/actions/index");
> +
> + store.dispatch(Actions.batchEnable(false));
I know this was copied from another test, but it would be nice if you could add a comment explaining what it achieves and why it is needed.
@@ +88,4 @@
> testAutocompleteContents(["protocol:"], document);
>
> // The new value of the text box should be previousTokens + latest value selected
> EventUtils.synthesizeKey("VK_RETURN", {});
Add comments to explain that the first RETURN selects "protocol" and the second selects "HTTP/1.1"
Attachment #8886835 -
Flags: review?(ntim.bugs)
Attachment #8886835 -
Flags: review?(jdescottes)
Attachment #8886835 -
Flags: review+
Reporter | ||
Comment 32•7 years ago
|
||
Comment on attachment 8886835 [details] [diff] [review]
fix-1364096-2.patch
Review of attachment 8886835 [details] [diff] [review]:
-----------------------------------------------------------------
I've tested this and it works great! Thanks for your valuable contribution :)
r+ from me as well :) You can address Julian and my comments and mark your next patch as review+.
::: devtools/client/netmonitor/src/utils/filter-autocomplete-provider.js
@@ +153,5 @@
> + }
> +
> + let autocompleteList;
> + let filledInFlags = getLastTokenFlagValues(lastToken, requests);
> + if (filledInFlags.length > 0) {
How about availableValues as variable name ?
Attachment #8886835 -
Flags: review?(ntim.bugs) → review+
Assignee | ||
Comment 33•7 years ago
|
||
> const { connect } = require("devtools/client/shared/vendor/react-redux");
> const Actions = require("../actions/index");
> const { FILTER_SEARCH_DELAY } = require("../constants");
> const {
> + getTypeFilteredRequests,
Wasn't quite sure of what this does, I've simply commented
// Let the requests load completely before the autocomplete tests begin
// as autocomplete values also rely on the network requests.
Attachment #8886835 -
Attachment is obsolete: true
Flags: needinfo?(ntim.bugs)
Attachment #8887331 -
Flags: review+
Reporter | ||
Comment 34•7 years ago
|
||
(In reply to Ruturaj Vartak from comment #33)
> Created attachment 8887331 [details] [diff] [review]
> fix-1364096-3.patch
>
> > const { connect } = require("devtools/client/shared/vendor/react-redux");
> > const Actions = require("../actions/index");
> > const { FILTER_SEARCH_DELAY } = require("../constants");
> > const {
> > + getTypeFilteredRequests,
>
> Wasn't quite sure of what this does, I've simply commented
>
> // Let the requests load completely before the autocomplete tests begin
> // as autocomplete values also rely on the network requests.
I'm not sure which review comment you're addressing by adding this comment.
If this is the Actions.batchEnable(false) one, it's just that we dispatch redux actions one at a time instead of multiple actions at a time. I'm not quite sure why we do it, but I guess it might be easier to process in the context of tests. Ricky should be able to answer your question.
Flags: needinfo?(ntim.bugs) → needinfo?(rchien)
Assignee | ||
Comment 35•7 years ago
|
||
I was referring to this comment by Julian
============================================================
> let { monitor } = await initNetMonitor(FILTERING_URL);
> + let { document, store, windowRequire } = monitor.panelWin;
> + let Actions = windowRequire("devtools/client/netmonitor/src/actions/index");
> +
> + store.dispatch(Actions.batchEnable(false));
I know this was copied from another test, but it would be nice if you could add a comment explaining what it achieves and why it is needed.
============================================================
Comment 36•7 years ago
|
||
Yep, Actions.batchEnable(false) doesn't help wait for network requests in this case. I think Ruturaj Vartak is doing the right thing to get the autocomplete list after all network requests are done.
BTW, can we update the autocomplete popup simultaneously when requests are updating?
Flags: needinfo?(rchien)
Assignee | ||
Comment 37•7 years ago
|
||
I'll check if its easily pluggable, probably will have to tinker with react methods
Reporter | ||
Comment 38•7 years ago
|
||
(In reply to Ricky Chien [:rickychien] from comment #36)
> BTW, can we update the autocomplete popup simultaneously when requests are
> updating?
I don't think it's too bad if we don't have it.
Assignee | ||
Comment 39•7 years ago
|
||
So Tim, would the last patch be good enough for checkin? Or ill try to have a go at updating it?
Flags: needinfo?(ntim.bugs)
Reporter | ||
Comment 40•7 years ago
|
||
(In reply to Ruturaj Vartak from comment #39)
> So Tim, would the last patch be good enough for checkin? Or ill try to have
> a go at updating it?
If it doesn't require too much effort, then feel free to update it.
Otherwise, file a new bug for it (so you can land this part first, and then focus on the other part later).
Flags: needinfo?(ntim.bugs)
Reporter | ||
Comment 41•7 years ago
|
||
I think it's worth filing a new bug for live updating autocomplete.
Keywords: checkin-needed
Comment 42•7 years ago
|
||
The attached patch doesn't apply cleanly. Please rebase.
Flags: needinfo?(ruturaj)
Keywords: checkin-needed
Assignee | ||
Comment 43•7 years ago
|
||
Hey Tim,
I'm unable to use devtools-launchpad after latest master merge into my branch, it shows up an error in browser's console `Error: could not find pref branch devtools.cache.disabled`.
I'm not sure if its related to bug#1349561
Flags: needinfo?(ruturaj) → needinfo?(ntim.bugs)
Assignee | ||
Comment 45•7 years ago
|
||
Hey Tim,
Addition of line `pref("devtools.cache.disabled", false);` in devtools/client/netmonitor/index.js is still not helping. Or is there anything else that is required?
Thanks.
Flags: needinfo?(ntim.bugs)
Assignee | ||
Comment 47•7 years ago
|
||
merged the latest master.
Attachment #8887331 -
Attachment is obsolete: true
Attachment #8891290 -
Flags: review+
Reporter | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 48•7 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af9a5f0846e6
Autocomplete for network monitor flag values. r=ntim, r=jdescottes
Keywords: checkin-needed
Comment 49•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•