616672 - Warn on access of LSProgressEvent.position, LSProgressEvent.totalSize

Status: RESOLVED FIXED

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

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Like this?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2

I didn't even realize I already had a patch here...

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v3

I found a Window... Does this look better?

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v3.1

And now a try that builds...

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/64eeb93a6ed4

Comment 9

[Francesco Lodolo [:flod]]

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?

Comment 10

[Marek Stępień [:marcoos, inactive]]

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. :)

Comment 11

[Francesco Lodolo [:flod]]

(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 ;-)