Last Comment Bug 875449 - Rename ListBase to DOMProxy
: Rename ListBase to DOMProxy
Status: RESOLVED FIXED
[mentor=bz][lang=C++]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla24
Assigned To: Anand Soni
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-23 12:19 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2013-06-07 09:03 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix for bug 875449. Attaching the first patch for review. (70.76 KB, patch)
2013-06-04 20:01 PDT, Anand Soni
kvijayan: review+
Details | Diff | Splinter Review
Modified fix for Bug 875449 with the comment changed. (70.76 KB, patch)
2013-06-06 11:22 PDT, Anand Soni
no flags Details | Diff | Splinter Review
Final attachment with all the required and suggested changes. (71.03 KB, patch)
2013-06-06 11:37 PDT, Anand Soni
no flags Details | Diff | Splinter Review
Final attachment with all the required and suggested changes. (71.06 KB, patch)
2013-06-06 11:52 PDT, Anand Soni
no flags Details | Diff | Splinter Review

Description User image Boris Zbarsky [:bz] (still a bit busy) 2013-05-23 12:19:17 PDT
ListBase is totally a misnomer at this point.  We should just call it DOMProxy, I think.
Comment 1 User image Saurabh Anand [:sawrubh] 2013-05-28 02:27:48 PDT
Is this bug simply about renaming all the occurences of ListBase to DOMProxy (https://mxr.mozilla.org/mozilla-central/search?string=ListBase&case=on&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central) or is there something more to it too ?
Comment 2 User image Boris Zbarsky [:bz] (still a bit busy) 2013-05-28 08:35:54 PDT
I think that's about all there is to it, if you ignore the mobile/ hits in mxr there.
Comment 3 User image Anand Soni 2013-06-02 10:07:18 PDT
Hi Boris,

I would like to help fixing this bug. Can you brief me on the procedure? I have no knowledge on submitting patches to mozilla.

Thanks!
Comment 4 User image Saurabh Anand [:sawrubh] 2013-06-02 10:26:06 PDT
Check out https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch and https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F . You can also join #introduction on irc.mozilla.org if you want to ask questions.

As far as this bug is concerned, I guess you simply need to rename all the occurences of ListBase to DOMProxy (https://mxr.mozilla.org/mozilla-central/search?string=ListBase&case=on&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central) except the ones in the files inside mobile/ directory, as pointed out by bz in https://bugzilla.mozilla.org/show_bug.cgi?id=875449#c2.
Comment 5 User image Boris Zbarsky [:bz] (still a bit busy) 2013-06-03 06:37:09 PDT
Yep, comment 4 about covers it.  ;)
Comment 6 User image Anand Soni 2013-06-03 11:04:14 PDT
Thank you for the pointers. I have set up mercurial and cloned the repository. I will soon submit a patch for review.

Thanks a lot!
Comment 7 User image Anand Soni 2013-06-04 10:38:14 PDT
Do I need to change only ListBase to DOMProxy or listBase also?
Comment 8 User image Kannan Vijayan [:djvj] 2013-06-04 10:40:54 PDT
Yeah, it's probably a good idea to change the variable names to reflect the class name.  There will be a number of different variants of these.  There are also method names that have "ListBase" in the name (e.g. IsCacheableListBaseProxy) in ion/IonCache.cpp.

It's good to change all of those to have new names to match the new "DOMProxy" naming scheme.

e.g. the listBase variable names would become domProxy, IsCacheableListBaseProxy would become IsCacheableDOMProxy, etc.
Comment 9 User image Anand Soni 2013-06-04 10:48:06 PDT
Ok. I got it. 
Now, ListBase will become DOMProxy for each occurence and, listBase will become domProxy for each occurence.
Comment 10 User image Anand Soni 2013-06-04 12:58:43 PDT
What flags should I request (for review and others below it) while attaching the patch?
Comment 11 User image Boris Zbarsky [:bz] (still a bit busy) 2013-06-04 13:03:13 PDT
I suggest requesting review from and :djvj.
Comment 12 User image Anand Soni 2013-06-04 20:01:26 PDT
Created attachment 758364 [details] [diff] [review]
Fix for bug 875449. Attaching the first patch for review.
Comment 13 User image Sankha Narayan Guria [:sankha] 2013-06-04 20:36:23 PDT
Comment on attachment 758364 [details] [diff] [review]
Fix for bug 875449. Attaching the first patch for review.

When you ask for review, put a r? flag against the attachment, and in this case set the requestee as :djvj.
Comment 14 User image Anand Soni 2013-06-04 20:39:19 PDT
Thank you for the correction sankha93. I was not aware of this.
Comment 15 User image Anand Soni 2013-06-06 00:03:18 PDT
Hi :djvj,

Is the patch correct? Do I need to modify/change it or add something? Please let me know.
Comment 16 User image Kannan Vijayan [:djvj] 2013-06-06 10:36:15 PDT
Sorry, got caught up by some other bugs.  Reviewing this now.
Comment 17 User image Kannan Vijayan [:djvj] 2013-06-06 11:05:29 PDT
Comment on attachment 758364 [details] [diff] [review]
Fix for bug 875449. Attaching the first patch for review.

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

Looks good.  Nice job :)  Just fix the minor issue with the comment and you're good to go.

