Closed Bug 796902 Opened 7 years ago Closed 7 years ago

Move PaintRequest to Paris bindings

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

No description provided.
Maybe khuey can help review these?
Attachment #678125 - Flags: review?(bent.mozilla) → review+
Comment on attachment 678126 [details] [diff] [review]
Part b: Make nsPaintRequest implement nsWrapperCache;

Review of attachment 678126 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with these:

::: content/events/src/nsPaintRequest.cpp
@@ +24,5 @@
>  
> +NS_IMPL_CYCLE_COLLECTING_ADDREF(nsPaintRequest)
> +NS_IMPL_CYCLE_COLLECTING_RELEASE(nsPaintRequest)
> +
> +/* virtual */ JSObject*

Nit: The virtual comment doesn't really add much here imo.

::: content/events/src/nsPaintRequest.h
@@ +18,4 @@
>  {
>  public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(nsPaintRequestList)

"nsPaintRequest" here.

Oh, looks like this is corrected in the next patch.

@@ +22,2 @@
>    NS_DECL_NSIDOMPAINTREQUEST
> +  

Nit: whitespace
Attachment #678126 - Flags: review?(bent.mozilla) → review+
Comment on attachment 678127 [details] [diff] [review]
Part c: Implement Paris bindings for PaintRequest;

Review of attachment 678127 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with these:

::: content/events/src/nsPaintRequest.cpp
@@ +56,3 @@
>  {
> +  nsString result;
> +  GetReason(result);

You won't need the temporary here after changing the signature.

::: content/events/src/nsPaintRequest.h
@@ +10,5 @@
>  #include "nsIDOMPaintRequestList.h"
>  #include "nsPresContext.h"
>  #include "nsIDOMEvent.h"
>  #include "mozilla/Attributes.h"
> +#include "nsClientRect.h"

Nit: You can just forward declare this.

@@ +36,5 @@
> +    return mParent;
> +  }
> +
> +  already_AddRefed<nsClientRect> ClientRect();
> +  void GetReason(nsString& aResult) const

Nit: nsAString

::: dom/interfaces/events/nsIDOMPaintRequest.idl
@@ +24,5 @@
>     * needed to repaint the rectangle due to scrolling, and "scroll copy", meaning
>     * that we updated the rectangle due to scrolling but instead of painting
>     * manually, we were able to do a copy from another area of the screen.
>     */
> +  [binaryname(DOMReason)]

Calling this "DOMReason" seems wrong as the actual DOM will not call this. What about XPCOMReason? IDLReason? I don't care too strongly as long as it's not "DOM".
Attachment #678127 - Flags: review?(bent.mozilla) → review+
(In reply to ben turner [:bent] from comment #6)
> Comment on attachment 678127 [details] [diff] [review]
> Part c: Implement Paris bindings for PaintRequest;
> 
> Review of attachment 678127 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/events/src/nsPaintRequest.h
> @@ +10,5 @@
> >  #include "nsIDOMPaintRequestList.h"
> >  #include "nsPresContext.h"
> >  #include "nsIDOMEvent.h"
> >  #include "mozilla/Attributes.h"
> > +#include "nsClientRect.h"
> 
> Nit: You can just forward declare this.

I tried; the generated code fails to build because it can't assign an nsClientRect* to an nsIDOMClientRect* without this include. I thought it was better to keep returning the concrete type, to avoid having to change this code when moving ClientRect to the new bindings.

Will address your other comments.
Ah, ok. Just remove the include from the cpp then.
https://hg.mozilla.org/mozilla-central/rev/c4267242d766
https://hg.mozilla.org/mozilla-central/rev/75157a7f9a06
https://hg.mozilla.org/mozilla-central/rev/8ca4011a1250
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.