Issue 786023002: Remove hard-coded font families in WebUI.

Issue 786023002: Remove hard-coded font families in WebUI. (Closed)

Created:
6 years ago by xdai1

Modified:
5 years, 4 months ago

Reviewers:
jungshik at Google, Dan Beam, Evan Stade

CC:
chromium-reviews, arv+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Remove hard-coded font families in WebUI. This is done by the following steps: 1) Remove hard-coded font-family from all css/html files in chrome/browser/resources, except 'monospace', 'Courier New, monospace' and 'inherit' font types. 2) Add <link rel="stylesheet" href="chrome://resources/css/text_defaults.css"> to all html files in chrome/browser/resources which not include chrome_shared.css file, and removed all the i18n-content="fontFamily:fontfamily;fontSize:fontSize" inside of the <body> and <html> tags. BUG=303849 Committed: https://crrev.com/b1648f74041be5bbb57260302b58acb5a5925526 Cr-Commit-Position: refs/heads/master@{#308755}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : The First Version. #

Total comments: 84

Patch Set 6 : Address the code review comments. #

Patch Set 7 : Pull rebase. Address dbeam@'s code review comments. #

Patch Set 8 : Revert some changes that fail the tests. #

Total comments: 5

Patch Set 9 : Pull Rebase. Revert some changes that fail the tests. #

Patch Set 10 : Address jshin@'s review comments. #

Patch Set 11 : Address jshin@'s comments. #

Patch Set 12 : Bring back changes for /resources/ntp4. Add some TODO comments. #

Patch Set 13 : Fix the presubmit warnings. #

Total comments: 2

