New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 771795 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Clean up old options page strings

Project Member Reported by rdevlin....@chromium.org, Oct 4 2017

Issue description

It looks like revision 1f2b58d76f2d61471b9afe04de743b127ae49685 missed pulling out some of the generated string resources.  Let's clean them up.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0a5d1d7260ffe84394cc8b8b6c7195cc36c48af5

commit 0a5d1d7260ffe84394cc8b8b6c7195cc36c48af5
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Thu Oct 05 00:46:23 2017

[Cleanup] Remove a bunch of unneeded strings

crrev.com/1f2b58d76f2d61471b9afe04de743b127ae49685 removed the old
settings page code, but missed cleaning up the strings. Remove some
that are no longer used.

Bug: 771795
Change-Id: Ide2ee7e5a55395dc63f46fa2d54bf047739402ec
Reviewed-on: https://chromium-review.googlesource.com/701640
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506592}
[modify] https://crrev.com/0a5d1d7260ffe84394cc8b8b6c7195cc36c48af5/chrome/app/chromium_strings.grd
[modify] https://crrev.com/0a5d1d7260ffe84394cc8b8b6c7195cc36c48af5/chrome/app/generated_resources.grd
[modify] https://crrev.com/0a5d1d7260ffe84394cc8b8b6c7195cc36c48af5/chrome/app/google_chrome_strings.grd

Project Member

Comment 2 by bugdroid1@chromium.org, Oct 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/48c6e5455359b6477c8df8c927270646302a7f01

commit 48c6e5455359b6477c8df8c927270646302a7f01
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Thu Oct 05 20:54:09 2017

[String Cleanup] Remove a bunch of unneeded strings (A - C)

crrev.com/1f2b58d76f2d61471b9afe04de743b127ae49685 removed the old
settings page code, but missed cleaning up the strings. Remove some
that are no longer used.
This covers: IDS_ABOUT_BUTTON -> IDS_CERT_MANAGER_CERT_PARSE_ERROR

Bug: 771795
Change-Id: I8b1ad50dcfc2d45e346945a36b62eb6807dac4e1
Reviewed-on: https://chromium-review.googlesource.com/702921
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506848}
[modify] https://crrev.com/48c6e5455359b6477c8df8c927270646302a7f01/chrome/app/chromium_strings.grd
[modify] https://crrev.com/48c6e5455359b6477c8df8c927270646302a7f01/chrome/app/generated_resources.grd
[modify] https://crrev.com/48c6e5455359b6477c8df8c927270646302a7f01/chrome/app/google_chrome_strings.grd

A few quick notes:

I'll be uploading a number of CLs to remove these.  I'm sharding the CLs into small-ish groups for easy tracking, reviewability, and revertability.

What's included?  I'm checking each instance of a string that was removed in revision 1f2b58d76 to see if it's used.  If it's not, I'm removing it.  Note, this'll mean there's still unused strings in generated_resources.grd.  There were before revision 1f2b58d76, too. :)  After cleaning all these up, I might see how hard it would be to clean up the full grd file more.

Why not just do one giant CL?  Pretty much, I don't trust my hacky script enough to think it won't break something, somewhere, and I'd rather have most cls stick than battle with relanding a giant patch.  Additionally (since I don't trust my hacky script) these should be reviewed (albeit quickly) instead of TBR'd/RS'd, and small CLs are easier to review.

Why write a hacky script and not just do it by hand?
$ git show 1f2b58d76 | grep "IDS_" | wc -l
> 1180
Cc: dschuyler@chromium.org
I just found this, it seems like it might be working?

./tools/check_grd_for_unused_strings.py

If and IDS is removed and the patch gets through the CQ, it should be safe...

Project Member

Comment 6 by bugdroid1@chromium.org, Oct 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fff664620c9416444e81ad73b83fcfdd77719dc9

commit fff664620c9416444e81ad73b83fcfdd77719dc9
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Thu Oct 05 21:48:26 2017

