convert uses of "defer" to "new Promise" in client/canvasdebugger

RESOLVED FIXED in Firefox 61

Status

()

Firefox
Developer Tools: Canvas Debugger
P3
normal
RESOLVED FIXED
a year ago
a month ago

People

(Reporter: mkohler, Assigned: Valentin Sánchez)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 61
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Attachment #8856226 - Flags: review?(nchevobbe)

Comment 2

a year ago
mozreview-review
Comment on attachment 8856226 [details]
Bug 1354883 - convert uses of 'defer' to 'new Promise' in client/canvasdebugger

https://reviewboard.mozilla.org/r/128174/#review130792

Thanks ! r+ if TRY is green
Attachment #8856226 - Flags: review?(nchevobbe) → review+

Comment 3

a year ago
mozreview-review
Comment on attachment 8856226 [details]
Bug 1354883 - convert uses of 'defer' to 'new Promise' in client/canvasdebugger

https://reviewboard.mozilla.org/r/128174/#review130810

::: commit-message-35c7b:1
(Diff revision 1)
> +Bug 1354883 - convert uses of 'defer' to 'new Promise' in client/canvasdebugger

add r=nchevobbe at the end
(Reporter)

Comment 4

11 months ago
Hi Nicolas

As far as I know autoland will add the r= automatically. I don't think I have the permission to land this myself (or it's not possible due to the remaining review issue raised?), could you please do this for me?

Cheers,
Michael
Flags: needinfo?(nchevobbe)
Hello Michael,

> As far as I know autoland will add the r= automatically.

Oh nice, I didn't knew that.
Let me push this to TRY and then I'll land the commit (or you can, I think it's blocked by the issue on the review, which I'll remove)
Flags: needinfo?(nchevobbe)
(Reporter)

Comment 6

11 months ago
I'll need to work a little bit more on this, seems like tests are failing and I'm not 100% sure that wasn't me. Will investigate. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b15f0bc3f3ba2c3d11642ba4519ded01db51e31a&selectedJob=99314637
I re-triggered the failing tests to make sure, but I don't think those failures are related to your patch
Ooops, ignore Comment 7, i thought this was the webaudio bug
(Reporter)

Comment 9

11 months ago
I'll have another look here soon.
(Reporter)

Comment 10

8 months ago
Unassigning myself as there has been no recent action from my side and I don't see myself doing any action on this for the next few weeks.

Sorry about this, if somebody wants to take this, go for it! :)
Assignee: me → nobody
Status: ASSIGNED → NEW

Updated

5 months ago
Priority: -- → P3
(Assignee)

Comment 11

2 months ago
I'm new to the community and would like to work on this.
(Assignee)

Comment 12

2 months ago
I'm not sure how to tell if a test has failed, but it seems to me that the current patch doesn't failed any tests.
Hello Valentin, I pushed to TRY again (our CI infra) to see if the patch is good to land. If not, we'll talk about what's left to do :)
Okay, so it seems like there's errors left in the current patch (See https://treeherder.mozilla.org/#/jobs?repo=try&revision=bad4abfe79c1b997b188cf70ee29e01d8f931103&selectedJob=165933242).

Valentin, I assigned you the issue. Do you need anything to get started ? Feel free to ask any question !
Assignee: nobody → valentin2507
(Assignee)

Comment 15

2 months ago
Is there a way I can run the tests on my local version (my own machine)?
Yes, you can have a look at http://docs.firefox-dev.tools/tests/mochitest-devtools.html

So in your case, this would do it: 

.mach test devtools/client/canvasdebugger/test/
Comment hidden (mozreview-request)
Attachment #8856226 - Attachment is obsolete: true
(Assignee)

Comment 18

a month ago
Thanks for your help!

I made sure that tests were passing in my local installation. Here is the patch. Let me know if something should be changed.

https://reviewboard.mozilla.org/r/227436/

Comment 19

a month ago
mozreview-review
Comment on attachment 8958510 [details]
Bug 1354883 - convert uses of 'defer' to 'new Promise' in client/canvasdebugger

https://reviewboard.mozilla.org/r/227436/#review233254

Thanks a lot Valentin, the patch looks good to me !
But I couldn't test it because it does not apply on the latest mozilla-central.
Can you update your repo and rebase your patch on top of the latest change ? 
I am sorry there will be quite some conflicts :/

::: commit-message-a4ef1:1
(Diff revision 1)
> +Bug 1354883 - convert uses of 'defer' to 'new Promise' in client/canvasdebugger

Here you need to add the nick of the reviewer: 

Bug 1354883 - convert uses of 'defer' to 'new Promise' in client/canvasdebugger; r=nchevobbe
Attachment #8958510 - Flags: review?(nchevobbe)
(Assignee)

Comment 20

a month ago
mozreview-review-reply
Comment on attachment 8958510 [details]
Bug 1354883 - convert uses of 'defer' to 'new Promise' in client/canvasdebugger

https://reviewboard.mozilla.org/r/227436/#review233254

I run hg pull && hg update before commiting changes. Also, I did the same thing right now and no changes were found.
I tried hg rebase, but I got
"abort: branch 'default' has 3 heads - please rebase to an explicit rev"

I'm more used to git and mercurial is new for me. Is hg rebase what I should be running instead? How can I solve the above problem?
(In reply to Valentin Sánchez from comment #20)
> Comment on attachment 8958510 [details]
> Bug 1354883 - convert uses of 'defer' to 'new Promise' in
> client/canvasdebugger
> 
> https://reviewboard.mozilla.org/r/227436/#review233254
> 
> I run hg pull && hg update before commiting changes. Also, I did the same
> thing right now and no changes were found.
> I tried hg rebase, but I got
> "abort: branch 'default' has 3 heads - please rebase to an explicit rev"
> 
> I'm more used to git and mercurial is new for me. Is hg rebase what I should
> be running instead? How can I solve the above problem?

So you need to specify which revision to want to rebase on top, which is done with: 

hg rebase -d central -b {your_patch_revision}
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

a month ago
Thanks!

Is the new patch correct?

Comment 25

a month ago
mozreview-review
Comment on attachment 8958532 [details]
Bug 1354883 - convert uses of 'defer' to 'new Promise' in client/canvasdebugger.

https://reviewboard.mozilla.org/r/227458/#review233392

Looks good, thanks Valentin.
I applied the patch and ran tests successfully.
I pushed to TRY (our CI infra) to see if everything is okay, and if it's the case, I'll push your patch :)
Attachment #8958532 - Flags: review?(nchevobbe) → review+
TRY looks good but there's 2 patches in the review. Can you squash them please ?
You should be able to do it with hg histedit , which gives you something similar to git rebase -i
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8958510 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #8958532 - Attachment is obsolete: true
(Assignee)

Comment 29

a month ago
I managed to squash everything in one commit.

Thank you very much for your guidance. Let me know if I need to change something.

Comment 30

a month ago
mozreview-review
Comment on attachment 8958840 [details]
Bug 1354883 - convert uses of 'defer' to 'new Promise' in client/canvasdebugger.

https://reviewboard.mozilla.org/r/227728/#review233934
Attachment #8958840 - Flags: review?(nchevobbe) → review+

Comment 31

a month ago
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/37243daf113e
convert uses of 'defer' to 'new Promise' in client/canvasdebugger. r=nchevobbe
Valentin, I pushed your patch :) Thanks a lot for bearing with me. If nothing is bad, this should hit Nightly in a couple of days.
(Assignee)

Comment 33

a month ago
Nicolas, thank you very much for your help!

Comment 34

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/37243daf113e
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.