Closed
Bug 989137
Opened 11 years ago
Closed 11 years ago
Telemetry Experiments: remaining AddonManager integration cleanups
Categories
(Firefox Health Report Graveyard :: Client: Desktop, defect)
Firefox Health Report Graveyard
Client: Desktop
Tracking
(Not tracked)
VERIFIED
FIXED
Firefox 31
People
(Reporter: gps, Assigned: gfritzsche)
References
(Blocks 1 open bug)
Details
(Whiteboard: p=8 s=it-31c-30a-29b.3 [qa-])
Attachments
(18 files, 3 obsolete files)
37 bytes,
text/x-review-board-request
|
Unfocused
:
review+
|
Details |
37 bytes,
text/x-review-board-request
|
gfritzsche
:
review+
|
Details |
37 bytes,
text/x-review-board-request
|
benjamin
:
review+
|
Details |
37 bytes,
text/x-review-board-request
|
gfritzsche
:
review+
|
Details |
37 bytes,
text/x-review-board-request
|
gfritzsche
:
review+
|
Details |
37 bytes,
text/x-review-board-request
|
Unfocused
:
review+
|
Details |
37 bytes,
text/x-review-board-request
|
gfritzsche
:
review+
Unfocused
:
review-
|
Details |
37 bytes,
text/x-review-board-request
|
gps
:
review+
|
Details |
37 bytes,
text/x-review-board-request
|
Unfocused
:
review+
|
Details |
37 bytes,
text/x-review-board-request
|
Unfocused
:
review+
|
Details |
37 bytes,
text/x-review-board-request
|
gfritzsche
:
review+
|
Details |
37 bytes,
text/x-review-board-request
|
gps
:
review+
|
Details |
37 bytes,
text/x-review-board-request
|
gfritzsche
:
review+
|
Details |
37 bytes,
text/x-review-board-request
|
gfritzsche
:
review+
|
Details |
37 bytes,
text/x-review-board-request
|
gfritzsche
:
review+
|
Details |
37 bytes,
text/x-review-board-request
|
Unfocused
:
review+
|
Details |
37 bytes,
text/x-review-board-request
|
Unfocused
:
review+
gfritzsche
:
review+
|
Details |
4.21 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #985084 +++
Opening a new bug to track remaining work on AddonManager/XPIProvider integration once all the other patches land. (I want to land the existing patches and close out the bugs rather than have bugs linger in a semi-opened state.)
This is assigned to me and is my #1 priority until it lands.
Reporter | ||
Comment 1•11 years ago
|
||
Attachment #8398207 -
Flags: review?(georg.fritzsche)
Updated•11 years ago
|
Attachment #8398207 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #8398207 -
Flags: review?(georg.fritzsche)
Reporter | ||
Comment 2•11 years ago
|
||
Attachment #8398606 -
Flags: review?(georg.fritzsche)
Assignee | ||
Comment 3•11 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #2)
> Created attachment 8398606 [details]
> Part 2: Associate ExperimentEntry with Experiments instance
Hm, sorry if that sounds too sceptical, but why do we need to make the relation-ship bi-directional?
I'd much prefer to keep it unidirectional if possible (for testability, reasoning, ...).
Reporter | ||
Comment 4•11 years ago
|
||
Attachment #8398726 -
Flags: review?(georg.fritzsche)
Reporter | ||
Comment 5•11 years ago
|
||
Need a Toolkit peer for this one.
Attachment #8398728 -
Flags: review?(benjamin)
Reporter | ||
Comment 6•11 years ago
|
||
Attachment #8398729 -
Flags: review?(georg.fritzsche)
Reporter | ||
Comment 7•11 years ago
|
||
Attachment #8398730 -
Flags: review?(georg.fritzsche)
Assignee | ||
Updated•11 years ago
|
Attachment #8398729 -
Flags: review?(georg.fritzsche) → review+
Assignee | ||
Updated•11 years ago
|
Attachment #8398730 -
Flags: review?(georg.fritzsche) → review+
Updated•11 years ago
|
Attachment #8398728 -
Flags: review?(benjamin) → review+
Assignee | ||
Updated•11 years ago
|
Attachment #8398726 -
Flags: review?(georg.fritzsche) → review+
Assignee | ||
Comment 8•11 years ago
|
||
Comment on attachment 8398606 [details]
Part 2: Associate ExperimentEntry with Experiments instance
Cancelling this for now per the RB comment.
Attachment #8398606 -
Flags: review?(georg.fritzsche)
Reporter | ||
Comment 9•11 years ago
|
||
Attachment #8398824 -
Flags: review?(bmcbride)
Reporter | ||
Comment 10•11 years ago
|
||
Comment on attachment 8398606 [details]
Part 2: Associate ExperimentEntry with Experiments instance
Patch not necessary.
Attachment #8398606 -
Attachment is obsolete: true
Reporter | ||
Updated•11 years ago
|
Attachment #8398726 -
Attachment description: Part 3: Don't use a global logger → Part 2: Don't use a global logger
Reporter | ||
Updated•11 years ago
|
Attachment #8398728 -
Attachment description: Part 4: Add a Log.jsm API for doing prefix logging → Part 3: Add a Log.jsm API for doing prefix logging
Reporter | ||
Updated•11 years ago
|
Attachment #8398729 -
Attachment description: Part 5: Use a prefixing logger in Experiments.jsm → Part 4: Use a prefixing logger in Experiments.jsm
Reporter | ||
Updated•11 years ago
|
Attachment #8398730 -
Attachment description: Part 6: Prefix logging with object identifier → Part 5: Prefix logging with object identifier
Reporter | ||
Updated•11 years ago
|
Attachment #8398824 -
Attachment description: Part 7: Don't allow unknown experiments to be installed → Part 6: Don't allow unknown experiments to be installed
Reporter | ||
Comment 11•11 years ago
|
||
Attachment #8398981 -
Flags: review?(georg.fritzsche)
Reporter | ||
Comment 12•11 years ago
|
||
Parts 1-5:
https://hg.mozilla.org/integration/fx-team/rev/60423f4aeda2
https://hg.mozilla.org/integration/fx-team/rev/0857d467b4ed
https://hg.mozilla.org/integration/fx-team/rev/5b408704c998
https://hg.mozilla.org/integration/fx-team/rev/c27967f904b6
https://hg.mozilla.org/integration/fx-team/rev/0efaafae5c60
Whiteboard: [leave open]
Reporter | ||
Comment 13•11 years ago
|
||
Not asking for review yet because tests are failing.
Assignee | ||
Comment 14•11 years ago
|
||
Anything we could take a look at to help? Any try push links?
Reporter | ||
Updated•11 years ago
|
Attachment #8398985 -
Attachment description: Part 8: Activate disabled experiments when necessary → Part X: Activate disabled experiments when necessary
Reporter | ||
Comment 15•11 years ago
|
||
Touches a Makefile.in, so bsmedberg gets review.
Attachment #8399128 -
Flags: review?(benjamin)
Reporter | ||
Comment 16•11 years ago
|
||
Attachment #8399129 -
Flags: review?(bmcbride)
Reporter | ||
Comment 17•11 years ago
|
||
Comment on attachment 8398985 [details]
Part 10: Activate disabled experiments when necessary
I could have kept X as the Roman numeral. Meh.
Attachment #8398985 -
Attachment description: Part X: Activate disabled experiments when necessary → Part 10: Activate disabled experiments when necessary
Assignee | ||
Comment 18•11 years ago
|
||
Comment on attachment 8398981 [details]
Part 9: More thorough testing of add-on enable state
r+ with nit per RB.
Attachment #8398981 -
Flags: review?(georg.fritzsche) → review+
Comment 19•11 years ago
|
||
Updated•11 years ago
|
Attachment #8398824 -
Flags: review?(bmcbride) → review+
Updated•11 years ago
|
Attachment #8398981 -
Flags: review+
Updated•11 years ago
|
Attachment #8399129 -
Flags: review?(bmcbride) → review+
Reporter | ||
Comment 20•11 years ago
|
||
Comment on attachment 8399128 [details]
Part 6: Generate XPIs as part of the build
bsmedberg granted review on RB.
I moved this up in the series.
Attachment #8399128 -
Attachment description: Part 8: Generate XPIs as part of the build → Part 6: Generate XPIs as part of the build
Attachment #8399128 -
Flags: review?(benjamin) → review+
Reporter | ||
Updated•11 years ago
|
Attachment #8399129 -
Attachment description: Part 9: Remove unncessary bits from install.rdf → Part 7: Remove unncessary bits from install.rdf
Reporter | ||
Updated•11 years ago
|
Attachment #8398824 -
Attachment description: Part 6: Don't allow unknown experiments to be installed → Part 8: Don't allow unknown experiments to be installed
Reporter | ||
Updated•11 years ago
|
Attachment #8398981 -
Attachment description: Part 7: More thorough testing of add-on enable state → Part 9: More thorough testing of add-on enable state
Reporter | ||
Comment 21•11 years ago
|
||
Reporter | ||
Comment 22•11 years ago
|
||
Comment on attachment 8398985 [details]
Part 10: Activate disabled experiments when necessary
Merged into previous patch.
Attachment #8398985 -
Attachment is obsolete: true
Reporter | ||
Comment 23•11 years ago
|
||
Comment on attachment 8398981 [details]
Part 9: More thorough testing of add-on enable state
Merged with subsequent part and updated. Needs re-review.
Attachment #8398981 -
Flags: review?(georg.fritzsche)
Attachment #8398981 -
Flags: review?(bmcbride)
Attachment #8398981 -
Flags: review+
This needed a clobber: http://hg.mozilla.org/mozilla-central/rev/1417d180a1d8
Blocks: clobber
Assignee | ||
Comment 26•11 years ago
|
||
Comment on attachment 8398981 [details]
Part 9: More thorough testing of add-on enable state
r+ pending one issue in RB.
Attachment #8398981 -
Flags: review?(georg.fritzsche) → review+
Reporter | ||
Comment 27•11 years ago
|
||
What a weird clobber.
rm -f '../../dist/bin/browser/modules/experiments/Experiments.jsm'
/builds/slave/m-cen-osx64-000000000000000000/build/obj-firefox/x86_64/_virtualenv/bin/python -m mozbuild.action.preprocessor --depend .deps/Experiments.jsm.pp -DNO_NSPR_10_SUPPORT -DX_DISPLAY_MISSING='1' -DHAVE_X86_AVX2='1' -DHAVE_64BIT_OS='1' -DMOZ_ENABLE_PROFILER_SPS='1' -DMOZ_INSTRUMENTS='1' -DMOZ_PROFILING='1' -DMOZILLA_VERSION='"31.0a1"' -DMOZILLA_VERSION_U='31.0a1' -DMOZILLA_UAVERSION='"31.0"' -DXP_MACOSX='1' -DXP_DARWIN='1' -DD_INO='d_ino' -DMOZ_DEBUG_SYMBOLS='1' -DSTDC_HEADERS='1' -DHAVE_SSIZE_T='1' -DHAVE_ST_BLKSIZE='1' -DHAVE_SIGINFO_T='1' -DHAVE_VISIBILITY_HIDDEN_ATTRIBUTE='1' -DHAVE_VISIBILITY_ATTRIBUTE='1' -DHAVE_DIRENT_H='1' -DHAVE_GETOPT_H='1' -DHAVE_MEMORY_H='1' -DHAVE_UNISTD_H='1' -DHAVE_NL_TYPES_H='1' -DHAVE_X11_XKBLIB_H='1' -DHAVE_CPUID_H='1' -DHAVE_SYS_TYPES_H='1' -DHAVE_NETINET_IN_H='1' -DHAVE_SIN_LEN='1' -DHAVE_SCONN_LEN='1' -DHAVE_SIN6_LEN='1' -DHAVE_SA_LEN='1' -DINCLUDE_MOZILLA_DTRACE='1' -DHAVE_SYS_CDEFS_H='1' -DHAVE_DLADDR='1' -DNO_X11='1' -DHAVE_STAT64='1' -DHAVE_LSTAT64='1' -DHAVE_LOCALTIME_R='1' -DHAVE_LANGINFO_CODESET='1' -DVA_COPY='va_copy' -DHAVE_VA_COPY='1' -DHAVE_VA_LIST_AS_ARRAY='1' -DMALLOC_H='<malloc/malloc.h>' -DHAVE_POSIX_MEMALIGN='1' -DHAVE_VALLOC='1' -DHAVE_I18N_LC_MESSAGES='1' -DHAVE_LOCALECONV='1' -DNS_ATTR_MALLOC='__attribute__((malloc))' -DNS_WARN_UNUSED_RESULT='__attribute__((warn_unused_result))' -DNIGHTLY_BUILD='1' -DMOZ_UPDATE_CHANNEL='default' -DEARLY_BETA_OR_EARLIER='1' -DMOZ_PHOENIX='1' -DMOZ_BUILD_APP='browser' -DMOZ_WIDGET_COCOA='1' -DMOZ_INSTRUMENT_EVENT_LOOP='1' -DNS_PRINTING='1' -DNS_PRINT_PREVIEW='1' -DMOZ_DISTRIBUTION_ID='"org.mozilla"' -DIBMBIDI='1' -DACCESSIBILITY='1' -DMOZ_WEBRTC='1' -DMOZ_WEBRTC_ASSERT_ALWAYS='1' -DMOZ_WEBRTC_SIGNALING='1' -DMOZ_PEERCONNECTION='1' -DMOZ_SCTP='1' -DMOZ_SRTP='1' -DMOZ_SAMPLE_TYPE_FLOAT32='1' -DMOZ_WEBSPEECH='1' -DMOZ_RAW='1' -DATTRIBUTE_ALIGNED_MAX='64' -DMOZ_WEBM='1' -DMOZ_APPLEMEDIA='1' -DMOZ_MEDIA_NAVIGATOR='1' -DMOZ_VPX='1' -DMOZ_VPX_ERROR_CONCEALMENT='1' -DVPX_X86_ASM='1' -DMOZ_WAVE='1' -DMOZ_VORBIS='1' -DMOZ_OPUS='1' -DMOZ_WEBM_ENCODER='1' -DENABLE_SYSTEM_EXTENSION_DIRS='1' -DMOZ_WEBGL='1' -DMOZ_WEBGL_CONFORMANT='1' -DMOZ_GAMEPAD='1' -DMOZ_CRASHREPORTER='1' -DMOZ_CRASHREPORTER_ENABLE_PERCENT='100' -DLIBJPEG_TURBO_X64_ASM='1' -DMOZ_WEBAPP_RUNTIME='1' -DMOZ_SIGNING='1' -DMOZ_ENABLE_SIGNMAR='1' -DMOZ_UPDATER='1' -DGTEST_HAS_RTTI='0' -DMOZ_CONTENT_SANDBOX_REPORTER='1' -DMOZ_FEEDS='1' -DMOZ_SAFE_BROWSING='1' -DMOZ_URL_CLASSIFIER='1' -DGL_PROVIDER_='1' -DMOZ_LOGGING='1' -DMOZ_MEMORY='1' -DMOZ_MEMORY_DARWIN='1' -DJS_CRASH_DIAGNOSTICS='1' -DJSGC_INCREMENTAL='1' -DJSGC_USE_EXACT_ROOTING='1' -DJSGC_GENERATIONAL='1' -DMOZ_PAY='1' -DMOZ_ACTIVITIES='1' -DMOZ_SERVICES_FXACCOUNTS='1' -DHAVE___CXA_DEMANGLE='1' -DHAVE__UNWIND_BACKTRACE='1' -DJS_DEFAULT_JITREPORT_GRANULARITY='3' -DMOZ_OMNIJAR='1' -DMOZ_USER_DIR='"Mozilla"' -DMOZ_TREE_PIXMAN='1' -DHAVE_STDINT_H='1' -DHAVE_INTTYPES_H='1' -DMOZ_TREE_CAIRO='1' -DMOZ_ENABLE_SKIA='1' -DUSE_SKIA='1' -DUSE_SKIA_GPU='1' -DMOZ_XUL='1' -DMOZ_PROFILELOCKING='1' -DENABLE_MARIONETTE='1' -DBUILD_CTYPES='1' -DMOZ_PLACES='1' -DMOZ_SOCIAL='1' -DMOZ_SERVICES_COMMON='1' -DMOZ_SERVICES_CRYPTO='1' -DMOZ_SERVICES_HEALTHREPORT='1' -DMOZ_SERVICES_METRICS='1' -DMOZ_SERVICES_SYNC='1' -DMOZ_JSDOWNLOADS='1' -DMOZ_MACBUNDLE_ID='org.mozilla.nightly' -DMOZ_B2G_VERSION='"1.0.0"' -DMOZ_B2G_OS_NAME='""' -DMOZ_APP_UA_NAME='""' -DMOZ_APP_UA_VERSION='"31.0a1"' -DFIREFOX_VERSION='31.0a1' -DMOZ_TELEMETRY_DISPLAY_REV='2' -DMOZ_TELEMETRY_REPORTING='1' -DMOZ_TELEMETRY_ON_BY_DEFAULT='1' -DMOZ_DATA_REPORTING='1' -DMOZ_DLL_SUFFIX='".dylib"' -DXP_UNIX='1' -DA11Y_LOG='1' -DEXPOSE_INTL_API='1' -DENABLE_INTL_API='1' -DU_STATIC_IMPLEMENTATION='1' -DU_USING_ICU_NAMESPACE='0' -DMOZ_STATIC_JS='1' -DNDEBUG -DTRIMMED '/builds/slave/m-cen-osx64-000000000000000000/build/browser/experiments/Experiments.jsm' -o '../../dist/bin/browser/modules/experiments/Experiments.jsm'
set -e; \
../../dist/bin/nsinstall -D /builds/slave/m-cen-osx64-000000000000000000/build/obj-firefox/x86_64/_tests/xpcshell/browser/experiments/test/xpcshell; \
for dir in /builds/slave/m-cen-osx64-000000000000000000/build/browser/experiments/test/addons/*; do \
base=`basename $dir`; \
(cd $dir && zip -qr /builds/slave/m-cen-osx64-000000000000000000/build/obj-firefox/x86_64/_tests/xpcshell/browser/experiments/test/xpcshell/$base.xpi *); \
done
zip error: Zip file structure invalid (/builds/slave/m-cen-osx64-000000000000000000/build/obj-firefox/x86_64/_tests/xpcshell/browser/experiments/test/xpcshell/experiment-1.xpi)
zip error: Zip file structure invalid (/builds/slave/m-cen-osx64-000000000000000000/build/obj-firefox/x86_64/_tests/xpcshell/browser/experiments/test/xpcshell/experiment-1a.xpi)
zip error: Zip file structure invalid (/builds/slave/m-cen-osx64-000000000000000000/build/obj-firefox/x86_64/_tests/xpcshell/browser/experiments/test/xpcshell/experiment-2.xpi)
make[6]: *** [libs] Error 3
make[5]: *** [browser/experiments/libs] Error 2
make[4]: *** [libs] Error 2
make[3]: *** [default] Error 2
make[2]: *** [realbuild] Error 2
make[1]: *** [realbuild] Error 2
make: *** [build] Error 2
Updated•11 years ago
|
Attachment #8398981 -
Flags: review?(bmcbride) → review-
Reporter | ||
Comment 28•11 years ago
|
||
Attachment #8400982 -
Flags: review?(bmcbride)
Updated•11 years ago
|
Attachment #8400982 -
Flags: review?(bmcbride) → review+
Reporter | ||
Comment 29•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/7e410c1d61e6
https://hg.mozilla.org/integration/fx-team/rev/d3053c4e4c51
https://hg.mozilla.org/integration/fx-team/rev/831a3ccce100
Parts 8, 9, 10. (I reordered some patches.)
Reporter | ||
Updated•11 years ago
|
Attachment #8400982 -
Attachment description: Part 11: Establish AddonManagerTesting.jsm to hold common Addon Manager routines → Part 10: Establish AddonManagerTesting.jsm to hold common Addon Manager routines
Comment 30•11 years ago
|
||
Backed out 8/9/10 in https://hg.mozilla.org/integration/fx-team/rev/317a809acdc8
You did fine, xpcshell was green on your push and the next two.
Then we closed, then there was a downtime this morning, a downtime where releng had been talking about creating a new image and recreating spot slave images to increase their size.
Now, suddenly, you're frequently failing like https://tbpl.mozilla.org/php/getParsedLog.php?id=37325506&tree=Fx-Team. Is that just coincidence, or is something getting left behind by the test (or some other test) which makes it fail when run a second time on the same slave, and since they (might have) created an image from a slave which ran on one of those earlier pushes, now some large percentage of the slaves "have run it before" even though they themselves haven't?
Assignee | ||
Comment 31•11 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #29)
> https://hg.mozilla.org/integration/fx-team/rev/7e410c1d61e6
> https://hg.mozilla.org/integration/fx-team/rev/d3053c4e4c51
> https://hg.mozilla.org/integration/fx-team/rev/831a3ccce100
>
> Parts 8, 9, 10. (I reordered some patches.)
What happened to the part 10 we discussed in RB?
https://reviewboard.allizom.org/r/55/
Assignee | ||
Comment 32•11 years ago
|
||
(In reply to Phil Ringnalda (:philor) from comment #30)
> Now, suddenly, you're frequently failing like
> https://tbpl.mozilla.org/php/getParsedLog.php?id=37325506&tree=Fx-Team.
Seeing the same thing locally after a clobber with current fxteam & the patches from comment 29 applied.
Assignee | ||
Comment 33•11 years ago
|
||
The interesting part:
> 20:05:17 INFO - 1396753516349 addons.manager WARN InstallListener threw exception when calling onInstallStarted
That might also have been what blocked me on bug 992258.
Pushed fix to try: https://tbpl.mozilla.org/?tree=Try&rev=10fe591aaf97
Assignee | ||
Comment 34•11 years ago
|
||
Requesting sign-off for this simple fix so we can re-land the above if the try push is fine.
Attachment #8402629 -
Flags: review?(benjamin)
Comment 35•11 years ago
|
||
Comment on attachment 8402629 [details] [diff] [review]
Part 11: Fix addon listener.
I don't understand why this method returns a set at all. We only ever expect one experiment addon to be installed at a time, and that comes from _evaluateExperiments.
Can we store the to-be-installed experiment in a similar way we store the to-be-uninstalled experiment (in this._pendingUninstall) and avoid the complexity with a map?
Assignee | ||
Updated•11 years ago
|
Attachment #8402629 -
Flags: review?(benjamin)
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(gps)
Reporter | ||
Comment 36•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/0e9ff2ea5085
https://hg.mozilla.org/integration/fx-team/rev/7ade80b09c73
https://hg.mozilla.org/integration/fx-team/rev/d7cbb56f75e1
Pushed part 8 with minor fixup (essentially part 11 rolled in).
Reporter | ||
Updated•11 years ago
|
Attachment #8402629 -
Attachment is obsolete: true
Flags: needinfo?(gps)
Reporter | ||
Comment 37•11 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #31)
> What happened to the part 10 we discussed in RB?
> https://reviewboard.allizom.org/r/55/
It has been bumped to part 11. Or 12. I can't remember any more. Personally, I find maintaining part numbers to be silly. But, our current review tooling dictates it because it otherwise makes it impossible to sort out patch order.
Assignee | ||
Comment 38•11 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #36)
> https://hg.mozilla.org/integration/fx-team/rev/0e9ff2ea5085
> https://hg.mozilla.org/integration/fx-team/rev/7ade80b09c73
> https://hg.mozilla.org/integration/fx-team/rev/d7cbb56f75e1
>
> Pushed part 8 with minor fixup (essentially part 11 rolled in).
What about comment 11?
Assignee | ||
Comment 39•11 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #38)
> What about comment 11?
Hm, i meant comment 35
Comment 40•11 years ago
|
||
Updated•11 years ago
|
Whiteboard: [leave open] → [leave open] p=0 s=it-31c-30a-29b.2
Reporter | ||
Comment 42•11 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #39)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #38)
> > What about comment 11?
>
> Hm, i meant comment 35
I ignored comment 35 because that would invalidate the r+. I don't think Set vs scalar makes much of a difference. If nothing else, I feel Set is more robust because it isn't susceptible to funky JavaScript equivalence issues. YAGNI applies, but I bet we'll someday have multiple experiments active at the same time.
Flags: needinfo?(gps)
Assignee | ||
Comment 43•11 years ago
|
||
Hm, it's just string comparison and the possibility of changes later doesn't seem like a great argument here.
The whole feature is designed for "only one experiment active", so we shouldn't worry about multiple experiments in single places yet.
But i guess this can be dealt with later.
Reporter | ||
Comment 44•11 years ago
|
||
Attachment #8404370 -
Flags: review?(georg.fritzsche)
Reporter | ||
Comment 45•11 years ago
|
||
r+ granted by gritzsche on RB.
Attachment #8404376 -
Flags: review+
Reporter | ||
Comment 46•11 years ago
|
||
Assignee | ||
Comment 47•11 years ago
|
||
What is actually left to do here?
Can we possibly start to file new bugs for actionable items?
Assignee | ||
Updated•11 years ago
|
Attachment #8404370 -
Flags: review?(georg.fritzsche)
Comment 48•11 years ago
|
||
Updated•11 years ago
|
Whiteboard: [leave open] p=0 s=it-31c-30a-29b.2 → [leave open] p=8 s=it-31c-30a-29b.2
Updated•11 years ago
|
Whiteboard: [leave open] p=8 s=it-31c-30a-29b.2 → [leave open] p=8 s=it-31c-30a-29b.2 [qa?]
Comment 49•11 years ago
|
||
Marking as [qa-] for now as it doesn't appear this needs QA attention. Kamil, please mark as [qa+] and test this if you find cause to do so.
QA Contact: kamiljoz
Whiteboard: [leave open] p=8 s=it-31c-30a-29b.2 [qa?] → [leave open] p=8 s=it-31c-30a-29b.2 [qa-]
Reporter | ||
Comment 50•11 years ago
|
||
Attachment #8404964 -
Flags: review?(georg.fritzsche)
Reporter | ||
Comment 51•11 years ago
|
||
Attachment #8404965 -
Flags: review?(georg.fritzsche)
Reporter | ||
Comment 52•11 years ago
|
||
Attachment #8404967 -
Flags: review?(georg.fritzsche)
Reporter | ||
Comment 53•11 years ago
|
||
Attachment #8405014 -
Flags: review?(bmcbride)
Reporter | ||
Comment 54•11 years ago
|
||
You won't believe how long it took me to make this patch work. I hate browser chrome testing so much.
Attachment #8405094 -
Flags: review?(georg.fritzsche)
Attachment #8405094 -
Flags: review?(bmcbride)
Updated•11 years ago
|
Flags: firefox-backlog+
Comment 55•11 years ago
|
||
Anthony,
Sounds good. I asked IRC channel if there was anything that could be tested here, most of this will be covered via automation. Once everything lands in this ticket, I'll go through this at a higher level and create any issues that are found.
Updated•11 years ago
|
Attachment #8405014 -
Flags: review?(bmcbride) → review+
Updated•11 years ago
|
Attachment #8405094 -
Flags: review?(bmcbride) → review+
Assignee | ||
Updated•11 years ago
|
Attachment #8404965 -
Flags: review?(georg.fritzsche) → review+
Assignee | ||
Updated•11 years ago
|
Attachment #8404967 -
Flags: review?(georg.fritzsche) → review+
Assignee | ||
Updated•11 years ago
|
Attachment #8405094 -
Flags: review?(georg.fritzsche) → review+
Assignee | ||
Comment 56•11 years ago
|
||
Comment on attachment 8404964 [details]
Part 13: Ability to ignore hash verification
r+, with ideally changed to using a pref.
Attachment #8404964 -
Flags: review?(georg.fritzsche) → review+
Assignee | ||
Updated•11 years ago
|
Attachment #8404370 -
Flags: review+
Updated•11 years ago
|
Whiteboard: [leave open] p=8 s=it-31c-30a-29b.2 [qa-] → [leave open] p=8 s=it-31c-30a-29b.3 [qa-]
Updated•11 years ago
|
Assignee: gps → georg.fritzsche
Assignee | ||
Comment 57•11 years ago
|
||
This landed up to part 11. I got part 12 to part 17 fixed up and pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=492051acc4af
Assignee | ||
Comment 58•11 years ago
|
||
Fixes issues i stumbled upon via rebase issues today:
* we shouldn't throw errors as part of a normal uninit()
* we should probably not skip pending cache saves on shutdown
https://tbpl.mozilla.org/?tree=Try&rev=206c7e911fa4
Attachment #8407607 -
Flags: review?(benjamin)
Comment 59•11 years ago
|
||
Comment on attachment 8407607 [details] [diff] [review]
Part 18: Fix experiments uninit behavior.
I'm fine with all of this except for the changes to _main.
In _main, the finally clause could eat real exceptions (if _saveToCache threw an error). I'm also skeptical that in general we should be writing the cache if there was an error here. Does this patch without the _main changes solve the test errors? r=me without that if so, or we should talk about changing the finally clause here to `catch (e if e instanceof AlreadyShutdownError)`
Attachment #8407607 -
Flags: review?(benjamin) → review+
Reporter | ||
Comment 60•11 years ago
|
||
Comment 61•11 years ago
|
||
Assignee | ||
Comment 62•11 years ago
|
||
* backed out the conflicting landings from comment 60 in favor of a known-good patch-stack that includes all the remaining patches here
* dropped the _main changes in part 18 that were discussed in comment 59
* should be done here now!
https://hg.mozilla.org/integration/fx-team/rev/b960c2026325
https://hg.mozilla.org/integration/fx-team/rev/cb9a5809f673
https://hg.mozilla.org/integration/fx-team/rev/9d5d5da02bf2
https://hg.mozilla.org/integration/fx-team/rev/27e2a7c2086e
https://hg.mozilla.org/integration/fx-team/rev/1648035f5d7b
https://hg.mozilla.org/integration/fx-team/rev/71fe0bf5ef84
https://hg.mozilla.org/integration/fx-team/rev/57bc0a610103
https://hg.mozilla.org/integration/fx-team/rev/36bc9e63aba7
Whiteboard: [leave open] p=8 s=it-31c-30a-29b.3 [qa-] → p=8 s=it-31c-30a-29b.3 [qa-]
Assignee | ||
Comment 63•11 years ago
|
||
Hm, and now i see that the imported patches didn't have the proper author information :(
I'm not keen on going back for this, let me know if i should.
Comment 65•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b960c2026325
https://hg.mozilla.org/mozilla-central/rev/cb9a5809f673
https://hg.mozilla.org/mozilla-central/rev/9d5d5da02bf2
https://hg.mozilla.org/mozilla-central/rev/27e2a7c2086e
https://hg.mozilla.org/mozilla-central/rev/1648035f5d7b
https://hg.mozilla.org/mozilla-central/rev/71fe0bf5ef84
https://hg.mozilla.org/mozilla-central/rev/57bc0a610103
https://hg.mozilla.org/mozilla-central/rev/36bc9e63aba7
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Updated•11 years ago
|
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•