Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2021753005:
[MD Feedback] Add basic chrome://feedback dialog. (Closed)
Description[MD Feedback] Add basic chrome://feedback dialog.
This adds a basic dialog delegate and controller to house the feedback WebUI. The dialog is not anchored or constrained to any other browser window.
Show() is only implemented for views. Mac will be done in a separate patch.
BUG=615533
Committed: https://crrev.com/cc877565b2977c7e8771eadb65afcecfeb8660e2
Cr-Commit-Position: refs/heads/master@{#397556}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : Changes per imcheng@'s comments. #
Total comments: 4
Patch Set 3 : Changes per imcheng@'s comments. #
Messages
Total messages: 16 (8 generated)
Description was changed from ========== feedback floaty dialog BUG= ========== to ========== [MD Feedback] Add basic chrome://feedback dialog. BUG=615533 ==========
Description was changed from ========== [MD Feedback] Add basic chrome://feedback dialog. BUG=615533 ========== to ========== [MD Feedback] Add basic chrome://feedback dialog. This adds a basic dialog delegate and controller to house the feedback WebUI. The dialog is not anchored or constrained to any other browser window. Show() is only implemented for views. Mac will be done in a separate patch. BUG=615533 ==========
apacible@chromium.org changed reviewers: + imcheng@chromium.org
Patchset #1 (id:1) has been deleted
Looks like you did a lot of the MR UI dialog setup. Future work includes a message handler, resource/strings provider, and the todos mentioned in the comments. PTAL, thanks!
https://codereview.chromium.org/2021753005/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.cc (right): https://codereview.chromium.org/2021753005/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.cc:71: MdFeedbackDialogController* MdFeedbackDialogController::GetOrCreate() { For now I do not see a need for the MdFeedbackDialogController class. I find controller class to be appropriate when you need to access/manipulate the dialog from the browser side in addition to the WebUI side. Consider removing this class for now and turning MdFeedbackDialogController::Show into a nonmember function. https://codereview.chromium.org/2021753005/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.cc:83: chrome::ShowWebDialog(NULL, browser_context, new MdFeedbackDialogDelegate()); Is the intention that at most one feedback UI dialog can be opened at a time, and does chrome::ShowWebDialog already provide that behavior? https://codereview.chromium.org/2021753005/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.h (right): https://codereview.chromium.org/2021753005/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.h:8: #include "content/public/browser/browser_context.h" content::BrowserContext can be forward declared in this file.
https://codereview.chromium.org/2021753005/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.cc (right): https://codereview.chromium.org/2021753005/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.cc:71: MdFeedbackDialogController* MdFeedbackDialogController::GetOrCreate() { On 2016/06/02 04:19:10, imcheng wrote: > For now I do not see a need for the MdFeedbackDialogController class. I find > controller class to be appropriate when you need to access/manipulate the dialog > from the browser side in addition to the WebUI side. Consider removing this > class for now and turning MdFeedbackDialogController::Show into a nonmember > function. Per offline conversation: - Removed MdFeedbackDialogController. - Moved MdFeedbackDialogDelegate to MdFeedbackUI. - Moved Show to MdFeedbackUI and renamed it. https://codereview.chromium.org/2021753005/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.cc:83: chrome::ShowWebDialog(NULL, browser_context, new MdFeedbackDialogDelegate()); On 2016/06/02 04:19:10, imcheng wrote: > Is the intention that at most one feedback UI dialog can be opened at a time, > and does chrome::ShowWebDialog already provide that behavior? Correct, at most one feedback UI dialog can be opened at a time, per profile. If you had multiple Chrome profiles up, you could open the dialog once per profile. ShowWebDialog does not provide this behavior. I'm also discussing some options for the Mac dialog (so this may change down the line) with the mac dev team. This is mostly an unblocker for starting the WebUI work, which should not be affected by the native dialog changes. https://codereview.chromium.org/2021753005/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.h (right): https://codereview.chromium.org/2021753005/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.h:8: #include "content/public/browser/browser_context.h" On 2016/06/02 04:19:10, imcheng wrote: > content::BrowserContext can be forward declared in this file. Done.
lgtm https://codereview.chromium.org/2021753005/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/md_feedback/md_feedback_ui.cc (right): https://codereview.chromium.org/2021753005/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_feedback/md_feedback_ui.cc:8: #if !defined(OS_MACOSX) put this below the unconditional #includes. https://codereview.chromium.org/2021753005/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_feedback/md_feedback_ui.cc:10: #endif // !OS_MACOSX nit: if the #if macro is short, no need to add comment on the #endif
https://codereview.chromium.org/2021753005/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/md_feedback/md_feedback_ui.cc (right): https://codereview.chromium.org/2021753005/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_feedback/md_feedback_ui.cc:8: #if !defined(OS_MACOSX) On 2016/06/02 19:50:00, imcheng wrote: > put this below the unconditional #includes. Done. https://codereview.chromium.org/2021753005/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_feedback/md_feedback_ui.cc:10: #endif // !OS_MACOSX On 2016/06/02 19:50:00, imcheng wrote: > nit: if the #if macro is short, no need to add comment on the #endif Done.
The CQ bit was checked by apacible@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from imcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2021753005/#ps60001 (title: "Changes per imcheng@'s comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021753005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2021753005/60001
Message was sent while issue was closed.
Description was changed from ========== [MD Feedback] Add basic chrome://feedback dialog. This adds a basic dialog delegate and controller to house the feedback WebUI. The dialog is not anchored or constrained to any other browser window. Show() is only implemented for views. Mac will be done in a separate patch. BUG=615533 ========== to ========== [MD Feedback] Add basic chrome://feedback dialog. This adds a basic dialog delegate and controller to house the feedback WebUI. The dialog is not anchored or constrained to any other browser window. Show() is only implemented for views. Mac will be done in a separate patch. BUG=615533 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [MD Feedback] Add basic chrome://feedback dialog. This adds a basic dialog delegate and controller to house the feedback WebUI. The dialog is not anchored or constrained to any other browser window. Show() is only implemented for views. Mac will be done in a separate patch. BUG=615533 ========== to ========== [MD Feedback] Add basic chrome://feedback dialog. This adds a basic dialog delegate and controller to house the feedback WebUI. The dialog is not anchored or constrained to any other browser window. Show() is only implemented for views. Mac will be done in a separate patch. BUG=615533 Committed: https://crrev.com/cc877565b2977c7e8771eadb65afcecfeb8660e2 Cr-Commit-Position: refs/heads/master@{#397556} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/cc877565b2977c7e8771eadb65afcecfeb8660e2 Cr-Commit-Position: refs/heads/master@{#397556} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
