Closed
Bug 1374609
Opened 7 years ago
Closed 7 years ago
Remove webideCli.js to avoid delaying Firefox first paint
Categories
(DevTools Graveyard :: WebIDE, enhancement, P3)
DevTools Graveyard
WebIDE
Tracking
(firefox56 fixed)
RESOLVED
FIXED
Firefox 56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: pbro, Assigned: foxt7ot)
Details
(Keywords: good-first-bug)
Attachments
(1 file, 2 obsolete files)
4.40 KB,
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
As part of Quantum/Photon, we are working on improving startup. More specifically, we care a lot about reducing the time it takes to first paint the browser window. Sometimes this means removing useless code. A new test was added to track what components are included at startup, before the first paint: browser_startup.js (http://searchfox.org/mozilla-central/rev/2bcd258281da848311769281daf735601685de2d/browser/base/content/test/performance/browser_startup.js). It detected webideCli.js. If I'm not mistaken, this handles the --webide command line argument that, when you start Firefox, actually just opens WebIDE instead. This was part of an old plan of ours to make WebIDE more like an actual IDE. This plan is obsolete, so we should remove this module (and associated build files, etc.)
Yes, seems fine to remove!
Reporter | ||
Comment 2•7 years ago
|
||
For context, this is the corresponding mailing list thread: https://groups.google.com/forum/#!topic/mozilla.dev.platform/6HFzXOb_tGQ. If someone wants to grab this bug, this should be an easy one to fix! Please first go through our contribution docs at http://docs.firefox-dev.tools Then, I think it's mostly going to be about deleting code: - deleting the webideCli file itself: http://searchfox.org/mozilla-central/source/devtools/client/webide/components/webideCli.js - then removing references to it in other files: http://searchfox.org/mozilla-central/source/devtools/client/webide/components/webideComponents.manifest http://searchfox.org/mozilla-central/source/devtools/client/webide/components/moz.build
Keywords: good-first-bug
Assignee | ||
Comment 3•7 years ago
|
||
Hi I want to work on this one.
Comment 4•7 years ago
|
||
Thank you Mohammed! Assigning the bug to you. On top of what Patrick said: you should be able to delete http://searchfox.org/mozilla-central/source/devtools/client/webide/components/webideComponents.manifest , because this manifest is only used to declare the webideCli.js component. Which means that in the end you can delete the whole webide/components folder. And update http://searchfox.org/mozilla-central/source/devtools/client/webide/moz.build (which references the folder). I think you will also have to delete the references to the deleted files in http://searchfox.org/mozilla-central/source/browser/installer/package-manifest.in Our docs are at http://docs.firefox-dev.tools in case you need pointers to get started (or you can needinfo us on this bug or ping on IRC #devtools).
Assignee: nobody → myaseen.khan
Status: NEW → ASSIGNED
Priority: -- → P3
Assignee | ||
Comment 5•7 years ago
|
||
Hi, Please find attached patch.
Attachment #8880645 -
Flags: review?(jryans)
Comment 6•7 years ago
|
||
Comment on attachment 8880645 [details] [diff] [review] Patch for the bug Review of attachment 8880645 [details] [diff] [review]: ----------------------------------------------------------------- Quick drive by review: the files of devtools/client/webide/components/ are still in the repository. They should be removed using > hg rm devtools/client/webide/components/
Comment 7•7 years ago
|
||
After reading the discussion on #devtools it looks like you had issues creating your patch. The documentation you found mentions a workflow that publishes your patch as a gist. To be honest, I'm not sure why, never seen this used here :)
For information, crafting a patch with mercurial is actually pretty easy:
> hg commit -m 'Bug 1374609 - your commit message. r=reviewer'
> hg export > /path/to/your/patch
This should create a properly formatted patch that you can attach here. It requires you to commit first, but then you can amend your commit if needed.
For in-progress reviews, attaching a diff as you did is fine though.
Comment on attachment 8880645 [details] [diff] [review] Patch for the bug Review of attachment 8880645 [details] [diff] [review]: ----------------------------------------------------------------- Looks like you are on the right track, but some files are missing, like :jdescottes mentions.
Attachment #8880645 -
Flags: review?(jryans)
Assignee | ||
Comment 9•7 years ago
|
||
Thanks julian for providing further details. Please find patch created after commit
Attachment #8880645 -
Attachment is obsolete: true
Attachment #8881122 -
Flags: review?(jryans)
Comment on attachment 8881122 [details] [diff] [review] Bug1374609.patch Review of attachment 8881122 [details] [diff] [review]: ----------------------------------------------------------------- Great, this looks good to me. Thanks! :) Can you change the commit message to say "r=jryans" instead of the full email? Then we can post this to try to see what tests say.
Attachment #8881122 -
Flags: review?(jryans) → review+
Assignee | ||
Comment 11•7 years ago
|
||
Hi@jryans, Thanks for the review comments. Please find attached new patch.
Attachment #8881122 -
Attachment is obsolete: true
Attachment #8882901 -
Flags: review?(jryans)
Comment on attachment 8882901 [details] [diff] [review] Patch Review of attachment 8882901 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this looks good to me! :) I submitted this to our try server CI system. Assuming it looks good, we can mark it for landing by setting "checkin-needed" in the keyword field of this bug, and a sheriff will land the patch. It will be marked FIXED once the patch merges to mozilla-central. https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cc64fe212b52a23d8ab8dfbe6a519d812bdc777
Attachment #8882901 -
Flags: review?(jryans) → review+
Tests look good, marking for checkin. Thanks again for helping out! :)
Keywords: checkin-needed
Comment 14•7 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5bc1108e125f Removed webide components and referneces as per issue. r=jryans
Keywords: checkin-needed
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5bc1108e125f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•4 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•