1429908 - Update Debugger Frontend v9.0

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Comment 1

[Jason Laster [:jlast]]

early try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1ad6c6919f5850dfbb2078f31169443aa06e5a6

Comment 2

[Jason Laster [:jlast]]

Attachment: 9.patch

Comment 3

[Jason Laster [:jlast]]

inbound try run has a known intermittent that looks pretty infrequent.

central try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2881547c7c2c6303c6c408445984524d9b686e87

Comment 4

[Jason Laster [:jlast]]

Attachment: 9.1.patch

The initial patch had enabled code folding, which is not ready.

sanity try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=31ffddbbc5a14b61f70d7e85e350f25360019bce

Comment 5

[Julian Descottes [:jdescottes]]

As we started code freeze for Release 59, let's check with release management how they feel about new debugger bundle updates.

Ritu: Last release we made the mistake of landing a significant update to the debugger right before the merge (Bug 1415300). 

We are in a much better place now since we do frequent smaller releases. We know we should not land significant changes starting today, and that this kind of patches are hard to gauge, because they mostly touch one big bundle file. But given that releases are smaller than before, and that you can check the list of included commits at https://github.com/devtools-html/debugger.html/compare/release-8...release-9, do you think it can be ok to land this now? Or do you prefer that we stop updating the bundle until merge day?

Comment 6

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Julien, what is the level of risk involved and scope of changes here? What kinds of automated tests were/will be run to ensure this doesn't cause a regression? If the answer to the former is low-risk and the latter is well-tested with high confidence so as not to disrupt Merge day, then I think we'd be fine taking this uplift. Liz owns 59 so I will let her make a final call here.

For future, would it be possible to plan this better such that it lands before the soft code freeze? Also, really appreciate your NI here and engagement with relman. This is great and helps reduce the element of surprise.

Comment 7

[Julian Descottes [:jdescottes]]

Thanks for the feedback!

> what is the level of risk involved and scope of changes here? What kinds of automated tests were/will be run to ensure this doesn't cause a regression? 

The code is covered by automated tests (browser mochitests), and the risk of regression is low. The changes here are all minor and if we were landing them as individual patches in mozilla-central, none of them would be considered risky. However, due to the way we release the debugger, the patch attached to this bug combines several of those small changes and can seem more complex than it is.

> For future, would it be possible to plan this better such that it lands before the soft code freeze?

We are "regularly" releasing the debugger from GitHub to mozilla-central, more or less once a day. We landed v8.0 yesterday, before the code freeze. Today we have release v9.0. Next week we will have more.

Beyond potentially getting a greenlight for this particular patch, the background question is "can we perform small debugger releases during a soft freeze". We know that we should not repeat what happened with release 58, but the releases we do now are much smaller. It's probably better to have a chat about this next week.

Comment 8

[Jason Laster [:jlast]]

> Risk / Reward

This patch is tested and has a couple important fixes. 

> Next week we will have more.

I think we can be flexible here. It is nice for us to do regular releases as it keeps each release small. With that said, I would be okay limiting our releases to just important fixes. 

We have a couple of fixes in the works that will help, but we can also schedule those patches for an uplift after the first beta goes out.

Comment 9

[Julien Cristau [:jcristau]]

A couple of things I'm wondering:
- do I read this right that the updated dependencies are only for testing and don't affect the code?
- how are translations handled?

The changes look reasonably contained, so I think this is probably fine.  Thanks for the consideration here, much appreciated.

Comment 10

[Julian Descottes [:jdescottes]]

(In reply to Jason Laster [:jlast] from comment #8)
> > Risk / Reward
> 
> This patch is tested and has a couple important fixes. 
> 
> > Next week we will have more.
> 
> I think we can be flexible here. It is nice for us to do regular releases as
> it keeps each release small. With that said, I would be okay limiting our
> releases to just important fixes.

I agree with that. Even if our releases are smaller now, during the freeze we should apply the same rules as if we were in m-c, and only release important fixes during the freeze.


(In reply to Julien Cristau [:jcristau] from comment #9)
> A couple of things I'm wondering:
> - do I read this right that the updated dependencies are only for testing
> and don't affect the code?

I think it's correct, both the Enzyme and flow updates are test only.

> - how are translations handled?
> 

Translations are handled in the same way as any devtools localization, they are not "merged" in the bundle. So if there is no update to debugger.properties in a given patch, it means the localized strings haven't changed. 

Here we have a new string "expressions.errorMsg" which would need to be translated by localization teams. And they will only know about this change once it lands in m-c (we don't have any early localization before we synchronize to m-c) 

> The changes look reasonably contained, so I think this is probably fine. 
> Thanks for the consideration here, much appreciated.

Thanks for the feedback, we will proceed with this release. If another release seems necessary during the code freeze we'll be careful to only handpick what is really needed.

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 8942057 [details] [diff] [review]
9.1.patch

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

LGTM, I will amend the README file. Let's wait for Liz to clear the ni? before landing.

::: devtools/client/debugger/new/README.mozilla
@@ +1,4 @@
>  This is the debugger.html project output.
>  See https://github.com/devtools-html/debugger.html
>  
> +Release 9

Let's have some consistency here. -> Version 9.0

Comment 12

[Julian Descottes [:jdescottes]]

Attachment: release-v9.0-RC2.patch

Fixed the README inconsistency.

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Looks ok to me, I agree with Julien. Please go ahead and land.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6132ddd09b01
Update Debugger Frontend v9.0. r=jdescottes

Comment 15

[Julian Descottes [:jdescottes]]

thanks for the feedback, landed on inbound!

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6132ddd09b01