Closed Bug 1405008 Opened 2 years ago Closed 2 years ago

WebIDE should warn when connecting to too old runtimes

Categories

(DevTools :: WebIDE, enhancement, P2)

enhancement

Tracking

(firefox57 wontfix, firefox58 fix-optional, firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox57 --- wontfix
firefox58 --- fix-optional
firefox59 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 3 open bugs)

Details

Attachments

(1 file)

In order to start removing compatibility code (see bug 1405007), we should make WebIDE warn users attempting to connect to very old Firefox runtimes.
We already have a check for warning users connecting to a runtime more recent than firefox, we should just add another one checking for too old ones.
If such patch looks good to you, I'll send a note to the mailing list to ensure noone object about doing that.
I think that is a necessary feature before being able to strip compatibility code (bug 1405007).
Comment on attachment 8914506 [details]
Bug 1405008 - Make WebIDE warn when connecting to old runtimes.

https://reviewboard.mozilla.org/r/185834/#review190778

I think we should do this, thanks!

::: devtools/client/locales/en-US/webide.properties:52
(Diff revision 1)
>  
>  # Variable: runtime app build ID (looks like this %Y%M%D format) and firefox build ID (same format)
>  error_runtimeVersionTooRecent=The connected runtime has a more recent build date (%1$S) than your desktop Firefox (%2$S) does. This is an unsupported setup and may cause DevTools to fail. Please update Firefox.
>  
> +# Variable: runtime app version (looks like this 52.a3) and firefox version (same format)
> +error_runtimeVersionTooOld=The connected runtime has a old version (%1$S). The minimum supported version is (%2$S). This is an unsupported setup and may cause DevTools to fail. Please update the connected runtime.

s/has a old/has an old

::: devtools/shared/client/main.js:24
(Diff revision 1)
>  
>  const noop = () => {};
>  
> +// Define the minimum officially supported version of Firefox when connecting to a remote
> +// runtime. (Use ".0a1" to support the very first nightly version)
> +exports.MIN_SUPPORTED_PLATFORM_VERSION = "52.0a1";

1- mention that this should match the oldest ESR version supported (I guess?)

