Make use of Scoped.h

RESOLVED FIXED in mozilla14

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

unspecified
mozilla14
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 11 obsolete attachments)

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!
Created attachment 599147 [details] [diff] [review]
Experimental patch

Attaching an experimental patch. Everything compiles, tests in progress.
Created attachment 599252 [details] [diff] [review]
Experimental patch

Ok, this seems to work.
Attachment #599147 - Attachment is obsolete: true

Comment 3

5 years ago
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)
Created attachment 600979 [details] [diff] [review]
Experimental patch

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+
Created attachment 601907 [details] [diff] [review]
Experimental patch

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
yes

phone
Created attachment 601918 [details] [diff] [review]
Variant, withoutnon-const get()

Attaching a variant matching a proposed change to Scoped.h without non-|const| |get()| and |operator->|. Which one do you think is best?
Created attachment 612221 [details] [diff] [review]
Using Scoped.h throughout the code
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.
Created attachment 613570 [details] [diff] [review]
Using Scoped.h throughout the code

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+
Created attachment 613910 [details] [diff] [review]
Using Scoped.h throughout the code
Assignee: general → dteller
Attachment #613570 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Created attachment 613960 [details] [diff] [review]
Using Scoped.h throughout the code

Somehow, when I call a socket a "socker", they take offense.
Attachment #613910 - Attachment is obsolete: true
Created attachment 613961 [details] [diff] [review]
Using Scoped.h throughout the code
Attachment #613960 - Attachment is obsolete: true

Comment 15

5 years ago
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
Created attachment 614045 [details] [diff] [review]
Using Scoped.h throughout the code
Attachment #613961 - Attachment is obsolete: true
Keywords: checkin-needed
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 → ---
Keywords: checkin-needed
Please post an unbitrotted patch (preferably with a passing Try run).
Keywords: checkin-needed
Created attachment 614324 [details] [diff] [review]
Using Scoped.h throughout the code

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

Comment 22

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/4058f038e97f backed out in https://hg.mozilla.org/mozilla-central/rev/084cc1528d4f.
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.

Comment 28

5 years ago
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.

Comment 30

5 years ago
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
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 745291
You need to log in before you can comment on or make changes to this bug.