Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2272213005:
Site Settings Desktop: Add links to Adobe settings and Learn more. (Closed)
|
Created:
4 years, 3 months ago by Finnur Modified:
4 years, 3 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSite Settings Desktop: Add links to Adobe settings and Learn more.
Adobe Flash settings are for ChromeOS only.
Learn More for all categories.
BUG=635850, 614277
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/dcf428ae8f05ccb8c936128c84565d44ccdc2fc8
Cr-Commit-Position: refs/heads/master@{#415003}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Working #
Total comments: 6
Patch Set 3 : Address feedback #
Total comments: 3
Patch Set 4 : Address feedback #
Messages
Total messages: 32 (19 generated)
Description was changed from ========== Site Settings Desktop: Add links to Adobe settings and Learn more. Adobe Flash settings are for ChromeOS only. Learn More for all categories. BUG=635850, 614277 ========== to ========== Site Settings Desktop: Add links to Adobe settings and Learn more. Adobe Flash settings are for ChromeOS only. Learn More for all categories. BUG=635850, 614277 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Site Settings Desktop: Add links to Adobe settings and Learn more. Adobe Flash settings are for ChromeOS only. Learn More for all categories. BUG=635850, 614277 ========== to ========== Site Settings Desktop: Add links to Adobe settings and Learn more. Adobe Flash settings are for ChromeOS only. Learn More for all categories. BUG=635850, 614277 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by finnur@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...
finnur@chromium.org changed reviewers: + dschuyler@chromium.org
https://codereview.chromium.org/2272213005/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2272213005/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/privacy_page/privacy_page.html:4: <link rel="import" href="chrome://resources/polymer/v1_0/iron-flex-layout/iron-flex-layout-classes.html"> I thought this line was making this message appear: This file is deprecated. Please use `iron-flex-layout/iron-flex-layout-classes.html`, and one of the specific dom-modules instead ... but it seems that the problem (also) lies elsewhere. Figured this doesn't hurt at least.
Assuming the comments below will be addressed, LGTM. https://codereview.chromium.org/2272213005/diff/20001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2272213005/diff/20001/chrome/app/settings_str... chrome/app/settings_strings.grdp:1387: Adobe Flash Player Storage settings Does that need any (tm) or (r) anywhere? Or is that handled in some other overarching statement elsewhere? If you're not 100% sure, this can be resolved by sending a bug to tbuckley@ to find the answer. I.e. I think this is ok during development, but we should find an answer before shipping. https://codereview.chromium.org/2272213005/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_settings_category.html (right): https://codereview.chromium.org/2272213005/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_category.html:54: <if expr="chromeos"> optional: I think these are normally left justified (without indentation) like #if would be. I didn't find a style guide rule on it. So it's up to you. Just thought I'd mention it. https://codereview.chromium.org/2272213005/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_settings_category.js (right): https://codereview.chromium.org/2272213005/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_category.js:120: * @param {string} category The current category. @return
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by finnur@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...
Thanks Dave, I know you've greenlighted this, but can you take a look at the last comment below... https://codereview.chromium.org/2272213005/diff/20001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2272213005/diff/20001/chrome/app/settings_str... chrome/app/settings_strings.grdp:1387: Adobe Flash Player Storage settings Well, the old string has been in the product for quite some time without (tm) or (r), so at least we're maintaining a level of consistency. :) But I'll ask tbuckley. https://codereview.chromium.org/2272213005/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_settings_category.html (right): https://codereview.chromium.org/2272213005/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_category.html:54: <if expr="chromeos"> On 2016/08/25 18:24:16, dschuyler wrote: > optional: I think these are normally left justified > (without indentation) like #if would be. I didn't > find a style guide rule on it. So it's up to you. > Just thought I'd mention it. Done. https://codereview.chromium.org/2272213005/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_settings_category.js (right): https://codereview.chromium.org/2272213005/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_category.js:120: * @param {string} category The current category. On 2016/08/25 18:24:17, dschuyler wrote: > @return Done. https://codereview.chromium.org/2272213005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_settings_category.html (right): https://codereview.chromium.org/2272213005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_category.html:64: </div> OK, I'm stumped now... I am pretty sure this <if> trick for ChromeOS worked when I was testing it earlier, but now it is not. Lets say I add this to the HTML: <if expr="chromeos">ChromeOS</if> <if expr="not chromeos">Not ChromeOs</if> What would you expect it to write on the page? Clearly, it should write either 'ChromeOS' or 'Not ChromeOs', right? Well, in my case it writes 'ChromeOS Not ChromeOs'. But if I copy the exact same code to settings/about_page/about_page.html, it writes only 'Not ChromeOs'. Huh!? How is that possible? I'm not even sure where to go to track this down? Do you have any ideas for me, Dave? Does it work the same way for you?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2272213005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_settings_category.html (right): https://codereview.chromium.org/2272213005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_category.html:64: </div> On 2016/08/26 13:09:21, Finnur wrote: > OK, I'm stumped now... > > I am pretty sure this <if> trick for ChromeOS worked when I was testing it > earlier, but now it is not. > > Lets say I add this to the HTML: > > <if expr="chromeos">ChromeOS</if> > <if expr="not chromeos">Not ChromeOs</if> > > What would you expect it to write on the page? Clearly, it should write either > 'ChromeOS' or 'Not ChromeOs', right? > > Well, in my case it writes 'ChromeOS Not ChromeOs'. > > But if I copy the exact same code to settings/about_page/about_page.html, it > writes only 'Not ChromeOs'. > > Huh!? > > How is that possible? I'm not even sure where to go to track this down? Do you > have any ideas for me, Dave? Does it work the same way for you? The feature to filter the html to strip out platform specifics is optional. It needs to be enabled on that file. The way to do this is to go into this file https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin... and add flattenhtml="true" allowexternalscript="true" similarly to the way it's done here https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin... Bonus info: I think this is where the <if> is being evaluated, if you're curious https://cs.chromium.org/chromium/src/tools/grit/grit/format/html_inline.py?q=... which appears to be triggered by https://cs.chromium.org/chromium/src/tools/grit/grit/gather/chrome_html.py?q=...
Thank you, Dave. That was it.
The CQ bit was checked by finnur@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org Link to the patchset: https://codereview.chromium.org/2272213005/#ps60001 (title: "Address feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by finnur@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by finnur@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Site Settings Desktop: Add links to Adobe settings and Learn more. Adobe Flash settings are for ChromeOS only. Learn More for all categories. BUG=635850, 614277 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Site Settings Desktop: Add links to Adobe settings and Learn more. Adobe Flash settings are for ChromeOS only. Learn More for all categories. BUG=635850, 614277 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/dcf428ae8f05ccb8c936128c84565d44ccdc2fc8 Cr-Commit-Position: refs/heads/master@{#415003} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/dcf428ae8f05ccb8c936128c84565d44ccdc2fc8 Cr-Commit-Position: refs/heads/master@{#415003}
Message was sent while issue was closed.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2272213005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_settings_category.html (right): https://codereview.chromium.org/2272213005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_category.html:64: </div> On 2016/08/26 17:34:59, dschuyler wrote: > On 2016/08/26 13:09:21, Finnur wrote: > > OK, I'm stumped now... > > > > I am pretty sure this <if> trick for ChromeOS worked when I was testing it > > earlier, but now it is not. > > > > Lets say I add this to the HTML: > > > > <if expr="chromeos">ChromeOS</if> > > <if expr="not chromeos">Not ChromeOs</if> > > > > What would you expect it to write on the page? Clearly, it should write either > > 'ChromeOS' or 'Not ChromeOs', right? > > > > Well, in my case it writes 'ChromeOS Not ChromeOs'. > > > > But if I copy the exact same code to settings/about_page/about_page.html, it > > writes only 'Not ChromeOs'. > > > > Huh!? > > > > How is that possible? I'm not even sure where to go to track this down? Do you > > have any ideas for me, Dave? Does it work the same way for you? > > The feature to filter the html to strip out platform specifics is optional. > It needs to be enabled on that file. The way to do this is to go into this > file > > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin... > > and add > > flattenhtml="true" > allowexternalscript="true" > > similarly to the way it's done here > > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin... > > Bonus info: I think this is where the <if> is being evaluated, if you're curious > > https://cs.chromium.org/chromium/src/tools/grit/grit/format/html_inline.py?q=... > > which appears to be triggered by > > https://cs.chromium.org/chromium/src/tools/grit/grit/gather/chrome_html.py?q=... prefer preprocess="true" when not using <include> |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
