Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1250823005:
Run the certificate management dialog with BaseShellDialogImpl. (Closed)
|
Created:
5 years, 5 months ago by davidben Modified:
5 years, 4 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, asanka Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRun the certificate management dialog with BaseShellDialogImpl.
This avoids the nested event loop.
BUG=513113
TEST=Set Windows to 'classic' theme.
Open Settings -> Show advanced settings -> Manage certificates
Move the dialog around. See screenshot in bug.
Committed: https://crrev.com/307679f882171bdaca4f5cc2015a93bb9fd7ef24
Cr-Commit-Position: refs/heads/master@{#341224}
Patch Set 1 #
Total comments: 11
Patch Set 2 : mmenke comments #
Total comments: 6
Patch Set 3 : comments #Patch Set 4 : check for existing dialog #
Total comments: 7
Patch Set 5 : and now with no state #Patch Set 6 : #Patch Set 7 : unnecessary includes #Messages
Total messages: 27 (5 generated)
davidben@chromium.org changed reviewers: + ananta@chromium.org
ananta: I'm guessing you're the right person to review this since you added BaseShellDialogImpl. asanka, mmenke: CC'ing you two in case you also want to look at it. https://codereview.chromium.org/1250823005/diff/1/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options/advanced_options_utils_win.cc (right): https://codereview.chromium.org/1250823005/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/advanced_options_utils_win.cc:33: // itself when the dialog is closed. I hate self-deleting classes, but the dialog is fire-and-forget, so I don't see a clear alternative. https://codereview.chromium.org/1250823005/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/advanced_options_utils_win.cc:54: DisableOwner(run_state_.owner); This seems kinda unnecessary since we're not doing anything with the result, but I didn't want to call EndRun with it enabled in case that confused anything.
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
Some cleanup suggestions https://codereview.chromium.org/1250823005/diff/1/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options/advanced_options_utils_win.cc (right): https://codereview.chromium.org/1250823005/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/advanced_options_utils_win.cc:41: run_state_.dialog_thread->message_loop()->PostTask( PostTaskAndReply? (You'll need to use task_runner() instead of message_loop(), I believe) https://codereview.chromium.org/1250823005/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/advanced_options_utils_win.cc:54: DisableOwner(run_state_.owner); On 2015/07/23 19:49:33, David Benjamin wrote: > This seems kinda unnecessary since we're not doing anything with the result, but > I didn't want to call EndRun with it enabled in case that confused anything. Doesn't BeginRun already do this? https://codereview.chromium.org/1250823005/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/advanced_options_utils_win.cc:62: EndRun(run_state_); Could you just do this in the destructor, and use a scoped_ptr in your putative PostTaskAndReply? Having something take ownership of itself in its constructor seems a little weird (Though that's what you're implicitly doing), so could make a separate method to do that after construction or something.
https://codereview.chromium.org/1250823005/diff/1/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options/advanced_options_utils_win.cc (right): https://codereview.chromium.org/1250823005/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/advanced_options_utils_win.cc:41: run_state_.dialog_thread->message_loop()->PostTask( On 2015/07/23 20:03:34, mmenke wrote: > PostTaskAndReply? (You'll need to use task_runner() instead of message_loop(), > I believe) Done. https://codereview.chromium.org/1250823005/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/advanced_options_utils_win.cc:54: DisableOwner(run_state_.owner); On 2015/07/23 20:03:34, mmenke wrote: > On 2015/07/23 19:49:33, David Benjamin wrote: > > This seems kinda unnecessary since we're not doing anything with the result, > but > > I didn't want to call EndRun with it enabled in case that confused anything. > > Doesn't BeginRun already do this? BeginRun disables it, but CryptUIDlgCertMgr will re-enable it, so DisableOwner exists to let you keep it disabled while the response is in transit. We don't actually care about this. I just wasn't sure if EndRun (which internally re-enables the window) would break, so I matched other callers. Do you or ananta know if this is okay? https://codereview.chromium.org/1250823005/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/advanced_options_utils_win.cc:62: EndRun(run_state_); On 2015/07/23 20:03:34, mmenke wrote: > Could you just do this in the destructor, and use a scoped_ptr in your putative > PostTaskAndReply? Having something take ownership of itself in its constructor > seems a little weird (Though that's what you're implicitly doing), so could make > a separate method to do that after construction or something. Done. Made it no longer self-owning.
https://codereview.chromium.org/1250823005/diff/1/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options/advanced_options_utils_win.cc (right): https://codereview.chromium.org/1250823005/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/advanced_options_utils_win.cc:54: DisableOwner(run_state_.owner); On 2015/07/23 20:21:55, David Benjamin wrote: > On 2015/07/23 20:03:34, mmenke wrote: > > On 2015/07/23 19:49:33, David Benjamin wrote: > > > This seems kinda unnecessary since we're not doing anything with the result, > > but > > > I didn't want to call EndRun with it enabled in case that confused anything. > > > > Doesn't BeginRun already do this? > > BeginRun disables it, but CryptUIDlgCertMgr will re-enable it, so DisableOwner > exists to let you keep it disabled while the response is in transit. > > We don't actually care about this. I just wasn't sure if EndRun (which > internally re-enables the window) would break, so I matched other callers. Do > you or ananta know if this is okay? Does CryptUIDlgCertMgr display a modal dialog?. From the description it seems that it does, and if yes then the parent window would not get any input or activation messages anyways?
https://codereview.chromium.org/1250823005/diff/1/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options/advanced_options_utils_win.cc (right): https://codereview.chromium.org/1250823005/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/advanced_options_utils_win.cc:54: DisableOwner(run_state_.owner); On 2015/07/23 20:21:55, David Benjamin wrote: > On 2015/07/23 20:03:34, mmenke wrote: > > On 2015/07/23 19:49:33, David Benjamin wrote: > > > This seems kinda unnecessary since we're not doing anything with the result, > > but > > > I didn't want to call EndRun with it enabled in case that confused anything. > > > > Doesn't BeginRun already do this? > > BeginRun disables it, but CryptUIDlgCertMgr will re-enable it, so DisableOwner > exists to let you keep it disabled while the response is in transit. > > We don't actually care about this. I just wasn't sure if EndRun (which > internally re-enables the window) would break, so I matched other callers. Do > you or ananta know if this is okay? Enabling an already enabled window should be fine. https://codereview.chromium.org/1250823005/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/advanced_options_utils_win.cc (right): https://codereview.chromium.org/1250823005/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/advanced_options_utils_win.cc:40: run_state_ = BeginRun(parent); Now this seems a bit weird...It really should be in Show(), and EndRun should be called just before invoking the callback, rather than in the destructor (Yea, I suggested putting it there in the first place, but now it seems weird. Other dialogs seem designed for re-use, too) https://codereview.chromium.org/1250823005/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/advanced_options_utils_win.cc:47: void Show(const base::Closure& callback) { Should we check if (IsRunningDialogForOwner) and just invoke the callback in that case, or something? Showing two modal dialogs at once just seems weird. https://codereview.chromium.org/1250823005/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/advanced_options_utils_win.cc:63: scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner_; No longer needed.
https://codereview.chromium.org/1250823005/diff/1/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options/advanced_options_utils_win.cc (right): https://codereview.chromium.org/1250823005/diff/1/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/advanced_options_utils_win.cc:54: DisableOwner(run_state_.owner); On 2015/07/23 20:34:29, mmenke wrote: > On 2015/07/23 20:21:55, David Benjamin wrote: > > On 2015/07/23 20:03:34, mmenke wrote: > > > On 2015/07/23 19:49:33, David Benjamin wrote: > > > > This seems kinda unnecessary since we're not doing anything with the > result, > > > but > > > > I didn't want to call EndRun with it enabled in case that confused > anything. > > > > > > Doesn't BeginRun already do this? > > > > BeginRun disables it, but CryptUIDlgCertMgr will re-enable it, so DisableOwner > > exists to let you keep it disabled while the response is in transit. > > > > We don't actually care about this. I just wasn't sure if EndRun (which > > internally re-enables the window) would break, so I matched other callers. Do > > you or ananta know if this is okay? > > Enabling an already enabled window should be fine. I was just going by the documentation of DisableOwner and how SelectFileDialog uses it: https://code.google.com/p/chromium/codesearch#chromium/src/ui/shell_dialogs/b... Removed. https://codereview.chromium.org/1250823005/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/advanced_options_utils_win.cc (right): https://codereview.chromium.org/1250823005/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/advanced_options_utils_win.cc:40: run_state_ = BeginRun(parent); On 2015/07/23 20:34:29, mmenke wrote: > Now this seems a bit weird...It really should be in Show(), and EndRun should be > called just before invoking the callback, rather than in the destructor (Yea, I > suggested putting it there in the first place, but now it seems weird. Other > dialogs seem designed for re-use, too) Done. https://codereview.chromium.org/1250823005/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/advanced_options_utils_win.cc:47: void Show(const base::Closure& callback) { On 2015/07/23 20:34:29, mmenke wrote: > Should we check if (IsRunningDialogForOwner) and just invoke the callback in > that case, or something? Showing two modal dialogs at once just seems weird. Done. https://codereview.chromium.org/1250823005/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/advanced_options_utils_win.cc:63: scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner_; On 2015/07/23 20:34:29, mmenke wrote: > No longer needed. Done.
LGTM!
https://codereview.chromium.org/1250823005/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/advanced_options_utils_win.cc (right): https://codereview.chromium.org/1250823005/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/advanced_options_utils_win.cc:37: : web_contents_(web_contents) {} optional: Should we take the WebContents as a parameter to show instead?
https://codereview.chromium.org/1250823005/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/advanced_options_utils_win.cc (right): https://codereview.chromium.org/1250823005/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/advanced_options_utils_win.cc:37: : web_contents_(web_contents) {} On 2015/07/27 18:45:33, mmenke wrote: > optional: Should we take the WebContents as a parameter to show instead? I guess the question here is "what is a dialog object?". And, honestly, I have no clue. BaseShellDialogImpl, despite being something you subclass, has no actual state.
https://codereview.chromium.org/1250823005/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/advanced_options_utils_win.cc (right): https://codereview.chromium.org/1250823005/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/advanced_options_utils_win.cc:37: : web_contents_(web_contents) {} On 2015/07/27 18:47:45, David Benjamin wrote: > On 2015/07/27 18:45:33, mmenke wrote: > > optional: Should we take the WebContents as a parameter to show instead? > > I guess the question here is "what is a dialog object?". And, honestly, I have > no clue. BaseShellDialogImpl, despite being something you subclass, has no > actual state. I agree, and I'm with you on having no clue. https://codereview.chromium.org/1250823005/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/advanced_options_utils_win.cc:68: const base::Closure& callback) { Should we have a weak pointer here? Or this should be static.
https://codereview.chromium.org/1250823005/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/advanced_options_utils_win.cc (right): https://codereview.chromium.org/1250823005/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/advanced_options_utils_win.cc:68: const base::Closure& callback) { On 2015/07/27 19:08:14, mmenke wrote: > Should we have a weak pointer here? Or this should be static. Oh, right...with your ownership model, not needed.
https://codereview.chromium.org/1250823005/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/advanced_options_utils_win.cc (right): https://codereview.chromium.org/1250823005/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/advanced_options_utils_win.cc:37: : web_contents_(web_contents) {} On 2015/07/27 19:08:14, mmenke wrote: > On 2015/07/27 18:47:45, David Benjamin wrote: > > On 2015/07/27 18:45:33, mmenke wrote: > > > optional: Should we take the WebContents as a parameter to show instead? > > > > I guess the question here is "what is a dialog object?". And, honestly, I have > > no clue. BaseShellDialogImpl, despite being something you subclass, has no > > actual state. > > I agree, and I'm with you on having no clue. ananta? You wrote BaseShellDialogImpl. How are dialog classes in our code supposed to work? (How much of what should be associated with the class and how much should be associated with an individual call to Show() and passed along the base::Binds?)
https://codereview.chromium.org/1250823005/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/advanced_options_utils_win.cc (right): https://codereview.chromium.org/1250823005/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/advanced_options_utils_win.cc:37: : web_contents_(web_contents) {} On 2015/07/27 19:24:19, David Benjamin wrote: > On 2015/07/27 19:08:14, mmenke wrote: > > On 2015/07/27 18:47:45, David Benjamin wrote: > > > On 2015/07/27 18:45:33, mmenke wrote: > > > > optional: Should we take the WebContents as a parameter to show instead? > > > > > > I guess the question here is "what is a dialog object?". And, honestly, I > have > > > no clue. BaseShellDialogImpl, despite being something you subclass, has no > > > actual state. > > > > I agree, and I'm with you on having no clue. > > ananta? You wrote BaseShellDialogImpl. How are dialog classes in our code > supposed to work? (How much of what should be associated with the class and how > much should be associated with an individual call to Show() and passed along the > base::Binds?) That is upto the caller. Please look at SelectFileDialogImpl for an example. For this use case, I don't see why Show cannot take the web_contents_ in as an argument. Alternatively you can pass in the parent HWND directly.
On 2015/07/27 19:59:23, ananta wrote: > https://codereview.chromium.org/1250823005/diff/60001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/options/advanced_options_utils_win.cc (right): > > https://codereview.chromium.org/1250823005/diff/60001/chrome/browser/ui/webui... > chrome/browser/ui/webui/options/advanced_options_utils_win.cc:37: : > web_contents_(web_contents) {} > On 2015/07/27 19:24:19, David Benjamin wrote: > > On 2015/07/27 19:08:14, mmenke wrote: > > > On 2015/07/27 18:47:45, David Benjamin wrote: > > > > On 2015/07/27 18:45:33, mmenke wrote: > > > > > optional: Should we take the WebContents as a parameter to show > instead? > > > > > > > > I guess the question here is "what is a dialog object?". And, honestly, I > > have > > > > no clue. BaseShellDialogImpl, despite being something you subclass, has no > > > > actual state. > > > > > > I agree, and I'm with you on having no clue. > > > > ananta? You wrote BaseShellDialogImpl. How are dialog classes in our code > > supposed to work? (How much of what should be associated with the class and > how > > much should be associated with an individual call to Show() and passed along > the > > base::Binds?) > > That is upto the caller. Please look at SelectFileDialogImpl for an example. For > this use case, I don't see why Show cannot take the web_contents_ in as an > argument. Alternatively you can pass in the parent HWND directly. Done. Although now this class is pretty silly. It has no members and exists entirely to deal with BaseShellDialogImpl's visibility constraints on its methods. :-)
On 2015/07/27 20:32:19, David Benjamin wrote: > On 2015/07/27 19:59:23, ananta wrote: > > > https://codereview.chromium.org/1250823005/diff/60001/chrome/browser/ui/webui... > > File chrome/browser/ui/webui/options/advanced_options_utils_win.cc (right): > > > > > https://codereview.chromium.org/1250823005/diff/60001/chrome/browser/ui/webui... > > chrome/browser/ui/webui/options/advanced_options_utils_win.cc:37: : > > web_contents_(web_contents) {} > > On 2015/07/27 19:24:19, David Benjamin wrote: > > > On 2015/07/27 19:08:14, mmenke wrote: > > > > On 2015/07/27 18:47:45, David Benjamin wrote: > > > > > On 2015/07/27 18:45:33, mmenke wrote: > > > > > > optional: Should we take the WebContents as a parameter to show > > instead? > > > > > > > > > > I guess the question here is "what is a dialog object?". And, honestly, > I > > > have > > > > > no clue. BaseShellDialogImpl, despite being something you subclass, has > no > > > > > actual state. > > > > > > > > I agree, and I'm with you on having no clue. > > > > > > ananta? You wrote BaseShellDialogImpl. How are dialog classes in our code > > > supposed to work? (How much of what should be associated with the class and > > how > > > much should be associated with an individual call to Show() and passed along > > the > > > base::Binds?) > > > > That is upto the caller. Please look at SelectFileDialogImpl for an example. > For > > this use case, I don't see why Show cannot take the web_contents_ in as an > > argument. Alternatively you can pass in the parent HWND directly. > > Done. Although now this class is pretty silly. It has no members and exists > entirely to deal with BaseShellDialogImpl's visibility constraints on its > methods. :-) Yea, I ran into the exact same issue with the network diagnostics dialog, which is why I thought I might tackle this one, first, to work my way through the weirdness in a more constrained CL.
lgtm
davidben@chromium.org changed reviewers: + estade@chromium.org
+estade for owners
estade: friendly ping
rubbery lgtm
The CQ bit was checked by davidben@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1250823005/#ps120001 (title: "unnecessary includes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1250823005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1250823005/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/307679f882171bdaca4f5cc2015a93bb9fd7ef24 Cr-Commit-Position: refs/heads/master@{#341224} |
