Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2804823002:
Revamp the Incognito NTP on Desktop (Closed)
|
Created:
3 years, 8 months ago by msramek Modified:
3 years, 8 months ago CC:
chromium-reviews, dbeam+watch-ntp_chromium.org, ntp-dev+reviews_chromium.org, noyau+watch_chromium.org, arv+watch_chromium.org, asvitkine+watch_chromium.org, pedrosimonetti+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRevamp the Incognito NTP on Desktop
Changes in this CL:
1. Added an about:flag for the "MD Incognito NTP". (Note that the primary
purpose of this change is to improve the text on the Incognito NTP, not
just to materialize it; but the latter results in a shorter flag name and
thematically fits with the other recent MD improvements).
2. Since almost none of the old CSS is valid for the new MD, and since
there are also related HTML changes, we use separate HTML and CSS files.
NTPResourceCache decides which one to serve according to the flag. Once
this fully launches, we will simply remove the previous implementation.
3. The new design follows the CSS rules described in
https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZeKud1YOmPiI.
The requirement that the two sets of bulletpoints determine the width of
the content box has been achieved by JS (the CSS is complicated enough as
it is :-( ).
Tested with the bookmark bar on/off, with a custom theme on/off, and in
front of a UX designer :)
BUG=693525
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2804823002
Cr-Commit-Position: refs/heads/master@{#466817}
Committed: https://chromium.googlesource.com/chromium/src/+/a8443118aa963ba364ee9687b82d1d9c7eb26af5
Patch Set 1 #
Total comments: 23
Patch Set 2 : Addressed comments. #Patch Set 3 : Rebase, fix merge conflict by moving the flag to its alphabetical position #Patch Set 4 : Font sizes #
Total comments: 3
Patch Set 5 : Rebase. #Patch Set 6 : Rebase and updates #Patch Set 7 : Fixed histograms.xml #
Total comments: 12
Patch Set 8 : Addressed comments. #
Total comments: 1
Messages
Total messages: 62 (47 generated)
Description was changed from ========== Revamp the Incognito NTP on Desktop Changes in this CL: 1. Added an about:flag for the "MD Incognito NTP". (Note that the primary purpose of this change is to improve the text on the Incognito NTP, not just to materialize it; but the latter results in a shorter flag name and thematically fits with the other recent MD improvements). 2. Since almost none of the old CSS is valid for the new MD, and since there are also related HTML changes, we use separate HTML and CSS files. NTPResourceCache decides which one to serve according to the flag. Once this fully launches, we will simply remove the previous implementation. 3. The new design follows the CSS rules described in https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZeKud1YOmPiI. The requirement that the two sets of bulletpoints determine the width of the content box has been achieved by JS (the CSS is complicated enough as it is :-( ). Tested with the bookmark bar on/off, with a custom theme on/off, and in front of a UX designer :) BUG=693525 ========== to ========== Revamp the Incognito NTP on Desktop Changes in this CL: 1. Added an about:flag for the "MD Incognito NTP". (Note that the primary purpose of this change is to improve the text on the Incognito NTP, not just to materialize it; but the latter results in a shorter flag name and thematically fits with the other recent MD improvements). 2. Since almost none of the old CSS is valid for the new MD, and since there are also related HTML changes, we use separate HTML and CSS files. NTPResourceCache decides which one to serve according to the flag. Once this fully launches, we will simply remove the previous implementation. 3. The new design follows the CSS rules described in https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZeKud1YOmPiI. The requirement that the two sets of bulletpoints determine the width of the content box has been achieved by JS (the CSS is complicated enough as it is :-( ). Tested with the bookmark bar on/off, with a custom theme on/off, and in front of a UX designer :) BUG=693525 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
msramek@chromium.org changed reviewers: + estade@chromium.org
Hi Evan! Please have a look! This is a super complicated stylesheet, but when you look at the specs linked in the CL description, I think you'll agree that there is a lot of entropy. The way I wrote it is that: - I divided it into semantical sections. - In each section, I started with the largest screen size and then wrote diffs for what happens at various breakpoints. It might be possible to make the CSS shorter, but I find the current approach well maintainable. Cheers, Martin
estade@chromium.org changed reviewers: + dbeam@chromium.org
sorry I didn't notice this review in a timely manner. I'm punting it to dbeam because he's more active in webui these days.
Description was changed from ========== Revamp the Incognito NTP on Desktop Changes in this CL: 1. Added an about:flag for the "MD Incognito NTP". (Note that the primary purpose of this change is to improve the text on the Incognito NTP, not just to materialize it; but the latter results in a shorter flag name and thematically fits with the other recent MD improvements). 2. Since almost none of the old CSS is valid for the new MD, and since there are also related HTML changes, we use separate HTML and CSS files. NTPResourceCache decides which one to serve according to the flag. Once this fully launches, we will simply remove the previous implementation. 3. The new design follows the CSS rules described in https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZeKud1YOmPiI. The requirement that the two sets of bulletpoints determine the width of the content box has been achieved by JS (the CSS is complicated enough as it is :-( ). Tested with the bookmark bar on/off, with a custom theme on/off, and in front of a UX designer :) BUG=693525 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Revamp the Incognito NTP on Desktop Changes in this CL: 1. Added an about:flag for the "MD Incognito NTP". (Note that the primary purpose of this change is to improve the text on the Incognito NTP, not just to materialize it; but the latter results in a shorter flag name and thematically fits with the other recent MD improvements). 2. Since almost none of the old CSS is valid for the new MD, and since there are also related HTML changes, we use separate HTML and CSS files. NTPResourceCache decides which one to serve according to the flag. Once this fully launches, we will simply remove the previous implementation. 3. The new design follows the CSS rules described in https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZeKud1YOmPiI. The requirement that the two sets of bulletpoints determine the width of the content box has been achieved by JS (the CSS is complicated enough as it is :-( ). Tested with the bookmark bar on/off, with a custom theme on/off, and in front of a UX designer :) BUG=693525 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
estade@chromium.org changed reviewers: - estade@chromium.org
Sorry for lag, will get to this Monday my time
https://codereview.chromium.org/2804823002/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2804823002/diff/1/chrome/browser/about_flags.... chrome/browser/about_flags.cc:2503: SINGLE_VALUE_TYPE(switches::kEnableMaterialDesignIncognitoNTP)}, can we use a feature instead of a switch? https://codereview.chromium.org/2804823002/diff/1/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/md_incognito_tab.css (right): https://codereview.chromium.org/2804823002/diff/1/chrome/browser/resources/nt... chrome/browser/resources/ntp4/md_incognito_tab.css:9: background-color: #303030 !important; i'm still confused as to why you need !important here. can we just make this selector more specific? maybe add a class="md" to the <html> element on the new page? https://codereview.chromium.org/2804823002/diff/1/chrome/browser/resources/nt... chrome/browser/resources/ntp4/md_incognito_tab.css:21: color: rgb(70%, 70%, 70%); note: an overwhelming majority of rgb() and rgba() colors in chrome currently use 0-255 decimal values rather than % but there's no style rule (just precedence) https://codereview.chromium.org/2804823002/diff/1/chrome/browser/resources/nt... chrome/browser/resources/ntp4/md_incognito_tab.css:22: font-size: 14px; can you make font-sizes in terms of scalable units (em, %, or rem)? this is so the user's default font size is respected from settings https://codereview.chromium.org/2804823002/diff/1/chrome/browser/resources/nt... chrome/browser/resources/ntp4/md_incognito_tab.css:31: opacity: rgb(80%, 80%, 80%); i don't think this is a valid opacity. did you mean opacity: .8; or opacity: 80%? or maybe color: rgba(0, 0, 0, .8)? https://codereview.chromium.org/2804823002/diff/1/chrome/browser/resources/nt... chrome/browser/resources/ntp4/md_incognito_tab.css:124: float: left; does this work in RTL? https://codereview.chromium.org/2804823002/diff/1/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/md_incognito_tab.html (right): https://codereview.chromium.org/2804823002/diff/1/chrome/browser/resources/nt... chrome/browser/resources/ntp4/md_incognito_tab.html:2: <html i18n-values="dir:textdirection; don't use i18n-values, but use $i18n{} and $i18nRaw{} instead https://codereview.chromium.org/2804823002/diff/1/chrome/browser/resources/nt... chrome/browser/resources/ntp4/md_incognito_tab.html:40: <script> prefer not to use inline scripts, it violates some forms of CSP https://codereview.chromium.org/2804823002/diff/1/chrome/browser/resources/nt... chrome/browser/resources/ntp4/md_incognito_tab.html:55: document.getElementById('incognitothemecss').href = can you use $() instead of document.getElementById? https://codereview.chromium.org/2804823002/diff/1/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): https://codereview.chromium.org/2804823002/diff/1/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:288: int new_tab_html_idr; all these uninitialized ints make me a little nervous https://codereview.chromium.org/2804823002/diff/1/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:300: // new_tab_features_ids is not used in the old version. new_tab_feature_ids is not initialized explicitly, meaning it could be garbage
Thanks, Dan! PTAL :) I addressed all your comments except the relative font sizes. I'll chat with our UX designer about what should be fixed and what should be relative. https://codereview.chromium.org/2804823002/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2804823002/diff/1/chrome/browser/about_flags.... chrome/browser/about_flags.cc:2503: SINGLE_VALUE_TYPE(switches::kEnableMaterialDesignIncognitoNTP)}, On 2017/04/11 00:31:12, Dan Beam wrote: > can we use a feature instead of a switch? Done. I also changed the OS to Desktop; I'll flip it back to All once I have the Android CL ready. https://codereview.chromium.org/2804823002/diff/1/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/md_incognito_tab.css (right): https://codereview.chromium.org/2804823002/diff/1/chrome/browser/resources/nt... chrome/browser/resources/ntp4/md_incognito_tab.css:9: background-color: #303030 !important; On 2017/04/11 00:31:12, Dan Beam wrote: > i'm still confused as to why you need !important here. can we just make this > selector more specific? maybe add a class="md" to the <html> element on the new > page? Done. Sorry, not sure how I didn't think of that. https://codereview.chromium.org/2804823002/diff/1/chrome/browser/resources/nt... chrome/browser/resources/ntp4/md_incognito_tab.css:21: color: rgb(70%, 70%, 70%); On 2017/04/11 00:31:12, Dan Beam wrote: > note: an overwhelming majority of rgb() and rgba() colors in chrome currently > use 0-255 decimal values rather than % but there's no style rule (just > precedence) Done. https://codereview.chromium.org/2804823002/diff/1/chrome/browser/resources/nt... chrome/browser/resources/ntp4/md_incognito_tab.css:22: font-size: 14px; On 2017/04/11 00:31:12, Dan Beam wrote: > can you make font-sizes in terms of scalable units (em, %, or rem)? > > this is so the user's default font size is respected from settings Well, this might be a problem. The default font size is 13px. 14px/13px has infinite decimal expansion, as do 24/14, 24/13, 20/14, and 20/13. Let me circle back with our UX designer if we really want to use 1px larger font than the default. https://codereview.chromium.org/2804823002/diff/1/chrome/browser/resources/nt... chrome/browser/resources/ntp4/md_incognito_tab.css:31: opacity: rgb(80%, 80%, 80%); On 2017/04/11 00:31:12, Dan Beam wrote: > i don't think this is a valid opacity. did you mean opacity: .8; or opacity: > 80%? or maybe color: rgba(0, 0, 0, .8)? Done. It was supposed to be just color, actually. https://codereview.chromium.org/2804823002/diff/1/chrome/browser/resources/nt... chrome/browser/resources/ntp4/md_incognito_tab.css:124: float: left; On 2017/04/11 00:31:12, Dan Beam wrote: > does this work in RTL? Done. Thanks for pointing this out! I flipped the floats manually using dir=rtl, and replaced some "-left" attributes with "-start". My understanding is that this should be sufficient, but it's a bit difficult to test, since HTML with dir=rtl and LTR language behaves a quite unpredictably, and Chrome doesn't allow changing the UI language on Linux. I'll check again tomorrow on Mac. https://codereview.chromium.org/2804823002/diff/1/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/md_incognito_tab.html (right): https://codereview.chromium.org/2804823002/diff/1/chrome/browser/resources/nt... chrome/browser/resources/ntp4/md_incognito_tab.html:2: <html i18n-values="dir:textdirection; On 2017/04/11 00:31:12, Dan Beam wrote: > don't use i18n-values, but use $i18n{} and $i18nRaw{} instead Done. For the record, the majority of this file is just a copy-paste from the current Incognito NTP. One important difference is that "hasCustomBackground" and "bookmarkbarattached" are set by the C++ code as booleans. i18n-values seems not to mind, but $i18n{} only works with strings. And indeed, ui::TemplateReplacementsFromDictionaryValue() ignores everything except strings. So I changed them to strings. I tested that nothing broke on the current Incognito NTP. https://codereview.chromium.org/2804823002/diff/1/chrome/browser/resources/nt... chrome/browser/resources/ntp4/md_incognito_tab.html:40: <script> On 2017/04/11 00:31:12, Dan Beam wrote: > prefer not to use inline scripts, it violates some forms of CSP Done. Extracted to a new file. https://codereview.chromium.org/2804823002/diff/1/chrome/browser/resources/nt... chrome/browser/resources/ntp4/md_incognito_tab.html:55: document.getElementById('incognitothemecss').href = On 2017/04/11 00:31:12, Dan Beam wrote: > can you use $() instead of document.getElementById? Done, but including util.js for a one-line method feels like an overkill. https://codereview.chromium.org/2804823002/diff/1/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): https://codereview.chromium.org/2804823002/diff/1/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:288: int new_tab_html_idr; On 2017/04/11 00:31:12, Dan Beam wrote: > all these uninitialized ints make me a little nervous Done. Me too, actually :) The current approach seemed cleaner than to assign one set of values, and then conditionally reassign. But we can do it that way too. https://codereview.chromium.org/2804823002/diff/1/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:300: // new_tab_features_ids is not used in the old version. On 2017/04/11 00:31:12, Dan Beam wrote: > new_tab_feature_ids is not initialized explicitly, meaning it could be garbage Indeed, but its only usage is guarded by IsMDIncognitoTabEnabled(). Let me initialize it to 0.
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2804823002/diff/1/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/md_incognito_tab.css (right): https://codereview.chromium.org/2804823002/diff/1/chrome/browser/resources/nt... chrome/browser/resources/ntp4/md_incognito_tab.css:22: font-size: 14px; On 2017/04/11 19:04:26, msramek wrote: > On 2017/04/11 00:31:12, Dan Beam wrote: > > can you make font-sizes in terms of scalable units (em, %, or rem)? > > > > this is so the user's default font size is respected from settings > > Well, this might be a problem. > > The default font size is 13px. 14px/13px has infinite decimal expansion, as do > 24/14, 24/13, 20/14, and 20/13. Let me circle back with our UX designer if we > really want to use 1px larger font than the default. Update in Patchset #4: I talked to our UX designer today, we decided to define our font sizes as an offset, i.e. calc(100% + ?), rather than a ratio of the standard font sizes. Large default font sizes cause the floating bulletpoints not to fit next to each other, so I added a few more lines of JS to work around that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi again Dan! We did another round with maxwalker@, tested screen sizes again, default font sizes, RTL, etc. The visuals should be correct now. I see that you updated the code for the old Incognito page, so during the rebase I also made equivalent changes to the new MD Incognito page code. I guess at this point it makes sense to ignore the previous patchsets and review the whole diff again. PTAL! Thanks, Martin
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2804823002/diff/60001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2804823002/diff/60001/chrome/browser/about_fl... chrome/browser/about_flags.cc:1978: {"enable-md-incognito-ntp", fwiw: the new tab page is already material design... if we really end up needing a feature/flag, maybe --enable-responsive-incognito-ntp maybe? probably doesn't matter, but it might be a little confusing https://codereview.chromium.org/2804823002/diff/60001/chrome/browser/resource... File chrome/browser/resources/ntp4/md_incognito_tab.html (right): https://codereview.chromium.org/2804823002/diff/60001/chrome/browser/resource... chrome/browser/resources/ntp4/md_incognito_tab.html:9: <title i18n-content="title"></title> thanks for converting to $i18n{} as well. seeing this file is what spurred me to change it, yes, for a better guide for you and others in the future as to how to do translations. if you ended up looking at the generated code (or performance) of the page it's a bit better with only $i18n{} stuff going on. https://codereview.chromium.org/2804823002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/ntp4/md_incognito_tab.css (right): https://codereview.chromium.org/2804823002/diff/120001/chrome/browser/resourc... chrome/browser/resources/ntp4/md_incognito_tab.css:167: margin: 4px 0 0 0; nit: the shorthand that makes it easy to see the left and right are the same (and is shorter) is: margin: 4px 0 0; https://codereview.chromium.org/2804823002/diff/120001/chrome/browser/resourc... chrome/browser/resources/ntp4/md_incognito_tab.css:198: padding: 8px 48px 24px 48px; can this be padding: 8px 48px 24px; ? https://codereview.chromium.org/2804823002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/ntp4/md_incognito_tab.js (right): https://codereview.chromium.org/2804823002/diff/120001/chrome/browser/resourc... chrome/browser/resources/ntp4/md_incognito_tab.js:24: var b1 = $('firstBulletpoints'); nit: why not just var bullets = querySelectorAll('.bulletpoints'); bullet[0]/bullet[1] then you can drop the IDs? maybe more brittle? just a nit. https://codereview.chromium.org/2804823002/diff/120001/chrome/browser/resourc... chrome/browser/resources/ntp4/md_incognito_tab.js:37: b2.className += " tooWide"; b2.classList.add('tooWide'); https://codereview.chromium.org/2804823002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): https://codereview.chromium.org/2804823002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:288: if (IsMDIncognitoTabEnabled()) { nit: because only CreateNewTabIncognitoHTML() uses IsMDIncognitoTabEnabled() at the moment, can we just make a local bool instead? const bool is_incognito_enabled = base::FeatureList::IsEnabled(features::kMaterialDesignIncognitoNTP); https://codereview.chromium.org/2804823002/diff/120001/chrome/common/chrome_f... File chrome/common/chrome_features.h (right): https://codereview.chromium.org/2804823002/diff/120001/chrome/common/chrome_f... chrome/common/chrome_features.h:114: extern const base::Feature kMaterialDesignIncognitoNTP; do we even need a flag/feature? can this just ship? it looks good in my local testing. is there much more to change?
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Patchset #8 (id:140001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #8 (id:160001) has been deleted
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, Dan! I addressed your remaining comments and fixed one more bug that I spotted in the CSS. I kept the reference to MD in the flag name though, see my explanation below. https://codereview.chromium.org/2804823002/diff/60001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2804823002/diff/60001/chrome/browser/about_fl... chrome/browser/about_flags.cc:1978: {"enable-md-incognito-ntp", On 2017/04/20 18:27:51, Dan Beam wrote: > fwiw: the new tab page is already material design... > > if we really end up needing a feature/flag, maybe > --enable-responsive-incognito-ntp maybe? > > probably doesn't matter, but it might be a little confusing I know that the native tab UI has been materialized, but the Incognito tab content was previously not conforming to the MD style guide (or so I was told). We don't have a flag for MD NTP in general, so I hope this wouldn't be confusing, and it will nicely fit with the other MD flags. I also want to reuse the flag on Android, where "responsive" might be more confusing, since you don't typically resize the browser window there. And the flag is hopefully shortlived. So I would prefer to keep the name. https://codereview.chromium.org/2804823002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/ntp4/md_incognito_tab.css (right): https://codereview.chromium.org/2804823002/diff/120001/chrome/browser/resourc... chrome/browser/resources/ntp4/md_incognito_tab.css:167: margin: 4px 0 0 0; On 2017/04/20 18:27:51, Dan Beam wrote: > nit: the shorthand that makes it easy to see the left and right are the same > (and is shorter) is: > > margin: 4px 0 0; Done. https://codereview.chromium.org/2804823002/diff/120001/chrome/browser/resourc... chrome/browser/resources/ntp4/md_incognito_tab.css:198: padding: 8px 48px 24px 48px; On 2017/04/20 18:27:51, Dan Beam wrote: > can this be > > padding: 8px 48px 24px; > > ? Done. https://codereview.chromium.org/2804823002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/ntp4/md_incognito_tab.js (right): https://codereview.chromium.org/2804823002/diff/120001/chrome/browser/resourc... chrome/browser/resources/ntp4/md_incognito_tab.js:24: var b1 = $('firstBulletpoints'); On 2017/04/20 18:27:51, Dan Beam wrote: > nit: why not just > > var bullets = querySelectorAll('.bulletpoints'); > bullet[0]/bullet[1] > > then you can drop the IDs? maybe more brittle? just a nit. Done. I'm actually using '.bulletpoints + .bulletpoints' selector in the CSS, so if it's brittle here, then it's brittle there already. https://codereview.chromium.org/2804823002/diff/120001/chrome/browser/resourc... chrome/browser/resources/ntp4/md_incognito_tab.js:37: b2.className += " tooWide"; On 2017/04/20 18:27:51, Dan Beam wrote: > b2.classList.add('tooWide'); Done. (Thanks, my JS-fu seems to have degraded over the years.) https://codereview.chromium.org/2804823002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): https://codereview.chromium.org/2804823002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:288: if (IsMDIncognitoTabEnabled()) { On 2017/04/20 18:27:51, Dan Beam wrote: > nit: because only CreateNewTabIncognitoHTML() uses IsMDIncognitoTabEnabled() at > the moment, can we just make a local bool instead? > > const bool is_incognito_enabled = > base::FeatureList::IsEnabled(features::kMaterialDesignIncognitoNTP); Done. https://codereview.chromium.org/2804823002/diff/120001/chrome/common/chrome_f... File chrome/common/chrome_features.h (right): https://codereview.chromium.org/2804823002/diff/120001/chrome/common/chrome_f... chrome/common/chrome_features.h:114: extern const base::Feature kMaterialDesignIncognitoNTP; On 2017/04/20 18:27:51, Dan Beam wrote: > do we even need a flag/feature? can this just ship? it looks good in my local > testing. is there much more to change? One reason for the flag is that the strings specification has recently changed several times over a short period of time, and in fact, we still haven't started the translation process (which will be different than the standard translation process as the strings are highly visible and prone to misunderstanding). So I don't want to ship it just yet. But I do expect to be able to remove the flag and clean up soon. https://codereview.chromium.org/2804823002/diff/110012/chrome/browser/resourc... File chrome/browser/resources/ntp4/md_incognito_tab.css (right): https://codereview.chromium.org/2804823002/diff/110012/chrome/browser/resourc... chrome/browser/resources/ntp4/md_incognito_tab.css:133: html[dir=rtl] .bulletpoints + .bulletpoints { This was a typo - it was supposed to be the mirror equivalent of the previous item. When I fixed it, it started overriding the "clear:both" on line 159, so I added a mirror version of that rule too. This either coincidentally worked or there was a particular combination of screen size and font size where it broke that I missed before.
msramek@chromium.org changed reviewers: + jwd@chromium.org
+Jesse, can you please have a quick look at histograms.xml? I'm just adding a new flag.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Jesse, friendly ping! It's a tiny change in histograms.xml, and I'd like to get it in before going OOO tomorrow. Thanks!
lgtm
Thanks!
The CQ bit was checked by msramek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2804823002/#ps110012 (title: "Addressed comments.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 110012, "attempt_start_ts": 1493070367376260,
"parent_rev": "5c1cec458f824baa15f38873d845aca2e8515bf5", "commit_rev":
"a8443118aa963ba364ee9687b82d1d9c7eb26af5"}
Message was sent while issue was closed.
Description was changed from ========== Revamp the Incognito NTP on Desktop Changes in this CL: 1. Added an about:flag for the "MD Incognito NTP". (Note that the primary purpose of this change is to improve the text on the Incognito NTP, not just to materialize it; but the latter results in a shorter flag name and thematically fits with the other recent MD improvements). 2. Since almost none of the old CSS is valid for the new MD, and since there are also related HTML changes, we use separate HTML and CSS files. NTPResourceCache decides which one to serve according to the flag. Once this fully launches, we will simply remove the previous implementation. 3. The new design follows the CSS rules described in https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZeKud1YOmPiI. The requirement that the two sets of bulletpoints determine the width of the content box has been achieved by JS (the CSS is complicated enough as it is :-( ). Tested with the bookmark bar on/off, with a custom theme on/off, and in front of a UX designer :) BUG=693525 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Revamp the Incognito NTP on Desktop Changes in this CL: 1. Added an about:flag for the "MD Incognito NTP". (Note that the primary purpose of this change is to improve the text on the Incognito NTP, not just to materialize it; but the latter results in a shorter flag name and thematically fits with the other recent MD improvements). 2. Since almost none of the old CSS is valid for the new MD, and since there are also related HTML changes, we use separate HTML and CSS files. NTPResourceCache decides which one to serve according to the flag. Once this fully launches, we will simply remove the previous implementation. 3. The new design follows the CSS rules described in https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZeKud1YOmPiI. The requirement that the two sets of bulletpoints determine the width of the content box has been achieved by JS (the CSS is complicated enough as it is :-( ). Tested with the bookmark bar on/off, with a custom theme on/off, and in front of a UX designer :) BUG=693525 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2804823002 Cr-Commit-Position: refs/heads/master@{#466817} Committed: https://chromium.googlesource.com/chromium/src/+/a8443118aa963ba364ee9687b82d... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:110012) as https://chromium.googlesource.com/chromium/src/+/a8443118aa963ba364ee9687b82d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
