undo incompatible IDL changes done in bug 663057

RESOLVED FIXED in Firefox 15

Status

Core Graveyard
File Handling
--
major
RESOLVED FIXED
5 years ago
10 months ago

People

(Reporter: Julian Reschke, Assigned: Julian Reschke)

Tracking

unspecified
mozilla17
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox15+ fixed, firefox16+ fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
The changes for bug 663057 have changed the IDL for nsIMIMEHeaderParam incompatibly.
Julian, do you have time to deal with this, or should I?
(Assignee)

Comment 2

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

Comment 3

5 years ago
Created attachment 644867 [details] [diff] [review]
Proposed patch (mozilla-central)

try results: https://tbpl.mozilla.org/?tree=Try&rev=e21ac319d8b6
Attachment #644867 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

5 years ago
Created attachment 644876 [details] [diff] [review]
Proposed patch (aurora)

[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?
(Assignee)

Comment 5

5 years ago
Created attachment 644877 [details] [diff] [review]
Proposed patch (beta)

[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?
(Assignee)

Comment 6

5 years ago
Posted patches for central/aurora/beta separately (although the changes identical; dunno what's better process-wise).

Updated

5 years ago
tracking-firefox15: ? → +
tracking-firefox16: ? → +
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-
(Assignee)

Comment 11

5 years ago
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+
(Assignee)

Comment 13

5 years ago
Created attachment 645281 [details] [diff] [review]
Proposed patch

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+
(Assignee)

Comment 15

5 years ago
Created attachment 645309 [details] [diff] [review]
Proposed patch
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+
(Assignee)

Comment 17

5 years ago
Comment on attachment 645309 [details] [diff] [review]
Proposed patch

Try submission: https://tbpl.mozilla.org/?tree=Try&rev=357d0d07d1fc
(Assignee)

Updated

5 years ago
Attachment #645309 - Flags: checkin?
(Assignee)

Comment 18

5 years ago
Created attachment 645413 [details] [diff] [review]
Proposed patch (aurora)

[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.
(Assignee)

Comment 20

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

Comment 21

5 years ago
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?
(Assignee)

Comment 22

5 years ago
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?
(Assignee)

Updated

5 years ago
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/integration/mozilla-inbound/rev/1c521e0a117e
Flags: in-testsuite-
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1c521e0a117e
Status: NEW → RESOLVED
Last Resolved: 5 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?
status-firefox15: --- → affected
status-firefox16: --- → affected
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
https://hg.mozilla.org/releases/mozilla-aurora/rev/c530848fdf53
https://hg.mozilla.org/releases/mozilla-beta/rev/2f42cf45a752
status-firefox15: affected → fixed
status-firefox16: affected → fixed
Keywords: checkin-needed
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.