Remove webideCli.js to avoid delaying Firefox first paint

RESOLVED FIXED in Firefox 56

Status

P3
normal
RESOLVED FIXED
a year ago
3 months ago

People

(Reporter: pbro, Assigned: foxt7ot)

Tracking

({good-first-bug})

unspecified
Firefox 56
good-first-bug

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

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

a year ago
Hi
I want to work on this one.
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

a year ago
Created attachment 8880645 [details] [diff] [review]
Patch for the bug

Hi,
Please find attached patch.
Attachment #8880645 - Flags: review?(jryans)
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/
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

a year ago
Created attachment 8881122 [details] [diff] [review]
Bug1374609.patch

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

a year ago
Created attachment 8882901 [details] [diff] [review]
Patch

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

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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5bc1108e125f
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Updated

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