|
|
Created:
3 years, 10 months ago by caseq Modified:
3 years, 10 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: decouple model logic from presentation within CSSTrackerView
This simplifies the rules usage and stylesheet processing logic in
CSSTrackerView from the presentation code that lays a way for future
splitting this view into several parts.
This also groups rules by styleshetts early that simplifies processing
in several places.
Review-Url: https://codereview.chromium.org/2705433003
Cr-Commit-Position: refs/heads/master@{#451429}
Committed: https://chromium.googlesource.com/chromium/src/+/d6fa448cd41d51f3b637f5bcfa4d6aac5d13abb8
Patch Set 1 #
Total comments: 12
Patch Set 2 : Review comments addressed #
Total comments: 1
Messages
Total messages: 28 (14 generated)
caseq@chromium.org changed reviewers: + alph@chromium.org
lgtm % comments https://codereview.chromium.org/2705433003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/css_tracker/CSSTrackerView.js (right): https://codereview.chromium.org/2705433003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/css_tracker/CSSTrackerView.js:85: cssModel.ruleListPromise() nit: wanna be a cool guy and start using async/await? This seems to be a perfect candidate for that. https://codereview.chromium.org/2705433003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/css_tracker/CSSTrackerView.js:98: var styleSheetHeader = cssModel.styleSheetHeaderForId(rule.styleSheetId); nit: || null; https://codereview.chromium.org/2705433003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/css_tracker/CSSTrackerView.js:107: return Promise.all(styleSheetUsages.map(entry => this._populateSourceInfo(entry))).then(() => styleSheetUsages); nit: if you make _populateSourceInfo return entry, you won't need an extra 'then'. https://codereview.chromium.org/2705433003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/css_tracker/CSSTrackerView.js:122: * @param {!CSSTracker.StyleSheetUsage} styleSheetUsage @return https://codereview.chromium.org/2705433003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/css_tracker/CSSTrackerView.js:160: unused += styleSheet.rules.reduce((count, rule) => rule.wasUsed ? count : count + 1); you need an initial value https://codereview.chromium.org/2705433003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/css_tracker/CSSTrackerView.js:162: var percentUnused = Math.round(100 * unused / total); can total be 0? https://codereview.chromium.org/2705433003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/css_tracker/CSSTrackerView.js:201: if (!sheet.styleSheetHeader) { can you plz reverse the condition
The CQ bit was checked by caseq@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org Link to the patchset: https://codereview.chromium.org/2705433003/#ps20001 (title: "Review comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2705433003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/css_tracker/CSSTrackerView.js (right): https://codereview.chromium.org/2705433003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/css_tracker/CSSTrackerView.js:85: cssModel.ruleListPromise() On 2017/02/17 19:08:55, alph wrote: > nit: wanna be a cool guy and start using async/await? This seems to be a perfect > candidate for that. No. https://codereview.chromium.org/2705433003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/css_tracker/CSSTrackerView.js:98: var styleSheetHeader = cssModel.styleSheetHeaderForId(rule.styleSheetId); On 2017/02/17 19:08:55, alph wrote: > nit: || null; It's already so in the function being called. https://codereview.chromium.org/2705433003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/css_tracker/CSSTrackerView.js:107: return Promise.all(styleSheetUsages.map(entry => this._populateSourceInfo(entry))).then(() => styleSheetUsages); On 2017/02/17 19:08:55, alph wrote: > nit: if you make _populateSourceInfo return entry, you won't need an extra > 'then'. done. https://codereview.chromium.org/2705433003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/css_tracker/CSSTrackerView.js:122: * @param {!CSSTracker.StyleSheetUsage} styleSheetUsage On 2017/02/17 19:08:55, alph wrote: > @return done. https://codereview.chromium.org/2705433003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/css_tracker/CSSTrackerView.js:160: unused += styleSheet.rules.reduce((count, rule) => rule.wasUsed ? count : count + 1); On 2017/02/17 19:08:55, alph wrote: > you need an initial value thanks, fixed!
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by caseq@chromium.org
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
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by caseq@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by caseq@chromium.org
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
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by caseq@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dgozman@chromium.org changed reviewers: + dgozman@chromium.org
https://codereview.chromium.org/2705433003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/css_tracker/CSSTrackerView.js (right): https://codereview.chromium.org/2705433003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/css_tracker/CSSTrackerView.js:84: var cssModel = mainTarget.model(SDK.CSSModel); While you are working on this, could you make it work for all models? We have a nice getter for them: SDK.targetManager.models(SDK.CSSModel).
CQ is committing da patch. Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487392397752340, "parent_rev": "009f63edd2aa777dd51cd48da3b7295636fe0235", "commit_rev": "d6fa448cd41d51f3b637f5bcfa4d6aac5d13abb8"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: decouple model logic from presentation within CSSTrackerView This simplifies the rules usage and stylesheet processing logic in CSSTrackerView from the presentation code that lays a way for future splitting this view into several parts. This also groups rules by styleshetts early that simplifies processing in several places. ========== to ========== DevTools: decouple model logic from presentation within CSSTrackerView This simplifies the rules usage and stylesheet processing logic in CSSTrackerView from the presentation code that lays a way for future splitting this view into several parts. This also groups rules by styleshetts early that simplifies processing in several places. Review-Url: https://codereview.chromium.org/2705433003 Cr-Commit-Position: refs/heads/master@{#451429} Committed: https://chromium.googlesource.com/chromium/src/+/d6fa448cd41d51f3b637f5bcfa4d... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/d6fa448cd41d51f3b637f5bcfa4d... |