I would like to be able to look at the historical data for summary pages on perf.webkit.org. This becomes very useful when trying to understand decisions made based on previous performance statuses, and how performance changes over time. Ideally, this would have an interface that looks similar to what's used on Charts pages.
<rdar://problem/51761726>
Created attachment 372540 [details] Patch
Created attachment 372600 [details] Patch
New patch uses `const` instead of `var` in new code on public/v3/pages/summary-page.js.
Comment on attachment 372600 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372600&action=review > Websites/perf.webkit.org/public/v3/pages/charts-toolbar.js:2 > -class ChartsToolbar extends DomainControlToolbar { > +class ChartsToolbar extends SliderToolbar { SliderToolbar is a presentational name, not semantic; i.e. it doesn't tell us what it's sliding. How about ContinuousDomainControlToolbar or ContinuousDomainToolbar? > Websites/perf.webkit.org/public/v3/pages/charts-toolbar.js:-54 > - setStartTime(startTime) This function needs to be updated to force startTime to be at least one day. > Websites/perf.webkit.org/public/v3/pages/charts-toolbar.js:75 > + ${ super.htmlTemplate() } I don't think this super call is needed. There is no template defined in DomainControlToolbar. > Websites/perf.webkit.org/public/v3/pages/domain-control-toolbar.js:31 > - if (!numberOfDays) > + if (!numberOfDays && numberOfDays !== 0) A better way to check this is typeof(numberOfDays) != 'number'. > Websites/perf.webkit.org/public/v3/pages/slider-toolbar.js:22 > + this._labelSpan = this.content().querySelector('.day-count'); _labelSpan is a terrible variable name. We should rename it to something more descriptive like _dayCount. We also use id content attribute to refer to it instead of a class. > Websites/perf.webkit.org/public/v3/pages/slider-toolbar.js:38 > + setStartTime(startTime) Use the default argument: startTime = this._defaultNumberOfDays > Websites/perf.webkit.org/public/v3/pages/slider-toolbar.js:59 > + this._editor.style.opacity = 1; Ugh... this is pretty bad. We should be simply enqueuing this element to be re-rendered instead. I guess we can leave it as is for now. > Websites/perf.webkit.org/public/v3/pages/slider-toolbar.js:102 > + <ul class="buttoned-toolbar"> Nit: We should outdent these markups by 4 space since this line should appear exactly 4 spaces to the right of the beginning of "return" above. In WebKit style guideline, each indentation is done by exactly 4 spaces, and it should never be aligned to anything else. > Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:16 > + this._labelSpan.style.width = '1.25rem'; > + this._dayCountDescription.style.width = '4.5rem'; In general, these kinds of style updates are best done via setting class names. > Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:27 > + { A better design is to replace the content of <span id="label"> in SliderToolbar. Here, you can do something like: super.render(); const shouldShowCurrent = !this._numberOfDays && !this._inTextMode; if (shouldShowCurrent) this.content('label').classList.add('current'); else this.content('label').classList.remove('current'); Then you'd hide day-count by setting `display: none` when the span matches `.current` > Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:29 > + if (this._numberOfDays === 0 && !this._inTextMode) { Nit: No curly braces around single statements. Nit: No comparison against 0. > Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:32 > + this._removeSliderTextFormattingForZeroDays(); Ditto. > Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:36 > + _enterTextMode(event) We should make _enterTextMode & _exitTextMode simply invoke this.enqueueToRender() instead. > Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:39 > + this._inTextMode = true; We should make the super class provide this functionality instead. But really, we should be setting `text-mode` class or something to avoid manually managing this state in JS. > Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:54 > + return `<label id="day-count-description" class="label"></label>`; This would generate incorrect markup. We should be having a label element inside another label element.
Comment on attachment 372600 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372600&action=review Thank you for the review comments! Applied most comments, and uploading a new patch now. Let's chat in-person to discuss CSS / styling on the editor and label text as I was having some trouble getting a coherent implementation together. >> Websites/perf.webkit.org/public/v3/pages/charts-toolbar.js:2 >> +class ChartsToolbar extends SliderToolbar { > > SliderToolbar is a presentational name, not semantic; i.e. it doesn't tell us what it's sliding. > How about ContinuousDomainControlToolbar or ContinuousDomainToolbar? Sure, will update to ContinuousDomainToolbar. >> Websites/perf.webkit.org/public/v3/pages/charts-toolbar.js:-54 >> - setStartTime(startTime) > > This function needs to be updated to force startTime to be at least one day. Updated patch adds a check for < 1, and forces to 1 if true. Please let me know if there's a cleaner way to do this for ChartsToolbar, given the new dependence on ContinousDomainToolbar. >> Websites/perf.webkit.org/public/v3/pages/charts-toolbar.js:75 >> + ${ super.htmlTemplate() } > > I don't think this super call is needed. There is no template defined in DomainControlToolbar. Due to how the inheritence structure changed, this is now calling to DomainControlToolbar (previously: SliderToolbar). And we do need the htmlTemplate from there, unless I'm misunderstanding something. >> Websites/perf.webkit.org/public/v3/pages/domain-control-toolbar.js:31 >> + if (!numberOfDays && numberOfDays !== 0) > > A better way to check this is typeof(numberOfDays) != 'number'. Updated. Note: Removed !numberOfDays check since it seemed redundant with the new one. >> Websites/perf.webkit.org/public/v3/pages/slider-toolbar.js:22 >> + this._labelSpan = this.content().querySelector('.day-count'); > > _labelSpan is a terrible variable name. We should rename it to something more descriptive like _dayCount. > We also use id content attribute to refer to it instead of a class. For what it's worth, this was just a migration from ChartsToolbar. I will make both requested changes. >> Websites/perf.webkit.org/public/v3/pages/slider-toolbar.js:38 >> + setStartTime(startTime) > > Use the default argument: startTime = this._defaultNumberOfDays Done. >> Websites/perf.webkit.org/public/v3/pages/slider-toolbar.js:102 >> + <ul class="buttoned-toolbar"> > > Nit: We should outdent these markups by 4 space since this line should appear > exactly 4 spaces to the right of the beginning of "return" above. > > In WebKit style guideline, each indentation is done by exactly 4 spaces, > and it should never be aligned to anything else. Understood, thank you. >> Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:27 >> + { > > A better design is to replace the content of <span id="label"> in SliderToolbar. > Here, you can do something like: > > super.render(); > const shouldShowCurrent = !this._numberOfDays && !this._inTextMode; > if (shouldShowCurrent) > this.content('label').classList.add('current'); > else > this.content('label').classList.remove('current'); > > Then you'd hide day-count by setting `display: none` when the span matches `.current` Let's chat in-person about this - my attempts at addressing your comments are coming out looking pretty bad compared to the current proposed implementation (readability / implementation-wise). >> Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:36 >> + _enterTextMode(event) > > We should make _enterTextMode & _exitTextMode simply invoke this.enqueueToRender() instead. Ah, great call. It doesn't seem like ContinuousDomainToolbar needs to use this.enqueueToRender(), so for now I'll do it in SummaryToolbar. Please let me know if you think it should be in ContinuousDomainToolbar instead. >> Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:39 >> + this._inTextMode = true; > > We should make the super class provide this functionality instead. > But really, we should be setting `text-mode` class or something to avoid manually managing this state in JS. Moved _inTextMode to ContinuousDomainToolbar. I can take a stab at a 'text-mode' class if you'd like, but would like to discuss more of exactly how that should look first if I do. >> Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:54 >> + return `<label id="day-count-description" class="label"></label>`; > > This would generate incorrect markup. We should be having a label element inside another label element. Thanks for the note. Will use span instead.
Created attachment 372867 [details] Patch
Comment on attachment 372867 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372867&action=review > Websites/perf.webkit.org/public/v3/pages/charts-toolbar.js:34 > + if (startTime < 1) > + startTime = 1; > + super.setStartTime(startTime); It's more succinct to just say: super.setStartTime(Math.max(1, startTime)); > Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:12 > + this._slider = this.content().querySelector('.slider'); I know you're just moving the code around but can we use the ID for slider? > Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:17 > + this._editor = this.content().querySelector('.editor'); Ditto. > Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:80 > + var numberOfDays = Math.round(Math.pow(parseFloat(this._slider.value), 3)); Use const? > Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:87 > + var rawNumber = Math.round(parseFloat(this._editor.value)); > + var numberOfDays = Math.max(this._minDayCount, Math.min(this._maxDayCount, rawNumber)); Ditto. > Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:95 > + var shouldUpdateState = event.type == 'change'; Ditto. > Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:96 > + if ((this.numberOfDays() != numberOfDays || shouldUpdateState) && this._numberOfDaysCallback) This code is so old... No use of LazilyEvaluatedFunction... > Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:100 > + static daysCountLabelText() { return ` days`; } No need for a space before "days". > Websites/perf.webkit.org/public/v3/pages/summary-page.js:34 > + this.toolbar().setNumberOfDays(0, true); In general, toolbar state shouldn't change from a page to page. If the user had selected 5 days ago, then we don't want to move that to "Current" just because the user navigated to a new summary page. > Websites/perf.webkit.org/public/v3/pages/summary-page.js:35 > + this.toolbar().enqueueToRender(); This isn't right. Toolbar should be enqueing itself to render on its own upon state transition. > Websites/perf.webkit.org/public/v3/pages/summary-page.js:36 > + this.toolbar().setNumberOfDaysCallback(this.setNumberOfDaysFromToolbar.bind(this)) Ugh... this is bad. We shouldn't have to do this but we'd have to for now. Also, the toolbar isn't set until super.open is called so this code isn't right. > Websites/perf.webkit.org/public/v3/pages/summary-page.js:42 > + updateAndRenderForRange(days) > + { We should be getting the number of days out of the toolbar instead. Also, this function never render anything. A better name would be _fetchForNumberOfDays > Websites/perf.webkit.org/public/v3/pages/summary-page.js:45 > + var oneDay = 24 * 3600 * 1000; > + var timeRange = [current - oneDay - (days * oneDay), current - (days * oneDay)]; Use const? > Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:7 > + this._minDayCount = 0; > + this._maxDayCount = 366; We don't want to be reaching into super class' instance variable like this. Probably better to add an explicit setter like _setAllowedDayCountRange or something. > Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:13 > + _addSliderTextFormattingForZeroDays() { > + this._dayCountSpan.textContent = ''; Let's discuss this in person.
Comment on attachment 372867 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372867&action=review Addressed most comments. Will chat in-person about styling, potential query params on Summary pages, and preserving slider state between Charts <-> Summary toolbars. >> Websites/perf.webkit.org/public/v3/pages/charts-toolbar.js:34 >> + super.setStartTime(startTime); > > It's more succinct to just say: super.setStartTime(Math.max(1, startTime)); Good catch. Fixed. >> Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:12 >> + this._slider = this.content().querySelector('.slider'); > > I know you're just moving the code around but can we use the ID for slider? Sure. Done. >> Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:17 >> + this._editor = this.content().querySelector('.editor'); > > Ditto. Done. >> Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:80 >> + var numberOfDays = Math.round(Math.pow(parseFloat(this._slider.value), 3)); > > Use const? Fixed. >> Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:87 >> + var numberOfDays = Math.max(this._minDayCount, Math.min(this._maxDayCount, rawNumber)); > > Ditto. Fixed. >> Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:95 >> + var shouldUpdateState = event.type == 'change'; > > Ditto. Fixed. >> Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:100 >> + static daysCountLabelText() { return ` days`; } > > No need for a space before "days". Fixed. >> Websites/perf.webkit.org/public/v3/pages/summary-page.js:34 >> + this.toolbar().setNumberOfDays(0, true); > > In general, toolbar state shouldn't change from a page to page. > If the user had selected 5 days ago, then we don't want to move that to "Current" just because the user navigated to a new summary page. I don't quite agree with this. The fundamental information being displayed is different between Charts and Summary - Charts is using 'days' to represent the # of days that should be shown in the view when looking at a list of charts. Summary is using 'days ago' to represent the state at a given # of days ago, and will always be evaluating only a 24 hour period. While both implementations share the same slider component, they represent fundamentally different sets of information. This does make me realize I forgot something though! Should modifying 'days ago' on the Summary page also modify the URL such that it can be sent to someone else? Similar to how we use the 'since' and 'zoom' query params for Charts as you modify the view? >> Websites/perf.webkit.org/public/v3/pages/summary-page.js:35 >> + this.toolbar().enqueueToRender(); > > This isn't right. Toolbar should be enqueing itself to render on its own upon state transition. Thank you. Fixed. >> Websites/perf.webkit.org/public/v3/pages/summary-page.js:36 >> + this.toolbar().setNumberOfDaysCallback(this.setNumberOfDaysFromToolbar.bind(this)) > > Ugh... this is bad. We shouldn't have to do this but we'd have to for now. > Also, the toolbar isn't set until super.open is called so this code isn't right. Addressed the second comment. >> Websites/perf.webkit.org/public/v3/pages/summary-page.js:42 >> + { > > We should be getting the number of days out of the toolbar instead. > Also, this function never render anything. > A better name would be _fetchForNumberOfDays Good points, thank you. Updated. >> Websites/perf.webkit.org/public/v3/pages/summary-page.js:45 >> + var timeRange = [current - oneDay - (days * oneDay), current - (days * oneDay)]; > > Use const? Fixed. >> Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:7 >> + this._maxDayCount = 366; > > We don't want to be reaching into super class' instance variable like this. > Probably better to add an explicit setter like _setAllowedDayCountRange or something. For my own understanding, what's the reason behind this? Made an attempt at this, let me know if you would like it done differently. >> Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:13 >> + this._dayCountSpan.textContent = ''; > > Let's discuss this in person. Yes, thank you.
Created attachment 372879 [details] Patch
(In reply to Dean Johnson from comment #9) > Comment on attachment 372867 [details] > Patch > > >> Websites/perf.webkit.org/public/v3/pages/summary-page.js:34 > >> + this.toolbar().setNumberOfDays(0, true); > > > > In general, toolbar state shouldn't change from a page to page. > > If the user had selected 5 days ago, then we don't want to move that to "Current" just because the user navigated to a new summary page. > > I don't quite agree with this. The fundamental information being displayed > is different between Charts and Summary - Charts is using 'days' to > represent the # of days that should be shown in the view when looking at a > list of charts. Summary is using 'days ago' to represent the state at a > given # of days ago, and will always be evaluating only a 24 hour period. ?? This number of days is specific to summary pages because your toolbar is only used by summary pages. It would mean that multiple summary pages would share the same number of days ago, not between summary pages and charts page. > This does make me realize I forgot something though! Should modifying 'days > ago' on the Summary page also modify the URL such that it can be sent to > someone else? Similar to how we use the 'since' and 'zoom' query params for > Charts as you modify the view? Yes, we should totally do that. See Charts page call this.scheduleUrlStateUpdate(). You'd need to modify serializeState and updateFromSerializedState.
(In reply to Ryosuke Niwa from comment #11) > (In reply to Dean Johnson from comment #9) > > Comment on attachment 372867 [details] > > Patch > > > > >> Websites/perf.webkit.org/public/v3/pages/summary-page.js:34 > > >> + this.toolbar().setNumberOfDays(0, true); > > > > > > In general, toolbar state shouldn't change from a page to page. > > > If the user had selected 5 days ago, then we don't want to move that to "Current" just because the user navigated to a new summary page. > > > > I don't quite agree with this. The fundamental information being displayed > > is different between Charts and Summary - Charts is using 'days' to > > represent the # of days that should be shown in the view when looking at a > > list of charts. Summary is using 'days ago' to represent the state at a > > given # of days ago, and will always be evaluating only a 24 hour period. > > ?? This number of days is specific to summary pages because your toolbar is > only used by summary pages. It would mean that multiple summary pages would > share the same number of days ago, not between summary pages and charts page. Oh! I misunderstood your comment. Will fix. > > > This does make me realize I forgot something though! Should modifying 'days > > ago' on the Summary page also modify the URL such that it can be sent to > > someone else? Similar to how we use the 'since' and 'zoom' query params for > > Charts as you modify the view? > > Yes, we should totally do that. See Charts page call > this.scheduleUrlStateUpdate(). You'd need to modify serializeState and > updateFromSerializedState. Will upload a new patch with this added, but not today.
Created attachment 393802 [details] Patch
Added support for 'since' query parameter using Ryosuke's suggestions. The URL should now allow links pointing to specific windows in time to be shared, and re-accessed. There is some support for the since keyword propagating between summary pages, but it doesn't completely work. For instance, if you've loaded a URL with a 'since' query parameter, when loading other Summary pages it will correctly use that parameter. However, if the toolbar is changed, then you try to visit another summary page, it resets back to the state it was on the first page's load. As far as I can tell this is caused by href's for each summary page embedding the initial load's 'since' query parameter. I spent a couple hours trying to diagnose and better understand how it *should* work, but couldn't come up with anything I felt confident wouldn't break. It should be noted that the request for the toolbar state to propagate across different pages of the same type (in this case, Summary) doesn't appear to be supported for existing 'Charts' views. This can be confirmed by: 1. Visiting https://perf.webkit.org/v3/#/dashboard/DOM?numberOfDays=7 2. Clicking "3M" in the toolbar to set the view window to 3 months. 3. Clicking on the "DOM" charts heading. 4. Observe the "3M" toolbar has been reset to "1W". Since most of the functionality for this feature is here, and we had many times this cycle where it would have been valuable, I'd like to work through any remaining code quality issues and land it without support for toolbar state propagating between intra-page navigations.
Comment on attachment 393802 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393802&action=review > Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:24 > + this._dayCountSpan = this.content('day-count'); Please don't creep element names into variable names like this. Just call it this._dayCountContainer or something. > Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:55 > + _setInputElementValue(value) This function name is misleading. It updates the slider as well as the input element. Maybe rename it to _updateSliderWithNumberOfDays? > Websites/perf.webkit.org/public/v3/pages/summary-page.js:37 > + var exactTime = null; > + if (state.since) > + exactTime = parseFloat(state.since); exactTime is a very confusing variable name. Why not just since or parsedSince? Also, just do: const since = state.since ? parseFloat(state.since) : null; > Websites/perf.webkit.org/public/v3/pages/summary-page.js:39 > + if (exactTime !== null) This isn't right. parseFloat can return NaN when state.since isn't a number. Check isNaN(since) instead. > Websites/perf.webkit.org/public/v3/pages/summary-page.js:49 > + const oneDay = 24 * 3600 * 1000; There is no need to re-compute all of this. this.toolbar().startTime() and this.toolbar().endTime() has this info. I think what you wanna do instead is to override endTime in SummaryToolbar. > Websites/perf.webkit.org/public/v3/pages/summary-page.js:60 > + if (this.toolbar()._defaultNumberOfDays == this.toolbar()._numberOfDays) { Nit: No curly braces for a single line statement. > Websites/perf.webkit.org/public/v3/pages/summary-page.js:61 > + return {} Missing a semicolon. There should be a space between { and }. > Websites/perf.webkit.org/public/v3/pages/summary-page.js:69 > + var startTime = Date.now() - (numberOfDays * oneDay); Use const? > Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:15 > + this._dayCountDescription.textContent = 'Current'; I'd use "Present" or "Now" instead. > Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:30 > + if (!this._numberOfDays && !this._inTextMode) > + this._addSliderTextFormattingForZeroDays(); > + else > + this._removeSliderTextFormattingForZeroDays(); We don't want to split functions like this in render functions. Instead, we want to have a single function which gets lazily evaluated based on arguments like this: this._lazilyRenderUpdateDayCountAndDescription(this._numberOfDays, this._inTextMode); See other examples of LazilyEvaluatedFunction.
Comment on attachment 393802 [details] Patch Can we also add some tests to browser-tests?
(In reply to Dean Johnson from comment #14) > > However, if the toolbar is changed, then you try to visit another summary > page, it resets back to the state it was on the first page's load. As far as > I can tell this is caused by href's for each summary page embedding the > initial load's 'since' query parameter. > > I spent a couple hours trying to diagnose and better understand how it > *should* work, but couldn't come up with anything I felt confident wouldn't > break. It should be noted that the request for the toolbar state to > propagate across different pages of the same type (in this case, Summary) > doesn't appear to be supported for existing 'Charts' views. You'd have to implement updateFromSerializedState on SummaryPage for that.
Created attachment 393896 [details] Patch
(In reply to Ryosuke Niwa from comment #15) > Comment on attachment 393802 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=393802&action=review > > > Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:24 > > + this._dayCountSpan = this.content('day-count'); > > Please don't creep element names into variable names like this. > Just call it this._dayCountContainer or something. Updated. > > > Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:55 > > + _setInputElementValue(value) Updated. > > This function name is misleading. It updates the slider as well as the input > element. > Maybe rename it to _updateSliderWithNumberOfDays? > > > Websites/perf.webkit.org/public/v3/pages/summary-page.js:37 > > + var exactTime = null; > > + if (state.since) > > + exactTime = parseFloat(state.since); > > exactTime is a very confusing variable name. Why not just since or > parsedSince? > Also, just do: const since = state.since ? parseFloat(state.since) : null; Thanks, updated. > > > Websites/perf.webkit.org/public/v3/pages/summary-page.js:39 > > + if (exactTime !== null) > > This isn't right. parseFloat can return NaN when state.since isn't a number. > Check isNaN(since) instead. Thanks, updated. > > > Websites/perf.webkit.org/public/v3/pages/summary-page.js:49 > > + const oneDay = 24 * 3600 * 1000; > > There is no need to re-compute all of this. this.toolbar().startTime() and > this.toolbar().endTime() has this info. > I think what you wanna do instead is to override endTime in SummaryToolbar. I don't think I understand the suggestion. Do you mean I should use this.toolbar()._present (in place of computing Date.now()) and this.toolbar()._millisecondsPerDay (in place of computing oneDay)? Updated patch with those, but please let me know if you meant something else. > > > Websites/perf.webkit.org/public/v3/pages/summary-page.js:60 > > + if (this.toolbar()._defaultNumberOfDays == this.toolbar()._numberOfDays) { Thanks, fixed. > > Nit: No curly braces for a single line statement. > > > Websites/perf.webkit.org/public/v3/pages/summary-page.js:61 > > + return {} > > Missing a semicolon. There should be a space between { and }. Fixed. > > > Websites/perf.webkit.org/public/v3/pages/summary-page.js:69 > > + var startTime = Date.now() - (numberOfDays * oneDay); > > Use const? Fixed. Also re-used this.toolbar()._present and this.toolbar()._millisecondsPerDay. > > > Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:15 > > + this._dayCountDescription.textContent = 'Current'; > > I'd use "Present" or "Now" instead. Fixed. > > > Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:30 > > + if (!this._numberOfDays && !this._inTextMode) > > + this._addSliderTextFormattingForZeroDays(); > > + else > > + this._removeSliderTextFormattingForZeroDays(); > > We don't want to split functions like this in render functions. > Instead, we want to have a single function which gets lazily evaluated based > on arguments like this: > this._lazilyRenderUpdateDayCountAndDescription(this._numberOfDays, > this._inTextMode); > See other examples of LazilyEvaluatedFunction. Updated. Thanks for pointing this out, I like the reduction in work from the new implementation much more! (In reply to Ryosuke Niwa from comment #17) > (In reply to Dean Johnson from comment #14) > > > > However, if the toolbar is changed, then you try to visit another summary > > page, it resets back to the state it was on the first page's load. As far as > > I can tell this is caused by href's for each summary page embedding the > > initial load's 'since' query parameter. > > > > I spent a couple hours trying to diagnose and better understand how it > > *should* work, but couldn't come up with anything I felt confident wouldn't > > break. It should be noted that the request for the toolbar state to > > propagate across different pages of the same type (in this case, Summary) > > doesn't appear to be supported for existing 'Charts' views. > > You'd have to implement updateFromSerializedState on SummaryPage for that. I don't think updateFromSerializedState would fix this, since the state of the current page doesn't seem to be propagated correctly. I implemented this another way, but am pretty sure it won't pass review. I'm happy to refactor my fix to better fit how other matches are working in PageRouter if you can give more explicit pointers in the right direction (examples of how it works between two pages with different routeNames would be very helpful!). I'm still working on writing new tests, but figured I'd put up the patch with all requested functionality since it's completed.
Comment on attachment 393896 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393896&action=review > Websites/perf.webkit.org/ChangeLog:71 > + I don't see a change log for `Websites/perf.webkit.org/public/v3/pages/page-router.js`, it may happen if you didn't change it in your first patch. I think it worthwhile to explain the change you made in `PageRouter.prototype.route` here. > Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:40 > + setStartTime(startTime=this._defaultNumberOfDays) I don't think we can set default value to an instance variable. class A { constructor(foo) { this._foo = 1; } print_foo(value=this._foo) { console.log(value) } }; a = new A(3); a.print_foo(); // <= showed "undefined" > Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:49 > + _setAllowedDayCountRange(minDayCount=this._defaultMinDayCount, maxDayCount=this._defaultMaxDayCount) ditto here > Websites/perf.webkit.org/public/v3/pages/page-router.js:64 > + if (this._currentPage && typeof(this._currentPage.fuzzyMatchesRouteName) === 'function' && > + this._currentPage.fuzzyMatchesRouteName(destinationPage.routeName()) && > + this._currentPage != destinationPage) { > + const currentPageState = this._currentPage.serializeState(); > + this._currentPage = destinationPage; > + destinationPage.open(currentPageState); > + } Could you explain a little bit more about this change? I think you mentioned in person that the state does not carry over between summary page navigations. I think there should be a better way to share state between summary pages. For example, all SummaryPages share same `SummaryToolbar` instance, may we can refer the state on SummaryPage.open from SummaryToolbar? > Websites/perf.webkit.org/public/v3/pages/summary-page.js:39 > + Nit: extra blank line? > Websites/perf.webkit.org/public/v3/pages/summary-page.js:44 > + this._fetchForNumberOfDaysAgo(this.toolbar().numberOfDays(), since); `_fetchForNumberOfDaysAgo` takes no argument per line 48. > Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:12 > + endTime() { Make this one liner? > Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:16 > + _renderUpdateDayCountAndDescription(numberOfDaysIsZero, inTextMode) { Nit: move '{' to separate line per https://webkit.org/code-style-guidelines/#braces > Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:23 > + } > + else { Use '} else {' instead per https://webkit.org/code-style-guidelines/#line-breaking
(In reply to dewei_zhu from comment #20) > Comment on attachment 393896 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=393896&action=review > > > Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:40 > > + setStartTime(startTime=this._defaultNumberOfDays) > > I don't think we can set default value to an instance variable. > class A { > constructor(foo) > { > this._foo = 1; > } > print_foo(value=this._foo) > { > console.log(value) > } > }; > a = new A(3); > a.print_foo(); // <= showed "undefined" I'm pretty sure that should work. Maybe someone broke that feature on trunk if you're seeing a failure on trunk? But there should be spaces around "=" here. This is not Python.
Comment on attachment 393896 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393896&action=review >>> Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:40 >>> + setStartTime(startTime=this._defaultNumberOfDays) >> >> I don't think we can set default value to an instance variable. >> class A { >> constructor(foo) >> { >> this._foo = 1; >> } >> print_foo(value=this._foo) >> { >> console.log(value) >> } >> }; >> a = new A(3); >> a.print_foo(); // <= showed "undefined" > > I'm pretty sure that should work. Maybe someone broke that feature on trunk if you're seeing a failure on trunk? But there should be spaces around "=" here. This is not Python. You are right, I didn't turn on "All" In console tab of Web Inspector.
(In reply to dewei_zhu from comment #20) > Comment on attachment 393896 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=393896&action=review > > > Websites/perf.webkit.org/ChangeLog:71 > > + > > I don't see a change log for > `Websites/perf.webkit.org/public/v3/pages/page-router.js`, it may happen if > you didn't change it in your first patch. > I think it worthwhile to explain the change you made in > `PageRouter.prototype.route` here. > > > Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:40 > > + setStartTime(startTime=this._defaultNumberOfDays) > > I don't think we can set default value to an instance variable. > class A { > constructor(foo) > { > this._foo = 1; > } > print_foo(value=this._foo) > { > console.log(value) > } > }; > a = new A(3); > a.print_foo(); // <= showed "undefined" > > > Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:49 > > + _setAllowedDayCountRange(minDayCount=this._defaultMinDayCount, maxDayCount=this._defaultMaxDayCount) > > ditto here > > > Websites/perf.webkit.org/public/v3/pages/page-router.js:64 > > + if (this._currentPage && typeof(this._currentPage.fuzzyMatchesRouteName) === 'function' && > > + this._currentPage.fuzzyMatchesRouteName(destinationPage.routeName()) && > > + this._currentPage != destinationPage) { > > + const currentPageState = this._currentPage.serializeState(); > > + this._currentPage = destinationPage; > > + destinationPage.open(currentPageState); > > + } > > Could you explain a little bit more about this change? I think you mentioned > in person that the state does not carry over between summary page > navigations. > I think there should be a better way to share state between summary pages. > For example, all SummaryPages share same `SummaryToolbar` instance, may we > can refer the state on SummaryPage.open from SummaryToolbar? > > > Websites/perf.webkit.org/public/v3/pages/summary-page.js:39 > > + > > Nit: extra blank line? > Fixed. > > Websites/perf.webkit.org/public/v3/pages/summary-page.js:44 > > + this._fetchForNumberOfDaysAgo(this.toolbar().numberOfDays(), since); > > `_fetchForNumberOfDaysAgo` takes no argument per line 48. > > > Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:12 > > + endTime() { > > Make this one liner? Fixed. > > > Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:16 > > + _renderUpdateDayCountAndDescription(numberOfDaysIsZero, inTextMode) { > > Nit: move '{' to separate line per > https://webkit.org/code-style-guidelines/#braces Fixed. Thank you. > > > Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:23 > > + } > > + else { > > Use '} else {' instead per > https://webkit.org/code-style-guidelines/#line-breaking Fixed. Thank you.
(In reply to dewei_zhu from comment #20) > Comment on attachment 393896 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=393896&action=review > > > Websites/perf.webkit.org/ChangeLog:71 > > + > > I don't see a change log for > `Websites/perf.webkit.org/public/v3/pages/page-router.js`, it may happen if > you didn't change it in your first patch. > I think it worthwhile to explain the change you made in > `PageRouter.prototype.route` here. > > > Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:40 > > + setStartTime(startTime=this._defaultNumberOfDays) > > I don't think we can set default value to an instance variable. > class A { > constructor(foo) > { > this._foo = 1; > } > print_foo(value=this._foo) > { > console.log(value) > } > }; > a = new A(3); > a.print_foo(); // <= showed "undefined" > > > Websites/perf.webkit.org/public/v3/pages/continuous-domain-toolbar.js:49 > > + _setAllowedDayCountRange(minDayCount=this._defaultMinDayCount, maxDayCount=this._defaultMaxDayCount) > > ditto here > > > Websites/perf.webkit.org/public/v3/pages/page-router.js:64 > > + if (this._currentPage && typeof(this._currentPage.fuzzyMatchesRouteName) === 'function' && > > + this._currentPage.fuzzyMatchesRouteName(destinationPage.routeName()) && > > + this._currentPage != destinationPage) { > > + const currentPageState = this._currentPage.serializeState(); > > + this._currentPage = destinationPage; > > + destinationPage.open(currentPageState); > > + } > > Could you explain a little bit more about this change? I think you mentioned > in person that the state does not carry over between summary page > navigations. > I think there should be a better way to share state between summary pages. > For example, all SummaryPages share same `SummaryToolbar` instance, may we > can refer the state on SummaryPage.open from SummaryToolbar? Sorry, I updated the commit message but not the ChangeLog. New patch has ChangeLog updated too. From ChangeLog: (PageRouter.prototype.route): Moving from one summary page to another summary does not currently propagate state, since routeName() between two summary pages is different (e.g. /summary/trunk, /summary/system). To get around this, we now check for a function called 'fuzzyMatchesRouteName' on _currentPage between navigations and use it to determine if the destinationPage should have its state propagated from _currentPage. Each 'Page' may implement their own fuzzyMatchesRouteName. I'm open to other ways of handling this if there are suggestions. > > > Websites/perf.webkit.org/public/v3/pages/summary-page.js:39 > > + > > Nit: extra blank line? > > > Websites/perf.webkit.org/public/v3/pages/summary-page.js:44 > > + this._fetchForNumberOfDaysAgo(this.toolbar().numberOfDays(), since); > > `_fetchForNumberOfDaysAgo` takes no argument per line 48. > > > Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:12 > > + endTime() { > > Make this one liner? > > > Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:16 > > + _renderUpdateDayCountAndDescription(numberOfDaysIsZero, inTextMode) { > > Nit: move '{' to separate line per > https://webkit.org/code-style-guidelines/#braces > > > Websites/perf.webkit.org/public/v3/pages/summary-toolbar.js:23 > > + } > > + else { > > Use '} else {' instead per > https://webkit.org/code-style-guidelines/#line-breaking
Created attachment 394128 [details] Patch
Comment on attachment 394128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394128&action=review > Websites/perf.webkit.org/public/v3/pages/page-router.js:63 > + if (this._currentPage && typeof(this._currentPage.fuzzyMatchesRouteName) === 'function' && > + this._currentPage.fuzzyMatchesRouteName(destinationPage.routeName()) && > + this._currentPage != destinationPage) { > + const currentPageState = this._currentPage.serializeState(); > + this._currentPage = destinationPage; > + destinationPage.open(currentPageState); I didn't go into detail, but I think if all summary pages share the same instance of toolbar, (which does in this case per your change in main.js), then we should favor the startTime in toolbar. Detailed code in the comment of summary-page.js. Ryosuke, would you prefer the one I mentioned above or what Dean is proposing, or you think there is a better way to achieve this. > Websites/perf.webkit.org/public/v3/pages/summary-page.js:34 > super.open(state); Here, we can do ` if (this.toolbar()) state.since = this.toolbar().startTime(); super.open(state); .... `` > Websites/perf.webkit.org/public/v3/pages/summary-page.js:35 > + var since = state.since ? parseFloat(state.since) : null; Use `const`?
Comment on attachment 394128 [details] Patch Clearing r? flag since this patch has been sitting over two years without a progress.