Closed Bug 1426634 Opened 8 years ago Closed 8 years ago

Rename devtools/client/shared/components/Tree.js to VirtualizedTree.js

Categories

(DevTools :: Shared Components, enhancement, P3)

Product:
DevTools
Component:
Shared Components
Type:
enhancement

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: nchevobbe, Assigned: leo89214749, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(1 file, 1 obsolete file)

Bug 1426634 - Rename devtools/client/shared/components/Tree.js to VirtualizedTree.js;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
We have different implementation of Tree components, which serves different needs. devtools/client/shared/components/Tree.js is used in the Memory panel (and others), and is doing some virtualization to make it very fast. This is great but only works when you have fixed height Tree nodes. Renaming the filename (as well as what is exported in the file) to VirtualizedTree would better express what the component is doing, and if it fits your need. When the component is renamed, we should take care of all the require call to it (see https://searchfox.org/mozilla-central/search?q=shared%2Fcomponents%2FTree&case=true&regexp=false&path=)
Mentor: nchevobbe
Keywords: good-first-bug
Priority: -- → P3
Hi Nicolas! I think I can fix the bug and I have already followed Linux build preparation(https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build/Linux_and_MacOS_build_preparation) and do ./mach build successfully on my virtual box with Ubuntu.
(In reply to Huang Li-Pang from comment #1) > Hi Nicolas! I think I can fix the bug and I have already followed Linux > build > preparation(https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/ > Build_Instructions/Simple_Firefox_build/Linux_and_MacOS_build_preparation) > and do ./mach build successfully on my virtual box with Ubuntu. Great ! Don't forget to rename the file using `hg mv` since we want to keep the history ;)
Hi Nicolas! I made the change by using 'hg mv' and commit it. I used 'hg export' and got the message and paste it to the attachment.
Attachment #8938607 - Flags: feedback?(nchevobbe)
Attachment #8938607 - Attachment is patch: true
Attachment #8938607 - Flags: feedback?(nchevobbe) → feedback-
Comment on attachment 8938607 [details] [diff] [review] Bug 1426634 - Rename devtools/client/shared/components/Tree.js to VirtualizedTree.js The file is not a valid patch according to bugzilla. But i'm not sure what's missing. I think there should be a ";" before r=nchevobbe. Also, could you try to use mozreview to submit patches ? http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html It makes things easier for both the reviewer and the person who submit the patch. Thanks !
(In reply to Nicolas Chevobbe [:nchevobbe] (On PTO until 2018-01-03) from comment #4) > Comment on attachment 8938607 [details] [diff] [review] > Bug 1426634 - Rename devtools/client/shared/components/Tree.js to > VirtualizedTree.js > > The file is not a valid patch according to bugzilla. But i'm not sure what's > missing. > > I think there should be a ";" before r=nchevobbe. > Also, could you try to use mozreview to submit patches ? > http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/ > install.html > > It makes things easier for both the reviewer and the person who submit the > patch. Thanks ! Hi Nicolas! I added a semicolon before r=nchevobbe and do "hg push review", but I cannot push. It says I need to do web login to https://reviewboard.mozilla.org/account/login, I have already login in Bugzilla on the web.
(In reply to Huang Li-Pang from comment #5) > (In reply to Nicolas Chevobbe [:nchevobbe] (On PTO until 2018-01-03) from > comment #4) > > Comment on attachment 8938607 [details] [diff] [review] > > Bug 1426634 - Rename devtools/client/shared/components/Tree.js to > > VirtualizedTree.js > > > > The file is not a valid patch according to bugzilla. But i'm not sure what's > > missing. > > > > I think there should be a ";" before r=nchevobbe. > > Also, could you try to use mozreview to submit patches ? > > http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/ > > install.html > > > > It makes things easier for both the reviewer and the person who submit the > > patch. Thanks ! > > Hi Nicolas! > I added a semicolon before r=nchevobbe and do "hg push review", but I cannot > push. It says I need to do web login to > https://reviewboard.mozilla.org/account/login, I have already login in > Bugzilla on the web. Hello :) Did you follow the steps from http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html ?
Hi Nicolas! Yes! I followed the steps and stuck in the LDAP account step. But I finally found the only thing I need to do is join the reviewboard group. I joined the group and pushed to the mozreview. https://reviewboard.mozilla.org/r/209494 Thank you for helping me!
Comment on attachment 8939073 [details] Bug 1426634 - Rename devtools/client/shared/components/Tree.js to VirtualizedTree.js; https://reviewboard.mozilla.org/r/209494/#review215248 Hello Leo, seems like you have everything set up now, and the file is well renamed. Now we need to reflech this filename change in the codebase where we are requiring this component. So for all the occcurences in https://searchfox.org/mozilla-central/search?q=require(%22devtools%2Fclient%2Fshared%2Fcomponents%2FTree%22)&case=false&regexp=false&path= , we need to change Tree to VirtualizedTree so the consumer load the correct component
Attachment #8939073 - Flags: review?(nchevobbe) → review-
Hi Nicolas, So I need to change all the path in these files form Tree to VirtualizedTree. Is that right?
Yes !
OK! I have changed all the path in files. Should I commit it again or edit last commit?
Amending your commit should be fine :)
OK! I amended the commit and pushed to mozreview. https://reviewboard.mozilla.org/r/209494/ P.S. I think Mercurial is amazing. It's first time I use "hg commit --amend". I just type the command and hg itself do the amend for me. It is very different from git. Very convenient. Thank you Nicolas!
Comment on attachment 8939073 [details] Bug 1426634 - Rename devtools/client/shared/components/Tree.js to VirtualizedTree.js; https://reviewboard.mozilla.org/r/209494/#review215336 Sounds good to me, thanks Leo ! The next step is pushing your patch to TRY, where all the tests will be run to make sure the patch is safe.
Attachment #8939073 - Flags: review?(nchevobbe) → review+
Here's the treeherder link https://treeherder.mozilla.org/#/jobs?repo=try&revision=e83e77dec85d87427739f251b6f70c5d6eb19d46 where you can watch all the jobs (i.e. chunks of tests) being run.
OK! So what should I do next? Should I push the patch to TRY? Or just watch the jobs down.
Comment on attachment 8938607 [details] [diff] [review] Bug 1426634 - Rename devtools/client/shared/components/Tree.js to VirtualizedTree.js Marking as obsolete since the patch is on mozreview now
Attachment #8938607 - Attachment is obsolete: true
So, the TRY jobs are done, and it says the build is busted : https://treeherder.mozilla.org/#/jobs?repo=try&revision=e83e77dec85d87427739f251b6f70c5d6eb19d46&selectedJob=153802535 I think it may be because in https://searchfox.org/mozilla-central/rev/51cd1093c5c2113e7b041e8cc5c9bf2186abda13/devtools/client/shared/components/moz.build#24, we need to modify the name as well. After doing that, you should be able to do a `./mach build faster && ./mach run` and see a Firefox window opening. After that point, you can amend your commit, push to review again, and we'll push to TRY to actually have the tests running :) Thanks !
Assignee: nobody → leo89214749
Status: NEW → ASSIGNED
OK! I changed the name 'Tree.js' in DevToolsModules to 'VirtualizedTree.js' and do './mach build faster && ./mach run' and see a Firefox window open successfully! Thank you for helping me!
Nice ! Let's push to TRY again to see if everything is right
So, is the patch ok?
No, there are still some errors : https://treeherder.mozilla.org/#/jobs?repo=try&revision=d52c88d3d142 You need to click the different orange button to see what the failures are. Some are not related to your patch (we call them intermittents, because they do not fail every time), some are definitely because of the patch. It may be places where the component is required by relative path like https://searchfox.org/mozilla-central/source/devtools/client/performance/components/jit-optimizations.js#14 :/ I think it is just this one. So we can fix it (and use an absolute path to be consistent with all the other places where we require this component)
Thank you for answering. I learned lot of things from you!
Comment on attachment 8939073 [details] Bug 1426634 - Rename devtools/client/shared/components/Tree.js to VirtualizedTree.js; https://reviewboard.mozilla.org/r/209494/#review216562 Thanks Leo. There's only a small nit left. After you fixed it, we will push to TRY again and hopefully land this patch if everything is correct. ::: devtools/client/performance/components/jit-optimizations.js:14 (Diff revision 5) > > const { assert } = require("devtools/shared/DevToolsUtils"); > const { Component, createFactory } = require("devtools/client/shared/vendor/react"); > const dom = require("devtools/client/shared/vendor/react-dom-factories"); > const PropTypes = require("devtools/client/shared/vendor/react-prop-types"); > -const Tree = createFactory(require("../../shared/components/Tree")); > +const Tree = createFactory(require("../../shared/components/VirtualizedTree")); Could you use the absolute path (`const Tree = createFactory(require("devtools/client/shared/components/VirtualizedTree"));`) so we are consistent with all the other places where we require this file ?
OK! I changed to absolute path and pushed to mozreview!
okay, try looks a bit weird but I don't think there's anything related to your patch. Let's land it !
Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ec12b49871a4 Rename devtools/client/shared/components/Tree.js to VirtualizedTree.js; r=nchevobbe
Wow thank you a lot, Nicolas.
Thank you for helping us and congrats landing your first patch ! Is there anything you might be interested in for your next patch ? if you plan to contribute and have enough time of course, no pressure at all !
Yes! I am interested in C++ and JavaScript and I will find the bugs through Bugs Ahoy! I like to contribute and I think I have enough time. It is really fun since the code I write will be part of firefox.
https://hg.mozilla.org/mozilla-central/rev/ec12b49871a4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: