Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2309443002:
NoState Prefetch: Refactor prerender_browsertests.cc (Closed)
DescriptionNoState Prefetch: Refactor prerender_browsertests.cc
This refactors prerender_browsertests in preparation for adding nostate prefetch
tests. The idea is to make the nostate prefetch tests independent from the
current prerender tests in order to make deprecation & removal of prerender as
easy as possible. prerender_test_utils.* contain code extracted from
prerender_browsertest.cc with only syntatic changes.
See crrev.com/2304953002 for nostate prefetch browser tests.
This does not change any functionality.
BUG=643652
Committed: https://crrev.com/215218973a66366ffa99d1bcfddbb4cd8113e089
Cr-Commit-Position: refs/heads/master@{#417220}
Patch Set 1 #Patch Set 2 : clean up split from nostate tests #Patch Set 3 : Remove unnecessary diff #
Total comments: 7
Patch Set 4 : comments/rebase #
Total comments: 16
Patch Set 5 : comments/format #Patch Set 6 : Comment #
Messages
Total messages: 27 (10 generated)
Description was changed from ========== NoState Prefetch: Refactor prerender_browsertests.cc This refactors prerender_browsertests in preparation for adding nostate prefetch tests. The idea is to make the nostate prefetch tests independent from the current prerender tests in order to make deprecation & removal of prerender as easy as possible. See crrev.com/XXX for nostate prefetch browser tests. This does not change any functionality. BUG=643652 ========== to ========== NoState Prefetch: Refactor prerender_browsertests.cc This refactors prerender_browsertests in preparation for adding nostate prefetch tests. The idea is to make the nostate prefetch tests independent from the current prerender tests in order to make deprecation & removal of prerender as easy as possible. prerender_test_utils.* contain code extracted from prerender_browsertest.cc with only syntatic changes. See crrev.com/XXX for nostate prefetch browser tests. This does not change any functionality. BUG=643652 ==========
Description was changed from ========== NoState Prefetch: Refactor prerender_browsertests.cc This refactors prerender_browsertests in preparation for adding nostate prefetch tests. The idea is to make the nostate prefetch tests independent from the current prerender tests in order to make deprecation & removal of prerender as easy as possible. prerender_test_utils.* contain code extracted from prerender_browsertest.cc with only syntatic changes. See crrev.com/XXX for nostate prefetch browser tests. This does not change any functionality. BUG=643652 ========== to ========== NoState Prefetch: Refactor prerender_browsertests.cc This refactors prerender_browsertests in preparation for adding nostate prefetch tests. The idea is to make the nostate prefetch tests independent from the current prerender tests in order to make deprecation & removal of prerender as easy as possible. prerender_test_utils.* contain code extracted from prerender_browsertest.cc with only syntatic changes. See crrev.com/2304953002 for nostate prefetch browser tests. This does not change any functionality. BUG=643652 ==========
mattcary@chromium.org changed reviewers: + droger@chromium.org, pasko@chromium.org
Egor and David: The first CL for browser tests in nostate prefetch. Note that I created a new bug for the tests as I didn't find an existing one that looked relevant. Thx Matt
looks good, most important issue is inlining functions in the .h https://codereview.chromium.org/2309443002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_test_utils.cc (right): https://codereview.chromium.org/2309443002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_test_utils.cc:34: namespace { nit: blank line after this. I believe the recommended style is to be symetrical: if you leave a blank space before closing the namespace, you must leave one after opening it. https://codereview.chromium.org/2309443002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_test_utils.h (right): https://codereview.chromium.org/2309443002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_test_utils.h:273: return std::unique_ptr<TestPrerender>(prerenders[0]); Inlining is discouraged in Chromium, although I'm not 100% sure about test code. You did not hit any presubmit warning for this? See http://www.chromium.org/developers/coding-style/cpp-dos-and-donts https://codereview.chromium.org/2309443002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_test_utils.h:297: virtual ScopedVector<TestPrerender> PrerenderTestURLImpl( nit: looks like this could be private.
https://codereview.chromium.org/2309443002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_test_utils.cc (right): https://codereview.chromium.org/2309443002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_test_utils.cc:34: namespace { On 2016/09/02 14:33:54, droger wrote: > nit: blank line after this. > > I believe the recommended style is to be symetrical: if you leave a blank space > before closing the namespace, you must leave one after opening it. Done. https://codereview.chromium.org/2309443002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_test_utils.h (right): https://codereview.chromium.org/2309443002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_test_utils.h:273: return std::unique_ptr<TestPrerender>(prerenders[0]); On 2016/09/02 14:33:54, droger wrote: > Inlining is discouraged in Chromium, although I'm not 100% sure about test code. > You did not hit any presubmit warning for this? > > See http://www.chromium.org/developers/coding-style/cpp-dos-and-donts I did hit lint warnings for other ftns, there seemed to be a complexity threshold for when it started to complain about inline functions (for example, the getters/setters below). So I didn't know if the idea was to inline the simple things. I'll move these larger PrerenderTestURLImpl wrappers to the cc, though, thanks. https://codereview.chromium.org/2309443002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_test_utils.h:297: virtual ScopedVector<TestPrerender> PrerenderTestURLImpl( On 2016/09/02 14:33:54, droger wrote: > nit: looks like this could be private. It needs to be protected to be overridden by subclasses, no?
https://codereview.chromium.org/2309443002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_test_utils.h (right): https://codereview.chromium.org/2309443002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_test_utils.h:297: virtual ScopedVector<TestPrerender> PrerenderTestURLImpl( On 2016/09/02 15:02:07, mattcary wrote: > On 2016/09/02 14:33:54, droger wrote: > > nit: looks like this could be private. > > It needs to be protected to be overridden by subclasses, no? No. A virtual private function can be overriden by subclasses. It cannot be called though.
On 2016/09/02 15:05:27, droger wrote: > https://codereview.chromium.org/2309443002/diff/40001/chrome/browser/prerende... > File chrome/browser/prerender/prerender_test_utils.h (right): > > https://codereview.chromium.org/2309443002/diff/40001/chrome/browser/prerende... > chrome/browser/prerender/prerender_test_utils.h:297: virtual > ScopedVector<TestPrerender> PrerenderTestURLImpl( > On 2016/09/02 15:02:07, mattcary wrote: > > On 2016/09/02 14:33:54, droger wrote: > > > nit: looks like this could be private. > > > > It needs to be protected to be overridden by subclasses, no? > > No. > > A virtual private function can be overriden by subclasses. > It cannot be called though. Oh, cool. I didn't know that.
lgtm
overall seems like the most workable approach I can think of, thanks! Before signing off, one concern about code copying, plus a few questions/nits. https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_browsertest.cc:960: prerender_contents_factory()->ExpectPrerenderContents( this is doing very similar things to oStatePrefetchBrowserTest::PrerenderTestURLImpl(), at least in the first half of this method. Would it be possible to extract the common parts in the ancestor of both? https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_test_utils.cc (right): https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_test_utils.cc:490: .release(&prerenders); is this how git cl format performs the formatting? I'm still learning the patterns it uses... https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_test_utils.h (right): https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_test_utils.h:17: #include "net/url_request/url_request_filter.h" is this needed to be included in the .h file? https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_test_utils.h:218: class PrerenderInProcessBrowserTest : virtual public InProcessBrowserTest { do you know why virtual inheritance is used here? https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_test_utils.h:268: void set_browser(Browser* browser) { nit: fits on one line https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_test_utils.h:277: virtual ScopedVector<TestPrerender> PrerenderTestURLImpl( What does this method do? Why having an "Impl" and "non-Impl" methods at the same time?
https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_browsertest.cc:960: prerender_contents_factory()->ExpectPrerenderContents( On 2016/09/05 13:46:27, pasko wrote: > this is doing very similar things to > oStatePrefetchBrowserTest::PrerenderTestURLImpl(), at least in the first half of > this method. Would it be possible to extract the common parts in the ancestor of > both? At this point, I don't think it is possible to extract the common bits. There's some loader path and such modification that I don't think will be necessary for NoStatePrefetchBrowserTest. If there are more bits in common, I'll put them in the other CL (which isn't finished yet). https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_test_utils.cc (right): https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_test_utils.cc:490: .release(&prerenders); On 2016/09/05 13:46:27, pasko wrote: > is this how git cl format performs the formatting? I'm still learning the > patterns it uses... I've run git cl format. Probably this is clang-format (see https://chromium.googlesource.com/chromium/src/+/master/docs/emacs.md#Use-Goo..., I guess that's not accurate...) https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_test_utils.h (right): https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_test_utils.h:17: #include "net/url_request/url_request_filter.h" On 2016/09/05 13:46:28, pasko wrote: > is this needed to be included in the .h file? Thanks, I had the counting interceptor methods in the .h file before the build scolded me about inline functions. https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_test_utils.h:218: class PrerenderInProcessBrowserTest : virtual public InProcessBrowserTest { On 2016/09/05 13:46:28, pasko wrote: > do you know why virtual inheritance is used here? Maybe. Some other uses of InProcessBrowserTest I've seen are non-virtual. In prerender_browsertest, there are is a subclass that inherits from another test classes (ExtensionApiTest). I suspect that's why (both the test classes inherit from the common test base class and we don't want to duplicate that instance), but I didn't investigate completely. https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_test_utils.h:268: void set_browser(Browser* browser) { On 2016/09/05 13:46:28, pasko wrote: > nit: fits on one line Done (git cl format run). https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_test_utils.h:277: virtual ScopedVector<TestPrerender> PrerenderTestURLImpl( On 2016/09/05 13:46:28, pasko wrote: > What does this method do? Why having an "Impl" and "non-Impl" methods at the > same time? This is copied from the existing browser test. The non-Impl methods are thin wrappers that do parameter canonicalization. I'm not sure why it was written this way rather than having a main PrerenderTestUrl with these parameters, but this is actually a lot more convenient for test subclassing as it nicely breaks out the method that needs to be subclassed independently from the different flavors of invocation.
lgtm with adding a comment for the abstract method in the base test class https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_browsertest.cc:960: prerender_contents_factory()->ExpectPrerenderContents( On 2016/09/06 08:06:20, mattcary wrote: > On 2016/09/05 13:46:27, pasko wrote: > > this is doing very similar things to > > oStatePrefetchBrowserTest::PrerenderTestURLImpl(), at least in the first half > of > > this method. Would it be possible to extract the common parts in the ancestor > of > > both? > > At this point, I don't think it is possible to extract the common bits. There's > some loader path and such modification that I don't think will be necessary for > NoStatePrefetchBrowserTest. If there are more bits in common, I'll put them in > the other CL (which isn't finished yet). OK, let's address the question of code sharing in that other change then. I am still concerned with mostly-same-but-subtly-different pieces of code spreading, even if they are just 68 lines long. https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_test_utils.cc (right): https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_test_utils.cc:490: .release(&prerenders); On 2016/09/06 08:06:20, mattcary wrote: > On 2016/09/05 13:46:27, pasko wrote: > > is this how git cl format performs the formatting? I'm still learning the > > patterns it uses... > > I've run git cl format. Probably this is clang-format (see > https://chromium.googlesource.com/chromium/src/+/master/docs/emacs.md#Use-Goo..., > I guess that's not accurate...) Yes, it is our flavor of clang-format https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_test_utils.h (right): https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_test_utils.h:218: class PrerenderInProcessBrowserTest : virtual public InProcessBrowserTest { On 2016/09/06 08:06:20, mattcary wrote: > On 2016/09/05 13:46:28, pasko wrote: > > do you know why virtual inheritance is used here? > > Maybe. Some other uses of InProcessBrowserTest I've seen are non-virtual. In > prerender_browsertest, there are is a subclass that inherits from another test > classes (ExtensionApiTest). I suspect that's why (both the test classes inherit > from the common test base class and we don't want to duplicate that instance), > but I didn't investigate completely. Acknowledged. https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_test_utils.h:277: virtual ScopedVector<TestPrerender> PrerenderTestURLImpl( On 2016/09/06 08:06:20, mattcary wrote: > On 2016/09/05 13:46:28, pasko wrote: > > What does this method do? Why having an "Impl" and "non-Impl" methods at the > > same time? > > This is copied from the existing browser test. > > The non-Impl methods are thin wrappers that do parameter canonicalization. I'm > not sure why it was written this way rather than having a main PrerenderTestUrl > with these parameters, but this is actually a lot more convenient for test > subclassing as it nicely breaks out the method that needs to be subclassed > independently from the different flavors of invocation. Ah, now I understand. It looked weird to have an abstract method named with a suffix "Impl"... I would still suggest to write a comment to say what the method is doing, since now it is supposed to be overridden at 2 points. Inheritance with huge classes is already confusing, let's try to make it a little clearer.
On 2016/09/06 14:40:56, pasko wrote: > lgtm with adding a comment for the abstract method in the base test class > > https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... > File chrome/browser/prerender/prerender_browsertest.cc (right): > > https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... > chrome/browser/prerender/prerender_browsertest.cc:960: > prerender_contents_factory()->ExpectPrerenderContents( > On 2016/09/06 08:06:20, mattcary wrote: > > On 2016/09/05 13:46:27, pasko wrote: > > > this is doing very similar things to > > > oStatePrefetchBrowserTest::PrerenderTestURLImpl(), at least in the first > half > > of > > > this method. Would it be possible to extract the common parts in the > ancestor > > of > > > both? > > > > At this point, I don't think it is possible to extract the common bits. > There's > > some loader path and such modification that I don't think will be necessary > for > > NoStatePrefetchBrowserTest. If there are more bits in common, I'll put them in > > the other CL (which isn't finished yet). > > OK, let's address the question of code sharing in that other change then. I am > still concerned with mostly-same-but-subtly-different pieces of code spreading, > even if they are just 68 lines long. > > https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... > File chrome/browser/prerender/prerender_test_utils.cc (right): > > https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... > chrome/browser/prerender/prerender_test_utils.cc:490: .release(&prerenders); > On 2016/09/06 08:06:20, mattcary wrote: > > On 2016/09/05 13:46:27, pasko wrote: > > > is this how git cl format performs the formatting? I'm still learning the > > > patterns it uses... > > > > I've run git cl format. Probably this is clang-format (see > > > https://chromium.googlesource.com/chromium/src/+/master/docs/emacs.md#Use-Goo..., > > I guess that's not accurate...) > > Yes, it is our flavor of clang-format > > https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... > File chrome/browser/prerender/prerender_test_utils.h (right): > > https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... > chrome/browser/prerender/prerender_test_utils.h:218: class > PrerenderInProcessBrowserTest : virtual public InProcessBrowserTest { > On 2016/09/06 08:06:20, mattcary wrote: > > On 2016/09/05 13:46:28, pasko wrote: > > > do you know why virtual inheritance is used here? > > > > Maybe. Some other uses of InProcessBrowserTest I've seen are non-virtual. In > > prerender_browsertest, there are is a subclass that inherits from another test > > classes (ExtensionApiTest). I suspect that's why (both the test classes > inherit > > from the common test base class and we don't want to duplicate that instance), > > but I didn't investigate completely. > > Acknowledged. > > https://codereview.chromium.org/2309443002/diff/60001/chrome/browser/prerende... > chrome/browser/prerender/prerender_test_utils.h:277: virtual > ScopedVector<TestPrerender> PrerenderTestURLImpl( > On 2016/09/06 08:06:20, mattcary wrote: > > On 2016/09/05 13:46:28, pasko wrote: > > > What does this method do? Why having an "Impl" and "non-Impl" methods at the > > > same time? > > > > This is copied from the existing browser test. > > > > The non-Impl methods are thin wrappers that do parameter canonicalization. I'm > > not sure why it was written this way rather than having a main > PrerenderTestUrl > > with these parameters, but this is actually a lot more convenient for test > > subclassing as it nicely breaks out the method that needs to be subclassed > > independently from the different flavors of invocation. > > Ah, now I understand. It looked weird to have an abstract method named with a > suffix "Impl"... > > I would still suggest to write a comment to say what the method is doing, since > now it is supposed to be overridden at 2 points. Inheritance with huge classes > is already confusing, let's try to make it a little clearer. Done
The CQ bit was checked by mattcary@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org, pasko@chromium.org Link to the patchset: https://codereview.chromium.org/2309443002/#ps100001 (title: "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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
mattcary@chromium.org changed reviewers: + mmenke@chromium.org
+mmenke for owner review
On 2016/09/07 12:01:47, mattcary wrote: > +mmenke for owner review LGTM, rubberstamp, deferring to pasko.
The CQ bit was checked by mattcary@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 ========== NoState Prefetch: Refactor prerender_browsertests.cc This refactors prerender_browsertests in preparation for adding nostate prefetch tests. The idea is to make the nostate prefetch tests independent from the current prerender tests in order to make deprecation & removal of prerender as easy as possible. prerender_test_utils.* contain code extracted from prerender_browsertest.cc with only syntatic changes. See crrev.com/2304953002 for nostate prefetch browser tests. This does not change any functionality. BUG=643652 ========== to ========== NoState Prefetch: Refactor prerender_browsertests.cc This refactors prerender_browsertests in preparation for adding nostate prefetch tests. The idea is to make the nostate prefetch tests independent from the current prerender tests in order to make deprecation & removal of prerender as easy as possible. prerender_test_utils.* contain code extracted from prerender_browsertest.cc with only syntatic changes. See crrev.com/2304953002 for nostate prefetch browser tests. This does not change any functionality. BUG=643652 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== NoState Prefetch: Refactor prerender_browsertests.cc This refactors prerender_browsertests in preparation for adding nostate prefetch tests. The idea is to make the nostate prefetch tests independent from the current prerender tests in order to make deprecation & removal of prerender as easy as possible. prerender_test_utils.* contain code extracted from prerender_browsertest.cc with only syntatic changes. See crrev.com/2304953002 for nostate prefetch browser tests. This does not change any functionality. BUG=643652 ========== to ========== NoState Prefetch: Refactor prerender_browsertests.cc This refactors prerender_browsertests in preparation for adding nostate prefetch tests. The idea is to make the nostate prefetch tests independent from the current prerender tests in order to make deprecation & removal of prerender as easy as possible. prerender_test_utils.* contain code extracted from prerender_browsertest.cc with only syntatic changes. See crrev.com/2304953002 for nostate prefetch browser tests. This does not change any functionality. BUG=643652 Committed: https://crrev.com/215218973a66366ffa99d1bcfddbb4cd8113e089 Cr-Commit-Position: refs/heads/master@{#417220} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/215218973a66366ffa99d1bcfddbb4cd8113e089 Cr-Commit-Position: refs/heads/master@{#417220} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
