Closed Bug 1367515 Opened 5 years ago Closed 4 years ago

Use ::mozilla::Maybe to de-duplicate the loop of ScriptLoader::GiveUpBytecodeEncoding


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

Not set



Tracking Status
firefox55 --- fixed


(Reporter: nbp, Assigned: waterftw, Mentored)



(Keywords: good-first-bug)


(1 file, 2 obsolete files)

In the dom/script/ScriptLoader.cpp, the function named ScriptLoader::GiveUpBytecodeEncoding has 2 loops which are mostly doing the same things, apart from the initialization of the AutoEntryScript structure.

Using ::mozilla::Maybe defined in mbft/Maybe.h, wrap the AutoEntryScript structure such that it can be initialize in a branch, and used after, in a single loop which contain a branch around FinishIncrementalEncoding, which test if the AutoEntryScript got initialized.
Hi Nicolas, can you assign me this bug?
(In reply to Stanley [:waterftw] from comment #1)
> Hi Nicolas, can you assign me this bug?


If you need any extra guidance, you can ask on, on the #jsapi channel, either Blake (mrbkap) or me (nbp).
Assignee: nobody → stanleyye1996
Also not too sure about the review flags..
Attachment #8874267 - Flags: review?(nicolas.b.pierron)
Attachment #8874267 - Flags: feedback?(nicolas.b.pierron)
Comment on attachment 8874267 [details] [diff] [review]
Wrapped the RootedScript and AutoEntryScript classes inside a Maybe container

Review of attachment 8874267 [details] [diff] [review]:

::: dom/script/ScriptLoader.cpp
@@ +2190,5 @@
>    // removal of all request from the current list and these large buffers would
>    // be removed at the same time as the source object.
>    nsCOMPtr<nsIScriptGlobalObject> globalObject = GetScriptGlobalObject();
> +  Maybe<AutoEntryScript> aes;
> +  Maybe<JS::RootedScript> script;

nit: JS::RootedScript are creating a link list of Rooted Script which are on the same stack.  Having this class under a Maybe would be dangerous if we were to have another Rooted in the same frame, and if we were to not respect the RAII order.

Having a single JS::RootedScript should be fine, but for future-proof safety, I suggest to move the JS::RootedScript inside the if branch instead of having a Maybe<> surrounding it.
Attachment #8874267 - Flags: review?(nicolas.b.pierron)
Attachment #8874267 - Flags: review+
Attachment #8874267 - Flags: feedback?(nicolas.b.pierron)
Attachment #8874715 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8874715 [details] [diff] [review]
Revision 1:  Removed the Maybe container for RootedScript

Review of attachment 8874715 [details] [diff] [review]:

This diff looks good. Make a proper patch, such that your patch can be imported and landed for you, while keeping the authorship.

Here is the documentation for making a patch that someone else can commit on your behalf:

::: dom/script/ScriptLoader.cpp
@@ +2205,5 @@
>      TRACE_FOR_TEST_NONE(request->mElement, "scriptloader_bytecode_failed");
> +
> +    if (aes.isSome()) {
> +      JS::RootedScript script(aes->cx());
> +      script.set(request->mScript);

nit: JS::RootedScript script(aes->cx(), request->mScript);
Attachment #8874715 - Flags: review?(nicolas.b.pierron) → review+
Attached patch Bug1367515.patchSplinter Review
Attachment #8874267 - Attachment is obsolete: true
Attachment #8874715 - Attachment is obsolete: true
Attachment #8875182 - Flags: checkin?(nicolas.b.pierron)
Pushed by
Use ::mozilla::Maybe to de-duplicate the loop of ScriptLoader::GiveUpBytecodeEncoding r=nbp
Comment on attachment 8875182 [details] [diff] [review]

(based on comment 8)
Attachment #8875182 - Flags: review+
Attachment #8875182 - Flags: checkin?(nicolas.b.pierron)
Attachment #8875182 - Flags: checkin+
Thanks you for your contribution!
If you are interested in contributing again, you can find more mentored issues at
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.