Created: 6 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+165 lines, -129 lines)			Patch
M	chrome/browser/resources/about_conflicts.html	View	1 2 3 4 5 6	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/about_credits.tmpl	View	1 2 3 4 5 6 7 8	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/about_flash.html	View	1 2 3 4 5 6	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/about_invalidations.css	View	1 2 3 4 5	1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/browser/resources/about_invalidations.html	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/about_memory.html	View	1 2 3 4 5 6	1 chunk	+2 lines, -9 lines	0 comments	Download
M	chrome/browser/resources/about_memory_linux.html	View	1 2 3 4 5 6	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/about_memory_mac.html	View	1 2 3 4 5 6	1 chunk	+1 line, -9 lines	0 comments	Download
M	chrome/browser/resources/about_nacl.html	View	1 2 3 4 5 6	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/about_stats.html	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/about_sys/about_sys.css	View	1 2 3 4 5	1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/browser/resources/about_sys/about_sys.html	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/about_version.css	View	1	1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/browser/resources/about_version.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	2 comments	Download
M	chrome/browser/resources/about_voicesearch.html	View	1 2 3 4 5 6	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/app_list/start_page.html	View	1 2 3 4 5 6	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/bookmark_manager/js/bmm_test.html	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/bookmark_manager/main.html	View	1 2 3 4 5 6	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/chromeos/about_os_credits.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/chromeos/choose_mobile_network.html	View	1 2 3 4 5 6	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/chromeos/chromevox/chromevox/background/kbexplorer.html	View	1 2 3 4 5 6 7 8	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/chromeos/cryptohome.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/chromeos/drive_internals.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/chromeos/first_run/app/main.html	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/chromeos/first_run/first_run.html	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/chromeos/guest_session_tab.html	View	1 2 3 4 5 6	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/chromeos/keyboard_overlay.css	View	1	1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/browser/resources/chromeos/keyboard_overlay.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/chromeos/login/demo_user_login.html	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/chromeos/login/login.html	View	1 2 3 4 5 6	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/chromeos/login/oobe.html	View	1 2 3 4 5 6	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/chromeos/login/oobe-screen.html	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/chromeos/login/oobe_screen_controller_pairing.css	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/chromeos/login/oobe_screen_host_pairing.html	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/chromeos/merge_session_load.html	View	1 2 3 4 5 6	3 chunks	+1 line, -2 lines	0 comments	Download
M	chrome/browser/resources/chromeos/mobile_setup.html	View	1 2 3 4 5 6	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/chromeos/mobile_setup_portal.html	View	1 2 3 4 5 6	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/chromeos/network_ui/network_ui.html	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+3 lines, -2 lines	0 comments	Download
M	chrome/browser/resources/chromeos/nfc_debug.html	View	1 2 3 4 5 6	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/chromeos/offline_app_load.html	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/chromeos/offline_net_load.html	View	1 2 3 4 5 6	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/chromeos/power.html	View	1 2 3 4 5 6	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/chromeos/provided_file_systems.css	View	1 2 3 4 5	1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/browser/resources/chromeos/provided_file_systems.html	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/chromeos/salsa.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/chromeos/sim_unlock.html	View	1 2 3 4 5 6	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/chromeos/slow.css	View	1 2 3 4 5	1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/browser/resources/chromeos/slow.html	View	1 2 3 4 5 6	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/chromeos/wallpaper_manager/main.html	View	1 2 3 4 5 6	2 chunks	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/resources/components.html	View	1 2 3 4 5 6	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/contextual_search/promo.css	View	1 2 3 4 5 6 7 8 9	1 chunk	+3 lines, -2 lines	0 comments	Download
M	chrome/browser/resources/contextual_search/promo.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/crashes.html	View	1 2 3 4 5 6	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/domain_reliability_internals.html	View	1 2 3 4 5 6	2 chunks	+2 lines, -1 line	0 comments	Download
A +	chrome/browser/resources/extensions/extension_info.css	View	1 2 3 4 5 6	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	chrome/browser/resources/extensions_infobar.css	View	1 2 3 4 5	1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/browser/resources/extensions_infobar_mac.css	View	1 2 3 4 5	1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/browser/resources/feedback/html/default.html	View	1 2 3 4 5 6	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/flags.html	View	1 2 3 4 5 6	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/gaia_auth/main.html	View	1 2 3 4 5 6	2 chunks	+3 lines, -2 lines	0 comments	Download
M	chrome/browser/resources/gaia_auth/offline.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/gaia_auth/success.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/gcm_internals.html	View	1 2 3 4 5 6	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/gesture_config.css	View	1 2 3 4 5	1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/browser/resources/hangout_services/background.html	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/hotword_audio_verification/main.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/hotword_audio_verification/style.css	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+2 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/identity_internals.html	View	1 2 3 4 5 6	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/identity_scope_approval_dialog/scope_approval_dialog.html	View	1 2 3 4 5	1 chunk	+2 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/inline_login/inline_login.html	View	1 2 3 4 5 6 7 8 9 10 11 12	2 chunks	+4 lines, -2 lines	0 comments	Download
M	chrome/browser/resources/inline_login/new_inline_login.html	View	1 2 3 4 5 6	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/inspect/inspect.css	View	1 2 3 4 5 6	1 chunk	+2 lines, -5 lines	0 comments	Download
M	chrome/browser/resources/inspect/inspect.html	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/instant/instant.css	View	1 2 3 4 5	1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/browser/resources/instant/instant.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/local_ntp/local_ntp.css	View	1 2 3 4 5 6 7 8 9 10 11 12	6 chunks	+12 lines, -9 lines	0 comments	Download
M	chrome/browser/resources/local_state/local_state.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/media/webrtc_logs.html	View	1 2 3 4 5 6	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/memory_internals/memory_internals.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/net_export/net_export.css	View	1 2 3 4 5	1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/browser/resources/net_export/net_export.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/net_internals/events_view.css	View	1	1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/browser/resources/net_internals/index.html	View	1 2 3 4 5 6	2 chunks	+3 lines, -2 lines	0 comments	Download
M	chrome/browser/resources/notification_1line.html	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/notification_2line.html	View	1 2 3 4 5 6	1 chunk	+5 lines, -6 lines	0 comments	Download
M	chrome/browser/resources/notification_icon.html	View	1 2 3 4 5 6	2 chunks	+1 line, -2 lines	0 comments	Download
M	chrome/browser/resources/ntp4/apps_page.css	View	1 9 10 11	1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/browser/resources/ntp4/guest_tab.html	View	1 2 3 4 5 6 9 10 11	2 chunks	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/resources/ntp4/incognito_tab.html	View	1 2 3 4 5 6 9 10 11	2 chunks	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/resources/ntp4/new_tab.html	View	1 2 3 4 5 6 9 10 11	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/omnibox/omnibox.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/pdf/index.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/plugins.html	View	1 2 3 4 5 6	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/predictors/predictors.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/print_preview/previewarea/margin_control.css	View	1	1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/browser/resources/print_preview/print_preview_page.html	View	1 2 3 4 5 6 7 8 9 10 11 12	2 chunks	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/profile_signin_confirmation.html	View	1 2 3 4 5 6	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/profiler/profiler.html	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/quota_internals/main.html	View	1 2 3 4 5 6	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/reader_out_of_date.html	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/security_warnings/interstitial_v2.html	View	1 2 3 4 5 6	1 chunk	+3 lines, -2 lines	0 comments	Download
M	chrome/browser/resources/signin_internals/signin_index.html	View	1 2 3 4 5 6	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/suggestions_internals/suggestions_internals.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/supervised_user_block_interstitial.css	View	1 2 3 4 5 6	1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/browser/resources/supervised_user_block_interstitial.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/sync_file_system_internals/main.css	View	1	1 chunk	+0 lines, -4 lines	0 comments	Download
M	chrome/browser/resources/sync_file_system_internals/main.html	View	1 2 3 4 5 6	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/sync_internals/about.css	View	1 2 3 4 5	2 chunks	+1 line, -4 lines	0 comments	Download
M	chrome/browser/resources/sync_internals/index.html	View	1 2 3 4 5 6	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/translate_internals/translate_internals.html	View	1 2 3 4 5 6	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/resources/user_actions/user_actions.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/user_manager/user_manager.html	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/whispernet_proxy/background.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 44 (12 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

xdai1

jshin@, can you help to review the code please? Thanks! Please start from Patch Set ...

6 years ago (2014-12-09 22:43:02 UTC) #3

jungshik at Google

Wow, there are so many files to change. Thanks for working on this. Please, see ...

6 years ago (2014-12-10 07:57:57 UTC) #4

jungshik at Google

estade@chromium.org, dbeam@chromium.org : can you take a look?

6 years ago (2014-12-10 07:59:42 UTC) #6

Dan Beam

https://codereview.chromium.org/786023002/diff/100001/chrome/browser/resources/about_memory.html File chrome/browser/resources/about_memory.html (right): https://codereview.chromium.org/786023002/diff/100001/chrome/browser/resources/about_memory.html#newcode11 chrome/browser/resources/about_memory.html:11: <link rel="stylesheet" href="chrome://resources/css/text_defaults.css"> put text_defaults.css above about_memory.css https://codereview.chromium.org/786023002/diff/100001/chrome/browser/resources/about_memory_linux.html File ...

6 years ago (2014-12-10 19:49:25 UTC) #7

Dan Beam

fyi: most of the nits are for my own remembering and very few of them ...

6 years ago (2014-12-10 19:50:31 UTC) #8

jungshik at Google

https://codereview.chromium.org/786023002/diff/100001/chrome/browser/resources/chromeos/wallpaper_manager/main.html File chrome/browser/resources/chromeos/wallpaper_manager/main.html (right): https://codereview.chromium.org/786023002/diff/100001/chrome/browser/resources/chromeos/wallpaper_manager/main.html#newcode53 chrome/browser/resources/chromeos/wallpaper_manager/main.html:53: .style.fontSize:webFontSize"> On 2014/12/10 19:49:24, Dan Beam wrote: > is ...

6 years ago (2014-12-10 20:14:14 UTC) #9

jungshik at Google

On 2014/12/10 20:14:14, Jungshik Shin wrote: > Moreover, we have to get rid of the ...

6 years ago (2014-12-10 20:25:20 UTC) #10

jungshik at Google

https://codereview.chromium.org/786023002/diff/100001/chrome/browser/resources/chromeos/mobile_setup.html File chrome/browser/resources/chromeos/mobile_setup.html (right): https://codereview.chromium.org/786023002/diff/100001/chrome/browser/resources/chromeos/mobile_setup.html#newcode2 chrome/browser/resources/chromeos/mobile_setup.html:2: <html> On 2014/12/10 19:49:24, Dan Beam wrote: > hmmmm, ...

6 years ago (2014-12-10 20:46:57 UTC) #11

Dan Beam

On 2014/12/10 20:25:20, Jungshik Shin wrote: > On 2014/12/10 20:14:14, Jungshik Shin wrote: > > ...

6 years ago (2014-12-10 21:15:38 UTC) #12

jungshik at Google

On 2014/12/10 21:15:38, Dan Beam wrote: > On 2014/12/10 20:25:20, Jungshik Shin wrote: > > ...

6 years ago (2014-12-11 01:42:12 UTC) #13

xdai1

Please take another look, thanks! The modification in this patch: 1) Bring font-size inside of ...

