Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2190653003:
[Md Feedback] Add initial data population for dialog. (Closed)
|
Created:
4 years, 4 months ago by apacible Modified:
4 years, 3 months ago CC:
chromium-reviews, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, apacible+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Md Feedback] Add initial data population for dialog.
This change populates the feedback dialog with some initial info, including profile email and current URL, by adding a new WebUI Message Handler for C++ <-> JS messaging. The handler will be build out to support more messaging in the future, e.g. system info, screenshots, and feedback submission.
BUG=632112
Committed: https://crrev.com/dc2b3be0e417c19e4b2df35594a4ab5bec917dd4
Cr-Commit-Position: refs/heads/master@{#418977}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rebase. #Patch Set 3 : Changes per afakhry@'s comments. #
Total comments: 27
Patch Set 4 : Changes per afakhry@'s comments (Part 1 from PS3). #Patch Set 5 : Rebase. #Patch Set 6 : Rebase. #Patch Set 7 : Fix GN build. #Patch Set 8 : Changes per afakhry@'s comments (Part 2 from PS3). #Patch Set 9 : Rebase. #
Total comments: 20
Patch Set 10 : Changes per afakhry@'s comments (Rebase). #Messages
Total messages: 82 (55 generated)
Description was changed from ========== message handler BUG= ========== to ========== message handler BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== message handler BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Work In Progress] [Md Feedback] Initial data population. BUG=632112 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [Work In Progress] [Md Feedback] Initial data population. BUG=632112 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Work In Progress] [Md Feedback] Add dialog data population. This change populates the feedback dialog with some initial data, including profile email and current URL. This adds a WebUI message handler to set up data messaging. BUG=632112 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [Work In Progress] [Md Feedback] Add dialog data population. This change populates the feedback dialog with some initial data, including profile email and current URL. This adds a WebUI message handler to set up data messaging. BUG=632112 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Work In Progress] [Md Feedback] Add dialog data population. This change populates the feedback dialog with some initial data, including profile email and current URL, by adding a new WebUI Message Handler for C++ <-> JS messaging. This does not include: - system information - screenshot BUG=632112 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
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...
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...
Patchset #2 (id:80001) has been deleted
Description was changed from ========== [Work In Progress] [Md Feedback] Add dialog data population. This change populates the feedback dialog with some initial data, including profile email and current URL, by adding a new WebUI Message Handler for C++ <-> JS messaging. This does not include: - system information - screenshot BUG=632112 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Md Feedback] Add initial data population for dialog. This change populates the feedback dialog with some initial info, including profile email and current URL, by adding a new WebUI Message Handler for C++ <-> JS messaging. The handler will be build out to support more messaging in the future, e.g. system info, screenshots, and feedback submission. BUG=632112 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:60001) has been deleted
apacible@chromium.org changed reviewers: + afakhry@chromium.org
PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
apacible, first thanks for your efforts! - Whenever you make a change that introduces UI changes, please consider helping your reviewers by adding links to screenshots to your CL description. - I had to patch your both CLs locally to see how it looks like. There were a bunch of conflicts, so you probably need to deal with that and decide which CL should go first. - So currently, this is how it looks for me: https://drive.google.com/a/google.com/file/d/0B6G_-uQnf1_LWkFjWU9DUVVSdkE/vie... - I understand that this is a WIP, but if there's something you can do to make it look better now, please do. - The privacy note has <strong> tags that weren't processed. You need to use something like <ph name="BEGIN_BOLD"><strong></ph>. For example: https://cs.chromium.org/chromium/src/chrome/app/chromeos_strings.grdp?q=IDS_L... Before we proceed with the review, please pick one that goes first, rebase the other on it. And let's handle them one by one. https://codereview.chromium.org/2190653003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_feedback/feedback_container.js (right): https://codereview.chromium.org/2190653003/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_feedback/feedback_container.js:37: this.i18n('privacyNote') : ''; This generates the following error: Uncaught TypeError: this.i18n is not a function. I fixed this by adding: behaviors: [I18nBehavior], before Properties: { and included <link rel="import" href="chrome://resources/html/i18n_behavior.html"> in the html file. https://codereview.chromium.org/2190653003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_feedback/md_feedback_ui.cc (right): https://codereview.chromium.org/2190653003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_ui.cc:127: handler_(new MdFeedbackWebUIMessageHandler(this)) { You don't need that internal reference |handler_| at all. See an example here: https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/chromeos/keyboar... https://codereview.chromium.org/2190653003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.cc (right): https://codereview.chromium.org/2190653003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.cc:24: GURL GetTargetTabUrl(int session_id, int index) { This is the same code as in chrome::ShowFeedbackPage(). There's no need for duplication. You can pass the URL in your other CL to the controller and remove this.
> Before we proceed with the review, please pick one that goes first, rebase the > other on it. And let's handle them one by one. Thanks for your comments! I'll be OOO today/tomorrow for an offsite but will respond to them soon (well, one for starters :) ).
> adding links to screenshots Noted! > if there's something you can do to make it look better Got it. I'll make it look nicer in a separate CL. :) > The privacy note has <strong> tags that weren't processed. Hmm, I'm using the same format here: https://cs.chromium.org/chromium/src/chrome/app/generated_resources.grd?l=7799 I thought the ph tags would be processed once translated. I'll have to look into this. https://codereview.chromium.org/2190653003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/md_feedback/feedback_container.js (right): https://codereview.chromium.org/2190653003/diff/100001/chrome/browser/resourc... chrome/browser/resources/md_feedback/feedback_container.js:37: this.i18n('privacyNote') : ''; On 2016/08/03 23:56:01, afakhry wrote: > This generates the following error: > > Uncaught TypeError: this.i18n is not a function. > > I fixed this by adding: > behaviors: [I18nBehavior], > before Properties: { > > and included > <link rel="import" href="chrome://resources/html/i18n_behavior.html"> > > in the html file. Done. https://codereview.chromium.org/2190653003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_feedback/md_feedback_ui.cc (right): https://codereview.chromium.org/2190653003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_ui.cc:127: handler_(new MdFeedbackWebUIMessageHandler(this)) { On 2016/08/03 23:56:01, afakhry wrote: > You don't need that internal reference |handler_| at all. See an example here: > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/chromeos/keyboar... Done. https://codereview.chromium.org/2190653003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.cc (right): https://codereview.chromium.org/2190653003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.cc:24: GURL GetTargetTabUrl(int session_id, int index) { On 2016/08/03 23:56:01, afakhry wrote: > This is the same code as in chrome::ShowFeedbackPage(). There's no need for > duplication. You can pass the URL in your other CL to the controller and remove > this. My plan is to remove this (and from chrome::ShowFeedbackPage once we switch over. I think it makes sense to have it in the handler to pull the data directly there rather than passing it around if we don't need to. WDYT?
Bear with me! :) https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_feedback/feedback.js (right): https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_feedback/feedback.js:5: cr.define('feedback', function() { First, thank you for the detailed comments and the clean code in this file. I'm not against this chrome-style way of defining scoping and objects, however, the more you work with me, you'll find that I'm a big fan of simplicity. I feel this way is a bit hairy (but I understand it is common in our chromium code base). But what if we can do better? :) Compare the code here to the simple one in this gPaste: https://paste.googleplex.com/5630126029012992 Which one do you like looking at and maintaining more? I'm not a JS expert at all, that's why I prefer simpler code. So if there's any advantage to this old chromium-style to the one in the gPaste, please let me know. [ Note that you'd have to rename your |kSetData| in the cc file to: constexpr char kSetData[] = "Feedback.UI.setData"; ] https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_feedback/feedback.js:72: window.addEventListener('load', feedback.initialize); Do you need to wait for the page to fully load (DOM, images, stylesheets, ...etc.) to initialize the form data? If not, then please use 'DOMContentLoaded' instead of 'load'. (I also want to note the difference visually, if possible. With load, the email and url are filled after the page shows up). https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_feedback/feedback_container.js (right): https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_feedback/feedback_container.js:41: this.i18n('privacyNote') : ''; I found a way to fix this <strong></strong> not being processed. in the HTML file: <div><p id="privacyNote"></p></div> and here in the JS file: ready: function() { this.$.privacyNote.innerHTML = loadTimeData.valueExists('privacyNote') ? loadTimeData.getString('privacyNote') : ''; } You can now safely remove getPrivacyNote_() and behaviors: [I18nBehavior], as well as the include <link rel="import" href="chrome://resources/html/i18n_behavior.html"> Please build the official chrome-branded version of Chrome to verify that you see the right string. https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_feedback/md_feedback_ui.cc (right): https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_ui.cc:79: web_ui->AddMessageHandler(handler); Since we won't use |handler| at all, how about this: ? web_ui->AddMessageHandler(new MdFeedbackWebUIMessageHandler(this)); https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.cc (right): https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.cc:19: const char kRequestData[] = "requestData"; constexpr https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.cc:22: const char kSetData[] = "feedback.ui.setData"; constexpr https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.cc:38: } I totally understand your intent and agree with you that this function should be local here. But that would be valid once we have your MD feedback dialog up and running and we remove the other one. :) Since this is shared code, we can't land duplicate code. So how about this: create something like feedback_dialog_utils.h/cc where you add GetFeedbackTargetTabUrl() and GetFeedbackProfile(). Have your code here as well as the one in show_feedback_page.cc use these two new functions and then remove all duplication. How does that sound? https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.cc:68: data.SetString("email", profile->GetProfileUserName()); For the email, in the current feedback app we use the SigninManagerFactory to get its value. Please see: https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/feedback_p... I think we should do the same thing here. https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.h (right): https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.h:13: } We don't need this forward declare here. Please remove. https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.h:24: // WebUIMessageHandler implementation. Recently this parent-class override comment has been shortened to just: // WebUIMessageHandler: https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.h:27: // Handlers for JavaScript messages. Hmmm, I'd rather this comment tells us what OnRequestData() actually does. https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.h:30: MdFeedbackUI* md_feedback_ui_; Nit [optional]: MdFeedbackUI* md_feedback_ui_; // Not owned.
Thanks for your comments! I'll be OOO for about a week, but acknowledge and will address the remaining comments when I get back. :) https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_feedback/feedback.js (right): https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_feedback/feedback.js:5: cr.define('feedback', function() { On 2016/08/11 23:31:31, afakhry wrote: > First, thank you for the detailed comments and the clean code in this file. I'm > not against this chrome-style way of defining scoping and objects, however, the > more you work with me, you'll find that I'm a big fan of simplicity. I feel this > way is a bit hairy (but I understand it is common in our chromium code base). > But what if we can do better? :) > > Compare the code here to the simple one in this gPaste: > https://paste.googleplex.com/5630126029012992 > > Which one do you like looking at and maintaining more? > I'm not a JS expert at all, that's why I prefer simpler code. So if there's any > advantage to this old chromium-style to the one in the gPaste, please let me > know. > > [ > Note that you'd have to rename your |kSetData| in the cc file to: > constexpr char kSetData[] = "Feedback.UI.setData"; > ] The snippet is definitely much simpler! The style here is the recommended format in the docs: https://www.chromium.org/developers/webui I'm strongly inclined to follow the general style for consistency with the rest of the chrome/browser/resources/* files (and also possibly because I'm more used to reading this type of format), but am otherwise unsure of a 'technical advantage' here. https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_feedback/feedback.js:72: window.addEventListener('load', feedback.initialize); On 2016/08/11 23:31:31, afakhry wrote: > Do you need to wait for the page to fully load (DOM, images, stylesheets, > ...etc.) to initialize the form data? If not, then please use 'DOMContentLoaded' > instead of 'load'. > > (I also want to note the difference visually, if possible. With load, the email > and url are filled after the page shows up). Done. https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_feedback/md_feedback_ui.cc (right): https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_ui.cc:79: web_ui->AddMessageHandler(handler); On 2016/08/11 23:31:31, afakhry wrote: > Since we won't use |handler| at all, how about this: ? > > web_ui->AddMessageHandler(new MdFeedbackWebUIMessageHandler(this)); Done. https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.cc (right): https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.cc:19: const char kRequestData[] = "requestData"; On 2016/08/11 23:31:31, afakhry wrote: > constexpr Done. https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.cc:22: const char kSetData[] = "feedback.ui.setData"; On 2016/08/11 23:31:31, afakhry wrote: > constexpr Done. https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.h (right): https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.h:13: } On 2016/08/11 23:31:31, afakhry wrote: > We don't need this forward declare here. Please remove. Done. https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.h:24: // WebUIMessageHandler implementation. On 2016/08/11 23:31:31, afakhry wrote: > Recently this parent-class override comment has been shortened to just: > > // WebUIMessageHandler: Done. https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.h:27: // Handlers for JavaScript messages. On 2016/08/11 23:31:32, afakhry wrote: > Hmmm, I'd rather this comment tells us what OnRequestData() actually does. Done. https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.h:30: MdFeedbackUI* md_feedback_ui_; On 2016/08/11 23:31:32, afakhry wrote: > Nit [optional]: > MdFeedbackUI* md_feedback_ui_; // Not owned. Done.
https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_feedback/feedback.js (right): https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_feedback/feedback.js:5: cr.define('feedback', function() { On 2016/08/12 01:46:41, apacible (ooo through 22nd) wrote: > On 2016/08/11 23:31:31, afakhry wrote: > > First, thank you for the detailed comments and the clean code in this file. > I'm > > not against this chrome-style way of defining scoping and objects, however, > the > > more you work with me, you'll find that I'm a big fan of simplicity. I feel > this > > way is a bit hairy (but I understand it is common in our chromium code base). > > But what if we can do better? :) > > > > Compare the code here to the simple one in this gPaste: > > https://paste.googleplex.com/5630126029012992 > > > > Which one do you like looking at and maintaining more? > > I'm not a JS expert at all, that's why I prefer simpler code. So if there's > any > > advantage to this old chromium-style to the one in the gPaste, please let me > > know. > > > > [ > > Note that you'd have to rename your |kSetData| in the cc file to: > > constexpr char kSetData[] = "Feedback.UI.setData"; > > ] > > The snippet is definitely much simpler! > > The style here is the recommended format in the docs: > https://www.chromium.org/developers/webui > > I'm strongly inclined to follow the general style for consistency with the rest > of the chrome/browser/resources/* files (and also possibly because I'm more used > to reading this type of format), but am otherwise unsure of a 'technical > advantage' here. Bear in mind that the webui doc you pointed out is probably too old and hasn't been updated in years! :D So this style was probably needed when we couldn't use the newer, cleaner ES6 class syntax. Now that we can, we should probably consider using it. By the way, I was the first to land a JS class syntax in our codebase. Please see: https://cs.chromium.org/chromium/src/chrome/browser/resources/feedback/js/eve.... As I mentioned earlier, I'm not a JS expert. So in order to decide what we should do here, when you come back, we can consult with dbeam@ about his opinion. Enjoy your vacation! :)
https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_feedback/feedback.js (right): https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_feedback/feedback.js:5: cr.define('feedback', function() { On 2016/08/12 19:22:38, afakhry wrote: > On 2016/08/12 01:46:41, apacible (ooo through 22nd) wrote: > > On 2016/08/11 23:31:31, afakhry wrote: > > > First, thank you for the detailed comments and the clean code in this file. > > I'm > > > not against this chrome-style way of defining scoping and objects, however, > > the > > > more you work with me, you'll find that I'm a big fan of simplicity. I feel > > this > > > way is a bit hairy (but I understand it is common in our chromium code > base). > > > But what if we can do better? :) > > > > > > Compare the code here to the simple one in this gPaste: > > > https://paste.googleplex.com/5630126029012992 > > > > > > Which one do you like looking at and maintaining more? > > > I'm not a JS expert at all, that's why I prefer simpler code. So if there's > > any > > > advantage to this old chromium-style to the one in the gPaste, please let me > > > know. > > > > > > [ > > > Note that you'd have to rename your |kSetData| in the cc file to: > > > constexpr char kSetData[] = "Feedback.UI.setData"; > > > ] > > > > The snippet is definitely much simpler! > > > > The style here is the recommended format in the docs: > > https://www.chromium.org/developers/webui > > > > I'm strongly inclined to follow the general style for consistency with the > rest > > of the chrome/browser/resources/* files (and also possibly because I'm more > used > > to reading this type of format), but am otherwise unsure of a 'technical > > advantage' here. > > Bear in mind that the webui doc you pointed out is probably too old and hasn't > been updated in years! :D > So this style was probably needed when we couldn't use the newer, cleaner ES6 > class syntax. Now that we can, we should probably consider using it. > By the way, I was the first to land a JS class syntax in our codebase. Please > see: > https://cs.chromium.org/chromium/src/chrome/browser/resources/feedback/js/eve.... > > As I mentioned earlier, I'm not a JS expert. So in order to decide what we > should do here, when you come back, we can consult with dbeam@ about his > opinion. > > Enjoy your vacation! :) +dbeam for thoughts (and/or future of WebUI guidelines as well)
Patchset #7 (id:220001) has been deleted
Patchset #6 (id:200001) has been deleted
Patchset #8 (id:280001) has been deleted
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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...
https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_feedback/feedback_container.js (right): https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_feedback/feedback_container.js:41: this.i18n('privacyNote') : ''; On 2016/08/11 23:31:31, afakhry wrote: > I found a way to fix this <strong></strong> not being processed. > > in the HTML file: > > <div><p id="privacyNote"></p></div> > > and here in the JS file: > > ready: function() { > this.$.privacyNote.innerHTML = > loadTimeData.valueExists('privacyNote') ? > loadTimeData.getString('privacyNote') : ''; > } > > You can now safely remove getPrivacyNote_() and behaviors: [I18nBehavior], as > well as the include <link rel="import" > href="chrome://resources/html/i18n_behavior.html"> > > Please build the official chrome-branded version of Chrome to verify that you > see the right string. Done. https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.cc (right): https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.cc:38: } On 2016/08/11 23:31:31, afakhry wrote: > I totally understand your intent and agree with you that this function should be > local here. But that would be valid once we have your MD feedback dialog up and > running and we remove the other one. :) > > Since this is shared code, we can't land duplicate code. So how about this: > > create something like feedback_dialog_utils.h/cc where you add > GetFeedbackTargetTabUrl() and GetFeedbackProfile(). Have your code here as well > as the one in show_feedback_page.cc use these two new functions and then remove > all duplication. How does that sound? Done. Also pulled out GetFeedbackProfile(), but since we don't have a Browser here to get the profile in the same way as show_feedback_page.cc, I used the Profile tied with what's used to create MdFeedbackUI. https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.cc:68: data.SetString("email", profile->GetProfileUserName()); On 2016/08/11 23:31:31, afakhry wrote: > For the email, in the current feedback app we use the SigninManagerFactory to > get its value. Please see: > https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/feedback_p... > > I think we should do the same thing here. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Almost there! :) https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/feedbac... File chrome/browser/feedback/feedback_dialog_utils.cc (right): https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/feedbac... chrome/browser/feedback/feedback_dialog_utils.cc:41: Profile* profile = NULL; nullptr. https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/feedbac... chrome/browser/feedback/feedback_dialog_utils.cc:42: if (browser) { Nit: please remove curly braces from single-line if/else. Actually, it would be better to use a ternary operator here. :) https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/feedbac... chrome/browser/feedback/feedback_dialog_utils.cc:48: return NULL; nullptr. https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/feedbac... chrome/browser/feedback/feedback_dialog_utils.cc:65: : profile; This, however, is more suitable for an if(), since profile should retain its original value if the condition is not satisfied. So: if (display_account_id.is_valid()) profile = multi_user_util::GetProfileFromAccountId(display_account_id); https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/resourc... File chrome/browser/resources/md_feedback/feedback.html (right): https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/resourc... chrome/browser/resources/md_feedback/feedback.html:3: <html> Two <html> tags? https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/resourc... File chrome/browser/resources/md_feedback/feedback.js (right): https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/resourc... chrome/browser/resources/md_feedback/feedback.js:6: 'use strict'; No word from dbeam yet? https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.cc (right): https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.cc:49: // TODO(apacible): Handle multiple profiles on CrOS. I wonder if we really need to worry about this TODO now! The profile you store in MdFeedbackUI seems to be the same one you got from GetFeedbackProfile(), since you used it to call: MdFeedbackDialogController::GetInstance()->Show(profile); Which is then used to show the WebDialog. It seems to me that you already fixed this problem! :) https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.cc:52: profile = profile->GetOriginalProfile(); If the above comment is correct, then probably we don't need this line at all now, since this is already done in GetFeedbackProfile(). Hmm, Looking at it more, maybe it's still needed? Since the profile you store in MdFeedbackUI is the one we get from the WebUI, and it might be different? https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.cc:64: browser->tab_strip_model()->active_index()); Indentation here is wrong. Can you please run `get cl format` to fix it?
https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/md_feedback/feedback.js (right): https://codereview.chromium.org/2190653003/diff/140001/chrome/browser/resourc... chrome/browser/resources/md_feedback/feedback.js:5: cr.define('feedback', function() { On 2016/08/29 17:00:20, apacible wrote: > On 2016/08/12 19:22:38, afakhry wrote: > > On 2016/08/12 01:46:41, apacible (ooo through 22nd) wrote: > > > On 2016/08/11 23:31:31, afakhry wrote: > > > > First, thank you for the detailed comments and the clean code in this > file. > > > I'm > > > > not against this chrome-style way of defining scoping and objects, > however, > > > the > > > > more you work with me, you'll find that I'm a big fan of simplicity. I > feel > > > this > > > > way is a bit hairy (but I understand it is common in our chromium code > > base). > > > > But what if we can do better? :) > > > > > > > > Compare the code here to the simple one in this gPaste: > > > > https://paste.googleplex.com/5630126029012992 > > > > > > > > Which one do you like looking at and maintaining more? > > > > I'm not a JS expert at all, that's why I prefer simpler code. So if > there's > > > any > > > > advantage to this old chromium-style to the one in the gPaste, please let > me > > > > know. > > > > > > > > [ > > > > Note that you'd have to rename your |kSetData| in the cc file to: > > > > constexpr char kSetData[] = "Feedback.UI.setData"; > > > > ] > > > > > > The snippet is definitely much simpler! > > > > > > The style here is the recommended format in the docs: > > > https://www.chromium.org/developers/webui > > > > > > I'm strongly inclined to follow the general style for consistency with the > > rest > > > of the chrome/browser/resources/* files (and also possibly because I'm more > > used > > > to reading this type of format), but am otherwise unsure of a 'technical > > > advantage' here. > > > > Bear in mind that the webui doc you pointed out is probably too old and hasn't > > been updated in years! :D > > So this style was probably needed when we couldn't use the newer, cleaner ES6 > > class syntax. Now that we can, we should probably consider using it. > > By the way, I was the first to land a JS class syntax in our codebase. Please > > see: > > > https://cs.chromium.org/chromium/src/chrome/browser/resources/feedback/js/eve.... > > > > As I mentioned earlier, I'm not a JS expert. So in order to decide what we > > should do here, when you come back, we can consult with dbeam@ about his > > opinion. > > > > Enjoy your vacation! :) > > +dbeam for thoughts (and/or future of WebUI guidelines as well) what do you want my specific thoughts on? 'use strict';? ES6 classes? I don't think the linter currently understands ES6 features, and support is dodgy on iOS (or at least differs from everywhere else because it's basically mobile Safari instead of Chrome). I have a bug to remove the linter somewhere, as it's kinda old and unmaintained. there has been efforts to replace it with clang-format and jscompiler.jar --js_comp_warn=lintChecks, but it's not quite the same. in general: if your code only runs on desktop and you don't care about presubmits preventing you from CQ'ing your changes, feel free to use ES6 features.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by apacible@chromium.org to run a CQ dry run
Patchset #10 (id:340001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/feedbac... File chrome/browser/feedback/feedback_dialog_utils.cc (right): https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/feedbac... chrome/browser/feedback/feedback_dialog_utils.cc:41: Profile* profile = NULL; On 2016/09/14 01:48:15, afakhry wrote: > nullptr. Done. https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/feedbac... chrome/browser/feedback/feedback_dialog_utils.cc:42: if (browser) { On 2016/09/14 01:48:15, afakhry wrote: > Nit: please remove curly braces from single-line if/else. > > Actually, it would be better to use a ternary operator here. :) Done. https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/feedbac... chrome/browser/feedback/feedback_dialog_utils.cc:48: return NULL; On 2016/09/14 01:48:15, afakhry wrote: > nullptr. Done. https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/feedbac... chrome/browser/feedback/feedback_dialog_utils.cc:65: : profile; On 2016/09/14 01:48:15, afakhry wrote: > This, however, is more suitable for an if(), since profile should retain its > original value if the condition is not satisfied. So: > > if (display_account_id.is_valid()) > profile = multi_user_util::GetProfileFromAccountId(display_account_id); Done. https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/resourc... File chrome/browser/resources/md_feedback/feedback.html (right): https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/resourc... chrome/browser/resources/md_feedback/feedback.html:3: <html> On 2016/09/14 01:48:15, afakhry wrote: > Two <html> tags? Removed. https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/resourc... File chrome/browser/resources/md_feedback/feedback.js (right): https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/resourc... chrome/browser/resources/md_feedback/feedback.js:6: 'use strict'; On 2016/09/14 01:48:15, afakhry wrote: > No word from dbeam yet? He commented right after your's last set of comments; I'm switching to your recommended JS snippet. > in general: if your code only runs on desktop and you don't care about presubmits preventing you from CQ'ing your changes, feel free to use ES6 features. https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.cc (right): https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.cc:49: // TODO(apacible): Handle multiple profiles on CrOS. On 2016/09/14 01:48:15, afakhry wrote: > I wonder if we really need to worry about this TODO now! The profile you store > in MdFeedbackUI seems to be the same one you got from GetFeedbackProfile(), > since you used it to call: > > MdFeedbackDialogController::GetInstance()->Show(profile); > > Which is then used to show the WebDialog. > > It seems to me that you already fixed this problem! :) Acknowledged. https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.cc:52: profile = profile->GetOriginalProfile(); On 2016/09/14 01:48:15, afakhry wrote: > If the above comment is correct, then probably we don't need this line at all > now, since this is already done in GetFeedbackProfile(). > > Hmm, Looking at it more, maybe it's still needed? Since the profile you store in > MdFeedbackUI is the one we get from the WebUI, and it might be different? Correct, we still need this. I keep track of the profile when the WebUI is initially created, which could happen in an incognito window. https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_feedback/md_feedback_webui_message_handler.cc:64: browser->tab_strip_model()->active_index()); On 2016/09/14 01:48:15, afakhry wrote: > Indentation here is wrong. Can you please run `get cl format` to fix it? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
Patchset #10 (id:360001) has been deleted
Patchset #10 (id:380001) has been deleted
The CQ bit was checked by apacible@chromium.org to run a CQ dry run
https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/resourc... File chrome/browser/resources/md_feedback/feedback.js (right): https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/resourc... chrome/browser/resources/md_feedback/feedback.js:6: 'use strict'; On 2016/09/14 17:53:20, apacible wrote: > On 2016/09/14 01:48:15, afakhry wrote: > > No word from dbeam yet? > > He commented right after your's last set of comments; I'm switching to your > recommended JS snippet. > > > in general: if your code only runs on desktop and you don't care about > presubmits preventing you from CQ'ing your changes, feel free to use ES6 > features. > Thanks! I hope it doesn't cause you trouble with the presubmit scripts as he mentioned. If it causes you any trouble, feel free to revert back to the old style.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #10 (id:400001) has been deleted
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: + sky@chromium.org
+sky for .gn, .grd OWNERS afakhry: I'll be sending out a CL to fix up some styling so it looks better soon. Also will be chatting with some folks on platform-specific styling and the general direction we're going there. https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/resourc... File chrome/browser/resources/md_feedback/feedback.js (right): https://codereview.chromium.org/2190653003/diff/320001/chrome/browser/resourc... chrome/browser/resources/md_feedback/feedback.js:6: 'use strict'; On 2016/09/14 18:07:37, afakhry wrote: > On 2016/09/14 17:53:20, apacible wrote: > > On 2016/09/14 01:48:15, afakhry wrote: > > > No word from dbeam yet? > > > > He commented right after your's last set of comments; I'm switching to your > > recommended JS snippet. > > > > > in general: if your code only runs on desktop and you don't care about > > presubmits preventing you from CQ'ing your changes, feel free to use ES6 > > features. > > > > Thanks! I hope it doesn't cause you trouble with the presubmit scripts as he > mentioned. > > If it causes you any trouble, feel free to revert back to the old style. Got it working, there were just a few small tweaks to do! :)
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
lgtm! Thanks! :)
The CQ bit was checked by apacible@chromium.org
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
Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by afakhry@chromium.org
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
Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
Description was changed from ========== [Md Feedback] Add initial data population for dialog. This change populates the feedback dialog with some initial info, including profile email and current URL, by adding a new WebUI Message Handler for C++ <-> JS messaging. The handler will be build out to support more messaging in the future, e.g. system info, screenshots, and feedback submission. BUG=632112 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Md Feedback] Add initial data population for dialog. This change populates the feedback dialog with some initial info, including profile email and current URL, by adding a new WebUI Message Handler for C++ <-> JS messaging. The handler will be build out to support more messaging in the future, e.g. system info, screenshots, and feedback submission. BUG=632112 ==========
The CQ bit was checked by apacible@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Removed closure compilation bot to unblock landing this change since we're not being typechecked: third_party/closure_compiler/compiled_resources2.gyp.
On 2016/09/15 20:21:08, apacible wrote: > Removed closure compilation bot to unblock landing this change since we're not > being typechecked: third_party/closure_compiler/compiled_resources2.gyp. it should also be fixed, fyi
On 2016/09/15 20:29:47, Dan Beam wrote: > On 2016/09/15 20:21:08, apacible wrote: > > Removed closure compilation bot to unblock landing this change since we're not > > being typechecked: third_party/closure_compiler/compiled_resources2.gyp. > > it should also be fixed, fyi also, why aren't you being typechecked? ;)
> it should also be fixed, fyi Oh cool, thanks :) > also, why aren't you being typechecked? ;) Soon.. I have a bug open for it!
Message was sent while issue was closed.
Description was changed from ========== [Md Feedback] Add initial data population for dialog. This change populates the feedback dialog with some initial info, including profile email and current URL, by adding a new WebUI Message Handler for C++ <-> JS messaging. The handler will be build out to support more messaging in the future, e.g. system info, screenshots, and feedback submission. BUG=632112 ========== to ========== [Md Feedback] Add initial data population for dialog. This change populates the feedback dialog with some initial info, including profile email and current URL, by adding a new WebUI Message Handler for C++ <-> JS messaging. The handler will be build out to support more messaging in the future, e.g. system info, screenshots, and feedback submission. BUG=632112 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:420001)
Message was sent while issue was closed.
Description was changed from ========== [Md Feedback] Add initial data population for dialog. This change populates the feedback dialog with some initial info, including profile email and current URL, by adding a new WebUI Message Handler for C++ <-> JS messaging. The handler will be build out to support more messaging in the future, e.g. system info, screenshots, and feedback submission. BUG=632112 ========== to ========== [Md Feedback] Add initial data population for dialog. This change populates the feedback dialog with some initial info, including profile email and current URL, by adding a new WebUI Message Handler for C++ <-> JS messaging. The handler will be build out to support more messaging in the future, e.g. system info, screenshots, and feedback submission. BUG=632112 Committed: https://crrev.com/dc2b3be0e417c19e4b2df35594a4ab5bec917dd4 Cr-Commit-Position: refs/heads/master@{#418977} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/dc2b3be0e417c19e4b2df35594a4ab5bec917dd4 Cr-Commit-Position: refs/heads/master@{#418977} |
