Closed Bug 1092311 Opened 5 years ago Closed 5 years ago

Fix IndexedDB profiler helpers

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached patch Patch, v0.1 (obsolete) — Splinter Review
This broke in bug 994190. Patch is mostly done, but still have a little bit to work out.
Attached patch Patch, v1 (obsolete) — Splinter Review
This should fix everything. The logging will be built in by default, piped to NSPR (so, subject to the NSPR environment variables), and the profiler markers can be enabled via preference changes.
Assignee: nobody → bent.mozilla
Attachment #8515198 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8520337 - Flags: review?(khuey)
Attached patch Patch, v1Splinter Review
Oops, other was a slightly older version. This is the right one.
Attachment #8520337 - Attachment is obsolete: true
Attachment #8520337 - Flags: review?(khuey)
Attachment #8520342 - Flags: review?(khuey)
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #1)
> This should fix everything. The logging will be built in by default, piped
> to NSPR (so, subject to the NSPR environment variables), and the profiler
> markers can be enabled via preference changes.

It's super awesome that the intent is for all builds to be PR_LOG enabled!

It looks like prlog.h still has the conditional in order to turn anything on:
  #if defined(DEBUG) || defined(FORCE_PR_LOG)

Should there be a FORCE_PR_LOG at the top of the profiler header or as a define in the moz.build or something?
(In reply to Andrew Sutherland [:asuth] from comment #3)
> Should there be a FORCE_PR_LOG at the top of the profiler header or as a
> define in the moz.build or something?

I believe that was handled in bug 806819.
I misse(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #4)
> (In reply to Andrew Sutherland [:asuth] from comment #3)
> > Should there be a FORCE_PR_LOG at the top of the profiler header or as a
> > define in the moz.build or something?
> 
> I believe that was handled in bug 806819.

I missed that, thanks much for linking me to the relevant bug!  This will save me a lot of hassle next time I suspect something going wrong in mozStorage too.
Comment on attachment 8520342 [details] [diff] [review]
Patch, v1

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

::: dom/indexedDB/IDBObjectStore.h
@@ +48,5 @@
>    : public nsISupports
>    , public nsWrapperCache
>  {
> +  // For AddOrPut() and DeleteInternal().
> +  friend class IDBCursor; 

moar whitespace

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +774,5 @@
> +    sLoggingMode = Logging_Disabled;
> +    return;
> +  }
> +
> +  bool useProfiler = 

whitespace at EOL

::: dom/indexedDB/PBackgroundIDBSharedTypes.ipdlh
@@ +259,5 @@
> +struct LoggingInfo
> +{
> +  nsID backgroundChildLoggingId;
> +  int64_t nextTransactionSerialNumber;
> +  int64_t nextVersionChangeTransactionSerialNumber;

Comment that this is negative.

::: dom/indexedDB/ProfilerHelpers.h
@@ +68,5 @@
> +    // NSID_LENGTH counts the null terminator, SetLength() does not.
> +    SetLength(NSID_LENGTH - 1);
> +
> +    aID.ToProvidedString(
> +      *reinterpret_cast<char(*)[NSID_LENGTH]>(BeginWriting()));

OMGWTFBBQ
Attachment #8520342 - Flags: review?(khuey) → review+
Attached patch InterdiffSplinter Review
This has r=khuey.
Attachment #8535363 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/306a2f060615
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Depends on: 1111370
You need to log in before you can comment on or make changes to this bug.