The default bug view has changed. See this FAQ.

Rename nsPLDOMEvent to nsAsyncDOMEvent

RESOLVED FIXED in mozilla11

Status

()

Core
DOM
--
trivial
RESOLVED FIXED
11 years ago
5 years ago

People

(Reporter: Darin Fisher, Assigned: mjschranz)

Tracking

({student-project})

Trunk
mozilla11
student-project
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

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

Updated

11 years ago
No longer depends on: 326273
Depends on: 326273
Assignee: general → nobody
QA Contact: ian → general
Assignee: nobody → schranz.m
Status: NEW → ASSIGNED
Keywords: student-project
OS: Linux → All
Hardware: x86 → All
(Assignee)

Comment 1

5 years ago
Created attachment 578013 [details] [diff] [review]
Proposed patch renaming all nsPLDOMEvent references

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-
(Assignee)

Comment 4

5 years ago
Created attachment 579708 [details] [diff] [review]
This patch preserves history through the user of git mv, otherwise is the same as the previous.

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)
(Assignee)

Comment 5

5 years ago
Created attachment 579710 [details] [diff] [review]
Proposed patch renaming all nsPLDOMEvent references

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)
(Assignee)

Comment 6

5 years ago
Created attachment 579724 [details] [diff] [review]
Proposed patch renaming all nsPLDOMEvent references

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

Comment 8

5 years ago
Created attachment 579942 [details] [diff] [review]
Proposed patch renaming all nsPLDOMEvent references
Attachment #579724 - Attachment is obsolete: true
Attachment #579942 - Flags: checkin?(jonas)
(Assignee)

Comment 9

5 years ago
Created attachment 580193 [details] [diff] [review]
Proposed patch renaming all nsLoadBlockingPLDOMEvent to nsLoadBlockingAsyncDOMEvent
Attachment #580193 - Flags: review?(jonas)
(Assignee)

Comment 10

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

Comment 13

5 years ago
Created attachment 580478 [details] [diff] [review]
Proposed patch renaming all nsLoadBlockingPLDOMEvent to nsLoadBlockingAsyncDOMEvent

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)
Attachment #580478 - Flags: review?(jonas) → review+
(Assignee)

Updated

5 years ago
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...
Keywords: checkin-needed
(Assignee)

Comment 15

5 years ago
(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?
(Assignee)

Comment 17

5 years ago
Created attachment 582361 [details] [diff] [review]
Proposed patch renaming all nsPLDOMEvent references

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)
(Assignee)

Updated

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/74b033b45f27
https://hg.mozilla.org/mozilla-central/rev/69fce3a31ee9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Attachment #582361 - Flags: checkin?(bzbarsky)
You need to log in before you can comment on or make changes to this bug.