Closed Bug 1016622 Opened 5 years ago Closed 5 years ago

Rename Seer to Predictor

Categories

(Core :: Networking, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: u408661, Assigned: jaypoulz)

References

Details

Attachments

(1 file, 5 obsolete files)

Some people think the name "seer" sounds a little too ominous and/or confusing. We should rename the prefs, class, and files to something less ominous/more descriptive. Let's say Predictor (unless you have a better idea... bikesheds welcome).
What about something like 'Anticipator'? If we want to have a little fun, we could try something like 'AnTCPator' or even just 'AnTCP'.

My reasoning here is that, given the context of the problem we are trying to solve, we are certainly trying to predict the user's next move, but we are also reacting to user the based stimulus. If you predict an event, your focus is on whether or not it will occur. When you anticipate an event, you are preparing yourself for the likely consequences.
I vote for mozilla::net::Predictor personally.
(In reply to Honza Bambas (:mayhemer) from comment #2)
> I vote for mozilla::net::Predictor personally.

After some examination of the code base and a little reflection, I'm inclined to agree. I think 'Predictor' is main idea of the code, and the simplest possible name that suggests its function.  While 'anticipation' is a better description for what this module does, further down the line, I think it will be easier for people to see a file called 'Predictor' and think to themselves, 'that's the prediction stuff.'
Attached patch bug-1016622-fix.patch (obsolete) — Splinter Review
Code de-witched.
Attachment #8431107 - Flags: review?(hurley)
Try is burning, I must have missed something.
(In reply to Jeremy Poulin [:jerzilla] from comment #6)
> Try is burning, I must have missed something.

Welcome to Mozilla, where if you haven't set try on fire, you're not doing it right :)
Comment on attachment 8431107 [details] [diff] [review]
bug-1016622-fix.patch

Review of attachment 8431107 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review since, by your own admission, try doesn't like you. Or your patch, one of the two :)
Attachment #8431107 - Flags: review?(hurley)
Attached patch bug-1016622-fix.patch (obsolete) — Splinter Review
Burning try = bad. Burning witches, probably OK.

Here's the try build.
https://tbpl.mozilla.org/?tree=Try&rev=fcbc94822fe6
Attachment #8431107 - Attachment is obsolete: true
Attachment #8431163 - Flags: review?(hurley)
This is a link to a try build running the same code, but with a different commit message.  Given that orange tests are green in the other try build, I don't think the errors are my fault.

https://tbpl.mozilla.org/?tree=Try&rev=1c2811df02fe
Here's a third one. I'm convinced none of the failures in the three try builds are my fault.

https://tbpl.mozilla.org/?tree=Try&rev=caa71976c54b
Comment on attachment 8431163 [details] [diff] [review]
bug-1016622-fix.patch

Review of attachment 8431163 [details] [diff] [review]:
-----------------------------------------------------------------

OK, here goes. Overall nice work, grep seems to have caught (almost) everything!

However, there are 4 actual issues, 1 thing grep "missed", and a whole bunch of whitespace fixups that are needed :)

I tried to catch all the indentation-related whitespace errors (since those were easy to see), but it's quite possible I missed a few.
The other class of whitespace errors are that now, a lot of lines exceed the 80-column limit (since "Predictor" is longer than "Seer"). These you'll probably have to look for yourself, as they don't show up nearly as easily in splinter. If you're lucky, I still have a .emacs incantation somewhere that will highlight lines that exceed 80 characters so you can see them better; I'll see if I can dig one up.

I've called out the actual issues (i.e. not whitespace) with ACTUAL ISSUE in all caps, so you can search for them more easily :)

