Closed Bug 1014533 Opened 6 years ago Closed 4 years ago

spectrum.css should probably be in browser/themes, not browser/devtools

Categories

(DevTools :: Inspector, defect, P3)

defect

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: vporof, Assigned: slayslot)

Details

(Whiteboard: [good first bug][lang=html])

Attachments

(1 file, 3 obsolete files)

No description provided.
Whiteboard: [good first bug][lang=html]
I would love to take this up.
Hi Nikhil, sorry for the delay.
Would you still like to work on this ?
Flags: needinfo?(nikhil.handa19)
Priority: -- → P3
Hi Nicolas, Yes I'm still up for this.
Flags: needinfo?(nikhil.handa19)
Great, I assign it to you.
Assignee: nobody → nikhil.handa19
HI Nicolas, can you provide me more info on where `spectrum.css` is? I have made the simple firefox build but I can't find `spectrum.css` anywhere in it.
Flags: needinfo?(chevobbe.nicolas)
Hi Nikhil, the file is in devtools/client/shared/widgets/spectrum.css (https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/widgets/spectrum.css)
Flags: needinfo?(chevobbe.nicolas)
Thanks for the quick reply Nicolas, and where must it be moved to exactly?
Flags: needinfo?(chevobbe.nicolas)
Here https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes I guess. There are already other widgets' css files ( like tooltip.css, eyedropper.css, ...)
Flags: needinfo?(chevobbe.nicolas)
Hi, Nicolas. So, I was able to find 5 references to spectrum.css in 3 files. The 3 files are as follows:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/jar.mn
https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/widgets/spectrum-frame.xhtml
https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-pc-linux-gnu/faster/install_dist_bin_browser

Let me know if I'm missing any reference and if I don't need not change references in a particular file. Thanks for all the support.
Flags: needinfo?(chevobbe.nicolas)
I think you got it all ( dxr does a good job :) ). 
You don't need to change the reference in https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-pc-linux-gnu/faster/install_dist_bin_browser , because it's a file which is generated by the build process.
The reference in jar.mn tells Firefox to include it in the sources, and the reference in spectrum-frame.xhtml is just a regular stylesheet link.
So you can change those 2 with the updated path.
Flags: needinfo?(chevobbe.nicolas)
Attached patch bug-1014533-fix.patch (obsolete) — Splinter Review
Apologies for the delay. I was struggling with Mercurial. Let me know if this needs any more changes.
Attachment #8748126 - Flags: review?(paul)
Attachment #8748126 - Flags: review?(paul) → review?(hholmes)
Hi Nikhil, 
Thank you for this ! No problem for the delay, work at your own pace.

Paul is not in the devtools team anymore, so I asked Helen to check your patch. 

One comment I have though : I think you deleted the old file and recreated it in the new location ? If so, it would be better if you'd use `hg move` so we keep the file history.
Hi Nicolas,

