Remove new Function from places tree.xml
Categories
(Firefox :: Bookmarks & History, enhancement, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox67 | --- | fixed |
People
(Reporter: vinoth, Assigned: jallmann)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
Reporter | ||
Comment 1•6 years ago
|
||
Comment 2•6 years ago
|
||
Comment 3•6 years ago
|
||
Hey, Can I work on this issue? If at all, how do I start?
Updated•6 years ago
|
Reporter | ||
Updated•6 years ago
|
Assignee | ||
Comment 4•6 years ago
|
||
(In reply to Vinothkumar Nagasayanan [:vinoth] from comment #1)
I am working[3] on adding a
custom event handler instead of using attribute in xul files. I will send
the patch for review after completing it.
Hi Vinoth, am I right that this patch is partly finished and I could use it as a starting point to fix this bug? If yes, could you give me a short hint on what is still missing / not working?
Reporter | ||
Comment 5•6 years ago
|
||
(In reply to Jonas Allmann [:jallmann] from comment #4)
(In reply to Vinothkumar Nagasayanan [:vinoth] from comment #1)
I am working[3] on adding a
custom event handler instead of using attribute in xul files. I will send
the patch for review after completing it.Hi Vinoth, am I right that this patch is partly finished and I could use it as a starting point to fix this bug? If yes, could you give me a short hint on what is still missing / not working?
Yes this patch is only partly complete. I tried to fire a custom event inside treeView.js, which will run the function inside places.js.
But because of some issue with this patch, this custom event is not triggered. You can use this patch or start your own approach from scratch.
You can take a look at a similar Bug 1521040 for reference. In Bug 1521040 we didn't remove the new Function for now, but in this bug you need to create event listener and remove the new Function from tree.xml.
Assignee | ||
Comment 6•6 years ago
|
||
Removed occurence of (new Function) in tree.xml as part of Bug 1473549. Adjusted affected code by adding custom event handler.
Assignee | ||
Comment 7•6 years ago
|
||
Thanks for the review, I applied the small change you suggested.
I haven't yet passed this patch by the try server, which would be recommended before landing, I guess?
Could you give me a hint on what would be a reasonable try push for this?
It's one of my very first patches in mozilla-central, and I have only ever interacted with the nss-try repository, so I'm not familiar yet with try for mozilla central.
Comment 8•6 years ago
|
||
(In reply to Jonas Allmann [:jallmann] from comment #7)
Thanks for the review, I applied the small change you suggested.
I haven't yet passed this patch by the try server, which would be recommended before landing, I guess?
Could you give me a hint on what would be a reasonable try push for this?
It's one of my very first patches in mozilla-central, and I have only ever interacted with the nss-try repository, so I'm not familiar yet with try for mozilla central.
Well, the only files you're modifying are in browser/, which indicates only desktop platforms will be affected. Because you're not modifying any gecko internals, you don't really need crashtest/reftest/webplatform-test/mochitest-plain. Because this is UI-only code, there's no point running xpcshell. Because it's JS/frontend-only, you can run just artifact builds (instead of compiling anything). Really, the only things you'd need to run are desktop mochitest-browser-chrome (e10s / non-e10s), mochitest-chrome, mochitest-clipboard (which is basically mochitests that require clipboard access, which are a bit finnicky) and marionette, marionette-e10s and firefox-ui-functional (the python-based functional tests).
You can achieve this using ./mach try fuzzy --artifact and selecting the tests you need, or you can use something like:
./mach try -b od -p win64,macosx64,linux64,linux64-asan -t none -u mochitest-bc,mochitest-clipboard,mochitest-chrome,marionette,marionette-e10s,firefox-ui-functional --artifact
(The e10s version of mochitest-bc should get run automatically without having to specify it.)
Note that I commented on phab that you haven't quite applied the nit that I asked for. :-)
Comment 9•6 years ago
|
||
Oh, and there's no real point running the pgo tests when checkign ./mach try fuzzy - opt and dbg should be fine. If the syntax confuses you (I have no idea how nss-try works and if it's similar or not), https://mozilla-releng.net/trychooser/ can help demystify some of it.
Comment 10•6 years ago
|
||
https://hg.mozilla.org/integration/autoland/rev/bb7602ef747f
pushbot seems to be broken atm?
Comment 11•6 years ago
|
||
bugherder |
Description
•