Remove GlobalPropertiesAreOwn().

RESOLVED FIXED in Firefox 45

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Ms2ger, Assigned: Sunny Sidhu, Mentored)

Tracking

unspecified
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8689146 [details] [diff] [review]
rev1 - Remove GlobalPropertiesAreOwn().

Hi,

I'm new to this. I've attempted to solve this bug, please see attached.
Attachment #8689146 - Flags: feedback?(Ms2ger)
(Reporter)

Comment 2

2 years ago
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+
(Assignee)

Comment 3

2 years ago
Created attachment 8689692 [details] [diff] [review]
rev2 - Remove GlobalPropertiesAreOwn()

Thank you for the feedback. Please find my revised patch attached.
Attachment #8689692 - Flags: feedback?(Ms2ger)
(Reporter)

Comment 4

2 years ago
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+
(Reporter)

Comment 6

2 years ago
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
(Reporter)

Updated

2 years ago
Assignee: nobody → sidhus11
(Assignee)

Comment 7

2 years ago
Created attachment 8690167 [details] [diff] [review]
rev2.1 - Remove GlobalPropertiesAreOwn()

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+

Comment 9

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/57f738fb6ab6
Keywords: checkin-needed

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/57f738fb6ab6
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.