Last Comment Bug 334573 - Rename nsPLDOMEvent to nsAsyncDOMEvent
: Rename nsPLDOMEvent to nsAsyncDOMEvent
Status: RESOLVED FIXED
: student-project
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: mozilla11
Assigned To: Matthew Schranz [:mjschranz]
:
Mentors:
Depends on: nsIThreadManager
Blocks:
  Show dependency treegraph
 
Reported: 2006-04-18 19:19 PDT by Darin Fisher
Modified: 2011-12-19 19:01 PST (History)
10 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed patch renaming all nsPLDOMEvent references (39.13 KB, patch)
2011-11-30 10:52 PST, Matthew Schranz [:mjschranz]
jonas: review-
Details | Diff | Splinter Review
This patch preserves history through the user of git mv, otherwise is the same as the previous. (31.54 KB, patch)
2011-12-07 08:57 PST, Matthew Schranz [:mjschranz]
no flags Details | Diff | Splinter Review
Proposed patch renaming all nsPLDOMEvent references (31.55 KB, patch)
2011-12-07 09:01 PST, Matthew Schranz [:mjschranz]
no flags Details | Diff | Splinter Review
Proposed patch renaming all nsPLDOMEvent references (32.18 KB, patch)
2011-12-07 09:43 PST, Matthew Schranz [:mjschranz]
jonas: review+
Details | Diff | Splinter Review
Proposed patch renaming all nsPLDOMEvent references (33.17 KB, patch)
2011-12-07 19:19 PST, Matthew Schranz [:mjschranz]
no flags Details | Diff | Splinter Review
Proposed patch renaming all nsLoadBlockingPLDOMEvent to nsLoadBlockingAsyncDOMEvent (33.21 KB, patch)
2011-12-08 14:20 PST, Matthew Schranz [:mjschranz]
jonas: review-
Details | Diff | Splinter Review
Proposed patch renaming all nsLoadBlockingPLDOMEvent to nsLoadBlockingAsyncDOMEvent (3.74 KB, patch)
2011-12-09 11:23 PST, Matthew Schranz [:mjschranz]
jonas: review+
Details | Diff | Splinter Review
Proposed patch renaming all nsPLDOMEvent references (33.16 KB, patch)
2011-12-16 12:46 PST, Matthew Schranz [:mjschranz]
no flags Details | Diff | Splinter Review

Description Darin Fisher 2006-04-18 19:19:20 PDT
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.
Comment 1 Matthew Schranz [:mjschranz] 2011-11-30 10:52:12 PST
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.
Comment 2 Ted Mielczarek [:ted.mielczarek] 2011-11-30 11:05:21 PST
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
Comment 3 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-12-05 21:19:30 PST
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.
Comment 4 Matthew Schranz [:mjschranz] 2011-12-07 08:57:40 PST
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.
Comment 5 Matthew Schranz [:mjschranz] 2011-12-07 09:01:03 PST
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.
Comment 6 Matthew Schranz [:mjschranz] 2011-12-07 09:43:26 PST
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.
Comment 7 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-12-07 10:53:03 PST
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
Comment 8 Matthew Schranz [:mjschranz] 2011-12-07 19:19:47 PST
Created attachment 579942 [details] [diff] [review]
Proposed patch renaming all nsPLDOMEvent references
Comment 9 Matthew Schranz [:mjschranz] 2011-12-08 14:20:33 PST
Created attachment 580193 [details] [diff] [review]
Proposed patch renaming all nsLoadBlockingPLDOMEvent to nsLoadBlockingAsyncDOMEvent
Comment 10 Matthew Schranz [:mjschranz] 2011-12-08 14:23:41 PST
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 11 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-12-08 14:30:24 PST
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?
Comment 12 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-12-08 14:35:14 PST
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.
Comment 13 Matthew Schranz [:mjschranz] 2011-12-09 11:23:47 PST
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.
Comment 14 Boris Zbarsky [:bz] 2011-12-15 11:35:26 PST
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...
Comment 15 Matthew Schranz [:mjschranz] 2011-12-15 16:12:54 PST
(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 Boris Zbarsky [:bz] 2011-12-15 18:41:46 PST
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?
Comment 17 Matthew Schranz [:mjschranz] 2011-12-16 12:46:14 PST
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.

Note You need to log in before you can comment on or make changes to this bug.