Yes, that's exactly what I did. I had no clue about `hg move`, should I create the patch again?
Flags: needinfo?(chevobbe.nicolas)
Yes, it would be better if you rollback the changes on the deleted file and the added one, hg move spectrum.css, amend your commit and create a new patch.
Flags: needinfo?(chevobbe.nicolas)
Attached patch bug-1014533-fix-2.patch (obsolete) — Splinter Review
Here's the patch in which I used `hg move` to move `spectrum.css`
Attachment #8748145 - Flags: review?(hholmes)
Attachment #8748126 - Attachment is obsolete: true
Attachment #8748126 - Flags: review?(hholmes)
(In reply to Nikhil Handa[:slayslot] from comment #15)
> Created attachment 8748145 [details] [diff] [review]
> bug-1014533-fix-2.patch
> 
> Here's the patch in which I used `hg move` to move `spectrum.css`

Cool ! When you upload a new patch, it is good to flag the old one as obsolete (there is a checkbox in the attachment upload page), so the reviewer know which patch to look at.
>Cool ! When you upload a new patch, it is good to flag the old one as obsolete (there is a checkbox in the >attachment upload page), so the reviewer know which patch to look at.

Thanks, I was wondering how to do that. I'll keep that in mind in my future patches.
Comment on attachment 8748145 [details] [diff] [review]
bug-1014533-fix-2.patch

Review of attachment 8748145 [details] [diff] [review]:
-----------------------------------------------------------------

I think this looks good Nikhil—I had one small comment below which should only take a moment to fix. 

If you could fixup any additional commit messages for the change and append "r=helenvholmes" to your commit message, that would be great!

::: devtools/client/shared/widgets/spectrum-frame.xhtml
@@ +6,5 @@
>  
>  <html xmlns="http://www.w3.org/1999/xhtml">
>  <head>
>    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"/>
> +  <link rel="stylesheet" href="chrome://devtools/skin/spectrum.css" ype="text/css"/>

Looks like there's a "t" missing on "type".
Attachment #8748145 - Flags: review?(hholmes) → feedback+
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #18)
> Comment on attachment 8748145 [details] [diff] [review]
> bug-1014533-fix-2.patch
> 
> Review of attachment 8748145 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this looks good Nikhil—I had one small comment below which should
> only take a moment to fix. 
> 
> If you could fixup any additional commit messages for the change and append
> "r=helenvholmes" to your commit message, that would be great!
> 
> ::: devtools/client/shared/widgets/spectrum-frame.xhtml
> @@ +6,5 @@
> >  
> >  <html xmlns="http://www.w3.org/1999/xhtml">
> >  <head>
> >    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"/>
> > +  <link rel="stylesheet" href="chrome://devtools/skin/spectrum.css" ype="text/css"/>
> 
> Looks like there's a "t" missing on "type".

Hi, Helen.

Thanks for the review. I have made the change mentioned by you and as I mentioned before, I'm not very well versed with Mercurial as I am with git. If you could help me with the fixup of the additional commit message, that would be great. Thanks.
Hey Gabe,

Could you help out Nikhil with his Mercurial question? I'm actually on the git workflow, so I have no idea how to fixup and reword commit messages with Mercurial.
Flags: needinfo?(gl)
I think I can answer for Gabriel.

Nikhil, you should be able to do `hg commit --amend` which, like in git, will open your configured editor to edit your commit message.
Flags: needinfo?(gl)
Hey Nikhil, see Nicolas' answer above.

Thanks Nicolas!
Flags: needinfo?(nikhil.handa19)
Hi Nicolas and Helen,

Thanks for all the support. Nicolas, that command would indeed help me with changing the commit message, but since I had a typo in the original commit, and changing that gives me two commits, I would need the mercurial equivalent of git interactive rebase, in which I could squash the two commits together. Also, part of what makes the squashing process hard for me to understand is that I'm already in a Mercurial Queue. Also, how can we have a git workflow for mozilla-central as stated by Helen. I would love to get into that workflow as I'm much more familiar with git. Thanks again for all the support.
Flags: needinfo?(nikhil.handa19) → needinfo?(chevobbe.nicolas)
(In reply to Nikhil Handa[:slayslot] from comment #23)
> Also, how can we
> have a git workflow for mozilla-central as stated by Helen. I would love to
> get into that workflow as I'm much more familiar with git. Thanks again for
> all the support.

I can't answer the Mercurial-specific questions, but we have a git read-only mirror of mozilla-central here: https://github.com/mozilla/gecko-dev

My particular workflow is to use the moz-git-tools to create patches. You can find those here: https://github.com/mozilla/moz-git-tools
Hi Nikhil, I'm happy to help :)

I don't use MQ but I think you can take a look at https://www.mercurial-scm.org/wiki/EditingHistory to do what you want. On a side note, I think you could have only amended your first commit to address Helen's comments, at least, that's what I do. Usually, people uses multiple commits when the bug need several distinct changes (like, actually fixing the bug, and fixing eslint errors in the files).

If you more familiar with git, use what Helen says, it works well for several people on the team.
Flags: needinfo?(chevobbe.nicolas)
Hi Nicolas,

So I realized that what you are saying is indeed true. It's just a typo and can be accommodated with `hg commit --amend`. So, I fixed the typo, I ran `hg qrefresh` and the change was accommodated into the patch. But, After I tried running `hg commit --amend`, both before and after I made the change and before and after I ran `hg qrefresh`, it gives me the following error `abort: cannot commit over an applied mq patch`. Hoping this doesn't complicate things.
Flags: needinfo?(chevobbe.nicolas)
Hi Nikhil,

As said earlier, I'm not a user of MQ and can't help you much with it. At this point, given that your patch is quite small, what I'd do is strip your commits (with https://www.mercurial-scm.org/wiki/StripExtension) , import your patch with no commit ( hg import --no-commit https://bug1014533.bmoattachments.org/attachment.cgi\?id\=8748145 ), fix your typo in devtools/client/shared/widgets/spectrum-frame.xhtml and commit (`hg commit -m "Bug 1014533: Moved spectrum.css to /client/themes/ from /client/shared/widgets and changed the respective references made to it. r=helenvholmes"`). 

You should then have a clean patch to upload here.

By the way, If you want to have faster answers for questions about mercurial, the workflow or so, you can also ask on IRC and there surely will be someone to answer you. 

Hope it helps !
Flags: needinfo?(chevobbe.nicolas)
Attached patch bug-1014533-fix-3.patch (obsolete) — Splinter Review
Nevermind, I got it to work by adding the m switch to `hg qrefresh`. Let me know if this needs anymore changes. And again, thanks for making the process of getting my first patch done so smooth.
Attachment #8748145 - Attachment is obsolete: true
Attachment #8750012 - Flags: review?(hholmes)
This looks good to me. I think Helen will confirm in the beginning of the week. Thank you again.
Comment on attachment 8750012 [details] [diff] [review]
bug-1014533-fix-3.patch

Review of attachment 8750012 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks so much for your hard work Nikhil! One last request:

Could you remove the quotation marks around my name in the commit message? We use that particular syntax to make it easier to search. The commit message should end up looking like this:

Bug 1014533: Moved spectrum.css to /client/themes/ from /client/shared/widgets and changed the respective references made to it. r=helenvholmes

Thanks!
Attachment #8750012 - Flags: review?(hholmes) → feedback+
Hey, apologies for the mistake in the previous patch. Here's the new one with the requested changes.
Attachment #8750012 - Attachment is obsolete: true
Attachment #8750312 - Flags: review?(hholmes)
Attachment #8750312 - Flags: review?(hholmes) → review+
(In reply to Nikhil Handa[:slayslot] from comment #31)
> Created attachment 8750312 [details] [diff] [review]
> bug-1014533-fix-4.patch
> 
> Hey, apologies for the mistake in the previous patch. Here's the new one
> with the requested changes.

No problem—thank you very much!
Thank you both as well. First patch approved :D. I'm going to try to setup the git workflow now and then take up another bug. Please, let me know if I need to do something to get this one pushed and merged. Thanks again!
Hi Nikhil, great job :)


As discussed with Helen on IRC, I'm removing the checkin-needed keyword here.
Now that your patch has a granted review, we want to make sure it does not break think we could have missed at review stage (your patch is fairly simple, but for other thing there might be race conditions on slower platforms for example).

This is done by pushing your patch to the TRY server (https://wiki.mozilla.org/ReleaseEngineering/TryServer), with a commit which has a special syntax (http://trychooser.pub.build.mozilla.org/). Depending on this, the TRY server will run tests against platform you asked for (Windows, OSX, Linux, ...). 
If the tests are good, we then add the checkin-needed keyword on this bug, so a sheriff can push your patch to the fx-team tree. After that, there will be some other tests that will be run to ensure your patch can be merged into mozilla-central (it takes ~ 1 day). And when your patch land into mozilla-central, a sheriff will mark the bug as resolved. 

Pushing to try require to have a level 1 commit access (https://www.mozilla.org/en-US/about/governance/policies/commit/), which I think you don't have for now. So I'm gonna get your patch and push it to TRY, and I'll add a comment here with the link to the webapp where TRY results are displayed.

So to recap, here's what the full workflow looks like :
1. Ask for assignment
2. Fix the bug
3. Ask for feedback/review
4. When r+ push to TRY
5.a. if there are errors on TRY, back to step 2
5.b. if everything is fine, add "checkin-needed" in the keyword field of the bug

And then you should be good !

If you have questions about this, feel free to ask us.
Keywords: checkin-needed
Hey Nicolas,

Thanks for pushing it to TRY server for me. Let me know if we face any errors over there.
https://hg.mozilla.org/mozilla-central/rev/5d2a315b876e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.