6 years ago (2014-12-11 02:59:23 UTC) #19

xdai1

On 2014/12/11 19:09:29, Dan Beam wrote: > why did you add back all the font-size ...

6 years ago (2014-12-11 19:12:38 UTC) #21

Dan Beam

https://codereview.chromium.org/786023002/diff/100001/chrome/browser/resources/about_credits.tmpl File chrome/browser/resources/about_credits.tmpl (left): https://codereview.chromium.org/786023002/diff/100001/chrome/browser/resources/about_credits.tmpl#oldcode10 chrome/browser/resources/about_credits.tmpl:10: font-size:84%; On 2014/12/10 07:57:57, Jungshik Shin wrote: > Really ...

6 years ago (2014-12-11 19:15:28 UTC) #22

Dan Beam

6 years ago (2014-12-11 19:21:14 UTC) #23

jungshik at Google

On 2014/12/11 19:21:14, Dan Beam wrote: > https://codereview.chromium.org/786023002/diff/100001/chrome/browser/resources/about_credits.tmpl > File chrome/browser/resources/about_credits.tmpl (left): > > https://codereview.chromium.org/786023002/diff/100001/chrome/browser/resources/about_credits.tmpl#oldcode10 ...

6 years ago (2014-12-12 00:08:47 UTC) #24

