Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9270018:
Make a separate histogram for MatchComplete Final Status'es and (Closed)
|
Created:
8 years, 11 months ago by tburkard Modified:
8 years, 11 months ago CC:
chromium-reviews, tburkard+watch_chromium.org, cbentzel+watch_chromium.org, dominich+watch_chromium.org, mmenke Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionMake a separate histogram for MatchComplete Final Status'es and
normal Final Statuses.
Also, show all control group Final Statuses, since we keep them now
in a separate histogram.
R=cbentzel@chromium.org, dominich@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=118935
Patch Set 1 #
Total comments: 6
Patch Set 2 : '' #
Total comments: 41
Patch Set 3 : '' #
Total comments: 18
Patch Set 4 : '' #
Total comments: 2
Patch Set 5 : '' #
Total comments: 21
Patch Set 6 : '' #
Total comments: 1
Patch Set 7 : '' #Messages
Total messages: 22 (0 generated)
Please review carefully. This should help us to better reconcile the difference between Control, normal, and MatchComplete.
I need to look at this more in depth, but I'm completely confused about the different groups and statuses by now. I'm wondering if we want to take a step back and consider which histograms we're actually interested in and restrict the code to those instead of having every possible permutation. Timo, can you write up exactly what MatchComplete vs Match is somewhere? And all the FirstAfterMiss variants. https://chromiumcodereview.appspot.com/9270018/diff/1/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.cc (right): https://chromiumcodereview.appspot.com/9270018/diff/1/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.cc:568: if (!prerender_contents->prerendering_has_started()) { I'm a bit puzzled why this is a different check to IsControlGroup() https://chromiumcodereview.appspot.com/9270018/diff/1/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.cc:578: // TODO: record MatchComplete status correctly. If the current addition of the mc_status doesn't allow this to be tracked correctly, maybe we need to consider an alternative solution for MatchComplete. https://chromiumcodereview.appspot.com/9270018/diff/1/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.cc:1111: FinalStatus final_status) const { if mc_status is MC_DEFAULT this will be recorded in two histograms. Is that expected?
Dominic, thanks for your feedback -- this was very helpful. I implemented the TODOs which I had completely forgotten to take care of. I also added the two elaborate explanations of what the respective items are, as you requested. Please take another look. Thanks. https://chromiumcodereview.appspot.com/9270018/diff/1/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.cc (right): https://chromiumcodereview.appspot.com/9270018/diff/1/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.cc:568: if (!prerender_contents->prerendering_has_started()) { This is necessary to also cover e.g. the case of a MatchComplete dummy. prerendering_has_started() is the easiest check to cover all these cases. It is also more robust in case one day, the field trial could change on the fly. On 2012/01/20 21:17:08, dominich wrote: > I'm a bit puzzled why this is a different check to IsControlGroup() https://chromiumcodereview.appspot.com/9270018/diff/1/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.cc:578: // TODO: record MatchComplete status correctly. It does suffice.. All that's needed is exercising it. I updated that in this latest revision, using a new helper function that conveniently takes care of this at all three places that I marked as TODO. On 2012/01/20 21:17:08, dominich wrote: > If the current addition of the mc_status doesn't allow this to be tracked > correctly, maybe we need to consider an alternative solution for MatchComplete. https://chromiumcodereview.appspot.com/9270018/diff/1/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.cc:1111: FinalStatus final_status) const { Yes. There will be 2 histograms for the Prerender gorups only: The actual FinalStatus'es as perceived by the user, and the MatchComplete FinalStatus'es, that should line up with the control group. Notice that the matchComplete final status histograms will not be generated for the control groups or the no use groups, so this is only adding a handful of new histograms. On 2012/01/20 21:17:08, dominich wrote: > if mc_status is MC_DEFAULT this will be recorded in two histograms. Is that > expected?
The comments are very helpful but they've raised more questions, I'm afraid. Also more comments in the code. https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... File chrome/browser/prerender/prerender_browsertest.cc (right): https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_browsertest.cc:148: EXPECT_EQ(mc_status(), MC_REPLACEMENT); Can you add some output like the below case that has the URL in it? It really helps when figuring out why a test fails. https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... File chrome/browser/prerender/prerender_contents.cc (right): https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_contents.cc:348: DCHECK(final_status_ == FINAL_STATUS_MAX); This is SO much cleaner and better. :) https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_contents.cc:359: prerender_manager_->RecordFinalStatusWithMatchCompleteStatus( Is RecordFinalStatus ever called directly? If not, maybe make this RecordFinalStatus (taking the mc_status) to simplify the API. https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... File chrome/browser/prerender/prerender_contents.h (right): https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_contents.h:84: enum MatchCompleteStatus { Needs a comment explaining what this is. https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_contents.h:115: MatchCompleteStatus mc_status() const { return mc_status_; } This is more of a style question: If the get and set are this simple and both public, is there any reason not to just make the member public directly? https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_contents.h:280: MatchCompleteStatus mc_status_; Needs a comment explaining what it is. https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_contents.h:280: MatchCompleteStatus mc_status_; mc_status_ should be match_complete_status_ https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... File chrome/browser/prerender/prerender_histograms.cc (right): https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_histograms.cc:173: // (all suffix PerceivedPLT) itym prefix https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_histograms.cc:178: // that were not swapped in so that the set of pages lines up more closely with what few that were not swapped in are included? What does it mean to have ...MatchedComplete...Control vs ...MatchedComplete...Experiment? Is there ever a time to compare ...Matched...Control with ...Matched...Experiment? If not, maybe we should only output both Control and Experiment for ...MatchedComplete? https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_histograms.cc:180: // ...FirstAfterMiss -- First page to finish loading after a prerender, which I can see that these might be useful, but when we do our analysis which are actually used and can we drop some of them? https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_histograms.cc:190: // FirstAfterMissNonOverlapping but NOT FirstAfterMiss Can these be compared between Control, Experiment, and NoUse? Do these have both ...Matched... and ...MatchedComplete...? If not, why not? https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_histograms.cc:289: void PrerenderHistograms::RecordMatchCompleteFinalStatus( nit: If the histogram is FinalStatusMatchComplete then the method should be RecordFinalStatusMatchComplete. https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... File chrome/browser/prerender/prerender_histograms.h (right): https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_histograms.h:50: void RecordFinalStatus(Origin origin, If either this or the below new method are not called directly, make one private and rename the public one to RecordFinalStatus to keep the API simple. https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... File chrome/browser/prerender/prerender_manager.cc (right): https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_manager.cc:87: // Traditionally, "Match" means that a prerendered page was actually visited & Match? or MatchComplete? https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_manager.cc:98: // This ensures that, by comparing a larger Match in Control vs. a smaller Match Match or MatchComplete? https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_manager.cc:103: return final_status != FINAL_STATUS_USED && Might this be easier as an inclusive check rather than exclusive? https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_manager.cc:560: prerender_contents->Destroy(FINAL_STATUS_DEVTOOLS_ATTACHED); This should be |final_status|. You might need a test for it. https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_manager.cc:596: if (!prerender_contents->prerendering_has_started()) { Why is this different to IsControlGroup? https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_manager.cc:738: if (entry->mc_status() == PrerenderContents::MC_DEFAULT && if mc_status is only set from this method, this condition should always be true. Could it be a DCHECK? https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_manager.cc:1139: FinalStatus final_status) const { MC_DEFAULT will be in both histograms. Is this expected? https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... File chrome/browser/prerender/prerender_manager.h (right): https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_manager.h:204: void RecordFinalStatus(Origin origin, Does this need to be public or can it be removed in favour of the new method? https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_manager.h:381: // Helper function to destory a PrerenderContents with the specified nit 'destroy'
Thanks for your comments. As I mentioned below, I will refrain from adding tests, and from running and verifying tests, until the code is stabilizing (ie, you are not giving me another list of 20 items to change). Thanks. https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... File chrome/browser/prerender/prerender_browsertest.cc (right): https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_browsertest.cc:148: EXPECT_EQ(mc_status(), MC_REPLACEMENT); I agree that in the other case, it's helpful, because you see what was expected and what it is. Here, the fact that it's not marked as MC_REPLACEMENT is cause for error. It's a binary check, is it marked or not. So re-printing it wouldn't help much, I think. On 2012/01/20 22:23:37, dominich wrote: > Can you add some output like the below case that has the URL in it? It really > helps when figuring out why a test fails. https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... File chrome/browser/prerender/prerender_contents.cc (right): https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_contents.cc:348: DCHECK(final_status_ == FINAL_STATUS_MAX); On 2012/01/20 22:23:37, dominich wrote: > This is SO much cleaner and better. :) Done. https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_contents.cc:359: prerender_manager_->RecordFinalStatusWithMatchCompleteStatus( Yes, RecordFinalStatus is called directly many times from within PrerenderManager, and mentioning PrerenderContents::MC_DEFAULT every time is tedious. On 2012/01/20 22:23:37, dominich wrote: > Is RecordFinalStatus ever called directly? If not, maybe make this > RecordFinalStatus (taking the mc_status) to simplify the API. https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... File chrome/browser/prerender/prerender_contents.h (right): https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_contents.h:84: enum MatchCompleteStatus { On 2012/01/20 22:23:37, dominich wrote: > Needs a comment explaining what this is. Done. https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_contents.h:115: MatchCompleteStatus mc_status() const { return mc_status_; } In general that's not the way things are usually done. Unless you have an all public struct, member variables should always be private, and provide getters/setters. On 2012/01/20 22:23:37, dominich wrote: > This is more of a style question: If the get and set are this simple and both > public, is there any reason not to just make the member public directly? https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_contents.h:280: MatchCompleteStatus mc_status_; On 2012/01/20 22:23:37, dominich wrote: > Needs a comment explaining what it is. Done. https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_contents.h:280: MatchCompleteStatus mc_status_; Since this is used so frequently, mc_status is much more convenient, esp. for getters/setters. On 2012/01/20 22:23:37, dominich wrote: > mc_status_ should be match_complete_status_ https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... File chrome/browser/prerender/prerender_histograms.cc (right): https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_histograms.cc:178: // that were not swapped in so that the set of pages lines up more closely with Coalescing histograms is unrelated to this change -- so let's discuss is separately and make it a separate change. On 2012/01/20 22:23:37, dominich wrote: > what few that were not swapped in are included? What does it mean to have > ...MatchedComplete...Control vs ...MatchedComplete...Experiment? Is there ever a > time to compare ...Matched...Control with ...Matched...Experiment? If not, maybe > we should only output both Control and Experiment for ...MatchedComplete? https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_histograms.cc:180: // ...FirstAfterMiss -- First page to finish loading after a prerender, which Coalescing histograms is unrelated to this change -- so let's discuss is separately and make it a separate change. On 2012/01/20 22:23:37, dominich wrote: > I can see that these might be useful, but when we do our analysis which are > actually used and can we drop some of them? https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_histograms.cc:190: // FirstAfterMissNonOverlapping but NOT FirstAfterMiss Yes, they appear in all variants, which is implied by not specifying it. I think the definition here + the definition of the field trials make it clear what is being recorded. I dont want to provide additional info on possible use cases in comments here. On 2012/01/20 22:23:37, dominich wrote: > Can these be compared between Control, Experiment, and NoUse? Do these have both > ...Matched... and ...MatchedComplete...? If not, why not? https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_histograms.cc:289: void PrerenderHistograms::RecordMatchCompleteFinalStatus( On 2012/01/20 22:23:37, dominich wrote: > nit: If the histogram is FinalStatusMatchComplete then the method should be > RecordFinalStatusMatchComplete. Done. https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... File chrome/browser/prerender/prerender_manager.cc (right): https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_manager.cc:87: // Traditionally, "Match" means that a prerendered page was actually visited & What I wrote is correct, I think. On 2012/01/20 22:23:37, dominich wrote: > Match? or MatchComplete? https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_manager.cc:98: // This ensures that, by comparing a larger Match in Control vs. a smaller Match On 2012/01/20 22:23:37, dominich wrote: > Match or MatchComplete? Done. https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_manager.cc:103: return final_status != FINAL_STATUS_USED && No, because as new status'es are added, the default should be to generate a MC dummy. On 2012/01/20 22:23:37, dominich wrote: > Might this be easier as an inclusive check rather than exclusive? https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_manager.cc:560: prerender_contents->Destroy(FINAL_STATUS_DEVTOOLS_ATTACHED); Great catch. Haven't even run/written tests yet while you are still providing comments and the code is changing so much. On 2012/01/20 22:23:37, dominich wrote: > This should be |final_status|. You might need a test for it. https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_manager.cc:596: if (!prerender_contents->prerendering_has_started()) { I already answered this before, I said: This is necessary to also cover e.g. the case of a MatchComplete dummy. prerendering_has_started() is the easiest check to cover all these cases. It is also more robust in case one day, the field trial could change on the fly. On 2012/01/20 22:23:37, dominich wrote: > Why is this different to IsControlGroup? https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_manager.cc:738: if (entry->mc_status() == PrerenderContents::MC_DEFAULT && huh? why? Can you give an example? A prerender may be destroyed, replaced with a matchcomplete dummy, which in turn may be destroyed later for some other reason, and you want to make sure to not create a matchcomplete dummy for another matchcomplete dummy currently being destroyed. that's why this check is needed. On 2012/01/20 22:23:37, dominich wrote: > if mc_status is only set from this method, this condition should always be true. > Could it be a DCHECK? https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_manager.cc:1139: FinalStatus final_status) const { not sure why stuff keeps being repeated. i said earlier: Yes. There will be 2 histograms for the Prerender gorups only: The actual FinalStatus'es as perceived by the user, and the MatchComplete FinalStatus'es, that should line up with the control group. Notice that the matchComplete final status histograms will not be generated for the control groups or the no use groups, so this is only adding a handful of new histograms. On 2012/01/20 22:23:37, dominich wrote: > MC_DEFAULT will be in both histograms. Is this expected? https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... File chrome/browser/prerender/prerender_manager.h (right): https://chromiumcodereview.appspot.com/9270018/diff/3002/chrome/browser/prere... chrome/browser/prerender/prerender_manager.h:381: // Helper function to destory a PrerenderContents with the specified On 2012/01/20 22:23:37, dominich wrote: > nit 'destroy' Done.
I likely won't get to this until Monday. Pretty backlogged at the moment.
http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_contents.h:92: // been replaced by a MachComplete dummy. Therefore, we will record nit: MatchComplete http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_contents.h:96: // prerender. Therefore, we iwll record this only for MatchComplete, nit: will http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_contents.h:126: MatchCompleteStatus mc_status() const { return mc_status_; } I understand this is used frequently, but it's Chromium style to not use abbreviations in variable names. Please expand it to match_complete_status_ http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_histograms.cc (right): http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_histograms.cc:279: void PrerenderHistograms::RecordFinalStatus(Origin origin, Could the logic for whether to record to FinalStatus, FinalStatusMatchComplete, or both be moved to here? Then this method would take a MatchCompleteStatus parameter. http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_histograms.h (right): http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_histograms.h:59: nit: remove the extra whitespace here. http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.cc:99: // of the pages forming the difference between the two sets. Could you create an issue for someone to look into minimizing the histograms to those that can actually be compared/are useful to people? http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.cc:597: prerender_contents.release()->Destroy(FINAL_STATUS_USED); Was this leaking before? http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.h:203: // Record a final status of a prerendered page in a histogram. Change this comment to reflect that this is essentially a utility for calling RecordFinalStatusWithMatchCompleteStatus with the default MatchCompleteStatus.
Addressed all your feedback, thanks. Please let me know if you have any more concerns. Thanks. http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_contents.h:92: // been replaced by a MachComplete dummy. Therefore, we will record On 2012/01/23 22:41:40, dominich wrote: > nit: MatchComplete Done. http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_contents.h:96: // prerender. Therefore, we iwll record this only for MatchComplete, On 2012/01/23 22:41:40, dominich wrote: > nit: will Done. http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_contents.h:126: MatchCompleteStatus mc_status() const { return mc_status_; } On 2012/01/23 22:41:40, dominich wrote: > I understand this is used frequently, but it's Chromium style to not use > abbreviations in variable names. Please expand it to match_complete_status_ Done. http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_histograms.cc (right): http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_histograms.cc:279: void PrerenderHistograms::RecordFinalStatus(Origin origin, I'd prefer not to. PrerenderHistograms only knows about the histograms and to record, and have no dependency on other classes or concepts in other classes (such as MatchCompleteStatus). On 2012/01/23 22:41:40, dominich wrote: > Could the logic for whether to record to FinalStatus, FinalStatusMatchComplete, > or both be moved to here? Then this method would take a MatchCompleteStatus > parameter. http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_histograms.h (right): http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_histograms.h:59: On 2012/01/23 22:41:40, dominich wrote: > nit: remove the extra whitespace here. Done. http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.cc:99: // of the pages forming the difference between the two sets. Created Issue 111130 On 2012/01/23 22:41:40, dominich wrote: > Could you create an issue for someone to look into minimizing the histograms to > those that can actually be compared/are useful to people? http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.cc:597: prerender_contents.release()->Destroy(FINAL_STATUS_USED); On 2012/01/23 22:41:40, dominich wrote: > Was this leaking before? No. Before, the final status in this case was e.g. FINAL_STATUS_CONTROL_GROUP, and needed not be overwritten. Now that we have a separate histogram and want the actual termination status, we have to declare this STATUS_USED, since it would have been used. http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.h:203: // Record a final status of a prerendered page in a histogram. On 2012/01/23 22:41:40, dominich wrote: > Change this comment to reflect that this is essentially a utility for calling > RecordFinalStatusWithMatchCompleteStatus with the default MatchCompleteStatus. Done.
http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_histograms.cc (right): http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_histograms.cc:279: void PrerenderHistograms::RecordFinalStatus(Origin origin, Then perhaps MatchCompleteStatus should be a member of PrerenderHistograms. It really is only important for histograms and it would mean that the PrerenderManager and PrerenderHistograms APIs would be cleaner. Right now, people calling into this have to understand the concept of a MatchCompleteStatus. If the logic was in here, noone needs to care. On 2012/01/24 00:59:56, tburkard wrote: > I'd prefer not to. PrerenderHistograms only knows about the histograms and to > record, and have no dependency on other classes or concepts in other classes > (such as MatchCompleteStatus). > > On 2012/01/23 22:41:40, dominich wrote: > > Could the logic for whether to record to FinalStatus, > FinalStatusMatchComplete, > > or both be moved to here? Then this method would take a MatchCompleteStatus > > parameter. > http://codereview.chromium.org/9270018/diff/9001/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/9270018/diff/9001/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_contents.h:90: MC_DEFAULT, MATCH_COMPLETE_DEFAULT
http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_histograms.cc (right): http://codereview.chromium.org/9270018/diff/2006/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_histograms.cc:279: void PrerenderHistograms::RecordFinalStatus(Origin origin, Moved it to PrerenderHistograms, but kept MC Status defined in prerender contents (so im including prerender contents in prerender histograms.h) On 2012/01/24 01:18:41, dominich wrote: > Then perhaps MatchCompleteStatus should be a member of PrerenderHistograms. It > really is only important for histograms and it would mean that the > PrerenderManager and PrerenderHistograms APIs would be cleaner. Right now, > people calling into this have to understand the concept of a > MatchCompleteStatus. If the logic was in here, noone needs to care. > > On 2012/01/24 00:59:56, tburkard wrote: > > I'd prefer not to. PrerenderHistograms only knows about the histograms and to > > record, and have no dependency on other classes or concepts in other classes > > (such as MatchCompleteStatus). > > > > On 2012/01/23 22:41:40, dominich wrote: > > > Could the logic for whether to record to FinalStatus, > > FinalStatusMatchComplete, > > > or both be moved to here? Then this method would take a MatchCompleteStatus > > > parameter. > > > http://codereview.chromium.org/9270018/diff/9001/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/9270018/diff/9001/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_contents.h:90: MC_DEFAULT, On 2012/01/24 01:18:41, dominich wrote: > MATCH_COMPLETE_DEFAULT Done.
I like how this simplifies the logic of FinalStatus recording for the Control group, and it could further simplify the recording in the NoUse group [see the IMPORTANT thing marked below]. The descriptive comments are also great additions. I'm less convinced that FinalStatus for the MATCH_COMPLETE_REPLACEMENT group is needed - won't this pretty much just be used, timed out, or evicted only? It seems like the main advantage of the MatchComplete work is to help minimize bias in the PPLT measurements, and not for the final status where truthfully the Prerender experiment group is sufficient enough to determine success. http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_contents.h:84: // Indicates how this PrerenderContents relates to MatchComplete. Nit: This makes more sense in prerender_histograms.h http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_histograms.cc (right): http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_histograms.cc:172: // Summary of all histograms Perceived PLT histograms: Thanks for adding this description, it's very helpful. http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_histograms.cc:175: // ...Windowed -- PPLT for pages in the 30s after a prerender. Nit: after a prerender is created? http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_histograms.cc:176: // ...Matched -- A prerendered page that was swapped in. Nit: perhaps this should discuss how this is recorded for the Control and NoUse case, since neither swap in. http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_histograms.cc:285: if (mc_status != PrerenderContents::MATCH_COMPLETE_REPLACEMENT) { Do you want this to get triggered when mc_status == MATCH_COMPLETE_REPLACED? http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_manager.cc (left): http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.cc:441: data.contents_->set_final_status(FINAL_STATUS_CONTROL_GROUP); You should add an OBSOLETE comment for FINAL_STATUS_CONTROL_GROUP in prerender_final_status.h http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.cc:101: bool NeedMatchCompleteDummyForFinalStatus(FinalStatus final_status) { This could return an enum which has three values MATCH_COMPLETE_DUMMY_NO MATCH_COMPLETE_DUMMY_YES MATCH_COMPLETE_DUMMY_IMMEDIATE This would remove the need for DestroyAndMarkMatchCompleteAsUsed. Inside MoveEntryToPendingDelete, you wouldn't create the dummy in the IMMEDIATE case, but would record the histogram. The reason I prefer this is that you only need to determine the policy in one location. In your code, you have to get it right in the NeedMatchCompleteDummyForFinalStatus, as well as at each caller in MaybeUsePrerenderedPage. http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.cc:101: bool NeedMatchCompleteDummyForFinalStatus(FinalStatus final_status) { <random thought> I wonder if we should add a table for this on FinalStatus with a compile-time assert. That would force people to think through whether a Match Complete Dummy is needed every time a new FinalStatus is introduced. Otherwise, it appears too easy to forget. </random thought> It's OK to do this in a separate CL. http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.cc:639: prerender_contents.release()->Destroy(FINAL_STATUS_NO_USE_GROUP); IMPORTANT: This should just be FINAL_STATUS_USED here, similar to how FINAL_STATUS_CONTROL_GROUP is not used anymore. http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.h:206: void RecordFinalStatus(Origin origin, Could you just remove this version of RecordFinalStatus? Or make it private, since the only caller is PrerenderManager itself?
http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_contents.h:84: // Indicates how this PrerenderContents relates to MatchComplete. I think a case could be made for either location. Based on your reasoning, FinalStatus should go into histograms, too. I think it makes more sense in here, because it is a property of a PrerenderContents. The other classes just look at that property, but PC is the owner of that property. On 2012/01/24 14:55:46, cbentzel wrote: > Nit: This makes more sense in prerender_histograms.h http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_histograms.cc (right): http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_histograms.cc:172: // Summary of all histograms Perceived PLT histograms: On 2012/01/24 14:55:46, cbentzel wrote: > Thanks for adding this description, it's very helpful. Done. http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_histograms.cc:175: // ...Windowed -- PPLT for pages in the 30s after a prerender. On 2012/01/24 14:55:46, cbentzel wrote: > Nit: after a prerender is created? Done. http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_histograms.cc:176: // ...Matched -- A prerendered page that was swapped in. On 2012/01/24 14:55:46, cbentzel wrote: > Nit: perhaps this should discuss how this is recorded for the Control and NoUse > case, since neither swap in. Done. http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_histograms.cc:285: if (mc_status != PrerenderContents::MATCH_COMPLETE_REPLACEMENT) { No, also in the MATCH_COMPLETE_DEFAULT case. The MC default case means no special treatment, so it becomes part of both FinalStatus and FinalStatusMatchComplete. Only when you have 2 instances (one for the actual perender and one dummy) do you have to account one in one histogram and the other in the other histogram. Added a comment explaining this. On 2012/01/24 14:55:46, cbentzel wrote: > Do you want this to get triggered when mc_status == MATCH_COMPLETE_REPLACED? http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_manager.cc (left): http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.cc:441: data.contents_->set_final_status(FINAL_STATUS_CONTROL_GROUP); On 2012/01/24 14:55:46, cbentzel wrote: > You should add an OBSOLETE comment for FINAL_STATUS_CONTROL_GROUP in > prerender_final_status.h Done. http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_manager.cc (right): http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.cc:101: bool NeedMatchCompleteDummyForFinalStatus(FinalStatus final_status) { Ok will think about this for a new CL. On 2012/01/24 14:55:46, cbentzel wrote: > <random thought> > I wonder if we should add a table for this on FinalStatus with a compile-time > assert. That would force people to think through whether a Match Complete Dummy > is needed every time a new FinalStatus is introduced. Otherwise, it appears too > easy to forget. > </random thought> > > It's OK to do this in a separate CL. http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.cc:101: bool NeedMatchCompleteDummyForFinalStatus(FinalStatus final_status) { Yes and no. Here is why I am hesitant to add something like this: Whether a status requires special treatment or not doesn't just depend on the status -- but also on the order in which checks are done in MaybeUsePrerenderedPage. Eg IMMEDIATE is only applicable for things that come after the has_prerendering_started check. It's conceivable that the order of these checks gets changed. In that case, I think having explicit code right at the place where the change happens brings people's attention that this case may need adjustment. Just having a table at a different place does not make that as obvious. Furthermore, another issue is that the same status could in theory originate at multiple places, requiring different handling based on the place where it originates from. This would not be possible with what you are proposing. Still an interesting thought, but I think we shoudl give it more thought and do it in a separate CL for now. Thanks. On 2012/01/24 14:55:46, cbentzel wrote: > This could return an enum which has three values > > MATCH_COMPLETE_DUMMY_NO > MATCH_COMPLETE_DUMMY_YES > MATCH_COMPLETE_DUMMY_IMMEDIATE > > This would remove the need for DestroyAndMarkMatchCompleteAsUsed. > > Inside MoveEntryToPendingDelete, you wouldn't create the dummy in the IMMEDIATE > case, but would record the histogram. > > The reason I prefer this is that you only need to determine the policy in one > location. In your code, you have to get it right in the > NeedMatchCompleteDummyForFinalStatus, as well as at each caller in > MaybeUsePrerenderedPage. > http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.cc:639: prerender_contents.release()->Destroy(FINAL_STATUS_NO_USE_GROUP); On 2012/01/24 14:55:46, cbentzel wrote: > IMPORTANT: This should just be FINAL_STATUS_USED here, similar to how > FINAL_STATUS_CONTROL_GROUP is not used anymore. Done. http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_manager.h (right): http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_manager.h:206: void RecordFinalStatus(Origin origin, On 2012/01/24 14:55:46, cbentzel wrote: > Could you just remove this version of RecordFinalStatus? Or make it private, > since the only caller is PrerenderManager itself? Done.
LGTM http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_contents.h (right): http://codereview.chromium.org/9270018/diff/8003/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_contents.h:84: // Indicates how this PrerenderContents relates to MatchComplete. On 2012/01/24 19:15:54, tburkard wrote: > I think a case could be made for either location. Based on your reasoning, > FinalStatus should go into histograms, too. I think it makes more sense in > here, because it is a property of a PrerenderContents. > > The other classes just look at that property, but PC is the owner of that > property. Your argument is reasonable. OK. > On 2012/01/24 14:55:46, cbentzel wrote: > > Nit: This makes more sense in prerender_histograms.h >
LGTM pending nit below. http://codereview.chromium.org/9270018/diff/13011/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_final_status.h (right): http://codereview.chromium.org/9270018/diff/13011/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_final_status.h:53: FINAL_STATUS_MATCH_COMPLETE_DUMMY = 38, Is MATCH_COMPLETE_DUMMY still used somewhere? I thought this would be obsolete too.
There is one issue: using final_status_used for control & experiment won't work due to matt's checks for final_status_used in prerender_tracker to determine whether to cancel resource loads. I will make a new final status WOULD_HAVE_BEEN_USED to use in those cases. Timo On Tue, Jan 24, 2012 at 12:52 PM, <dominich@chromium.org> wrote: > LGTM pending nit below. > > > http://codereview.chromium.**org/9270018/diff/13011/chrome/** > browser/prerender/prerender_**final_status.h<http://codereview.chromium.org/9270018/diff/13011/chrome/browser/prerender/prerender_final_status.h> > File chrome/browser/prerender/**prerender_final_status.h (right): > > http://codereview.chromium.**org/9270018/diff/13011/chrome/** > browser/prerender/prerender_**final_status.h#newcode53<http://codereview.chromium.org/9270018/diff/13011/chrome/browser/prerender/prerender_final_status.h#newcode53> > chrome/browser/prerender/**prerender_final_status.h:53: > FINAL_STATUS_MATCH_COMPLETE_**DUMMY = 38, > Is MATCH_COMPLETE_DUMMY still used somewhere? I thought this would be > obsolete too. > > http://codereview.chromium.**org/9270018/<http://codereview.chromium.org/9270... >
Oofa. Do we have tests for this case? On Tue, Jan 24, 2012 at 3:58 PM, Timo Burkard <tburkard@chromium.org> wrote: > There is one issue: > > using final_status_used for control & experiment won't work due to matt's > checks for final_status_used in prerender_tracker to determine whether to > cancel resource loads. > > I will make a new final status WOULD_HAVE_BEEN_USED to use in those cases. > > Timo > > On Tue, Jan 24, 2012 at 12:52 PM, <dominich@chromium.org> wrote: > >> LGTM pending nit below. >> >> >> http://codereview.chromium.**org/9270018/diff/13011/chrome/** >> browser/prerender/prerender_**final_status.h<http://codereview.chromium.org/9270018/diff/13011/chrome/browser/prerender/prerender_final_status.h> >> File chrome/browser/prerender/**prerender_final_status.h (right): >> >> http://codereview.chromium.**org/9270018/diff/13011/chrome/** >> browser/prerender/prerender_**final_status.h#newcode53<http://codereview.chromium.org/9270018/diff/13011/chrome/browser/prerender/prerender_final_status.h#newcode53> >> chrome/browser/prerender/**prerender_final_status.h:53: >> FINAL_STATUS_MATCH_COMPLETE_**DUMMY = 38, >> Is MATCH_COMPLETE_DUMMY still used somewhere? I thought this would be >> obsolete too. >> >> http://codereview.chromium.**org/9270018/<http://codereview.chromium.org/9270... >> > >
Thanks for catching it as well. On Tue, Jan 24, 2012 at 4:01 PM, Chris Bentzel <cbentzel@chromium.org>wrote: > Oofa. > > Do we have tests for this case? > > > On Tue, Jan 24, 2012 at 3:58 PM, Timo Burkard <tburkard@chromium.org>wrote: > >> There is one issue: >> >> using final_status_used for control & experiment won't work due to matt's >> checks for final_status_used in prerender_tracker to determine whether to >> cancel resource loads. >> >> I will make a new final status WOULD_HAVE_BEEN_USED to use in those cases. >> >> Timo >> >> On Tue, Jan 24, 2012 at 12:52 PM, <dominich@chromium.org> wrote: >> >>> LGTM pending nit below. >>> >>> >>> http://codereview.chromium.**org/9270018/diff/13011/chrome/** >>> browser/prerender/prerender_**final_status.h<http://codereview.chromium.org/9270018/diff/13011/chrome/browser/prerender/prerender_final_status.h> >>> File chrome/browser/prerender/**prerender_final_status.h (right): >>> >>> http://codereview.chromium.**org/9270018/diff/13011/chrome/** >>> browser/prerender/prerender_**final_status.h#newcode53<http://codereview.chromium.org/9270018/diff/13011/chrome/browser/prerender/prerender_final_status.h#newcode53> >>> chrome/browser/prerender/**prerender_final_status.h:53: >>> FINAL_STATUS_MATCH_COMPLETE_**DUMMY = 38, >>> Is MATCH_COMPLETE_DUMMY still used somewhere? I thought this would be >>> obsolete too. >>> >>> http://codereview.chromium.**org/9270018/<http://codereview.chromium.org/9270... >>> >> >> >
I'm also removing the FINAL_STATUS_MATCH_COMPLETE dummy (making it obsolete). I had overloaded its use in the browser test, so I'm changing that. Timo On Tue, Jan 24, 2012 at 1:02 PM, Chris Bentzel <cbentzel@chromium.org>wrote: > Thanks for catching it as well. > > > On Tue, Jan 24, 2012 at 4:01 PM, Chris Bentzel <cbentzel@chromium.org>wrote: > >> Oofa. >> >> Do we have tests for this case? >> >> >> On Tue, Jan 24, 2012 at 3:58 PM, Timo Burkard <tburkard@chromium.org>wrote: >> >>> There is one issue: >>> >>> using final_status_used for control & experiment won't work due to >>> matt's checks for final_status_used in prerender_tracker to determine >>> whether to cancel resource loads. >>> >>> I will make a new final status WOULD_HAVE_BEEN_USED to use in those >>> cases. >>> >>> Timo >>> >>> On Tue, Jan 24, 2012 at 12:52 PM, <dominich@chromium.org> wrote: >>> >>>> LGTM pending nit below. >>>> >>>> >>>> http://codereview.chromium.**org/9270018/diff/13011/chrome/** >>>> browser/prerender/prerender_**final_status.h<http://codereview.chromium.org/9270018/diff/13011/chrome/browser/prerender/prerender_final_status.h> >>>> File chrome/browser/prerender/**prerender_final_status.h (right): >>>> >>>> http://codereview.chromium.**org/9270018/diff/13011/chrome/** >>>> browser/prerender/prerender_**final_status.h#newcode53<http://codereview.chromium.org/9270018/diff/13011/chrome/browser/prerender/prerender_final_status.h#newcode53> >>>> chrome/browser/prerender/**prerender_final_status.h:53: >>>> FINAL_STATUS_MATCH_COMPLETE_**DUMMY = 38, >>>> Is MATCH_COMPLETE_DUMMY still used somewhere? I thought this would be >>>> obsolete too. >>>> >>>> http://codereview.chromium.**org/9270018/<http://codereview.chromium.org/9270... >>>> >>> >>> >> >
Adding a test now, will send an update once everything is finished. On Tue, Jan 24, 2012 at 1:01 PM, Chris Bentzel <cbentzel@chromium.org>wrote: > Oofa. > > Do we have tests for this case? > > > On Tue, Jan 24, 2012 at 3:58 PM, Timo Burkard <tburkard@chromium.org>wrote: > >> There is one issue: >> >> using final_status_used for control & experiment won't work due to matt's >> checks for final_status_used in prerender_tracker to determine whether to >> cancel resource loads. >> >> I will make a new final status WOULD_HAVE_BEEN_USED to use in those cases. >> >> Timo >> >> On Tue, Jan 24, 2012 at 12:52 PM, <dominich@chromium.org> wrote: >> >>> LGTM pending nit below. >>> >>> >>> http://codereview.chromium.**org/9270018/diff/13011/chrome/** >>> browser/prerender/prerender_**final_status.h<http://codereview.chromium.org/9270018/diff/13011/chrome/browser/prerender/prerender_final_status.h> >>> File chrome/browser/prerender/**prerender_final_status.h (right): >>> >>> http://codereview.chromium.**org/9270018/diff/13011/chrome/** >>> browser/prerender/prerender_**final_status.h#newcode53<http://codereview.chromium.org/9270018/diff/13011/chrome/browser/prerender/prerender_final_status.h#newcode53> >>> chrome/browser/prerender/**prerender_final_status.h:53: >>> FINAL_STATUS_MATCH_COMPLETE_**DUMMY = 38, >>> Is MATCH_COMPLETE_DUMMY still used somewhere? I thought this would be >>> obsolete too. >>> >>> http://codereview.chromium.**org/9270018/<http://codereview.chromium.org/9270... >>> >> >> >
This has dominic's request to deprecate the match_compelte_dummy, the fix with WOULD_HAVE_BEEN_USED, and a new unit test to verify it. This should address all concerns you have had.
lgtm
Lgtm On Jan 24, 2012 5:29 PM, <dominich@chromium.org> wrote: > lgtm > > > > http://codereview.chromium.**org/9270018/<http://codereview.chromium.org/9270... > |
