Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2266653002:
Previews infobar tests (Closed)
DescriptionPreviews infobar tests
Unit tests for both the PreviewsInfoBarDelegate and
PreviewsInfoBarTabHelper. Verfies that the infobar shows
with the correct strings and only shows once for a page.
BUG=615566
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/c38aa26f9df508479d57a93c8db3dd424541b869
Cr-Commit-Position: refs/heads/master@{#418617}
Patch Set 1 #
Total comments: 4
Patch Set 2 : clamy comments #
Total comments: 8
Patch Set 3 : bengr comments #Patch Set 4 : rebase #
Total comments: 10
Patch Set 5 : format tests #Patch Set 6 : bengr comments #Depends on Patchset: Messages
Total messages: 55 (37 generated)
Description was changed from ========== WIP infobar working tests, need to add uma check BUG= ========== to ========== WIP infobar working tests, need to add uma check BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== WIP infobar working tests, need to add uma check BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Preview infobar tests Unit tests for both the PreviewsInfoBarDelegate and PreviewsInfoBarTabHelper. Verfies that the infobar shows with the correct strings and only shows once for a page. BUG=615566 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Preview infobar tests Unit tests for both the PreviewsInfoBarDelegate and PreviewsInfoBarTabHelper. Verfies that the infobar shows with the correct strings and only shows once for a page. BUG=615566 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Previews infobar tests Unit tests for both the PreviewsInfoBarDelegate and PreviewsInfoBarTabHelper. Verfies that the infobar shows with the correct strings and only shows once for a page. BUG=615566 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
megjablon@chromium.org changed reviewers: + bengr@chromium.org
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
megjablon@chromium.org changed reviewers: + avi@chromium.org
+avi for content/*
avi@chromium.org changed reviewers: + clamy@chromium.org, nasko@chromium.org - avi@chromium.org
NavHandle -> Nasko/Camille.
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Thanks! A few comments on the content/ public interface. https://codereview.chromium.org/2266653002/diff/140001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/2266653002/diff/140001/content/public/browser... content/public/browser/navigation_handle.h:216: bool same_page, I only see one instance of CallDidCommitNavigationForTesting being used in this patch, in which |same_page| is set to false. If this is indeed the case, can we remove this parameter? https://codereview.chromium.org/2266653002/diff/140001/content/public/browser... content/public/browser/navigation_handle.h:217: RenderFrameHost* render_frame_host) = 0; It would seem less error prone to not have a RFH there, in particular in light of this patch https://codereview.chromium.org/2321543002/ since the one that commits the navigation is supposed to be the one that is inside the NavigationHandle.
https://chromiumcodereview.appspot.com/2266653002/diff/140001/content/public/... File content/public/browser/navigation_handle.h (right): https://chromiumcodereview.appspot.com/2266653002/diff/140001/content/public/... content/public/browser/navigation_handle.h:216: bool same_page, On 2016/09/08 13:11:07, clamy wrote: > I only see one instance of CallDidCommitNavigationForTesting being used in this > patch, in which |same_page| is set to false. If this is indeed the case, can we > remove this parameter? Removed. https://chromiumcodereview.appspot.com/2266653002/diff/140001/content/public/... content/public/browser/navigation_handle.h:217: RenderFrameHost* render_frame_host) = 0; On 2016/09/08 13:11:07, clamy wrote: > It would seem less error prone to not have a RFH there, in particular in light > of this patch https://codereview.chromium.org/2321543002/ since the one that > commits the navigation is supposed to be the one that is inside the > NavigationHandle. Good call. Done.
Thanks! content/ lgtm.
megjablon@chromium.org changed reviewers: + battre@chromium.org
battre: subresource_filter
https://codereview.chromium.org/2266653002/diff/160001/chrome/browser/preview... File chrome/browser/previews/previews_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/2266653002/diff/160001/chrome/browser/preview... chrome/browser/previews/previews_infobar_delegate_unittest.cc:44: enum PreviewsInfoBarAction { This is defined elsewhere, too right? Why can't you increase the visibility of that definition to these tests? https://codereview.chromium.org/2266653002/diff/160001/chrome/browser/preview... File chrome/browser/previews/previews_infobar_tab_helper_unittest.cc (right): https://codereview.chromium.org/2266653002/diff/160001/chrome/browser/preview... chrome/browser/previews/previews_infobar_tab_helper_unittest.cc:23: nit: can remove blank lines above and below. https://codereview.chromium.org/2266653002/diff/160001/chrome/browser/preview... chrome/browser/previews/previews_infobar_tab_helper_unittest.cc:71: std::string(data_reduction_proxy::chrome_proxy_header()) + ": " + I'd use StringPrintF in base to construct the string. https://codereview.chromium.org/2266653002/diff/160001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc (right): https://codereview.chromium.org/2266653002/diff/160001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:119: main_rfh(), scoped_refptr<net::HttpResponseHeaders>()); Is there any way to do this without taking on a net dependency?
Patchset #3 (id:180001) has been deleted
https://chromiumcodereview.appspot.com/2266653002/diff/160001/chrome/browser/... File chrome/browser/previews/previews_infobar_delegate_unittest.cc (right): https://chromiumcodereview.appspot.com/2266653002/diff/160001/chrome/browser/... chrome/browser/previews/previews_infobar_delegate_unittest.cc:44: enum PreviewsInfoBarAction { On 2016/09/09 19:40:52, bengr wrote: > This is defined elsewhere, too right? Why can't you increase the visibility of > that definition to these tests? Done. https://chromiumcodereview.appspot.com/2266653002/diff/160001/chrome/browser/... File chrome/browser/previews/previews_infobar_tab_helper_unittest.cc (right): https://chromiumcodereview.appspot.com/2266653002/diff/160001/chrome/browser/... chrome/browser/previews/previews_infobar_tab_helper_unittest.cc:23: On 2016/09/09 19:40:52, bengr wrote: > nit: can remove blank lines above and below. Done. https://chromiumcodereview.appspot.com/2266653002/diff/160001/chrome/browser/... chrome/browser/previews/previews_infobar_tab_helper_unittest.cc:71: std::string(data_reduction_proxy::chrome_proxy_header()) + ": " + On 2016/09/09 19:40:52, bengr wrote: > I'd use StringPrintF in base to construct the string. Done. https://chromiumcodereview.appspot.com/2266653002/diff/160001/components/subr... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc (right): https://chromiumcodereview.appspot.com/2266653002/diff/160001/components/subr... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:119: main_rfh(), scoped_refptr<net::HttpResponseHeaders>()); On 2016/09/09 19:40:52, bengr wrote: > Is there any way to do this without taking on a net dependency? Passing in strings and then creating the headers in the handle.
On 2016/09/09 20:38:41, megjablon wrote: > https://chromiumcodereview.appspot.com/2266653002/diff/160001/chrome/browser/... > File chrome/browser/previews/previews_infobar_delegate_unittest.cc (right): > > https://chromiumcodereview.appspot.com/2266653002/diff/160001/chrome/browser/... > chrome/browser/previews/previews_infobar_delegate_unittest.cc:44: enum > PreviewsInfoBarAction { > On 2016/09/09 19:40:52, bengr wrote: > > This is defined elsewhere, too right? Why can't you increase the visibility of > > that definition to these tests? > > Done. > > https://chromiumcodereview.appspot.com/2266653002/diff/160001/chrome/browser/... > File chrome/browser/previews/previews_infobar_tab_helper_unittest.cc (right): > > https://chromiumcodereview.appspot.com/2266653002/diff/160001/chrome/browser/... > chrome/browser/previews/previews_infobar_tab_helper_unittest.cc:23: > On 2016/09/09 19:40:52, bengr wrote: > > nit: can remove blank lines above and below. > > Done. > > https://chromiumcodereview.appspot.com/2266653002/diff/160001/chrome/browser/... > chrome/browser/previews/previews_infobar_tab_helper_unittest.cc:71: > std::string(data_reduction_proxy::chrome_proxy_header()) + ": " + > On 2016/09/09 19:40:52, bengr wrote: > > I'd use StringPrintF in base to construct the string. > > Done. > > https://chromiumcodereview.appspot.com/2266653002/diff/160001/components/subr... > File > components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc > (right): > > https://chromiumcodereview.appspot.com/2266653002/diff/160001/components/subr... > components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:119: > main_rfh(), scoped_refptr<net::HttpResponseHeaders>()); > On 2016/09/09 19:40:52, bengr wrote: > > Is there any way to do this without taking on a net dependency? > > Passing in strings and then creating the headers in the handle. A friendly ping
megjablon@chromium.org changed reviewers: - battre@chromium.org
megjablon@chromium.org changed reviewers: + battre@chromium.org, engedy@chromium.org
+engedy: subresource_filter in place of battre
The CQ bit was checked by megjablon@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...
lgtm with nits https://codereview.chromium.org/2266653002/diff/220001/chrome/browser/preview... File chrome/browser/previews/previews_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/2266653002/diff/220001/chrome/browser/preview... chrome/browser/previews/previews_infobar_delegate_unittest.cc:97: std::unique_ptr<data_reduction_proxy::DataReductionProxyTestContext> #include <memory> https://codereview.chromium.org/2266653002/diff/220001/chrome/browser/preview... File chrome/browser/previews/previews_infobar_tab_helper_unittest.cc (right): https://codereview.chromium.org/2266653002/diff/220001/chrome/browser/preview... chrome/browser/previews/previews_infobar_tab_helper_unittest.cc:66: std::string headers = base::StringPrintf( #include <string> https://codereview.chromium.org/2266653002/diff/220001/chrome/browser/preview... chrome/browser/previews/previews_infobar_tab_helper_unittest.cc:81: std::unique_ptr<content::NavigationHandle> test_handle_; #include <memory> https://codereview.chromium.org/2266653002/diff/220001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc (right): https://codereview.chromium.org/2266653002/diff/220001/components/subresource... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:116: handle()->CallWillProcessResponseForTesting(main_rfh(), std::string()); #include <string> https://codereview.chromium.org/2266653002/diff/220001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2266653002/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:121: const std::string& raw_response_header) override; #include <string>
The CQ bit was checked by megjablon@chromium.org to run a CQ dry run
https://chromiumcodereview.appspot.com/2266653002/diff/220001/chrome/browser/... File chrome/browser/previews/previews_infobar_delegate_unittest.cc (right): https://chromiumcodereview.appspot.com/2266653002/diff/220001/chrome/browser/... chrome/browser/previews/previews_infobar_delegate_unittest.cc:97: std::unique_ptr<data_reduction_proxy::DataReductionProxyTestContext> On 2016/09/13 23:18:18, bengr wrote: > #include <memory> Done. https://chromiumcodereview.appspot.com/2266653002/diff/220001/chrome/browser/... File chrome/browser/previews/previews_infobar_tab_helper_unittest.cc (right): https://chromiumcodereview.appspot.com/2266653002/diff/220001/chrome/browser/... chrome/browser/previews/previews_infobar_tab_helper_unittest.cc:66: std::string headers = base::StringPrintf( On 2016/09/13 23:18:18, bengr wrote: > #include <string> Done. https://chromiumcodereview.appspot.com/2266653002/diff/220001/chrome/browser/... chrome/browser/previews/previews_infobar_tab_helper_unittest.cc:81: std::unique_ptr<content::NavigationHandle> test_handle_; On 2016/09/13 23:18:18, bengr wrote: > #include <memory> Done. https://chromiumcodereview.appspot.com/2266653002/diff/220001/components/subr... File components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc (right): https://chromiumcodereview.appspot.com/2266653002/diff/220001/components/subr... components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc:116: handle()->CallWillProcessResponseForTesting(main_rfh(), std::string()); On 2016/09/13 23:18:18, bengr wrote: > #include <string> Done. https://chromiumcodereview.appspot.com/2266653002/diff/220001/content/browser... File content/browser/frame_host/navigation_handle_impl.h (right): https://chromiumcodereview.appspot.com/2266653002/diff/220001/content/browser... content/browser/frame_host/navigation_handle_impl.h:121: const std::string& raw_response_header) override; On 2016/09/13 23:18:18, bengr wrote: > #include <string> Done.
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 ========== Previews infobar tests Unit tests for both the PreviewsInfoBarDelegate and PreviewsInfoBarTabHelper. Verfies that the infobar shows with the correct strings and only shows once for a page. BUG=615566 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Previews infobar tests Unit tests for both the PreviewsInfoBarDelegate and PreviewsInfoBarTabHelper. Verfies that the infobar shows with the correct strings and only shows once for a page. TBR=engedy@chromium.org BUG=615566 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Previews infobar tests Unit tests for both the PreviewsInfoBarDelegate and PreviewsInfoBarTabHelper. Verfies that the infobar shows with the correct strings and only shows once for a page. TBR=engedy@chromium.org BUG=615566 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Previews infobar tests Unit tests for both the PreviewsInfoBarDelegate and PreviewsInfoBarTabHelper. Verfies that the infobar shows with the correct strings and only shows once for a page. BUG=615566 TBR=engedy@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Previews infobar tests Unit tests for both the PreviewsInfoBarDelegate and PreviewsInfoBarTabHelper. Verfies that the infobar shows with the correct strings and only shows once for a page. BUG=615566 TBR=engedy@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Previews infobar tests Unit tests for both the PreviewsInfoBarDelegate and PreviewsInfoBarTabHelper. Verfies that the infobar shows with the correct strings and only shows once for a page. TBR=engedy@chromium.org BUG=615566 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
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_...)
components/subresource_filter LGTM.
engedy@chromium.org changed reviewers: - battre@chromium.org
The CQ bit was checked by megjablon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org, bengr@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2266653002/#ps260001 (title: "bengr comments")
The CQ bit was unchecked by megjablon@chromium.org
The CQ bit was checked by megjablon@chromium.org
The CQ bit was unchecked by megjablon@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2258283002 Patch 80001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
Description was changed from ========== Previews infobar tests Unit tests for both the PreviewsInfoBarDelegate and PreviewsInfoBarTabHelper. Verfies that the infobar shows with the correct strings and only shows once for a page. TBR=engedy@chromium.org BUG=615566 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Previews infobar tests Unit tests for both the PreviewsInfoBarDelegate and PreviewsInfoBarTabHelper. Verfies that the infobar shows with the correct strings and only shows once for a page. BUG=615566 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by megjablon@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 ========== Previews infobar tests Unit tests for both the PreviewsInfoBarDelegate and PreviewsInfoBarTabHelper. Verfies that the infobar shows with the correct strings and only shows once for a page. BUG=615566 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Previews infobar tests Unit tests for both the PreviewsInfoBarDelegate and PreviewsInfoBarTabHelper. Verfies that the infobar shows with the correct strings and only shows once for a page. BUG=615566 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #6 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Previews infobar tests Unit tests for both the PreviewsInfoBarDelegate and PreviewsInfoBarTabHelper. Verfies that the infobar shows with the correct strings and only shows once for a page. BUG=615566 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Previews infobar tests Unit tests for both the PreviewsInfoBarDelegate and PreviewsInfoBarTabHelper. Verfies that the infobar shows with the correct strings and only shows once for a page. BUG=615566 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/c38aa26f9df508479d57a93c8db3dd424541b869 Cr-Commit-Position: refs/heads/master@{#418617} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c38aa26f9df508479d57a93c8db3dd424541b869 Cr-Commit-Position: refs/heads/master@{#418617} |