[String Cleanup] Remove a bunch of unneeded strings (CERT - COOKIES)

crrev.com/1f2b58d76f2d61471b9afe04de743b127ae49685 removed the old
settings page code, but missed cleaning up the strings. Remove some
that are no longer used.
This covers: IDS_CERT_MANAGER_WRITE_ERROR_FORMAT ->
             IDS_COOKIES_COOKIE_ACCESSIBLE_TO_SCRIPT_LABEL

Bug: 771795
Change-Id: Ib37177a27c9b57d234bf26e059adf0fc65b7861e
Reviewed-on: https://chromium-review.googlesource.com/703154
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506870}
[modify] https://crrev.com/fff664620c9416444e81ad73b83fcfdd77719dc9/chrome/app/generated_resources.grd

> I just found this, it seems like it might be working?
> ./tools/check_grd_for_unused_strings.py

Wild!  Looks like it might be.  Unfortunately, it doesn't do the work of modifying the grd file for me.  Maybe I can hack them both together... :)

> If and IDS is removed and the patch gets through the CQ, it should be safe...
Not always.  One concern is in the difference between chrome-branded/official builds, which we don't have any CQ for.  This would result in the patch landing, waiting 12 hours, and then getting reverted - which would be painful once, and agonizing if it happens <n> times. :)  The other slight concern is that my script will rewrite the GRD file, but in the case of
<if chromeos>
  <message>
</if>
won't know to also cleanup the <if>s, so a manual review is helpful (and a manual review of thousands of lines is harder).
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a7e03267b8782b38b6bc9b412dc0b50b14072db6

commit a7e03267b8782b38b6bc9b412dc0b50b14072db6
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Fri Oct 06 16:24:55 2017

[String Cleanup] Remove a bunch of unneeded strings (COOKIES - EXCEPTIONS)

crrev.com/1f2b58d76f2d61471b9afe04de743b127ae49685 removed the old
settings page code, but missed cleaning up the strings. Remove some
that are no longer used.
This covers: IDS_COOKIES_DATABASE_STORAGE ->
             IDS_EXCEPTIONS_ADD_NEW_INSTRUCTIONS

Bug: 771795
Change-Id: I23234c3e0d3ee3b9baeb4de5171b61240046f340
Reviewed-on: https://chromium-review.googlesource.com/703470
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Dave Schuyler <dschuyler@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507091}
[modify] https://crrev.com/a7e03267b8782b38b6bc9b412dc0b50b14072db6/chrome/app/generated_resources.grd

Project Member

Comment 9 by bugdroid1@chromium.org, Oct 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fa708278f12c2946942202b788740da5d0cdc4d6

commit fa708278f12c2946942202b788740da5d0cdc4d6
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Tue Oct 10 02:12:03 2017

[String Cleanup] Remove a bunch of unneeded strings (CERT_MANAGER*)

crrev.com/1f2b58d76f2d61471b9afe04de743b127ae49685 removed the old
settings page code, but missed cleaning up the strings. Remove some
that are no longer used.
This covers: IDS_CERT_MANAGER_CONFIRM_PASSWORD_LABEL ->
             IDS_CERT_MANAGER_VIEW_CERT_BUTTON

Bug: 771795
Change-Id: I5a485a81c2321478af7695d061dc6efc3ba84c6a
Reviewed-on: https://chromium-review.googlesource.com/702727
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507567}
[modify] https://crrev.com/fa708278f12c2946942202b788740da5d0cdc4d6/chrome/app/generated_resources.grd

Project Member

Comment 10 by bugdroid1@chromium.org, Oct 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c77300b3e0de8693a7fe450b9057459817ea2aaa

commit c77300b3e0de8693a7fe450b9057459817ea2aaa
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Thu Oct 12 22:35:44 2017

[String Cleanup] Remove unneeded strings (EXCEPTIONS - NOTIFICATIONS)