2- why put it here rather than next to the code that uses it in devtools/client/webide/content/webide.js? Plans to reuse it in other parts of devtools?
Attachment #8914506 - Flags: review?(jdescottes) → review+
(In reply to Julian Descottes [:jdescottes] from comment #3)
> ::: devtools/shared/client/main.js:24
> (Diff revision 1)
> >  
> >  const noop = () => {};
> >  
> > +// Define the minimum officially supported version of Firefox when connecting to a remote
> > +// runtime. (Use ".0a1" to support the very first nightly version)
> > +exports.MIN_SUPPORTED_PLATFORM_VERSION = "52.0a1";
> 
> 1- mention that this should match the oldest ESR version supported (I guess?)

Yes, I used the current ESR version.

> 2- why put it here rather than next to the code that uses it in
> devtools/client/webide/content/webide.js? Plans to reuse it in other parts
> of devtools?

So even if WebIDE uses this and is the one to display the warning, I felt that it was a more low level data.
Something that is tightly related to the Client/Fronts rather than WebIDE/about:debugging.
To me it is the counterpart of actor's "traits".
Having said that, yes, I totally agree, devtools/shared/main.js also feels a bit random.
Nicolas just refactored this in bug 1403895. May be devtools/shared/debugger-client.js would sound better?
Or devtools/client/framework/target?

And may be having a client.isRuntimeOutdated() or target.isRuntimeOutdated() (or something alike), would help following?
(In reply to Alexandre Poirot [:ochameau] from comment #4)
> (In reply to Julian Descottes [:jdescottes] from comment #3)
> > ::: devtools/shared/client/main.js:24
> > (Diff revision 1)
> > >  
> > >  const noop = () => {};
> > >  
> > > +// Define the minimum officially supported version of Firefox when connecting to a remote
> > > +// runtime. (Use ".0a1" to support the very first nightly version)
> > > +exports.MIN_SUPPORTED_PLATFORM_VERSION = "52.0a1";
> > 
> > 1- mention that this should match the oldest ESR version supported (I guess?)
> 
> Yes, I used the current ESR version.
> 
> > 2- why put it here rather than next to the code that uses it in
> > devtools/client/webide/content/webide.js? Plans to reuse it in other parts
> > of devtools?
> 
> So even if WebIDE uses this and is the one to display the warning, I felt
> that it was a more low level data.
> Something that is tightly related to the Client/Fronts rather than
> WebIDE/about:debugging.
> To me it is the counterpart of actor's "traits".
> Having said that, yes, I totally agree, devtools/shared/main.js also feels a
> bit random.
> Nicolas just refactored this in bug 1403895. May be
> devtools/shared/debugger-client.js would sound better?
> Or devtools/client/framework/target?
> 
> And may be having a client.isRuntimeOutdated() or target.isRuntimeOutdated()
> (or something alike), would help following?

+1 , having this as a public API on client will make it easier to understand and maintain. The mapping between runtime and client feels more logical than runtime and target.
Assignee: nobody → poirot.alex
Priority: -- → P2
Comment on attachment 8914506 [details]
Bug 1405008 - Make WebIDE warn when connecting to old runtimes.

I moved things to DebuggerClient.isRuntimeOutdated.
Now, I'm wondering if I should also move the "runtime is too recent" to client and instead have a "isRuntimeSupported" function which would return the translated error string.

Note that the getDeviceFront/listTabsResponse would be simplified by bug 1222047.
I want root actor response to be cached on the client and no longer have to manually pass it around like that.
Attachment #8914506 - Flags: review+ → review?(jdescottes)
Attachment #8914506 - Flags: review?(jdescottes)
Summary: WebIDE should warn when connection to too old runtimes → WebIDE should warn when connecting to too old runtimes
Comment on attachment 8914506 [details]
Bug 1405008 - Make WebIDE warn when connecting to old runtimes.

https://reviewboard.mozilla.org/r/185834/#review191338

Thanks for the update. Some comments and questions below.

::: devtools/shared/client/debugger-client.js:197
(Diff revision 3)
>    },
>  
>    /**
> +   * Tells if the remote device is using an outdated, unsupported version of Firefox.
> +   *
> +   * @return Object|null

I would stick to return true/false for this method.

The caller can the read MIN_SUPPORT_PLATFORM_VERSION if they want to build an error message. Displaying the version that was tested is nice but not mandatory here, and makes the method harder to understand.

::: devtools/shared/client/debugger-client.js:202
(Diff revision 3)
> +   * @return Object|null
> +   *         If outdated, returns an object with `version` attribute to say which
> +   *         version is outdated. And `expectedVersion` stating the minimum supported
> +   *         version.
> +   */
> +  async isRuntimeOutdated(listTabsResponse) {

Let's try to see if we can avoid requesting this weird listTabsResponse.

getDeviceFront takes a "form" argument only to read a deviceActor string property. While isRuntimeOutdated(deviceActorId) would be understandable, isRuntimeOutdated(listTabsResponse) is confusing to me. We don't really need a listTabsResponse, just something that has the deviceActor property. client.getRoot() would be enough here, right?

Not sure what's the best way to go here. 

We could modify the signature, all the way down to getDeviceFront to only use a string parameter (or even use destructuring -> { deviceActor } if it makes it easier).

But I thought that moving the method to client meant we could retrieve the version without asking extra context information from the caller. "I have a debugger client, tell me if its version is ok".

I understand that we don't want to have a call to listTabs just for that since it's expensive, but we can do client.getRoot(), which should be less expensive.

If that's not ok for some reason, I would still prefer to have a simple API, where we pass either the device description (if so -> makes no sense in client anymore, move it back to a more generic location) or the device actor id.
Attachment #8914506 - Flags: review?(jdescottes)
(In reply to Alexandre Poirot [:ochameau] from comment #7)
> Comment on attachment 8914506 [details]
> Bug 1405008 - Make WebIDE warn when connecting to old runtimes.
> 
> I moved things to DebuggerClient.isRuntimeOutdated.
> Now, I'm wondering if I should also move the "runtime is too recent" to
> client and instead have a "isRuntimeSupported" function which would return
> the translated error string.

If we go and complexify the method in this way then let's rename isRuntimeSupported/Outdated to "checkRuntimeVersion". Trying to keep isSomething methods to return booleans. I think the method should still only return metadata that explains if the version is compatible, and if not why (isTooOld, isTooRecent, max/minVersion etc...) so that the caller can still control which error message to create and display.

> 
> Note that the getDeviceFront/listTabsResponse would be simplified by bug
> 1222047.
> I want root actor response to be cached on the client and no longer have to
> manually pass it around like that.

Ah I missed this comment before diving into the review. So I would say this is a must. If caching is the only way to go in order to quickly get access to the deviceActorId, if getRoot() is too expensive, then let's pass deviceActor in the meantime with a comment referencing the other bug + log a follow up in order to cleanup this when Bug 1222047 lands.
Comment on attachment 8914506 [details]
Bug 1405008 - Make WebIDE warn when connecting to old runtimes.

https://reviewboard.mozilla.org/r/185834/#review193094

::: devtools/client/webide/content/webide.js:25
(Diff revision 4)
>  const utils = require("devtools/client/webide/modules/utils");
>  const Telemetry = require("devtools/client/shared/telemetry");
>  const {RuntimeScanners} = require("devtools/client/webide/modules/runtimes");
>  const {showDoorhanger} = require("devtools/client/shared/doorhanger");
>  const {Task} = require("devtools/shared/task");
> +const {MIN_SUPPORTED_PLATFORM_VERSION} = require("devtools/shared/client/debugger-client");

Imported but not used.

::: devtools/shared/client/debugger-client.js:38
(Diff revision 4)
>  const noop = () => {};
>  
> +// Define the minimum officially supported version of Firefox when connecting to a remote
> +// runtime. (Use ".0a1" to support the very first nightly version)
> +// This is usually the current ESR version.
> +const MIN_SUPPORTED_PLATFORM_VERSION = exports.MIN_SUPPORTED_PLATFORM_VERSION = "52.0a1";

depending how you address my other comment, only export if used somewhere else.

::: devtools/shared/client/debugger-client.js:215
(Diff revision 4)
> +   *            Build ID of remote runtime. A date with like this: YYYYMMDD.
> +   */
> +  async checkRuntimeVersion() {
> +    let incompatible = null;
> +
> +    let rootForm = await this.mainRoot.getRoot();

Ergh, I feel sorry for suggesting that :( This API only appeared in FF55 (https://bugzilla.mozilla.org/show_bug.cgi?id=1352157)

Since we are supposed to support versions down to 52ESR, I guess that means we cannot use that right now. The only alternative 54 and older is probably to use listTabs. 

So let's pass the listTabsForm with a good comment to explain that we have to do this until getRoot is available in all the versions of Firefox we support.
Attachment #8914506 - Flags: review?(jdescottes)
(In reply to Julian Descottes [:jdescottes] from comment #12)
> Comment on attachment 8914506 [details]
> 
> Ergh, I feel sorry for suggesting that :( This API only appeared in FF55
> (https://bugzilla.mozilla.org/show_bug.cgi?id=1352157)
> 
> Since we are supposed to support versions down to 52ESR, I guess that means
> we cannot use that right now. The only alternative 54 and older is probably
> to use listTabs. 
> 
> So let's pass the listTabsForm with a good comment to explain that we have
> to do this until getRoot is available in all the versions of Firefox we
> support.

Note that since most runtimes should support getRoot, we could also simply use listTabs as a fallback if getRoot is not available.
Blocks: 1405288
Tested against ESR52. Works fine.
Comment on attachment 8914506 [details]
Bug 1405008 - Make WebIDE warn when connecting to old runtimes.

https://reviewboard.mozilla.org/r/185834/#review205920

Works for me.
Attachment #8914506 - Flags: review?(jdescottes) → review+
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ae7380552794
Make WebIDE warn when connecting to old runtimes. r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/ae7380552794
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
Blocks: 1487337
Blocks: 1492187
Blocks: 1492460
Blocks: 1492464
You need to log in before you can comment on or make changes to this bug.