Last Comment Bug 776399 - undo incompatible IDL changes done in bug 663057
: undo incompatible IDL changes done in bug 663057
Status: RESOLVED FIXED
:
Product: Core Graveyard
Classification: Graveyard
Component: File Handling (show other bugs)
: unspecified
: All All
: -- major (vote)
: mozilla17
Assigned To: Julian Reschke
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-22 14:05 PDT by Julian Reschke
Modified: 2016-06-22 12:16 PDT (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
+
fixed


Attachments
Proposed patch (mozilla-central) (5.95 KB, patch)
2012-07-23 01:48 PDT, Julian Reschke
no flags Details | Diff | Splinter Review
Proposed patch (aurora) (5.95 KB, patch)
2012-07-23 02:23 PDT, Julian Reschke
bzbarsky: review-
Details | Diff | Splinter Review
Proposed patch (beta) (5.95 KB, patch)
2012-07-23 02:24 PDT, Julian Reschke
bzbarsky: review-
Details | Diff | Splinter Review
Proposed patch (5.35 KB, patch)
2012-07-24 06:57 PDT, Julian Reschke
bzbarsky: review+
Details | Diff | Splinter Review
Proposed patch (5.36 KB, patch)
2012-07-24 08:12 PDT, Julian Reschke
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Proposed patch (aurora) (5.36 KB, patch)
2012-07-24 11:55 PDT, Julian Reschke
no flags Details | Diff | Splinter Review

Description Julian Reschke 2012-07-22 14:05:10 PDT
The changes for bug 663057 have changed the IDL for nsIMIMEHeaderParam incompatibly.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2012-07-22 14:06:46 PDT
Julian, do you have time to deal with this, or should I?
Comment 2 Julian Reschke 2012-07-22 14:18:20 PDT
(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.
Comment 3 Julian Reschke 2012-07-23 01:48:51 PDT
Created attachment 644867 [details] [diff] [review]
Proposed patch (mozilla-central)

try results: https://tbpl.mozilla.org/?tree=Try&rev=e21ac319d8b6
Comment 4 Julian Reschke 2012-07-23 02:23:50 PDT
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
Comment 5 Julian Reschke 2012-07-23 02:24:45 PDT
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
Comment 6 Julian Reschke 2012-07-23 02:26:18 PDT
Posted patches for central/aurora/beta separately (although the changes identical; dunno what's better process-wise).
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2012-07-23 19:04:24 PDT
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 8 Boris Zbarsky [:bz] (still a bit busy) 2012-07-23 19:05:13 PDT
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.)
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2012-07-23 19:05:31 PDT
Comment on attachment 644876 [details] [diff] [review]
Proposed patch (aurora)

r- for Aurora.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2012-07-23 19:05:46 PDT
Comment on attachment 644877 [details] [diff] [review]
Proposed patch (beta)

r- for beta
Comment 11 Julian Reschke 2012-07-24 00:04:41 PDT
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 12 Boris Zbarsky [:bz] (still a bit busy) 2012-07-24 06:45:40 PDT
Comment on attachment 644867 [details] [diff] [review]
Proposed patch (mozilla-central)

Er, I meant to just unset the flag, not set it to +.  ;)
Comment 13 Julian Reschke 2012-07-24 06:57:56 PDT
Created attachment 645281 [details] [diff] [review]
Proposed patch

A patch that makes the new parameter optional, and lets it default to "true".
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2012-07-24 07:06:55 PDT
Comment on attachment 645281 [details] [diff] [review]
Proposed patch

s/aArgc/aOptionalArgc/ and r=me
Comment 15 Julian Reschke 2012-07-24 08:12:42 PDT
Created attachment 645309 [details] [diff] [review]
Proposed patch
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2012-07-24 08:38:05 PDT
Comment on attachment 645309 [details] [diff] [review]
Proposed patch

r=me
Comment 17 Julian Reschke 2012-07-24 08:39:40 PDT
Comment on attachment 645309 [details] [diff] [review]
Proposed patch

Try submission: https://tbpl.mozilla.org/?tree=Try&rev=357d0d07d1fc
Comment 18 Julian Reschke 2012-07-24 11:55:29 PDT
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:
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2012-07-24 12:08:49 PDT
Isn't that the same patch?  Just ask for approval on the already-existing patch, if so.
Comment 20 Julian Reschke 2012-07-24 12:22:24 PDT
(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 21 Julian Reschke 2012-07-24 12:23:47 PDT
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:
Comment 22 Julian Reschke 2012-07-24 13:28:38 PDT
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:
Comment 23 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-24 13:48:38 PDT
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.
Comment 24 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-24 13:49:27 PDT
Comment on attachment 645413 [details] [diff] [review]
Proposed patch (aurora)

(cleaning up unnecessary approval flags)
Comment 25 Ryan VanderMeulen [:RyanVM] 2012-07-24 18:36:20 PDT
Comment on attachment 645309 [details] [diff] [review]
Proposed patch

Just use checkin-needed.
Comment 26 Ryan VanderMeulen [:RyanVM] 2012-07-24 19:07:29 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c521e0a117e
Comment 27 Ed Morley [:emorley] 2012-07-25 08:10:29 PDT
https://hg.mozilla.org/mozilla-central/rev/1c521e0a117e
Comment 28 Alex Keybl [:akeybl] 2012-07-26 14:56:59 PDT
Comment on attachment 645309 [details] [diff] [review]
Proposed patch

[Triage Comment]
Now that Comment 7 has been addressed, approving for branches.
Comment 29 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-02 12:03:30 PDT
Can someone land this on Julian's behalf to the branches?
Comment 30 Ryan VanderMeulen [:RyanVM] 2012-08-02 12:15:10 PDT
Do the branch patches have r+? If so, put checkin-needed in the whiteboard.
Comment 31 Boris Zbarsky [:bz] (still a bit busy) 2012-08-02 12:33:56 PDT
Yes, because they're the same exact patch.  ;)
Comment 32 Ryan VanderMeulen [:RyanVM] 2012-08-02 12:36:15 PDT
Comment on attachment 644877 [details] [diff] [review]
Proposed patch (beta)

In that case, marking obsolete patches as such is helpful.

Note You need to log in before you can comment on or make changes to this bug.