Closed Bug 1225782 Opened 4 years ago Closed 4 years ago

Remove GlobalPropertiesAreOwn().

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: Ms2ger, Assigned: sidhus11, Mentored)

References

Details

(Whiteboard: [lang=c++])

Attachments

(1 file, 2 obsolete files)

Code: https://mxr.mozilla.org/mozilla-central/search?string=GlobalPropertiesAreOwn

This was added in bug 1005898 to make it easier to turn off the new behaviour, but we haven't needed to turn it off in the year and a half since, so it should be safe to remove.
Hi,

I'm new to this. I've attempted to solve this bug, please see attached.
Attachment #8689146 - Flags: feedback?(Ms2ger)
Comment on attachment 8689146 [details] [diff] [review]
rev1 - Remove GlobalPropertiesAreOwn().

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

Thanks for the patch! I think I discovered one mistake below:

::: dom/bindings/BindingUtils.cpp
@@ +1738,5 @@
>        return false;
>      }
>    }
>  
> +  return (type == eGlobalInterfacePrototype) ||

I'd remove the parentheses here.

::: dom/bindings/Codegen.py
@@ +3669,5 @@
>          self.descriptor = descriptor
>          self.properties = properties
>  
>      def definition_body(self):
> +        properties = "nullptr"

I think you got the condition backwards here.
Attachment #8689146 - Flags: feedback?(Ms2ger) → feedback+
Thank you for the feedback. Please find my revised patch attached.
Attachment #8689692 - Flags: feedback?(Ms2ger)
Comment on attachment 8689692 [details] [diff] [review]
rev2 - Remove GlobalPropertiesAreOwn()

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

Thank you, this looks great to me. Asking bz to do the final review.
Attachment #8689692 - Flags: review?(bzbarsky)
Attachment #8689692 - Flags: feedback?(Ms2ger)
Attachment #8689692 - Flags: feedback+
Comment on attachment 8689692 [details] [diff] [review]
rev2 - Remove GlobalPropertiesAreOwn()

r=me
Attachment #8689692 - Flags: review?(bzbarsky) → review+
This should be a simple refactoring; I am fairly confident no try run is necessary.

NOTE: commit message should be updated to say r=bz.

Thanks!
Keywords: checkin-needed
Assignee: nobody → sidhus11
Thank you!

Hi Boris, please find my patch attached.
Attachment #8689146 - Attachment is obsolete: true
Attachment #8689692 - Attachment is obsolete: true
Attachment #8690167 - Flags: review?(bzbarsky)
Comment on attachment 8690167 [details] [diff] [review]
rev2.1 - Remove GlobalPropertiesAreOwn()

r=me, but in general no need for more rounds of review on commit message tweaks.  ;)
Attachment #8690167 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/57f738fb6ab6
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.