Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1928243002:
MD History: Only load needed paper-icon-buttons instead of whole set of icons (Closed)
|
Created:
4 years, 7 months ago by lshang Modified:
4 years, 7 months ago CC:
chromium-reviews, Patrick Dubroy, michaelpg+watch-md-ui_chromium.org, dbeam+watch-history_chromium.org, pam+watch_chromium.org, arv+watch_chromium.org, chrome-apps-syd-reviews_chromium.org, Dan Beam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD History: Only load needed paper-icon-buttons instead of whole set of icons
Use common icons in cr_elements/icons.html which uses polyicon to pick out needed
paper-icon-buttons and make icon sets that contain svg icons for those icons,
instead of load the whole set of icons.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/c84812a9ef3ff011616640084a8d1bb4b2caeac6
Cr-Commit-Position: refs/heads/master@{#393439}
Patch Set 1 : #Patch Set 2 : change cr_search_field icons #
Total comments: 7
Patch Set 3 : rebase #Patch Set 4 : use cr-icons in md_history #Patch Set 5 : format #
Total comments: 8
Patch Set 6 : update after rebase & addressing review comments #Patch Set 7 : remove history_icons.html #Patch Set 8 : change more icons #
Messages
Total messages: 34 (14 generated)
Patchset #1 (id:1) has been deleted
lshang@chromium.org changed reviewers: + tsergeant@chromium.org
PTAL thanks!
tsergeant@chromium.org changed reviewers: + dbeam@chromium.org
md_history lgtm. +dbeam@ for cr_elements https://codereview.chromium.org/1928243002/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_search_field/cr_search_icons.html (right): https://codereview.chromium.org/1928243002/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_search_field/cr_search_icons.html:5: <iron-iconset-svg size="20" name="cr-search-icons"> You could maybe make this a general shared set of `cr-icons` for all the cr_elements? dbeam@, what do you think?
dbeam@chromium.org changed reviewers: + michaelpg@chromium.org
i think it makes more sense for michaelpg@ to look at this as he's been thinking about SVG bundling a bit lately
NLGTM +1 for doing this, but the sizing looks wrong. Does it look OK when you build it? I'll upload a patch tomorrow morning which does things in a way I'll approve of ;-) https://codereview.chromium.org/1928243002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_history/history_icons.html (right): https://codereview.chromium.org/1928243002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_history/history_icons.html:4: <iron-iconset-svg size="20" name="history-icons"> 20? The SVG you copied is 24px so this is going to be clipped: https://jsfiddle.net/03yokgjt/1/ https://codereview.chromium.org/1928243002/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_search_field/cr_search_icons.html (right): https://codereview.chromium.org/1928243002/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_search_field/cr_search_icons.html:5: <iron-iconset-svg size="20" name="cr-search-icons"> On 2016/05/02 16:46:46, tsergeant (MTV) wrote: > You could maybe make this a general shared set of `cr-icons` for all the > cr_elements? Yes, I'd put this in cr_elements/cr_icons.html. We'll include a few other common icons. I have a CL I will put up tomorrow that does this (remote desktop is failing tonight) that you can then rebase this off of. > > dbeam@, what do you think?
On 2016/05/03 02:44:34, michaelpg wrote: > NLGTM "no lgtm"? > > +1 for doing this, but the sizing looks wrong. Does it look OK when you build > it? > > I'll upload a patch tomorrow morning which does things in a way I'll approve of > ;-) > > https://codereview.chromium.org/1928243002/diff/40001/chrome/browser/resource... > File chrome/browser/resources/md_history/history_icons.html (right): > > https://codereview.chromium.org/1928243002/diff/40001/chrome/browser/resource... > chrome/browser/resources/md_history/history_icons.html:4: <iron-iconset-svg > size="20" name="history-icons"> > 20? The SVG you copied is 24px so this is going to be clipped: > https://jsfiddle.net/03yokgjt/1/ > > https://codereview.chromium.org/1928243002/diff/40001/ui/webui/resources/cr_e... > File ui/webui/resources/cr_elements/cr_search_field/cr_search_icons.html > (right): > > https://codereview.chromium.org/1928243002/diff/40001/ui/webui/resources/cr_e... > ui/webui/resources/cr_elements/cr_search_field/cr_search_icons.html:5: > <iron-iconset-svg size="20" name="cr-search-icons"> > On 2016/05/02 16:46:46, tsergeant (MTV) wrote: > > You could maybe make this a general shared set of `cr-icons` for all the > > cr_elements? > > Yes, I'd put this in cr_elements/cr_icons.html. We'll include a few other common > icons. I have a CL I will put up tomorrow that does this (remote desktop is > failing tonight) that you can then rebase this off of. > > > > > dbeam@, what do you think?
On 2016/05/03 02:45:06, michaelpg wrote: > On 2016/05/03 02:44:34, michaelpg wrote: > > NLGTM > > "no lgtm"? "not lgtm"? > > > > > +1 for doing this, but the sizing looks wrong. Does it look OK when you build > > it? > > > > I'll upload a patch tomorrow morning which does things in a way I'll approve > of > > ;-) > > > > > https://codereview.chromium.org/1928243002/diff/40001/chrome/browser/resource... > > File chrome/browser/resources/md_history/history_icons.html (right): > > > > > https://codereview.chromium.org/1928243002/diff/40001/chrome/browser/resource... > > chrome/browser/resources/md_history/history_icons.html:4: <iron-iconset-svg > > size="20" name="history-icons"> > > 20? The SVG you copied is 24px so this is going to be clipped: > > https://jsfiddle.net/03yokgjt/1/ > > > > > https://codereview.chromium.org/1928243002/diff/40001/ui/webui/resources/cr_e... > > File ui/webui/resources/cr_elements/cr_search_field/cr_search_icons.html > > (right): > > > > > https://codereview.chromium.org/1928243002/diff/40001/ui/webui/resources/cr_e... > > ui/webui/resources/cr_elements/cr_search_field/cr_search_icons.html:5: > > <iron-iconset-svg size="20" name="cr-search-icons"> > > On 2016/05/02 16:46:46, tsergeant (MTV) wrote: > > > You could maybe make this a general shared set of `cr-icons` for all the > > > cr_elements? > > > > Yes, I'd put this in cr_elements/cr_icons.html. We'll include a few other > common > > icons. I have a CL I will put up tomorrow that does this (remote desktop is > > failing tonight) that you can then rebase this off of. > > > > > > > > dbeam@, what do you think?
On 2016/05/03 02:45:42, michaelpg wrote: > On 2016/05/03 02:45:06, michaelpg wrote: > > On 2016/05/03 02:44:34, michaelpg wrote: > > > NLGTM > > > > "no lgtm"? > > "not lgtm"? ahh there we go. > > > > > > > > > +1 for doing this, but the sizing looks wrong. Does it look OK when you > build > > > it? > > > > > > I'll upload a patch tomorrow morning which does things in a way I'll approve > > of > > > ;-) > > > > > > > > > https://codereview.chromium.org/1928243002/diff/40001/chrome/browser/resource... > > > File chrome/browser/resources/md_history/history_icons.html (right): > > > > > > > > > https://codereview.chromium.org/1928243002/diff/40001/chrome/browser/resource... > > > chrome/browser/resources/md_history/history_icons.html:4: <iron-iconset-svg > > > size="20" name="history-icons"> > > > 20? The SVG you copied is 24px so this is going to be clipped: > > > https://jsfiddle.net/03yokgjt/1/ > > > > > > > > > https://codereview.chromium.org/1928243002/diff/40001/ui/webui/resources/cr_e... > > > File ui/webui/resources/cr_elements/cr_search_field/cr_search_icons.html > > > (right): > > > > > > > > > https://codereview.chromium.org/1928243002/diff/40001/ui/webui/resources/cr_e... > > > ui/webui/resources/cr_elements/cr_search_field/cr_search_icons.html:5: > > > <iron-iconset-svg size="20" name="cr-search-icons"> > > > On 2016/05/02 16:46:46, tsergeant (MTV) wrote: > > > > You could maybe make this a general shared set of `cr-icons` for all the > > > > cr_elements? > > > > > > Yes, I'd put this in cr_elements/cr_icons.html. We'll include a few other > > common > > > icons. I have a CL I will put up tomorrow that does this (remote desktop is > > > failing tonight) that you can then rebase this off of. > > > > > > > > > > > dbeam@, what do you think?
https://codereview.chromium.org/1928243002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_history/history_icons.html (right): https://codereview.chromium.org/1928243002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_history/history_icons.html:1: <link rel="import" href="chrome://resources/polymer/v1_0/iron-icon/iron-icon.html"> Unused import. https://codereview.chromium.org/1928243002/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_search_field/cr_search_icons.html (right): https://codereview.chromium.org/1928243002/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_search_field/cr_search_icons.html:5: <iron-iconset-svg size="20" name="cr-search-icons"> On 2016/05/03 02:44:34, michaelpg wrote: > On 2016/05/02 16:46:46, tsergeant (MTV) wrote: > > You could maybe make this a general shared set of `cr-icons` for all the > > cr_elements? > > Yes, I'd put this in cr_elements/cr_icons.html. We'll include a few other common > icons. I have a CL I will put up tomorrow that does this (remote desktop is > failing tonight) that you can then rebase this off of. > > > > > dbeam@, what do you think? > This is done now, please rebase.
On 2016/05/05 15:04:52, michaelpg wrote: > https://codereview.chromium.org/1928243002/diff/40001/chrome/browser/resource... > File chrome/browser/resources/md_history/history_icons.html (right): > > https://codereview.chromium.org/1928243002/diff/40001/chrome/browser/resource... > chrome/browser/resources/md_history/history_icons.html:1: <link rel="import" > href="chrome://resources/polymer/v1_0/iron-icon/iron-icon.html"> > Unused import. > > https://codereview.chromium.org/1928243002/diff/40001/ui/webui/resources/cr_e... > File ui/webui/resources/cr_elements/cr_search_field/cr_search_icons.html > (right): > > https://codereview.chromium.org/1928243002/diff/40001/ui/webui/resources/cr_e... > ui/webui/resources/cr_elements/cr_search_field/cr_search_icons.html:5: > <iron-iconset-svg size="20" name="cr-search-icons"> > On 2016/05/03 02:44:34, michaelpg wrote: > > On 2016/05/02 16:46:46, tsergeant (MTV) wrote: > > > You could maybe make this a general shared set of `cr-icons` for all the > > > cr_elements? > > > > Yes, I'd put this in cr_elements/cr_icons.html. We'll include a few other > common > > icons. I have a CL I will put up tomorrow that does this (remote desktop is > > failing tonight) that you can then rebase this off of. > > > > > > > > dbeam@, what do you think? > > > > This is done now, please rebase. OK, I'll rebase this and use the shared icons:-)
Description was changed from ========== MD History: Only load needed paper-icon-buttons instead of whole set of icons Use polyicon to pick out needed paper-icon-buttons for md_history and make icon sets that contain svg icons for those icons, instead of load the whole set of icons. ========== to ========== MD History: Only load needed paper-icon-buttons instead of whole set of icons Use polyicon to pick out needed paper-icon-buttons for md_history and make icon sets that contain svg icons for those icons, instead of load the whole set of icons. ==========
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
Description was changed from ========== MD History: Only load needed paper-icon-buttons instead of whole set of icons Use polyicon to pick out needed paper-icon-buttons for md_history and make icon sets that contain svg icons for those icons, instead of load the whole set of icons. ========== to ========== MD History: Only load needed paper-icon-buttons instead of whole set of icons Use polyicon to pick out needed paper-icon-buttons for md_history and make icon sets that contain svg icons for those icons, instead of load the whole set of icons. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Hi tsergeant@ michaelpg@, I have rebased this CL and changed to use the common icons in cr-icons. Couly you please take a look at this again? thanks! https://codereview.chromium.org/1928243002/diff/40001/chrome/browser/resource... File chrome/browser/resources/md_history/history_icons.html (right): https://codereview.chromium.org/1928243002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_history/history_icons.html:1: <link rel="import" href="chrome://resources/polymer/v1_0/iron-icon/iron-icon.html"> On 2016/05/05 15:04:52, michaelpg wrote: > Unused import. Done. https://codereview.chromium.org/1928243002/diff/40001/chrome/browser/resource... chrome/browser/resources/md_history/history_icons.html:4: <iron-iconset-svg size="20" name="history-icons"> On 2016/05/03 02:44:34, michaelpg wrote: > 20? The SVG you copied is 24px so this is going to be clipped: > https://jsfiddle.net/03yokgjt/1/ Done.
We landed another change a couple days ago just to simplify naming conventions, I pointed out the differences. You may not need the custom history-icons file anymore unless you add more icons. https://codereview.chromium.org/1928243002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_icons.html (right): https://codereview.chromium.org/1928243002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_history/history_icons.html:3: <iron-iconset-svg size="24" name="history-icons"> optional nit: i'm trying to start a convention of naming the file just "icons.html" and the iconset just "history", so the usage is a bit shorter: chrome://md-history/icons.html <iron-icon icon="history:star"></iron-icon> https://codereview.chromium.org/1928243002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_history/history_icons.html:6: <g id="star"><path d="M12 17.27L18.18 21l-1.64-7.03L22 9.24l-7.19-.61L12 2 9.19 8.63 2 9.24l5.46 4.73L5.82 21z"></path></g> you can use the one in cr_elements/icons.html, just make it not chromeos-only anymore https://codereview.chromium.org/1928243002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_item.html (right): https://codereview.chromium.org/1928243002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_history/history_item.html:1: <link rel="import" href="chrome://resources/cr_elements/cr_icons.html"> now "icons.html" https://codereview.chromium.org/1928243002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_history/history_item.html:135: <paper-icon-button icon="cr-icons:more-vert" id="menu-button" now "cr:more-vert"
Sorry for missing the update lately. I've updated the name. PTAL thanks! https://codereview.chromium.org/1928243002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_icons.html (right): https://codereview.chromium.org/1928243002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_history/history_icons.html:3: <iron-iconset-svg size="24" name="history-icons"> On 2016/05/11 16:50:48, michaelpg wrote: > optional nit: i'm trying to start a convention of naming the file just > "icons.html" and the iconset just "history", so the usage is a bit shorter: > > chrome://md-history/icons.html > > <iron-icon icon="history:star"></iron-icon> > I use cr:star so this history-icons.html is not needed any more. But thanks for the kind reminding! https://codereview.chromium.org/1928243002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_history/history_icons.html:6: <g id="star"><path d="M12 17.27L18.18 21l-1.64-7.03L22 9.24l-7.19-.61L12 2 9.19 8.63 2 9.24l5.46 4.73L5.82 21z"></path></g> On 2016/05/11 16:50:48, michaelpg wrote: > you can use the one in cr_elements/icons.html, just make it not chromeos-only > anymore Done. https://codereview.chromium.org/1928243002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_item.html (right): https://codereview.chromium.org/1928243002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_history/history_item.html:1: <link rel="import" href="chrome://resources/cr_elements/cr_icons.html"> On 2016/05/11 16:50:48, michaelpg wrote: > now "icons.html" Done. https://codereview.chromium.org/1928243002/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_history/history_item.html:135: <paper-icon-button icon="cr-icons:more-vert" id="menu-button" On 2016/05/11 16:50:48, michaelpg wrote: > now "cr:more-vert" Done.
lgtm
Looking good! There are a couple more icons that can be converted at the same time (they're all in the shared icons.html already): - synced_devices.js/html has 'expand-less' and 'expand-more' - history_toolbar.html has 'clear' Then I think we'll be free of iron-icons all together =D
Description was changed from ========== MD History: Only load needed paper-icon-buttons instead of whole set of icons Use polyicon to pick out needed paper-icon-buttons for md_history and make icon sets that contain svg icons for those icons, instead of load the whole set of icons. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD History: Only load needed paper-icon-buttons instead of whole set of icons Use polyicon to pick out needed paper-icon-buttons for md_history and make icon sets that contain svg icons for those icons, instead of load the whole set of icons. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
On 2016/05/12 06:36:34, tsergeant wrote: > Looking good! > > There are a couple more icons that can be converted at the same time (they're > all in the shared icons.html already): > > - synced_devices.js/html has 'expand-less' and 'expand-more' > - history_toolbar.html has 'clear' > > Then I think we'll be free of iron-icons all together =D Done. Please confirm:-)
slgtm, thanks for doing this!
lgtm!
Description was changed from ========== MD History: Only load needed paper-icon-buttons instead of whole set of icons Use polyicon to pick out needed paper-icon-buttons for md_history and make icon sets that contain svg icons for those icons, instead of load the whole set of icons. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD History: Only load needed paper-icon-buttons instead of whole set of icons Use common icons in cr_elements/icons.html which uses polyicon to pick out needed paper-icon-buttons and make icon sets that contain svg icons for those icons, instead of load the whole set of icons. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD History: Only load needed paper-icon-buttons instead of whole set of icons Use common icons in cr_elements/icons.html which uses polyicon to pick out needed paper-icon-buttons and make icon sets that contain svg icons for those icons, instead of load the whole set of icons. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD History: Only load needed paper-icon-buttons instead of whole set of icons Use common icons in cr_elements/icons.html which uses polyicon to pick out needed paper-icon-buttons and make icon sets that contain svg icons for those icons, instead of load the whole set of icons. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by lshang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/1928243002/#ps160001 (title: "change more icons")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1928243002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1928243002/160001
Message was sent while issue was closed.
Description was changed from ========== MD History: Only load needed paper-icon-buttons instead of whole set of icons Use common icons in cr_elements/icons.html which uses polyicon to pick out needed paper-icon-buttons and make icon sets that contain svg icons for those icons, instead of load the whole set of icons. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD History: Only load needed paper-icon-buttons instead of whole set of icons Use common icons in cr_elements/icons.html which uses polyicon to pick out needed paper-icon-buttons and make icon sets that contain svg icons for those icons, instead of load the whole set of icons. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== MD History: Only load needed paper-icon-buttons instead of whole set of icons Use common icons in cr_elements/icons.html which uses polyicon to pick out needed paper-icon-buttons and make icon sets that contain svg icons for those icons, instead of load the whole set of icons. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD History: Only load needed paper-icon-buttons instead of whole set of icons Use common icons in cr_elements/icons.html which uses polyicon to pick out needed paper-icon-buttons and make icon sets that contain svg icons for those icons, instead of load the whole set of icons. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/c84812a9ef3ff011616640084a8d1bb4b2caeac6 Cr-Commit-Position: refs/heads/master@{#393439} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/c84812a9ef3ff011616640084a8d1bb4b2caeac6 Cr-Commit-Position: refs/heads/master@{#393439} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
