Closed Bug 939323 Opened 11 years ago Closed 10 years ago

Warn about XMLHttpRequest sendAsBinary usage

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: sicking, Assigned: jefry.reyes)

References

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [good first bug])

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #853162 +++

We should warn whenever XMLHttpRequest.sendAsBinary is called. This way we can eventually remove this function.

Using nsIDocument::WarnOnceAbout should be enough.
When I look at the file "workers/XMLHttpRequest.cpp" I find the following code:

void
XMLHttpRequest::SendAsBinary(const nsAString& aBody, ErrorResult& aRv)
{
  NS_NOTYETIMPLEMENTED("Implement me!");
  aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);
  return;
}


Does that mean that the method is not implemented or am I looking in the wrong place? Is it really necesary to warn about its use, when it throws an implementation error everytime it is called?
We can probably safely remove it from workers. However, http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#2240 is not as safe.
Bug #853162 already talks about removing it. So perhaps this ticket should be closed so that we can try how to safely remove it in #853162.
This bug is for implementing the warning. Then later on we remove it. That is why this bug blocks that bug.
So the warning should be in nsXMLHttpRequest.cpp?
Yes.
Can I please get a review for that patch, and also get assigned to this ticket.
Attachment #8409336 - Flags: review?(jefry.reyes)
Attachment #8409336 - Flags: feedback?(jefry.reyes)
You need to request review from someone who is not you :) http://hg.mozilla.org/mozilla-central/filelog/53a6c96cea62/content/base/src/nsXMLHttpRequest.cpp does not have a clear candidate, unfortunately, based on the r= annotations, but I recommend choosing sicking. For example, there's "Olli Pettay - Bug 969671 - Warn about use of sync XHR in the main thread, r=sicking/ehsan" in the history.
Attachment #8409336 - Flags: review?(jonas)
Attachment #8409336 - Flags: review?(jefry.reyes)
Attachment #8409336 - Flags: feedback?(jonas)
Attachment #8409336 - Flags: feedback?(jefry.reyes)
Comment on attachment 8409336 [details] [diff] [review]
Displays a warning everytime sendAsBinary is used

Jonas is a reasonable guess, but he doesn't really have time to do reviews. Let's try smaug.
Attachment #8409336 - Flags: review?(jonas)
Attachment #8409336 - Flags: review?(bugs)
Attachment #8409336 - Flags: feedback?(jonas)
Comment on attachment 8409336 [details] [diff] [review]
Displays a warning everytime sendAsBinary is used

In the future use -U 8 -p when generating patches.
Attachment #8409336 - Flags: review?(bugs) → review+
Assignee: nobody → jefry.reyes
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/215336ef09be

Thanks for the patch, Jefry! One request, please make sure that future patches contain commit information when requesting checkin. Thanks!
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Please don't resolve bugs unless they've landed on mozilla-central.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/215336ef09be
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.