Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2401853005:
Adding client code for new Windows 10 First Run Experience (Closed)
|
Created:
4 years, 2 months ago by Patrick Monette Modified:
4 years, 1 month ago CC:
arv+watch_chromium.org, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding client code for new Windows 10 First Run Experience
BUG=648686
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/218e0aa27614cab0d01aa72f01cbb33d853a500d
Cr-Commit-Position: refs/heads/master@{#427263}
Patch Set 1 #
Total comments: 37
Patch Set 2 : Comments #1 #
Total comments: 8
Patch Set 3 : Second round #
Total comments: 14
Patch Set 4 : Using Polymer dom-bind #
Total comments: 29
Patch Set 5 : Addressing comments #1 #
Total comments: 2
Patch Set 6 : Adding Readme #Messages
Total messages: 25 (9 generated)
Description was changed from ========== Adding client code for new Windows 10 First Run Experience BUG=648686 ========== to ========== Adding client code for new Windows 10 First Run Experience BUG=648686 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Adding client code for new Windows 10 First Run Experience BUG=648686 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Adding client code for new Windows 10 First Run Experience BUG=648686 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
pmonette@chromium.org changed reviewers: + tmartino@chromium.org
PTAL Tommy. This is ready for first pass.
Thanks again for looking at this and sorry for taking so long to get back to you. https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/inline/combined.html (right): https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/combined.html:22: <img src="logo4x.png" class="header-logo"> We need to support 2x resolutions, so let's change this to a div and then in the CSS do: background-image: -webkit-image-set(url('logo4x.png' 1x, 'logo8x.png' 2x)); (Same pattern applies to the other images that are already defined in the CSS.) https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/combined.html:23: <p class="heading">$i18n{headerText}</p> In general, I've been told the WebUI folks prefer <div>s instead of <p>s. https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/inline/combined.js (right): https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/combined.js:1: // Copyright 2016 The Chromium Authors. All rights reserved. We have a lot of dupe code between these JS files that should probably be condensed into a common file. We definitely want to do this for things like onContinue and onSetDefaultBrowser. I'm undecided on whether we want to do this for the functions that contain HTML ids but it might make sense. https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/combined.js:19: function toggleSections(sections, screenshotImages) { |screenshotImages| unused? https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/inline/default.html (right): https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/default.html:23: <div class="section section--expanded"> What is the effect of using section and section--expanded here when there are no other sections? https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/inline/welcome.css (right): https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/welcome.css:5: * { Are these necessary? https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/welcome.css:13: color: rgba(0, 0, 0, .87); I was asked to replace the alpha colors with regular non-translucent grays. I ended up settling on: color: var(--paper-grey-900); which should work since it looks like you're already including the color.html file. https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/welcome.css:16: font: 16px/24px Roboto, 'Helvetica Neue', 'Lucida Grande', sans-serif; You've already included text_defaults_md in the HTML, so this isn't necessary. The caveat is that you might need to leave behind font-size: 100% to keep it from getting scaled down to 13px. https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/welcome.css:22: color: rgb(0, 120, 215); Where possible, let's use polymer colors instead of rolling our own. This one is: var(--google-blue-500) go/colors for what they correspond to https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/welcome.css:27: color: rgba(0, 0, 0, .87); This is redundant if you've already defined it in body https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/welcome.css:37: font-size: 34px; In general, I've been asked to use ems for font sizes and line-heights instead of px. Where it makes sense, also do this for padding/margins around text. e.g., this one is 34px / 16px (default font size) = 2.125em https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/welcome.css:38: letter-spacing: -.44px; This probably isn't necessary. https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/welcome.css:56: border-top: 1px solid rgba(0, 0, 0, .12); I think this is paper-gray-300 https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_handler.cc (right): https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_handler.cc:25: web_ui()->GetWebContents()->GetController().LoadURL( Why use LoadURL instead of Navigate? https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/welcome_handler.... https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_ui.cc (right): https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_ui.cc:36: bool IsInlineVariant(const GURL& url) { Let's set this only using the URL, and not with the feature. Having it both ways seems to be unnecessary complexity, and I'd like to make this decision in the same place that we decide whether or not to show the page at all. When we add the logic to actually show this on startup, we'll: - Add the relevant Feature to startup_features.h https://cs.chromium.org/chromium/src/chrome/browser/ui/startup/startup_featur... - Add the logic which reads the feature and decides which URL to open to startup_tab_provider.cc https://cs.chromium.org/chromium/src/chrome/browser/ui/startup/startup_tab_pr... https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_ui.cc:49: html_source->AddLocalizedString("headerText", IDS_WELCOME_HEADER); I landed Win10-specific versions of the header strings, so let's use IDS_WIN10_WELCOME_HEADER instead. There's also a second string ("Get to Chrome faster" / IDS_WIN10_WELCOME_HEADER_AFTER_FIRST_RUN) that we want to use in place of the regular "Welcome to Chrome" messaging in the instance that they've already run Chrome. We'll also need a URL param that swaps out that text (something like &text=faster). https://cs.chromium.org/chromium/src/chrome/app/chromium_strings.grd?q=IDS_WI... https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_ui.cc:51: // The subheader describes the browser as being "by Google", so we only show I don't think we're actually using this text, are we? https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_ui.cc:134: Profile* profile = Profile::FromWebUI(web_ui); If we're not using profile again, just inline it in the next call.
Thanks for the comments. I'm ready for a second round! https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/inline/combined.html (right): https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/combined.html:22: <img src="logo4x.png" class="header-logo"> On 2016/10/12 22:06:39, tmartino wrote: > We need to support 2x resolutions, so let's change this to a div and then in the > CSS do: > > background-image: -webkit-image-set(url('logo4x.png' 1x, 'logo8x.png' 2x)); > > (Same pattern applies to the other images that are already defined in the CSS.) Done, but I went with content: instead of background-image:. https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/combined.html:23: <p class="heading">$i18n{headerText}</p> On 2016/10/12 22:06:39, tmartino wrote: > In general, I've been told the WebUI folks prefer <div>s instead of <p>s. Done. https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/inline/combined.js (right): https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/combined.js:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/10/12 22:06:40, tmartino wrote: > We have a lot of dupe code between these JS files that should probably be > condensed into a common file. We definitely want to do this for things like > onContinue and onSetDefaultBrowser. I'm undecided on whether we want to do this > for the functions that contain HTML ids but it might make sense. I tried something. What do you think now? https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/combined.js:19: function toggleSections(sections, screenshotImages) { On 2016/10/12 22:06:40, tmartino wrote: > |screenshotImages| unused? Good catch. Removed. https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/inline/default.html (right): https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/default.html:23: <div class="section section--expanded"> On 2016/10/12 22:06:40, tmartino wrote: > What is the effect of using section and section--expanded here when there are no > other sections? The style for this unique section is the same as the visible one in the combined.html. It was the easy way to do it. https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/inline/welcome.css (right): https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/welcome.css:5: * { On 2016/10/12 22:06:40, tmartino wrote: > Are these necessary? Most elements doesn't need the default padding and margin and this seems simpler than to remove it everywhere. font-weight removed. https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/welcome.css:13: color: rgba(0, 0, 0, .87); On 2016/10/12 22:06:40, tmartino wrote: > I was asked to replace the alpha colors with regular non-translucent grays. I > ended up settling on: > color: var(--paper-grey-900); > > which should work since it looks like you're already including the color.html > file. Done. https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/welcome.css:16: font: 16px/24px Roboto, 'Helvetica Neue', 'Lucida Grande', sans-serif; On 2016/10/12 22:06:40, tmartino wrote: > You've already included text_defaults_md in the HTML, so this isn't necessary. > The caveat is that you might need to leave behind font-size: 100% to keep it > from getting scaled down to 13px. Removed and added font-size: 100%. https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/welcome.css:22: color: rgb(0, 120, 215); On 2016/10/12 22:06:40, tmartino wrote: > Where possible, let's use polymer colors instead of rolling our own. This one > is: > > var(--google-blue-500) > > go/colors for what they correspond to Done. https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/welcome.css:27: color: rgba(0, 0, 0, .87); On 2016/10/12 22:06:40, tmartino wrote: > This is redundant if you've already defined it in body Removed. https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/welcome.css:37: font-size: 34px; On 2016/10/12 22:06:40, tmartino wrote: > In general, I've been asked to use ems for font sizes and line-heights instead > of px. Where it makes sense, also do this for padding/margins around text. > > e.g., this one is 34px / 16px (default font size) = 2.125em Done. https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/welcome.css:38: letter-spacing: -.44px; On 2016/10/12 22:06:40, tmartino wrote: > This probably isn't necessary. Removed. https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/welcome.css:56: border-top: 1px solid rgba(0, 0, 0, .12); On 2016/10/12 22:06:40, tmartino wrote: > I think this is paper-gray-300 Done. https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_handler.cc (right): https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_handler.cc:25: web_ui()->GetWebContents()->GetController().LoadURL( On 2016/10/12 22:06:40, tmartino wrote: > Why use LoadURL instead of Navigate? > > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/welcome_handler.... The chrome::Navigate() function is the generalized version where you don't have access to a WebContents. Internally, it will call LoadURL from the controller. Also, using chrome::Navigate() requires an inefficient call to GetBrowserForWebContents() (loops over all WebContents of the browser process). IMO we should change the navigate in welcome_ui.cc too. https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_ui.cc (right): https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_ui.cc:36: bool IsInlineVariant(const GURL& url) { On 2016/10/12 22:06:40, tmartino wrote: > Let's set this only using the URL, and not with the feature. Having it both ways > seems to be unnecessary complexity, and I'd like to make this decision in the > same place that we decide whether or not to show the page at all. When we add > the logic to actually show this on startup, we'll: > > - Add the relevant Feature to startup_features.h > https://cs.chromium.org/chromium/src/chrome/browser/ui/startup/startup_featur... > > - Add the logic which reads the feature and decides which URL to open to > startup_tab_provider.cc > https://cs.chromium.org/chromium/src/chrome/browser/ui/startup/startup_tab_pr... Done since I don't have a strong opinion. IMO the additional complexity here is negligible (2 lines of code) and would have made the code in startup_tab_provider only deal with real functionality. https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_ui.cc:49: html_source->AddLocalizedString("headerText", IDS_WELCOME_HEADER); On 2016/10/12 22:06:40, tmartino wrote: > I landed Win10-specific versions of the header strings, so let's use > IDS_WIN10_WELCOME_HEADER instead. > > There's also a second string ("Get to Chrome faster" / > IDS_WIN10_WELCOME_HEADER_AFTER_FIRST_RUN) that we want to use in place of the > regular "Welcome to Chrome" messaging in the instance that they've already run > Chrome. We'll also need a URL param that swaps out that text (something like > &text=faster). > > https://cs.chromium.org/chromium/src/chrome/app/chromium_strings.grd?q=IDS_WI... Done. https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_ui.cc:51: // The subheader describes the browser as being "by Google", so we only show On 2016/10/12 22:06:40, tmartino wrote: > I don't think we're actually using this text, are we? Removed. https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_ui.cc:134: Profile* profile = Profile::FromWebUI(web_ui); On 2016/10/12 22:06:40, tmartino wrote: > If we're not using profile again, just inline it in the next call. Done.
lgtm https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_handler.cc (right): https://codereview.chromium.org/2401853005/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_handler.cc:25: web_ui()->GetWebContents()->GetController().LoadURL( On 2016/10/13 at 21:30:05, Patrick Monette wrote: > On 2016/10/12 22:06:40, tmartino wrote: > > Why use LoadURL instead of Navigate? > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/welcome_handler.... > > The chrome::Navigate() function is the generalized version where you don't have access to a WebContents. Internally, it will call LoadURL from the controller. > > Also, using chrome::Navigate() requires an inefficient call to GetBrowserForWebContents() (loops over all WebContents of the browser process). > > IMO we should change the navigate in welcome_ui.cc too. Noted, will change in tech debt phase. https://codereview.chromium.org/2401853005/diff/40001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/default-browser.js (right): https://codereview.chromium.org/2401853005/diff/40001/chrome/browser/resource... chrome/browser/resources/welcome/win10/default-browser.js:1: // Copyright 2016 The Chromium Authors. All rights reserved. I'm going to defer to your OWNERS reviewer on whether it's better to split these included JS files up so we're only including the explicitly necessary portions, or to treat them like a broader API and have one JS file for all the possible variations, where only a subset of the methods are called. https://codereview.chromium.org/2401853005/diff/40001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/inline/welcome.css (right): https://codereview.chromium.org/2401853005/diff/40001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/welcome.css:222: @media (min-width: 312px) { IIRC these are the values keeping the font at the right size when the image scales. Can you add a comment explaining that? They seem pretty random right now. https://codereview.chromium.org/2401853005/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_ui.cc (right): https://codereview.chromium.org/2401853005/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_ui.cc:34: // combinaison of |is_first_run| and |is_combined_variant|. nit: combination, not combinaison https://codereview.chromium.org/2401853005/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_ui.cc:38: // Don't show the "Welcome to Chrome" text when it's th Something got deleted here?
pmonette@chromium.org changed reviewers: + michaelpg@chromium.org
Thanks Tommy. michaelpg@ Please take a look! https://codereview.chromium.org/2401853005/diff/40001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/default-browser.js (right): https://codereview.chromium.org/2401853005/diff/40001/chrome/browser/resource... chrome/browser/resources/welcome/win10/default-browser.js:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/10/14 19:32:09, tmartino wrote: > I'm going to defer to your OWNERS reviewer on whether it's better to split these > included JS files up so we're only including the explicitly necessary portions, > or to treat them like a broader API and have one JS file for all the possible > variations, where only a subset of the methods are called. Acknowledged. https://codereview.chromium.org/2401853005/diff/40001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/inline/welcome.css (right): https://codereview.chromium.org/2401853005/diff/40001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/welcome.css:222: @media (min-width: 312px) { On 2016/10/14 19:32:09, tmartino wrote: > IIRC these are the values keeping the font at the right size when the image > scales. Can you add a comment explaining that? They seem pretty random right > now. Done. https://codereview.chromium.org/2401853005/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_ui.cc (right): https://codereview.chromium.org/2401853005/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_ui.cc:34: // combinaison of |is_first_run| and |is_combined_variant|. On 2016/10/14 19:32:09, tmartino wrote: > nit: combination, not combinaison Done. https://codereview.chromium.org/2401853005/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_ui.cc:38: // Don't show the "Welcome to Chrome" text when it's th On 2016/10/14 19:32:10, tmartino wrote: > Something got deleted here? Fixed.
Only part-way through the CL. Before I review further, can you explain why there are so many files with similar content? It would seem simpler to have one UI with certain parts shown or hidden depending on whatever state decides that. (Polymer makes that relatively easy and cheap, e.g. <template is="dom-if">) https://codereview.chromium.org/2401853005/diff/60001/chrome/browser/browser_... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/2401853005/diff/60001/chrome/browser/browser_... chrome/browser/browser_resources.grd:81: <structure name="IDR_WELCOME_WIN10_DEFAULT_BROWSER_JS" file="resources\welcome\win10\default-browser.js" type="chrome_html" preprocess="true"/> most of these don't need preprocess="true", unless they use <if> type expressions (or you want to inline URLs in css maybe?) https://codereview.chromium.org/2401853005/diff/60001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/default-browser.js (right): https://codereview.chromium.org/2401853005/diff/60001/chrome/browser/resource... chrome/browser/resources/welcome/win10/default-browser.js:9: $('section-steps--click-edge').innerHTML = why are these not done with $i18n{}? or $i18nRaw{} if you need to preserve HTML https://codereview.chromium.org/2401853005/diff/60001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/inline/combined.html (right): https://codereview.chromium.org/2401853005/diff/60001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/combined.html:11: <link rel="import" href="chrome://resources/polymer/v1_0/iron-icons/iron-icons.html"> I recommend creating your own iconset by copying the icons you need instead of importing 1,000 icons: https://www.chromium.org/developers/updating-webui-for-material-design/using-... https://codereview.chromium.org/2401853005/diff/60001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/combined.html:15: <link rel="stylesheet" href="chrome://welcome-win10/welcome.css"> strongly recommend using root-absolute paths: href="/welcome.css" so if the base "welcome-win10" name changes, we don't have to change everything https://codereview.chromium.org/2401853005/diff/60001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/combined.html:17: <script src="chrome://welcome-win10/welcome-win10.js"></script> These are a lot of dependencies. It's better to enforce (and dedupe) script dependencies via HTML imports (and then we can just use HTML imports here and alphabetize them). Please: * create a corresponding HTML file for each script * in each HTML file, <script src=""> the corresponding JS file * in each HTML file, add <link rel="import"> to import the HTML file for any script that script depends on * replace these <script>s with <link rel="import"> and alphabetize https://codereview.chromium.org/2401853005/diff/60001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/combined.html:28: <div class="section section--expanded"> I don't understand the -- scoping here. Classes can and should stack: <div class="section expanded"> <div class="heading expandable"> // do something with visible expandable headings: document.querySelector('.expanded heading.expandable'); https://codereview.chromium.org/2401853005/diff/60001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/combined.html:30: <div class="section-heading-text">$i18n{defaultBrowserSubheaderText}</div> 80-col limit, throughout (excluding tags w/ URLs like <link rel>)
Thanks! I didn't want to create a Polymer custom element just to use dom-if so I tried something using dom-bind. Let me know what you think. https://codereview.chromium.org/2401853005/diff/60001/chrome/browser/browser_... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/2401853005/diff/60001/chrome/browser/browser_... chrome/browser/browser_resources.grd:81: <structure name="IDR_WELCOME_WIN10_DEFAULT_BROWSER_JS" file="resources\welcome\win10\default-browser.js" type="chrome_html" preprocess="true"/> On 2016/10/16 23:09:17, michaelpg wrote: > most of these don't need preprocess="true", unless they use <if> type > expressions (or you want to inline URLs in css maybe?) Removed. https://codereview.chromium.org/2401853005/diff/60001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/default-browser.js (right): https://codereview.chromium.org/2401853005/diff/60001/chrome/browser/resource... chrome/browser/resources/welcome/win10/default-browser.js:9: $('section-steps--click-edge').innerHTML = On 2016/10/16 23:09:17, michaelpg wrote: > why are these not done with $i18n{}? or $i18nRaw{} if you need to preserve HTML I was not aware of $i18nRaw{}; it is doing exactly what I want. Fixed. https://codereview.chromium.org/2401853005/diff/60001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/inline/combined.html (right): https://codereview.chromium.org/2401853005/diff/60001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/combined.html:11: <link rel="import" href="chrome://resources/polymer/v1_0/iron-icons/iron-icons.html"> On 2016/10/16 23:09:18, michaelpg wrote: > I recommend creating your own iconset by copying the icons you need instead of > importing 1,000 icons: > https://www.chromium.org/developers/updating-webui-for-material-design/using-... The icons I need already exist in the cr-elments/icons.html file so I used it. Let me know if you'd still prefer I create my own. https://codereview.chromium.org/2401853005/diff/60001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/combined.html:15: <link rel="stylesheet" href="chrome://welcome-win10/welcome.css"> On 2016/10/16 23:09:17, michaelpg wrote: > strongly recommend using root-absolute paths: > > href="/welcome.css" > > so if the base "welcome-win10" name changes, we don't have to change everything Done. https://codereview.chromium.org/2401853005/diff/60001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/combined.html:17: <script src="chrome://welcome-win10/welcome-win10.js"></script> On 2016/10/16 23:09:17, michaelpg wrote: > These are a lot of dependencies. It's better to enforce (and dedupe) script > dependencies via HTML imports (and then we can just use HTML imports here and > alphabetize them). Please: > > * create a corresponding HTML file for each script > * in each HTML file, <script src=""> the corresponding JS file > * in each HTML file, add <link rel="import"> to import the HTML file for any > script that script depends on > * replace these <script>s with <link rel="import"> and alphabetize I consolidated the JS files so I think this is not needed anymore. https://codereview.chromium.org/2401853005/diff/60001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/combined.html:28: <div class="section section--expanded"> On 2016/10/16 23:09:17, michaelpg wrote: > I don't understand the -- scoping here. Classes can and should stack: > <div class="section expanded"> > <div class="heading expandable"> > > // do something with visible expandable headings: > document.querySelector('.expanded heading.expandable'); Done. https://codereview.chromium.org/2401853005/diff/60001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline/combined.html:30: <div class="section-heading-text">$i18n{defaultBrowserSubheaderText}</div> On 2016/10/16 23:09:17, michaelpg wrote: > 80-col limit, throughout (excluding tags w/ URLs like <link rel>) Done.
Thank you, looking better. https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/inline.css (right): https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline.css:5: * { err, can we be more specific? You probably mean "html, body" and possibly "ol". https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline.css:100: margin-left: 1.25em; make RTL friendly https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline.css:176: url(https://www.gstatic.com/chrome/login/win10/default-small.webp) 1x, o_O why are we loading from the web? https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline.css:228: scaling at the same time the image starts scaling too. */ nit: /* This value... * scaling at... */ https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/sectioned.css (right): https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/resource... chrome/browser/resources/welcome/win10/sectioned.css:1: /* Copyright 2016 The Chromium Authors. All rights reserved. There is still a great deal of duplicated code here. Any reason we can't make a "common.css"? https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:578: /**************************************************************************** existing spacing was correct (preprocessor directives are always 0-aligned and do not affect indentation of enclosed code) https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_handler.cc (right): https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_handler.cc:14: WelcomeWin10Handler::WelcomeWin10Handler(content::WebUI* web_ui) {} don't take an unused parameter https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_handler.cc:18: void WelcomeWin10Handler::HandleSetDefaultBrowser(const base::ListValue* args) { order by header file declaration (ie public, then private) https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_handler.h (right): https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_handler.h:21: void HandleSetDefaultBrowser(const base::ListValue* args); nit: forward declare ListValue https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_ui.h (right): https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_ui.h:9: #include "url/gurl.h" nit: forward declare GURL instead and include in .cc https://codereview.chromium.org/2401853005/diff/80001/chrome/common/url_const... File chrome/common/url_constants.h (right): https://codereview.chromium.org/2401853005/diff/80001/chrome/common/url_const... chrome/common/url_constants.h:91: extern const char kChromeUIWelcomeWin10URL[]; Wondering if it would be better to just use kChromeUIWelcomeURL (chrome://welcome) and create the webui depending on the OS.
Thanks! I've responded inline. PTAnL https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/inline.css (right): https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline.css:5: * { On 2016/10/22 03:19:00, michaelpg wrote: > err, can we be more specific? You probably mean "html, body" and possibly "ol". Done. https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline.css:100: margin-left: 1.25em; On 2016/10/22 03:19:00, michaelpg wrote: > make RTL friendly First time doing this. I added a second styling for [dir="rtl"] that overrides the regular style. Is that enough? https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline.css:176: url(https://www.gstatic.com/chrome/login/win10/default-small.webp) 1x, On 2016/10/22 03:19:00, michaelpg wrote: > o_O why are we loading from the web? It was a conscious decision made in order to not inflate the binary size. I don't think there is a policy against that, but if this is a problem, we could bundle the images. That would add ~78 kb to the binary. https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline.css:228: scaling at the same time the image starts scaling too. */ On 2016/10/22 03:19:00, michaelpg wrote: > nit: > > /* This value... > * scaling at... */ > Done. https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/sectioned.css (right): https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/resource... chrome/browser/resources/welcome/win10/sectioned.css:1: /* Copyright 2016 The Chromium Authors. All rights reserved. On 2016/10/22 03:19:00, michaelpg wrote: > There is still a great deal of duplicated code here. Any reason we can't make a > "common.css"? Not a very good reason but we'll eventually get rid of one of the versions after doing some A/B testing. Keeping them separate will make this removal as easy as possible. If you feel strongly about this I'll make a common .css file. https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:578: /**************************************************************************** On 2016/10/22 03:19:00, michaelpg wrote: > existing spacing was correct > > (preprocessor directives are always 0-aligned and do not affect indentation of > enclosed code) Fixed. https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_handler.cc (right): https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_handler.cc:14: WelcomeWin10Handler::WelcomeWin10Handler(content::WebUI* web_ui) {} On 2016/10/22 03:19:01, michaelpg wrote: > don't take an unused parameter Removed. As an aside, it's a bit weird that web_ui_ is not initialized in the constructor, but in AddMessageHandler() via set_web_ui(); https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_handler.cc:18: void WelcomeWin10Handler::HandleSetDefaultBrowser(const base::ListValue* args) { On 2016/10/22 03:19:01, michaelpg wrote: > order by header file declaration (ie public, then private) Done. https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_handler.h (right): https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_handler.h:21: void HandleSetDefaultBrowser(const base::ListValue* args); On 2016/10/22 03:19:01, michaelpg wrote: > nit: forward declare ListValue Done. https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_ui.h (right): https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_ui.h:9: #include "url/gurl.h" On 2016/10/22 03:19:01, michaelpg wrote: > nit: forward declare GURL instead and include in .cc Done. https://codereview.chromium.org/2401853005/diff/80001/chrome/common/url_const... File chrome/common/url_constants.h (right): https://codereview.chromium.org/2401853005/diff/80001/chrome/common/url_const... chrome/common/url_constants.h:91: extern const char kChromeUIWelcomeWin10URL[]; On 2016/10/22 03:19:01, michaelpg wrote: > Wondering if it would be better to just use kChromeUIWelcomeURL > (chrome://welcome) and create the webui depending on the OS. I don't think we've quite figured out what we're gonna do about the URL when we start using it (and the chrome://welcome-win10 is more of a placeholder), but I've been specifically asked to create a new URL for easier testing by the designers. This is also why there's support for key/value query support in welcome_win10_ui.cc. In theory, the page could just load the right one based directly on an experiment.
https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/inline.css (right): https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline.css:176: url(https://www.gstatic.com/chrome/login/win10/default-small.webp) 1x, On 2016/10/24 at 20:49:39, Patrick Monette wrote: > On 2016/10/22 03:19:00, michaelpg wrote: > > o_O why are we loading from the web? > > It was a conscious decision made in order to not inflate the binary size. > > I don't think there is a policy against that, but if this is a problem, we could bundle the images. That would add ~78 kb to the binary. Michael, I'm forwarding you the thread where this decision was made. https://codereview.chromium.org/2401853005/diff/80001/chrome/common/url_const... File chrome/common/url_constants.h (right): https://codereview.chromium.org/2401853005/diff/80001/chrome/common/url_const... chrome/common/url_constants.h:91: extern const char kChromeUIWelcomeWin10URL[]; On 2016/10/24 at 20:49:40, Patrick Monette wrote: > On 2016/10/22 03:19:01, michaelpg wrote: > > Wondering if it would be better to just use kChromeUIWelcomeURL > > (chrome://welcome) and create the webui depending on the OS. > > I don't think we've quite figured out what we're gonna do about the URL when we start using it (and the chrome://welcome-win10 is more of a placeholder), but I've been specifically asked to create a new URL for easier testing by the designers. > > This is also why there's support for key/value query support in welcome_win10_ui.cc. In theory, the page could just load the right one based directly on an experiment. This was my choice so I'll chime in here. The UI testing piece is definitely part of it, but it's a bit more substantial than that. I've been doing a rewrite of the code that determines what tabs to show at startup. A basic overriding principle of the rewrite was to try and cut down on the number of layers that interact with system state. The old flow is some truly atrocious spaghetti, making it really hard to determine which levels of examining/branching on state may preclude which other ones. Restructuring also allowed me to introduce unit tests to make sure the correct sets of system state would land on the correct pages (and their correct variants). I chose to use URLs to modify the welcome and welcome-win10 pages as an extension of that principle. A lot of the variables we would be switching on for these variants are also used when deciding whether we want to show the page at all. We could check these values at construct-time, but that introduces a second, independent place where we're checking these factors and making decisions based on them, which IMO is a risk for future bugs and spaghetti code.
lgtm. LMK if you'd like me to take a final look after the indent fixes and possibly README. https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/inline.css (right): https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline.css:100: margin-left: 1.25em; On 2016/10/24 20:49:39, Patrick Monette wrote: > On 2016/10/22 03:19:00, michaelpg wrote: > > make RTL friendly > > First time doing this. I added a second styling for [dir="rtl"] that overrides > the regular style. > > Is that enough? That's fine. Technically, the style guide prefers using -webkit-padding-start, -webkit-padding-end, -webkit-margin-start, etc.: http://www.chromium.org/developers/web-development-style-guide but it amounts to the same thing. https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline.css:176: url(https://www.gstatic.com/chrome/login/win10/default-small.webp) 1x, On 2016/10/24 21:06:20, tmartino wrote: > On 2016/10/24 at 20:49:39, Patrick Monette wrote: > > On 2016/10/22 03:19:00, michaelpg wrote: > > > o_O why are we loading from the web? > > > > It was a conscious decision made in order to not inflate the binary size. > > > > I don't think there is a policy against that, but if this is a problem, we > could bundle the images. That would add ~78 kb to the binary. > > Michael, I'm forwarding you the thread where this decision was made. If laforge is ok with it and it went through a security review, SGTM. https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/sectioned.css (right): https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/resource... chrome/browser/resources/welcome/win10/sectioned.css:1: /* Copyright 2016 The Chromium Authors. All rights reserved. On 2016/10/24 20:49:39, Patrick Monette wrote: > On 2016/10/22 03:19:00, michaelpg wrote: > > There is still a great deal of duplicated code here. Any reason we can't make > a > > "common.css"? > > Not a very good reason but we'll eventually get rid of one of the versions after > doing some A/B testing. I didn't realize that. Can you add comments -- or even better, a README.md -- to explain that, with a link to the bug to make the decision? > Keeping them separate will make this removal as easy as > possible. > > If you feel strongly about this I'll make a common .css file. https://codereview.chromium.org/2401853005/diff/80001/chrome/common/url_const... File chrome/common/url_constants.h (right): https://codereview.chromium.org/2401853005/diff/80001/chrome/common/url_const... chrome/common/url_constants.h:91: extern const char kChromeUIWelcomeWin10URL[]; On 2016/10/24 21:06:20, tmartino wrote: > On 2016/10/24 at 20:49:40, Patrick Monette wrote: > > On 2016/10/22 03:19:01, michaelpg wrote: > > > Wondering if it would be better to just use kChromeUIWelcomeURL > > > (chrome://welcome) and create the webui depending on the OS. > > > > I don't think we've quite figured out what we're gonna do about the URL when > we start using it (and the chrome://welcome-win10 is more of a placeholder), but > I've been specifically asked to create a new URL for easier testing by the > designers. > > > > This is also why there's support for key/value query support in > welcome_win10_ui.cc. In theory, the page could just load the right one based > directly on an experiment. > > This was my choice so I'll chime in here. The UI testing piece is definitely > part of it, but it's a bit more substantial than that. > > I've been doing a rewrite of the code that determines what tabs to show at > startup. > A basic overriding principle of the rewrite was to try and cut down on > the number of layers that interact with system state. The old flow is some truly > atrocious spaghetti, making it really hard to determine which levels of > examining/branching on state may preclude which other ones. Restructuring also > allowed me to introduce unit tests to make sure the correct sets of system state > would land on the correct pages (and their correct variants). > > I chose to use URLs to modify the welcome and welcome-win10 pages as an > extension of that principle. A lot of the variables we would be switching on for > these variants are also used when deciding whether we want to show the page at > all. We could check these values at construct-time, but that introduces a > second, independent place where we're checking these factors and making > decisions based on them, which IMO is a risk for future bugs and spaghetti code. Thanks for the context. https://codereview.chromium.org/2401853005/diff/100001/chrome/browser/resourc... File chrome/browser/resources/welcome/win10/inline.html (right): https://codereview.chromium.org/2401853005/diff/100001/chrome/browser/resourc... chrome/browser/resources/welcome/win10/inline.html:42: id="screenshot-html-overlay--browser"> throughout: 4-space indent (not align)
Thanks. If you don't mind, you can definitely take a last look. https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/resource... File chrome/browser/resources/welcome/win10/inline.css (right): https://codereview.chromium.org/2401853005/diff/80001/chrome/browser/resource... chrome/browser/resources/welcome/win10/inline.css:100: margin-left: 1.25em; On 2016/10/24 21:30:08, michaelpg wrote: > On 2016/10/24 20:49:39, Patrick Monette wrote: > > On 2016/10/22 03:19:00, michaelpg wrote: > > > make RTL friendly > > > > First time doing this. I added a second styling for [dir="rtl"] that overrides > > the regular style. > > > > Is that enough? > > That's fine. Technically, the style guide prefers using -webkit-padding-start, > -webkit-padding-end, -webkit-margin-start, etc.: > http://www.chromium.org/developers/web-development-style-guide but it amounts to > the same thing. Okay I went with -webkit-*-start. I like it better and there are less lines of code. https://codereview.chromium.org/2401853005/diff/100001/chrome/browser/resourc... File chrome/browser/resources/welcome/win10/inline.html (right): https://codereview.chromium.org/2401853005/diff/100001/chrome/browser/resourc... chrome/browser/resources/welcome/win10/inline.html:42: id="screenshot-html-overlay--browser"> On 2016/10/24 21:30:08, michaelpg wrote: > throughout: 4-space indent (not align) Done.
great, slgtm
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tmartino@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/2401853005/#ps120001 (title: "Adding Readme")
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.
Description was changed from ========== Adding client code for new Windows 10 First Run Experience BUG=648686 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Adding client code for new Windows 10 First Run Experience BUG=648686 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Adding client code for new Windows 10 First Run Experience BUG=648686 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Adding client code for new Windows 10 First Run Experience BUG=648686 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/218e0aa27614cab0d01aa72f01cbb33d853a500d Cr-Commit-Position: refs/heads/master@{#427263} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/218e0aa27614cab0d01aa72f01cbb33d853a500d Cr-Commit-Position: refs/heads/master@{#427263} |
