Closed Bug 1333493 Opened 3 years ago Closed 3 years ago

add BUG_COMPONENT to js/* files

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jmaher, Assigned: jmaher)

References

Details

Attachments

(2 files, 2 obsolete files)

In order to file bugs in the right locations and to establish a point of contact for source/test files, it is important to know what bugzilla component each file should be associated with.  I looked through some of the files and tried to figure it out, here is what I have:

Core::JavaScript Engine
js/ductwork/*
js/examples/*
js/public/*

Core::JavaScript Engine, or JavaScript Engine: GC ?
js/ipc/*

Core::XPConnect?
js/xpconnect/*

js/src/ has a lot of subdirs, I suspect those could be split up more accordingly. I sort of gave up here.


Here are some bugzilla components which I believe might be interesting to ensure we have source/test files mapped to:
Core::JavaScript Engine
Core::JavaScript Engine: GC
Core::JavaScript Engine: JIT
Core::JavaScript Engine: Standard Library
Core::JavaScript Engine: Internationalization API
Core::js-ctypes
:jorendorff, can you help me out and recommend bugzilla components for either specific files, file patterns, or sub directories?
Flags: needinfo?(jorendorff)
For what it's worth, /js/src/moz.build already contains a few mappings we made in the past.
excellent, possibly we can quickly validate those and ensure the other files under js/* have a reasonable component.
Attached patch add BUG_COMPONENT to js/* (obsolete) — Splinter Review
proposed patch in case the answer is simple :)
Assignee: nobody → jmaher
Status: NEW → ASSIGNED
In addition to what's in this patch:

js/src/jit/** => JavaScript Engine: JIT
js/src/gc/** => JavaScript: GC
js/src/builtin/Intl* => JavaScript: Internationalization API
js/src/builtin/** => JavaScript: Standard Library
Flags: needinfo?(jorendorff)
oh, in fact those are already in place for js/src:
https://dxr.mozilla.org/mozilla-central/source/js/src/moz.build

that is cool!

What is missing is some of the top level files and the public/ directory- I am happy to put everything else as Core::JavaScript Engine if that makes sense.
(In reply to Joel Maher ( :jmaher) from comment #6)
> What is missing is some of the top level files and the public/ directory- I
> am happy to put everything else as Core::JavaScript Engine if that makes
> sense.

Sounds good. Thanks.
Attached patch add BUG_COMPONENT to js/* (obsolete) — Splinter Review
thanks for the feedback, this patch should do the trick!
Attachment #8831161 - Attachment is obsolete: true
Attachment #8833284 - Flags: review?(jorendorff)
checking in here- it has been 2 weeks...feel free to redirect the review as needed.
:naveed- can you help me find someone to review this patch and the patch in bug 1344878?
Flags: needinfo?(nihsanullah)
I did the initial work for js/src, I'll steal the review to help things move forward.
Flags: needinfo?(nihsanullah)
Comment on attachment 8833284 [details] [diff] [review]
add BUG_COMPONENT to js/*

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

Looks good to me, thanks.

::: js/ductwork/debugger/moz.build
@@ +22,5 @@
>  
>  FINAL_LIBRARY = 'xul'
> +
> +with Files("**"):
> +    BUG_COMPONENT = ("Core", "JavaScript Engine")

I've got a nice preference for keeping this at the top of the file, like for other files (js/src/moz.build in particular).

::: js/moz.build
@@ +1,2 @@
> +with Files("**"):
> +    BUG_COMPONENT = ("Core", "JavaScript Engine")

For the case of js/public, which doesn't have a moz.build file, we decided to use a relative reference in js/src/moz.build: `with Files("../public"):`
Would it make sense to do the same here, to prevent scattering?
Attachment #8833284 - Flags: review?(jorendorff) → review+
thanks for the notes, I have moved the work to the top of the file, but on the second point, I see some issues:

it appears the ../ notation doesn't work for BUG_COMPONENTS:

$ ./mach file-info bugzilla-component js/public/**
UNKNOWN
  js/public/CallArgs.h
  js/public/CallNonGenericMethod.h
  js/public/CharacterEncoding.h
  js/public/Class.h
  js/public/Conversions.h
  js/public/Date.h
  js/public/Debug.h
  js/public/GCAPI.h
  js/public/GCAnnotations.h
  js/public/GCHashTable.h
  js/public/GCPolicyAPI.h
  js/public/GCVariant.h
  js/public/GCVector.h
  js/public/HashTable.h
  js/public/HeapAPI.h
  js/public/Id.h
  js/public/Initialization.h
  js/public/LegacyIntTypes.h
  js/public/MemoryMetrics.h
  js/public/Principals.h
  js/public/ProfilingFrameIterator.h
  js/public/ProfilingStack.h
  js/public/Proxy.h
  js/public/Realm.h
  js/public/RefCounted.h
  js/public/RequiredDefines.h
  js/public/Result.h
  js/public/RootingAPI.h
  js/public/SliceBudget.h
  js/public/StructuredClone.h
  js/public/SweepingAPI.h
  js/public/TraceKind.h
  js/public/TracingAPI.h
  js/public/TrackedOptimizationInfo.h
  js/public/TypeDecls.h
  js/public/UbiNode.h
  js/public/UbiNodeBreadthFirst.h
  js/public/UbiNodeCensus.h
  js/public/UbiNodeDominatorTree.h
  js/public/UbiNodePostOrder.h
  js/public/UbiNodeShortestPaths.h
  js/public/UniquePtr.h
  js/public/Utility.h
  js/public/Value.h
  js/public/Vector.h
  js/public/WeakMapPtr.h


I took a stab at moving all of existing ../public references to the new root level js/moz.build file.  I know you wanted to avoid that which I tried to do until I realized it wasn't working for ../public.

If you are not comfortable with this, possibly we can look at working on the moz.build code for handing BUG_COMPONENTS.
Attachment #8833284 - Attachment is obsolete: true
Attachment #8850464 - Flags: review?(bbouvier)
forgot to pick up recent changes, this looks right
Attachment #8850464 - Attachment is obsolete: true
Attachment #8850464 - Flags: review?(bbouvier)
Attachment #8850470 - Flags: review?(bbouvier)
Comment on attachment 8850464 [details] [diff] [review]
add BUG_COMPONENT to js/*

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

Looks fine to me; would the lines for public/ files from js/src/moz.build need to be removed, then? (rs=me for that) Thanks!

::: js/moz.build
@@ +4,5 @@
> +
> +with Files("**"):
> +    BUG_COMPONENT = component_engine
> +
> +with Files("examples/**"):

Out of curiosity, doesn't the line `with Files("**")` already cover the examples, ipc and public directories? (as well as ductwork/debugger)

@@ +13,5 @@
> +
> +with Files('public/**'):
> +    BUG_COMPONENT = component_engine
> +
> +for header in ('GCAnnotations.h', 'GCAPI.h', 'HeapAPI.h', 'RootingAPI.h', 'SliceBudget.h', 'SweepingAPI.h', 'TraceKind.h', 'TracingAPI.h', 'WeakMapPtr.h'):

Can you please add here: GCHashTable.h, GCPolicyAPI.h, GCVariant.h, GCVector.h?

@@ +20,5 @@
> +
> +with Files('public/TrackedOptimizationInfo.h'):
> +    BUG_COMPONENT = component_jit
> +
> +

nit: 2 unnecessary blank lines at EOF
Attachment #8850464 - Attachment is obsolete: false
Comment on attachment 8850470 [details] [diff] [review]
add BUG_COMPONENT to js/*

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

oops, mid-air collision; see above comments, some of which still apply here. Thanks!
Attachment #8850470 - Flags: review?(bbouvier) → review+
Pushed by jmaher@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91d1dce57468
add BUG_COMPONENT to js/* files. r=bbouvier
https://hg.mozilla.org/mozilla-central/rev/91d1dce57468
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.