Warn on access of LSProgressEvent.position, LSProgressEvent.totalSize

RESOLVED FIXED in mozilla10

Status

()

Core
DOM
P1
normal
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

Trunk
mozilla10
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

7 years ago
Created attachment 495288 [details] [diff] [review]
Patch v1

Like this?
Attachment #495288 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

7 years ago
Target Milestone: --- → mozilla2.0

Comment 2

7 years ago
Comment on attachment 495288 [details] [diff] [review]
Patch v1


>+static void
>+ReportUseOfDeprecatedMethod(nsDOMProgressEvent* aPresContext, const char* aWarning)
Strangely named parameter.


>+{
>+  nsContentUtils::ReportToConsole(nsContentUtils::eDOM_PROPERTIES,
>+                                  aWarning,
>+                                  nsnull, 0,
>+                                  aInner->PresContext()->Document()->GetDocumentURI(),
>+                                  EmptyString(), 0, 0,
>+                                  nsIScriptError::warningFlag,
>+                                  "DOM Events");
Does this really work? How do you guarantee that PresContext() doesn't return null.
And I think we should report the warning only once per document.
(We do that for nsDocument::GetBoxObjectFor)
Perhaps you could use the target of the event to get the document?
Attachment #495288 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 3

6 years ago
Created attachment 561217 [details] [diff] [review]
Patch v2

I didn't even realize I already had a patch here...
Attachment #495288 - Attachment is obsolete: true
Attachment #561217 - Flags: review?(Olli.Pettay)

Comment 4

6 years ago
Comment on attachment 561217 [details] [diff] [review]
Patch v2

>+nsXMLHttpProgressEvent::WarnAboutLSProgressEvent(nsIDocument::DeprecatedOperations aOperation)
>+{
>+  nsPresContext* presContext = mInner->GetPresContext();
>+  if (!presContext) {
>+    return;
>+  }
Is presContext actually ever set?

I think it would be better to add something so that you could get
document from the target of the event.
(XHR's owner's document)
Attachment #561217 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Comment 5

6 years ago
Created attachment 561506 [details] [diff] [review]
Patch v3

I found a Window... Does this look better?
Attachment #561217 - Attachment is obsolete: true
Attachment #561506 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

6 years ago
Target Milestone: mozilla2.0 → mozilla9
(Assignee)

Comment 6

6 years ago
Created attachment 561547 [details] [diff] [review]
Patch v3.1

And now a try that builds...
Attachment #561506 - Attachment is obsolete: true
Attachment #561547 - Flags: review?(Olli.Pettay)
Attachment #561506 - Flags: review?(Olli.Pettay)

Comment 7

6 years ago
Comment on attachment 561547 [details] [diff] [review]
Patch v3.1

>   // Use nsDOMProgressEvent so that we can forward
>   // most of the method calls easily.
>   nsRefPtr<nsDOMProgressEvent> mInner;
>+  nsCOMPtr<nsPIDOMWindow> mWindow;
You need to add this to cycle collection


>diff --git a/content/events/src/nsDOMEvent.h b/content/events/src/nsDOMEvent.h
>--- a/content/events/src/nsDOMEvent.h
>+++ b/content/events/src/nsDOMEvent.h
>@@ -218,16 +218,19 @@ public:
> 
>   static PopupControlState GetEventPopupControlState(nsEvent *aEvent);
> 
>   static void PopupAllowedEventsChanged();
> 
>   static void Shutdown();
> 
>   static const char* GetEventName(PRUint32 aEventType);
>+
>+  nsPresContext* GetPresContext() { return mPresContext; }
Leftover?
Attachment #561547 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 8

6 years ago
https://hg.mozilla.org/mozilla-central/rev/64eeb93a6ed4
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [strings]
Target Milestone: mozilla9 → mozilla10
PositionWarning=Use XMLHttpRequest's progress events' position attribute is deprecated.
TotalSizeWarning=Use XMLHttpRequest's progress events' totalSize attribute is deprecated.

I suppose it should be "Use of" instead of "Use", but honestly I have big problems translating this string. Could you please try to rephrase it or give an equivalent sentence?
flod, what this means is that:
- XMLHttpRequest progress events have attributes called "position" and "totalSize".
- Use of these attributes is now deprecated and they will be removed at a later time.
- When a web developer uses these attributes, he'll get this warning telling them not to use these attributes.

To rephrase this for you:

'Using the "position'/'totalSize' attributes of XMLHttpRequest progress events is deprecated'

Hope this makes it clear for you. :)
(In reply to Marek Stępień :marcoos from comment #10)
> Hope this makes it clear for you. :)

Yes, a lot clearer. I already translated in this way, but now I'm sure that's right ;-)

Updated

4 years ago
Blocks: 695811
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.