The default bug view has changed. See this FAQ.

Remove unnecessary query of nsIURI to nsIURL when opening attachment

RESOLVED FIXED in Thunderbird 53.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: Jorg K (GMT+1), Assigned: Jorg K (GMT+1))

Tracking

(Blocks: 1 bug)

52 Branch
Thunderbird 53.0

Thunderbird Tracking Flags

(thunderbird52 fixed, thunderbird53 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 months ago
Remove redundant query of nsURI to nsURI. Patch coming.
(Assignee)

Comment 1

3 months ago
Oops, that wasn't quite right, this describes it better:
Remove unnecessary query of nsIURI to nsIURL when opening attachment.

The problem came up when using add-on "Shrunked Image Resizer" with attaches attached images as data URIs now. Such a URI cannot be opened since it can't be QI'ed to an nsURL.
Summary: Remove redundant query of nsURI to nsURI → Remove unnecessary query of nsIURI to nsIURL when opening attachment
(Assignee)

Comment 2

3 months ago
Created attachment 8821679 [details] [diff] [review]
1325713-remove-QI-url.patch (v1).
Attachment #8821679 - Flags: review?(acelists)
(Assignee)

Comment 3

3 months ago
Green try here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6eeb14c76ee28a38e5d8793e357bf099d5eba657
(Assignee)

Updated

3 months ago
Blocks: 1322155
(Assignee)

Updated

3 months ago
Assignee: nobody → jorgk
Status: NEW → ASSIGNED

Comment 4

3 months ago
Comment on attachment 8821679 [details] [diff] [review]
1325713-remove-QI-url.patch (v1).

Review of attachment 8821679 [details] [diff] [review]:
-----------------------------------------------------------------

This seems like the correct thing to do, newChannelFromURI2 takes an nsIURI.

What about the other nsIURL occurence in loadBlockedImage() ?
(Assignee)

Comment 5

3 months ago
Comment on attachment 8821679 [details] [diff] [review]
1325713-remove-QI-url.patch (v1).

Aceman, thanks for the quick action here.

This is Magnus's new code, so let's ask him.

Magnus, why do you query this so an nsIRUL here:
https://dxr.mozilla.org/comm-central/rev/b5cbf4a5f294c84cee905a0e22c32ea34cc66359/mail/components/compose/content/MsgComposeCommands.js#5666
Should we just lose the
  url = url.QueryInterface(Components.interfaces.nsIURL); ?
Flags: needinfo?(mkmelin+mozilla)

Comment 6

3 months ago
If it works without, you can lose it. I probably copied the pattern from somewhere...
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 7

3 months ago
Created attachment 8822199 [details] [diff] [review]
1325713-remove-QI-url.patch (v2).

OK, removed another unnecessary QI call. It still works ;-)
Attachment #8821679 - Attachment is obsolete: true
Attachment #8821679 - Flags: review?(acelists)
Attachment #8822199 - Flags: review?(acelists)

Comment 8

3 months ago
Comment on attachment 8822199 [details] [diff] [review]
1325713-remove-QI-url.patch (v2).

Review of attachment 8822199 [details] [diff] [review]:
-----------------------------------------------------------------

Ok.
Attachment #8822199 - Flags: review?(acelists) → review+
(Assignee)

Comment 9

3 months ago
Comment on attachment 8822199 [details] [diff] [review]
1325713-remove-QI-url.patch (v2).

This needs to go to Aurora since the original patch landed on TB 52.
Attachment #8822199 - Flags: approval-comm-aurora+
(Assignee)

Updated

3 months ago
Keywords: checkin-needed
(Assignee)

Comment 10

3 months ago
https://hg.mozilla.org/comm-central/rev/bcbc94d7ff6c354fcadfa2717a66afdd5e17cdd8
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-thunderbird52: --- → affected
status-thunderbird53: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
(Assignee)

Comment 11

3 months ago
Aurora (TB 52):
https://hg.mozilla.org/releases/comm-aurora/rev/dc9b6f9bb194387a21508832d0c02eed865ea70e
status-thunderbird52: affected → fixed
You need to log in before you can comment on or make changes to this bug.