Dan Beam

On 2014/12/12 00:08:47, Jungshik Shin wrote: > On 2014/12/11 19:21:14, Dan Beam wrote: > > ...

6 years ago (2014-12-12 00:10:36 UTC) #25

jungshik at Google

On 2014/12/12 00:10:36, Dan Beam wrote: > On 2014/12/12 00:08:47, Jungshik Shin wrote: > > ...

6 years ago (2014-12-12 09:02:28 UTC) #26

On 2014/12/12 00:10:36, Dan Beam wrote:
> On 2014/12/12 00:08:47, Jungshik Shin wrote:
> > On 2014/12/11 19:21:14, Dan Beam wrote:
> > >
> >
>
https://codereview.chromium.org/786023002/diff/100001/chrome/browser/resource...
> > > File chrome/browser/resources/about_credits.tmpl (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/786023002/diff/100001/chrome/browser/resource...
> > > chrome/browser/resources/about_credits.tmpl:10: font-size:84%;
> > > On 2014/12/11 19:15:28, Dan Beam wrote:
> > > > On 2014/12/10 07:57:57, Jungshik Shin wrote:
> > > > > Really sorry that I told you to remove font-size for body. I have a
> second
> > > > > thought and believe that we should keep font-size for body for now in
> this
> > > > file
> > > > > and other files. 
> > > > 
> > > > why?  this will cause chrome:// pages to continue to be inconsistent and
> > > flicker
> > > > (as the font-size will be loaded with JS in some cases, and that'll
happen
> > > > *after* the first frame is rendered [as of recently]).
> > > 
> > > re: flicker -- see
> https://code.google.com/p/chromium/issues/detail?id=434974
> > 
> > I'm aware of that issue (flicker). 
> > 
> > What need to happen is to define a set of classes (for different font sizes
> and
> > different generic font family such as 'monospace'), put them in
> text_default.css
> > and use them throughout our Web UI as they see fit. Moreover, this has to
> happen
> > if we want our Web UI to be better aligned with MR-design spec If we want
to. 
> > 
> > Without that, if we remove font-size that IS different from what we put in
> > text_default.css, we're gonna have unexpected changes in webUI.   
> > 
> > Let's not widen the scope of this CL beyond its original intent. This CL is
to
> > keep the current Web UI as  much as possible while making it possible to do
> > Roboto UI font experiment.
> 
> ah, didn't know that was the goal (to experiment with Roboto)

Sorry if that was a bit confusing. And, my previous comment about flickering was
a bit off, too (I should never write a comment right before rushing to pick up
my kid !). 

To avoid a flicker, i18n-value*font{Size,Family}  should go away for sure. And,
this CL does that as well as removing two functions setting fontSize and
fontFamily. 

What I meant by keeping 'font-size' is NOT keeping 'i18n-value*fontSize' BUT
keeping the hard-coded 'font-size: foo' in html.  There are several instances
where font-size is hard-coded to be 80%, 84%, etc in html files. If we get rid
of those hard-coded font-size in htmls, they'll use the value from
text_default.css (which is different from the hard-coded values) and lead to
changes in Web UI layout (line wrapping, etc) .  Needless to say, a font change
to Roboto will also results in some layout changes due to different font
metrics, but we should not introduce any other factor (such as font-size change)
to keep the experiment simple to analyze.

Dan Beam

lgtm if you revert changes to Helvetica Neue and the textdirection stuff (and make new ...

6 years ago (2014-12-13 02:29:54 UTC) #27

jungshik at Google

https://codereview.chromium.org/786023002/diff/260001/chrome/browser/resources/bookmark_manager/js/bmm_test.html File chrome/browser/resources/bookmark_manager/js/bmm_test.html (right): https://codereview.chromium.org/786023002/diff/260001/chrome/browser/resources/bookmark_manager/js/bmm_test.html#newcode6 chrome/browser/resources/bookmark_manager/js/bmm_test.html:6: <link rel="stylesheet" href="chrome://resources/css/text_defaults.css"> This may not be necessary because ...

6 years ago (2014-12-16 20:44:01 UTC) #28

xdai1

On 2014/12/16 20:44:01, Jungshik Shin wrote: > https://codereview.chromium.org/786023002/diff/260001/chrome/browser/resources/bookmark_manager/js/bmm_test.html > File chrome/browser/resources/bookmark_manager/js/bmm_test.html (right): > > https://codereview.chromium.org/786023002/diff/260001/chrome/browser/resources/bookmark_manager/js/bmm_test.html#newcode6 ...

6 years ago (2014-12-16 23:35:03 UTC) #30

On 2014/12/16 20:44:01, Jungshik Shin wrote:
>
https://codereview.chromium.org/786023002/diff/260001/chrome/browser/resource...
> File chrome/browser/resources/bookmark_manager/js/bmm_test.html (right):
> 
>
https://codereview.chromium.org/786023002/diff/260001/chrome/browser/resource...
> chrome/browser/resources/bookmark_manager/js/bmm_test.html:6: <link
> rel="stylesheet" href="chrome://resources/css/text_defaults.css">
> This may not be necessary because it's a test file. 
> Perhaps, it does not matter either way.
> 
>
https://codereview.chromium.org/786023002/diff/260001/chrome/browser/resource...
> File chrome/browser/resources/chromeos/login/oobe.html (right):
> 
>
https://codereview.chromium.org/786023002/diff/260001/chrome/browser/resource...
> chrome/browser/resources/chromeos/login/oobe.html:10: <link rel="stylesheet"
> href="roboto_font.css">
> FYI: This file (that imports Roboto as web fonts from chrome:// url) and oobe
> html files have 'hard-coded Roboto'. 
> 
> See
>
https://code.google.com/p/chromium/codesearch#search/&q=font-family:.*oboto%2...
> 
> Anyway, this change is fine. (it won't hurt / won't make any difference from
the
> current behavior. )
> 
>
https://codereview.chromium.org/786023002/diff/260001/chrome/browser/resource...
> File
chrome/browser/resources/chromeos/login/oobe_screen_controller_pairing.css
> (left):
> 
>
https://codereview.chromium.org/786023002/diff/260001/chrome/browser/resource...
> chrome/browser/resources/chromeos/login/oobe_screen_controller_pairing.css:12:
> font-family: 'Roboto';
> Let's leave alone html files (mostly OOBE) where Roboto is hard-coded. 
> You can also ask dzhioev about OOBE.
> 
>
https://codereview.chromium.org/786023002/diff/260001/chrome/browser/resource...
> File chrome/browser/resources/contextual_search/promo.css (left):
> 
>
https://codereview.chromium.org/786023002/diff/260001/chrome/browser/resource...
> chrome/browser/resources/contextual_search/promo.css:14: font-family:
'Roboto2',
> sans-serif;
> Again, let's leave this alone because this file is importing a local version
of
> Roboto (see @font-face above) for now. 
> This need to be cleaned up later.@font-face will not be necessary when we
> cleaned it up. How about adding TODO comment about the clean-up?
> 
>
https://codereview.chromium.org/786023002/diff/260001/chrome/browser/resource...
> File chrome/browser/resources/local_ntp/local_ntp.css (left):
> 
>
https://codereview.chromium.org/786023002/diff/260001/chrome/browser/resource...
> chrome/browser/resources/local_ntp/local_ntp.css:8: font-family: arial,
> sans-serif;
> This is a tricky case. Local NTP these days has Google Web search box. So, it
> should look like http://google.com which still uses 'Arial, sans-serif'. 
> 
> Add TODO comment saying it needs to be discussed with NTP folks and leave
> local-ntp alone.


Please take another look! Thanks!

Modifications in Patch 7: (Address dbeam@'s review comments)
1) Revert changes to ui/strings/app_locale_settings.grd. Will make this change
in a new cl.
2) Revert changes to textdirction stuff and will make a new cl to add
textdirection for all html files.

Modifications in Patch 9: (Rebase and revert some changes that fail the tests.)
1) Revert changes in directory: chrome/browser/resources/local_npt
1) Revert changes in directory: chrome/browser/resources/ntp4
3) Revert changes in directory:
chrome/browser/resources/chromeos/chromevox/chromevox/
Since these changes would fail those tests: linux_chromium_chromeos_rel_ng,
linux_chromium_rel_ng, mac_chromium_rel_ng, linux_chromium_asan_rel. I'll deal
with these issues in a new cl.

