Closed
Bug 1051221
Opened 10 years ago
Closed 10 years ago
Saving MathML source fails - view source shouldn't blindly substring its href because MathML source doesn't start with view-source: protocol but with data: instead.
Categories
(Toolkit :: View Source, defect)
Toolkit
View Source
Tracking
()
VERIFIED
FIXED
mozilla34
People
(Reporter: Fanolian+BMO, Assigned: hharchani, Mentored)
References
(Blocks 1 open bug)
Details
(Whiteboard: [good first bug][lang=js])
Attachments
(2 files, 2 obsolete files)
1.29 KB,
patch
|
hharchani
:
review+
|
Details | Diff | Splinter Review |
4.00 KB,
text/html
|
Details |
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 (Beta/Release) Build ID: 20140808030201 Steps to reproduce: 1. Visit any MathML sources, e.g. https://developer.mozilla.org/en-US/docs/Mozilla/MathML_Project/MathML_Torture_Test 2. Right click on a MathML source and choose "View MathML Source". 3. Try to save the source from File > Save page as… Actual results: No files is created. In browser console it shows 2 errors after clicking save: 22:01:29.168 aSource is null DownloadLegacy.js:232 22:01:29.168 [Exception... "[JavaScript Error: "aSource is null" {file: "resource://gre/components/DownloadLegacy.js" line: 232}]'[JavaScript Error: "aSource is null" {file: "resource://gre/components/DownloadLegacy.js" line: 232}]' when calling method: [nsITransfer::init]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "JS frame :: chrome://global/content/contentAreaUtils.js :: internalPersist :: line 385" data: yes] Promise-backend.js:869 Expected results: A file should be created. Note: On the same page, I can save the page source. Only MathML source is not working. This is not a recent regression. I can reproduce it as far back as 2012-01-01 build with a different error message in error console: Error: uncaught exception: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsITransfer.init]" nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)" location: "JS frame :: chrome://global/content/contentAreaUtils.js :: internalPersist :: line 389" data: no]
Updated•10 years ago
|
Component: File Handling → View Source
Product: Firefox → Toolkit
Summary: Unable to save MathML sources → saving MathML source fails with nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"
Comment 1•10 years ago
|
||
So the issue is that this code: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/viewsource/content/viewSource.js?rev=69d61e42d5df&mark=326-328#323 is dumb and just substrings the content href, expecting it to be of the form "view-source:whatever", leaving the "whatever" bit. That would be fine except that for MathML view source, the content href is of the form: data:text/html,... I don't really know why. In any case, I expect this could be fixed by making the above-linked bit of code slightly smarter and just, say, regex-replace /^view-source:/i with "" instead. Mentoring this as a good first bug.
Mentor: gijskruitbosch+bugs
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 8.1 → All
Hardware: x86_64 → All
Summary: saving MathML source fails with nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" → Saving MathML source fails - view source shouldn't blindly substring its href because MathML source doesn't start with view-source: protocol but with data: instead.
Whiteboard: [good first bug][lang=js]
Comment 3•10 years ago
|
||
(In reply to hharchani from comment #2) > I want to work on this Bug, assign this to me. Sounds good! Let me know if you need more help beyond comment #1. :-)
Assignee: nobody → hharchani
Status: NEW → ASSIGNED
Successfully build and run. Now file is being saved.
Attachment #8472983 -
Flags: review?(gijskruitbosch+bugs)
Comment 5•10 years ago
|
||
Comment on attachment 8472983 [details] [diff] [review] bug-1051221.patch I'm sorry, I'm technically not a toolkit peer, so I'm redirecting the review to Matt. :-)
Attachment #8472983 -
Flags: review?(gijskruitbosch+bugs) → review?(MattN+bmo)
Comment 6•10 years ago
|
||
Comment on attachment 8472983 [details] [diff] [review] bug-1051221.patch Review of attachment 8472983 [details] [diff] [review]: ----------------------------------------------------------------- The alternative solution would be to make MathML also use |view-source:| but it wasn't immediately obvious to me how to do that at quick glance and this is definitely an improvement. Thanks ::: toolkit/components/viewsource/content/viewSource.js @@ +322,5 @@ > > // Strips the |view-source:| for internalSave() > function ViewSourceSavePage() > { > + internalSave(window.content.location.href.replace(/^view-source:/i,""), Please include a space after the comma to separate the arguments.
Attachment #8472983 -
Flags: review?(MattN+bmo) → review+
Added a space to separate parameters.
Attachment #8472983 -
Attachment is obsolete: true
Attachment #8473677 -
Flags: review?(MattN+bmo)
Comment 8•10 years ago
|
||
Comment on attachment 8473677 [details] [diff] [review] bug-1051221.patch Hello, Since I gave r+ with only simple changes, you don't need to re-request review. You should update the commit message before requesting checkin-needed though. See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment Thanks
Attachment #8473677 -
Flags: review?(MattN+bmo) → review+
Attachment #8473677 -
Attachment is obsolete: true
Keywords: checkin-needed
Attachment #8475548 -
Flags: review+
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/459e1a537e75
Keywords: checkin-needed
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/459e1a537e75
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Updated•10 years ago
|
QA Whiteboard: [good first verify]
Flags: qe-verify+
Comment 12•10 years ago
|
||
Hi, I've begun to work on this bug for QA's verification bugday. I've noticed that while the problem seems to have been fixed, after reviewing older builds of Nightly and the newest one, there's an inconsistency of behavior. When I try to save source of an HTML file, or otherwise when I make a partial selection and try to save the selected source, I always get a file with the actual code inside, and thus, when opened in Firefox, I get a webpage, or in the other case I get the element. For MathMl however, the Save command will actually save the HTML *view* of the MathML view-source window. This means that the file produced, when opened, will not display the actual equation, but the source in a stylized HTML manner. Is this the intended behavior?
Updated•10 years ago
|
QA Whiteboard: [good first verify] → [good first verify] [bugday-20140827]
Comment 13•10 years ago
|
||
I'm sorry the Comment #12 did not produce the need-info even though I had selected the option (a bug in the attachment form?). Mentor, can you please answer the question on that comment? Thanks :)
Comment 14•10 years ago
|
||
(In reply to Ricardo Maçãs [:Ricmacas] from comment #13) > I'm sorry the Comment #12 did not produce the need-info even though I had > selected the option (a bug in the attachment form?). > Mentor, can you please answer the question on that comment? Thanks :) Fascinating! This is not ideal, no, but I think we should figure out the right behaviour here in a followup, as the current behaviour is strictly better than the previous. :-) Can you file a followup bug?
Comment 15•10 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #14) > (In reply to Ricardo Maçãs [:Ricmacas] from comment #13) > > I'm sorry the Comment #12 did not produce the need-info even though I had > > selected the option (a bug in the attachment form?). > > Mentor, can you please answer the question on that comment? Thanks :) > > Fascinating! This is not ideal, no, but I think we should figure out the > right behaviour here in a followup, as the current behaviour is strictly > better than the previous. :-) > > Can you file a followup bug? Of course, thanks for answering, it seems all my needinfos are going to /dev/null and I was afraid I was spamming you with bugmail already. I'll file a followup bug, should we VERIFY this bug then? (I'll try needinfo with "other" this time).
Flags: needinfo?(gijskruitbosch+bugs)
Comment 16•10 years ago
|
||
Bug 1059297 filed as requested.
Comment 17•10 years ago
|
||
Yup, I think we can mark this verified. Thanks!
You need to log in
before you can comment on or make changes to this bug.
Description
•