::: browser/base/content/sanitize.js
@@ +221,5 @@
>          catch (e) { }
>  
>          try {
> +          var predictor = Components.classes["@mozilla.org/network/predictor;1"]
> +                               .getService(Components.interfaces.nsINetworkPredictor);

re-align the period before getService with the one before classes (Don't worry if it causes the line to go over 80 columns)

::: docshell/base/nsDocShell.cpp
@@ +9594,5 @@
>        srcdoc = aSrcdoc;
>      else
>        srcdoc = NullString();
>  
> +    mozilla::net::PredictorPredict(aURI, nullptr, nsINetworkPredictor::PREDICT_LOAD,

Wrap, this line now exceeds 80 columns (one of the few I caught)

@@ +9599,1 @@
>                                this, nullptr);

re-align this line with the arguments above, as it was in the original version

@@ +12724,5 @@
>    nsAutoString uStr;
>    rv = textToSubURI->UnEscapeURIForUI(charset, spec, uStr);    
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  mozilla::net::PredictorPredict(aURI, mCurrentURI, 

You added trailing whitespace on this line, please remove it

::: layout/style/Loader.cpp
@@ +1423,5 @@
>      }
>  
>      if (mDocument) {
> +      mozilla::net::PredictorLearn(aLoadData->mURI, mDocument->GetDocumentURI(),
> +                              nsINetworkPredictor::LEARN_LOAD_SUBRESOURCE,

re-align indents

@@ +1612,5 @@
>    }
>  
>    if (mDocument) {
> +    mozilla::net::PredictorLearn(aLoadData->mURI, mDocument->GetDocumentURI(),
> +                            nsINetworkPredictor::LEARN_LOAD_SUBRESOURCE, mDocument);

re-align indents. I think this means the second line will need wrapped again to fit in 80 columns

::: layout/style/nsFontFaceLoader.cpp
@@ +383,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    nsIDocument *document = ps->GetDocument();
> +  mozilla::net::PredictorLearn(aFontFaceSrc->mURI, document->GetDocumentURI(),
> +                          nsINetworkPredictor::LEARN_LOAD_SUBRESOURCE, loadGroup);

re-align indents

::: netwerk/base/public/nsINetworkPredictor.idl
@@ +1,1 @@
> +/* vim: set ts=2 sts=2 et sw=2: */

Note: I noticed a lot of lines in this file (comments, mostly) that now exceed 80 characters.

@@ +13,5 @@
> +typedef unsigned long PredictorLearnReason;
> +
> +/**
> + * nsINetworkPredictor - learn about pages users visit, and allow us to take
> + *                  predictive actions upon future visits.

re-align wrapped lines with the previous line

@@ +16,5 @@
> + * nsINetworkPredictor - learn about pages users visit, and allow us to take
> + *                  predictive actions upon future visits.
> + *                  NOTE: nsINetworkPredictor should only be used on the main thread
> + */
> +[scriptable, uuid(25e323b6-99e0-4274-b5b3-1a9eb56e28ac)]

ACTUAL ISSUE - we need to change the uuid here to avoid any clashes. Look into uuidgen (or just "/msg firebot uuid" on irc) to get a new uuid.

@@ +139,5 @@
> +
> +namespace mozilla {
> +namespace net {
> +
> +nsresult PredictorPredict(nsIURI *targetURI,

You need to re-align the arguments in all these functions, since the first line of each is now longer.

::: netwerk/base/public/nsINetworkPredictorVerifier.idl
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * nsINetworkPredictorVerifier - used for testing the network predictor to ensure it
> + *                          does what we expect it to do.

re-align "does" with "used"

@@ +11,5 @@
> +#include "nsISupports.idl"
> +
> +interface nsIURI;
> +
> +[scriptable, uuid(ea273653-43a8-4632-8b30-4032e0918e8b)]

ACTUAL ISSUE - Just like with nsINetworkPredictor, generate a new uuid for this, as well.

::: netwerk/base/src/Predictor.cpp
@@ +58,5 @@
> +    } \
> +  } while (0)
> +
> +const char PREDICTOR_ENABLED_PREF[] = "network.predictor.enabled";
> +const char PREDICTOR_SSL_HOVER_PREF[] = "network.predictor.enable-hover-on-ssl";

A lot of these pref lines are now too long. Wrap the ones that are like the ones that were already wrapped (after the =)

@@ +152,5 @@
> +NS_IMPL_ISUPPORTS(PredictorDNSListener, nsIDNSListener);
> +
> +NS_IMETHODIMP
> +PredictorDNSListener::OnLookupComplete(nsICancelable *request,
> +                                  nsIDNSRecord *rec,

re-indent the continuation lines

@@ +246,5 @@
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +
> +  Preferences::AddBoolVarCache(&mEnabled, PREDICTOR_ENABLED_PREF, true);
> +  Preferences::AddBoolVarCache(&mEnableHoverOnSSL, PREDICTOR_SSL_HOVER_PREF, false);

Wrapping needs fixed on a lot of these pref caches, as they now exceed 80 columns

@@ +485,5 @@
> +  nsCOMPtr<nsIFile> oldDBFile;
> +  nsresult rv = mDBFile->GetParent(getter_AddRefs(oldDBFile));
> +  RETURN_IF_FAILED(rv);
> +
> +  rv = oldDBFile->AppendNative(NS_LITERAL_CSTRING("predictor.sqlite"));

ACTUAL ISSUE - This needs to stay "seer.sqlite", since no one will ever have a "predictor.sqlite" file on disk.

@@ +931,5 @@
> +class PredictionEvent : public nsRunnable
> +{
> +public:
> +  PredictionEvent(nsIURI *targetURI, nsIURI *sourceURI,
> +                      PredictorPredictReason reason,

re-align arguments

@@ +1087,5 @@
> +      nsAutoCString hostname;
> +      uri->GetAsciiHost(hostname);
> +      nsCOMPtr<nsICancelable> tmpCancelable;
> +      gPredictor->mDnsService->AsyncResolve(hostname,
> +                                       (nsIDNSService::RESOLVE_PRIORITY_MEDIUM |

re-align indentation

@@ +1148,5 @@
> +// @param globalDegradation - the degradation for this top-level load as
> +//                            determined by CalculateGlobalDegradation
> +int
> +Predictor::CalculateConfidence(int baseConfidence, PRTime lastHit,
> +                          PRTime lastPossible, int globalDegradation)

re-align indentation

@@ +1197,5 @@
> +// (Maybe) adds a predictive action to the prediction runner, based on our
> +// calculated confidence for the subresource in question.
> +void
> +Predictor::SetupPrediction(int confidence, const nsACString &uri,
> +                      PredictionRunner *runner)

re-align indentation

@@ +1211,5 @@
> +// the pages table (which is specific to a particular URI), or from the hosts
> +// table (which is for a particular origin).
> +bool
> +Predictor::LookupTopLevel(QueryType queryType, const nsACString &key,
> +                     TopLevelInfo &info)

re-align

@@ +1309,5 @@
> +
> +  int32_t newLoadCount = info.loadCount + 1;
> +  if (newLoadCount <= 0) {
> +    PREDICTOR_LOG(("Predictor::UpdateTopLevel type %d id %d load count overflow\n",
> +              queryType, info.id));

re-align

@@ +1490,5 @@
> +
> +// This is the driver for prediction based on a new pageload.
> +void
> +Predictor::PredictForPageload(const UriInfo &uri, PredictorVerifierHandle &verifier,
> +                         int stackCount, TimeStamp &predictStartTime)

re-align

@@ +1558,5 @@
> +// This is the driver for predicting at browser startup time based on pages that
> +// have previously been loaded close to startup.
> +void
> +Predictor::PredictForStartup(PredictorVerifierHandle &verifier,
> +                        TimeStamp &predictStartTime)

re-align

@@ +1674,5 @@
> +
> +// Called from the main thread to initiate predictive actions
> +NS_IMETHODIMP
> +Predictor::Predict(nsIURI *targetURI, nsIURI *sourceURI, PredictorPredictReason reason,
> +              nsILoadContext *loadContext, nsINetworkPredictorVerifier *verifier)

re-align

@@ +1730,5 @@
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +
> +  nsRefPtr<PredictionEvent> event = new PredictionEvent(targetURI,
> +                                                                sourceURI,

re-align all arguments

@@ +1835,5 @@
> +// Queries to look up information about a *specific* subresource associated
> +// with a *specific* top-level load.
> +bool
> +Predictor::LookupSubresource(QueryType queryType, const int32_t parentId,
> +                        const nsACString &key, SubresourceInfo &info)

re-align

@@ +1881,5 @@
> +
> +// Add information about a new subresource associated with a top-level load.
> +void
> +Predictor::AddSubresource(QueryType queryType, const int32_t parentId,
> +                     const nsACString &key, const PRTime now)

re-align

@@ +1919,5 @@
> +// Update the information about a particular subresource associated with a
> +// top-level load
> +void
> +Predictor::UpdateSubresource(QueryType queryType, const SubresourceInfo &info,
> +                        const PRTime now, const int32_t parentCount)

re-align

@@ +2196,5 @@
> +
> +// Called from the main thread to update the database
> +NS_IMETHODIMP
> +Predictor::Learn(nsIURI *targetURI, nsIURI *sourceURI, PredictorLearnReason reason,
> +            nsILoadContext *loadContext)

re-align

@@ +2243,5 @@
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +
> +  nsRefPtr<LearnEvent> event = new LearnEvent(targetURI, sourceURI,
> +                                                      reason);

Re-align (or move up, it might fit within the 80 column limit now)

@@ +2730,5 @@
> +EnsureGlobalPredictor(nsINetworkPredictor **aPredictor)
> +{
> +  nsresult rv;
> +  nsCOMPtr<nsINetworkPredictor> predictor = do_GetService("@mozilla.org/network/predictor;1",
> +                                                &rv);

re-align

@@ +2740,5 @@
> +
> +nsresult
> +PredictorPredict(nsIURI *targetURI, nsIURI *sourceURI, PredictorPredictReason reason,
> +            nsILoadContext *loadContext, nsINetworkPredictorVerifier *verifier)
> +{

re-align

@@ +2754,5 @@
> +}
> +
> +nsresult
> +PredictorLearn(nsIURI *targetURI, nsIURI *sourceURI, PredictorLearnReason reason,
> +          nsILoadContext *loadContext)

re-align

@@ +2769,5 @@
> +}
> +
> +nsresult
> +PredictorLearn(nsIURI *targetURI, nsIURI *sourceURI, PredictorLearnReason reason,
> +          nsILoadGroup *loadGroup)

re-align

@@ +2794,5 @@
> +}
> +
> +nsresult
> +PredictorLearn(nsIURI *targetURI, nsIURI *sourceURI, PredictorLearnReason reason,
> +          nsIDocument *document)

re-align

@@ +2815,5 @@
> +}
> +
> +nsresult
> +PredictorLearnRedirect(nsIURI *targetURI, nsIChannel *channel,
> +                  nsILoadContext *loadContext)

re-align

@@ +2838,5 @@
> +  rv = EnsureGlobalPredictor(getter_AddRefs(predictor));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  return predictor->Learn(targetURI, sourceURI,
> +                     nsINetworkPredictor::LEARN_LOAD_REDIRECT, loadContext);

re-align

::: netwerk/build/nsNetCID.h
@@ +448,5 @@
>  
> +// service implementing nsINetworkPredictor
> +#define NS_NETWORKPREDICTOR_CONTRACTID \
> +    "@mozilla.org/network/predictor;1"
> +#define NS_NETWORKPREDICTOR_CID \

ACTUAL ISSUE - let's change the CID. It's also a uuid (though different from the one in the IDLs), so use the same method to get one, and format it as appropriate.

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm
@@ +221,5 @@
>      });
>  
>      // Predictive network data - like cache, no way to clear this per
>      // domain, so just trash it all
> +    let ns = Cc["@mozilla.org/network/predictor;1"].

THE THING GREP "MISSED" - change this to np, since ns originally stood for "network seer" (and change it 2 lines down, of course, as well).
Attachment #8431163 - Flags: review?(hurley)
For the next review, let's do 2 things: (1) Upload a brand new patch, just like the one you have now, with all the fixes incorporated, and (2) upload another patch that is *just* the differences between the current patch and the new one. That will make it easier for me to review the changes that I've asked for, and we'll still have a patch that's checkin-able.
(In reply to Jeremy Poulin [:jerzilla] from comment #11)
> Here's a third one. I'm convinced none of the failures in the three try
> builds are my fault.

I, too, am convinced of that. However, a better way to check is:

Look at the failures by clicking on the orange test. On your first try run, for example, none of the failures are direct matches, but from looking at the bug suggestions, it's obvious that test has a lot of issues already, so chances are you're ok (especially given that this feature is disabled everyewhere). On your second two runs, both failures have exact matches, so you should have definitely been confident in those. (I can show you more on how to recognize this on Monday, if you like.)

If you're still not sure, instead of doing another full run, you can just re-trigger specific jobs in tbpl. You do this by clicking on the orange test, and then clicking the blue + that is next to the test name near the bottom of the screen, just a little bit left of center. You can do this as many times as you want, and you won't waste resources re-building bits that are already built, and re-running tests that don't need re-run.
Attached patch bug-1016622-fix-part2.patch (obsolete) — Splinter Review
Will reupload as single patch if passes inspection.
Attachment #8431986 - Flags: feedback?(hurley)
Comment on attachment 8431986 [details] [diff] [review]
bug-1016622-fix-part2.patch

Review of attachment 8431986 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just a couple minor nits we can check in an updated big patch :)

::: netwerk/base/src/Predictor.cpp
@@ +1328,5 @@
>    mozStorageStatementScoper scope(stmt);
>  
>    int32_t newLoadCount = info.loadCount + 1;
>    if (newLoadCount <= 0) {
> +    PREDICTOR_LOG(

Normally for this, we'll just wrap the string, so something like

PREDICTOR_LOG(("Some text goes here. "
               "Continuation text goes here.", args));

@@ +1814,5 @@
>      }
>  
>      gPredictor->FreeSpaceInQueue();
>  
> +    Telemetry::AccumulateTimeDelta(

You did the other telemetry::accumulates right, but you got this one indented a bit too far (or really, just move the key up a line, assuming it doesn't already overflow 80 columns)
Attachment #8431986 - Flags: feedback?(hurley) → feedback+
Attached patch bug-1016622-fix.patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=684737d005aa
Attachment #8431163 - Attachment is obsolete: true
Attachment #8431986 - Attachment is obsolete: true
Attachment #8432734 - Flags: review?(hurley)
Comment on attachment 8432734 [details] [diff] [review]
bug-1016622-fix.patch

Review of attachment 8432734 [details] [diff] [review]:
-----------------------------------------------------------------

OK, just a couple little things I noticed on this final run-through. I'm going to go ahead and r+ this, since I think we're reaching the point of diminishing returns on whitespace cleanups :) However, do upload a new patch containing these fixes, and mark it r+. Once you have that uploaded, I'll r? the right people to review the non-netwerk/ bits of code, and only after they all r+ the patch can you set checkin-needed.

::: netwerk/base/src/Predictor.cpp
@@ +1755,5 @@
> +    ++mAccumulators->mPredictFullQueue;
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +
> +  nsRefPtr<PredictionEvent> event = new PredictionEvent(targetURI,

Looks like either we both missed this last time, or there's some tabs that got stuck in on these arguments.

::: netwerk/base/src/Predictor.h
@@ +35,5 @@
> +class PredictionRunner;
> +struct PredictorTelemetryAccumulators;
> +class PredictorDNSListener;
> +
> +class Predictor : public nsINetworkPredictor

Ooh, I missed this one last time (sorry!). Align the commas with the colon, please
Attachment #8432734 - Flags: review?(hurley) → review+
Oh, no need for any more try runs, either, since this is all whitespace changes.
Attached patch bug-1016622-fix.patch (obsolete) — Splinter Review
Whitespace changes. Last try run is still valid. (https://tbpl.mozilla.org/?tree=Try&rev=684737d005aa)
Attachment #8432734 - Attachment is obsolete: true
Attachment #8432820 - Flags: review+
Comment on attachment 8432820 [details] [diff] [review]
bug-1016622-fix.patch

Smaug - this giant patch is just a renaming of an interface. You reviewed the original patch over in bug 881804, just looking for a quick r+ on the renaming in the docshell bits
Attachment #8432820 - Flags: review?(bugs)
Comment on attachment 8432820 [details] [diff] [review]
bug-1016622-fix.patch

Johnny - just like I said to smaug - this is a rename of stuff you already reviewed in bug 881804, just looking for a quick r+ on the rename in the scriptloader bits
Attachment #8432820 - Flags: review?(jst)
Comment on attachment 8432820 [details] [diff] [review]
bug-1016622-fix.patch

Boris - this is a rename of work already reviewed in bug 881804, just looking for a quick r+ on the changes to layout/ for the rename to move forward
Attachment #8432820 - Flags: review?(bzbarsky)
Comment on attachment 8432820 [details] [diff] [review]
bug-1016622-fix.patch

Seth - this is a rename involving code already reviewed in bug 881804, just looking for a quick r+ on the imagelib stuff for the sake of completeness
Attachment #8432820 - Flags: review?(seth)
Comment on attachment 8432820 [details] [diff] [review]
bug-1016622-fix.patch

Tim - Jeremy has just renamed some stuff that was already reviewed in bug 881804, just looking for a quick r+ on the browser bits to get this landed
Attachment #8432820 - Flags: review?(ttaubert)
Jeremy - that should cover all the rest of the reviews we need for this (unless I missed something). Hopefully they'll be quick, since the code has already been reviewed, and this is just a rename :)
Comment on attachment 8432820 [details] [diff] [review]
bug-1016622-fix.patch

r=me on the layout bits
Attachment #8432820 - Flags: review?(bzbarsky) → review+
Comment on attachment 8432820 [details] [diff] [review]
bug-1016622-fix.patch

"PredictorPredict" sounds a bit odd
Attachment #8432820 - Flags: review?(bugs) → review+
Comment on attachment 8432820 [details] [diff] [review]
bug-1016622-fix.patch

Review of attachment 8432820 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/Histograms.json
@@ +2089,5 @@
>      "n_buckets": 50,
>      "extended_statistics_ok": true,
>      "description": "Time for an unsuccessful DNS OS resolution (msec)"
>    },
> +  "PREDICTOR_PREDICT_ATTEMPTS": {

Do we really want to lose all the Telemetry Histogram data? It might be worth keeping the legacy names around. That would also mean to keep the current histogram IDs in the code.
Attachment #8432820 - Flags: review?(ttaubert) → review+
(In reply to Tim Taubert [:ttaubert] from comment #29) 
> Do we really want to lose all the Telemetry Histogram data? It might be
> worth keeping the legacy names around. That would also mean to keep the
> current histogram IDs in the code.

Normally I'd agree, but given that this code is not actually enabled on any branch, we won't be losing anything at this point (aside from the few people like me who have explicitly enabled the feature), so I'm not worried.
Comment on attachment 8432820 [details] [diff] [review]
bug-1016622-fix.patch

r=jst, but it's not clear to me that this patch does a hg rename instead of deleting the old files and adding the new ones (i.e. nsINetworkPredictor.idl etc).
Attachment #8432820 - Flags: review?(jst) → review+
(In reply to Nicholas Hurley [:hurley] from comment #30)
> (In reply to Tim Taubert [:ttaubert] from comment #29) 
> > Do we really want to lose all the Telemetry Histogram data? It might be
> > worth keeping the legacy names around. That would also mean to keep the
> > current histogram IDs in the code.
> 
> Normally I'd agree, but given that this code is not actually enabled on any
> branch, we won't be losing anything at this point (aside from the few people
> like me who have explicitly enabled the feature), so I'm not worried.

Ah, I didn't know that. Totally fine then.
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #31)
> Comment on attachment 8432820 [details] [diff] [review]
> bug-1016622-fix.patch
> 
> r=jst, but it's not clear to me that this patch does a hg rename instead of
> deleting the old files and adding the new ones (i.e. nsINetworkPredictor.idl
> etc).

Ah, indeed. Jeremy, did you use hg rename, or hg rm/hg add? If you did the latter, please upload a new patch that does the rename instead. Also, once all the r+'s are in, you'll want to update your commit message to have r=<list of names here> instead of the usual one-name r= bit :)
Flags: needinfo?(jaypoulz)
(In reply to Nicholas Hurley [:hurley] from comment #33)
> Ah, indeed. Jeremy, did you use hg rename, or hg rm/hg add? If you did the
> latter, please upload a new patch that does the rename instead. Also, once
> all the r+'s are in, you'll want to update your commit message to have
> r=<list of names here> instead of the usual one-name r= bit :)

Being new to mecurial, I recall actually doing both - implicitly.  I used hg qref whenever possible, which suggests that as least a few of the files are 'renamed'. I also remember adding some explicitly.  I'll sort it out and have everyone look over it again.
Flags: needinfo?(jaypoulz)
Just have me look over it, that's fine
This undoes all the bad hg version control stuff I did. I know you said not to worry about try from this point on, but I was worried I might have broken something when I was messing around in hg land.

It builds locally though. Just to be safe.
https://tbpl.mozilla.org/?tree=Try&rev=efd7e7c60e45
Attachment #8432820 - Attachment is obsolete: true
Attachment #8432820 - Flags: review?(seth)
Attachment #8433607 - Flags: review?(hurley)
Comment on attachment 8433607 [details] [diff] [review]
bug-fix-1016622-with-rename.patch

Review of attachment 8433607 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.
Attachment #8433607 - Flags: review?(hurley) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d6c1d9adf38b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Blocks: 1021370
Summary: Rename Seer → Rename Seer to Predictor
You need to log in before you can comment on or make changes to this bug.