::: js/src/ion/BaselineIC.h
@@ +4196,4 @@
>  {
>    friend class ICStubSpace;
>    protected:
> +    // Shape of the DOMProxy proxy

Remove 'proxy' here.  "// Shape of the DOMProxy" should be fine.
Comment 18 User image Boris Zbarsky [:bz] (still a bit busy) 2013-06-06 11:20:43 PDT
DOMListShadows should probably become DOMProxyShadows too.
Comment 19 User image Anand Soni 2013-06-06 11:22:31 PDT
Created attachment 759282 [details] [diff] [review]
Modified fix for Bug 875449 with the comment changed.
Comment 20 User image Anand Soni 2013-06-06 11:25:20 PDT
Fix on the comment by :djvj. I will change the occurence of DOMListShadow to DOMProxyShadow in the following files now:
https://mxr.mozilla.org/mozilla-central/search?string=DOMListShadow&case=1&find=&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central

Will that be it?
Comment 21 User image Anand Soni 2013-06-06 11:25:57 PDT
Also, the bug has not been assigned to me yet. :)
Comment 22 User image Boris Zbarsky [:bz] (still a bit busy) 2013-06-06 11:26:25 PDT
You also want a sane commit comment.  ;)
Comment 23 User image Anand Soni 2013-06-06 11:37:24 PDT
Created attachment 759305 [details] [diff] [review]
Final attachment with all the required and suggested changes.
Comment 24 User image Anand Soni 2013-06-06 11:38:40 PDT
Hi :bz and :djvj,
I think I have made the suggested changes. Please review again.
Comment 25 User image Kannan Vijayan [:djvj] 2013-06-06 11:40:51 PDT
(In reply to Anand Soni from comment #23)
> Created attachment 759305 [details] [diff] [review]
> Final attachment with all the required and suggested changes.

For future reference:

If you're just making minor changes, and addressing comments, you don't have to ask for a re-review.  You can use the previous r+.

If I felt that the requested changes needed a re-review, I would have canceled or r-ed the review.

I'd say you can go ahead and cancel the review request and just commit.
Comment 26 User image Anand Soni 2013-06-06 11:44:21 PDT
What does that mean? I mean, what should I do exactly? Should I change it to r+?
Comment 27 User image Kannan Vijayan [:djvj] 2013-06-06 11:47:50 PDT
To cancel a review request, just click on the "Details" link for the patch, and change the flag associated with "Review" to the blank option (available on the pulldown).  It'll remove the review request.
Comment 28 User image Boris Zbarsky [:bz] (still a bit busy) 2013-06-06 11:48:39 PDT
Comment on attachment 759305 [details] [diff] [review]
Final attachment with all the required and suggested changes.

How about this for the commit message:

Bug 875449: rename ListBase to DOMProxy, since these proxies are used for all sorts of objects that aren't in any way like lists.  r=djvj

?
Comment 29 User image Anand Soni 2013-06-06 11:52:24 PDT
Created attachment 759319 [details] [diff] [review]
Final attachment with all the required and suggested changes.
Comment 30 User image Anand Soni 2013-06-06 11:53:11 PDT
:bz and :djvj, is everything good now?
Comment 31 User image Anand Soni 2013-06-06 12:00:46 PDT
What is the next step?
Comment 32 User image Anand Soni 2013-06-06 12:13:55 PDT
:djvj and :bz,

Will you push it in the tree for me? I donot have the access.
Comment 33 User image Boris Zbarsky [:bz] (still a bit busy) 2013-06-06 12:57:42 PDT
The next step is to mark all the obsolete stuff as obsolete so it's clear what needs to be checked in and then adding the "checkin-needed" keyword.  ;)

That way anyone who checks for those will push it if djvj and I don't get around to pushing things soon enough.
Comment 34 User image Anand Soni 2013-06-06 21:39:48 PDT
Thank you Boris! I hope this gets checked-in soon. Do we need to change the status of this bug?
Comment 35 User image Sankha Narayan Guria [:sankha] 2013-06-06 21:42:10 PDT
No, you can now relax! You have fixed your first bug. :)

The person who will check in, will update the status of the bug.
Comment 36 User image Anand Soni 2013-06-06 21:55:06 PDT
Alright. I got it now. Thank you Sankha!
Comment 37 User image Ryan VanderMeulen [:RyanVM] 2013-06-07 05:25:23 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/0261df8a6a8e

Thanks for the patch, Anand!
Comment 38 User image Ryan VanderMeulen [:RyanVM] 2013-06-07 05:54:24 PDT
And a bustage follow-up.
https://hg.mozilla.org/integration/mozilla-inbound/rev/166c6df896ba
Comment 39 User image Anand Soni 2013-06-07 07:35:14 PDT
Hi RyanVM,

I suppose this means that the patch will be further tested and then pushed to mozilla-central. And then this bug will be closed (tagged as resolved). Is that so?
Comment 40 User image Ryan VanderMeulen [:RyanVM] 2013-06-07 07:35:54 PDT
Yes, exactly :)
Comment 41 User image Anand Soni 2013-06-07 07:39:13 PDT
Alright, thanks a lot! 
Can I know something about the mozilla try server and can I have a level 1 commit access to it now? Please let me know.
Comment 42 User image Ryan VanderMeulen [:RyanVM] 2013-06-07 07:42:07 PDT
If you want to request Level 1 commit access, you can follow the directions below.
http://www.mozilla.org/hacking/commit-access-policy/

I'd say there are plenty of people involved in this bug who can give you a vouch.
Comment 43 User image Anand Soni 2013-06-07 08:03:06 PDT
Thank you RyanVM for the guidance!

bz, djvj, sankha93.. can anyone vouch for me? I am filing a bug for level-1 access to the try server.
Comment 44 User image Ryan VanderMeulen [:RyanVM] 2013-06-07 08:29:45 PDT
https://hg.mozilla.org/mozilla-central/rev/0261df8a6a8e
Comment 45 User image Ryan VanderMeulen [:RyanVM] 2013-06-07 08:32:23 PDT
And because I'm an idiot who merged bustage over to m-c...
https://hg.mozilla.org/mozilla-central/rev/6cafe68983ca
Comment 46 User image Anand Soni 2013-06-07 09:03:00 PDT
Finally resolved! :)

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