Remove some old references to chrome-metro

RESOLVED FIXED in Firefox 63

Status

()

RESOLVED FIXED
9 months ago
4 months ago

People

(Reporter: sylvestre, Assigned: lifanfzeng, Mentored)

Tracking

({good-first-bug})

Trunk
mozilla63
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox61 wontfix, firefox62 wontfix, firefox63 fixed)

Details

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

Attachments

(4 attachments)

(In reply to Sylvestre Ledru [:sylvestre] from comment #0)
> These is now useless.
> 
> Should probably be done in two separate commits with different reviewers.
> 
> commit 1
> https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/
> testing.py#59
> https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/test/
> frontend/test_emitter.py#822
> 
> commit 2
> https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/configs/
> unittests/win_taskcluster_unittest.py#179
> https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/configs/
> unittests/win_unittest.py#171

Hi Sylvestre,

I am interested to work on this. Does this need deep knowledge of Javascript or it's about removing the references and then see if things look okay?
(Reporter)

Comment 2

9 months ago
The later ;)
This should be pretty easy!
(In reply to Sylvestre Ledru [:sylvestre] from comment #2)
> The later ;)
> This should be pretty easy!

Alright, thank you. May I know the reviewers for the two commits?
(In reply to Sylvestre Ledru [:sylvestre] from comment #2)
> The later ;)
> This should be pretty easy!

Also, should the two commits reference the same bug?
(Reporter)

Comment 5

9 months ago
> Alright, thank you. May I know the reviewers for the two commits?
See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction#Step_4_-_Get_your_code_reviewed

> Also, should the two commits reference the same bug?
Yes
Comment hidden (mozreview-request)
(In reply to Sylvestre Ledru [:sylvestre] from comment #5)
> > Alright, thank you. May I know the reviewers for the two commits?
> See
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Introduction#Step_4_-_Get_your_code_reviewed
> 
> > Also, should the two commits reference the same bug?
> Yes

Thank you.

I submitted first commit for review. Should I wait for this to get reviewed and then push the next commit?
(Reporter)

Comment 8

9 months ago
Thanks for your patch.
You should submit the two commits here.
You can do that with:
hg push -r <revision_1>:<revision_2> review
Assignee: nobody → venkateshprabhu2
Comment hidden (mozreview-request)
(In reply to Sylvestre Ledru [:sylvestre] from comment #8)
> Thanks for your patch.
> You should submit the two commits here.
> You can do that with:
> hg push -r <revision_1>:<revision_2> review

Done. Thank you.
Flags: a11y-review?
Flags: a11y-review?
(In reply to Sylvestre Ledru [:sylvestre] from comment #8)
> Thanks for your patch.
> You should submit the two commits here.
> You can do that with:
> hg push -r <revision_1>:<revision_2> review

Do I have to do anything else w.r.t this? Should I set a review flag?
Flags: needinfo?(sledru)
(Reporter)

Comment 12

9 months ago
Yes, you should look for a reviewer!
See comment #5
Flags: needinfo?(sledru)
Assignee: venkateshprabhu2 → nobody
(Reporter)

Comment 13

8 months ago
vprabhu, do you need help to finish this patch?
Flags: needinfo?(venkateshprabhu2)
(In reply to Sylvestre Ledru [:sylvestre] from comment #13)
> vprabhu, do you need help to finish this patch?

I couldn't figure the reviewers. You are the only suggested reviewer for this bug. Is the triage owner also a reviewer?
Flags: needinfo?(venkateshprabhu2)
(Reporter)

Comment 15

8 months ago
from the hg log, jlund is a potential reviewer!
Flags: needinfo?(venkateshprabhu2)
Flags: needinfo?(venkateshprabhu2)
Attachment #8960580 - Flags: review?(jlund)
Attachment #8960643 - Flags: review?(jlund)

Comment 16

8 months ago
mozreview-review
Comment on attachment 8960580 [details]
Bug 1446923 - Removed old references to chrome-metro

https://reviewboard.mozilla.org/r/229324/#review244270
Attachment #8960580 - Flags: review?(jlund) → review+

Comment 17

8 months ago
mozreview-review
Comment on attachment 8960643 [details]
Bug 1446923 - Removed old references to chrome-metro

https://reviewboard.mozilla.org/r/229424/#review244272

stamping this but fyi, I'm not familiar with this harness. The patch looks harmless though so I'm less concerned. I am assuming that if this was scheduled via Taskcluster, that scheduling logic was removed elsewhere.
Attachment #8960643 - Flags: review?(jlund) → review+

Comment 18

8 months ago
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bc5ab6e2db10
Removed old references to chrome-metro r=jlund
https://hg.mozilla.org/integration/autoland/rev/b9a5aab21d71
Removed old references to chrome-metro r=jlund
(Reporter)

Comment 20

8 months ago
See comment #19 :)
Flags: needinfo?(sledru) → needinfo?(venkateshprabhu2)
(Reporter)

Comment 21

7 months ago
Venkatesh, any update?
(Assignee)

Comment 22

4 months ago
Is this still available to work on?
(Reporter)

Comment 23

4 months ago
Yes, please go ahead :)
(Assignee)

Comment 24

4 months ago
Hey I have some questions.

commit 1
https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/testing.py#59
https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/test/frontend/test_emitter.py#822

The second link appears to just link to a bracket, which doesn't seem to call the metro-chrome part here https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/test/frontend/test_emitter.py#870 . Should I be deleting just the stuff on lines 870-875, or 818-822, or both.



commit 2
https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/configs/unittests/win_taskcluster_unittest.py#179
https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/configs/unittests/win_unittest.py#171

The two jsreftests don't seem to obviously use chrome-metro, but does this commit just ask for them to be removed?
(Reporter)

Comment 25

4 months ago
Commit 1, Seem that #870 is the right approach

For commit 2, you should experiment using try to see what fixes the issue!
Flags: needinfo?(venkateshprabhu2)
(Assignee)

Comment 26

4 months ago
For commit 2, does that mean I should be trying out various things to see what works? Or is "try" a mach/mozilla specific thing?
(Reporter)

Comment 27

4 months ago
Yes, this is a mozilla specific thing: https://wiki.mozilla.org/ReleaseEngineering/TryServer
You can use it to see if the issue can be reproduce and if you can fix it.
(Assignee)

Comment 28

4 months ago
Okay thanks I will look into it.
(Assignee)

Comment 29

4 months ago
Created attachment 8999659 [details] [diff] [review]
Removed old references to chrome-metro in  mozilla-source\mozilla-central\python\mozbuild\mozbuild\test\frontend\test_emitter.py
Attachment #8999659 - Flags: review?(sledru)
(Assignee)

Comment 30

4 months ago
Who else should I ask for a review? When I try using hg blame I says "no such file in rev f650c0df72f9", but I'm not really sure what that means. Also did I submit the patch correctly?
(Reporter)

Comment 31

4 months ago
Comment on attachment 8999659 [details] [diff] [review]
Removed old references to chrome-metro in  mozilla-source\mozilla-central\python\mozbuild\mozbuild\test\frontend\test_emitter.py

You uploaded the whole file. We expect a patch.
See some doc here:
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
Attachment #8999659 - Flags: review?(sledru)
(Assignee)

Comment 32

4 months ago
Okay I'll fix that. Also I shouldn't be using Mozreview right? I'm seeing a lot of things about using it but remember reading that it's being deprecated for phabricator?
(Reporter)

Comment 33

4 months ago
yes, mozreview will be decommissioned soon. I don't know if phabricator is ready for external contributor yet. don't hesitate to try!
(Assignee)

Comment 34

4 months ago
Created attachment 8999667 [details]
Bug 1446923 - Remove Some Old References to Chrome-Metro
(Assignee)

Comment 35

4 months ago
I tried to submit using phabricator, is this better?
(Assignee)

Comment 36

4 months ago
Also I think I need a voucher https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/ to get the level 1 commit access I need for the try server.
Flags: needinfo?(sledru)
(Reporter)

Comment 37

4 months ago
sure but this isn't probably the place to vouch for you :)
you should follow the process!
Flags: needinfo?(sledru)
(Assignee)

Comment 38

4 months ago
Here's my vouch bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1483024
(Assignee)

Comment 39

4 months ago
Also what is a Mozilla LDAP account? I tried looking it up, but only really found https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Quick_start_guide/Initial_setup
(Assignee)

Comment 40

4 months ago
On treeherder, should I be running all jobs? I'm pretty confused by the interface.
(Assignee)

Comment 41

4 months ago
Also should I be clicking the black or the orangey thing?
(Reporter)

Comment 42

4 months ago
You should just run the one which were failing in comment #19
(Assignee)

Comment 43

4 months ago
My treeherder seems to have a lot of bustages, 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=45f19dacd664404cbf31333d2a75eb1193c3f2d7

bug I (should) have the same changes as in comment #19. Are the tests in 19 the ones I'm supposed to be running? Also how come when I click view tests it says 

"No failed tests to show"


Also when I click on the the link for push with failures


https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b9a5aab21d71b786f5196b1c1cbf59890a0c41fc&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=running&filter-classifiedState=unclassified&selectedJob=175811609


It highlights that Bb* of linux x64 opt failed, but if I click another test, Bb* disappears?
(Assignee)

Comment 44

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=719c3ce4e90de73eda95e966e8f6e790984414f5

Hey I ran all tests, but not sure how to fix the failures. Please advise. Also for the valgrind error I'm not sure where to find 

style::properties::longhands::clip_path::cascade_property / style::properties::cascade_rules / ?!? / ?!?
Flags: needinfo?(sledru)
(Reporter)

Comment 45

4 months ago
you should focus on the one reported in comment #19. The failures you see seem unrelated.
Flags: needinfo?(sledru)
(Assignee)

Comment 46

4 months ago
Oh I (think I) fixed that already: Because I deleted the flavor:metro-chrome entry in metadata, I just changed the assertion to 8.
(Assignee)

Comment 47

4 months ago
As in I used to get the same error as comment #19 but now I don't.
(Assignee)

Comment 48

4 months ago
I have a bunch of commits, but only some of them are relevant. How do I get rid of the rest?
(Reporter)

Comment 49

4 months ago
Please have a look to the hg documentation for that :)
(Assignee)

Comment 50

4 months ago
Hey I think I managed to upload all my changes to the phabricator, https://phabricator.services.mozilla.com/D3223, I'm not sure how to add other reviewers though.
(Assignee)

Updated

4 months ago
Attachment #8999667 - Flags: review?(jlund)
(Assignee)

Updated

4 months ago
Attachment #8999667 - Flags: review?(jlund)
(Assignee)

Comment 51

4 months ago
Can you review my patch please Sylvestre?
Flags: needinfo?(sledru)
(Assignee)

Comment 52

4 months ago
Can you review my patch please Jordan? Sylvestre is busy.
Flags: needinfo?(jlund)
needinfo: see phabricator
Flags: needinfo?(jlund) → needinfo?(lifanfzeng)
(Assignee)

Comment 54

4 months ago
Hey I updated with a treeherder run.
Flags: needinfo?(lifanfzeng) → needinfo?(jlund)
Comment on attachment 8999667 [details]
Bug 1446923 - Remove Some Old References to Chrome-Metro

Jordan Lund (:jlund) has approved the revision.
Attachment #8999667 - Flags: review+

Comment 56

4 months ago
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3d81b4beb659
Remove Some Old References to Chrome-Metro r=jlund

Comment 57

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3d81b4beb659
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(Reporter)

Comment 58

4 months ago
Bravo :)
Flags: needinfo?(sledru)
(Reporter)

Updated

4 months ago
Assignee: nobody → lifanfzeng
Flags: needinfo?(jlund)
status-firefox61: affected → wontfix
status-firefox62: --- → wontfix

Updated

4 months ago
Attachment #8999667 - Flags: review?(jlund)
You need to log in before you can comment on or make changes to this bug.