Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2159323003:
[MD Feedback] Add placeholder feedback entry point. (Closed)
Description[MD Feedback] Add placeholder feedback entry point.
This change adds a placeholder entry point for the redesigned feedback dialog. This makes it easy to do preliminary manual testing. This does not affect the current feedback UI.
Pulled out MdFeedbackDialogDelegate and created a Controller (more to come) to be able to create and show the dialog, as there should be a way to determine whether to create a new dialog or bring an existing one to the front depending on the current browser profile. That work TBD, as we're still investigating how to determine the dialog-to-profile mapping.
BUG=615533
Committed: https://crrev.com/7ef2fc0dbea076597d474315e15bae6fffb12de8
Cr-Commit-Position: refs/heads/master@{#410955}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Changes per afakhry@'s comments. Rebase. #
Total comments: 4
Patch Set 3 : Changes per afakhry@'s comments. #
Total comments: 2
Messages
Total messages: 32 (18 generated)
Description was changed from ========== . BUG= ========== to ========== [MD Feedback] Add placeholder feedback entry point. BUG=615533 ==========
apacible@chromium.org changed reviewers: + rkc@chromium.org
Description was changed from ========== [MD Feedback] Add placeholder feedback entry point. BUG=615533 ========== to ========== [MD Feedback] Add placeholder feedback entry point. This change adds a placeholder entry point for the redesigned feedback dialog. This makes it easy to do preliminary manual testing. This does not affect the current feedback UI. Pulled out MdFeedbackDialogDelegate and created a Controller (more to come) to be able to create and show the dialog, as there should be a way to determine whether to create a new dialog or bring an existing one to the front depending on the current browser profile. That work TBD, as we're still investigating how to determine the dialog-to-profile mapping. BUG=615533 ==========
The CQ bit was checked by apacible@chromium.org to run a CQ dry run
PTAL, thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
friendly ping!
rkc@chromium.org changed reviewers: + afakhry@chromium.org
Ahmed is the owner of Feedback. Please keep him in the loop for any changes you make to Feedback.
rkc@chromium.org changed reviewers: - rkc@chromium.org
https://codereview.chromium.org/2159323003/diff/1/chrome/browser/feedback/sho... File chrome/browser/feedback/show_feedback_page.cc (right): https://codereview.chromium.org/2159323003/diff/1/chrome/browser/feedback/sho... chrome/browser/feedback/show_feedback_page.cc:85: MdFeedbackDialogController::GetOrCreate(); It's actually GetOrCreateAndGet() ;) To be consistent with the rest of the code in the codebase, where Get() or GetInstance() are far more common, you might want to call it just Get() and then shorten your code like: MdFeedbackDialogController::Get()->Show(profile); https://codereview.chromium.org/2159323003/diff/1/chrome/browser/ui/webui/md_... File chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.cc (right): https://codereview.chromium.org/2159323003/diff/1/chrome/browser/ui/webui/md_... chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.cc:66: MdFeedbackDialogController::MdFeedbackDialogController() {} Please keep the order of your functions implementations to match that of their declarations in the .h file. https://codereview.chromium.org/2159323003/diff/1/chrome/browser/ui/webui/md_... chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.cc:73: return new MdFeedbackDialogController(); Hmmm, even with the presence of the above TODO, I don't think we should land it like this. Can we please, for now, make sure that it gets created only once? Maybe use a singleton for now? https://cs.chromium.org/chromium/src/base/memory/singleton.h?type=cs&q=f:+sin... https://codereview.chromium.org/2159323003/diff/1/chrome/browser/ui/webui/md_... chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.cc:82: chrome::ShowWebDialog(NULL, browser_context, new MdFeedbackDialogDelegate()); Nit: NULL --> nullptr. https://codereview.chromium.org/2159323003/diff/1/chrome/browser/ui/webui/md_... chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.cc:83: #endif Nit: #endif // !defined(OS_MACOSX) https://codereview.chromium.org/2159323003/diff/1/chrome/browser/ui/webui/md_... File chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.h (right): https://codereview.chromium.org/2159323003/diff/1/chrome/browser/ui/webui/md_... chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.h:10: class MdFeedbackDialogController { Please document your class, its responsibility and its key functions like Show(). https://codereview.chromium.org/2159323003/diff/1/chrome/browser/ui/webui/md_... chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.h:12: ~MdFeedbackDialogController(); You can just say ~MdFeedbackDialogController() = default; And get rid of the empty implementation in the cc file.
Thank you for the comments! I forgot to upload a rebase patch; there's a couple changes pulled into the md_feedback_ui.cc and chrome_browser_ui.gypi files that are unrelated in the diff. https://codereview.chromium.org/2159323003/diff/1/chrome/browser/feedback/sho... File chrome/browser/feedback/show_feedback_page.cc (right): https://codereview.chromium.org/2159323003/diff/1/chrome/browser/feedback/sho... chrome/browser/feedback/show_feedback_page.cc:85: MdFeedbackDialogController::GetOrCreate(); On 2016/08/02 22:15:56, afakhry wrote: > It's actually GetOrCreateAndGet() ;) > To be consistent with the rest of the code in the codebase, where Get() or > GetInstance() are far more common, you might want to call it just Get() and then > shorten your code like: > > MdFeedbackDialogController::Get()->Show(profile); Done. https://codereview.chromium.org/2159323003/diff/1/chrome/browser/ui/webui/md_... File chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.cc (right): https://codereview.chromium.org/2159323003/diff/1/chrome/browser/ui/webui/md_... chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.cc:66: MdFeedbackDialogController::MdFeedbackDialogController() {} On 2016/08/02 22:15:56, afakhry wrote: > Please keep the order of your functions implementations to match that of their > declarations in the .h file. Done. https://codereview.chromium.org/2159323003/diff/1/chrome/browser/ui/webui/md_... chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.cc:73: return new MdFeedbackDialogController(); On 2016/08/02 22:15:56, afakhry wrote: > Hmmm, even with the presence of the above TODO, I don't think we should land it > like this. Can we please, for now, make sure that it gets created only once? > > Maybe use a singleton for now? > https://cs.chromium.org/chromium/src/base/memory/singleton.h?type=cs&q=f:+sin... SGTM. Switched to using a singleton. https://codereview.chromium.org/2159323003/diff/1/chrome/browser/ui/webui/md_... chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.cc:82: chrome::ShowWebDialog(NULL, browser_context, new MdFeedbackDialogDelegate()); On 2016/08/02 22:15:56, afakhry wrote: > Nit: NULL --> nullptr. Done. https://codereview.chromium.org/2159323003/diff/1/chrome/browser/ui/webui/md_... chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.cc:83: #endif On 2016/08/02 22:15:56, afakhry wrote: > Nit: #endif // !defined(OS_MACOSX) Done. https://codereview.chromium.org/2159323003/diff/1/chrome/browser/ui/webui/md_... File chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.h (right): https://codereview.chromium.org/2159323003/diff/1/chrome/browser/ui/webui/md_... chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.h:10: class MdFeedbackDialogController { On 2016/08/02 22:15:56, afakhry wrote: > Please document your class, its responsibility and its key functions like > Show(). Done. https://codereview.chromium.org/2159323003/diff/1/chrome/browser/ui/webui/md_... chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.h:12: ~MdFeedbackDialogController(); On 2016/08/02 22:15:56, afakhry wrote: > You can just say > ~MdFeedbackDialogController() = default; > And get rid of the empty implementation in the cc file. Done.
Thank you! LGTM % 2 tiny nits. https://codereview.chromium.org/2159323003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.h (right): https://codereview.chromium.org/2159323003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.h:11: // This class manages MD Feedback dialog creation, destruction, and showing a Nit: you can just say "Manages ...". :) https://codereview.chromium.org/2159323003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.h:16: // Return the singleton instance of MdFeedbackDialogController. Nit: you can safely remove this comment. It's understandable enough.
The CQ bit was checked by apacible@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
apacible@chromium.org changed reviewers: + rkc@chromium.org
Thanks! +rkc again for OWNERS. The other c/b/feedback owner seems to be working on android a11y. (Does this file need to be updated to include afakhry?) https://codereview.chromium.org/2159323003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.h (right): https://codereview.chromium.org/2159323003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.h:11: // This class manages MD Feedback dialog creation, destruction, and showing a On 2016/08/09 23:36:31, afakhry wrote: > Nit: you can just say "Manages ...". :) Done. https://codereview.chromium.org/2159323003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.h:16: // Return the singleton instance of MdFeedbackDialogController. On 2016/08/09 23:36:31, afakhry wrote: > Nit: you can safely remove this comment. It's understandable enough. Done.
rs-lgtm Yes, that OWNERs file needs to be updated.
On 2016/08/10 00:13:32, rkc wrote: > Yes, that OWNERs file needs to be updated. Thanks!
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 apacible@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from afakhry@chromium.org Link to the patchset: https://codereview.chromium.org/2159323003/#ps40001 (title: "Changes per afakhry@'s comments.")
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 ========== [MD Feedback] Add placeholder feedback entry point. This change adds a placeholder entry point for the redesigned feedback dialog. This makes it easy to do preliminary manual testing. This does not affect the current feedback UI. Pulled out MdFeedbackDialogDelegate and created a Controller (more to come) to be able to create and show the dialog, as there should be a way to determine whether to create a new dialog or bring an existing one to the front depending on the current browser profile. That work TBD, as we're still investigating how to determine the dialog-to-profile mapping. BUG=615533 ========== to ========== [MD Feedback] Add placeholder feedback entry point. This change adds a placeholder entry point for the redesigned feedback dialog. This makes it easy to do preliminary manual testing. This does not affect the current feedback UI. Pulled out MdFeedbackDialogDelegate and created a Controller (more to come) to be able to create and show the dialog, as there should be a way to determine whether to create a new dialog or bring an existing one to the front depending on the current browser profile. That work TBD, as we're still investigating how to determine the dialog-to-profile mapping. BUG=615533 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [MD Feedback] Add placeholder feedback entry point. This change adds a placeholder entry point for the redesigned feedback dialog. This makes it easy to do preliminary manual testing. This does not affect the current feedback UI. Pulled out MdFeedbackDialogDelegate and created a Controller (more to come) to be able to create and show the dialog, as there should be a way to determine whether to create a new dialog or bring an existing one to the front depending on the current browser profile. That work TBD, as we're still investigating how to determine the dialog-to-profile mapping. BUG=615533 ========== to ========== [MD Feedback] Add placeholder feedback entry point. This change adds a placeholder entry point for the redesigned feedback dialog. This makes it easy to do preliminary manual testing. This does not affect the current feedback UI. Pulled out MdFeedbackDialogDelegate and created a Controller (more to come) to be able to create and show the dialog, as there should be a way to determine whether to create a new dialog or bring an existing one to the front depending on the current browser profile. That work TBD, as we're still investigating how to determine the dialog-to-profile mapping. BUG=615533 Committed: https://crrev.com/7ef2fc0dbea076597d474315e15bae6fffb12de8 Cr-Commit-Position: refs/heads/master@{#410955} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7ef2fc0dbea076597d474315e15bae6fffb12de8 Cr-Commit-Position: refs/heads/master@{#410955}
Message was sent while issue was closed.
https://codereview.chromium.org/2159323003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.cc (right): https://codereview.chromium.org/2159323003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.cc:82: MdFeedbackDialogController::MdFeedbackDialogController() {} apacible, I just noticed you didn't remove this implementation, even though you wrote = default; in the header. Could you please remove this as part of your other CL I'm currently reviewing once I send you the comments? Thanks!
Message was sent while issue was closed.
https://codereview.chromium.org/2159323003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.cc (right): https://codereview.chromium.org/2159323003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_feedback/md_feedback_dialog_controller.cc:82: MdFeedbackDialogController::MdFeedbackDialogController() {} On 2016/08/11 20:44:39, afakhry wrote: > apacible, I just noticed you didn't remove this implementation, even though you > wrote = default; in the header. Could you please remove this as part of your > other CL I'm currently reviewing once I send you the comments? Thanks! Ah, I only wrote = default for the destructor and removed that on the .cc side. I'll do that for the constructor as well in the other CL. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
