Closed Bug 728171 Opened 12 years ago Closed 12 years ago

Make use of Scoped.h

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(1 file, 11 obsolete files)

18.87 KB, patch
Details | Diff | Splinter Review
Bug 718938 introduces mfbt/Scoped.h and a number of general-purpose auto_ptr-style utilities. Let's use them!
Attached patch Experimental patch (obsolete) — Splinter Review
Attaching an experimental patch. Everything compiles, tests in progress.
Attached patch Experimental patch (obsolete) — Splinter Review
Ok, this seems to work.
Attachment #599147 - Attachment is obsolete: true
Try run for 872d1fb41e05 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=872d1fb41e05
Results (out of 217 total builds):
    exception: 3
    success: 180
    warnings: 13
    failure: 21
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dteller@mozilla.com-872d1fb41e05
Attachment #599252 - Flags: feedback?(jones.chris.g)
Attached patch Experimental patch (obsolete) — Splinter Review
Same patch, with slight clean-up in PluginModuleParent.cpp.
Attachment #600979 - Flags: review?(jones.chris.g)
Attachment #599252 - Attachment is obsolete: true
Attachment #599252 - Flags: feedback?(jones.chris.g)
Comment on attachment 600979 [details] [diff] [review]
Experimental patch

>diff --git a/xpcom/glue/FileUtils.h b/xpcom/glue/FileUtils.h

>+struct ScopedClosePRFDTraits

>+  static void release(PRFileDesc* fd) {
>+    if (fd) {
>+      PR_Close(fd);
>+    }

This check is unnecessary.

r=me with that fix.
Attachment #600979 - Flags: review?(jones.chris.g) → review+
Attached patch Experimental patch (obsolete) — Splinter Review
Attaching a new version. I cleaned up FileUtils.h a little, and I propagating a tiny change from Scoped.h to dom/plugins/ipc/PluginModuleParent.cpp. \n Chris, do you think that we should land this patch once Scoped.h is landed?
Attachment #600979 - Attachment is obsolete: true
Attached patch Variant, withoutnon-const get() (obsolete) — Splinter Review
Attaching a variant matching a proposed change to Scoped.h without non-|const| |get()| and |operator->|. Which one do you think is best?
Attachment #601907 - Attachment is obsolete: true
Attachment #601918 - Attachment is obsolete: true
Updating to reflect the changes in Scoped.h and in the rest of m-c.
Updated to take into account changes in parent bug and to m-c.

Chris, is your r+ sufficient to land this? Otherwise, who should I contact?
Attachment #612221 - Attachment is obsolete: true
Attachment #613570 - Flags: review?(jones.chris.g)
Attachment #613570 - Flags: review?(jones.chris.g) → review+
Assignee: general → dteller
Attachment #613570 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Somehow, when I call a socket a "socker", they take offense.
Attachment #613910 - Attachment is obsolete: true
Attachment #613960 - Attachment is obsolete: true
Try run for 6a632ac82fe1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6a632ac82fe1
Results (out of 221 total builds):
    exception: 1
    success: 187
    warnings: 32
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dteller@mozilla.com-6a632ac82fe1
Attachment #613961 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e03eb171e08
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla14
And a bustage fix for poor un-bitrotting on my part.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4058f038e97f
(In reply to Ryan VanderMeulen from comment #18)
> And a bustage fix for poor un-bitrotting on my part.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4058f038e97f

Backed out per Yoric for bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/084cc1528d4f
Target Milestone: mozilla14 → ---
Please post an unbitrotted patch (preferably with a passing Try run).
Keywords: checkin-needed
I have some difficulties understanding what happened. I have just pulled and my patch seems to apply cleanly. Michael Wu pointed another error, tough, so I take the opportunity to fix it.

According to TryServer, that patch builds on B2G. Currently running it on all platforms.
Attachment #614045 - Attachment is obsolete: true
Try run for 6951c5ef44eb is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6951c5ef44eb
Results (out of 15 total builds):
    exception: 2
    success: 11
    warnings: 1
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dteller@mozilla.com-6951c5ef44eb
https://hg.mozilla.org/mozilla-central/rev/0e03eb171e08
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
I have difficulties following: is it in or out?
So the patch is actually out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Ok, today, I finally have a merge issue. I suspect that the previous merge issue was only on inbound, not on master.

Launching one (last?) TryServer run.
Try run for fda720573a26 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=fda720573a26
Results (out of 2 total builds):
    success: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dteller@mozilla.com-fda720573a26
(In reply to David Rajchenbach Teller [:Yoric] from comment #26)
> So the patch is actually out.

To confirm: I backed out 4058f038e97f and 0e03eb171e08, so yes, this was backed out.
Try run for 85d3f2f97ab2 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=85d3f2f97ab2
Results (out of 15 total builds):
    success: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dteller@mozilla.com-85d3f2f97ab2
Crossing fingers...
Keywords: checkin-needed
*fingers crossed* (and landed by itself for easier backouts :P)
https://hg.mozilla.org/integration/mozilla-inbound/rev/db23b2dac69c

Ril.cpp still had bitrot issues, but I made sure to be more careful about it this time.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/db23b2dac69c
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Depends on: 745291
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: