Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2704303003:
Linux/Windows: Crash in profile switcher menu with account consistency enabled (Closed)
DescriptionLinux/Windows: Crash in profile switcher menu with account consistency enabled
BackgoundColorHoverButton always needs the ProfileChooserView to know
the system theme.
BUG=694506
Review-Url: https://codereview.chromium.org/2704303003
Cr-Commit-Position: refs/heads/master@{#452682}
Committed: https://chromium.googlesource.com/chromium/src/+/431dfbf17e0b37a057f16b587d8c134193392dfd
Patch Set 1 #
Total comments: 3
Patch Set 2 : Disable the button if no reauth required #Messages
Total messages: 25 (8 generated)
jlebel@chromium.org changed reviewers: + msarda@chromium.org
Hello Mihai, Can you review this patch? I can't find any reasons why we would want to pass NULL to the BackgroundColorHoverButton for the ProfileChooserView. Jérôme,
msarda@chromium.org changed reviewers: + anthonyvd@chromium.org
Code wise - l g t m. But I do not really know why this was special cased. Adding Anthony that may know the reason. Anthony: Please take a look at this CL.
On 2017/02/21 at 12:30:09, msarda wrote: > Code wise - l g t m. But I do not really know why this was special cased. Adding Anthony that may know the reason. > > Anthony: Please take a look at this CL. I think this is because the email only becomes clickable if there's a auth error to fix, so it makes sense to have a null delegate for the LabelButton otherwise (there are no events to handle if there's no auth error). I have to admit this might not be true after the switch to the new User Menu though, which would explain why this special case doesn't seem relevant anymore. In any case, if the User Menu still looks and works properly after this change (including auth error fixing), this lgtm
On 2017/02/22 15:47:51, anthonyvd wrote: > On 2017/02/21 at 12:30:09, msarda wrote: > > Code wise - l g t m. But I do not really know why this was special cased. > Adding Anthony that may know the reason. > > > > Anthony: Please take a look at this CL. > > I think this is because the email only becomes clickable if there's a auth error > to fix, so it makes sense to have a null delegate for the LabelButton otherwise > (there are no events to handle if there's no auth error). I have to admit this > might not be true after the switch to the new User Menu though, which would > explain why this special case doesn't seem relevant anymore. > > In any case, if the User Menu still looks and works properly after this change > (including auth error fixing), this lgtm The look is the same as other buttons.
jlebel@chromium.org changed reviewers: + pkasting@chromium.org
Hello Peter, Can you review this small patch related to a crash? Thank you,
LGTM https://codereview.chromium.org/2704303003/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (left): https://codereview.chromium.org/2704303003/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:2119: reauth_required ? this : NULL, Why were we trying to pass null before? Usually that means we're trying to toggle some behavior the constructed object will key off.
On 2017/02/23 01:35:02, Peter Kasting (backlogged) wrote: > LGTM > > https://codereview.chromium.org/2704303003/diff/1/chrome/browser/ui/views/pro... > File chrome/browser/ui/views/profiles/profile_chooser_view.cc (left): > > https://codereview.chromium.org/2704303003/diff/1/chrome/browser/ui/views/pro... > chrome/browser/ui/views/profiles/profile_chooser_view.cc:2119: reauth_required ? > this : NULL, > Why were we trying to pass null before? Usually that means we're trying to > toggle some behavior the constructed object will key off. I was not able to find any reason. I don't understand how this bug was not found before.
On 2017/02/23 01:36:34, jlebel wrote: > On 2017/02/23 01:35:02, Peter Kasting (backlogged) wrote: > > LGTM > > > > > https://codereview.chromium.org/2704303003/diff/1/chrome/browser/ui/views/pro... > > File chrome/browser/ui/views/profiles/profile_chooser_view.cc (left): > > > > > https://codereview.chromium.org/2704303003/diff/1/chrome/browser/ui/views/pro... > > chrome/browser/ui/views/profiles/profile_chooser_view.cc:2119: reauth_required > ? > > this : NULL, > > Why were we trying to pass null before? Usually that means we're trying to > > toggle some behavior the constructed object will key off. > > I was not able to find any reason. I don't understand how this bug was not found > before. It's because the crashing code was just added: https://codereview.chromium.org/2624683002/diff/200001/chrome/browser/ui/view... Before that, the param was only used to define a button listener. Which raises an issue I didn't think to ask about. This will result in the constructing view getting listener callbacks in more cases. Did you ensure none of those functions will do the wrong thing in these cases (i.e. they don't explicitly or implicitly assume that we're in the reauth_required case)?
https://codereview.chromium.org/2704303003/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (left): https://codereview.chromium.org/2704303003/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:2119: reauth_required ? this : NULL, On 2017/02/23 01:35:02, Peter Kasting wrote: > Why were we trying to pass null before? Usually that means we're trying to > toggle some behavior the constructed object will key off. You are right, I missed a feature related to this button. The button should be disable if reauth is not required.
The CQ bit was checked by jlebel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, anthonyvd@chromium.org Link to the patchset: https://codereview.chromium.org/2704303003/#ps20001 (title: "Disable the button if no reauth required")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487890888617950,
"parent_rev": "a618a8b81cafcde70d53510a03fb222605eb1cd4", "commit_rev":
"431dfbf17e0b37a057f16b587d8c134193392dfd"}
Message was sent while issue was closed.
Description was changed from ========== Linux/Windows: Crash in profile switcher menu with account consistency enabled BackgoundColorHoverButton always needs the ProfileChooserView to know the system theme. BUG=694506 ========== to ========== Linux/Windows: Crash in profile switcher menu with account consistency enabled BackgoundColorHoverButton always needs the ProfileChooserView to know the system theme. BUG=694506 Review-Url: https://codereview.chromium.org/2704303003 Cr-Commit-Position: refs/heads/master@{#452682} Committed: https://chromium.googlesource.com/chromium/src/+/431dfbf17e0b37a057f16b587d8c... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/431dfbf17e0b37a057f16b587d8c...
Message was sent while issue was closed.
LGTM if the answer to the question below is "no". https://codereview.chromium.org/2704303003/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (left): https://codereview.chromium.org/2704303003/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:2119: reauth_required ? this : NULL, On 2017/02/23 18:02:57, jlebel wrote: > On 2017/02/23 01:35:02, Peter Kasting wrote: > > Why were we trying to pass null before? Usually that means we're trying to > > toggle some behavior the constructed object will key off. > > You are right, I missed a feature related to this button. The button should be > disable if reauth is not required. Will disabling the button cause it to draw differently (different colors)?
Message was sent while issue was closed.
On 2017/02/24 00:51:19, Peter Kasting wrote: > LGTM if the answer to the question below is "no". > > https://codereview.chromium.org/2704303003/diff/1/chrome/browser/ui/views/pro... > File chrome/browser/ui/views/profiles/profile_chooser_view.cc (left): > > https://codereview.chromium.org/2704303003/diff/1/chrome/browser/ui/views/pro... > chrome/browser/ui/views/profiles/profile_chooser_view.cc:2119: reauth_required ? > this : NULL, > On 2017/02/23 18:02:57, jlebel wrote: > > On 2017/02/23 01:35:02, Peter Kasting wrote: > > > Why were we trying to pass null before? Usually that means we're trying to > > > toggle some behavior the constructed object will key off. > > > > You are right, I missed a feature related to this button. The button should be > > disable if reauth is not required. > > Will disabling the button cause it to draw differently (different colors)? I created: crbug.com/695828
Message was sent while issue was closed.
On 2017/02/24 12:23:40, jlebel wrote: > On 2017/02/24 00:51:19, Peter Kasting wrote: > > Will disabling the button cause it to draw differently (different colors)? > > I created: crbug.com/695828 Did you cause that with this CL? A functional change like adding disabling probably should have waited for re-review before landing. I'm concerned about the series of changes here. You've made several CLs, which have introduced some problems at the same time they've fixed others. Your descriptions and comments have made clear that, as is true for me as well, it's not clear to you how and why the old code is structured as it is. My review comments have generally been concerned and conservative, because it's clear the existing code is complicated and in need of some love, and, as noted above, I don't understand it (I was not the original reviewer for any of this). But in the face of those, it feels a bit like the patches here have been more "this seems like it works" than "I spent a week of my time to really research and test every conditional in this code and thoroughly understand it, and I can speak with authority on it". This is, sadly, not the kind of code where the first approach works. You need to execute the second, even though it costs much more time and effort. I don't have a specific fix to request -- I'm not going to ask that you revert this patch or something. I assume as a Chromium account holder you're plenty capable of figuring out the right route forward. I write all this merely to urge you, please take the time to truly grasp this code and get things right, and think about all the edge cases, and call out in review everything you don't understand and every possible behavior change, to ask for others to backstop you. The amount of time and series of regressions in this area so far concern me, and I want to make sure we don't continue to have them. Sorry if this seems depressing. I aim to be a helper; let's make the code better :)
Message was sent while issue was closed.
Hmm. I don't know that I phrased that very well. It's very late and I probably should have waited until the next work day to ponder it. I ask your forgiveness for any unclear, inaccurate, or inadvisable comments.
Message was sent while issue was closed.
nsphu.cnht@gmail.com changed reviewers: + nsphu.cnht@gmail.com
Message was sent while issue was closed.
Message was sent while issue was closed.
lgtm |
