Closed
Bug 334573
Opened 18 years ago
Closed 12 years ago
Rename nsPLDOMEvent to nsAsyncDOMEvent
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: darin.moz, Assigned: mjschranz)
References
Details
(Keywords: student-project)
Attachments
(2 files, 6 obsolete files)
3.74 KB,
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
33.16 KB,
patch
|
Details | Diff | Splinter Review |
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•18 years ago
|
No longer depends on: nsIThreadManager
Updated•18 years ago
|
Depends on: nsIThreadManager
Updated•15 years ago
|
Assignee: general → nobody
QA Contact: ian → general
Updated•13 years ago
|
Assignee: nobody → schranz.m
Status: NEW → ASSIGNED
Updated•13 years ago
|
Assignee | ||
Comment 1•13 years ago
|
||
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 2•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
Attachment #579724 -
Attachment is obsolete: true
Attachment #579942 -
Flags: checkin?(jonas)
Assignee | ||
Comment 9•13 years ago
|
||
Attachment #580193 -
Flags: review?(jonas)
Assignee | ||
Comment 10•13 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•12 years ago
|
||
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•12 years ago
|
Keywords: checkin-needed
Comment 14•12 years ago
|
||
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...
Updated•12 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 15•12 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.
Comment 16•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
Whiteboard: checkin-needed
Comment 18•12 years ago
|
||
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
Comment 19•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/74b033b45f27 https://hg.mozilla.org/mozilla-central/rev/69fce3a31ee9
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Attachment #582361 -
Flags: checkin?(bzbarsky)
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•