Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1574063003:
MD History: Add basic material design history cards and history items (Closed)
|
Created:
4 years, 11 months ago by yingran Modified:
4 years, 10 months ago CC:
chromium-reviews, Patrick Dubroy, pam+watch_chromium.org, arv+watch_chromium.org, vitalyp+closure_chromium.org, dbeam+watch-closure_chromium.org, jlklein+watch-closure_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD History: Add basic material design history cards and history items
This is the first of two patches for the start of MD History. This patch
includes elements for individual items and grouped items. A followup patch will
hook these elements into the page.
BUG=425625
Committed: https://crrev.com/bd322a00b660027df64b4e7cb2452e1daeeb334a
Cr-Commit-Position: refs/heads/master@{#372589}
Committed: https://crrev.com/94ef8fb7c4744b37b34f3d04045f65ab888afdc7
Cr-Commit-Position: refs/heads/master@{#372606}
Patch Set 1 #
Total comments: 66
Patch Set 2 : Addressed reviewer's comments #Patch Set 3 : Closure compile fixes #Patch Set 4 : Replaced with correct file names and links #
Total comments: 14
Patch Set 5 : Addressed reviewer's comments & changed default values of history result #
Total comments: 38
Patch Set 6 : Addressed reviewer's comments #
Total comments: 12
Patch Set 7 : Addressed reviewer's comments #
Total comments: 6
Patch Set 8 : removed 1 line #
Total comments: 6
Patch Set 9 : Addressed reviewer's comments #Patch Set 10 : Color changes #Patch Set 11 : PLS #Messages
Total messages: 52 (17 generated)
Description was changed from ========== First material history patch. BUG= ========== to ========== MD History: Add basic material design history cards and history items This is the first of two patches for the start of MD History. This patch includes elements for individual items and grouped items. A followup patch will hook these elements into the page. BUG=425625 ==========
yingran@google.com changed reviewers: + calamity@chromium.org, dbeam@chromium.org, tsergeant@chromium.org
Please take a look
Description was changed from ========== MD History: Add basic material design history cards and history items This is the first of two patches for the start of MD History. This patch includes elements for individual items and grouped items. A followup patch will hook these elements into the page. BUG=425625 ========== to ========== MD History: Add basic material design history cards and history items This is the first of two patches for the start of MD History. This patch includes elements for individual items and grouped items. A followup patch will hook these elements into the page. BUG=425625 ==========
let's start with this https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/compiled_resources.gyp (right): https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/compiled_resources.gyp:21: 'history-card-manager.js', these should be underscores instead of dashes (i.e. history_card_manager.js) https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/history-card-manager.html (right): https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.html:26: top: 0; does this work in RTL? https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.html:53: <paper-item i18n-content="moreFromSite" on-tap="searchDomain"> event handlers should be of the form: id="thing" on-action="onThingAction_" i.e. on-tap="onMoreFromSiteTap_" https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/history-card-manager.js (right): https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:13: value: [] should also probably be funtion() { return []; } https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:32: X_OFFSET: 30, can this be @private? https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:44: this.menuOpen = false; why not just always do this.menuOpen = false; and observe menuOpen for changes? https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:45: } no curlies https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:79: // are rendered. why is this necessary? https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:84: } no curlies https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:121: } no curlies https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:135: var lastIndex = this.historyDataByDay_.length - 1; could this be oldestDay or lastDay? https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:138: // pass by reference implementation. can you give an example of when this happens or what problem exhibits? https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:141: this.splice('historyDataByDay_', lastIndex, 1, { how is splice('thing_', this.thing_.length - 1, 1, ...) different than this.push('thing_', ...)? https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:156: scrollHandler: function(e) { make @private (being called from the template is not reason to make it public, IMO) https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:165: * of the window. @param https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:167: doInfiniteScroll_: function(e) { nit: loadMoreIfAtEnd_? "doInfiniteScroll_" makes very little sense to me as a name https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:169: var scrollElem = e.srcElement; srcElement -> target, but why are we using |e| at all? why not just use a reference to the element we know is the scroller? i.e this.$['infinite-list'] https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:172: scrollElem.scrollTop + scrollElem.clientHeight + scrollOffset) { this looks a lot like the downloads implementation... https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/history-card.html (right): https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card.html:8: <style> is there a reason why you're doing the styling inline vs in a separate .css file? https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card.html:24: border-left: 1px solid #888; border-left -> -webkit-border-start if it should be flipped in RTL https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card.html:26: margin-left: 77px; margin-left -> -webkit-margin-start if it should be flipped in RTL https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card.html:34: border-radius: 2px 2px 0 0; RTL? https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card.html:36: font-size: 14px; this should be expressed in % or em instead of px (because % / em scale when the default text size changes) https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card.html:50: </div> nit: <div id="date-accessed">{{historyDate}}</div> also, can this be [[historyDate]] instead? (1-way binding) https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/history-card.js (right): https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card.js:6: is: 'history-card', nit: \n https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card.js:17: value: [] this should be: value: function() { return []; }, otherwise it'll be shared between instances (I think?) https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card.js:31: (items[index].time - items[index + 1].time > BROWSING_GAP_TIME)); isn't this equivalent to return index + 1 < items.length && items[index].time - items[index + 1].time > BROWSING_GAP_TIME; https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/history-item.html (right): https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-item.html:62: margin-right: 16px; RTL https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-item.html:69: margin: 0 12px 0 2px; RTL https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-item.html:76: margin: 0 10px 0 20px; RTL https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-item.html:83: checked="{{selected}}"> indent off https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/history-item.js (right): https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-item.js:47: } nit: arguably \n between each property
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/compiled_resources.gyp (right): https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/compiled_resources.gyp:21: 'history-card-manager.js', On 2016/01/16 02:54:35, Dan Beam wrote: > these should be underscores instead of dashes (i.e. history_card_manager.js) Done. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/history-card-manager.html (right): https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.html:26: top: 0; On 2016/01/16 02:54:35, Dan Beam wrote: > does this work in RTL? yep- position_util,js handles the actual positioning. The top & left styles are set as absolute positioning requires initial values. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.html:53: <paper-item i18n-content="moreFromSite" on-tap="searchDomain"> On 2016/01/16 02:54:35, Dan Beam wrote: > event handlers should be of the form: > > id="thing" on-action="onThingAction_" > > i.e. on-tap="onMoreFromSiteTap_" Done. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/history-card-manager.js (right): https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:13: value: [] On 2016/01/16 02:54:35, Dan Beam wrote: > should also probably be funtion() { return []; } Done. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:32: X_OFFSET: 30, On 2016/01/16 02:54:35, Dan Beam wrote: > can this be @private? Done. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:44: this.menuOpen = false; On 2016/01/16 02:54:35, Dan Beam wrote: > why not just always do this.menuOpen = false; and observe menuOpen for changes? Done. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:45: } On 2016/01/16 02:54:36, Dan Beam wrote: > no curlies Done. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:79: // are rendered. On 2016/01/16 02:54:35, Dan Beam wrote: > why is this necessary? now that i test it - it doesn't seem to be necessary... https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:84: } On 2016/01/16 02:54:35, Dan Beam wrote: > no curlies Done. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:121: } On 2016/01/16 02:54:35, Dan Beam wrote: > no curlies Done. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:135: var lastIndex = this.historyDataByDay_.length - 1; On 2016/01/16 02:54:35, Dan Beam wrote: > could this be oldestDay or lastDay? Done. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:138: // pass by reference implementation. On 2016/01/16 02:54:35, Dan Beam wrote: > can you give an example of when this happens or what problem exhibits? Notify path only works if the oldValue!=newValue so polymer doesn't know that the value changes if we just modify the array (even with a notifyPath()). Example: So if I do for(var i = 0; i < historyItems.length; i++) this.historyDataByDay_[lastDay].historyItems.push(historyItems[i]); this.notifyPath('historyDataByDay_.' + lastDay + '.historyItems', this.historyDataByDay_[lastDay].historyItems); then the path is not properly notified since "this.historyDataByDay_[lastDay].historyItems" is still the same array. This caused confusion because we expected that modifying the elements of an array and then notifying would update the Polymer elements. Using array concat which creates a new array, we resolved the problem. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:141: this.splice('historyDataByDay_', lastIndex, 1, { On 2016/01/16 02:54:35, Dan Beam wrote: > how is splice('thing_', this.thing_.length - 1, 1, ...) different than > this.push('thing_', ...)? Done. this.push won't work because |historyItems| is a list so I need concat. We didn't know you could do sub path indexing at the time. Now it is changed to this.set() https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:156: scrollHandler: function(e) { On 2016/01/16 02:54:35, Dan Beam wrote: > make @private (being called from the template is not reason to make it public, > IMO) Done. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:165: * of the window. On 2016/01/16 02:54:35, Dan Beam wrote: > @param Removed param e since no longer needed as per comment below. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:167: doInfiniteScroll_: function(e) { On 2016/01/16 02:54:35, Dan Beam wrote: > nit: loadMoreIfAtEnd_? "doInfiniteScroll_" makes very little sense to me as a > name Done. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:169: var scrollElem = e.srcElement; On 2016/01/16 02:54:35, Dan Beam wrote: > srcElement -> target, but why are we using |e| at all? why not just use a > reference to the element we know is the scroller? i.e this.$['infinite-list'] Done. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:172: scrollElem.scrollTop + scrollElem.clientHeight + scrollOffset) { On 2016/01/16 02:54:35, Dan Beam wrote: > this looks a lot like the downloads implementation... :) https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/history-card.html (right): https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card.html:8: <style> On 2016/01/16 02:54:36, Dan Beam wrote: > is there a reason why you're doing the styling inline vs in a separate .css > file? We're following what is suggested by the Polymer style guide: https://www.polymer-project.org/1.0/docs/devguide/styling.html https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card.html:24: border-left: 1px solid #888; On 2016/01/16 02:54:36, Dan Beam wrote: > border-left -> -webkit-border-start if it should be flipped in RTL Done. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card.html:26: margin-left: 77px; On 2016/01/16 02:54:36, Dan Beam wrote: > margin-left -> -webkit-margin-start if it should be flipped in RTL Done. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card.html:34: border-radius: 2px 2px 0 0; On 2016/01/16 02:54:36, Dan Beam wrote: > RTL? Border radius also applies for top left & right in RTL https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card.html:36: font-size: 14px; On 2016/01/16 02:54:36, Dan Beam wrote: > this should be expressed in % or em instead of px (because % / em scale when the > default text size changes) Can we change all the font sizes for %/em in a separate follow up patch to make it a bit easier? It'll be hard to make the change across the two patches(me & hannah's) separately. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card.html:50: </div> On 2016/01/16 02:54:36, Dan Beam wrote: > nit: <div id="date-accessed">{{historyDate}}</div> > > also, can this be [[historyDate]] instead? (1-way binding) Done. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/history-card.js (right): https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card.js:6: is: 'history-card', On 2016/01/16 02:54:36, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card.js:17: value: [] On 2016/01/16 02:54:36, Dan Beam wrote: > this should be: > > value: function() { return []; }, > > otherwise it'll be shared between instances (I think?) Done. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card.js:31: (items[index].time - items[index + 1].time > BROWSING_GAP_TIME)); On 2016/01/16 02:54:36, Dan Beam wrote: > isn't this equivalent to > > return index + 1 < items.length && > items[index].time - items[index + 1].time > BROWSING_GAP_TIME; yep https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/history-item.html (right): https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-item.html:62: margin-right: 16px; On 2016/01/16 02:54:36, Dan Beam wrote: > RTL Done. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-item.html:69: margin: 0 12px 0 2px; On 2016/01/16 02:54:36, Dan Beam wrote: > RTL Done. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-item.html:76: margin: 0 10px 0 20px; On 2016/01/16 02:54:36, Dan Beam wrote: > RTL Done. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-item.html:83: checked="{{selected}}"> On 2016/01/16 02:54:36, Dan Beam wrote: > indent off Done. https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/history-item.js (right): https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-item.js:47: } On 2016/01/16 02:54:36, Dan Beam wrote: > nit: arguably \n between each property Done.
Ping dbeam. Could you please take another look?
On 2016/01/21 23:26:59, yingran wrote: > Ping dbeam. Could you please take another look? I enabled your hosts so you wouldn't be blocked on my OWNERs stamping ;)
https://codereview.chromium.org/1574063003/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/history_card.html (right): https://codereview.chromium.org/1574063003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/history_card.html:3: <link rel="import" href="chrome://resources/polymer/v1_0/iron-flex-layout/iron-flex-layout.html"> Nit: Move this up above paper-styles https://codereview.chromium.org/1574063003/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/history_card_manager.js (right): https://codereview.chromium.org/1574063003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/history_card_manager.js:156: */ @private https://codereview.chromium.org/1574063003/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/history_item.html (right): https://codereview.chromium.org/1574063003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/history_item.html:11: :host([starred]) #bookmark { Move this down below #bookmark so the two states are together. https://codereview.chromium.org/1574063003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/history_item.html:84: <paper-checkbox id="checkbox" on-tap="checkboxSelected" checkboxSelected -> onCheckboxSelected_ https://codereview.chromium.org/1574063003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/history_item.html:92: <paper-icon-button icon="more-vert" id="menu-button" on-tap="openMenu"> openMenu -> onMenuButtonTap_
On 2016/01/23 01:21:00, Dan Beam wrote: > On 2016/01/21 23:26:59, yingran wrote: > > Ping dbeam. Could you please take another look? > > I enabled your hosts so you wouldn't be blocked on my OWNERs stamping ;) Chris and I will review the changes in c/b/r/md_history. Can you please still review the changes in c/b/r/history and ui/webui/resources ?
https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/history-card-manager.js (right): https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:138: // pass by reference implementation. On 2016/01/18 05:40:28, yingran wrote: > On 2016/01/16 02:54:35, Dan Beam wrote: > > can you give an example of when this happens or what problem exhibits? > > Notify path only works if the oldValue!=newValue so polymer doesn't know that > the value changes if we just modify the array (even with a notifyPath()). > > Example: > So if I do > > for(var i = 0; i < historyItems.length; i++) > this.historyDataByDay_[lastDay].historyItems.push(historyItems[i]); > > this.notifyPath('historyDataByDay_.' + lastDay + '.historyItems', > this.historyDataByDay_[lastDay].historyItems); > > then the path is not properly notified since > "this.historyDataByDay_[lastDay].historyItems" is still the same array. > > This caused confusion because we expected that modifying the elements of an > array and then notifying would update the Polymer elements. Using array concat > which creates a new array, we resolved the problem. did you try: this.notifyPath('historyDataByDay_.' + lastDay + '.historyItems.*'); ? https://codereview.chromium.org/1574063003/diff/80001/chrome/browser/resource... File chrome/browser/resources/history/externs.js (right): https://codereview.chromium.org/1574063003/diff/80001/chrome/browser/resource... chrome/browser/resources/history/externs.js:23: * snippet: (string|undefined), can you just change the C++ so that these are more narrowly typed? number|undefined -> number, default: -1 (or some invalid number) string|undefined -> string, default: '' boolean|undefined -> boolean, default: false observers in polymer wont work until all listed properties !== undefined, so if you have: observers: ['fooChanged_(foo, bar, baz)'], and this.foo is undefined, and bar/baz change, the observer will never fire https://www.polymer-project.org/1.0/docs/devguide/properties.html#multi-prope... https://codereview.chromium.org/1574063003/diff/80001/chrome/browser/resource... chrome/browser/resources/history/externs.js:36: * hasSyncedResults: (boolean|undefined), can this just always be a boolean?
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/1574063003/diff/80001/chrome/browser/resource... File chrome/browser/resources/history/externs.js (right): https://codereview.chromium.org/1574063003/diff/80001/chrome/browser/resource... chrome/browser/resources/history/externs.js:23: * snippet: (string|undefined), On 2016/01/26 23:07:39, Dan Beam wrote: > can you just change the C++ so that these are more narrowly typed? > > number|undefined -> number, default: -1 (or some invalid number) > string|undefined -> string, default: '' > boolean|undefined -> boolean, default: false > > observers in polymer wont work until all listed properties !== undefined, so if > you have: > > observers: ['fooChanged_(foo, bar, baz)'], > > and this.foo is undefined, and bar/baz change, the observer will never fire > > https://www.polymer-project.org/1.0/docs/devguide/properties.html#multi-prope... Done. https://codereview.chromium.org/1574063003/diff/80001/chrome/browser/resource... chrome/browser/resources/history/externs.js:36: * hasSyncedResults: (boolean|undefined), On 2016/01/26 23:07:39, Dan Beam wrote: > can this just always be a boolean? Done. https://codereview.chromium.org/1574063003/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/history_card.html (right): https://codereview.chromium.org/1574063003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/history_card.html:3: <link rel="import" href="chrome://resources/polymer/v1_0/iron-flex-layout/iron-flex-layout.html"> On 2016/01/25 00:12:43, tsergeant wrote: > Nit: Move this up above paper-styles Done. https://codereview.chromium.org/1574063003/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/history_card_manager.js (right): https://codereview.chromium.org/1574063003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/history_card_manager.js:156: */ On 2016/01/25 00:12:43, tsergeant wrote: > @private Done. https://codereview.chromium.org/1574063003/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/history_item.html (right): https://codereview.chromium.org/1574063003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/history_item.html:11: :host([starred]) #bookmark { On 2016/01/25 00:12:43, tsergeant wrote: > Move this down below #bookmark so the two states are together. Done. https://codereview.chromium.org/1574063003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/history_item.html:84: <paper-checkbox id="checkbox" on-tap="checkboxSelected" On 2016/01/25 00:12:43, tsergeant wrote: > checkboxSelected -> onCheckboxSelected_ Done. https://codereview.chromium.org/1574063003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/history_item.html:92: <paper-icon-button icon="more-vert" id="menu-button" on-tap="openMenu"> On 2016/01/25 00:12:43, tsergeant wrote: > openMenu -> onMenuButtonTap_ Done.
https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/history/externs.js (right): https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/history/externs.js:36: * hasSyncedResults: (boolean|undefined), this is now only boolean, right? https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/history/history.js:123: if (result.hostFilteringBehavior != '') nit: just if (result.hostFilteringBehavior) https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_card.html (right): https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card.html:60: <template is="dom-if" if="{{needsTimeGap_(index, historyItem)}}"> i think many of these could be one-way bound with [[ ]] instead of {{ }}, but I'm too lazy to find out which ones https://www.polymer-project.org/1.0/docs/devguide/data-binding.html https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_card.js (right): https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card.js:13: }, nit: \n between props https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_card_manager.js (right): https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card_manager.js:14: }, nit: \n between props https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card_manager.js:57: this.menuOpen = false; nit: closeMenu() https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card_manager.js:72: * @param {Array<HistoryEntry>} results The new history results. nit: I think you want !Array<!HistoryEntry> https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card_manager.js:83: if (currentDate == undefined) nit: you could probably say !currentDate https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card_manager.js:94: if (currentDate != undefined) if (currentDate) https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card_manager.js:147: // Closes overflow menu on scroll. nit: Closes -> Close https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_item.html (right): https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_item.html:67: min-width: 36px; why both min-width and width? https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_item.html:98: nit: why \n here? https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_item.js (right): https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_item.js:6: is: 'history-item', nit: \n https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_item.js:75: 'url("chrome://favicon/size/16@2x/' + this.websiteUrl + '") 2x)'; can this use getFaviconImageSet? https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_item.js:89: }); nit: \n https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/browsing_history_handler.cc (right): https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/browsing_history_handler.cc:243: result->SetString("dateTimeOfDay", ""); general nit: std::string() is preferred over "" for stuff like this (they end up being similar depending on whether "small strings" compiler optimizations are on, but std::string() is basically guaranteed to never be slower, kind of like ++i vs i++) https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/browsing_history_handler.cc:258: } this seems like a slightly better overall pattern to avoid duplicating/possibly mistyping key names and possibly not setting std::string snippet; if (blah) { // ... maybe set snippet ... } result->SetString("snippet", snippet);
https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/history-card-manager.js (right): https://codereview.chromium.org/1574063003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/history-card-manager.js:138: // pass by reference implementation. On 2016/01/26 23:07:39, Dan Beam wrote: > On 2016/01/18 05:40:28, yingran wrote: > > On 2016/01/16 02:54:35, Dan Beam wrote: > > > can you give an example of when this happens or what problem exhibits? > > > > Notify path only works if the oldValue!=newValue so polymer doesn't know that > > the value changes if we just modify the array (even with a notifyPath()). > > > > Example: > > So if I do > > > > for(var i = 0; i < historyItems.length; i++) > > this.historyDataByDay_[lastDay].historyItems.push(historyItems[i]); > > > > this.notifyPath('historyDataByDay_.' + lastDay + '.historyItems', > > this.historyDataByDay_[lastDay].historyItems); > > > > then the path is not properly notified since > > "this.historyDataByDay_[lastDay].historyItems" is still the same array. > > > > This caused confusion because we expected that modifying the elements of an > > array and then notifying would update the Polymer elements. Using array concat > > which creates a new array, we resolved the problem. > > did you try: > > this.notifyPath('historyDataByDay_.' + lastDay + '.historyItems.*'); > > ? Notify path for 'historyDataByDay_.' + lastDay + '.historyItems', this.historyDataByDay_[lastDay].historyItems works if we remove the oldValue!==newValue check on _notifyPath(). this.notifyPath('historyDataByDay_.' + lastDay + '.historyItems.*') doesn't work with or without the check. https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/history/externs.js (right): https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/history/externs.js:36: * hasSyncedResults: (boolean|undefined), On 2016/01/27 03:07:30, Dan Beam wrote: > this is now only boolean, right? Done. Oops yea - my bad https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/history/history.js:123: if (result.hostFilteringBehavior != '') On 2016/01/27 03:07:30, Dan Beam wrote: > nit: just if (result.hostFilteringBehavior) Done. https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_card.html (right): https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card.html:60: <template is="dom-if" if="{{needsTimeGap_(index, historyItem)}}"> On 2016/01/27 03:07:30, Dan Beam wrote: > i think many of these could be one-way bound with [[ ]] instead of {{ }}, but > I'm too lazy to find out which ones > > https://www.polymer-project.org/1.0/docs/devguide/data-binding.html Done. https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_card.js (right): https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card.js:13: }, On 2016/01/27 03:07:30, Dan Beam wrote: > nit: \n between props Done. https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_card_manager.js (right): https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card_manager.js:14: }, On 2016/01/27 03:07:31, Dan Beam wrote: > nit: \n between props Done. https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card_manager.js:57: this.menuOpen = false; On 2016/01/27 03:07:31, Dan Beam wrote: > nit: closeMenu() Done. https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card_manager.js:72: * @param {Array<HistoryEntry>} results The new history results. On 2016/01/27 03:07:31, Dan Beam wrote: > nit: I think you want !Array<!HistoryEntry> Done. https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card_manager.js:83: if (currentDate == undefined) On 2016/01/27 03:07:31, Dan Beam wrote: > nit: you could probably say !currentDate Done. https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card_manager.js:94: if (currentDate != undefined) On 2016/01/27 03:07:31, Dan Beam wrote: > if (currentDate) Done. https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card_manager.js:147: // Closes overflow menu on scroll. On 2016/01/27 03:07:31, Dan Beam wrote: > nit: Closes -> Close Done. https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_item.html (right): https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_item.html:67: min-width: 36px; On 2016/01/27 03:07:31, Dan Beam wrote: > why both min-width and width? Done. https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_item.html:98: On 2016/01/27 03:07:31, Dan Beam wrote: > nit: why \n here? Done. https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_item.js (right): https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_item.js:6: is: 'history-item', On 2016/01/27 03:07:31, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_item.js:89: }); On 2016/01/27 03:07:31, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/browsing_history_handler.cc (right): https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/browsing_history_handler.cc:243: result->SetString("dateTimeOfDay", ""); On 2016/01/27 03:07:31, Dan Beam wrote: > general nit: std::string() is preferred over "" for stuff like this (they end up > being similar depending on whether "small strings" compiler optimizations are > on, but std::string() is basically guaranteed to never be slower, kind of like > ++i vs i++) Done. https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/browsing_history_handler.cc:258: } On 2016/01/27 03:07:31, Dan Beam wrote: > this seems like a slightly better overall pattern to avoid duplicating/possibly > mistyping key names and possibly not setting > > std::string snippet; > > if (blah) { > // ... maybe set snippet ... > } > > result->SetString("snippet", snippet); Done.
looking pretty gooood https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_card.js (right): https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card.js:13: }, On 2016/01/27 05:05:03, yingran wrote: > On 2016/01/27 03:07:30, Dan Beam wrote: > > nit: \n between props > > Done. (not yet done) https://codereview.chromium.org/1574063003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_card.html (right): https://codereview.chromium.org/1574063003/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card.html:12: padding: 0 24px 20px 24px; nit: padding: 0 24px 20px; https://codereview.chromium.org/1574063003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_card_manager.html (right): https://codereview.chromium.org/1574063003/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card_manager.html:24: left: 0; have you tried this in RTL? https://codereview.chromium.org/1574063003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_card_manager.js (right): https://codereview.chromium.org/1574063003/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card_manager.js:96: } nit: \n https://codereview.chromium.org/1574063003/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card_manager.js:153: this.loadMoreIfAtEnd_(); is loadMoreIfAtEnd_ only called from here? if so, can we just inline it here and remove the extra method name? https://codereview.chromium.org/1574063003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_item.js (right): https://codereview.chromium.org/1574063003/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_history/history_item.js:54: } can any of these attributes be private? (note: I would not make starred private because of reflectToAttribute) https://codereview.chromium.org/1574063003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/browsing_history_handler.cc (right): https://codereview.chromium.org/1574063003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/browsing_history_handler.cc:286: can we do the same pattern with blockedVisit and hostFilteringBehavior?
https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_card.js (right): https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card.js:13: }, On 2016/01/27 18:20:07, Dan Beam wrote: > On 2016/01/27 05:05:03, yingran wrote: > > On 2016/01/27 03:07:30, Dan Beam wrote: > > > nit: \n between props > > > > Done. > > (not yet done) Done. https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_item.html (right): https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_item.html:67: min-width: 36px; On 2016/01/27 03:07:31, Dan Beam wrote: > why both min-width and width? Actually needed both - just took a close look. Min-width is set to keep the icon from shrinking when the window becomes smaller. Width is set so that the icon does not expand past 36px with the flex layout.When width is removed, the icon width became 40px, and some icons didn't align with the rest. https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_item.js (right): https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_item.js:75: 'url("chrome://favicon/size/16@2x/' + this.websiteUrl + '") 2x)'; On 2016/01/27 03:07:31, Dan Beam wrote: > can this use getFaviconImageSet? > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... Done. https://codereview.chromium.org/1574063003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_card.html (right): https://codereview.chromium.org/1574063003/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card.html:12: padding: 0 24px 20px 24px; On 2016/01/27 18:20:07, Dan Beam wrote: > nit: padding: 0 24px 20px; Done. https://codereview.chromium.org/1574063003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_card_manager.html (right): https://codereview.chromium.org/1574063003/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card_manager.html:24: left: 0; On 2016/01/27 18:20:07, Dan Beam wrote: > have you tried this in RTL? Yep - position_util,js handles the actual positioning. The top & left styles are set as absolute positioning requires initial values. https://codereview.chromium.org/1574063003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_card_manager.js (right): https://codereview.chromium.org/1574063003/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card_manager.js:96: } On 2016/01/27 18:20:07, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/1574063003/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_history/history_card_manager.js:153: this.loadMoreIfAtEnd_(); On 2016/01/27 18:20:07, Dan Beam wrote: > is loadMoreIfAtEnd_ only called from here? if so, can we just inline it here > and remove the extra method name? Done. https://codereview.chromium.org/1574063003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_item.js (right): https://codereview.chromium.org/1574063003/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_history/history_item.js:54: } On 2016/01/27 18:20:07, Dan Beam wrote: > can any of these attributes be private? > > (note: I would not make starred private because of reflectToAttribute) Done. https://codereview.chromium.org/1574063003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/browsing_history_handler.cc (right): https://codereview.chromium.org/1574063003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/browsing_history_handler.cc:286: On 2016/01/27 18:20:07, Dan Beam wrote: > can we do the same pattern with blockedVisit and hostFilteringBehavior? Done.
can you `git add chrome/browser/resources/md_history/history.js` or remove this file from browser_resources.grd? https://codereview.chromium.org/1574063003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/md_history/compiled_resources.gyp (right): https://codereview.chromium.org/1574063003/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_history/compiled_resources.gyp:10: # ninja -C out_closure/Default why don't you want to check that this compiles continuously?
also, will any of the files you're adding be utilized if this CL were to land right now? I don't see any modifications to md_history/history.html :(
https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_item.html (right): https://codereview.chromium.org/1574063003/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_history/history_item.html:67: min-width: 36px; On 2016/01/28 00:51:00, yingran wrote: > On 2016/01/27 03:07:31, Dan Beam wrote: > > why both min-width and width? > > Actually needed both - just took a close look. > > Min-width is set to keep the icon from shrinking when the window becomes > smaller. Width is set so that the icon does not expand past 36px with the flex > layout.When width is removed, the icon width became 40px, and some icons didn't > align with the rest. you should probably be setting a min-width or flex-basis at a much higher level. downloads doesn't let the page get smaller than ~670px before causing a horizontal scrollbar, for instance. i tried to patch in this CL for more concrete guidance but didn't have much look trying to run this UI
If you patch this CL, and then patch the follow-up CL (https://codereview.chromium.org/1572383006/) on top of it, you should get a working version of the UI. Not having the first patch affect the page was a deliberate decision in how we split the two patches apart. https://codereview.chromium.org/1574063003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/md_history/compiled_resources.gyp (right): https://codereview.chromium.org/1574063003/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_history/compiled_resources.gyp:10: # ninja -C out_closure/Default On 2016/01/28 01:11:19, Dan Beam wrote: > why don't you want to check that this compiles continuously? Do you mean using the closure compile bot? I made that decision in early December, but at this stage I think we're familiar enough with it to enable continuous compilation.
for chrome/browser/resources/history and ui/webui https://codereview.chromium.org/1574063003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/md_history/compiled_resources.gyp (right): https://codereview.chromium.org/1574063003/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_history/compiled_resources.gyp:10: # ninja -C out_closure/Default On 2016/01/28 02:27:55, tsergeant wrote: > On 2016/01/28 01:11:19, Dan Beam wrote: > > why don't you want to check that this compiles continuously? > > Do you mean using the closure compile bot? yes > I made that decision in early > December, but at this stage I think we're familiar enough with it to enable > continuous compilation. lgtm if you fix this
https://codereview.chromium.org/1574063003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/md_history/compiled_resources.gyp (right): https://codereview.chromium.org/1574063003/diff/160001/chrome/browser/resourc... chrome/browser/resources/md_history/compiled_resources.gyp:10: # ninja -C out_closure/Default On 2016/01/28 20:35:18, Dan Beam wrote: > On 2016/01/28 02:27:55, tsergeant wrote: > > On 2016/01/28 01:11:19, Dan Beam wrote: > > > why don't you want to check that this compiles continuously? > > > > Do you mean using the closure compile bot? > > yes > > > I made that decision in early > > December, but at this stage I think we're familiar enough with it to enable > > continuous compilation. > > lgtm if you fix this It turns out that the closure compile doesn't actually pass with this patch alone due to missing history.js. It will pass correctly with the second patch (https://codereview.chromium.org/1572383006/) applied. The better solution would obviously be to fix it, but the two patches are otherwise ready to go, so we'd like to land this patch as-is and enable continuous compilation in the second patch, which should also land today.
One more thing to fix https://codereview.chromium.org/1574063003/diff/160001/chrome/browser/browser... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/1574063003/diff/160001/chrome/browser/browser... chrome/browser/browser_resources.grd:212: <include name="IDR_MD_HISTORY_HISTORY_JS" file="resources\md_history\history.js" type="BINDATA" /> You will still need to remove this, which will prevent this patch from compiling.
A couple more things (almost there!) https://codereview.chromium.org/1574063003/diff/180001/chrome/browser/resourc... File chrome/browser/resources/history/externs.js (right): https://codereview.chromium.org/1574063003/diff/180001/chrome/browser/resourc... chrome/browser/resources/history/externs.js:12: * chrome/browser/ui/webui/history_ui.cc: While you're here, can you change this out of date filename? chrome/browser/ui/webui/browsing_history_handler.cc https://codereview.chromium.org/1574063003/diff/180001/chrome/browser/resourc... chrome/browser/resources/history/externs.js:33: * chrome/browser/ui/webui/history_ui.cc: Change this as well https://codereview.chromium.org/1574063003/diff/180001/ui/webui/resources/js/... File ui/webui/resources/js/cr/ui/position_util.js (right): https://codereview.chromium.org/1574063003/diff/180001/ui/webui/resources/js/... ui/webui/resources/js/cr/ui/position_util.js:229: * @param {cr.ui.AnchorType} opt_anchorType The type of anchoring we want. You'll need to mark this as optional for this to closure compile correctly (other projects use the 3 argument version and will fail to compile otherwise): {cr.ui.AnchorType=}
https://codereview.chromium.org/1574063003/diff/160001/chrome/browser/browser... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/1574063003/diff/160001/chrome/browser/browser... chrome/browser/browser_resources.grd:212: <include name="IDR_MD_HISTORY_HISTORY_JS" file="resources\md_history\history.js" type="BINDATA" /> On 2016/01/29 02:52:16, tsergeant wrote: > You will still need to remove this, which will prevent this patch from > compiling. Done. https://codereview.chromium.org/1574063003/diff/180001/chrome/browser/resourc... File chrome/browser/resources/history/externs.js (right): https://codereview.chromium.org/1574063003/diff/180001/chrome/browser/resourc... chrome/browser/resources/history/externs.js:12: * chrome/browser/ui/webui/history_ui.cc: On 2016/01/29 03:27:19, tsergeant wrote: > While you're here, can you change this out of date filename? > > chrome/browser/ui/webui/browsing_history_handler.cc Done. https://codereview.chromium.org/1574063003/diff/180001/chrome/browser/resourc... chrome/browser/resources/history/externs.js:33: * chrome/browser/ui/webui/history_ui.cc: On 2016/01/29 03:27:19, tsergeant wrote: > Change this as well Done. https://codereview.chromium.org/1574063003/diff/180001/ui/webui/resources/js/... File ui/webui/resources/js/cr/ui/position_util.js (right): https://codereview.chromium.org/1574063003/diff/180001/ui/webui/resources/js/... ui/webui/resources/js/cr/ui/position_util.js:229: * @param {cr.ui.AnchorType} opt_anchorType The type of anchoring we want. On 2016/01/29 03:27:19, tsergeant wrote: > You'll need to mark this as optional for this to closure compile correctly > (other projects use the 3 argument version and will fail to compile otherwise): > > {cr.ui.AnchorType=} Done.
lgtm!
lgtm without running on closure bots for now
The CQ bit was checked by tsergeant@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1574063003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1574063003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by yingran@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tsergeant@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1574063003/#ps220001 (title: "Color changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1574063003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1574063003/220001
Message was sent while issue was closed.
Description was changed from ========== MD History: Add basic material design history cards and history items This is the first of two patches for the start of MD History. This patch includes elements for individual items and grouped items. A followup patch will hook these elements into the page. BUG=425625 ========== to ========== MD History: Add basic material design history cards and history items This is the first of two patches for the start of MD History. This patch includes elements for individual items and grouped items. A followup patch will hook these elements into the page. BUG=425625 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== MD History: Add basic material design history cards and history items This is the first of two patches for the start of MD History. This patch includes elements for individual items and grouped items. A followup patch will hook these elements into the page. BUG=425625 ========== to ========== MD History: Add basic material design history cards and history items This is the first of two patches for the start of MD History. This patch includes elements for individual items and grouped items. A followup patch will hook these elements into the page. BUG=425625 Committed: https://crrev.com/bd322a00b660027df64b4e7cb2452e1daeeb334a Cr-Commit-Position: refs/heads/master@{#372589} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/bd322a00b660027df64b4e7cb2452e1daeeb334a Cr-Commit-Position: refs/heads/master@{#372589}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:240001) has been created in https://codereview.chromium.org/1654703002/ by calamity@chromium.org. The reason for reverting is: Broke the closure compile..
Message was sent while issue was closed.
Patchset #11 (id:240001) has been deleted
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:220001) has been created in https://codereview.chromium.org/1650993002/ by calamity@chromium.org. The reason for reverting is: Broke the closure compile on the tree builders..
Message was sent while issue was closed.
Description was changed from ========== MD History: Add basic material design history cards and history items This is the first of two patches for the start of MD History. This patch includes elements for individual items and grouped items. A followup patch will hook these elements into the page. BUG=425625 Committed: https://crrev.com/bd322a00b660027df64b4e7cb2452e1daeeb334a Cr-Commit-Position: refs/heads/master@{#372589} ========== to ========== MD History: Add basic material design history cards and history items This is the first of two patches for the start of MD History. This patch includes elements for individual items and grouped items. A followup patch will hook these elements into the page. BUG=425625 Committed: https://crrev.com/bd322a00b660027df64b4e7cb2452e1daeeb334a Cr-Commit-Position: refs/heads/master@{#372589} ==========
The CQ bit was checked by yingran@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tsergeant@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1574063003/#ps260001 (title: "PLS")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1574063003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1574063003/260001
Message was sent while issue was closed.
Description was changed from ========== MD History: Add basic material design history cards and history items This is the first of two patches for the start of MD History. This patch includes elements for individual items and grouped items. A followup patch will hook these elements into the page. BUG=425625 Committed: https://crrev.com/bd322a00b660027df64b4e7cb2452e1daeeb334a Cr-Commit-Position: refs/heads/master@{#372589} ========== to ========== MD History: Add basic material design history cards and history items This is the first of two patches for the start of MD History. This patch includes elements for individual items and grouped items. A followup patch will hook these elements into the page. BUG=425625 Committed: https://crrev.com/bd322a00b660027df64b4e7cb2452e1daeeb334a Cr-Commit-Position: refs/heads/master@{#372589} ==========
Message was sent while issue was closed.
Committed patchset #11 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== MD History: Add basic material design history cards and history items This is the first of two patches for the start of MD History. This patch includes elements for individual items and grouped items. A followup patch will hook these elements into the page. BUG=425625 Committed: https://crrev.com/bd322a00b660027df64b4e7cb2452e1daeeb334a Cr-Commit-Position: refs/heads/master@{#372589} ========== to ========== MD History: Add basic material design history cards and history items This is the first of two patches for the start of MD History. This patch includes elements for individual items and grouped items. A followup patch will hook these elements into the page. BUG=425625 Committed: https://crrev.com/bd322a00b660027df64b4e7cb2452e1daeeb334a Cr-Commit-Position: refs/heads/master@{#372589} Committed: https://crrev.com/94ef8fb7c4744b37b34f3d04045f65ab888afdc7 Cr-Commit-Position: refs/heads/master@{#372606} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/94ef8fb7c4744b37b34f3d04045f65ab888afdc7 Cr-Commit-Position: refs/heads/master@{#372606} |
