Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2343773002:
DevTools: Autocomplete class names in ClassesPaneWidget (Closed)
|
Created:
4 years, 3 months ago by ahmetemirercin Modified:
4 years, 2 months ago 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. |
DescriptionDevTools: Autocomplete class names in ClassesPaneWidget
BUG=636089
Committed: https://crrev.com/dca69570d35a1f982f72543d81d66a240a030696
Cr-Commit-Position: refs/heads/master@{#422184}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Removed caching #
Total comments: 4
Patch Set 3 : Simplified #
Total comments: 2
Patch Set 4 : Moved all logic to prompt #Patch Set 5 : Rebased #
Total comments: 8
Patch Set 6 : Reviewed #
Total comments: 3
Patch Set 7 : Reviewed #
Total comments: 9
Patch Set 8 : Reviewed #
Messages
Total messages: 34 (5 generated)
Description was changed from ========== DevTools: Autocomplete class names in ClassesPaneWidget DevTools: Autocomplete class names in ClassesPaneWidget DevTools: Autocomplete class names in ClassesPaneWidget DevTools: Autocomplete class names in ClassesPaneWidget BUG= DevTools: Autocomplete class names in ClassesPaneWidget ========== to ========== DevTools: Autocomplete class names in ClassesPaneWidget BUG=636089 ==========
ahmetemiremir@gmail.com changed reviewers: + lushnikov@chromium.org, paulirish@chromium.org, pfeldman@chromium.org
Can you please review this?
https://codereview.chromium.org/2343773002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2343773002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:383: this._cssClassNames = new Map(); can we avoid caching class names altogether? it should be fast enough and it will greatly simplify code If requesting classnames is slow, then who's in charge - DOM classnames or CSS classnames? https://codereview.chromium.org/2343773002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/sdk/CSSModel.js (right): https://codereview.chromium.org/2343773002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/CSSModel.js:538: * @return {!Promise<?Array<string>>} let's fulfill with !Array<string> https://codereview.chromium.org/2343773002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/sdk/DOMModel.js (right): https://codereview.chromium.org/2343773002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/DOMModel.js:1756: * @param {?string} error nit: comment indentation https://codereview.chromium.org/2343773002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/DOMModel.js:1764: fulfill(null); let's fulfill []
https://codereview.chromium.org/2343773002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2343773002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:383: this._cssClassNames = new Map(); On 2016/09/15 17:58:31, lushnikov wrote: > can we avoid caching class names altogether? it should be fast enough and > it will greatly simplify code > > If requesting classnames is slow, then who's in charge - DOM classnames or CSS > classnames? Speed of getting classnames depends on dom structure and size of stylesheet so will vary from site to site. I will send a patch not caching classnames but not sure that it will simplify code so much since it is already too simple.
https://codereview.chromium.org/2343773002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2343773002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:383: this._cssClassNames = new Map(); On 2016/09/15 17:58:31, lushnikov wrote: > can we avoid caching class names altogether? it should be fast enough and > it will greatly simplify code > > If requesting classnames is slow, then who's in charge - DOM classnames or CSS > classnames? Done. https://codereview.chromium.org/2343773002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/sdk/DOMModel.js (right): https://codereview.chromium.org/2343773002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/DOMModel.js:1756: * @param {?string} error On 2016/09/15 17:58:31, lushnikov wrote: > nit: comment indentation Done. https://codereview.chromium.org/2343773002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/DOMModel.js:1764: fulfill(null); On 2016/09/15 17:58:31, lushnikov wrote: > let's fulfill [] Done.
Thanks, I think we can simplify it even further! https://codereview.chromium.org/2343773002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2343773002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:81: _documentUpdated: function(event) let's pull the classnames whenever the text prompt gets focused. This will free you of listening to all these events. I would also move all the logic related to className collection down to the ClassNamePrompt
https://codereview.chromium.org/2343773002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2343773002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:81: _documentUpdated: function(event) On 2016/09/16 16:40:01, lushnikov wrote: > let's pull the classnames whenever the text prompt gets focused. This will free > you of listening to all these events. > > I would also move all the logic related to className collection down to the > ClassNamePrompt But what if these events occurs when the text prompt is already focused? Either way, we have to listen them, don't we?
https://codereview.chromium.org/2343773002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2343773002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:81: _documentUpdated: function(event) If the completions change while the suggest box is opened, we wouldn't want to change them dynamically - user is already interacting with the control. So listening to "focus" to pull completions looks like a good-enough solution for me. However, you can go even further and pull the classnames in the every beginning of the completion session (say, re-pull completions on ctrl-space). This seems to be slightly harder to implement, but should address your concern.
On 2016/09/16 17:09:17, lushnikov wrote: > https://codereview.chromium.org/2343773002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js > (right): > > https://codereview.chromium.org/2343773002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:81: > _documentUpdated: function(event) > If the completions change while the suggest box is opened, we wouldn't want to > change them dynamically - user is > already interacting with the control. > > So listening to "focus" to pull completions looks like a good-enough solution > for me. > > However, you can go even further and pull the classnames in the every beginning > of the completion session (say, re-pull completions on > ctrl-space). This seems to be slightly harder to implement, but should address > your concern. Actually,I thought listening only focus event will be insufficient since my implementation shows suggestions after first character typed and focus event would be too early to collect classnames. But collecting before opening of suggestion box seems to work.From the beginning, all I want to achieve is realize autocomplete feature without sacrificing performance.(We moved collecting classnames to backend for this reason and patch set 1 tries to minimize merging classnames after any change). I'm not sure how it will perform but send new patch implementing your suggestion.
Thank you Ahmet,looking forward to your patch. In case of performance issues, we can add caching layer at the CSSStyleSheetHeader level
https://codereview.chromium.org/2343773002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2343773002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:81: _documentUpdated: function(event) On 2016/09/16 17:09:17, lushnikov wrote: > If the completions change while the suggest box is opened, we wouldn't want to > change them dynamically - user is > already interacting with the control. > > So listening to "focus" to pull completions looks like a good-enough solution > for me. > > However, you can go even further and pull the classnames in the every beginning > of the completion session (say, re-pull completions on > ctrl-space). This seems to be slightly harder to implement, but should address > your concern. Done.
@Ahmet: one of two comments published weirdly but should be visible inline in the code https://codereview.chromium.org/2343773002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2343773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:85: _hidePrompt: function() let's inline this
https://codereview.chromium.org/2343773002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2343773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:90: _getDomClasses: function() On 2016/09/20 16:46:21, lushnikov wrote: > Can we move all the logic inside the ClassNamePrompt? > Something along the lines: > > _buildClassNameCompletions: function(completionsReady) > { > var selectedNode = WebInspector.context.flavor(WI.DOMNode); > var allClasses = new Set(); > var stylesheets = cssModel.allStyleSheets(); > var promises = []; > for (var stylesheet of stylesheets) { > var promise = stylesheet.getClassNames().then(classes => > allClasses.addAll(classes)); > promises.push(promise); > } > var domClassesPromise = > selectedNode.ownerDocument.getClassNames().then(classes => > allClasses.addAll(classes)); > promises.push(domClassesPromise); > Promise.all(promises).then(() => completionsReady(allClasses.valuesArray())) > } > Correct me if I'm wrong: cssModel will collect stylesheets from all frames but we have to suggest only for selected node's owner frame so need that frameToClassName stuff.Beside this promises can resolve after selected frame is changed so resulting in suggesting invalid classes for node. If these are wrong I will simplify my patch according to your suggestion otherwise it seems there is no need to further simplify and move it. Wdyt?
> Correct me if I'm wrong: cssModel will collect stylesheets from all frames but > we have to suggest only for selected node's owner frame so need that > frameToClassName stuff. More or less! But i'd rather suggest filtering the stylesheets for the frameid: var stylesheets = cssModel.allStyleSheets().filter(styleSheet => styleSheet.frameId === selectedNode.frameId); > Beside this promises can resolve after selected frame is > changed so resulting in suggesting invalid classes for node. Just closing autocomplete in this case would be enough. Isn't it? > If these are wrong > I will simplify my patch according to your suggestion otherwise it seems there > is no need to further simplify and move it. Wdyt? I'm sorry for asking you so many times to iterate over this patch! But I believe we can improve on the patch, thus making life of future devtools maintainers easier :)
On 2016/09/20 19:02:08, lushnikov wrote: > > Correct me if I'm wrong: cssModel will collect stylesheets from all frames but > > we have to suggest only for selected node's owner frame so need that > > frameToClassName stuff. > More or less! But i'd rather suggest filtering the stylesheets for the frameid: > > var stylesheets = cssModel.allStyleSheets().filter(styleSheet => > styleSheet.frameId === selectedNode.frameId); > > > Beside this promises can resolve after selected frame is > > changed so resulting in suggesting invalid classes for node. > > Just closing autocomplete in this case would be enough. Isn't it? > > > If these are wrong > > I will simplify my patch according to your suggestion otherwise it seems there > > is no need to further simplify and move it. Wdyt? > > I'm sorry for asking you so many times to iterate over this patch! But I believe > we can improve on the patch, thus making life of future devtools maintainers > easier :) Closing a prompt while it's open just because of an early promise would be distracting (especially for user). We can go on and keep the results if needed? with frame id. May be we can simplify some stuff here with filtering. And I'm very happy with your suggestions (even waiting them for days). So I will try to improve and send a new patch.Thanks for your time.:)
please take a look at this?
As I've applied the patch, it keeps throwing an error: "Uncaught TypeError: this._prompt.clearAutoComplete is not a function" Could you please re-baseline the patch so that I can play with it?
On 2016/09/28 18:41:17, lushnikov wrote: > As I've applied the patch, it keeps throwing an error: > > "Uncaught TypeError: this._prompt.clearAutoComplete is not a function" > > Could you please re-baseline the patch so that I can play with it? Sorry for that. I rebaselined it.
Works good! The only thing I missed is the autocompletion for the query, starting with ".". Can we add it? Example: 1. goto google.com 2. in classes pane, type ".g" 3. hit "ctrl-space" Actual: no autocomplete Expected: I'd expect autocomplete to work just fine https://codereview.chromium.org/2343773002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2343773002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:237: onKeyDown: function(event) why do you need to override onKeyDown? Looks like the default behavior will suite good. https://codereview.chromium.org/2343773002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:268: var selectedNode = WebInspector.context.flavor(WebInspector.DOMNode); at times selectedNode is null - let's bail out in this case. https://codereview.chromium.org/2343773002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:273: for (var stylesheet of allStyleSheets) { this code runs for every keypress in ClassesPaneWidget. Can we do it less often?
I implemented searching with "." and think that completions must start with "." if queries so since I assume it's better to give user something like what he searched and without this inline completion slips considering the dot as a part of query. https://codereview.chromium.org/2343773002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2343773002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:237: onKeyDown: function(event) On 2016/09/28 21:19:39, lushnikov wrote: > why do you need to override onKeyDown? Looks like the default behavior will > suite good. Done. https://codereview.chromium.org/2343773002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:268: var selectedNode = WebInspector.context.flavor(WebInspector.DOMNode); On 2016/09/28 21:19:39, lushnikov wrote: > at times selectedNode is null - let's bail out in this case. Done. https://codereview.chromium.org/2343773002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:273: for (var stylesheet of allStyleSheets) { On 2016/09/28 21:19:40, lushnikov wrote: > this code runs for every keypress in ClassesPaneWidget. Can we do it less often? I propose keeping old completions when user moving forward assuming he probably found the class name that he is looking for and invalidate while returning back. Wdyt?
https://codereview.chromium.org/2343773002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2343773002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:273: for (var stylesheet of allStyleSheets) { On 2016/09/28 23:40:56, ahmetemirercin wrote: > On 2016/09/28 21:19:40, lushnikov wrote: > > this code runs for every keypress in ClassesPaneWidget. Can we do it less > often? > > I propose keeping old completions when user moving forward assuming he probably > found the class name that he is looking for and invalidate while returning back. > Wdyt? It's better, but as someone who explores completions via typing-and-deleting, I feel uneasy. How would you like invalidating completions if either "force" is true or prefix is empty? Looks like it should satisfy both of us. https://codereview.chromium.org/2343773002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2343773002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:253: this._completions = new Set(); We usually employ promises to cache values. One great benefit is that It makes code more modular and easier to read. With this approach, this "if" statement will look like this: if (!this._classNamesPromise || this._selectedFrameId !== selectedNode.frameId() || !prefxis.startsWith(this._lastQuery)) this._classNamesPromise = this._getClassNames(); this._classNamesPromise.then(completions => completionsReadyCallback(<filter completions>)); The "_getClassNames" function does not mutate state of the object. Instead, it returns a promise which resolves with fetched completions: /** * @return {!Promise<!Set<string>>} */ _getClassNames: function() { var completions = new Set(); .... the code .... return Promise.all(promises).then(() => completions); } https://codereview.chromium.org/2343773002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:274: value = "." + value; i agree with this
https://codereview.chromium.org/2343773002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2343773002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:273: for (var stylesheet of allStyleSheets) { On 2016/09/29 16:03:15, lushnikov wrote: > On 2016/09/28 23:40:56, ahmetemirercin wrote: > > On 2016/09/28 21:19:40, lushnikov wrote: > > > this code runs for every keypress in ClassesPaneWidget. Can we do it less > > often? > > > > I propose keeping old completions when user moving forward assuming he > probably > > found the class name that he is looking for and invalidate while returning > back. > > Wdyt? > > It's better, but as someone who explores completions via typing-and-deleting, I > feel uneasy. > > How would you like invalidating completions if either "force" is true or prefix > is empty? Looks like it should satisfy both of us. I was afraid that classnames can be ready when user is already typed something and it's unusual to clear whole word or ctrl+d if you can't see a suggestion and try to correct it. I think we can invalidate a little bit more than waiting !prefix && force. https://codereview.chromium.org/2343773002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2343773002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:253: this._completions = new Set(); On 2016/09/29 16:03:15, lushnikov wrote: > We usually employ promises to cache values. One great benefit is that It makes > code more modular and easier to read. > > With this approach, this "if" statement will look like this: > > if (!this._classNamesPromise || this._selectedFrameId !== > selectedNode.frameId() || !prefxis.startsWith(this._lastQuery)) > this._classNamesPromise = this._getClassNames(); > > this._classNamesPromise.then(completions => completionsReadyCallback(<filter > completions>)); > > The "_getClassNames" function does not mutate state of the object. > Instead, it returns a promise which resolves with fetched completions: > > /** > * @return {!Promise<!Set<string>>} > */ > _getClassNames: function() > { > var completions = new Set(); > .... the code .... > return Promise.all(promises).then(() => completions); > } I just see that promises are used like this after your comment. Thanks.
Played with this, works like a charm. Thanks! Last bunch of comments https://codereview.chromium.org/2343773002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2343773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:235: _getClassNames: function() let's add jsdoc and pass in selectedNode. This will explicitly state that selectedNode is not null https://codereview.chromium.org/2343773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:240: this._selectedFrameId = selectedNode.frameId() || WebInspector.ResourceTreeModel.fromTarget(selectedNode.target()).mainFrame.id; you can drop "|| WebInspector.ResourceTreeModel.." - the node doesn't have frameId if it is detached, and detached node is never selected. https://codereview.chromium.org/2343773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:265: if (!prefix || force) let's embed this in the "if" on line 269 https://codereview.chromium.org/2343773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:280: if (prefix.substring(0, 1) === ".") let's filter/map if (prefix[0] === ".") completions = completions.map(value => "." + value); var results = comlpetions.filter(value => value.startsWith(prefix)); completionsReadyCallback(results, 0);
https://codereview.chromium.org/2343773002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2343773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:235: _getClassNames: function() On 2016/09/29 21:01:50, lushnikov wrote: > let's add jsdoc and pass in selectedNode. This will explicitly state that > selectedNode is not null Done. https://codereview.chromium.org/2343773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:240: this._selectedFrameId = selectedNode.frameId() || WebInspector.ResourceTreeModel.fromTarget(selectedNode.target()).mainFrame.id; On 2016/09/29 21:01:50, lushnikov wrote: > you can drop "|| WebInspector.ResourceTreeModel.." - the node doesn't have > frameId if it is detached, and detached node is never selected. Done. https://codereview.chromium.org/2343773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:265: if (!prefix || force) On 2016/09/29 21:01:50, lushnikov wrote: > let's embed this in the "if" on line 269 We can't embed this with that if while expecting force to be true, can we? https://codereview.chromium.org/2343773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:280: if (prefix.substring(0, 1) === ".") On 2016/09/29 21:01:50, lushnikov wrote: > let's filter/map > > if (prefix[0] === ".") > completions = completions.map(value => "." + value); > var results = comlpetions.filter(value => value.startsWith(prefix)); > completionsReadyCallback(results, 0); > Done.
lgtm https://codereview.chromium.org/2343773002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2343773002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:265: if (!prefix || force) On 2016/09/30 17:38:40, ahmetemirercin wrote: > On 2016/09/29 21:01:50, lushnikov wrote: > > let's embed this in the "if" on line 269 > > We can't embed this with that if while expecting force to be true, can we? ah, indeed!
The CQ bit was checked by ahmetemiremir@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/30 18:05:55, lushnikov wrote: > lgtm > > https://codereview.chromium.org/2343773002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js > (right): > > https://codereview.chromium.org/2343773002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:265: > if (!prefix || force) > On 2016/09/30 17:38:40, ahmetemirercin wrote: > > On 2016/09/29 21:01:50, lushnikov wrote: > > > let's embed this in the "if" on line 269 > > > > We can't embed this with that if while expecting force to be true, can we? > > ah, indeed! Thanks for your time and patience!
Message was sent while issue was closed.
Description was changed from ========== DevTools: Autocomplete class names in ClassesPaneWidget BUG=636089 ========== to ========== DevTools: Autocomplete class names in ClassesPaneWidget BUG=636089 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== DevTools: Autocomplete class names in ClassesPaneWidget BUG=636089 ========== to ========== DevTools: Autocomplete class names in ClassesPaneWidget BUG=636089 Committed: https://crrev.com/dca69570d35a1f982f72543d81d66a240a030696 Cr-Commit-Position: refs/heads/master@{#422184} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/dca69570d35a1f982f72543d81d66a240a030696 Cr-Commit-Position: refs/heads/master@{#422184} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
