Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1845233003:
SiteInstance unittest improvements (Closed)
|
Created:
4 years, 8 months ago by ncarter (slow) Modified:
4 years, 8 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, jam, darin-cc_chromium.org, nasko+codewatch_chromium.org, creis+watch_chromium.org, ajwong+watch_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@doghouse_now Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSiteInstance unittest improvements
- Add a new unittest for GetDefaultSubframeSiteInstance.
- Rewrite tests to not depend on inheritance; BrowsingInstance calls
the ctor of SiteInstanceImpl directly, and this made it hard to
do deletion counters for the defaul subframe siteinstance using
the inheritance tricks.
- Rework delete counters, using existing mechanisms in the API.
- mark SiteInstanceImpl/BrowsingInstance final, since we can.
BUG=594217
Committed: https://crrev.com/a9f3ad7a01940804a4741f02721b459add84a9e8
Cr-Commit-Position: refs/heads/master@{#385639}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Rebase & apply Charlie's suggestions. #Patch Set 3 : NULL -> nullptr #Patch Set 4 : Friend test. #Patch Set 5 : Add missing #include for FRIEND_TEST #
Dependent Patchsets: Messages
Total messages: 21 (11 generated)
Description was changed from ========== SiteInstance unittest fixes: - Add a new unittest for GetDefaultSubframeSiteInstance. - Rewrite tests to not depend on inheritance; BrowsingInstance calls the ctor of SiteInstanceImpl directly, and this made it hard to do deletion counters for the defaul subframe siteinstance using the inheritance tricks. - Rework delete counters, using existing mechanisms in the API. - mark SiteInstanceImpl/BrowsingInstance final, since we can. BUG=594217 ========== to ========== SiteInstance unittest improvements - Add a new unittest for GetDefaultSubframeSiteInstance. - Rewrite tests to not depend on inheritance; BrowsingInstance calls the ctor of SiteInstanceImpl directly, and this made it hard to do deletion counters for the defaul subframe siteinstance using the inheritance tricks. - Rework delete counters, using existing mechanisms in the API. - mark SiteInstanceImpl/BrowsingInstance final, since we can. BUG=594217 ==========
nick@chromium.org changed reviewers: + creis@chromium.org
Charlie, this is conditional on the TDI patch landing, but ready for review anyhow. https://codereview.chromium.org/1845233003/diff/1/content/browser/site_instan... File content/browser/site_instance_impl.h (right): https://codereview.chromium.org/1845233003/diff/1/content/browser/site_instan... content/browser/site_instance_impl.h:132: protected: Do we still need a protected section? I think this is one of those cases where we're pretending that 'protected friends' exist -- ie. friends that can only access protected methods. But C++ doesn't work that way. I understand the idiom, and I'm happy to leave it in place, but in general my rule is 'protected is for inheritance', and friends should be in the private: section. Let me know what you think.
LGTM with nits, assuming the bots are happy with it! https://codereview.chromium.org/1845233003/diff/1/content/browser/site_instan... File content/browser/site_instance_impl.h (right): https://codereview.chromium.org/1845233003/diff/1/content/browser/site_instan... content/browser/site_instance_impl.h:132: protected: On 2016/03/31 16:53:47, ncarter wrote: > Do we still need a protected section? > > I think this is one of those cases where we're pretending that 'protected > friends' exist -- ie. friends that can only access protected methods. But C++ > doesn't work that way. > > I understand the idiom, and I'm happy to leave it in place, but in general my > rule is 'protected is for inheritance', and friends should be in the private: > section. > > Let me know what you think. Ah yes, we should reflect reality. Putting the friend line in the private section makes sense. And yes, now that we're final, we don't need the protected section. https://codereview.chromium.org/1845233003/diff/1/content/browser/site_instan... content/browser/site_instance_impl.h:137: // Create a new SiteInstance. Protected to give access to BrowsingInstance; Guess this will be stale if we remove protected. Let's update. https://codereview.chromium.org/1845233003/diff/1/content/browser/site_instan... content/browser/site_instance_impl.h:147: friend class SiteInstanceTestBrowserClient; nit: Add blank line after. https://codereview.chromium.org/1845233003/diff/1/content/browser/site_instan... File content/browser/site_instance_impl_unittest.cc (right): https://codereview.chromium.org/1845233003/diff/1/content/browser/site_instan... content/browser/site_instance_impl_unittest.cc:65: void SiteInstanceDeleting(content::SiteInstance* site_instance) override { Cool! Nice use of this method, which we didn't have at the time. I suppose we lose the granularity of having a counter per SiteInstance, but your reset approach makes that a bit cleaner. Seems fine. https://codereview.chromium.org/1845233003/diff/1/content/browser/site_instan... content/browser/site_instance_impl_unittest.cc:166: scoped_refptr<SiteInstanceImpl> instance = SiteInstanceImpl::Create(NULL); nit: nullptr while we're at it? (Same below.) https://codereview.chromium.org/1845233003/diff/1/content/browser/site_instan... content/browser/site_instance_impl_unittest.cc:753: switches::kTopDocumentIsolation); Does this need to return early for --site-per-process?
The CQ bit was checked by nick@chromium.org to run a CQ dry run
https://codereview.chromium.org/1845233003/diff/1/content/browser/site_instan... File content/browser/site_instance_impl.h (right): https://codereview.chromium.org/1845233003/diff/1/content/browser/site_instan... content/browser/site_instance_impl.h:132: protected: On 2016/04/01 21:50:41, Charlie Reis (slow til 4-8) wrote: > On 2016/03/31 16:53:47, ncarter wrote: > > Do we still need a protected section? > > > > I think this is one of those cases where we're pretending that 'protected > > friends' exist -- ie. friends that can only access protected methods. But C++ > > doesn't work that way. > > > > I understand the idiom, and I'm happy to leave it in place, but in general my > > rule is 'protected is for inheritance', and friends should be in the private: > > section. > > > > Let me know what you think. > > Ah yes, we should reflect reality. Putting the friend line in the private > section makes sense. And yes, now that we're final, we don't need the protected > section. Done. https://codereview.chromium.org/1845233003/diff/1/content/browser/site_instan... content/browser/site_instance_impl.h:137: // Create a new SiteInstance. Protected to give access to BrowsingInstance; On 2016/04/01 21:50:41, Charlie Reis (slow til 4-8) wrote: > Guess this will be stale if we remove protected. Let's update. Done. https://codereview.chromium.org/1845233003/diff/1/content/browser/site_instan... content/browser/site_instance_impl.h:147: friend class SiteInstanceTestBrowserClient; On 2016/04/01 21:50:41, Charlie Reis (slow til 4-8) wrote: > nit: Add blank line after. Done. https://codereview.chromium.org/1845233003/diff/1/content/browser/site_instan... File content/browser/site_instance_impl_unittest.cc (right): https://codereview.chromium.org/1845233003/diff/1/content/browser/site_instan... content/browser/site_instance_impl_unittest.cc:65: void SiteInstanceDeleting(content::SiteInstance* site_instance) override { On 2016/04/01 21:50:41, Charlie Reis (slow til 4-8) wrote: > Cool! Nice use of this method, which we didn't have at the time. > > I suppose we lose the granularity of having a counter per SiteInstance, but your > reset approach makes that a bit cleaner. Seems fine. Done. https://codereview.chromium.org/1845233003/diff/1/content/browser/site_instan... content/browser/site_instance_impl_unittest.cc:166: scoped_refptr<SiteInstanceImpl> instance = SiteInstanceImpl::Create(NULL); On 2016/04/01 21:50:41, Charlie Reis (slow til 4-8) wrote: > nit: nullptr while we're at it? (Same below.) NULL->nullptr replaced globally in the file. https://codereview.chromium.org/1845233003/diff/1/content/browser/site_instan... content/browser/site_instance_impl_unittest.cc:753: switches::kTopDocumentIsolation); On 2016/04/01 21:50:41, Charlie Reis (slow til 4-8) wrote: > Does this need to return early for --site-per-process? Good catch. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1845233003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1845233003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by nick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/1845233003/#ps60001 (title: "Friend test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1845233003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1845233003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by nick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/1845233003/#ps80001 (title: "Add missing #include for FRIEND_TEST")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1845233003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1845233003/80001
Message was sent while issue was closed.
Description was changed from ========== SiteInstance unittest improvements - Add a new unittest for GetDefaultSubframeSiteInstance. - Rewrite tests to not depend on inheritance; BrowsingInstance calls the ctor of SiteInstanceImpl directly, and this made it hard to do deletion counters for the defaul subframe siteinstance using the inheritance tricks. - Rework delete counters, using existing mechanisms in the API. - mark SiteInstanceImpl/BrowsingInstance final, since we can. BUG=594217 ========== to ========== SiteInstance unittest improvements - Add a new unittest for GetDefaultSubframeSiteInstance. - Rewrite tests to not depend on inheritance; BrowsingInstance calls the ctor of SiteInstanceImpl directly, and this made it hard to do deletion counters for the defaul subframe siteinstance using the inheritance tricks. - Rework delete counters, using existing mechanisms in the API. - mark SiteInstanceImpl/BrowsingInstance final, since we can. BUG=594217 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== SiteInstance unittest improvements - Add a new unittest for GetDefaultSubframeSiteInstance. - Rewrite tests to not depend on inheritance; BrowsingInstance calls the ctor of SiteInstanceImpl directly, and this made it hard to do deletion counters for the defaul subframe siteinstance using the inheritance tricks. - Rework delete counters, using existing mechanisms in the API. - mark SiteInstanceImpl/BrowsingInstance final, since we can. BUG=594217 ========== to ========== SiteInstance unittest improvements - Add a new unittest for GetDefaultSubframeSiteInstance. - Rewrite tests to not depend on inheritance; BrowsingInstance calls the ctor of SiteInstanceImpl directly, and this made it hard to do deletion counters for the defaul subframe siteinstance using the inheritance tricks. - Rework delete counters, using existing mechanisms in the API. - mark SiteInstanceImpl/BrowsingInstance final, since we can. BUG=594217 Committed: https://crrev.com/a9f3ad7a01940804a4741f02721b459add84a9e8 Cr-Commit-Position: refs/heads/master@{#385639} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a9f3ad7a01940804a4741f02721b459add84a9e8 Cr-Commit-Position: refs/heads/master@{#385639} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
