Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2129253002:
Multi-pod support for lock screen. (Closed)
|
Created:
4 years, 5 months ago by sammiequon Modified:
4 years, 4 months ago CC:
chromium-reviews, dzhioev+watch_chromium.org, achuith+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMulti-pod support for lock screen.
BUG=616536
Committed: https://crrev.com/a8e4ad8678760fe8d08c24ad1788347990042b34
Cr-Commit-Position: refs/heads/master@{#408734}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Rebased. #Patch Set 3 : FIxed patch set 1 errors. #
Total comments: 13
Patch Set 4 : Fixed patch set 3 errors. #
Total comments: 1
Patch Set 5 : Fixed patch set 4 errors. #
Total comments: 6
Patch Set 6 : Fixed patch set 5 errors. #
Total comments: 17
Patch Set 7 : Fixed patch set 6 errors. #
Total comments: 2
Patch Set 8 : Fixed patch set 7 errors. #
Total comments: 2
Patch Set 9 : Fixed patch set 8 errors. #
Total comments: 2
Patch Set 10 : Fixed patch set 9 errors. #
Total comments: 2
Patch Set 11 : Removed log messages. #Patch Set 12 : Rebased. #
Messages
Total messages: 37 (9 generated)
Description was changed from ========== Multi-pod support for lock screen. BUG=616536 ========== to ========== Multi-pod support for lock screen. BUG=616536 ==========
sammiequon@chromium.org changed reviewers: + jdufault@chromium.org
jdufault@ - Please take a look. https://codereview.chromium.org/2129253002/diff/1/ui/login/account_picker/use... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2129253002/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:2951: Remove this line.
jdufault@ - Please take a look.
https://codereview.chromium.org/2129253002/diff/1/ui/login/account_picker/use... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2129253002/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.css:48: transition: all 180ms ease-in-out; Why's this change needed? https://codereview.chromium.org/2129253002/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.css:200: visibility 200ms ease-in-out 180ms; Why 200ms duration instead of 180ms? https://codereview.chromium.org/2129253002/diff/1/ui/login/account_picker/use... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2129253002/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:706: pinKeyboardShown_: false, Make this a getter and do a DOM query based on if the pin-enabled or pin-disabled class is active. https://codereview.chromium.org/2129253002/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:1142: var elements = [this, this.authElement, this.imageElement, This list is getting pretty long now and a number of selectors have been added specifically for it. What about adding a new class, pin-animatable, and building this list by querying for all DOM elements that have it defined? https://codereview.chromium.org/2129253002/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:2577: Oobe.getInstance().displayType == DISPLAY_TYPE.LOCK) Why the Oobe.getInstance().displayType check? The showPin check should always be false if that's not true. https://codereview.chromium.org/2129253002/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:3054: pinPadding = PIN_EXTRA_WIDTH/2; space between / operands. https://codereview.chromium.org/2129253002/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:3173: this.placePods_(); Was this missing before?
https://codereview.chromium.org/2129253002/diff/1/ui/login/account_picker/use... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2129253002/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.css:48: transition: all 180ms ease-in-out; On 2016/07/12 00:22:25, jdufault wrote: > Why's this change needed? Done. https://codereview.chromium.org/2129253002/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.css:200: visibility 200ms ease-in-out 180ms; On 2016/07/12 00:22:25, jdufault wrote: > Why 200ms duration instead of 180ms? Done. https://codereview.chromium.org/2129253002/diff/1/ui/login/account_picker/use... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2129253002/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:706: pinKeyboardShown_: false, On 2016/07/12 00:22:25, jdufault wrote: > Make this a getter and do a DOM query based on if the pin-enabled or > pin-disabled class is active. Done. https://codereview.chromium.org/2129253002/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:1142: var elements = [this, this.authElement, this.imageElement, On 2016/07/12 00:22:25, jdufault wrote: > This list is getting pretty long now and a number of selectors have been added > specifically for it. What about adding a new class, pin-animatable, and building > this list by querying for all DOM elements that have it defined? Done. https://codereview.chromium.org/2129253002/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:2577: Oobe.getInstance().displayType == DISPLAY_TYPE.LOCK) On 2016/07/12 00:22:25, jdufault wrote: > Why the Oobe.getInstance().displayType check? > > The showPin check should always be false if that's not true. Done. https://codereview.chromium.org/2129253002/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:2951: On 2016/07/08 18:04:32, sammiequon wrote: > Remove this line. Done. https://codereview.chromium.org/2129253002/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:3054: pinPadding = PIN_EXTRA_WIDTH/2; On 2016/07/12 00:22:25, jdufault wrote: > space between / operands. Done. https://codereview.chromium.org/2129253002/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:3173: this.placePods_(); On 2016/07/12 00:22:25, jdufault wrote: > Was this missing before? I added this because the pods shift when the pin keyboard is brought up now.
https://codereview.chromium.org/2129253002/diff/40001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2129253002/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1111: var elements = this.querySelectorAll('.pin-modifiable'); What about var elements = Array(this.querySelectorAll('.pin-modifiable')) I think a DOMList also exposes a 'length' property and supports indexing, so you could iterate the return value directly. The only issue is the elements_array.push(this) bit. Does getElementsByClassName work? What about 'pin-status-tag' or 'pin-tag' instead of 'pin-modifiable'? https://codereview.chromium.org/2129253002/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:2556: if (this.pods[i].classList.contains('pin-enabled')) This class check is happening in a couple of places, right? I think it'd make sense to add a helper method that does it. https://codereview.chromium.org/2129253002/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:3015: pod.offsetHeight != CROS_PIN_POD_HEIGHT) { Can we just set height/width to the expected values instead of forking the logic? https://codereview.chromium.org/2129253002/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:3030: pinPadding *= -1; We want negative padding?
https://codereview.chromium.org/2129253002/diff/40001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2129253002/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1111: var elements = this.querySelectorAll('.pin-modifiable'); On 2016/07/12 19:51:26, jdufault wrote: > What about > > var elements = Array(this.querySelectorAll('.pin-modifiable')) > > I think a DOMList also exposes a 'length' property and supports indexing, so you > could iterate the return value directly. The only issue is the > elements_array.push(this) bit. > > Does getElementsByClassName work? > > What about 'pin-status-tag' or 'pin-tag' instead of 'pin-modifiable'? Array() seems to make an array of arrays where the first element is the array and the rest are undefined. Done. Done. https://codereview.chromium.org/2129253002/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:2556: if (this.pods[i].classList.contains('pin-enabled')) On 2016/07/12 19:51:26, jdufault wrote: > This class check is happening in a couple of places, right? I think it'd make > sense to add a helper method that does it. Done. I think the other check is in another CL though. https://codereview.chromium.org/2129253002/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:3015: pod.offsetHeight != CROS_PIN_POD_HEIGHT) { On 2016/07/12 19:51:26, jdufault wrote: > Can we just set height/width to the expected values instead of forking the > logic? The height/width is being set in another function based on whether it is CrOS or desktop and I guess this is too check that the pods are the right size. I didn't know exactly what this is for so I just added a extra check because sometimes the pod is expanded for the pin. https://codereview.chromium.org/2129253002/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:3030: pinPadding *= -1; On 2016/07/12 19:51:26, jdufault wrote: > We want negative padding? I renamed it offsetFromPin. The pods on the left of the pod showing the pin add a negative offset.
https://codereview.chromium.org/2129253002/diff/40001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2129253002/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1111: var elements = this.querySelectorAll('.pin-modifiable'); > Array() seems to make an array of arrays where the first element is the array > and the rest are undefined. Okay - how about just making a helper function then that toggles the pin state? function setPinClass(element, enabled) { element.classList.toggle('pin-enabled', enabled); element.classList.toggle('pin-disabled', !enabled); } setPinClass(this, visible) for (...) setPinClass(element, visible) https://codereview.chromium.org/2129253002/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:2556: if (this.pods[i].classList.contains('pin-enabled')) On 2016/07/12 23:20:18, sammiequon wrote: > On 2016/07/12 19:51:26, jdufault wrote: > > This class check is happening in a couple of places, right? I think it'd make > > sense to add a helper method that does it. > > Done. I think the other check is in another CL though. Okay - please make sure you update that CL as well. You can make that other patch set depend on this one. https://codereview.chromium.org/2129253002/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:3015: pod.offsetHeight != CROS_PIN_POD_HEIGHT) { On 2016/07/12 23:20:18, sammiequon wrote: > On 2016/07/12 19:51:26, jdufault wrote: > > Can we just set height/width to the expected values instead of forking the > > logic? > > The height/width is being set in another function based on whether it is CrOS or > desktop and I guess this is too check that the pods are the right size. I didn't > know exactly what this is for so I just added a extra check because sometimes > the pod is expanded for the pin. It's probably worthwhile to investigate then to make sure we're doing the right thing. https://codereview.chromium.org/2129253002/diff/60001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2129253002/diff/60001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:3001: if (this.isAnyPinShown_()) { This logic is a bit confusing. 1. isAnyPinShown_ implies that more than one pod may be showing a pin keyboard, or that a pod other than podWithPin is showing a pin keyboard. 2. The width/height adjustment only works for podWithPin. So 1 is making an assumption that 2 is not dealing with. So if a pod that's not podWithPin is showing a PIN, then it will not be positioned correctly. I think it's fine to drop the assumption and assume that only the focusedPod_ is showing a PIN. If that's not the case, we should emit an error or warning message.
Patchset #5 (id:80001) has been deleted
sammiequon@chromium.org changed reviewers: + nkostylev@chromium.org
nkostylev@ - Since we added a pin keyboard to the pod, the offset height and offset width differ from the crOS user pod width and user pod height under certain circumstances. Please take a look at line 3012 and line 3017 in user_pod_row.js to see if this is a good way to address this.
https://codereview.chromium.org/2129253002/diff/40001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2129253002/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1111: var elements = this.querySelectorAll('.pin-modifiable'); On 2016/07/12 23:37:46, jdufault wrote: > > Array() seems to make an array of arrays where the first element is the array > > and the rest are undefined. > > Okay - how about just making a helper function then that toggles the pin state? > > function setPinClass(element, enabled) { > element.classList.toggle('pin-enabled', enabled); > element.classList.toggle('pin-disabled', !enabled); > } > > > setPinClass(this, visible) > for (...) > setPinClass(element, visible) Done. https://codereview.chromium.org/2129253002/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:2556: if (this.pods[i].classList.contains('pin-enabled')) On 2016/07/12 23:37:46, jdufault wrote: > On 2016/07/12 23:20:18, sammiequon wrote: > > On 2016/07/12 19:51:26, jdufault wrote: > > > This class check is happening in a couple of places, right? I think it'd > make > > > sense to add a helper method that does it. > > > > Done. I think the other check is in another CL though. > > Okay - please make sure you update that CL as well. You can make that other > patch set depend on this one. Done.
https://codereview.chromium.org/2129253002/diff/100001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2129253002/diff/100001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1105: for (var idx = 0; idx < elements_array.length; idx++) { Iterate over elements directly and add an explicit call to this.setPinClass(this, visible). That way we avoid the array allocation. https://codereview.chromium.org/2129253002/diff/100001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2988: var podWithPin = this.focusedPod_; What about extracting this logic into a helper function, something like: var pinPodLocation = this.findPodLocation(this.focusedPod_); /** * Returns the row/column of the given pod. * @return {!{row: Number, column: Number}} **/ function findPodLocation(pod) { /* ... */ } // ... if (row == pinPodLocation.row) {} if (column == pinPodLocation.column) {} https://codereview.chromium.org/2129253002/diff/100001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2999: console.error('Pod that is not the focused pod is showing its' + nit: add space after its Or what about: console.error('Non-focused pod is showing a PIN keyboard')
https://codereview.chromium.org/2129253002/diff/100001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2129253002/diff/100001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1105: for (var idx = 0; idx < elements_array.length; idx++) { On 2016/07/18 18:28:44, jdufault wrote: > Iterate over elements directly and add an explicit call to > this.setPinClass(this, visible). That way we avoid the array allocation. Done. https://codereview.chromium.org/2129253002/diff/100001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2988: var podWithPin = this.focusedPod_; On 2016/07/18 18:28:44, jdufault wrote: > What about extracting this logic into a helper function, something like: > > var pinPodLocation = this.findPodLocation(this.focusedPod_); > > /** > * Returns the row/column of the given pod. > * @return {!{row: Number, column: Number}} > **/ > function findPodLocation(pod) { /* ... */ } > > > // ... > > if (row == pinPodLocation.row) {} > if (column == pinPodLocation.column) {} Done. https://codereview.chromium.org/2129253002/diff/100001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2999: console.error('Pod that is not the focused pod is showing its' + On 2016/07/18 18:28:44, jdufault wrote: > nit: add space after its > > Or what about: > > console.error('Non-focused pod is showing a PIN keyboard') Done.
https://codereview.chromium.org/2129253002/diff/120001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2129253002/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1101: [].forEach.call( I don't think the forEach improves readability here since it requires a call, this binding, and the formatting is so wonky. I'd go with a regular for loop. var elements = this.getElementsByClassName('pin-tag'); for (var i = 0; i < elements.length; ++i) this.setPinClass_(elements[i], visible); this.setPinClass_(this, visible); Also, what do you think of updatePinClass_ instead of setPinClass_ for naming? https://codereview.chromium.org/2129253002/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2965: * @return {{columns: number, rows: number}} Add type annotations for parameters. Return type should be non-nullable? https://codereview.chromium.org/2129253002/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2967: findPinPodLocation_: function(pod, columns, rows, maxPods) { Rename to findPodLocation_ https://codereview.chromium.org/2129253002/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2970: if (pod && pod.isPinShown()) { pod should always be valid. This function doesn't need to be restricted to only PIN. https://codereview.chromium.org/2129253002/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2978: // Only the focused pod should ever show the PIN keyboard. Remove error message, this function doesn't need to know about PIN. It'd be pretty easy to add this validation back in somewhere else if we need it. https://codereview.chromium.org/2129253002/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2983: return {column: column, row: row}; Initialize column/row to undefined. If they're undefined at this point emit an error message that the pod couldn't be found. https://codereview.chromium.org/2129253002/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:3006: var pinPodLocation = this.findPinPodLocation_( Initialize pinPodLocation by default to { column+1, row+1 }, if focusedPod_ is showing a pin then initialize it to the pod location? var pinPodLocation = { column: columns + 1, row: rows + 1 }; // Or what you had before for the default value. if (this.focusedPod_.isPinShown()) pinPodLocation = this.findPodLocation_(this.focusedPod_);
https://codereview.chromium.org/2129253002/diff/120001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2129253002/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1101: [].forEach.call( On 2016/07/19 18:22:19, jdufault wrote: > I don't think the forEach improves readability here since it requires a call, > this binding, and the formatting is so wonky. I'd go with a regular for loop. > > var elements = this.getElementsByClassName('pin-tag'); > for (var i = 0; i < elements.length; ++i) > this.setPinClass_(elements[i], visible); > > this.setPinClass_(this, visible); > > Also, what do you think of updatePinClass_ instead of setPinClass_ for naming? Done. https://codereview.chromium.org/2129253002/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2965: * @return {{columns: number, rows: number}} On 2016/07/19 18:22:19, jdufault wrote: > Add type annotations for parameters. > > Return type should be non-nullable? Added type annotations. Return type can be nullable if this function is not used with a valid pod. https://codereview.chromium.org/2129253002/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2967: findPinPodLocation_: function(pod, columns, rows, maxPods) { On 2016/07/19 18:22:19, jdufault wrote: > Rename to findPodLocation_ Done. https://codereview.chromium.org/2129253002/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2970: if (pod && pod.isPinShown()) { On 2016/07/19 18:22:19, jdufault wrote: > pod should always be valid. > > This function doesn't need to be restricted to only PIN. I moved this check outside as suggested, put focusPod_ is sometimes null, because placePods is used when nothing is in focus as well. https://codereview.chromium.org/2129253002/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2978: // Only the focused pod should ever show the PIN keyboard. On 2016/07/19 18:22:19, jdufault wrote: > Remove error message, this function doesn't need to know about PIN. > > It'd be pretty easy to add this validation back in somewhere else if we need it. Done. https://codereview.chromium.org/2129253002/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2983: return {column: column, row: row}; On 2016/07/19 18:22:19, jdufault wrote: > Initialize column/row to undefined. If they're undefined at this point emit an > error message that the pod couldn't be found. Done. https://codereview.chromium.org/2129253002/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:3006: var pinPodLocation = this.findPinPodLocation_( On 2016/07/19 18:22:19, jdufault wrote: > Initialize pinPodLocation by default to { column+1, row+1 }, if focusedPod_ is > showing a pin then initialize it to the pod location? > > var pinPodLocation = { column: columns + 1, row: rows + 1 }; // Or what you > had before for the default value. > if (this.focusedPod_.isPinShown()) > pinPodLocation = this.findPodLocation_(this.focusedPod_); Done.
https://codereview.chromium.org/2129253002/diff/120001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2129253002/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2965: * @return {{columns: number, rows: number}} On 2016/07/20 17:11:06, sammiequon wrote: > On 2016/07/19 18:22:19, jdufault wrote: > > Add type annotations for parameters. > > > > Return type should be non-nullable? > > Added type annotations. Return type can be nullable if this function is not used > with a valid pod. console.error doesn't abort JS execution, so { column: undefined, row: undefined } is returned in that case. https://codereview.chromium.org/2129253002/diff/140001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2129253002/diff/140001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2985: row = currentRow; Eliminate the currentColumn and currentRow temporary values; directly store the computed value.
https://codereview.chromium.org/2129253002/diff/140001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2129253002/diff/140001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2985: row = currentRow; On 2016/07/20 17:41:26, jdufault wrote: > Eliminate the currentColumn and currentRow temporary values; directly store the > computed value. Done.
https://codereview.chromium.org/2129253002/diff/120001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2129253002/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2965: * @return {{columns: number, rows: number}} On 2016/07/20 17:41:26, jdufault wrote: > On 2016/07/20 17:11:06, sammiequon wrote: > > On 2016/07/19 18:22:19, jdufault wrote: > > > Add type annotations for parameters. > > > > > > Return type should be non-nullable? > > > > Added type annotations. Return type can be nullable if this function is not > used > > with a valid pod. > > console.error doesn't abort JS execution, so { column: undefined, row: undefined > } is returned in that case. Please address this comment :) https://codereview.chromium.org/2129253002/diff/160001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2129253002/diff/160001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2981: this.pods.forEach(function(currentPod, index) { What about using this.pods.indexOf(pod)?
https://codereview.chromium.org/2129253002/diff/120001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2129253002/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2965: * @return {{columns: number, rows: number}} On 2016/07/20 18:20:04, jdufault wrote: > On 2016/07/20 17:41:26, jdufault wrote: > > On 2016/07/20 17:11:06, sammiequon wrote: > > > On 2016/07/19 18:22:19, jdufault wrote: > > > > Add type annotations for parameters. > > > > > > > > Return type should be non-nullable? > > > > > > Added type annotations. Return type can be nullable if this function is not > > used > > > with a valid pod. > > > > console.error doesn't abort JS execution, so { column: undefined, row: > undefined > > } is returned in that case. > > Please address this comment :) As per offline conversation, {column: -1, row: -1} returning is expected behavior. https://codereview.chromium.org/2129253002/diff/160001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2129253002/diff/160001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2981: this.pods.forEach(function(currentPod, index) { On 2016/07/20 18:20:04, jdufault wrote: > What about using this.pods.indexOf(pod)? Done.
https://codereview.chromium.org/2129253002/diff/180001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2129253002/diff/180001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2986: if (column == -1 || row == -1) { How about if (index >= 0) { /* ... */ } else { console.error(/*...*/); }
https://codereview.chromium.org/2129253002/diff/180001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2129253002/diff/180001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2986: if (column == -1 || row == -1) { On 2016/07/21 19:46:22, jdufault wrote: > How about > > if (index >= 0) { > /* ... */ > } else { > console.error(/*...*/); > } Done.
https://codereview.chromium.org/2129253002/diff/200001/ui/message_center/view... File ui/message_center/views/message_center_view.cc (right): https://codereview.chromium.org/2129253002/diff/200001/ui/message_center/view... ui/message_center/views/message_center_view.cc:159: LOG(ERROR) << "ERMMMM"; Revert all of these LOG messages.
Sorry haha. https://codereview.chromium.org/2129253002/diff/200001/ui/message_center/view... File ui/message_center/views/message_center_view.cc (right): https://codereview.chromium.org/2129253002/diff/200001/ui/message_center/view... ui/message_center/views/message_center_view.cc:159: LOG(ERROR) << "ERMMMM"; On 2016/07/21 20:59:50, jdufault wrote: > Revert all of these LOG messages. Done.
On 2016/07/21 22:03:30, sammiequon wrote: > Sorry haha. > > https://codereview.chromium.org/2129253002/diff/200001/ui/message_center/view... > File ui/message_center/views/message_center_view.cc (right): > > https://codereview.chromium.org/2129253002/diff/200001/ui/message_center/view... > ui/message_center/views/message_center_view.cc:159: LOG(ERROR) << "ERMMMM"; > On 2016/07/21 20:59:50, jdufault wrote: > > Revert all of these LOG messages. > > Done. lgtm
sammiequon@chromium.org changed reviewers: + xiyuan@chromium.org
On 2016/07/22 00:43:46, jdufault wrote: > On 2016/07/21 22:03:30, sammiequon wrote: > > Sorry haha. > > > > > https://codereview.chromium.org/2129253002/diff/200001/ui/message_center/view... > > File ui/message_center/views/message_center_view.cc (right): > > > > > https://codereview.chromium.org/2129253002/diff/200001/ui/message_center/view... > > ui/message_center/views/message_center_view.cc:159: LOG(ERROR) << "ERMMMM"; > > On 2016/07/21 20:59:50, jdufault wrote: > > > Revert all of these LOG messages. > > > > Done. > > lgtm xiyuan@ - Please take a look.
lgtm
The CQ bit was checked by sammiequon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdufault@chromium.org Link to the patchset: https://codereview.chromium.org/2129253002/#ps240001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Multi-pod support for lock screen. BUG=616536 ========== to ========== Multi-pod support for lock screen. BUG=616536 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Multi-pod support for lock screen. BUG=616536 ========== to ========== Multi-pod support for lock screen. BUG=616536 Committed: https://crrev.com/a8e4ad8678760fe8d08c24ad1788347990042b34 Cr-Commit-Position: refs/heads/master@{#408734} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/a8e4ad8678760fe8d08c24ad1788347990042b34 Cr-Commit-Position: refs/heads/master@{#408734} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
