Closed Bug 1362119 Opened 2 years ago Closed 2 years ago

Moving ScriptLoader in dom/script and split the code in multiple files

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(3 files, 1 obsolete file)

No description provided.
Attached patch part 1 - dom/script (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8864584 - Flags: review?(ehsan)
Attached patch part 2 - splitSplinter Review
Attachment #8864585 - Flags: review?(ehsan)
Attachment #8864586 - Flags: review?(ehsan)
This isn't touching worker script loader yet, right?  HoPang has some patches in progress against the worker script loader that it would be nice not to conflict with.
No workers involved at the moment.
Attachment #8864584 - Attachment is obsolete: true
Attachment #8864584 - Flags: review?(ehsan)
Attachment #8864820 - Flags: review?(ehsan)
Sorry Andrea something else came up today which I couldn't push further back.  I'll try to review your patches during the weekend since I may again not be able to get back to them next week either...
Comment on attachment 8864820 [details] [diff] [review]
part 1 - dom/script

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

::: dom/script/ScriptLoader.h
@@ +72,3 @@
>                        uint32_t aVersion,
>                        mozilla::CORSMode aCORSMode,
>                        const mozilla::dom::SRIMetadata &aIntegrity)

Nit: indentation has been changed like this in a bunch of places, do you mind having a look over the patches and fixing things where you can?  The patches are way too large for me to nit on every single indentation issue and probably not really worth your time to fix each and every single issue either.

I really *really* wish we'd do automatic code formatting already.  :/
Attachment #8864820 - Flags: review?(ehsan) → review+
Attachment #8864585 - Flags: review?(ehsan) → review+
Attachment #8864586 - Flags: review?(ehsan) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d77f6b14633
part 1 - Moving dom/base/Script{Loader,Element}.* in dom/script, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8abdd77a92c
part 2 - Splitting ScriptLoader in multiple files, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/30104bffcd4b
part 2 - Fixing the code style in dom/script, r=ehsan
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c440a4d4cc6c
Backed out changeset 30104bffcd4b 
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a935b38ef45
Backed out changeset a8abdd77a92c 
https://hg.mozilla.org/integration/mozilla-inbound/rev/62680dc80fb7
Backed out changeset 5d77f6b14633 for android bustage in nsCCUncollectableMarker.cpp:500:7: error: 'TraceScriptHolder' is not a member of 'mozilla'
note: [task 2017-05-08T08:08:57.863366Z] 08:08:57     INFO - GECKO(3072) | [Child 3128] ###!!! ASSERTION: Destroying a currently-showing document: '!mIsShowing', file /home/worker/workspace/build/src/dom/base/nsDocument.cpp, line 1490 

might be also related - https://treeherder.mozilla.org/logviewer.html#?job_id=97237424&repo=mozilla-inbound&lineNumber=9076
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0788c41e592b
part 1 - Moving dom/base/Script{Loader,Element}.* in dom/script, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/692529b223ec
part 2 - Splitting ScriptLoader in multiple files, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0b31688e9f3
part 2 - Fixing the code style in dom/script, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/1054eb4b2782
part 4 - Fixing includes, r=me
Flags: needinfo?(amarchesini)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.