Closed Bug 776399 Opened 8 years ago Closed 8 years ago

undo incompatible IDL changes done in bug 663057

Categories

(Core Graveyard :: File Handling, defect, major)

defect
Not set
major

Tracking

(firefox15+ fixed, firefox16+ fixed)

RESOLVED FIXED
mozilla17
Tracking Status
firefox15 + fixed
firefox16 + fixed

People

(Reporter: julian.reschke, Assigned: julian.reschke)

Details

Attachments

(1 file, 5 obsolete files)

The changes for bug 663057 have changed the IDL for nsIMIMEHeaderParam incompatibly.
Julian, do you have time to deal with this, or should I?
(In reply to Boris Zbarsky (:bz) from comment #1)
> Julian, do you have time to deal with this, or should I?

I'm currently checking out beta/aurora/central, and try to fix and test all of these tomorrow. Hopefully the same patch will work on all of these.
Attached patch Proposed patch (mozilla-central) (obsolete) — Splinter Review
try results: https://tbpl.mozilla.org/?tree=Try&rev=e21ac319d8b6
Attachment #644867 - Flags: review?(bzbarsky)
Attached patch Proposed patch (aurora) (obsolete) — Splinter Review
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 776399
User impact if declined: n/a (IDL change backed out)
Testing completed (on m-c, etc.): https://tbpl.mozilla.org/?tree=Try&rev=e21ac319d8b6 (m-c)
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none
Attachment #644876 - Flags: review?(bzbarsky)
Attachment #644876 - Flags: approval-mozilla-aurora?
Attached patch Proposed patch (beta) (obsolete) — Splinter Review
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 776399
User impact if declined: n/a (IDL change backed out)
Testing completed (on m-c, etc.): https://tbpl.mozilla.org/?tree=Try&rev=e21ac319d8b6 (m-c)
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none
Attachment #644877 - Flags: review?(bzbarsky)
Attachment #644877 - Flags: approval-mozilla-beta?
Posted patches for central/aurora/beta separately (although the changes identical; dunno what's better process-wise).
No, this doesn't work.  At this point this would be an incompatible change from what we've already shipped on aurora/beta, which is just as bad, if not worse.

Again, I think the right way to make this work is to make aAllowSubstitution an optional argument and use optional_argc to allow it to default to true...
Comment on attachment 644867 [details] [diff] [review]
Proposed patch (mozilla-central)

Why are we making this exact change?

(Also, for future reference, uploading the one patch and setting the approval flags on it would have been fine.)
Attachment #644867 - Flags: review?(bzbarsky) → review+
Comment on attachment 644876 [details] [diff] [review]
Proposed patch (aurora)

r- for Aurora.
Attachment #644876 - Flags: review?(bzbarsky) → review-
Comment on attachment 644877 [details] [diff] [review]
Proposed patch (beta)

r- for beta
Attachment #644877 - Flags: review?(bzbarsky) → review-
Boris, you confused me with comments 7 and 8. If this is not the right thing to do, why r+ for the change in mozilla-central?
Comment on attachment 644867 [details] [diff] [review]
Proposed patch (mozilla-central)

Er, I meant to just unset the flag, not set it to +.  ;)
Attachment #644867 - Flags: review+
Attached patch Proposed patch (obsolete) — Splinter Review
A patch that makes the new parameter optional, and lets it default to "true".
Attachment #644867 - Attachment is obsolete: true
Attachment #645281 - Flags: review?(bzbarsky)
Comment on attachment 645281 [details] [diff] [review]
Proposed patch

s/aArgc/aOptionalArgc/ and r=me
Attachment #645281 - Flags: review?(bzbarsky) → review+
Attached patch Proposed patchSplinter Review
Attachment #645281 - Attachment is obsolete: true
Attachment #645309 - Flags: review?(bzbarsky)
Comment on attachment 645309 [details] [diff] [review]
Proposed patch

r=me
Attachment #645309 - Flags: review?(bzbarsky) → review+
Attachment #645309 - Flags: checkin?
Attached patch Proposed patch (aurora) (obsolete) — Splinter Review
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 663057
User impact if declined: n/a
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): n/a
String or UUID changes made by this patch:
Attachment #644876 - Attachment is obsolete: true
Attachment #644876 - Flags: approval-mozilla-aurora?
Attachment #645413 - Flags: review?(bzbarsky)
Attachment #645413 - Flags: approval-mozilla-aurora?
Isn't that the same patch?  Just ask for approval on the already-existing patch, if so.
(In reply to Boris Zbarsky (:bz) from comment #19)
> Isn't that the same patch?  Just ask for approval on the already-existing
> patch, if so.

OK, will do.
Comment on attachment 645309 [details] [diff] [review]
Proposed patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 663057
User impact if declined: n/a
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): n/a
String or UUID changes made by this patch:
Attachment #645309 - Flags: approval-mozilla-aurora?
Comment on attachment 645309 [details] [diff] [review]
Proposed patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 663057
User impact if declined: n/a
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): n/a
String or UUID changes made by this patch:
Attachment #645309 - Flags: approval-mozilla-beta?
Keywords: checkin-needed
Julian, assigning this to you so we don't have a tracked 15/16 bug with no one assigned to it. We'll wait for this to land on mozilla-central first before looking at approving for aurora/beta.
Assignee: nobody → julian.reschke
Attachment #644877 - Flags: approval-mozilla-beta?
Comment on attachment 645413 [details] [diff] [review]
Proposed patch (aurora)

(cleaning up unnecessary approval flags)
Attachment #645413 - Flags: approval-mozilla-aurora?
Comment on attachment 645309 [details] [diff] [review]
Proposed patch

Just use checkin-needed.
Attachment #645309 - Flags: checkin?
https://hg.mozilla.org/mozilla-central/rev/1c521e0a117e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment on attachment 645309 [details] [diff] [review]
Proposed patch

[Triage Comment]
Now that Comment 7 has been addressed, approving for branches.
Attachment #645309 - Flags: approval-mozilla-beta?
Attachment #645309 - Flags: approval-mozilla-beta+
Attachment #645309 - Flags: approval-mozilla-aurora?
Attachment #645309 - Flags: approval-mozilla-aurora+
Can someone land this on Julian's behalf to the branches?
Do the branch patches have r+? If so, put checkin-needed in the whiteboard.
Yes, because they're the same exact patch.  ;)
Keywords: checkin-needed
Attachment #645413 - Attachment is obsolete: true
Attachment #645413 - Flags: review?(bzbarsky)
Comment on attachment 644877 [details] [diff] [review]
Proposed patch (beta)

In that case, marking obsolete patches as such is helpful.
Attachment #644877 - Attachment is obsolete: true
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.