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

RESOLVED FIXED in Firefox 59

Status

enhancement
P3
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: nchevobbe, Assigned: leo89214749, Mentored)

Tracking

({good-first-bug})

Trunk
Firefox 59

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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=)
Reporter

Updated

a year ago
Mentor: nchevobbe
Keywords: good-first-bug
Priority: -- → P3
Assignee

Comment 1

a year ago
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.
Reporter

Comment 2

a year ago
(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 ;)
Assignee

Comment 3

a year ago
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.
Reporter

Updated

a year ago
Attachment #8938607 - Flags: feedback?(nchevobbe)
Reporter

Updated

a year ago
Attachment #8938607 - Attachment is patch: true
Attachment #8938607 - Flags: feedback?(nchevobbe) → feedback-
Reporter

Comment 4

a year ago
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 !
Assignee

Comment 5

a year ago
(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.
Reporter

Comment 6

a year ago
(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 ?
Comment hidden (mozreview-request)
Assignee

Comment 8

a year ago
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!
Reporter

Comment 9

a year ago
mozreview-review
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-
Assignee

Comment 10

a year ago
Hi Nicolas,
So I need to change all the path in these files form Tree to VirtualizedTree. Is that right?
Yes !
Assignee

Comment 12

a year ago
OK!
I have changed all the path in files. Should I commit it again or edit last commit?
Amending your commit should be fine :)
Comment hidden (mozreview-request)
Assignee

Comment 15

a year ago
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 hidden (mozreview-request)
Reporter

Comment 17

a year ago
mozreview-review
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.
Assignee

Comment 19

a year ago
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 !
Reporter

Updated

a year ago
Assignee: nobody → leo89214749
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Assignee

Comment 23

a year ago
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
Assignee

Comment 25

a year ago
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)
Comment hidden (mozreview-request)
Assignee

Comment 28

a year ago
Thank you for answering. I learned lot of things from you!
Reporter

Comment 29

a year ago
mozreview-review
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 ?
Comment hidden (mozreview-request)
Assignee

Comment 31

a year ago
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 !

Comment 34

a year ago
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
Assignee

Comment 35

a year ago
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 !
Assignee

Comment 37

a year ago
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
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.