Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2630243003:
MacViews: Enable views based Download recovery dialog behind secondary-ui-md flag. (Closed)
|
Created:
3 years, 11 months ago by karandeepb Modified:
3 years, 11 months ago CC:
chromium-reviews, asanka, tfarina, dbeam+watch-downloads_chromium.org, mac-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Enable views based Download Recovery dialog behind secondary-ui-md flag.
This CL puts the views based Download Recovery dialog behind the secondary-ui-md
flag. As a result, the views based version is shown with the flag "secondary-ui-
md" enabled and the Cocoa version is shown without it.
This CL also does some clean-up in download_danger_prompt_browsertest.cc and
parameterizes the test fixture to test it both with and without the secondary-
ui-md flag.
BUG=662128, 654142
Review-Url: https://codereview.chromium.org/2630243003
Cr-Commit-Position: refs/heads/master@{#445995}
Committed: https://chromium.googlesource.com/chromium/src/+/5fba2166ddb709508371545bdb88a9c87929615d
Patch Set 1 #Patch Set 2 : -- #Patch Set 3 : Second approach #Patch Set 4 : Also test with secondary-ui-md. #Patch Set 5 : -- #
Total comments: 2
Patch Set 6 : Cleanup #
Total comments: 4
Patch Set 7 : Rebase and Nit #
Total comments: 10
Patch Set 8 : Address review #
Total comments: 2
Patch Set 9 : Correct method comment. #
Messages
Total messages: 45 (32 generated)
Description was changed from ========== -- -- BUG= ========== to ========== -- -- BUG= ==========
The CQ bit was checked by karandeepb@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.
The CQ bit was checked by karandeepb@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.
The CQ bit was checked by karandeepb@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...
Description was changed from ========== -- -- BUG= ========== to ========== MacViews: Enable views based Download Recovery dialog behind secondary-ui-md flag. This CL puts the views based Download Recovery dialog behind the secondary-ui-md flag. As a result, the views based version is shown with the flag "secondary-ui- md" enabled and the Cocoa version is shown without it. BUG=662128, 654142 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #6 (id:100001) has been deleted
Description was changed from ========== MacViews: Enable views based Download Recovery dialog behind secondary-ui-md flag. This CL puts the views based Download Recovery dialog behind the secondary-ui-md flag. As a result, the views based version is shown with the flag "secondary-ui- md" enabled and the Cocoa version is shown without it. BUG=662128, 654142 ========== to ========== MacViews: Enable views based Download Recovery dialog behind secondary-ui-md flag. This CL puts the views based Download Recovery dialog behind the secondary-ui-md flag. As a result, the views based version is shown with the flag "secondary-ui- md" enabled and the Cocoa version is shown without it. This CL also does some clean-up in download_danger_prompt_browsertest.cc and parameterizes the test fixture to test it both with and without the secondary- ui-md flag. BUG=662128, 654142 ==========
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by karandeepb@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...
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent. Attached screenshot on linked bug. On views, Cancel is the default action and on Cocoa, Keep anyway is. There seems to be a long discussion about this here - https://docs.google.com/document/d/1M9AvvXafVRSNquKGhzjAqo3Pl7sSATuEECA78pfkp.... https://codereview.chromium.org/2630243003/diff/120001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt.h (right): https://codereview.chromium.org/2630243003/diff/120001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.h:71: static DownloadDangerPrompt* CreateDownloadDangerPromptViews( Should this be in a #ifdef (TOOLKIT_VIEWS)? This file is not included on android. Also, I see some functions in browser_dialogs.h which are declared on android but not defined. There is no linker error because they are not called on android. Is this considered ok? https://codereview.chromium.org/2630243003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_danger_prompt_views.cc (right): https://codereview.chromium.org/2630243003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_danger_prompt_views.cc:329: #if !defined(OS_MACOSX) || BUILDFLAG(MAC_VIEWS_BROWSER) maybe we can have a #define COCOA_BROWSER #defined(OS_MACOSX) && !BUILDFLAG(MAC_VIEWS_BROWSER) somwhere, as this pattern becomes more common.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Can you please hold the review Trent.
PTAL Trent. Please also see my comments with the previous patchset. https://codereview.chromium.org/2630243003/diff/140001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt.h (right): https://codereview.chromium.org/2630243003/diff/140001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.h:71: static DownloadDangerPrompt* CreateDownloadDangerPromptViews( I would prefer a function in a namespace to a static one. Keeping static here based on your comments in the review for External protocol dialog conversion. https://codereview.chromium.org/2630243003/diff/140001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt_browsertest.cc (right): https://codereview.chromium.org/2630243003/diff/140001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:338: INSTANTIATE_TEST_CASE_P(, Is it ok to test both with and without secondary-ui-md for non-Mac platforms? https://codereview.chromium.org/2630243003/diff/160001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt_browsertest.cc (right): https://codereview.chromium.org/2630243003/diff/160001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:93: command_line->AppendSwitch(switches::kExtendMdToSecondaryUi); Tried using MaterialDesignControllerTestApi but it didn't work. It's difficult to hook into the point MaterialDesignController is initialized for a browser test.
lgtm with the following https://codereview.chromium.org/2630243003/diff/140001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt.h (right): https://codereview.chromium.org/2630243003/diff/140001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.h:71: static DownloadDangerPrompt* CreateDownloadDangerPromptViews( On 2017/01/24 09:17:21, karandeepb wrote: > I would prefer a function in a namespace to a static one. Keeping static here > based on your comments in the review for External protocol dialog conversion. Acknowledged. https://codereview.chromium.org/2630243003/diff/140001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt_browsertest.cc (right): https://codereview.chromium.org/2630243003/diff/140001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:338: INSTANTIATE_TEST_CASE_P(, On 2017/01/24 09:17:21, karandeepb wrote: > Is it ok to test both with and without secondary-ui-md for non-Mac platforms? yeah - there's some ripple effects and things that could do with extra test coverage. https://codereview.chromium.org/2630243003/diff/160001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt_browsertest.cc (right): https://codereview.chromium.org/2630243003/diff/160001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:49: // Functor to generate the test name suffix depending on the value of the Can this be a regular function? See e.g. https://codereview.chromium.org/2394123002/diff/370001/ui/views/controls/menu... https://codereview.chromium.org/2630243003/diff/160001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:75: test_safe_browsing_factory_(new TestSafeBrowsingServiceFactory()) {} nit: (since you're changing it), I think the preference is for base::MakeUnique<TestSafeBrowsingServiceFactory>() https://codereview.chromium.org/2630243003/diff/160001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:90: // TODO(crbug.com/630357): Remove when secondary-ui-md is enabled by default I don't think we need the TODO. When that happens, switches::kExtendMdToSecondaryUi will disappear, so it will "automatically" get cleaned up. But! What might be missed is that the whole WithParamInterface can go too. So perhaps update the TODO to mention that. Then when someone gets a compile error here, hopefully they see the TODO and remember to remove WithParamInterface as well as just delete the SetUpCommandLine override. https://codereview.chromium.org/2630243003/diff/160001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:93: command_line->AppendSwitch(switches::kExtendMdToSecondaryUi); On 2017/01/24 09:17:21, karandeepb wrote: > Tried using MaterialDesignControllerTestApi but it didn't work. It's difficult > to hook into the point MaterialDesignController is initialized for a browser > test. This is good - that's how I've been doing it https://codereview.chromium.org/2630243003/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/download/download_danger_prompt_impl.cc (right): https://codereview.chromium.org/2630243003/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/download/download_danger_prompt_impl.cc:260: if (ui::MaterialDesignController::IsSecondaryUiMaterial()) nit: needs curlies since the block is over two lines
https://codereview.chromium.org/2630243003/diff/160001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt_browsertest.cc (right): https://codereview.chromium.org/2630243003/diff/160001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:49: // Functor to generate the test name suffix depending on the value of the On 2017/01/25 00:32:17, tapted wrote: > Can this be a regular function? See e.g. > https://codereview.chromium.org/2394123002/diff/370001/ui/views/controls/menu... Done. https://codereview.chromium.org/2630243003/diff/160001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:75: test_safe_browsing_factory_(new TestSafeBrowsingServiceFactory()) {} On 2017/01/25 00:32:17, tapted wrote: > nit: (since you're changing it), I think the preference is for > > base::MakeUnique<TestSafeBrowsingServiceFactory>() Done. https://codereview.chromium.org/2630243003/diff/160001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt_browsertest.cc:90: // TODO(crbug.com/630357): Remove when secondary-ui-md is enabled by default On 2017/01/25 00:32:17, tapted wrote: > I don't think we need the TODO. When that happens, > switches::kExtendMdToSecondaryUi will disappear, so it will "automatically" get > cleaned up. > > But! What might be missed is that the whole WithParamInterface can go too. So > perhaps update the TODO to mention that. Then when someone gets a compile error > here, hopefully they see the TODO and remember to remove WithParamInterface as > well as just delete the SetUpCommandLine override. Done. https://codereview.chromium.org/2630243003/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/download/download_danger_prompt_impl.cc (right): https://codereview.chromium.org/2630243003/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/download/download_danger_prompt_impl.cc:260: if (ui::MaterialDesignController::IsSecondaryUiMaterial()) On 2017/01/25 00:32:17, tapted wrote: > nit: needs curlies since the block is over two lines Done.
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2630243003/#ps180001 (title: "Address review")
The CQ bit was unchecked by karandeepb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
karandeepb@chromium.org changed reviewers: + sky@chromium.org
PTAL sky@ for chrome/browser owner's review.
LGTM https://codereview.chromium.org/2630243003/diff/180001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt.h (right): https://codereview.chromium.org/2630243003/diff/180001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.h:75: const OnDone& done); Document what done is.
https://codereview.chromium.org/2630243003/diff/180001/chrome/browser/downloa... File chrome/browser/download/download_danger_prompt.h (right): https://codereview.chromium.org/2630243003/diff/180001/chrome/browser/downloa... chrome/browser/download/download_danger_prompt.h:75: const OnDone& done); On 2017/01/25 04:45:44, sky wrote: > Document what done is. Have documented it near DownloadDangerPrompt::Create(). Seems https://codereview.chromium.org/16924017 changed the method signature without updating its comment. Have corrected that.
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2630243003/#ps200001 (title: "Correct method 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: 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 karandeepb@chromium.org
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": 200001, "attempt_start_ts": 1485340434450170,
"parent_rev": "14ab21ed2e07a78a0b4af6b8a09ff5de06b6d33d", "commit_rev":
"5fba2166ddb709508371545bdb88a9c87929615d"}
Message was sent while issue was closed.
Description was changed from ========== MacViews: Enable views based Download Recovery dialog behind secondary-ui-md flag. This CL puts the views based Download Recovery dialog behind the secondary-ui-md flag. As a result, the views based version is shown with the flag "secondary-ui- md" enabled and the Cocoa version is shown without it. This CL also does some clean-up in download_danger_prompt_browsertest.cc and parameterizes the test fixture to test it both with and without the secondary- ui-md flag. BUG=662128, 654142 ========== to ========== MacViews: Enable views based Download Recovery dialog behind secondary-ui-md flag. This CL puts the views based Download Recovery dialog behind the secondary-ui-md flag. As a result, the views based version is shown with the flag "secondary-ui- md" enabled and the Cocoa version is shown without it. This CL also does some clean-up in download_danger_prompt_browsertest.cc and parameterizes the test fixture to test it both with and without the secondary- ui-md flag. BUG=662128, 654142 Review-Url: https://codereview.chromium.org/2630243003 Cr-Commit-Position: refs/heads/master@{#445995} Committed: https://chromium.googlesource.com/chromium/src/+/5fba2166ddb709508371545bdb88... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:200001) as https://chromium.googlesource.com/chromium/src/+/5fba2166ddb709508371545bdb88... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
