Closed Bug 1446923 Opened 7 years ago Closed 6 years ago

Remove some old references to chrome-metro

Categories

(Core :: General, enhancement)

Product:
Core
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: Sylvestre, Assigned: lifanfzeng, Mentored)

Details

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

Attachments

(4 files)

Bug 1446923 - Removed old references to chrome-metro
59 bytes, text/x-review-board-request
jlund
: review+
Details
Bug 1446923 - Removed old references to chrome-metro
59 bytes, text/x-review-board-request
jlund
: review+
Details
Removed old references to chrome-metro in mozilla-source\mozilla-central\python\mozbuild\mozbuild\test\frontend\test_emitter.py
65.16 KB, patch
Details | Diff | Splinter Review
Bug 1446923 - Remove Some Old References to Chrome-Metro
46 bytes, text/x-phabricator-request
jlund
: review+
Details | Review
(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?
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?
> 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
(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?
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
(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)
Yes, you should look for a reviewer! See comment #5
Flags: needinfo?(sledru)
Assignee: venkateshprabhu2 → nobody
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)
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 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 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+
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
See comment #19 :)
Flags: needinfo?(sledru) → needinfo?(venkateshprabhu2)
Venkatesh, any update?
Is this still available to work on?
Yes, please go ahead :)
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?
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)
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?
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.
Okay thanks I will look into it.
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?
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)
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?
yes, mozreview will be decommissioned soon. I don't know if phabricator is ready for external contributor yet. don't hesitate to try!
I tried to submit using phabricator, is this better?
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)
sure but this isn't probably the place to vouch for you :) you should follow the process!
Flags: needinfo?(sledru)
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
On treeherder, should I be running all jobs? I'm pretty confused by the interface.
Also should I be clicking the black or the orangey thing?
You should just run the one which were failing in comment #19
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?
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)
you should focus on the one reported in comment #19. The failures you see seem unrelated.
Flags: needinfo?(sledru)
Oh I (think I) fixed that already: Because I deleted the flavor:metro-chrome entry in metadata, I just changed the assertion to 8.
As in I used to get the same error as comment #19 but now I don't.
I have a bunch of commits, but only some of them are relevant. How do I get rid of the rest?
Please have a look to the hg documentation for that :)
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.
Attachment #8999667 - Flags: review?(jlund)
Attachment #8999667 - Flags: review?(jlund)
Can you review my patch please Sylvestre?
Flags: needinfo?(sledru)
Can you review my patch please Jordan? Sylvestre is busy.
Flags: needinfo?(jlund)
needinfo: see phabricator
Flags: needinfo?(jlund) → needinfo?(lifanfzeng)
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+
Pushed by sledru@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3d81b4beb659 Remove Some Old References to Chrome-Metro r=jlund
https://hg.mozilla.org/mozilla-central/rev/3d81b4beb659
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Bravo :)
Flags: needinfo?(sledru)
Assignee: nobody → lifanfzeng
Flags: needinfo?(jlund)
Attachment #8999667 - Flags: review?(jlund)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: