Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(173)

Issue 2825493003: MD Settings: change outlinks to actually use <a> (Closed)

Created:
3 years, 8 months ago by scottchen
Modified:
3 years, 8 months ago
Reviewers:
dschuyler, Dan Beam, dpapad
CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, Dan Beam, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

MD Settings: change outlinks to actually use <a> BUG=684005 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2825493003 Cr-Commit-Position: refs/heads/master@{#467169} Committed: https://chromium.googlesource.com/chromium/src/+/c3e69c366f3e5c247276fec6c1dba93106f14ba9

Patch Set 1 #

Patch Set 2 : replace more, add target blank #

Total comments: 3

Patch Set 3 : remove dead code #

Patch Set 4 : do one with loc string #

Patch Set 5 : replace more #

Total comments: 25

Patch Set 6 : updates based on comments #

Total comments: 5

Patch Set 7 : fixes based on comments #

Patch Set 8 : move anotation order to be consistent #

Total comments: 4

Patch Set 9 : use a more generic inherit-color class #

Total comments: 1

Patch Set 10 : merge #

Unified diffs Side-by-side diffs Delta from patch set Stats (+62 lines, -87 lines) Patch
M chrome/browser/resources/settings/a11y_page/a11y_page.html View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -4 lines 0 comments Download
M chrome/browser/resources/settings/a11y_page/a11y_page.js View 1 chunk +0 lines, -6 lines 0 comments Download
M chrome/browser/resources/settings/a11y_page/manage_a11y_page.html View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -3 lines 0 comments Download
M chrome/browser/resources/settings/a11y_page/manage_a11y_page.js View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -6 lines 0 comments Download
M chrome/browser/resources/settings/appearance_page/appearance_page.html View 1 2 3 4 5 6 7 8 2 chunks +4 lines, -3 lines 0 comments Download
M chrome/browser/resources/settings/appearance_page/appearance_page.js View 1 2 3 4 5 6 7 3 chunks +10 lines, -6 lines 0 comments Download
M chrome/browser/resources/settings/people_page/people_page.html View 1 2 3 4 5 6 7 8 1 chunk +6 lines, -4 lines 0 comments Download
M chrome/browser/resources/settings/people_page/people_page.js View 1 2 3 4 1 chunk +0 lines, -5 lines 0 comments Download
M chrome/browser/resources/settings/people_page/sync_browser_proxy.js View 1 2 3 4 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/resources/settings/people_page/sync_page.html View 1 2 3 4 5 6 7 8 2 chunks +8 lines, -6 lines 0 comments Download
M chrome/browser/resources/settings/people_page/sync_page.js View 1 2 3 1 chunk +0 lines, -5 lines 0 comments Download
M chrome/browser/resources/settings/printing_page/cloud_printers.html View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -3 lines 0 comments Download
M chrome/browser/resources/settings/printing_page/cloud_printers.js View 1 2 3 4 1 chunk +0 lines, -5 lines 0 comments Download
M chrome/browser/resources/settings/privacy_page/privacy_page.html View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -4 lines 0 comments Download
M chrome/browser/resources/settings/privacy_page/privacy_page.js View 1 2 2 chunks +0 lines, -14 lines 0 comments Download
M chrome/browser/resources/settings/search_page/search_page.html View 1 2 3 4 5 6 7 8 9 2 chunks +6 lines, -4 lines 0 comments Download
M chrome/browser/resources/settings/search_page/search_page.js View 1 2 3 4 1 chunk +0 lines, -5 lines 0 comments Download
M chrome/browser/resources/settings/settings_shared_css.html View 1 2 3 4 5 6 7 8 9 1 chunk +7 lines, -2 lines 0 comments Download
M chrome/browser/resources/settings/settings_ui/settings_ui.html View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/resources/settings/settings_vars_css.html View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 47 (23 generated)
scottchen
I grepped chrome/browser/resources/settings/ for all "window.open" and replaced most of them. There's 4 instances of ...
3 years, 8 months ago (2017-04-18 19:12:49 UTC) #5
dpapad
+dbeam https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/appearance_page/appearance_page.js File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/appearance_page/appearance_page.js#newcode170 chrome/browser/resources/settings/appearance_page/appearance_page.js:170: return this.themeUrl_ || loadTimeData.getString('themesGalleryUrl'); On 2017/04/18 at 19:12:49, ...
3 years, 8 months ago (2017-04-18 21:29:27 UTC) #8
Dan Beam
https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/appearance_page/appearance_page.js File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/appearance_page/appearance_page.js#newcode170 chrome/browser/resources/settings/appearance_page/appearance_page.js:170: return this.themeUrl_ || loadTimeData.getString('themesGalleryUrl'); On 2017/04/18 21:29:27, dpapad wrote: ...
3 years, 8 months ago (2017-04-18 21:59:49 UTC) #10
scottchen
https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/appearance_page/appearance_page.js File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/appearance_page/appearance_page.js#newcode170 chrome/browser/resources/settings/appearance_page/appearance_page.js:170: return this.themeUrl_ || loadTimeData.getString('themesGalleryUrl'); On 2017/04/18 21:59:49, Dan Beam ...
3 years, 8 months ago (2017-04-18 22:41:51 UTC) #11
dschuyler
https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/appearance_page/appearance_page.js File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/appearance_page/appearance_page.js#newcode168 chrome/browser/resources/settings/appearance_page/appearance_page.js:168: /** @private */ comment like, "URL for either current ...
3 years, 8 months ago (2017-04-18 23:22:45 UTC) #12
dpapad
LGTM, assuming other reviewer's comments will be addressed.
3 years, 8 months ago (2017-04-19 21:56:22 UTC) #13
Dan Beam
https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/settings_shared_css.html File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/settings_shared_css.html#newcode76 chrome/browser/resources/settings/settings_shared_css.html:76: a[href][tabindex='-1'] { this is kindddda ghetto
3 years, 8 months ago (2017-04-20 00:24:34 UTC) #14
Dan Beam
https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/search_page/search_page.html File chrome/browser/resources/settings/search_page/search_page.html (right): https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/search_page/search_page.html#newcode111 chrome/browser/resources/settings/search_page/search_page.html:111: href="$i18nRaw{manageAudioHistoryUrl}"> why are you using $i18nRaw{}? in case there's ...
3 years, 8 months ago (2017-04-20 00:28:35 UTC) #15
scottchen
https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/appearance_page/appearance_page.js File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/appearance_page/appearance_page.js#newcode168 chrome/browser/resources/settings/appearance_page/appearance_page.js:168: /** @private */ On 2017/04/18 23:22:44, dschuyler wrote: > ...
3 years, 8 months ago (2017-04-20 18:41:50 UTC) #16
scottchen
https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/settings_shared_css.html File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/settings_shared_css.html#newcode77 chrome/browser/resources/settings/settings_shared_css.html:77: color: var(--paper-grey-900); On 2017/04/20 18:41:50, scottchen wrote: > On ...
3 years, 8 months ago (2017-04-20 18:43:23 UTC) #17
Dan Beam
https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/search_page/search_page.html File chrome/browser/resources/settings/search_page/search_page.html (right): https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/search_page/search_page.html#newcode111 chrome/browser/resources/settings/search_page/search_page.html:111: href="$i18nRaw{manageAudioHistoryUrl}"> On 2017/04/20 18:41:49, scottchen wrote: > On 2017/04/20 ...
3 years, 8 months ago (2017-04-20 18:44:46 UTC) #18
Dan Beam
https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/settings_shared_css.html File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/settings_shared_css.html#newcode76 chrome/browser/resources/settings/settings_shared_css.html:76: a[href][tabindex='-1'] { On 2017/04/20 18:41:50, scottchen wrote: > On ...
3 years, 8 months ago (2017-04-20 18:49:41 UTC) #19
dschuyler
https://codereview.chromium.org/2825493003/diff/100001/chrome/browser/resources/settings/appearance_page/appearance_page.js File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/2825493003/diff/100001/chrome/browser/resources/settings/appearance_page/appearance_page.js#newcode191 chrome/browser/resources/settings/appearance_page/appearance_page.js:191: * URL for either current theme or the theme ...
3 years, 8 months ago (2017-04-20 18:50:04 UTC) #20
Dan Beam
https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/settings_shared_css.html File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/settings_shared_css.html#newcode76 chrome/browser/resources/settings/settings_shared_css.html:76: a[href][tabindex='-1'] { On 2017/04/20 18:49:41, Dan Beam wrote: > ...
3 years, 8 months ago (2017-04-20 18:52:54 UTC) #21
dschuyler
https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/settings_shared_css.html File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/settings_shared_css.html#newcode76 chrome/browser/resources/settings/settings_shared_css.html:76: a[href][tabindex='-1'] { On 2017/04/20 18:52:54, Dan Beam wrote: > ...
3 years, 8 months ago (2017-04-20 18:58:17 UTC) #22
Dan Beam
https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/settings_shared_css.html File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/settings_shared_css.html#newcode76 chrome/browser/resources/settings/settings_shared_css.html:76: a[href][tabindex='-1'] { On 2017/04/20 18:58:17, dschuyler wrote: > On ...
3 years, 8 months ago (2017-04-20 21:15:40 UTC) #23
scottchen
https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/search_page/search_page.html File chrome/browser/resources/settings/search_page/search_page.html (right): https://codereview.chromium.org/2825493003/diff/80001/chrome/browser/resources/settings/search_page/search_page.html#newcode111 chrome/browser/resources/settings/search_page/search_page.html:111: href="$i18nRaw{manageAudioHistoryUrl}"> On 2017/04/20 18:44:46, Dan Beam wrote: > On ...
3 years, 8 months ago (2017-04-20 21:48:34 UTC) #24
dschuyler
lgtm
3 years, 8 months ago (2017-04-20 23:09:35 UTC) #30
Dan Beam
oh, i get it now: you were putting [actionable] on <a href> so the buttons ...
3 years, 8 months ago (2017-04-20 23:22:07 UTC) #31
scottchen
https://codereview.chromium.org/2825493003/diff/140001/chrome/browser/resources/settings/settings_shared_css.html File chrome/browser/resources/settings/settings_shared_css.html (right): https://codereview.chromium.org/2825493003/diff/140001/chrome/browser/resources/settings/settings_shared_css.html#newcode77 chrome/browser/resources/settings/settings_shared_css.html:77: a.unstyled-link { On 2017/04/20 23:22:07, Dan Beam wrote: > ...
3 years, 8 months ago (2017-04-21 20:07:04 UTC) #32
Dan Beam
lgtm
3 years, 8 months ago (2017-04-22 01:21:14 UTC) #33
commit-bot: I haz the power
This CL has an open dependency (Issue 2818283002 Patch 20001). Please resolve the dependency and ...
3 years, 8 months ago (2017-04-25 19:08:03 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2825493003/180001
3 years, 8 months ago (2017-04-25 23:52:26 UTC) #44
commit-bot: I haz the power
3 years, 8 months ago (2017-04-25 23:59:51 UTC) #47
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/c3e69c366f3e5c247276fec6c1db...

Powered by Google App Engine
This is Rietveld 408576698