|
|
DescriptionMerges two options reset overlay classes
This merges ResetProfileSettingsOverlay and
TriggeredResetProfileSettingsOverlay into a single class.
BUG=636435
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/7410211a6122f9b5a6b4dd22f0a85bbd9dab8ddf
Cr-Commit-Position: refs/heads/master@{#418876}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed dbeam's comments #
Total comments: 2
Patch Set 3 : Fix variable name. #
Messages
Total messages: 30 (20 generated)
Description was changed from ========== Merges two options reset overlay classes This merges ResetProfileSettingsOverlay and TriggeredResetProfileSettingsOverlay into a single class. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation BUG= ========== to ========== Merges two options reset overlay classes This merges ResetProfileSettingsOverlay and TriggeredResetProfileSettingsOverlay into a single class. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by alito@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: Your CL was about to rely on recently removed CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without "master." prefix is no longer supported: tryserver.chromium.linux For more details, see http://crbug.com/617627.
Description was changed from ========== Merges two options reset overlay classes This merges ResetProfileSettingsOverlay and TriggeredResetProfileSettingsOverlay into a single class. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Merges two options reset overlay classes This merges ResetProfileSettingsOverlay and TriggeredResetProfileSettingsOverlay into a single class. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by alito@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: This issue passed the CQ dry run.
alito@chromium.org changed reviewers: + dbeam@chromium.org
Dan, could you PTAL? Thank you!
https://codereview.chromium.org/2334003006/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/options.js (right): https://codereview.chromium.org/2334003006/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/options.js:167: new ResetProfileSettingsOverlay(false), new ResetProfileSettingsOverlay(false /* isTriggered */) https://codereview.chromium.org/2334003006/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2334003006/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/reset_profile_settings_overlay.js:21: function ResetProfileSettingsOverlay(is_triggered) { jsVarsLikeThis https://codereview.chromium.org/2334003006/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/reset_profile_settings_overlay.js:32: } this.isTriggered_ = isTriggered; Page.call( this, isTriggered ? 'triggeredResetProfileSettings' : 'resetProfileSettings', loadTimeData.getString(isTriggered ? 'triggeredResetProfileSettingsOverlay' : 'resetProfileSettingsOverlayTabTitle'), 'reset-profile-settings-overlay'); https://codereview.chromium.org/2334003006/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/reset_profile_settings_overlay.js:44: is_triggered_reset_: false, jsPrivateMembersLikeThis_ https://codereview.chromium.org/2334003006/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/reset_profile_settings_overlay.js:78: loadTimeData.getString('triggeredResetProfileSettingsOverlay'); $('reset-profile-settings-title').textContent = loadTimeData.getString(isTriggered ? 'triggeredResetProfileSettingsOverlay' : 'resetProfileSettingsOverlay'); $('reset-profile-settings-explanation').textContent = loadTimeData.getString(isTriggered ? 'triggeredResetProfileSettingsExplanation' : 'resetProfileSettingsOverlay');
can you add a BUG=? https://codereview.chromium.org/2334003006/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2334003006/diff/20001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:80: 'resetProfileSettingsOverlay'); this needs to be 'resetProfileSettingsExplanation'
Description was changed from ========== Merges two options reset overlay classes This merges ResetProfileSettingsOverlay and TriggeredResetProfileSettingsOverlay into a single class. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Merges two options reset overlay classes This merges ResetProfileSettingsOverlay and TriggeredResetProfileSettingsOverlay into a single class. BUG=636435 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Addressed Dan's comments. https://codereview.chromium.org/2334003006/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/options.js (right): https://codereview.chromium.org/2334003006/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/options.js:167: new ResetProfileSettingsOverlay(false), On 2016/09/14 21:22:37, Dan Beam wrote: > new ResetProfileSettingsOverlay(false /* isTriggered */) Done. https://codereview.chromium.org/2334003006/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2334003006/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/reset_profile_settings_overlay.js:21: function ResetProfileSettingsOverlay(is_triggered) { On 2016/09/14 21:22:37, Dan Beam wrote: > jsVarsLikeThis Done. https://codereview.chromium.org/2334003006/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/reset_profile_settings_overlay.js:32: } On 2016/09/14 21:22:37, Dan Beam wrote: > this.isTriggered_ = isTriggered; > Page.call( > this, > isTriggered ? 'triggeredResetProfileSettings' : 'resetProfileSettings', > loadTimeData.getString(isTriggered ? > 'triggeredResetProfileSettingsOverlay' : > 'resetProfileSettingsOverlayTabTitle'), > 'reset-profile-settings-overlay'); Done. https://codereview.chromium.org/2334003006/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/reset_profile_settings_overlay.js:44: is_triggered_reset_: false, On 2016/09/14 21:22:38, Dan Beam wrote: > jsPrivateMembersLikeThis_ Done. https://codereview.chromium.org/2334003006/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/reset_profile_settings_overlay.js:78: loadTimeData.getString('triggeredResetProfileSettingsOverlay'); On 2016/09/14 21:22:37, Dan Beam wrote: > $('reset-profile-settings-title').textContent = > loadTimeData.getString(isTriggered ? > 'triggeredResetProfileSettingsOverlay' : > 'resetProfileSettingsOverlay'); > $('reset-profile-settings-explanation').textContent = > loadTimeData.getString(isTriggered ? > 'triggeredResetProfileSettingsExplanation' : > 'resetProfileSettingsOverlay'); Done. https://codereview.chromium.org/2334003006/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/reset_profile_settings_overlay.js (right): https://codereview.chromium.org/2334003006/diff/20001/chrome/browser/resource... chrome/browser/resources/options/reset_profile_settings_overlay.js:80: 'resetProfileSettingsOverlay'); On 2016/09/14 21:54:48, Dan Beam wrote: > this needs to be 'resetProfileSettingsExplanation' Done.
The CQ bit was checked by alito@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: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
alito@chromium.org changed reviewers: + robertshield@chromium.org
Adding Robert who wrote the original triggered reset overlay.
lgtm
lgtm
The CQ bit was checked by alito@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.
Description was changed from ========== Merges two options reset overlay classes This merges ResetProfileSettingsOverlay and TriggeredResetProfileSettingsOverlay into a single class. BUG=636435 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Merges two options reset overlay classes This merges ResetProfileSettingsOverlay and TriggeredResetProfileSettingsOverlay into a single class. BUG=636435 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Merges two options reset overlay classes This merges ResetProfileSettingsOverlay and TriggeredResetProfileSettingsOverlay into a single class. BUG=636435 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Merges two options reset overlay classes This merges ResetProfileSettingsOverlay and TriggeredResetProfileSettingsOverlay into a single class. BUG=636435 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/7410211a6122f9b5a6b4dd22f0a85bbd9dab8ddf Cr-Commit-Position: refs/heads/master@{#418876} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7410211a6122f9b5a6b4dd22f0a85bbd9dab8ddf Cr-Commit-Position: refs/heads/master@{#418876} |