Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2500653002:
MD Settings: Add import data dialog. (Closed)
|
Created:
4 years, 1 month ago by dpapad Modified:
4 years, 1 month ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, dbeam+watch-closure_chromium.org, jlklein+watch-closure_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org, tfarina, vitalyp+closure_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Add import data dialog.
BUG=659259
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/c2a979f3c638e45d6481c68d0cea907d63bb61cb
Cr-Commit-Position: refs/heads/master@{#432268}
Patch Set 1 #Patch Set 2 : More stuff. #Patch Set 3 : Make non-CrOS only #
Total comments: 24
Patch Set 4 : Address comments. #Patch Set 5 : Add comment. #
Total comments: 3
Patch Set 6 : Change to settings-checkbox, use prefs. #Patch Set 7 : Address comment. #
Total comments: 2
Patch Set 8 : Fix bad merge #Messages
Total messages: 44 (20 generated)
Description was changed from ========== MD Settings: Add import data dialog. BUG=659259 ========== to ========== MD Settings: Add import data dialog. BUG=659259 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dpapad@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 dpapad@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...
dpapad@chromium.org changed reviewers: + tommycli@chromium.org
Screenshots of the dialog as well as of the UI surface that triggers the dialog at http://imgur.com/a/WhUEF. The dialog is not fully hooked up to the proxy or the WebUI events yet. Will do that as a follow up as well as add tests when functionality is completed. https://codereview.chromium.org/2500653002/diff/40001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (left): https://codereview.chromium.org/2500653002/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:2154: <if expr="not is_android"> settings_strings.grdp as a whole is skipped on android, so no need for this, see https://cs.chromium.org/chromium/src/chrome/app/generated_resources.grd?q=set.... https://codereview.chromium.org/2500653002/diff/40001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2500653002/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:2152: <if expr="not chromeos"> After examining current Options UI, this UI is non-ChromeOS only, so I've updated the existing code (as well as new code) to reflect that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm sans below comments https://codereview.chromium.org/2500653002/diff/40001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (left): https://codereview.chromium.org/2500653002/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:2154: <if expr="not is_android"> On 2016/11/12 01:49:38, dpapad wrote: > settings_strings.grdp as a whole is skipped on android, so no need for this, see > https://cs.chromium.org/chromium/src/chrome/app/generated_resources.grd?q=set.... Excellent thank you! https://codereview.chromium.org/2500653002/diff/40001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2500653002/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:2152: <if expr="not chromeos"> On 2016/11/12 01:49:38, dpapad wrote: > After examining current Options UI, this UI is non-ChromeOS only, so I've > updated the existing code (as well as new code) to reflect that. Excellent thank you. https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/import_data_dialog.html (right): https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.html:15: } Seems odd that we would need to define a custom style her. You sure this isn't another instance of list-frame and list-items? https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/people... https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/import_data_dialog.js (right): https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.js:31: ready: function() { nit: but when is the earliest that we could do this? Any reason to wait until ready instead of attached? https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.js:48: return this.selected_.index == this.browserProfiles_.length - 1; The last item in the list of browser profiles is always "import from file"? That wasn't obvious to me from the C++ handler... https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.js:62: this.selected_ = this.browserProfiles_[this.$$('select').selectedIndex]; Maybe marginally faster to use event.target.value instead of doing the dom selection again? Also prevents accidentally selecting wrong 'select' element. May be good to change the name to onSelectedBrowserProfileChanged or something
https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/import_data_dialog.html (right): https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.html:38: <paper-checkbox hidden="[[!selected_.history]]"> aren't they persistently stored as prefs somewhere? can we re-use <settings-checkbox pref="..."> or is there something that's preventing us from doing that?
https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/import_data_dialog.html (right): https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.html:38: <paper-checkbox hidden="[[!selected_.history]]"> On 2016/11/14 19:07:20, Dan Beam wrote: > aren't they persistently stored as prefs somewhere? can we re-use > <settings-checkbox pref="..."> or is there something that's preventing us from > doing that? isn't this list persistently stored*
https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/import_data_dialog.html (right): https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.html:15: } On 2016/11/14 at 18:15:57, tommycli wrote: > Seems odd that we would need to define a custom style her. You sure this isn't another instance of list-frame and list-items? > > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/people... Tried using list-frame, list-item. The problem is that list-frame adds 56px of left padding, which looks ok in a subpage (as in the sync_page link above), but within a dialog where there is already a 24px left padding (total 56+24) seems odd, http://imgur.com/a/12Hzv. Also the precedent in dialogs seems to be to not indent checkboxes. Example1: CBD dialog, Example2: Edit certificate authority dialog. https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.html:38: <paper-checkbox hidden="[[!selected_.history]]"> On 2016/11/14 at 19:07:50, Dan Beam wrote: > On 2016/11/14 19:07:20, Dan Beam wrote: > > aren't they persistently stored as prefs somewhere? can we re-use > > <settings-checkbox pref="..."> or is there something that's preventing us from > > doing that? > > isn't this list persistently stored* I don't think there are any prefs related to this UI. All info comes from the browser proxy, and it is sent back through it. Perhaps @tommycli can answer this question more definitely since implemented/migrated the C++ side of it. https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/import_data_dialog.js (right): https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.js:31: ready: function() { On 2016/11/14 at 18:15:57, tommycli wrote: > nit: but when is the earliest that we could do this? Any reason to wait until ready instead of attached? No good reason. Moved to attached. https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.js:48: return this.selected_.index == this.browserProfiles_.length - 1; On 2016/11/14 at 18:15:57, tommycli wrote: > The last item in the list of browser profiles is always "import from file"? That wasn't obvious to me from the C++ handler... Did not look at the C++, but the old JS code implies that, see https://cs.chromium.org/chromium/src/chrome/browser/resources/options/import_.... https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.js:62: this.selected_ = this.browserProfiles_[this.$$('select').selectedIndex]; On 2016/11/14 at 18:15:57, tommycli wrote: > Maybe marginally faster to use event.target.value instead of doing the dom selection again? In order to associate event.target.value with a settings.BrowserProfile I would have to do a linear search within the array, which is avoided by using selectedIndex. > Also prevents accidentally selecting wrong 'select' element. Done. Gave an id to the <select>, to avoid searching the DOM. >May be good to change the name to onSelectedBrowserProfileChanged or something Given that there is only one <select> in the dialog I think |onChange_| naming is sufficient (no other element can fire change event).
thanks! still lgtm. https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/import_data_dialog.html (right): https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.html:15: } On 2016/11/14 20:35:22, dpapad wrote: > On 2016/11/14 at 18:15:57, tommycli wrote: > > Seems odd that we would need to define a custom style her. You sure this isn't > another instance of list-frame and list-items? > > > > > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/people... > > Tried using list-frame, list-item. The problem is that list-frame adds 56px of > left padding, which looks ok in a subpage (as in the sync_page link above), but > within a dialog where there is already a 24px left padding (total 56+24) seems > odd, http://imgur.com/a/12Hzv. > > Also the precedent in dialogs seems to be to not indent checkboxes. > Example1: CBD dialog, > Example2: Edit certificate authority dialog. Hmm.. if the only problem is the 24px left padding, maybe just override that left padding with another rule. I leave it to your best judgement. Either way is okay with me. I always thought it was very lame that list-frame adds 56px of padding (since it's not within a settings-box). I would have preferred it to just add 32px of padding and be within (an invisible) settings-box. - but that's outside the scope of this patch. https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.html:38: <paper-checkbox hidden="[[!selected_.history]]"> On 2016/11/14 20:35:22, dpapad wrote: > On 2016/11/14 at 19:07:50, Dan Beam wrote: > > On 2016/11/14 19:07:20, Dan Beam wrote: > > > aren't they persistently stored as prefs somewhere? can we re-use > > > <settings-checkbox pref="..."> or is there something that's preventing us > from > > > doing that? > > > > isn't this list persistently stored* > > I don't think there are any prefs related to this UI. All info comes from the > browser proxy, and it is sent back through it. Perhaps @tommycli can answer this > question more definitely since implemented/migrated the C++ side of it. The Old Options code does seem to indicate some interaction with prefs: https://cs.chromium.org/chromium/src/chrome/browser/resources/options/import_... That can be in a followup CL. I didn't get a chance to test this, but try opening two instances of this dialog in old options in different tabs and see if they synchronize. https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/import_data_dialog.js (right): https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.js:48: return this.selected_.index == this.browserProfiles_.length - 1; On 2016/11/14 20:35:22, dpapad wrote: > On 2016/11/14 at 18:15:57, tommycli wrote: > > The last item in the list of browser profiles is always "import from file"? > That wasn't obvious to me from the C++ handler... > > Did not look at the C++, but the old JS code implies that, see > https://cs.chromium.org/chromium/src/chrome/browser/resources/options/import_.... Hmm interesting. Maybe investigate and add a comment for future devs, since this seems pretty non-obvious as to why.
https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/import_data_dialog.html (right): https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.html:15: } > Hmm.. if the only problem is the 24px left padding, maybe just override that left padding with another rule. I leave it to your best judgement. Either way is okay with me. > > I always thought it was very lame that list-frame adds 56px of padding (since it's not within a settings-box). I would have preferred it to just add 32px of padding and be within (an invisible) settings-box. - but that's outside the scope of this patch. I'll leave as-is for now, and will make sure that @bettes takes a look at this dialog before we consider it polished. https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.html:38: <paper-checkbox hidden="[[!selected_.history]]"> On 2016/11/14 at 20:45:34, tommycli wrote: > On 2016/11/14 20:35:22, dpapad wrote: > > On 2016/11/14 at 19:07:50, Dan Beam wrote: > > > On 2016/11/14 19:07:20, Dan Beam wrote: > > > > aren't they persistently stored as prefs somewhere? can we re-use > > > > <settings-checkbox pref="..."> or is there something that's preventing us > > from > > > > doing that? > > > > > > isn't this list persistently stored* > > > > I don't think there are any prefs related to this UI. All info comes from the > > browser proxy, and it is sent back through it. Perhaps @tommycli can answer this > > question more definitely since implemented/migrated the C++ side of it. > > The Old Options code does seem to indicate some interaction with prefs: > https://cs.chromium.org/chromium/src/chrome/browser/resources/options/import_... > > That can be in a followup CL. I didn't get a chance to test this, but try opening two instances of this dialog in old options in different tabs and see if they synchronize. Double checked with old Options, and the checkboxes do synchronize across tabs. So will address this in follow up. https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/import_data_dialog.js (right): https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.js:48: return this.selected_.index == this.browserProfiles_.length - 1; On 2016/11/14 at 20:45:34, tommycli wrote: > On 2016/11/14 20:35:22, dpapad wrote: > > On 2016/11/14 at 18:15:57, tommycli wrote: > > > The last item in the list of browser profiles is always "import from file"? > > That wasn't obvious to me from the C++ handler... > > > > Did not look at the C++, but the old JS code implies that, see > > https://cs.chromium.org/chromium/src/chrome/browser/resources/options/import_.... > > Hmm interesting. Maybe investigate and add a comment for future devs, since this seems pretty non-obvious as to why. Done. The answer is that the last browser profile will always be about importing straight from a bookmarks file, see [1] and [2]. [1] https://cs.chromium.org/chromium/src/chrome/browser/importer/importer_list.cc... [2] https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/setting...
awesome thank you!
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org Link to the patchset: https://codereview.chromium.org/2500653002/#ps80001 (title: "Add comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/import_data_dialog.html (right): https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.html:38: <paper-checkbox hidden="[[!selected_.history]]"> On 2016/11/14 21:47:57, dpapad wrote: > On 2016/11/14 at 20:45:34, tommycli wrote: > > On 2016/11/14 20:35:22, dpapad wrote: > > > On 2016/11/14 at 19:07:50, Dan Beam wrote: > > > > On 2016/11/14 19:07:20, Dan Beam wrote: > > > > > aren't they persistently stored as prefs somewhere? can we re-use > > > > > <settings-checkbox pref="..."> or is there something that's preventing > us > > > from > > > > > doing that? > > > > > > > > isn't this list persistently stored* > > > > > > I don't think there are any prefs related to this UI. All info comes from > the > > > browser proxy, and it is sent back through it. Perhaps @tommycli can answer > this > > > question more definitely since implemented/migrated the C++ side of it. > > > > The Old Options code does seem to indicate some interaction with prefs: > > > https://cs.chromium.org/chromium/src/chrome/browser/resources/options/import_... > > > > That can be in a followup CL. I didn't get a chance to test this, but try > opening two instances of this dialog in old options in different tabs and see if > they synchronize. > > Double checked with old Options, and the checkboxes do synchronize across tabs. > So will address this in follow up. yeah, all the pref magic in options is done in JS and HTML
https://codereview.chromium.org/2500653002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/import_data_dialog.js (right): https://codereview.chromium.org/2500653002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.js:19: selected_: Object, can we change this from a set of booleans to just prefs?
The CQ bit was unchecked by dpapad@chromium.org
https://codereview.chromium.org/2500653002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/import_data_dialog.js (right): https://codereview.chromium.org/2500653002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.js:19: selected_: Object, On 2016/11/14 at 22:06:51, Dan Beam wrote: > can we change this from a set of booleans to just prefs? My plan was to investigate the prefs involved in follow up CL, to address the checkboxes syncing across tabs. I am not familiar with the C++ side of things yet. Goal of this CL is mainly to add the dummy dialog, not hooked up to anything, and not functional yet. Do you mean, in this CL? Or is it OK to investigate your suggestion for follow up? FWIW, in this CL I relied on using the proxy as it was landed at https://chromium.googlesource.com/chromium/src/+/4a4963ec66964b69df3c677384f6....
https://codereview.chromium.org/2500653002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/import_data_dialog.js (right): https://codereview.chromium.org/2500653002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.js:19: selected_: Object, On 2016/11/14 at 22:19:30, dpapad wrote: > On 2016/11/14 at 22:06:51, Dan Beam wrote: > > can we change this from a set of booleans to just prefs? > > My plan was to investigate the prefs involved in follow up CL, to address the checkboxes syncing across tabs. I am not familiar with the C++ side of things yet. Goal of this CL is mainly to add the dummy dialog, not hooked up to anything, and not functional yet. > > Do you mean, in this CL? Or is it OK to investigate your suggestion for follow up? FWIW, in this CL I relied on using the proxy as it was landed at https://chromium.googlesource.com/chromium/src/+/4a4963ec66964b69df3c677384f6.... Done, I added this in this CL. Checkboxes are now syncing across tabs. Regarding the contents of selected_, the booleans are not same as the prefs. These booleans depict whether the pref is relevant for the selected browser (for example the "Import from HTML file" case has all booleans set to false except one (not prefs).
lgtm https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/import_data_dialog.html (right): https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.html:20: height: 40px; can we use a var() here for the height (and preferrably use min-height)?
https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/import_data_dialog.html (right): https://codereview.chromium.org/2500653002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/import_data_dialog.html:20: height: 40px; On 2016/11/15 at 01:27:46, Dan Beam wrote: > can we use a var() here for the height (and preferrably use min-height)? Done.
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2500653002/#ps120001 (title: "Address comment.")
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: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2500653002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc (right): https://codereview.chromium.org/2500653002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:1778: AddImportDataStrings(html_source); remove this FAILED: [snip] ../../chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc: In function 'void settings::AddLocalizedStrings(content::WebUIDataSource*, Profile*)': ../../chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:1786:35: error: 'AddImportDataStrings' was not declared in this scope AddImportDataStrings(html_source); ^
https://codereview.chromium.org/2500653002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc (right): https://codereview.chromium.org/2500653002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:1778: AddImportDataStrings(html_source); On 2016/11/15 at 03:41:00, Dan Beam wrote: > remove this > > FAILED: [snip] > ../../chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc: In function 'void settings::AddLocalizedStrings(content::WebUIDataSource*, Profile*)': > ../../chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:1786:35: error: 'AddImportDataStrings' was not declared in this scope > AddImportDataStrings(html_source); > ^ Fixed. I forgot to remove this line when I resolved conflicts with https://codereview.chromium.org/2497113002.
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2500653002/#ps140001 (title: "Fix bad merge")
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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dpapad@chromium.org
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.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Add import data dialog. BUG=659259 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Add import data dialog. BUG=659259 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/c2a979f3c638e45d6481c68d0cea907d63bb61cb Cr-Commit-Position: refs/heads/master@{#432268} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/c2a979f3c638e45d6481c68d0cea907d63bb61cb Cr-Commit-Position: refs/heads/master@{#432268} |
