Closed
Bug 1016622
Opened 10 years ago
Closed 10 years ago
Rename Seer to Predictor
Categories
(Core :: Networking, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: u408661, Assigned: jaypoulz)
References
Details
Attachments
(1 file, 5 obsolete files)
144.84 KB,
patch
|
u408661
:
review+
|
Details | Diff | Splinter Review |
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).
Assignee | ||
Comment 1•10 years ago
|
||
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.
Comment 2•10 years ago
|
||
I vote for mozilla::net::Predictor personally.
Assignee | ||
Comment 3•10 years ago
|
||
(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.'
Assignee | ||
Comment 5•10 years ago
|
||
The is the current try build: https://tbpl.mozilla.org/?tree=Try&rev=d45b53882a3d
Assignee | ||
Comment 6•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
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
Assignee | ||
Comment 11•10 years ago
|
||
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
Reporter | ||
Comment 12•10 years ago
|
||
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)
Reporter | ||
Comment 13•10 years ago
|
||
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.
Reporter | ||
Comment 14•10 years ago
|
||
(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.
Assignee | ||
Comment 15•10 years ago
|
||
Will reupload as single patch if passes inspection.
Attachment #8431986 -
Flags: feedback?(hurley)
Reporter | ||
Comment 16•10 years ago
|
||
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+
Assignee | ||
Comment 17•10 years ago
|
||
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)
Reporter | ||
Comment 18•10 years ago
|
||
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+
Reporter | ||
Comment 19•10 years ago
|
||
Oh, no need for any more try runs, either, since this is all whitespace changes.
Assignee | ||
Comment 20•10 years ago
|
||
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+
Reporter | ||
Comment 21•10 years ago
|
||
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)
Reporter | ||
Comment 22•10 years ago
|
||
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)
Reporter | ||
Comment 23•10 years ago
|
||
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)
Reporter | ||
Comment 24•10 years ago
|
||
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)
Reporter | ||
Comment 25•10 years ago
|
||
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)
Reporter | ||
Comment 26•10 years ago
|
||
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 27•10 years ago
|
||
Comment on attachment 8432820 [details] [diff] [review] bug-1016622-fix.patch r=me on the layout bits
Attachment #8432820 -
Flags: review?(bzbarsky) → review+
Comment 28•10 years ago
|
||
Comment on attachment 8432820 [details] [diff] [review] bug-1016622-fix.patch "PredictorPredict" sounds a bit odd
Attachment #8432820 -
Flags: review?(bugs) → review+
Comment 29•10 years ago
|
||
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+
Reporter | ||
Comment 30•10 years ago
|
||
(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 31•10 years ago
|
||
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+
Comment 32•10 years ago
|
||
(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.
Reporter | ||
Comment 33•10 years ago
|
||
(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)
Assignee | ||
Comment 34•10 years ago
|
||
(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)
Reporter | ||
Comment 35•10 years ago
|
||
Just have me look over it, that's fine
Assignee | ||
Comment 36•10 years ago
|
||
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)
Reporter | ||
Comment 37•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 38•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6c1d9adf38b
Keywords: checkin-needed
Comment 39•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d6c1d9adf38b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Updated•10 years ago
|
Summary: Rename Seer → Rename Seer to Predictor
You need to log in
before you can comment on or make changes to this bug.
Description
•