Integrate the VariablesView as a manifest editor

RESOLVED FIXED in Firefox 27

Status

()

Firefox
Developer Tools: WebIDE
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jryans, Assigned: jryans)

Tracking

Trunk
Firefox 27
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Comment hidden (empty)
Created attachment 814725 [details] [diff] [review]
Add VariablesView as manifest editor in App Manager

This is a rough (but functional) version of the manifest editor in the App Manager.  I'll attach a screenshot of the current appearance.

I have not attempted to style the editor at all.  That will be handled in bug 922144.

I left in the "save on update" functionality for now.  If you think it should wait until the buttons are re-done, or possibly a new button should be added, let me know.
Attachment #814725 - Flags: review?(paul)
Try push: https://tbpl.mozilla.org/?tree=Try&rev=6ea9fe4b6bcb
Created attachment 814726 [details]
Basic Appearance

Here's a screenshot of the current (unstyled) appearance.
Created attachment 815131 [details] [diff] [review]
Add VariablesView as manifest editor in App Manager v2

* Improved test stability (was leaking data)
* Added pref to enable editor, on by default for Nightly only

Try push: https://tbpl.mozilla.org/?tree=Try&rev=9cb55febe192
Attachment #814725 - Attachment is obsolete: true
Attachment #814725 - Flags: review?(paul)
Attachment #815131 - Flags: review?(paul)
Created attachment 815764 [details] [diff] [review]
Add VariablesView as manifest editor in App Manager v3

* More test fixing

The new tests were passing locally for me, but then failing intermittently on try.  Should be good now!  Sorry for r? spam.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=054c4eb26dd2
Attachment #815131 - Attachment is obsolete: true
Attachment #815131 - Flags: review?(paul)
Attachment #815764 - Flags: review?(paul)

Comment 6

4 years ago
Comment on attachment 815764 [details] [diff] [review]
Add VariablesView as manifest editor in App Manager v3

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

This looks good. The main issues I see are:

- having to click on update to re-validate the manifest, update the project store (which updates the UI) and save the manifest
- using `eval` to update the manifest

Don't bother addressing all of these. We can iterate as long as this stays preffed off.

--


Is there a way to save the manifest without using the refresh button?
Also, can we get the validation errors without having to click update?

Changing the name property doesn't update the project name in the UI until the manifest is saved. But it works for the description. Maybe we should use manifest.name in the UI.

Do we have to use eval() ? It looks fragile (easy to break). Can't we rebuilt the manifest from the variable view instead of updating it with eval()?

Can we hide _proto_?

::: browser/branding/nightly/pref/firefox-branding.js
@@ +29,5 @@
>  pref("browser.search.param.yahoo-fr-ja", "");
>  pref("browser.search.param.yahoo-f-CN", "");
> +
> +// Enable the manifest editor on Nightly
> +pref("devtools.appmanager.manifestEditor.enabled", true);

We usually don't do that. Just keep it preffed-off.

::: browser/devtools/app-manager/content/manifest-editor.js
@@ +96,5 @@
> +
> +      let encoder = new TextEncoder();
> +      let data = encoder.encode(JSON.stringify(this.manifest, null, 2));
> +
> +      return OS.File.writeAtomic(path, data, { tmpPath: path + ".tmp" });

Is it required to have a tmp file?

::: browser/devtools/app-manager/content/projects.js
@@ +55,1 @@
>      });

We usually use "ready" instead of "initialize"

::: browser/devtools/app-manager/content/projects.xhtml
@@ +33,1 @@
>            <div onclick="UI.addHosted()" id="new-hosted-project-click" title="&projects.addHostedTooltip;"></div>

I guess this is for tests. Any alternative way to do this?

@@ +59,5 @@
>    <div>
>      <div class="project-details" template='{"type":"attribute","path":"validationStatus","name":"status"}'>
>        <div class="project-header">
>          <img class="project-icon" template='{"type":"attribute","path":"icon","name":"src"}'/>
> +        <div class="project-metadata">

I'd prefer if we don't rename classes (extension and themes might not like that).

::: browser/themes/shared/devtools/app-manager/projects.css
@@ +462,5 @@
> +  width: 100%;
> +  height: 100%;
> +  border: 0;
> +  border-top: 5px solid #C9C9C9;
> +}

Can't you use flex-grow here?

@@ +465,5 @@
> +  border-top: 5px solid #C9C9C9;
> +}
> +
> +/* These blocks can removed when the manifest editor is always on */
> +

You a word.

Also, add a bug number here.
Attachment #815764 - Flags: review?(paul)
Created attachment 816128 [details] [diff] [review]
Add VariablesView as manifest editor in App Manager v4

Try push: https://tbpl.mozilla.org/?tree=Try&rev=7c2dee0320d8