Modification in Patch 10: (Address jshin@'s review comments)
1) Revert changes to hard-coded font-family for 'Roboto' and 'Roboto2'.
2) Add TODO comments in resources/local_ntp/local_ntp.css and
resources/contextual_search/promo.css.

jungshik at Google

On 2014/12/16 23:35:03, xdai1 wrote: > Modifications in Patch 9: (Rebase and revert some changes ...

6 years ago (2014-12-17 00:41:51 UTC) #31

xdai1

On 2014/12/17 00:41:51, Jungshik Shin wrote: > On 2014/12/16 23:35:03, xdai1 wrote: > > > ...

6 years ago (2014-12-17 01:16:04 UTC) #32

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/786023002/360001

6 years ago (2014-12-17 03:49:22 UTC) #34

commit-bot: I haz the power

Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/31004)

6 years ago (2014-12-17 03:56:19 UTC) #36

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/786023002/380001

6 years ago (2014-12-17 06:38:56 UTC) #38

jungshik at Google

On 2014/12/17 03:56:19, I haz the power (commit-bot) wrote: > Try jobs failed on following ...

6 years ago (2014-12-17 06:44:35 UTC) #39

jungshik at Google

On 2014/12/17 03:56:19, I haz the power (commit-bot) wrote: > Try jobs failed on following ...

