Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2687643004:
MD Settings: CrOS: Add secondary user banner (Closed)
|
Created:
3 years, 10 months ago by stevenjb Modified:
3 years, 10 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, dbeam+watch-settings_chromium.org, srahim+watch_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: CrOS: Add secondary user banner
BUG=687749
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2687643004
Cr-Commit-Position: refs/heads/master@{#450486}
Committed: https://chromium.googlesource.com/chromium/src/+/27725d345f9b9044e8534f3f0e3bad0e0a0a893b
Patch Set 1 #
Total comments: 8
Patch Set 2 : Rebase + feedback #
Total comments: 5
Patch Set 3 : Add TODO #Patch Set 4 : Rebase #
Messages
Total messages: 28 (10 generated)
Description was changed from ========== MD Settings: CrOS: Add secondary user banner BUG=687749 ========== to ========== MD Settings: CrOS: Add secondary user banner BUG=687749 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
stevenjb@chromium.org changed reviewers: + dschuyler@chromium.org
Screenshot is attached to issue.
https://codereview.chromium.org/2687643004/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/basic_page/basic_page.html (right): https://codereview.chromium.org/2687643004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/basic_page/basic_page.html:94: <template is="dom-if" if="[[showSecondaryUserBanner_]]"> Since this just contains divs, this may be lighter (faster to set up) as a hidden on the div below rather than a template. When a template hides something that outweighs the contents then the template saves set up time, but I think it's likely that the template outweighs the contents in this case. https://codereview.chromium.org/2687643004/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/basic_page/basic_page.js (right): https://codereview.chromium.org/2687643004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/basic_page/basic_page.js:62: // </if> Can this var be removed and computeShowSecondaryUserBanner_ be used from HTML? https://codereview.chromium.org/2687643004/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc (right): https://codereview.chromium.org/2687643004/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:1008: base::ASCIIToUTF16(primary_user_email))); I believe this will be converted back to UTF8 again. Is it reasonable to keep this in utf8 rather than converting back and forth?
https://codereview.chromium.org/2687643004/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/basic_page/basic_page.html (right): https://codereview.chromium.org/2687643004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/basic_page/basic_page.html:94: <template is="dom-if" if="[[showSecondaryUserBanner_]]"> On 2017/02/11 02:54:56, dschuyler wrote: > Since this just contains divs, this may be lighter (faster to set up) as a > hidden on the div below rather than a template. > > When a template hides something that outweighs the contents then the template > saves set up time, but I think it's likely that the template outweighs the > contents in this case. I'm pretty sure it doesn't really matter, especially since there are other dom-ifs here, and I kind of prefer the similarity with the code above, but that's going away, and it probably doesn't matter... so done :) https://codereview.chromium.org/2687643004/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/basic_page/basic_page.js (right): https://codereview.chromium.org/2687643004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/basic_page/basic_page.js:62: // </if> On 2017/02/11 02:54:56, dschuyler wrote: > Can this var be removed and computeShowSecondaryUserBanner_ be used from HTML? This needs to be computed after loadTimeData is available. Added a comment. https://codereview.chromium.org/2687643004/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc (right): https://codereview.chromium.org/2687643004/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:1008: base::ASCIIToUTF16(primary_user_email))); On 2017/02/11 02:54:56, dschuyler wrote: > I believe this will be converted back to UTF8 again. Is it reasonable to keep > this in utf8 rather than converting back and forth? It's not UTF8, this is all UTF16.
https://codereview.chromium.org/2687643004/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc (right): https://codereview.chromium.org/2687643004/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:1008: base::ASCIIToUTF16(primary_user_email))); On 2017/02/13 18:28:00, stevenjb wrote: > On 2017/02/11 02:54:56, dschuyler wrote: > > I believe this will be converted back to UTF8 again. Is it reasonable to keep > > this in utf8 rather than converting back and forth? > > It's not UTF8, this is all UTF16. Maybe I'm missing something(?). It looks like primary_user_email is converted from ascii(utf8) to utf16 here on line 1008; then it's passed so AddString which internally converts back from utf16 to utf8 on line 119 in web_ui_data_source_impl.cc. (The secondary path on line 118 in web_ui_data_source_impl.cc converts from utf16 to utf8 on line 160 within values.cc). I accept that converting strings is fairly fast, but doing it here and elsewhere can add up -- it's a trend I'm trying to turn the other way (into doing fewer conversions). It's not easy to see how many conversions are happening, so I'm trying to highlight that (or if I'm wrong, I'd like to find out where and how).
Does this UI have a mock or UX feedback? (or is it a stop-gap to get something in there and follow-up with UX afterwards)? https://codereview.chromium.org/2687643004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/basic_page/basic_page.html (right): https://codereview.chromium.org/2687643004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/basic_page/basic_page.html:60: background-color: rgb(210, 210, 212); Is there an MD color we can use here (maybe grey 200)?
Sorry, I didn't intend to send notes piecemeal like this. https://codereview.chromium.org/2687643004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/basic_page/basic_page.html (right): https://codereview.chromium.org/2687643004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/basic_page/basic_page.html:100: <if expr="chromeos"> Can lines 99 and 100 be deleted?
On 2017/02/13 21:45:59, dschuyler wrote: > Does this UI have a mock or UX feedback? (or is it a stop-gap to get something > in there and follow-up with UX afterwards)? > > https://codereview.chromium.org/2687643004/diff/20001/chrome/browser/resource... > File chrome/browser/resources/settings/basic_page/basic_page.html (right): > > https://codereview.chromium.org/2687643004/diff/20001/chrome/browser/resource... > chrome/browser/resources/settings/basic_page/basic_page.html:60: > background-color: rgb(210, 210, 212); > Is there an MD color we can use here (maybe grey 200)? See the issue. I am waiting on UX feedback, but not having this is a P1 blocker, whereas making it look prettier will be a P2 "nice to have".
https://codereview.chromium.org/2687643004/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc (right): https://codereview.chromium.org/2687643004/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:1008: base::ASCIIToUTF16(primary_user_email))); On 2017/02/13 21:37:09, dschuyler wrote: > On 2017/02/13 18:28:00, stevenjb wrote: > > On 2017/02/11 02:54:56, dschuyler wrote: > > > I believe this will be converted back to UTF8 again. Is it reasonable to > keep > > > this in utf8 rather than converting back and forth? > > > > It's not UTF8, this is all UTF16. > > Maybe I'm missing something(?). It looks like primary_user_email is converted > from ascii(utf8) to utf16 here on line 1008; then it's passed so AddString which > internally converts back from utf16 to utf8 on line 119 in > web_ui_data_source_impl.cc. (The secondary path on line 118 in > web_ui_data_source_impl.cc converts from utf16 to utf8 on line 160 within > values.cc). > > I accept that converting strings is fairly fast, but doing it here and elsewhere > can add up -- it's a trend I'm trying to turn the other way (into doing fewer > conversions). It's not easy to see how many conversions are happening, so I'm > trying to highlight that (or if I'm wrong, I'd like to find out where and how). While you are not wrong, that seems like something we should address in general and not in this CL? Or do you have a specific suggestion here that would be better that I am missing? Combining the strings in C++ is almost certainly going to be faster than doing so in JS, conversion or no. https://codereview.chromium.org/2687643004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/basic_page/basic_page.html (right): https://codereview.chromium.org/2687643004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/basic_page/basic_page.html:100: <if expr="chromeos"> On 2017/02/13 21:51:50, dschuyler wrote: > Can lines 99 and 100 be deleted? In theory, yes, in practice, the blocks are unrelated (Internet happens to be the first section, but that might change and this is unrelated) so I'd rather make the <if> separate, although I don't feel super strongly.
I agree that the notes on the utf8/16 can be separated and the if expr is opinion (our opinions differ, but the code is ok as-is). LGTM with the rgb color declaration updated (with a var or a TODO). https://codereview.chromium.org/2687643004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/basic_page/basic_page.html (right): https://codereview.chromium.org/2687643004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/basic_page/basic_page.html:60: background-color: rgb(210, 210, 212); On 2017/02/13 21:45:59, dschuyler wrote: > Is there an MD color we can use here (maybe grey 200)? To give more options, any of these would be ok with me: - use a predefined color variable - create a new color var (in settings_vars_css.html) and use it here - add a TODO that this rgb color needs to be replaced with a var (The goal is to define colors in just a few places so that UX updates to colors can be applied easily).
Thanks for the review! https://codereview.chromium.org/2687643004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/basic_page/basic_page.html (right): https://codereview.chromium.org/2687643004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/basic_page/basic_page.html:60: background-color: rgb(210, 210, 212); On 2017/02/13 22:18:47, dschuyler wrote: > On 2017/02/13 21:45:59, dschuyler wrote: > > Is there an MD color we can use here (maybe grey 200)? > > To give more options, any of these would be ok with me: > - use a predefined color variable > - create a new color var (in settings_vars_css.html) and use it here > - add a TODO that this rgb color needs to be replaced with a var > > (The goal is to define colors in just a few places so that UX updates to colors > can be applied easily). TODO added. I wanted to avoid an arbitrary change from the old UI without a design. FWIW, I am a huge fan of using shared colors/variables. However, I am concerned about the current overhead of having a zillion variables. I know we are working to optimize that automatically, but I still suspect we may benefit from doing a usage pass and create smaller shared files for less frequently used values, and move any one-off values to where they are used.
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org Link to the patchset: https://codereview.chromium.org/2687643004/#ps40001 (title: "Add TODO")
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 dbeam@chromium.org
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
not lgtm i don't think Alan wants a separate card for this, please sync with him first then update this CL
On 2017/02/14 01:30:55, Dan Beam wrote: > not lgtm > > i don't think Alan wants a separate card for this, please sync with him first > then update this CL Alan gave a thumbs up for this as-is as better than not having it. We will then re-prioritize the issue as a P2 would-like-to-have with the new design. (This is not what Alan thought it was; this ChrOS multi-profile only message refers to a handful of things like Internet settings and language).
On 2017/02/14 01:59:32, stevenjb wrote: > On 2017/02/14 01:30:55, Dan Beam wrote: > > not lgtm > > > > i don't think Alan wants a separate card for this, please sync with him first > > then update this CL > > Alan gave a thumbs up for this as-is as better than not having it. We will then > re-prioritize the issue as a P2 would-like-to-have with the new design. > > (This is not what Alan thought it was; this ChrOS multi-profile only message > refers to a handful of things like Internet settings and language). did he give you a thumbs up before our UX sync at 3-3:30pm today? because his mind was different then
I spoke to him after you made your comment, which I'm pretty sure was after your UX sync? Again, I don't think the purpose of the banner was well understood at the time of your conversation. I showed him the existing one and explained why we need it and he agreed that having something was better than nothing and that he would consider a better design. Feel free to sync up with him again though, On Mon, Feb 13, 2017 at 7:53 PM, <dbeam@chromium.org> wrote: > On 2017/02/14 01:59:32, stevenjb wrote: > > On 2017/02/14 01:30:55, Dan Beam wrote: > > > not lgtm > > > > > > i don't think Alan wants a separate card for this, please sync with him > first > > > then update this CL > > > > Alan gave a thumbs up for this as-is as better than not having it. We > will > then > > re-prioritize the issue as a P2 would-like-to-have with the new design. > > > > (This is not what Alan thought it was; this ChrOS multi-profile only > message > > refers to a handful of things like Internet settings and language). > > did he give you a thumbs up before our UX sync at 3-3:30pm today? because > his > mind was different then > > https://codereview.chromium.org/2687643004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/02/14 17:43:45, stevenjb wrote: > I spoke to him after you made your comment, which I'm pretty sure was after > your UX sync? > > Again, I don't think the purpose of the banner was well understood at the > time of your conversation. I showed him the existing one and explained why > we need it and he agreed that having something was better than nothing and > that he would consider a better design. > > Feel free to sync up with him again though, > > > On Mon, Feb 13, 2017 at 7:53 PM, <mailto:dbeam@chromium.org> wrote: > > > On 2017/02/14 01:59:32, stevenjb wrote: > > > On 2017/02/14 01:30:55, Dan Beam wrote: > > > > not lgtm > > > > > > > > i don't think Alan wants a separate card for this, please sync with him > > first > > > > then update this CL > > > > > > Alan gave a thumbs up for this as-is as better than not having it. We > > will > > then > > > re-prioritize the issue as a P2 would-like-to-have with the new design. > > > > > > (This is not what Alan thought it was; this ChrOS multi-profile only > > message > > > refers to a handful of things like Internet settings and language). > > > > did he give you a thumbs up before our UX sync at 3-3:30pm today? because > > his > > mind was different then > > > > https://codereview.chromium.org/2687643004/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. ok, clarified with Alan, thanks for following up lgtm
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2687643004/#ps60001 (title: "Rebase")
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": 60001, "attempt_start_ts": 1487104952838150,
"parent_rev": "5c328211dc652bed2edc3fb24858079b0e632ed5", "commit_rev":
"27725d345f9b9044e8534f3f0e3bad0e0a0a893b"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: CrOS: Add secondary user banner BUG=687749 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: CrOS: Add secondary user banner BUG=687749 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2687643004 Cr-Commit-Position: refs/heads/master@{#450486} Committed: https://chromium.googlesource.com/chromium/src/+/27725d345f9b9044e8534f3f0e3b... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/27725d345f9b9044e8534f3f0e3b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
