Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2924443002:
WebUI: Remove last usage of paper-material. (Closed)
DescriptionWebUI: Remove last usage of <paper-material>.
This CL also removes the last usage of paper-material-shared-styles,
because it is not really needed. Instead its usage can be replaced
with the already existing paper-styles/shadow.html
BUG=598516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2924443002
Cr-Commit-Position: refs/heads/master@{#477177}
Committed: https://chromium.googlesource.com/chromium/src/+/c4689b08719e552c8f361a35ce0c3a31935606d7
Patch Set 1 #Patch Set 2 : More #
Total comments: 1
Messages
Total messages: 24 (15 generated)
Description was changed from ========== WebUI: Remove last usage of paper-material. BUG= ========== to ========== WebUI: Remove last usage of paper-material. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== WebUI: Remove last usage of paper-material. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI: Remove last usage of paper-material. BUG=598516 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== WebUI: Remove last usage of paper-material. BUG=598516 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI: Remove last usage of <paper-material>. This CL also removes the last usage of paper-material-shared-styles, because it is not really needed. Instead its usage can be replaced with the already existing paper-styles/shadow.html BUG=598516 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dpapad@chromium.org changed reviewers: + michaelpg@chromium.org
I believe these are the last usages of paper-material and paper-material-shared-styles from our own code, see [1]. I don't think we can fully remove it from our code though, since paper-button is using paper-material-shared-styles internally, see [2]. [1] https://cs.chromium.org/search/?q=paper-material+file:%5Esrc/chrome/+package:... [2] https://cs.chromium.org/search/?q=paper-material+file:%5Esrc/third_party/poly... https://codereview.chromium.org/2924443002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/display_layout.html (left): https://codereview.chromium.org/2924443002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/display_layout.html:11: <style include="settings-shared paper-material-shared-styles"> I had converted <paper-material> usage to paper-material-shared-styles usage at https://codereview.chromium.org/2885363006, but now I am realizing that the only dependency we actually need is paper-styles/shadow.html.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Woooo!!! Can you go ahead and remove paper-material? Doesn't look like it's used by any other UI or any Polymer element either.
On 2017/06/05 at 19:18:57, michaelpg wrote: > Woooo!!! Can you go ahead and remove paper-material? Doesn't look like it's used by any other UI or any Polymer element either. Can you specify what you suggest it can be removed? The direct dependency to paper-material from our bower.json? If so I can give it a try, but otherwise, the dependency will be pulled in anyway, see https://cs.chromium.org/search/?q=paper-material+file:%5Esrc/third_party/poly... (a few elements depend on paper-material-shared-styles, which resides in the same repo as paper-material. I would prefer doing further cleanup in a separate CL, to reduce the chance of messing something up and then having to revert this CL as a whole.
On 2017/06/05 19:25:44, dpapad wrote: > On 2017/06/05 at 19:18:57, michaelpg wrote: > > Woooo!!! Can you go ahead and remove paper-material? Doesn't look like it's > used by any other UI or any Polymer element either. > > Can you specify what you suggest it can be removed? The direct dependency to > paper-material from our bower.json? If so I can give it a try, but otherwise, > the dependency will be pulled in anyway, see > https://cs.chromium.org/search/?q=paper-material+file:%5Esrc/third_party/poly... > (a few elements depend on paper-material-shared-styles, which resides in the > same repo as paper-material. > > I would prefer doing further cleanup in a separate CL, to reduce the chance of > messing something up and then having to revert this CL as a whole. You're right, I lied, even after managing to run find_unused_elements.py I was holding it wrong. However, I Think you can remove paper-material.html and paper-material-extracted.js from polymer_resources.grdp: $ git ls-files -z -- '**/*.'{html,css,js,cc} | xargs -0 grep 'paper-material\.html' Now or as a follow-up.
and lgtm
> You're right, I lied, even after managing to run find_unused_elements.py I was
> holding it wrong.
>
> However, I Think you can remove paper-material.html and
> paper-material-extracted.js from polymer_resources.grdp:
>
> $ git ls-files -z -- '**/*.'{html,css,js,cc} | xargs -0 grep
'paper-material\.html'
>
> Now or as a follow-up.
Will try this as a follow up. Thanks.
The CQ bit was checked by dpapad@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by dpapad@chromium.org
The CQ bit was checked by dpapad@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496708273424390,
"parent_rev": "e7deaa85c11854d2066e061ed93665ea920f4cbf", "commit_rev":
"c4689b08719e552c8f361a35ce0c3a31935606d7"}
Message was sent while issue was closed.
Description was changed from ========== WebUI: Remove last usage of <paper-material>. This CL also removes the last usage of paper-material-shared-styles, because it is not really needed. Instead its usage can be replaced with the already existing paper-styles/shadow.html BUG=598516 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI: Remove last usage of <paper-material>. This CL also removes the last usage of paper-material-shared-styles, because it is not really needed. Instead its usage can be replaced with the already existing paper-styles/shadow.html BUG=598516 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2924443002 Cr-Commit-Position: refs/heads/master@{#477177} Committed: https://chromium.googlesource.com/chromium/src/+/c4689b08719e552c8f361a35ce0c... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/c4689b08719e552c8f361a35ce0c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