6 years ago (2014-12-17 06:44:42 UTC) #40

commit-bot: I haz the power

Patchset 13 (id:??) landed as https://crrev.com/b1648f74041be5bbb57260302b58acb5a5925526 Cr-Commit-Position: refs/heads/master@{#308755}

6 years ago (2014-12-17 07:36:35 UTC) #42

Dan Beam

https://codereview.chromium.org/786023002/diff/380001/chrome/browser/resources/about_version.html File chrome/browser/resources/about_version.html (right): https://codereview.chromium.org/786023002/diff/380001/chrome/browser/resources/about_version.html#newcode11 chrome/browser/resources/about_version.html:11: <link rel="stylesheet" href="chrome://resources/css/text_defaults.css"> fyi: this changed the font size ...

6 years ago (2014-12-17 22:56:53 UTC) #43

jungshik at Google

6 years ago (2014-12-17 23:46:50 UTC) #44

Message was sent while issue was closed.

https://codereview.chromium.org/786023002/diff/380001/chrome/browser/resource...
File chrome/browser/resources/about_version.html (right):

https://codereview.chromium.org/786023002/diff/380001/chrome/browser/resource...
chrome/browser/resources/about_version.html:11: <link rel="stylesheet"
href="chrome://resources/css/text_defaults.css">
On 2014/12/17 22:56:53, Dan Beam wrote:
> fyi: this changed the font size on about:version significantly... we should
> consider adding a specific override if this isn't OK

hmm.. I guess it's about 80% of the previous size. 

about:version relied on the default font size for sans-serif (16px) and
monospace (13px). I agree that we should consider explicitly setting the font
size in about_version.css. Perhaps, I'll just do it now to minimize the
difference due to this CL because xdai is ooo.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages