Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2860663003:
Make CWS Report Abuse page the active tab after report abuse and uninstall (Closed)
|
Created:
3 years, 7 months ago by catmullings Modified:
3 years, 7 months ago Reviewers:
Devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake CWS Report Abuse page the active tab after report abuse and uninstall
After a user clicks "Report Abuse" in the extension uninstall dialog, two
webpages may open -- the CWS Report Abuse page and an extension uninstall page,
if the extension specifies a custom uninstall page via
chrome.runtime.setUninstallURL.
Between these two webpages, the extension uninstall page becomes the in-focus,
active tab. Thus users will likely only fill out this form over the CWS Report
Abuse page.
This CL changes the active tab to become the CWS report abuse page instead of
the extension's uninstall page.
BUG=690050
Review-Url: https://codereview.chromium.org/2860663003
Cr-Commit-Position: refs/heads/master@{#473401}
Committed: https://chromium.googlesource.com/chromium/src/+/ff559d943fb20ee2c46571979631296c80aed1d6
Patch Set 1 #Patch Set 2 : Rebase master #
Total comments: 2
Patch Set 3 : Refactor #
Total comments: 6
Patch Set 4 : Added Tests #
Total comments: 21
Patch Set 5 #Patch Set 6 : Polishing test #
Total comments: 2
Patch Set 7 : Addressing last nits #
Messages
Total messages: 43 (29 generated)
Description was changed from ========== After a user clicks "Report Abuse" in the extension uninstall dialog, two webpages may open -- the CWS Report Abuse page and an extension uninstall page, if the extension specifies a custom uninstall page via chrome.runtime.setUninstallURL. Between these two webpages, the extension uninstall page becomes the in-focus, active tab. Thus users will likely only fill out this form over the CWS Report Abuse page. This CL changes the active tab to become the CWS report abuse page instead of the extension's uninstall page. BUG=690050 ========== to ========== Make CWS Report Abuse page the active tab after report abuse and uninstall After a user clicks "Report Abuse" in the extension uninstall dialog, two webpages may open -- the CWS Report Abuse page and an extension uninstall page, if the extension specifies a custom uninstall page via chrome.runtime.setUninstallURL. Between these two webpages, the extension uninstall page becomes the in-focus, active tab. Thus users will likely only fill out this form over the CWS Report Abuse page. This CL changes the active tab to become the CWS report abuse page instead of the extension's uninstall page. BUG=690050 ==========
The CQ bit was checked by catmullings@chromium.org to run a CQ dry run
catmullings@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
https://codereview.chromium.org/2860663003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_uninstall_dialog.cc (right): https://codereview.chromium.org/2860663003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.cc:178: // Fall through. This isn't quite right, because it means that we'll *always* handle the report abuse case in an uninstallation. Instead, I think the right thing to do is use a PostTask() in HandleReportAbuse() to ensure the tab is opened after the extension uninstallation finishes. Also, I don't see trybot runs for this - if this passes all tests, it indicates a gap in our testing that could be addressed. :) In either case, we'll also certainly need a test for the new part (ensuring the active tab in the browser is correct).
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by catmullings@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.
https://codereview.chromium.org/2860663003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_uninstall_dialog.cc (right): https://codereview.chromium.org/2860663003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.cc:178: // Fall through. On 2017/05/03 14:22:37, Devlin wrote: > This isn't quite right, because it means that we'll *always* handle the report > abuse case in an uninstallation. Instead, I think the right thing to do is use > a PostTask() in HandleReportAbuse() to ensure the tab is opened after the > extension uninstallation finishes. > > Also, I don't see trybot runs for this - if this passes all tests, it indicates > a gap in our testing that could be addressed. :) In either case, we'll also > certainly need a test for the new part (ensuring the active tab in the browser > is correct). Documentation: The PostTask(HandleReportAbuse() ... ) approach doesn't work because by the time PostTask is called, the Extension object is destroyed/uninstalled. HandleReportAbuse() calls chrome::Navigate() which tries to get the extension object. Instead of doing the fall through/break approach, the uninstall functionality in CLOSE_ACTION_UNINSTALL is refactored into an Uninstall() function. Then in the REPORT_ABUSE case, Uninstall() is called and then break. The same is done in the CLOSE_ACTION_UNINSTALL case.
On 2017/05/03 23:34:01, catmullings wrote: > https://codereview.chromium.org/2860663003/diff/20001/chrome/browser/extensio... > File chrome/browser/extensions/extension_uninstall_dialog.cc (right): > > https://codereview.chromium.org/2860663003/diff/20001/chrome/browser/extensio... > chrome/browser/extensions/extension_uninstall_dialog.cc:178: // Fall through. > On 2017/05/03 14:22:37, Devlin wrote: > > This isn't quite right, because it means that we'll *always* handle the report > > abuse case in an uninstallation. Instead, I think the right thing to do is > use > > a PostTask() in HandleReportAbuse() to ensure the tab is opened after the > > extension uninstallation finishes. > > > > Also, I don't see trybot runs for this - if this passes all tests, it > indicates > > a gap in our testing that could be addressed. :) In either case, we'll also > > certainly need a test for the new part (ensuring the active tab in the browser > > is correct). > > Documentation: The PostTask(HandleReportAbuse() ... ) approach doesn't work > because by the time PostTask is called, the Extension object is > destroyed/uninstalled. HandleReportAbuse() calls chrome::Navigate() which tries > to get the extension object. > > Instead of doing the fall through/break approach, the uninstall functionality in > CLOSE_ACTION_UNINSTALL is refactored into an Uninstall() function. Then in the > REPORT_ABUSE case, Uninstall() is called and then break. The same is done in the > CLOSE_ACTION_UNINSTALL case. Verified expected behavior on manual tests of the extension uninstall dialog: - Click Uninstall button - Check "Report Abuse", then Click Uninstall button
Nice! We should add a test case for this. https://codereview.chromium.org/2860663003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_uninstall_dialog.cc (right): https://codereview.chromium.org/2860663003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.cc:166: Uninstall(success, error); We should add a comment indicating why this order is important. https://codereview.chromium.org/2860663003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_uninstall_dialog.h (right): https://codereview.chromium.org/2860663003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.h:99: void Uninstall(bool& success, base::string16& error); two notes: - We shouldn't pass POD (like bool) by reference - Arguments passed by reference should be const &; if the value needs to be changed (as in this case), they should be passed by pointer (*).
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by catmullings@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/2860663003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_uninstall_dialog.cc (right): https://codereview.chromium.org/2860663003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.cc:166: Uninstall(success, error); On 2017/05/05 20:46:38, Devlin wrote: > We should add a comment indicating why this order is important. Done. https://codereview.chromium.org/2860663003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_uninstall_dialog.h (right): https://codereview.chromium.org/2860663003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.h:99: void Uninstall(bool& success, base::string16& error); On 2017/05/05 20:46:38, Devlin wrote: > two notes: > - We shouldn't pass POD (like bool) by reference > - Arguments passed by reference should be const &; if the value needs to be > changed (as in this case), they should be passed by pointer (*). What does POD mean?
Nice! Great job on the tests. One other note: this currently only addresses Views platforms (win/linux/cros). That's totally fine for this patch, but we should test mac in a followup to see if it's a problem (and if so, fix it) https://codereview.chromium.org/2860663003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_uninstall_dialog.h (right): https://codereview.chromium.org/2860663003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.h:99: void Uninstall(bool& success, base::string16& error); On 2017/05/15 22:27:55, catmullings wrote: > On 2017/05/05 20:46:38, Devlin wrote: > > two notes: > > - We shouldn't pass POD (like bool) by reference > > - Arguments passed by reference should be const &; if the value needs to be > > changed (as in this case), they should be passed by pointer (*). > > What does POD mean? POD refers to "Plain Old Data", and basically means any raw c-type. These types don't have constructors, destructors, members, methods, etc, and the footprint in memory is roughly the same as a pointer or reference. For instance, an int takes up 32 - 64 bits in memory, so passing it by reference is actually no cheaper than copying the data, since the reference itself also takes up 32 - 64 bits in memory. Additionally, passing by references has drawbacks of potentially mutating it without intending to. As a result, we should only pass POD types (like bool, int, and char) by value or by pointer (by pointer when we expect it to either be null or to be mutated). (Other codebases could also allow by reference in the case that we mutate it, but that's disallowed in chromium.) https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_install_prompt.cc (right): https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_install_prompt.cc:87: case extensions::ScopedTestDialogAutoConfirm::ACCEPT_AND_OPTION: Since this does the same thing as the case above, we can group them: case extensions::ScopedTestDialogAutoConfirm::NONE: return false; // We use PostTask instead of calling the callback directly here, because in // the real implementations it's highly likely the message loop will be // pumping a few times before the user clicks accept or cancel. case extensions::ScopedTestDialogAutoConfirm::ACCEPT: case extensions::ScopedTestDialogAutoConfirm::ACCEPT_AND_OPTION: base::ThreadTaskRunnerHandle::Get()->PostTask( FROM_HERE, base::BindOnce(base::ResetAndReturn(callback), ExtensionInstallPrompt::Result::ACCEPTED)); return true; case extensions::ScopedTestDialogAutoConfirm::CANCEL: https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_uninstall_dialog.h (right): https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_uninstall_dialog.h:98: // Uninstalls the extension. nit: we could make this return a bool for success, which might be a bit cleaner. Something like: // Uninstalls the extension. Returns true on success, and populates |error| on failure. bool Uninstall(base::string16* error); https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:38: return browser->tab_strip_model()->GetActiveWebContents()->GetVisibleURL(); We should use GetLastCommittedURL() (everywhere in this file) https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:141: extensions::ExtensionPrefs::Get(extension_service->GetBrowserContext()), Since we always set the same uninstall url, we could actually just pass a profile and extension id to SetUninstallUrl() to make this code a little shorter: SetUninstallUrl(browser()->profile(), extension->id()); https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:143: extension_service->AddExtension(extension.get()); nit: let's AddExtension() before setting its uninstall url, just since that's more representative of the real flow. https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:171: // Kill parent window. Is this necessary? https://codereview.chromium.org/2860663003/diff/100001/extensions/browser/ext... File extensions/browser/extension_dialog_auto_confirm.h (right): https://codereview.chromium.org/2860663003/diff/100001/extensions/browser/ext... extensions/browser/extension_dialog_auto_confirm.h:18: ACCEPT_AND_OPTION, // The prompt will always check an option and accept. nit: "an option (if any)"
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by catmullings@chromium.org to run a CQ dry run
https://codereview.chromium.org/2860663003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_uninstall_dialog.h (right): https://codereview.chromium.org/2860663003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_uninstall_dialog.h:99: void Uninstall(bool& success, base::string16& error); On 2017/05/15 23:28:04, Devlin (catching up) wrote: > On 2017/05/15 22:27:55, catmullings wrote: > > On 2017/05/05 20:46:38, Devlin wrote: > > > two notes: > > > - We shouldn't pass POD (like bool) by reference > > > - Arguments passed by reference should be const &; if the value needs to be > > > changed (as in this case), they should be passed by pointer (*). > > > > What does POD mean? > > POD refers to "Plain Old Data", and basically means any raw c-type. These types > don't have constructors, destructors, members, methods, etc, and the footprint > in memory is roughly the same as a pointer or reference. For instance, an int > takes up 32 - 64 bits in memory, so passing it by reference is actually no > cheaper than copying the data, since the reference itself also takes up 32 - 64 > bits in memory. Additionally, passing by references has drawbacks of > potentially mutating it without intending to. As a result, we should only pass > POD types (like bool, int, and char) by value or by pointer (by pointer when we > expect it to either be null or to be mutated). (Other codebases could also > allow by reference in the case that we mutate it, but that's disallowed in > chromium.) Done. https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_install_prompt.cc (right): https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_install_prompt.cc:87: case extensions::ScopedTestDialogAutoConfirm::ACCEPT_AND_OPTION: On 2017/05/15 23:28:04, Devlin (catching up) wrote: > Since this does the same thing as the case above, we can group them: > > case extensions::ScopedTestDialogAutoConfirm::NONE: > return false; > // We use PostTask instead of calling the callback directly here, because in > // the real implementations it's highly likely the message loop will be > // pumping a few times before the user clicks accept or cancel. > case extensions::ScopedTestDialogAutoConfirm::ACCEPT: > case extensions::ScopedTestDialogAutoConfirm::ACCEPT_AND_OPTION: > base::ThreadTaskRunnerHandle::Get()->PostTask( > FROM_HERE, base::BindOnce(base::ResetAndReturn(callback), > ExtensionInstallPrompt::Result::ACCEPTED)); > return true; > case extensions::ScopedTestDialogAutoConfirm::CANCEL: Done. https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_uninstall_dialog.h (right): https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_uninstall_dialog.h:98: // Uninstalls the extension. On 2017/05/15 23:28:04, Devlin (catching up) wrote: > nit: we could make this return a bool for success, which might be a bit cleaner. > Something like: > // Uninstalls the extension. Returns true on success, and populates |error| on > failure. > bool Uninstall(base::string16* error); Done. https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:38: return browser->tab_strip_model()->GetActiveWebContents()->GetVisibleURL(); On 2017/05/15 23:28:04, Devlin (catching up) wrote: > We should use GetLastCommittedURL() (everywhere in this file) GetLastCommitedURL returns an empty string when used. https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:141: extensions::ExtensionPrefs::Get(extension_service->GetBrowserContext()), On 2017/05/15 23:28:04, Devlin (catching up) wrote: > Since we always set the same uninstall url, we could actually just pass a > profile and extension id to SetUninstallUrl() to make this code a little > shorter: > SetUninstallUrl(browser()->profile(), extension->id()); Did you mean profile or extension prefs? https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:143: extension_service->AddExtension(extension.get()); On 2017/05/15 23:28:04, Devlin (catching up) wrote: > nit: let's AddExtension() before setting its uninstall url, just since that's > more representative of the real flow. Done. https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:171: // Kill parent window. On 2017/05/15 23:28:04, Devlin (catching up) wrote: > Is this necessary? The comment? Or the code (browser()->window()->Close()) ? The comment is just copy/paste from the previous tests, but I can remove it here. https://codereview.chromium.org/2860663003/diff/100001/extensions/browser/ext... File extensions/browser/extension_dialog_auto_confirm.h (right): https://codereview.chromium.org/2860663003/diff/100001/extensions/browser/ext... extensions/browser/extension_dialog_auto_confirm.h:18: ACCEPT_AND_OPTION, // The prompt will always check an option and accept. On 2017/05/15 23:28:05, Devlin (catching up) wrote: > nit: "an option (if any)" Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
(just responding) https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:38: return browser->tab_strip_model()->GetActiveWebContents()->GetVisibleURL(); On 2017/05/18 18:26:00, catmullings wrote: > On 2017/05/15 23:28:04, Devlin (catching up) wrote: > > We should use GetLastCommittedURL() (everywhere in this file) > > GetLastCommitedURL returns an empty string when used. Ah, I see. We're checking this a bit too soon in the flow. We should wait for the web contents to stop loading before checking the URL. https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:141: extensions::ExtensionPrefs::Get(extension_service->GetBrowserContext()), On 2017/05/18 18:26:00, catmullings wrote: > On 2017/05/15 23:28:04, Devlin (catching up) wrote: > > Since we always set the same uninstall url, we could actually just pass a > > profile and extension id to SetUninstallUrl() to make this code a little > > shorter: > > SetUninstallUrl(browser()->profile(), extension->id()); > > Did you mean profile or extension prefs? I meant profile so that SetUninstallUrl() could get the prefs and avoid duplicating that code, but either is fine. https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:171: // Kill parent window. On 2017/05/18 18:26:00, catmullings wrote: > On 2017/05/15 23:28:04, Devlin (catching up) wrote: > > Is this necessary? > > The comment? Or the code (browser()->window()->Close()) ? > The comment is just copy/paste from the previous tests, but I can remove it > here. The code. The previous tests were checking destruction patterns, so they kill the parent window. This test doesn't need to do that, I think?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_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 catmullings@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/2860663003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:38: return browser->tab_strip_model()->GetActiveWebContents()->GetVisibleURL(); On 2017/05/18 19:14:20, Devlin (catching up) wrote: > On 2017/05/18 18:26:00, catmullings wrote: > > On 2017/05/15 23:28:04, Devlin (catching up) wrote: > > > We should use GetLastCommittedURL() (everywhere in this file) > > > > GetLastCommitedURL returns an empty string when used. > > Ah, I see. We're checking this a bit too soon in the flow. We should wait for > the web contents to stop loading before checking the URL. Done. https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:141: extensions::ExtensionPrefs::Get(extension_service->GetBrowserContext()), On 2017/05/18 19:14:20, Devlin (catching up) wrote: > On 2017/05/18 18:26:00, catmullings wrote: > > On 2017/05/15 23:28:04, Devlin (catching up) wrote: > > > Since we always set the same uninstall url, we could actually just pass a > > > profile and extension id to SetUninstallUrl() to make this code a little > > > shorter: > > > SetUninstallUrl(browser()->profile(), extension->id()); > > > > Did you mean profile or extension prefs? > > I meant profile so that SetUninstallUrl() could get the prefs and avoid > duplicating that code, but either is fine. Sg. I will keep the prefs as a param. https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:141: extensions::ExtensionPrefs::Get(extension_service->GetBrowserContext()), On 2017/05/18 19:14:20, Devlin (catching up) wrote: > On 2017/05/18 18:26:00, catmullings wrote: > > On 2017/05/15 23:28:04, Devlin (catching up) wrote: > > > Since we always set the same uninstall url, we could actually just pass a > > > profile and extension id to SetUninstallUrl() to make this code a little > > > shorter: > > > SetUninstallUrl(browser()->profile(), extension->id()); > > > > Did you mean profile or extension prefs? > > I meant profile so that SetUninstallUrl() could get the prefs and avoid > duplicating that code, but either is fine. Done. https://codereview.chromium.org/2860663003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:171: // Kill parent window. On 2017/05/18 19:14:20, Devlin (catching up) wrote: > On 2017/05/18 18:26:00, catmullings wrote: > > On 2017/05/15 23:28:04, Devlin (catching up) wrote: > > > Is this necessary? > > > > The comment? Or the code (browser()->window()->Close()) ? > > The comment is just copy/paste from the previous tests, but I can remove it > > here. > > The code. The previous tests were checking destruction patterns, so they kill > the parent window. This test doesn't need to do that, I think? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Nice! LGTM https://codereview.chromium.org/2860663003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2860663003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:142: std::string uninstall_url = "https://www.google.com/"; nit: we should pull this up to a const char (near kReferrerId) and use it both here and in SetUninstallURL(). https://codereview.chromium.org/2860663003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc:177: EXPECT_EQ(GetActiveUrl(browser()), uninstall_url); nit: EXPECT_EQ(expected, actual) so: EXPECT_EQ(uninstall_url, GetActiveURL(browser()));
Patchset #7 (id:160001) has been deleted
The CQ bit was checked by catmullings@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2860663003/#ps180001 (title: "Addressing last nits")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1495241788201670,
"parent_rev": "37c906bd514195222ab091fa98a461cc40b25577", "commit_rev":
"ff559d943fb20ee2c46571979631296c80aed1d6"}
Message was sent while issue was closed.
Description was changed from ========== Make CWS Report Abuse page the active tab after report abuse and uninstall After a user clicks "Report Abuse" in the extension uninstall dialog, two webpages may open -- the CWS Report Abuse page and an extension uninstall page, if the extension specifies a custom uninstall page via chrome.runtime.setUninstallURL. Between these two webpages, the extension uninstall page becomes the in-focus, active tab. Thus users will likely only fill out this form over the CWS Report Abuse page. This CL changes the active tab to become the CWS report abuse page instead of the extension's uninstall page. BUG=690050 ========== to ========== Make CWS Report Abuse page the active tab after report abuse and uninstall After a user clicks "Report Abuse" in the extension uninstall dialog, two webpages may open -- the CWS Report Abuse page and an extension uninstall page, if the extension specifies a custom uninstall page via chrome.runtime.setUninstallURL. Between these two webpages, the extension uninstall page becomes the in-focus, active tab. Thus users will likely only fill out this form over the CWS Report Abuse page. This CL changes the active tab to become the CWS report abuse page instead of the extension's uninstall page. BUG=690050 Review-Url: https://codereview.chromium.org/2860663003 Cr-Commit-Position: refs/heads/master@{#473401} Committed: https://chromium.googlesource.com/chromium/src/+/ff559d943fb20ee2c46571979631... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001) as https://chromium.googlesource.com/chromium/src/+/ff559d943fb20ee2c46571979631...
Message was sent while issue was closed.
On 2017/05/20 at 01:43:54, commit-bot wrote: > Committed patchset #7 (id:180001) as https://chromium.googlesource.com/chromium/src/+/ff559d943fb20ee2c46571979631... Note that both tests that were added in this CL were disabled (in https://codereview.chromium.org/2898763003 and https://codereview.chromium.org/2895203002) because of flaky failures. Possibly reverting this CL would have been the better choice. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