crrev.com/1f2b58d76f2d61471b9afe04de743b127ae49685 removed the old
settings page code, but missed cleaning up the strings. Remove some
that are no longer used.
This covers: IDS_EXCEPTIONS_ALLOW_BUTTON ->
             IDS_NOTIFICATIONS_TAB_LABEL

Bug: 771795
Change-Id: Ie9e69334c1e99474543f8023dec2d661fc18ed2a
Reviewed-on: https://chromium-review.googlesource.com/703635
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508507}
[modify] https://crrev.com/c77300b3e0de8693a7fe450b9057459817ea2aaa/chrome/app/chromium_strings.grd
[modify] https://crrev.com/c77300b3e0de8693a7fe450b9057459817ea2aaa/chrome/app/generated_resources.grd
[modify] https://crrev.com/c77300b3e0de8693a7fe450b9057459817ea2aaa/chrome/app/google_chrome_strings.grd

Project Member

Comment 11 by bugdroid1@chromium.org, Nov 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/528eec57d56872a0a4f82548ace0e1cfa834cfaf

commit 528eec57d56872a0a4f82548ace0e1cfa834cfaf
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Fri Nov 10 19:21:19 2017

CrOS: Eliminate unused strings from chromeos_strings.grdp

Includes a minor change to check_grd_for_unused_strings.py

Found running the following with the change to
check_grd_for_unused_strings.py:
 ./tools/check_grd_for_unused_strings.py chrome \
    chrome/app/chromeos_strings.grdp

Some of these came from the switch to the new Settings UI and were
missed, some have just been lingering for a very long time.

Bug: 771795,  783396 
Change-Id: If6d9ab01f909c69d6375459d314e97ce3633ceaf
Reviewed-on: https://chromium-review.googlesource.com/762383
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515629}
[modify] https://crrev.com/528eec57d56872a0a4f82548ace0e1cfa834cfaf/chrome/app/chromeos_strings.grdp
[modify] https://crrev.com/528eec57d56872a0a4f82548ace0e1cfa834cfaf/tools/check_grd_for_unused_strings.py

Project Member

Comment 12 by bugdroid1@chromium.org, Nov 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ff1be1ca52597bc9aee5df64c75e6ca1d49f5e96

commit ff1be1ca52597bc9aee5df64c75e6ca1d49f5e96
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Fri Nov 10 21:29:45 2017

Eliminate some unused CrOS/panel/app list related strings

Found running the following with the change to
check_grd_for_unused_strings.py:
 ./tools/check_grd_for_unused_strings.py chrome \
    chrome/app/generated_resources.grd

Only a subset of strings from deprecated CrOS/panel/app list
features were removed in this CL.

Bug: 771795,  783396 

CrOS: Elim unised strings from chromeos_strings.grdp
Change-Id: I9accc2027f66d5b7b8ff9d45461a29bacafbb301
Reviewed-on: https://chromium-review.googlesource.com/762521
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515687}
[modify] https://crrev.com/ff1be1ca52597bc9aee5df64c75e6ca1d49f5e96/chrome/app/generated_resources.grd

Project Member

Comment 13 by bugdroid1@chromium.org, Apr 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2296e057e0d0f174a54582120952edad56cd0af7

commit 2296e057e0d0f174a54582120952edad56cd0af7
Author: dpapad <dpapad@chromium.org>
Date: Thu Apr 12 05:28:34 2018

WebUI cleanup: Remove more unused strings from old Options page.

Bug: 771795
Change-Id: Ie6aa2ede841a29f1e68e29ae1e3bd832abdb28b8
Reviewed-on: https://chromium-review.googlesource.com/1008069
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550018}
[modify] https://crrev.com/2296e057e0d0f174a54582120952edad56cd0af7/chrome/app/chromium_strings.grd
[modify] https://crrev.com/2296e057e0d0f174a54582120952edad56cd0af7/chrome/app/google_chrome_strings.grd

Sign in to add a comment