Last Comment Bug 616672 - Warn on access of LSProgressEvent.position, LSProgressEvent.totalSize
: Warn on access of LSProgressEvent.position, LSProgressEvent.totalSize
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla10
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
Mentors:
Depends on:
Blocks: 615507 695811
  Show dependency treegraph
 
Reported: 2010-12-04 02:04 PST by :Ms2ger (⌚ UTC+1/+2)
Modified: 2013-04-04 13:53 PDT (History)
2 users (show)
Ms2ger: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (4.07 KB, patch)
2010-12-04 14:18 PST, :Ms2ger (⌚ UTC+1/+2)
bugs: review+
Details | Diff | Splinter Review
Patch v2 (4.43 KB, patch)
2011-09-20 09:33 PDT, :Ms2ger (⌚ UTC+1/+2)
bugs: review-
Details | Diff | Splinter Review
Patch v3 (6.85 KB, patch)
2011-09-21 10:15 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Patch v3.1 (6.88 KB, patch)
2011-09-21 12:44 PDT, :Ms2ger (⌚ UTC+1/+2)
bugs: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2010-12-04 02:04:26 PST

    
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2010-12-04 14:18:53 PST
Created attachment 495288 [details] [diff] [review]
Patch v1

Like this?
Comment 2 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-12-05 11:58:51 PST
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 (⌚ UTC+1/+2) 2011-09-20 09:33:18 PDT
Created attachment 561217 [details] [diff] [review]
Patch v2

I didn't even realize I already had a patch here...
Comment 4 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-09-21 01:55:49 PDT
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 (⌚ UTC+1/+2) 2011-09-21 10:15:45 PDT
Created attachment 561506 [details] [diff] [review]
Patch v3

I found a Window... Does this look better?
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2011-09-21 12:44:53 PDT
Created attachment 561547 [details] [diff] [review]
Patch v3.1

And now a try that builds...
Comment 7 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-09-21 23:40:53 PDT
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 (⌚ UTC+1/+2) 2011-10-02 07:18:27 PDT
https://hg.mozilla.org/mozilla-central/rev/64eeb93a6ed4
Comment 9 Francesco Lodolo [:flod] 2011-10-05 12:16:45 PDT
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] 2011-10-23 07:22:20 PDT
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] 2011-10-23 08:04:57 PDT
(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 ;-)

Note You need to log in before you can comment on or make changes to this bug.