Warn about XMLHttpRequest sendAsBinary usage

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
6 years ago
3 months ago

People

(Reporter: sicking, Assigned: jefry.reyes)

Tracking

({dev-doc-complete, site-compat})

unspecified
mozilla31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment)

+++ 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.
Assignee

Comment 1

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

Comment 2

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

Comment 3

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

Comment 4

5 years ago
This bug is for implementing the warning. Then later on we remove it. That is why this bug blocks that bug.
Assignee

Comment 5

5 years ago
So the warning should be in nsXMLHttpRequest.cpp?

Comment 6

5 years ago
Yes.
Assignee

Comment 8

5 years ago
Can I please get a review for that patch, and also get assigned to this ticket.
Assignee

Updated

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

Updated

5 years ago
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

Updated

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

Updated

5 years ago
Status: NEW → RESOLVED
Closed: 5 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: 5 years ago5 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.