(In reply to Paul Rouget [:paul] from comment #6)
> Comment on attachment 815764 [details] [diff] [review]
> Add VariablesView as manifest editor in App Manager v3
> 
> Review of attachment 815764 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the review! :D

> Is there a way to save the manifest without using the refresh button?
> Also, can we get the validation errors without having to click update?

Yes, those things can be done.  Mostly I was just unsure if it made sense to save the file without some kind of explicit user action to that effect.  I've filed bug 925957 about this, so please add some feedback there!  :)

> Changing the name property doesn't update the project name in the UI until
> the manifest is saved. But it works for the description. Maybe we should use
> manifest.name in the UI.

Updated UI to use manifest.name, so it now updates immediately.

> Do we have to use eval() ? It looks fragile (easy to break). Can't we
> rebuilt the manifest from the variable view instead of updating it with
> eval()?

I agree is it unfortunate...  VariablesView is geared towards clients that use eval or something like it.  See webconsole (https://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/webconsole.js#3495) and other clients for examples.

But, we should surely be able to expose a nicer API for local (non-remote) objects so that eval is not needed.  Filed bug 925923.

> Can we hide _proto_?

Should be pretty straightforward to add an option to VariablesView to omit this.  Filed 925928.

> ::: browser/branding/nightly/pref/firefox-branding.js
> @@ +29,5 @@
> >  pref("browser.search.param.yahoo-fr-ja", "");
> >  pref("browser.search.param.yahoo-f-CN", "");
> > +
> > +// Enable the manifest editor on Nightly
> > +pref("devtools.appmanager.manifestEditor.enabled", true);
> 
> We usually don't do that. Just keep it preffed-off.

Removed the default on for Nightly.

> ::: browser/devtools/app-manager/content/manifest-editor.js
> @@ +96,5 @@
> > +
> > +      let encoder = new TextEncoder();
> > +      let data = encoder.encode(JSON.stringify(this.manifest, null, 2));
> > +
> > +      return OS.File.writeAtomic(path, data, { tmpPath: path + ".tmp" });
> 
> Is it required to have a tmp file?

Yes, for some reason the API requires it.  Surely it could be defaulted...

> ::: browser/devtools/app-manager/content/projects.js
> @@ +55,1 @@
> >      });
> 
> We usually use "ready" instead of "initialize"

Changed.

> ::: browser/devtools/app-manager/content/projects.xhtml
> @@ +33,1 @@
> >            <div onclick="UI.addHosted()" id="new-hosted-project-click" title="&projects.addHostedTooltip;"></div>
> 
> I guess this is for tests. Any alternative way to do this?

Yes, it is for tests.  I think this is probably the best approach, and it doesn't seem that bad to allow chrome://.  Mostly you just want to ensure that there is some URL in that field before trying to add, so this modification of the pattern seems fine.

I experimented with having the test add the project directly to store, but then you have to duplicate much of the logic from project.js, like ensuring you validate after adding to store, etc.  This approach removes that duplication, and also tests the add hosted project UI code path.

> @@ +59,5 @@
> >    <div>
> >      <div class="project-details" template='{"type":"attribute","path":"validationStatus","name":"status"}'>
> >        <div class="project-header">
> >          <img class="project-icon" template='{"type":"attribute","path":"icon","name":"src"}'/>
> > +        <div class="project-metadata">
> 
> I'd prefer if we don't rename classes (extension and themes might not like
> that).

That makes sense as a general rule.  In this case though, the "project-details" class was originally used for two elements that are unrelated, and it seemed like it was just a mistake.  As you can see, it is still applied to the element that wraps the one I changed, a few lines up from there.

The styles that were applied previously were only actually needed on the outer one with that class, so that's why I gave the inner one a new class, so it could be styled separately.

> ::: browser/themes/shared/devtools/app-manager/projects.css
> @@ +462,5 @@
> > +  width: 100%;
> > +  height: 100%;
> > +  border: 0;
> > +  border-top: 5px solid #C9C9C9;
> > +}
> 
> Can't you use flex-grow here?

Yep, changed.

> @@ +465,5 @@
> > +  border-top: 5px solid #C9C9C9;
> > +}
> > +
> > +/* These blocks can removed when the manifest editor is always on */
> > +
> 
> You a word.
> 
> Also, add a bug number here.

Reworded and filed bug 925921.
Attachment #815764 - Attachment is obsolete: true
Attachment #816128 - Flags: review?(paul)

Comment 8

4 years ago
Comment on attachment 816128 [details] [diff] [review]
Add VariablesView as manifest editor in App Manager v4

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

::: browser/locales/en-US/chrome/browser/devtools/app-manager.dtd
@@ +77,1 @@
>  

I'm not sure we want this label yet. We'll add that once we get the full design.
Attachment #816128 - Flags: review?(paul) → review+

Comment 9

4 years ago
Splinter…

> <!ENTITY projects.manifestEditor "Manifest Editor">

I'm not sure we want this label yet. We'll add that once we get the full design.
(In reply to Paul Rouget [:paul] from comment #9)
> Splinter…
> 
> > <!ENTITY projects.manifestEditor "Manifest Editor">
> 
> I'm not sure we want this label yet. We'll add that once we get the full
> design.

I think it's fine to leave the label in for now.  If we need to change or remove it, we just have to update the key, so it's not that bad.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/f12bb231ae37
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f12bb231ae37
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
You need to log in before you can comment on or make changes to this bug.