Closed Bug 334573 Opened 18 years ago Closed 12 years ago

Rename nsPLDOMEvent to nsAsyncDOMEvent

Categories

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

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: darin.moz, Assigned: mjschranz)

References

Details

(Keywords: student-project)

Attachments

(2 files, 6 obsolete files)

Once my patch for bug 326273 lands, there will no longer be any PLEvent struct.  So, this class should probably be renamed.  nsPLDOMEvent.h will have a comment linking to this bug.
No longer depends on: nsIThreadManager
Assignee: general → nobody
QA Contact: ian → general
Assignee: nobody → schranz.m
Status: NEW → ASSIGNED
Keywords: student-project
OS: Linux → All
Hardware: x86 → All
After a quick glance it looked like I did the conversion correctly from working with git to an hg acceptable format but it's a first time thing here so I'm thinking I may have missed something.
Attachment #578013 - Flags: review?(ted.mielczarek)
Comment on attachment 578013 [details] [diff] [review]
Proposed patch renaming all nsPLDOMEvent references

I'm not actually a content peer. I'll punt this to Sicking, but anyone here would be fine as a reviewer:
https://wiki.mozilla.org/Modules/Core#Document_Object_Model
Attachment #578013 - Flags: review?(ted.mielczarek) → review?(jonas)
Comment on attachment 578013 [details] [diff] [review]
Proposed patch renaming all nsPLDOMEvent references

The changes look good.

However you have both done changes to the existing nsPLDOMEvent.h/cpp files as well as added new nsAsyncDOMEvent.h/cpp files. So now both files are in the tree but with the old files not being built.

Instead of adding the new files, make the changes to the old files and then do a |hg move| to rename the files. That way we preserve history of the files.
Attachment #578013 - Flags: review?(jonas) → review-
Patch is essentially the same as the previous but it should be properly maintaining the files history now. This can be seen by the rename lines in the file.
Attachment #578013 - Attachment is obsolete: true
Attachment #579708 - Flags: review?(jonas)
Forgot to place an appropriate message at the top of the bug, fixed now.
Attachment #579708 - Attachment is obsolete: true
Attachment #579710 - Flags: review?(jonas)
Attachment #579708 - Flags: review?(jonas)
Sorry for the spam! David Humphrey pointed out I forgot indenting issues in some cases and fixed those real quick. Let me know if I've missed anything else.
Attachment #579710 - Attachment is obsolete: true
Attachment #579724 - Flags: review?(jonas)
Attachment #579710 - Flags: review?(jonas)
Comment on attachment 579724 [details] [diff] [review]
Proposed patch renaming all nsPLDOMEvent references

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

r=me with the indentation fix. Though it'd be awesome if you want to do a separate patch for the other rename (should be much simpler).

Please attach a new file with the indentation fix, but no need to ask for review again.

::: content/events/public/nsPLDOMEvent.h
@@ +77,5 @@
>    bool                  mBubbles;
>    bool                  mDispatchChromeOnly;
>  };
>  
> +class nsLoadBlockingPLDOMEvent : public nsAsyncDOMEvent {

We really should rename this class too.

::: layout/xul/base/src/tree/src/nsTreeSelection.cpp
@@ +691,2 @@
>                       (aIndex != -1 ? DOMMenuItemActive : DOMMenuItemInactive),
>                       true, false);

Fix indentation
Attachment #579724 - Flags: review?(jonas) → review+
Attachment #579724 - Attachment is obsolete: true
Fat fingered my enter key before I got to type this message.

I wasn't 100% sure what code I should have been constructing the patch so I decided to make one that sort of had all of the changes combined.
Comment on attachment 580193 [details] [diff] [review]
Proposed patch renaming all nsLoadBlockingPLDOMEvent to nsLoadBlockingAsyncDOMEvent

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

This doesn't seem to fix nsLoadBlockingPLDOMEvent at all?
Attachment #580193 - Flags: review?(jonas) → review-
The attached patch doesn't have the combined changes. It looks basically identical to the previous patch you attached.

Ideal would be if you could make any new patches that you make for this bug apply on top of attachment 579942 [details] [diff] [review]. That way I won't have to re-review everything again.
Patch should hopefully be in proper format, all indenting issues as well from what I can tell.
Attachment #580193 - Attachment is obsolete: true
Attachment #580478 - Flags: review?(jonas)
Keywords: checkin-needed
I assume that the two patches are meant to land in order (the bigger one first, then the smaller)?

The first patch in this bug doesn't apply on tip...
(In reply to Boris Zbarsky (:bz) from comment #14)
> I assume that the two patches are meant to land in order (the bigger one
> first, then the smaller)?
> 
> The first patch in this bug doesn't apply on tip...

Yes that is the plan. As far as having the keywords checkin-needed I it before because it was suggested to me by another developer. Hope it didn't cause any issues.
It didn't cause issues.  I just tried to check this in, but it doesn't apply, so I can't exactly do that.  Can you merge it tip and post the updated patch, then add the keyword again?
Up to date with m-c as of well, the time of this post! Hopefully this is the last one.
Attachment #579942 - Attachment is obsolete: true
Attachment #582361 - Flags: checkin?(bzbarsky)
Attachment #579942 - Flags: checkin?(jonas)
Whiteboard: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/74b033b45f27
https://hg.mozilla.org/integration/mozilla-inbound/rev/69fce3a31ee9

Thanks for updating the patch!
Flags: in-testsuite-
Whiteboard: checkin-needed
Target Milestone: --- → mozilla11
Attachment #582361 - Flags: checkin?(bzbarsky)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.