Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1680743006:
[Media Router] Show user email in header if cloud sink is present. (Closed)
|
Created:
4 years, 10 months ago by amp Modified:
4 years, 10 months ago CC:
chromium-reviews, media-router+watch_chromium.org, arv+watch_chromium.org, jonnymack Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Media Router] Show user email in header if cloud sink is present.
BUG=570797
Committed: https://crrev.com/91d24d370588abe93a3dbbed34ccbe0d6e44d7ea
Cr-Commit-Position: refs/heads/master@{#377582}
Patch Set 1 #Patch Set 2 : Check sinks before including email, update styling to change height dynamically. #Patch Set 3 : rebase #Patch Set 4 : Updated to be more dynamic on sink list since relevant sinks may not be available when the UI loads. #Patch Set 5 : More adjustments to get dynamic height change working and updated domain handling. #
Total comments: 28
Patch Set 6 : Address comments #Patch Set 7 : Overhauled implementation to use observers and fire events as well as push down logic into native m… #
Total comments: 24
Patch Set 8 : Rebase, message_handler.cc still in broken state #Patch Set 9 : Fix breaks in native code and address comments. Everything works with no bugs detected in this pat… #
Total comments: 24
Patch Set 10 : Address comments #
Total comments: 5
Patch Set 11 : Address comments. Also fix display of long sink names hiding domain name and addressed bug with ac… #Patch Set 12 : rebase #Patch Set 13 : Added tests (still not passing for header) #Patch Set 14 : Fixing js tests. Native tests still in progress... #
Total comments: 16
Patch Set 15 : All tests updated and passing, addressed comments. #Patch Set 16 : Fix ellipsis direction for LTR #
Total comments: 20
Patch Set 17 : Address final comments #
Total comments: 2
Messages
Total messages: 38 (9 generated)
amp@chromium.org changed reviewers: + apacible@chromium.org, imcheng@chromium.org
There may be better approaches to accomplish this, but the latest patch works (modulo a bug on sometimes showing the domain when it shouldn't be). I'm still updating tests.
Haven't taken a look yet, but can you add some screenshots? Thanks!
On 2016/02/10 21:47:43, apacible wrote: > Haven't taken a look yet, but can you add some screenshots? Thanks! Oops, I knew there was something I was forgetting to do. One hangout sink that causes the email to display, but no domain shows because it is the same as the user domain: RTL https://screenshot.googleplex.com/esQt1Raz5AS LTR https://screenshot.googleplex.com/JdKZy5P5uQY Added another hangout with a different domain to show that both domains will then show up: RTL https://screenshot.googleplex.com/TtZWfhLYs4j LTR https://screenshot.googleplex.com/ejMkhff2G1p Cast mode picker to show that email still shows up there with appropriate spacing: LTR https://screenshot.googleplex.com/867xOHqE6Ai RTL https://screenshot.googleplex.com/MGRwjR34ySu
It may be easier to update tests after we finalize what goes into the container versus header; we should move as much header-specific functionality to the header element. Could you update the description with expected behavior of the different scenarios? See log in/out related comment. https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html (right): https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:55: email-text="[[computeEmailText_(showEmail, userEmail)]]" userEmail isn't defined in media_router_container.js. Since it's only used in the header, set it directly as a property in media-router-header, and have media-router-container update whether or not to show the email (showEmail). https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:199: headerWithEmailHeight_: { Since the height there would be dependent on whether we show the email text, could we move this property and maybeChangeHeaderHeight_() to media-router-header? https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:201: value: 62, Add: readOnly: true, https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:329: * Whether to show the sink domain in the list.. nit: one period at the end. https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:461: this.$$('#container-header').$$('#header-toolbar').style.height = What happens when the height is undefined? https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:595: var text = ''; return showEmail ? userEmail : ''; https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:784: var domainText = domain; return domainText == 'default' ? this.userDomain : domain; https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1276: this.maybeChangeHeaderHeight_(); We only want to call this if there was a change to |showEmail|. https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_header/media_router_header.css (right): https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.css:38: #user-email-container { nit: alphabetize by selectors https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_header/media_router_header.html (right): https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.html:30: <div id="user-email-container" class="bottom fit" Wrap this in a conditional template since we may never show this. It can be conditional on whether we should show the email (showEmail from media-router-container). https://www.polymer-project.org/1.0/docs/devguide/templates.html#dom-if https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:396: initial_data.SetString("userEmail", What would the behavior be like for scenarios where the user logs in/out while the dialog is open? For example: 1. User logs in. User opens dialog. User logs out. 2. User is not logged in. User opens dialog. User logs in. 3. (similar to 2) User logs in. User opens dialog. User logs out. User logs into a second email.
https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1131: hasExternalDomain = sink.domain != 'default' && If both hasSinkWithDomain and hasExternalDomain are true, break out of the loop early. Otherwise, hasExternalDomain could be reset back to false.
Replying to comments, but still working on moving parts into the header instead of the main container. I expect things will work smoother once that is done, but I may need some help getting there. Also I'll expect to have to rebase this against the changes in cr/1678293003 so the webui_message_handler.cc will have to be updated. https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html (right): https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:55: email-text="[[computeEmailText_(showEmail, userEmail)]]" On 2016/02/11 17:09:14, apacible wrote: > userEmail isn't defined in media_router_container.js. > > Since it's only used in the header, set it directly as a property in > media-router-header, and have media-router-container update whether or not to > show the email (showEmail). I'll try to do this, but I will need a little more context on polymer (I've just been looking up small parts so far and don't have a good grasp of how everything works together). https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:199: headerWithEmailHeight_: { On 2016/02/11 17:09:14, apacible wrote: > Since the height there would be dependent on whether we show the email text, > could we move this property and maybeChangeHeaderHeight_() to > media-router-header? I'll attempt this as well, but it felt easier to keep the logic in here since the list and mode views are also dependent on the height of the header. I guess we can just send an event from the header up to the container (but I'll need to look up how to do that). https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:201: value: 62, On 2016/02/11 17:09:14, apacible wrote: > Add: readOnly: true, Done. https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:329: * Whether to show the sink domain in the list.. On 2016/02/11 17:09:14, apacible wrote: > nit: one period at the end. Done. https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:461: this.$$('#container-header').$$('#header-toolbar').style.height = On 2016/02/11 17:09:14, apacible wrote: > What happens when the height is undefined? It starts out undefined so if it is set back to undefined it should revert to the styling it was before. You can also set it to '' for the same effect. https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:595: var text = ''; On 2016/02/11 17:09:14, apacible wrote: > return showEmail ? userEmail : ''; Done. https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:784: var domainText = domain; On 2016/02/11 17:09:14, apacible wrote: > return domainText == 'default' ? this.userDomain : domain; Done. https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1131: hasExternalDomain = sink.domain != 'default' && On 2016/02/11 17:14:12, apacible wrote: > If both hasSinkWithDomain and hasExternalDomain are true, break out of the loop > early. Otherwise, hasExternalDomain could be reset back to false. Nice catch, I can't break out of the loop here though, because the sinkMap is still being rebuilt, but I'll make sure to only set showSinkDomain once. Even with this change though, I still end up with this.showSinkDomain set to true when it shouldn't be and I can't figure out why... https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1276: this.maybeChangeHeaderHeight_(); On 2016/02/11 17:09:14, apacible wrote: > We only want to call this if there was a change to |showEmail|. Yea, there is probably a better way to do this with observers or something, but I was just trying different things out and haven't dug into how to make it work best with the available platform libraries etc. A quick overview of what is possible/recommended in this regard would be useful. https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_header/media_router_header.css (right): https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.css:38: #user-email-container { On 2016/02/11 17:09:14, apacible wrote: > nit: alphabetize by selectors Done. https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_header/media_router_header.html (right): https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.html:30: <div id="user-email-container" class="bottom fit" On 2016/02/11 17:09:14, apacible wrote: > Wrap this in a conditional template since we may never show this. > > It can be conditional on whether we should show the email (showEmail from > media-router-container). > > https://www.polymer-project.org/1.0/docs/devguide/templates.html#dom-if Working on this. I originally had it in a template, but it didn't start working until I removed the template and used hidden on the div. It probably had something to do with using the text itself as the 'show' variable though. https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:396: initial_data.SetString("userEmail", On 2016/02/11 17:09:14, apacible wrote: > What would the behavior be like for scenarios where the user logs in/out while > the dialog is open? > > For example: > 1. User logs in. User opens dialog. User logs out. If the dialogue is still open and still shows sinks with domains, then it should still show the emails associated with those sinks. If the cloud mrp updates the list and clears them out (perhaps we should add a bug to track doing that) then the email should be removed as there are no more sinks associated with it. > 2. User is not logged in. User opens dialog. User logs in. Probably nothing as I don't think the cloud mrp will run discover until the next time the dialog is opened. > 3. (similar to 2) User logs in. User opens dialog. User logs out. User logs into > a second email. That one gets tricky, but the cloud mrp should track when login changes and clear it's list (removing the email). If the user logs back in I'm still thinking nothing will show up (same as #2) until the next run of the dialog, which should give us a chance to update the email address to the new one (I think).
I'll take a look at everything again after the container/header changes. Let me know if you have any other questions getting that working! https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:461: this.$$('#container-header').$$('#header-toolbar').style.height = On 2016/02/11 21:21:24, amp wrote: > On 2016/02/11 17:09:14, apacible wrote: > > What happens when the height is undefined? > > It starts out undefined so if it is set back to undefined it should revert to > the styling it was before. You can also set it to '' for the same effect. Acknowledged. https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:396: initial_data.SetString("userEmail", > > 2. User is not logged in. User opens dialog. User logs in. > Probably nothing as I don't think the cloud mrp will run discover until the next > time the dialog is opened. Would the cloud mrp run discover when we do a sink query in general? I'm not familiar with the expected behavior here. I know MR can/will send updated sink lists while the dialog is open. I'd also expect that with the cloud sinks. If we don't do that, is that for performance reasons?
Code is updated for the new implementation mostly and is ready to start looking at, but I haven't verified everything works yet (wanted to get something out now rather than forget where I was over the long weekend). The message handler code in particular doesn't compile yet as I was just guessing what some of the method calls were (I was wrong) and I'm sure I'm not doing things correctly anyway. So I could use some help on that part when we get back. https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:396: initial_data.SetString("userEmail", On 2016/02/12 21:26:15, apacible wrote: > > > 2. User is not logged in. User opens dialog. User logs in. > > Probably nothing as I don't think the cloud mrp will run discover until the > next > > time the dialog is opened. > > Would the cloud mrp run discover when we do a sink query in general? I'm not > familiar with the expected behavior here. > > I know MR can/will send updated sink lists while the dialog is open. I'd also > expect that with the cloud sinks. If we don't do that, is that for performance > reasons? The CloudMRP will get the same queries, but it ignores them unless they are for tab or desktop casting. It also may not be responding to all the apis for observing things. The cloud sinks do generally show up after the UI has been open for a few seconds (the first time, subsequent times they are usually there from the beginning), so it is dynamic in that sense. I'm not sure I entirely understand how it all works together either though. I'll try to verify the behavior here with logging in and out etc and see what is actually going to happen.
I started looking through this before I left the office, so publishing comments now while thoughts are still fresh. You may also want to rebase, especially with my latest changes with the message handler. https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1680743006/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:396: initial_data.SetString("userEmail", On 2016/02/13 02:11:05, amp wrote: > On 2016/02/12 21:26:15, apacible wrote: > > > > 2. User is not logged in. User opens dialog. User logs in. > > > Probably nothing as I don't think the cloud mrp will run discover until the > > next > > > time the dialog is opened. > > > > Would the cloud mrp run discover when we do a sink query in general? I'm not > > familiar with the expected behavior here. > > > > I know MR can/will send updated sink lists while the dialog is open. I'd also > > expect that with the cloud sinks. If we don't do that, is that for performance > > reasons? > > The CloudMRP will get the same queries, but it ignores them unless they are for > tab or desktop casting. It also may not be responding to all the apis for > observing things. > > The cloud sinks do generally show up after the UI has been open for a few > seconds (the first time, subsequent times they are usually there from the > beginning), so it is dynamic in that sense. I'm not sure I entirely understand > how it all works together either though. > > I'll try to verify the behavior here with logging in and out etc and see what is > actually going to happen. Acknowledged. https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_header/media_router_header.css (right): https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.css:49: #user-email-container { nit: alphabetize by selectors. https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_header/media_router_header.html (right): https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.html:32: <span id="email-text" title="[[userEmail]]">[[userEmail]]</span> Declare |userEmail| as a property of media-router-header. https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.html:32: <span id="email-text" title="[[userEmail]]">[[userEmail]]</span> What is the id used for? https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js (right): https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js:42: headerWithEmailHeight_: { nit: here and below, alphabetize properties. https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js:148: maybeChangeHeaderHeight_: function() { The function can return early if |showEmail| was never changed. maybeChangeHeaderHeight_: function(newValue, oldValue) { if (newValue == oldValue) return; ... } https://www.polymer-project.org/1.0/docs/devguide/properties.html#change-call... https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js:151: // can we get the offset Height like this? Yes. (just checked) https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js:153: if (this.showEmail && this.userEmail) { this.$$('#header-toolbar').style.height = this.showEmail && this.userEmail ? this.headerWithEmailHeight_ + 'px' : undefined; https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js:153: if (this.showEmail && this.userEmail) { Check if |userEmail| is empty/null/whitespace? https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js:163: if (currentHeigt != newHeight) { currentHeight https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router.js (right): https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router.js:35: media_router.ui.setElements(container, header); Inline the header here. We set |container| as a variable because it's used in multiple functions. https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:104: // Don't reset visibliity to 1 if it was already 2 nit: "visibility" misspelled https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:105: if (identityVisibility == 0) Declare identity visibility options as enums here and in the js (see media_router_data.js for examples). This will make it clear what each type of visibility refers to and that they're properly used if/when updated.
PTAL. Code now works end to end without any noticeable issues and feels much cleaner. Still holding off on tests until I'm sure this is the final implementation as there could be some changes with the sign in manager and other details. cc'ing Jonny as there is one UI thing I noticed when the meeting name overflows the sink name width (the domain doesn't get displayed in that case). See https://screenshot.googleplex.com/FvzMietEt6x which should have a domain (of google.com) for the second sink, but the name overflow overrides the domain apparently. Should we move the overflow of the name back and to let the domain remain visible, or are there other UI options available here? https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_header/media_router_header.css (right): https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.css:49: #user-email-container { On 2016/02/13 08:13:02, apacible wrote: > nit: alphabetize by selectors. Done. I thought this included the '#' in the alphabetization, but this file was tricky in that it worked either way until my addition here. :) https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_header/media_router_header.html (right): https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.html:32: <span id="email-text" title="[[userEmail]]">[[userEmail]]</span> On 2016/02/13 08:13:02, apacible wrote: > What is the id used for? It was an old carry over from when I was trying to get things showing up at this level. It's not needed, so I removed it. https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.html:32: <span id="email-text" title="[[userEmail]]">[[userEmail]]</span> On 2016/02/13 08:13:02, apacible wrote: > Declare |userEmail| as a property of media-router-header. Done. https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js (right): https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js:42: headerWithEmailHeight_: { On 2016/02/13 08:13:02, apacible wrote: > nit: here and below, alphabetize properties. Done. https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js:45: value: 62, Is there anything we should do here to reference the css somehow? I get the feeling that we will update something in the css and this value will become invalid and won't get noticed or be hard to find. https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js:148: maybeChangeHeaderHeight_: function() { On 2016/02/13 08:13:02, apacible wrote: > The function can return early if |showEmail| was never changed. > > maybeChangeHeaderHeight_: function(newValue, oldValue) { > if (newValue == oldValue) > return; > ... > } > > https://www.polymer-project.org/1.0/docs/devguide/properties.html#change-call... Done. https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js:151: // can we get the offset Height like this? On 2016/02/13 08:13:02, apacible wrote: > Yes. (just checked) Acknowledged. https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js:153: if (this.showEmail && this.userEmail) { On 2016/02/13 08:13:02, apacible wrote: > Check if |userEmail| is empty/null/whitespace? Done. I didn't want to redefine the method in the header as well as the container, so I call back up into the container, but it looks kind of ugly. Let me know if you would prefer something else in that regard. https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js:163: if (currentHeigt != newHeight) { On 2016/02/13 08:13:02, apacible wrote: > currentHeight Done. https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router.js (right): https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router.js:35: media_router.ui.setElements(container, header); On 2016/02/13 08:13:02, apacible wrote: > Inline the header here. We set |container| as a variable because it's used in > multiple functions. Done. https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:104: // Don't reset visibliity to 1 if it was already 2 On 2016/02/13 08:13:02, apacible wrote: > nit: "visibility" misspelled Done. https://codereview.chromium.org/1680743006/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:105: if (identityVisibility == 0) On 2016/02/13 08:13:02, apacible wrote: > Declare identity visibility options as enums here and in the js (see > media_router_data.js for examples). This will make it clear what each type of > visibility refers to and that they're properly used if/when updated. Done. I declared a local enum to this class but decided that it was easier to just set the boolean's directly instead of passing across the enum. The end result is the js side doesn't need to know about it.
https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:735: return !this.showDomain || this.isEmptyOrWhitespace_(sink.domain); showDomain should be defined above in the properties object. https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_header/media_router_header.html (right): https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.html:32: <span title="[[userEmail]]">[[userEmail]]</span> is title needed? https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js (right): https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js:45: value: 62, nit: seems like you can just make this a String '62px'. https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:84: signin_manager->GetAuthenticatedAccountInfo().hosted_domain; nit: store the result of signin_manager->GetAuthenticatedAccountInfo() in a local variable to avoid looking up twice. https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:84: signin_manager->GetAuthenticatedAccountInfo().hosted_domain; the documentation says GetAuthenticatedAccountInfo() returns an empty struct if user is not signed in. Is that case being handled? https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:103: #if defined(GOOGLE_CHROME_BUILD) Personally I think the logic to determine whether to show email and domain should be in JS. But I understand the need to guard this with GOOGLE_CHROME_BUILD. Though I wonder if there's a clean way for the UI to implement this logic without having to use GOOGLE_CHROME_BUILD. What you have here is fine though. https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:114: MediaRouterWebUIMessageHandler::IdentityVisibility::HIDDEN) nit: need some braces for these if statements. https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:393: media_router_ui_->sinks(), profile)); nit: inline profile. https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:465: Profile::FromWebUI(web_ui())->GetPrefs()->SetBoolean( nit: extract PrefService* into a local variable. https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h (right): https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h:35: enum IdentityVisibility { I think this should be removed since you are not passing them into the ui anymore. These values are not mutually exclusive anyway, so I suggest just sticking with 2 booleans.
https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:735: return !this.showDomain || this.isEmptyOrWhitespace_(sink.domain); On 2016/02/17 00:51:08, imcheng1 wrote: > showDomain should be defined above in the properties object. Done. https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_header/media_router_header.html (right): https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.html:32: <span title="[[userEmail]]">[[userEmail]]</span> On 2016/02/17 00:51:08, imcheng1 wrote: > is title needed? Not sure. I assumed it was for accessibility as we also use a title on the header text (line 16). But as the text is the same here perhaps it doesn't make a difference? https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js (right): https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js:45: value: 62, On 2016/02/17 00:51:08, imcheng1 wrote: > nit: seems like you can just make this a String '62px'. It would work as a string, but if we ever need to calculate anything off this value (which doesn't sound too far fetched) a number makes more sense. Also we have a different Height variable in the container.js that is also a number, so there is parity with the style there. I can change it if you feel strongly about it. https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:84: signin_manager->GetAuthenticatedAccountInfo().hosted_domain; On 2016/02/17 00:51:08, imcheng1 wrote: > the documentation says GetAuthenticatedAccountInfo() returns an empty struct if > user is not signed in. Is that case being handled? Done. It should now, I'll make sure to have tests that cover this case (and I'll try to verify what happens on my local builds). https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:84: signin_manager->GetAuthenticatedAccountInfo().hosted_domain; On 2016/02/17 00:51:08, imcheng1 wrote: > nit: store the result of signin_manager->GetAuthenticatedAccountInfo() in a > local variable to avoid looking up twice. Done. https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:103: #if defined(GOOGLE_CHROME_BUILD) On 2016/02/17 00:51:08, imcheng1 wrote: > Personally I think the logic to determine whether to show email and domain > should be in JS. But I understand the need to guard this with > GOOGLE_CHROME_BUILD. Though I wonder if there's a clean way for the UI to > implement this logic without having to use GOOGLE_CHROME_BUILD. What you have > here is fine though. With the change to the check on the account info struct above I can just have this check that |user_domain| is not empty and remove the chrome build checks. https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:114: MediaRouterWebUIMessageHandler::IdentityVisibility::HIDDEN) On 2016/02/17 00:51:08, imcheng1 wrote: > nit: need some braces for these if statements. With the removal of the visibility enum's these become simple boolean assignments. I'm not sure exactly what the style guidelines are, but that seems to be allowed in other places in this file. https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:393: media_router_ui_->sinks(), profile)); On 2016/02/17 00:51:08, imcheng1 wrote: > nit: inline profile. Done. https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:465: Profile::FromWebUI(web_ui())->GetPrefs()->SetBoolean( On 2016/02/17 00:51:08, imcheng1 wrote: > nit: extract PrefService* into a local variable. I'll get to this if I can. Did these lines show up as being changed by me? https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h (right): https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h:35: enum IdentityVisibility { On 2016/02/17 00:51:08, imcheng1 wrote: > I think this should be removed since you are not passing them into the ui > anymore. These values are not mutually exclusive anyway, so I suggest just > sticking with 2 booleans. Done.
https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:114: MediaRouterWebUIMessageHandler::IdentityVisibility::HIDDEN) On 2016/02/17 02:11:37, amp wrote: > On 2016/02/17 00:51:08, imcheng1 wrote: > > nit: need some braces for these if statements. > > With the removal of the visibility enum's these become simple boolean > assignments. I'm not sure exactly what the style guidelines are, but that seems > to be allowed in other places in this file. I find adding braces help readability for single statements that span more than 1 line. https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:465: Profile::FromWebUI(web_ui())->GetPrefs()->SetBoolean( On 2016/02/17 02:11:37, amp wrote: > On 2016/02/17 00:51:08, imcheng1 wrote: > > nit: extract PrefService* into a local variable. > > I'll get to this if I can. Did these lines show up as being changed by me? No these lines are not affected by your change. Just thought it is a small change that can be included since you are modifying this file anyway :) https://codereview.chromium.org/1680743006/diff/180001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js (right): https://codereview.chromium.org/1680743006/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js:171: !$('media-router-container').isEmptyOrWhitespace_(this.userEmail) ? I am all for reusing code, but it seems a bit odd to me that we have to call into the container to use it. WDYT about defining isEmptyOrWhitespace_ here and using it, or even better extracting it to a common place (e.g., media_router.js)? https://codereview.chromium.org/1680743006/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1680743006/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:84: AccountInfo accnt_info = signin_manager->GetAuthenticatedAccountInfo(); nit: account_info
Test coming after this last patch and a rebase. Per my previous comment about long sink names covering up the domain I updated the style to overflow the sink name and let the domain name remain visible. Screenshots of long sink names No domain name LTR: https://screenshot.googleplex.com/uvQOtR1fiEg No domain name RTL: https://screenshot.googleplex.com/8sTAZOzg1q9 With domain name LRT: https://screenshot.googleplex.com/7QAf5MVTGWQ With domain name RTL: https://screenshot.googleplex.com/b48cuxNw7ps https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:114: MediaRouterWebUIMessageHandler::IdentityVisibility::HIDDEN) On 2016/02/18 18:58:54, imcheng1 wrote: > On 2016/02/17 02:11:37, amp wrote: > > On 2016/02/17 00:51:08, imcheng1 wrote: > > > nit: need some braces for these if statements. > > > > With the removal of the visibility enum's these become simple boolean > > assignments. I'm not sure exactly what the style guidelines are, but that > seems > > to be allowed in other places in this file. > > I find adding braces help readability for single statements that span more than > 1 line. Acknowledged. https://codereview.chromium.org/1680743006/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:465: Profile::FromWebUI(web_ui())->GetPrefs()->SetBoolean( On 2016/02/18 18:58:54, imcheng1 wrote: > On 2016/02/17 02:11:37, amp wrote: > > On 2016/02/17 00:51:08, imcheng1 wrote: > > > nit: extract PrefService* into a local variable. > > > > I'll get to this if I can. Did these lines show up as being changed by me? > No these lines are not affected by your change. Just thought it is a small > change that can be included since you are modifying this file anyway :) Done. https://codereview.chromium.org/1680743006/diff/180001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js (right): https://codereview.chromium.org/1680743006/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js:171: !$('media-router-container').isEmptyOrWhitespace_(this.userEmail) ? On 2016/02/18 18:58:54, imcheng1 wrote: > I am all for reusing code, but it seems a bit odd to me that we have to call > into the container to use it. WDYT about defining isEmptyOrWhitespace_ here and > using it, or even better extracting it to a common place (e.g., > media_router.js)? Putting it in media_router.js would also require having a weird lookup (I think, I'm not actually sure how it would be referenced). It's small enough that I just added it to the header here as well. https://codereview.chromium.org/1680743006/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1680743006/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:84: AccountInfo accnt_info = signin_manager->GetAuthenticatedAccountInfo(); On 2016/02/18 18:58:54, imcheng1 wrote: > nit: account_info Done.
Just minor comments. https://codereview.chromium.org/1680743006/diff/180001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js (right): https://codereview.chromium.org/1680743006/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js:171: !$('media-router-container').isEmptyOrWhitespace_(this.userEmail) ? On 2016/02/20 02:00:55, amp wrote: > On 2016/02/18 18:58:54, imcheng1 wrote: > > I am all for reusing code, but it seems a bit odd to me that we have to call > > into the container to use it. WDYT about defining isEmptyOrWhitespace_ here > and > > using it, or even better extracting it to a common place (e.g., > > media_router.js)? > > Putting it in media_router.js would also require having a weird lookup (I think, > I'm not actually sure how it would be referenced). > > It's small enough that I just added it to the header here as well. After extraction you can make it accessible similar to how it's done in media_router_data.js: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... https://codereview.chromium.org/1680743006/diff/260001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css (right): https://codereview.chromium.org/1680743006/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css:155: .sink-name { Should this treatment also be used for very long domains? https://codereview.chromium.org/1680743006/diff/260001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router_ui_interface.js (right): https://codereview.chromium.org/1680743006/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router_ui_interface.js:92: * userEmail: string, Update comments. https://codereview.chromium.org/1680743006/diff/260001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1680743006/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:76: nit: remove newline. https://codereview.chromium.org/1680743006/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:93: scoped_ptr<base::ListValue> sinks_val(new base::ListValue); nit organizational suggestion: move this right below where you create |sink_list_and_identity|. https://codereview.chromium.org/1680743006/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:110: if (sink.domain() == "default") { if (sink.domain() == "default" && user_domain != "NO_HOSTED_DOMAIN") { domain = user_domain; } https://codereview.chromium.org/1680743006/diff/260001/chrome/test/data/webui... File chrome/test/data/webui/media_router/media_router_header_tests.js (right): https://codereview.chromium.org/1680743006/diff/260001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_header_tests.js:33: // Checks whether |element| is hidden. Update comment to mention unstamped elements would also be considered "hidden." https://codereview.chromium.org/1680743006/diff/260001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_header_tests.js:63: header.userEmail = 'user@example.com'; Set |userEmail| into the tests when relevant since we won't have this set in the header all the time. https://codereview.chromium.org/1680743006/diff/260001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_header_tests.js:198: test('visibility of UI depending on email', function(done) { We should also test if the header doesn't show the user-email-container if there was no email set, even if |showEmail| was set to true. This isn't *supposed* to ever happen, but the guard for this is in the native code rather than webui.
PTAL. Everything should be updated and ready for final review. https://codereview.chromium.org/1680743006/diff/260001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css (right): https://codereview.chromium.org/1680743006/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css:155: .sink-name { On 2016/02/23 21:51:00, apacible wrote: > Should this treatment also be used for very long domains? I considered this. As it is now, a long domain name can completely overflow the sink name. I think we want to set it's max width to something like 20% so there will always be at least part of the sink name to compare against, but in practice I expect the vast majority of domain names to be the shorter of the two. WDYT? I tried out some various combinations in the css, but nothing looks really good in all the different scenarios (perhaps easier to discuss offline). https://codereview.chromium.org/1680743006/diff/260001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router_ui_interface.js (right): https://codereview.chromium.org/1680743006/diff/260001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router_ui_interface.js:92: * userEmail: string, On 2016/02/23 21:51:01, apacible wrote: > Update comments. Done. https://codereview.chromium.org/1680743006/diff/260001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1680743006/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:76: On 2016/02/23 21:51:01, apacible wrote: > nit: remove newline. Done. https://codereview.chromium.org/1680743006/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:93: scoped_ptr<base::ListValue> sinks_val(new base::ListValue); On 2016/02/23 21:51:01, apacible wrote: > nit organizational suggestion: move this right below where you create > |sink_list_and_identity|. Done. https://codereview.chromium.org/1680743006/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:110: if (sink.domain() == "default") { On 2016/02/23 21:51:01, apacible wrote: > if (sink.domain() == "default" && > user_domain != "NO_HOSTED_DOMAIN") { > domain = user_domain; > } I don't think that will work. If the user domain is NO_HOSTED_DOMAIN, then the sink domain should be set on line 118 should be an empty string (which is why it's cleared on line 114). https://codereview.chromium.org/1680743006/diff/260001/chrome/test/data/webui... File chrome/test/data/webui/media_router/media_router_header_tests.js (right): https://codereview.chromium.org/1680743006/diff/260001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_header_tests.js:33: // Checks whether |element| is hidden. On 2016/02/23 21:51:01, apacible wrote: > Update comment to mention unstamped elements would also be considered "hidden." Done. https://codereview.chromium.org/1680743006/diff/260001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_header_tests.js:63: header.userEmail = 'user@example.com'; On 2016/02/23 21:51:01, apacible wrote: > Set |userEmail| into the tests when relevant since we won't have this set in the > header all the time. Done. https://codereview.chromium.org/1680743006/diff/260001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_header_tests.js:198: test('visibility of UI depending on email', function(done) { On 2016/02/23 21:51:01, apacible wrote: > We should also test if the header doesn't show the user-email-container if there > was no email set, even if |showEmail| was set to true. This isn't *supposed* to > ever happen, but the guard for this is in the native code rather than webui. Done.
lgtm with nits https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css (right): https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css:157: .sink-name { nit: I'd do something like -- .sink-domain { -webkit-padding-start: 6px; color: var(--subtext-color); } .sink-domain, .sink-subtext { overflow: hidden; text-overflow: ellipsis; } That way any future changes related to the text and how they're handled will be handled at once, and this will deter from styling not staying uniform. (per offline discussion) Also, add a TODO to update this based on discussion with UX? https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js (right): https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js:182: !this.isEmptyOrWhitespace_(this.userEmail) ? nit: Fix indenting here and the next line. Also, does line 182 fit on the previous line? https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js:187: if (currentHeight != newHeight) { nit: Use |this.offsetHeight| here rather than declaring |newHeight|? https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc (right): https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc:7: #include "base/strings/utf_string_conversions.h" Are all of these needed? https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc:53: // info.hosted_domain = "NO_HOSTED_DOMAIN"; nit: Remove comment. https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc:162: std::string user_email("nobody@example.com"); Pull out the actual user email/domain into constants. It'll make the tests a bit easier to follow, e.g. whether we're checking different domains. https://codereview.chromium.org/1680743006/diff/300001/chrome/test/data/webui... File chrome/test/data/webui/media_router/media_router_header_tests.js (right): https://codereview.chromium.org/1680743006/diff/300001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_header_tests.js:204: test('visibility and style of UI depending on email', function(done) { nit: Add comment for test, e.g. "// Tests that the email is shown and header is updated when..." https://codereview.chromium.org/1680743006/diff/300001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_header_tests.js:219: // Verify no email is shown if email is empty nit: Move this comment right above the previous line as an overall comment for the test.
lgtm + comments https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:75: AccountInfo account_info) { const AccountInfo& https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h (right): https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h:93: virtual AccountInfo GetAccountInfo(); add a comment here and it is marked virtual for tests.
Some final screenshots to show the domain name handling in all scenarios (long domain overflow will be addressed with crbug/589697). Long sink name, no domain: RTL - https://screenshot.googleplex.com/k4Yh5Q9MQ5D LTR - https://screenshot.googleplex.com/a4CdLwzGsGW Long sink name, short domain: RTL - https://screenshot.googleplex.com/OHXnzxKD4b5 LTR - https://screenshot.googleplex.com/bao0M7qKuSS Long sink name, long domain: RTL - https://screenshot.googleplex.com/n0q3VxHzGYc LTR - https://screenshot.googleplex.com/taUTZUHCqkL https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css (right): https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.css:157: .sink-name { On 2016/02/25 00:16:02, apacible wrote: > nit: I'd do something like -- > > .sink-domain { > -webkit-padding-start: 6px; > color: var(--subtext-color); > } > > .sink-domain, > .sink-subtext { > overflow: hidden; > text-overflow: ellipsis; > } > > That way any future changes related to the text and how they're handled will be > handled at once, and this will deter from styling not staying uniform. > > (per offline discussion) Also, add a TODO to update this based on discussion > with UX? I ended up not having overflow for the domain (per previous security/privacy discussion about making sure we always show it), but I did add a TODO to address this at some point. In practice I don't think there will be that many long domain names to worry about. I also set the sink-name to have a min-width of 10% so at least something shows up, and set a title on both (in the html) so that a mouseover will display the full content (I'm assuming accessibility works in this case as well). https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js (right): https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js:182: !this.isEmptyOrWhitespace_(this.userEmail) ? On 2016/02/25 00:16:02, apacible wrote: > nit: Fix indenting here and the next line. Also, does line 182 fit on the > previous line? Done. 182 does fit on the line above. showone=Code_formatting&cl=head#Code_formatting https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js:187: if (currentHeight != newHeight) { On 2016/02/25 00:16:02, apacible wrote: > nit: Use |this.offsetHeight| here rather than declaring |newHeight|? Done. https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:75: AccountInfo account_info) { On 2016/02/25 01:52:45, imcheng1 wrote: > const AccountInfo& Done. https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h (right): https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h:93: virtual AccountInfo GetAccountInfo(); On 2016/02/25 01:52:45, imcheng1 wrote: > add a comment here and it is marked virtual for tests. Done. https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc (right): https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc:7: #include "base/strings/utf_string_conversions.h" On 2016/02/25 00:16:02, apacible wrote: > Are all of these needed? Oops, no. This was from a previous patch that required mocking out more than just the account info. I've removed them. https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc:53: // info.hosted_domain = "NO_HOSTED_DOMAIN"; On 2016/02/25 00:16:02, apacible wrote: > nit: Remove comment. Done. https://codereview.chromium.org/1680743006/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc:162: std::string user_email("nobody@example.com"); On 2016/02/25 00:16:02, apacible wrote: > Pull out the actual user email/domain into constants. It'll make the tests a bit > easier to follow, e.g. whether we're checking different domains. Done. https://codereview.chromium.org/1680743006/diff/300001/chrome/test/data/webui... File chrome/test/data/webui/media_router/media_router_header_tests.js (right): https://codereview.chromium.org/1680743006/diff/300001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_header_tests.js:204: test('visibility and style of UI depending on email', function(done) { On 2016/02/25 00:16:02, apacible wrote: > nit: Add comment for test, e.g. "// Tests that the email is shown and header is > updated when..." Done. https://codereview.chromium.org/1680743006/diff/300001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_header_tests.js:219: // Verify no email is shown if email is empty On 2016/02/25 00:16:02, apacible wrote: > nit: Move this comment right above the previous line as an overall comment for > the test. Done.
The CQ bit was checked by amp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from imcheng@chromium.org, apacible@chromium.org Link to the patchset: https://codereview.chromium.org/1680743006/#ps320001 (title: "Address final comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1680743006/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1680743006/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by amp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1680743006/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1680743006/320001
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 amp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1680743006/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1680743006/320001
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Show user email in header if cloud sink is present. BUG=570797 ========== to ========== [Media Router] Show user email in header if cloud sink is present. BUG=570797 Committed: https://crrev.com/91d24d370588abe93a3dbbed34ccbe0d6e44d7ea Cr-Commit-Position: refs/heads/master@{#377582} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/91d24d370588abe93a3dbbed34ccbe0d6e44d7ea Cr-Commit-Position: refs/heads/master@{#377582}
Message was sent while issue was closed.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
Message was sent while issue was closed.
https://build.chromium.org/p/chromium.fyi/builders/Closure%20Compilation%20Li... https://codereview.chromium.org/1680743006/diff/320001/chrome/browser/resourc... File chrome/browser/resources/media_router/media_router_ui_interface.js (right): https://codereview.chromium.org/1680743006/diff/320001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router_ui_interface.js:84: * sinksAndIdentity: {{ {{ -> { https://codereview.chromium.org/1680743006/diff/320001/chrome/browser/resourc... chrome/browser/resources/media_router/media_router_ui_interface.js:90: * }}, }} -> }
Message was sent while issue was closed.
fixing here: https://codereview.chromium.org/1730163005/
Message was sent while issue was closed.
On 2016/02/25 16:34:30, Dan Beam wrote: > fixing here: https://codereview.chromium.org/1730163005/ Thanks for the fix, I'll make sure to check the closure compilation in the future. |
