Closed Bug 913711 Opened 11 years ago Closed 10 years ago

Pull manifest from regular sites

Categories

(DevTools Graveyard :: WebIDE, enhancement, P2)

Product:
DevTools Graveyard
Component:
WebIDE
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 35

People

(Reporter: jryans, Assigned: tehsis, Mentored)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 11 obsolete files)

913711.patch
12.19 KB, patch
tehsis
: review+
Details | Diff | Splinter Review 
The Simulator offers the (hidden?) ability to pull a manifest from a regular URL, such as "http://bugmotodo.org" instead of a direct link to the manifest.

This is missing from the App Manager, but it would be nice to bring it back.
Nice to have, but doesn't look like something important enough to prioritize for this week.
I agree, does not seem critical.
No longer blocks: appmgr_v1
Priority: -- → P2
Whiteboard: [good-first-bug][lang=js][mentor=paul]
Hi Paul

I did some code research:

When Simulator add a hosted app,
it validates first
https://github.com/mozilla/r2d2b2g/blob/devkit/addon/lib/simulator.js#L650

if failed, append '/manifest.app' to original URL
https://github.com/mozilla/r2d2b2g/blob/devkit/addon/lib/simulator.js#L662


But what App Manager dose seems in a reverse order
it calls addHosted first, then validate manifestURL via Promise callback
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/app-manager/content/projects.js#102


So I failed to implement similar logic on App Manager, just add a run-time string comparison check right after manifestURL is grabbed from user input textbox.
-----------------------------------------------------------------
     let urlInput = document.querySelector("#url-input");
     let manifestURL = urlInput.value;
+    if (!manifestURL.endsWith("manifest.webapp"))
+      manifestURL += "/manifest.webapp";
+
     return AppProjects.addHosted(manifestURL)
                       .then(function (project) {
                         UI.validate(project);
                         UI.selectProject(project.location);
                       }); 
-----------------------------------------------------------------

Is this appropriate?

        Attachment #820871 -
        Flags: review?(paul)

    
    

    
  
Comment on attachment 820871 [details] [diff] [review]
Bug-913711-Pull-manifest-from-regular-sites.patch

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

::: browser/devtools/app-manager/content/projects.js
@@ +99,5 @@
>  
>      let urlInput = document.querySelector("#url-input");
>      let manifestURL = urlInput.value;
> +    if (!manifestURL.endsWith("manifest.webapp"))
> +      manifestURL += "/manifest.webapp";

There is no strict rule about the manifest file name for hosted app. So that it will break any usecase using a different name for the manifest.

Ideally, we should fallback to append `manifest.webapp` only when the given url isn't a manifest.

May be we should start validating before adding the project, with something like:
  let manifestURL = urlInput.value;
  checkManifest(manifestURL)
    .then(null, () => {
      manifestURL += "/manifest.webapp";
      return checkManifest(manifestURL);
    })
    .then(() => {
      AppProjects.addHosted(manifestURL)
        .then(function (project) {
          UI.validate(project);
          UI.selectProject(project.location);
        });
    }, () => {
      // We would need to send the localized string
      // from appvalidator
      alert("invalid manifest url");
    });

Here check manifest is a striped down version of AppValidator._fetchManifest:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/app-manager/app-validator.js#53
function checkManifest(url) {
  let deferred = promise.defer();
  let req = new XMLHttpRequest();
  try {
    req.open("GET", url, true);
  } catch(e) {
    deferred.reject();
  }
  req.channel.loadFlags |= Ci.nsIRequest.LOAD_BYPASS_CACHE | Ci.nsIRequest.INHIBIT_CACHING;
  req.onload = (function () {
    let manifest = null;
    try {
      JSON.parse(req.responseText);
      deferred.resolve();
    } catch(e) {
      deferred.reject();
    } 
  }
}

May be we can factorize with AppValidator somehow. For example we could expose _fetchManifest without having to instanciate a AppValidator instance and make it reject the error message via the promise:
AppValidator.fetchManifest = function (url) {
  // instead of this.error(...), do deferred.reject(...)
}

        Attachment #820871 -
        Flags: review?(paul)

    
  
Whiteboard: [good-first-bug][lang=js][mentor=paul] → [good first bug][lang=js][mentor=paul]

    
    

    
  
Er, I don't think I am capable of doing this,
I'll give up.

    
    

    
  
Hello, I would like to be assigned this bug please.

    
    

    
  


  
I have attached my patch for this, based on feedback in Comment 5. Let me know if these changes are on the right track, thanks!

        Attachment #8373789 -
        Flags: review?(poirot.alex)

    
  

        Attachment #8373789 -
        Flags: review?(poirot.alex) → review?(paul)

    
    

    
  
Comment on attachment 8373789 [details] [diff] [review]
rev1 - Pull manifest from regular site

Paul will be out on PTO for 3 weeks, so I'll review this soon.

        Attachment #8373789 -
        Flags: review?(paul) → review?(jryans)
Whiteboard: [good first bug][lang=js][mentor=paul] → [good first bug][lang=js][mentor=jryans]

        Attachment #820871 -
        Attachment is obsolete: true
Assignee: nobody → agentmirv
Status: NEW → ASSIGNED

    
    

    
  
Comment on attachment 8373789 [details] [diff] [review]
rev1 - Pull manifest from regular site

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

We're getting closer here, good work.  I have a number of things we should address before landing this.

Please test your patch manually to be sure it functions well next time...  For me, none of my app info appears any more with this (title, description, etc.).

In your patch commit message, use a summary that says what the patch does instead of just repeating the bug title.  Also, include "r=jryans" at the end.

Watch out for trailing whitespace.  There are number of places with that in your patch, so check for that next time.

You should also add an automated test, so we can be sure this keeps working.  Take a look at others in the browser/devtools/app-manager/test directory for some ideas there.

::: browser/devtools/app-manager/app-validator.js
@@ +54,5 @@
>  AppValidator.prototype._fetchManifest = function (manifestURL) {
>    let deferred = promise.defer();
>    this.manifestURL = manifestURL;
>  
> +  return this.checkManifest(manifestURL).then(

Your deferred is being ignored because of the |return| here, so remove that.

@@ +67,5 @@
> +  
> +  return deferred.promise;
> +};
> +
> +AppValidator.prototype.checkManifest = function (manifestURL) {

Let's move this off the prototype, so we don't have to create an AppValidator instance to use it.  So, just |AppValidator.checkManifest = ...|.

We'll need to remove any references to |this| for it to work, but it seems possible to make this change.

@@ +93,5 @@
>    }).bind(this);
>  
>    try {
>      req.send(null);
> +    let temp = "";

Seems like this should be removed.

::: browser/devtools/app-manager/content/projects.js
@@ +115,5 @@
>        return;
>  
>      let urlInput = document.querySelector("#url-input");
>      let manifestURL = urlInput.value;
> +    let validation = new AppValidator(null);

This will go away once you move |checkManifest| off the prototype.

@@ +134,5 @@
> +    AppProjects.addHosted(manifestURL).then(
> +      function (project) {
> +        UI.validate(project).then(
> +          function(manifest) {
> +            UI.selectProject(project.location);

It seems better to select the project right away (rather than waiting on validation) because the UI will update faster, and also I believe this would skip selecting the project if there is a validation error, so errors might be hidden.

So, let's make this block look like it did before:

> UI.validate(project);
> UI.selectProject(project.location);

@@ +164,5 @@
>    validate: function(project) {
>      let validation = new AppValidator(project);
>      return validation.validate()
>        .then(function () {
> +        let deferred = promise.defer();

The changes here become unnecessary, since we no longer need the promise above.

        Attachment #8373789 -
        Flags: review?(jryans)

    
    

    
  
Marvin, are you actively working on this?  If not, I'll unassign the bug in case someone else is interested.
Flags: needinfo?(agentmirv)

    
    

    
  
No response, unassigning.
Assignee: agentmirv → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(agentmirv)
Mentor: jryans
Whiteboard: [good first bug][lang=js][mentor=jryans] → [good first bug][lang=js]

    
    

    
  
I'll work on this.

Is it Ok that for the project to be responsible of "fix" the manifest url?

I think that would be better if _fetchManifest tries to get the file and "fix" it on error.

This way, the project will just request the manifest using an url.

Is it ok or is a SoC problem?

I'll be working with this approach meanwhile.

thanks

    
    

    
  
(In reply to Pablo Terradillos from comment #13)
> I'll work on this.
> 
> Is it Ok that for the project to be responsible of "fix" the manifest url?
> 
> I think that would be better if _fetchManifest tries to get the file and
> "fix" it on error.

This approach may work, but I'd have to see the patch to be sure.

> This way, the project will just request the manifest using an url.
> 
> Is it ok or is a SoC problem?

I am not sure what SoC means in this context.

Also, to be clear, we are no longer interested in fixing this for the App Manager.  Instead, we would now be adding this feature to WebIDE[1] (which replaces the App Manager).

[1]: https://developer.mozilla.org/en-US/docs/Tools/WebIDE

    
    

    
  


  
Ok, I think I need some help here since I'm running on some issues and cannot figure out why.

I've modified the AppValidator.checkManifest function outside of the prototype, so I need to handle the errors differently (no access to this.error). 
I just storing the error msg on 'error' and passing it by argument when resolving the promise.
The idea is: if there is an error, store it (with this.error), try to fix the manifest and try again.

Expected: 

1 - If no error occurred, the promise will resolve without problem.
2 - If there is an error, it will try again
  2a. If no error occurred, the promise will resolve without problem.
  2b. If there is an error, it will resolve the problem with the failed manifest (or undefined)

Actual results:

1 - The manifest is fetched and is parsed without problem (I get the "VALID" message) but the project is not loaded.

2 - It tries to fetch the "fixed manifest" (I can see it at my webserver logs) but I always get an error of JSON parsing.

I guess I'm messing up with the promises, but can't actually figure out where or how.

Thanks for your help :)

    
    

    
  
Since you've attached a first patch for this, I've assigned the bug to you.
Assignee: nobody → tehsis
Status: NEW → ASSIGNED

    
    

    
  
Comment on attachment 8486403 [details] [diff] [review]
rev2 - Pull manifest from regular site

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

For future patches, please set "feedback?" to my email if you need some help or have questions.  Once you believe you are totally done, you'd later set "review?" to my email.  By using these flags, there is less chance your comments will get lost as Bugzilla sends reminders to me.

When you attach your next patch, mark the old one as "obsolete" so it will hide the old attachment.

Also, you should try using the Browser Toolbox[1].  This will let you debug the validator, like you would with a website.

[1]: https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox

::: browser/devtools/app-manager/app-validator.js
@@ +74,5 @@
> +    } catch(e) {
> +      error = strings.formatStringFromName("validator.invalidManifestJSON", [e, manifestURL], 2);
> +    }
> +
> +    deferred.resolve(error, manifest);

resolve() only accepts one argument.  To pass multiple things, you need to wrap them in an object or array.

However, I think for this function, you should use |deferred.reject(error)| when there is an error.  Then for the "good" path, you'd only need to |deferred.resolve(manifest)| since there are no errors.

@@ +98,5 @@
>    this.manifestURL = manifestURL;
>  
> +  AppValidator.checkManifest(manifestURL)
> +              .then(function(err, manifest) {
> +                 if (err) {

Then, instead of testing |err| here, you'd supply a separate rejection handler.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch proposal (obsolete)
        — 
          — Splinter Review
      

    


  
Playing with the debugger did the trick :)

Since deferred.resolve could only accept one argument, the manifest was lost.

This seems to work fine.

I'll try to work on the tests now. Please tell me what do you think.

Thanks again!

        Attachment #8486403 -
        Attachment is obsolete: true

        Attachment #8486852 -
        Flags: review?(jryans)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        913711.patch (obsolete)
        — 
          — Splinter Review
      

    


  
I've modified the tests so we can specify the manifest file as a second argument of |createHosted|. Then I test that no errors or warnings should be stored.

I also have modified my original patch to just store the error that was generated by the original url. Since I think it makes more sense.

Thank you :)

        Attachment #8486852 -
        Attachment is obsolete: true

        Attachment #8486852 -
        Flags: review?(jryans)

        Attachment #8486932 -
        Flags: review?(jryans)

    
    

    
  
Comment on attachment 8486932 [details] [diff] [review]
913711.patch

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

This is coming together well.

If we do find a manifest at an alternate URL, we should also update the project passed into the validator, so we use the alternate URL the first time when we validate again in the future.

You'll have to think about a good way to do this.  The code that calls the validator is here[1].

[1]: http://hg.mozilla.org/mozilla-central/annotate/843332cc69af/browser/devtools/webide/modules/app-manager.js#l551

::: browser/devtools/app-manager/app-validator.js
@@ +99,5 @@
>    this.manifestURL = manifestURL;
>  
> +  AppValidator.checkManifest(manifestURL)
> +              .then(function(manifest) {
> +                deferred.resolve(manifest);

Nit: Indent one more space

@@ +101,5 @@
> +  AppValidator.checkManifest(manifestURL)
> +              .then(function(manifest) {
> +                deferred.resolve(manifest);
> +               }.bind(this), function(error) {
> +                 manifestURL += '/manifest.webapp';

We should check if the original URL ends with manifest.webapp first, and stop there if it does, since hopefully that means the original URL was set correctly.

Then, you could try adding manifest.webapp and see what happens.

As a third choice, you could try the URL's origin plus manifest.webapp.  Then you can add a site's app via any URL, assuming the manifest is at the root of the site.

::: browser/devtools/app-manager/test/test_app_validator.html
@@ +39,5 @@
>  
>        next();
>      }
>  
> +    function createHosted(path, manifestFile) {

Use a default parameter:

manifestFile = "/manifest.webapp"

so you don't need to change the existing callers.

        Attachment #8486932 -
        Flags: review?(jryans)

    
    

    
  


  
I have created two new methods for trying to find the manifest by appending 'manifest.webapp' or for trying to get it from the origins. 
I've also created the corresponding test.

imho, I'm not sure about trying to find the manifest at the origin. What is the use case for this? I think that can lead the user to confusion if he try to use "http://my.domain.com/folder/subfolder/manifest.webapp" but the manifest is at the root so he gets a valid project.

If the project.location was changed during the validation, it will update the project at IDB as well.
I've added a test for this too.

Let me know what do you think.

        Attachment #8487683 -
        Flags: review?(jryans)

    
    

    
      
  

    
  

        Attachment #8487683 -
        Attachment description: Added methods for fiding manifest → Added methods for finding manifest

    
    

    
  


  
I've realized that I was repeating the origin's verification when trying to find the manifest path. Fixed that. I'm sorry for this.

        Attachment #8487683 -
        Attachment is obsolete: true

        Attachment #8487683 -
        Flags: review?(jryans)

        Attachment #8487821 -
        Flags: review?(jryans)

    
    

    
  
Comment on attachment 8487821 [details] [diff] [review]
Added methods for finding manifest

I've also realized that the added test on test-imports still passes without updating the project so I'll need to find a way to properly test this. Anyway, I need feedback and probably some clue about the test.

I apologize for the mess, I'll try to be more careful the next time :)

        Attachment #8487821 -
        Flags: review?(jryans) → feedback?(jryans)

    
    

    
  
(In reply to Pablo Terradillos from comment #21)
> Created attachment 8487683 [details] [diff] [review]
> Added methods for finding manifest
> 
> imho, I'm not sure about trying to find the manifest at the origin. What is
> the use case for this? I think that can lead the user to confusion if he try
> to use "http://my.domain.com/folder/subfolder/manifest.webapp" but the
> manifest is at the root so he gets a valid project.

Well, we are just trying to be helpful by searching in alternate locations.

I guess the possibly confusing case is if there are multiple, different manifests at:

1. http://my.domain.com/folder/subfolder/manifest.webapp
2. http://my.domain.com/manifest.webapp

but as long as we check the more specific case (#1) before the origin case (#2), hopefully it won't be too confusing.  If we do end up picking the wrong one, it can always be resolved by the user specifying the exact URL to the manifest in the first place.

    
    

    
  
That's what's happening now. Should I add a test for that specific scenario?

    
    

    
  
(In reply to Pablo Terradillos from comment #26)
> That's what's happening now. Should I add a test for that specific scenario?

Yep, I was just trying to clarify why it seems like a good idea to.

Sure, an extra test would be nice.  But, I am about to post more comments on the current patch, so please wait to see those first.

    
    

    
  
Comment on attachment 8487821 [details] [diff] [review]
Added methods for finding manifest

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

So, lots of style feedback and other comments.  Hopefully it's not overwhelming!

The main thing to remember is that in the browser, we can take advantage of all implemented ES6 features, unlike with general web dev, where it's much harder to do so.

Mainly, I want to make sure the logic is easy to follow here for future readers of this code.  Promises can make that tricky at times, so it's best to uses languages features and other things to help when possible.

::: browser/devtools/app-manager/app-validator.js
@@ +75,5 @@
> +      error = strings.formatStringFromName("validator.invalidManifestJSON", [e, manifestURL], 2);
> +      deferred.reject(error);
> +    }
> +
> +    deferred.resolve(manifest);

Your other functions will likely become simpler if you change this to always resolve an object with the manifest and current URL, as I suggest later on below.

@@ +80,5 @@
> +  };
> +
> +  req.onerror = function () {
> +    error = strings.formatStringFromName("validator.noAccessManifestURL", [req.statusText, manifestURL], 2);
> +   deferred.reject(error);

Nit: Indent one more space.

@@ +98,5 @@
> +  let deferred = promise.defer();
> +
> +  let fixedManifest = Services.io.newURI(manifestURL, null, null).prePath + '/manifest.webapp';
> +  AppValidator.checkManifest(fixedManifest)
> +              .then(function(manifest) {

In this case, there's no need to use a extra deferred in this function.  |checkManifest| already returns a promise that is resolved / rejected exactly as you want, so you can just do:

return AppValidator.checkManifest(fixedManifest)

For future reference (since you don't actually need the callbacks now), callbacks like this are a nice place to use ES6 arrow functions:

.then(manifest => {
  ...
})

You can likely change other places in this patch where you do still need callbacks to use an arrow function instead.

@@ +100,5 @@
> +  let fixedManifest = Services.io.newURI(manifestURL, null, null).prePath + '/manifest.webapp';
> +  AppValidator.checkManifest(fixedManifest)
> +              .then(function(manifest) {
> +                deferred.resolve(manifest);
> +              }.bind(this),

Arrow functions bind this to the outer context automatically, so |bind(this)| is not needed.  However you also aren't using |this| here anyway... ;)

You have many |bind(this)| calls in this patch.  Take a look at each to think about if it's needed, as many seem like they are not, or an arrow function could be used anyway.

@@ +116,5 @@
> +    deferred.reject();
> +  } else {
> +    let fixedManifest = manifestURL + '/manifest.webapp';
> +
> +    AppValidator.checkManifest(fixedManifest)

I believe this block can change to:

deferred.resolve(AppValidator.checkManifest(fixedManifest));

@@ +128,5 @@
> +
> +  return deferred.promise;
> +};
> +
> +AppValidator.findHostedManifest = function(manifestURL) {

Let's rename this to |checkAlternateManifest|

@@ +131,5 @@
> +
> +AppValidator.findHostedManifest = function(manifestURL) {
> +  let deferred = promise.defer();
> +
> +  AppValidator.findManifestPath(manifestURL)

I would encourage you to look at the |Task.spawn| function[1] that used in the many places[2] in our code.

It allows you to use promises, while avoiding all the nested callbacks.  I won't require you to do it, but you may find it makes things easier to read.

With Task, this whole function becomes something like:

return Task.spawn(function*() {
  let result;
  try {
    // yield will await the result of the promise
    result = yield AppValidator.findManifestPath(manifestURL);
  } catch() {
    // we come in here if the promise above is rejected
    result = yield AppValidator.findManifestAtOrigin(manifestURL)
  }
  return result;
});

[1]: http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Task.jsm
[2]: http://dxr.mozilla.org/mozilla-central/search?q=Task.spawn&case=true

@@ +135,5 @@
> +  AppValidator.findManifestPath(manifestURL)
> +              .then(function(manifest) {
> +                deferred.resolve(manifest);
> +              }.bind(this),
> +              function() {

Nit: move up, use arrow

@@ +140,5 @@
> +                AppValidator.findManifestAtOrigin(manifestURL)
> +                            .then(function(manifest) {
> +                              deferred.resolve(manifest);
> +                            }.bind(this),
> +                            function() {

Nit: move up, use arrow

@@ +159,5 @@
> +                 deferred.resolve(manifest);
> +               }.bind(this), function(error) {
> +                 AppValidator.findHostedManifest(manifestURL)
> +                             .then(function(manifest) {
> +                               this.project.location = manifestURL;

The validator should avoid mutating the project like this.  Instead, since you're save the new manifest URL on the validator, we have the WebIDE code that calls the validator check if the project's location is different from the validator's manifest URL.

@@ +160,5 @@
> +               }.bind(this), function(error) {
> +                 AppValidator.findHostedManifest(manifestURL)
> +                             .then(function(manifest) {
> +                               this.project.location = manifestURL;
> +                               this.manifestURL = manifestURL;

These lines don't seem right to me...  Don't you need the *new* manifest URL?  |findHostedManifest| (and the things it calls) would need to resolve with both the manifest and the new URL.

Since you can only resolve with one thing, I'd say use an object, and then you can destructure the object in the callbacks args:

At resolve time (using new literal shorthand[1]):

deferred.resolve({manifest, manifestURL})

At callback time (using object destructuring[2]):

({manifest, manifestURL}) => {
  ...
}

[1]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Lexical_grammar#Object_literals
[2]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment#Object_destructuring

@@ +163,5 @@
> +                               this.project.location = manifestURL;
> +                               this.manifestURL = manifestURL;
> +                               deferred.resolve(manifest);
> +                             }.bind(this),
> +                             function() {

Nit: Move this up to the line before, use arrow.

@@ +165,5 @@
> +                               deferred.resolve(manifest);
> +                             }.bind(this),
> +                             function() {
> +                               this.error(error);
> +                               deferred.reject(null);

The promise returned from this function is resolved even when there are errors.  The errors are logged (like you do on the line above), but we still resolve the promise to let the remaining code continue.

::: browser/devtools/webide/test/test_import.html
@@ +50,5 @@
> +            let hostedAppManifest = TEST_BASE + "/app";
> +            yield win.Cmds.importHostedApp(hostedAppManifest);
> +
> +            project = win.AppManager.selectedProject;
> +            is(project.location, hostedAppManifest, "Location is valid");

Here you're testing that the project location is equal to the path you made up above, but you need to test that it's the end result after validating I believe (with manifest.webapp).

You'll likely to make this test yield on promise from the validator before testing the location.

        Attachment #8487821 -
        Flags: feedback?(jryans)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        913711.patch (obsolete)
        — 
          — Splinter Review
      

    


  
I've made all the changes you recommended. Thanks for all the references!

For the project updating, after the validation I set:            

project.location = validation.manifestURL;

This is working fine on my manual tests (without the fix it always tries to fetch /folder before /folder/manifest.webapp) but I cant figure out how to make this test.

What I think we should test is that when the user provided a regular URL (http://my.domain.com/folder) as the path, the project.location gets updated with the correct url (http://my.domain.com/folder/manifest.webapp).

I was thinking on compare the location I originally provide with the one that gets stored on IDB, that is what I was trying to accomplish.
You have said that I should yield on the validator response, but I think have no access to the validator at test_import.html.
Do you think this is te correct place to put this test?

Thank you.

        Attachment #8487821 -
        Attachment is obsolete: true

        Attachment #8488975 -
        Flags: feedback?(jryans)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        913711.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Ok, so the problem was not on the test but on I was commiting a mistake.

Basically, I was updating |project.location| only when |iconPath| was defined.
I have added a test to test_import.html that yield on the validator and tests if the project.location ends with manifest.webapp what indicates that the project was updated correctly.

I have also noticed that in my previous patch I was storing the error generated by the alternate manifest instead of the original one.

        Attachment #8488975 -
        Attachment is obsolete: true

        Attachment #8488975 -
        Flags: feedback?(jryans)

        Attachment #8489785 -
        Flags: review?(jryans)

    
    

    
  
Actually, |importHostedApp| already calls the validator, so I think there's no need of call it again.

    
    

    
  
Comment on attachment 8489785 [details] [diff] [review]
913711.patch

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

Great, thanks for absorbing all the various style and language suggestions!  I think it looks much more readable now, and I hope you agree. :)

I tested it out and noticed an issue with how we store the updated location.  Once you've resolved it, we should be ready to land this.

::: browser/devtools/app-manager/app-validator.js
@@ +133,5 @@
> +   AppValidator.checkManifest(manifestURL)
> +               .then(({manifest, manifestURL}) => {
> +                 deferred.resolve(manifest);
> +               }, error => {
> +                   AppValidator.checkAlternateManifest(manifestURL)

Nit: This block should be moved back 2 spaces.

::: browser/devtools/webide/modules/app-manager.js
@@ +606,5 @@
>          project.errorsCount = 0;
>        }
>  
> +      if (project.type === "hosted") {
> +        project.location = validation.manifestURL;

This part is not quite right because we store the projects in IndexedDB, and the location is used as the primary key.

A few lines later, we try to find the project by the (new) location and then update it.  But if we got an alternate URL from validation, we'll end up not finding it, so no update is done to IDB, but we do change the |project| object.

If you test locally, you may see this prevents some things from working now, like removing a project (if it needed an alternate URL).  This is because we try to remove it based the current location in the |project| which will be the modified one, but there is no such object in IDB.  It will appear to remove, but then it comes back next time you restart Firefox.

So, I think you need to skip this change here, and...

@@ +613,5 @@
>        if (project.warningsCount && project.errorsCount) {
>          project.validationStatus = "error warning";
>        }
>  
>        if (AppProjects.get(project.location)) {

over here, change this to: 

if (project.location !== validation.manifestURL) {
  // Location is the IDB primary key, so must remove / add
  yield AppProjects.updateKey(project, validation.manifestURL);
} else if // existing code

You'd need to add a new method to AppProjects to support this.  It would remove the object from IDB, set the location to the new value, and re-add the object to IDB (and the projects array that AppProjects maintains).

Let me know if this is unclear at all.

::: browser/devtools/webide/test/test_import.html
@@ +46,5 @@
>              is(project.name, "hosted manifest name property", "name field has been updated");
>  
> +            yield nextTick();
> +
> +            let hostedAppManifest = TEST_BASE + "/app";

Don't use |let| again since the variable is already defined above.

When I try this test locally, it causes the test to fail.  I believe this is related to some |let| changes that landed very recently, so you may not have them in your local branch yet.

        Attachment #8489785 -
        Flags: review?(jryans)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        913711.patch (obsolete)
        — 
          — Splinter Review
      

    


  
I've just made the changes you addressed with small modifications:

1 - The project.location is not equal to the manifest's location on packaged apps. So I checked that besides of the equality from location and manifest.

2 - I've named the method |updateLocation| instead of |updateKey|. I think is more accurate for what is actually happening   (otherwise it may seems that it only changes the key). 
Let me know if you still want to proceed with the name you suggested.

3 - I can't reproduce the error you mentioned on app_import.html but seems that is the expected behavior now. I've just removed the redeclaration (i.e. the let keyword).
I'll try to update my branch and retry this (I'm not so confident with mercurial so I have to make sure how to do this without affecting my changes)

Thanks.

        Attachment #8489785 -
        Attachment is obsolete: true

        Attachment #8490473 -
        Flags: review?(jryans)

    
    

    
  
I have confirmed number 3, It now works with the latest changes :)

    
    

    
  
Comment on attachment 8490473 [details] [diff] [review]
913711.patch

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

Great, this seems to work quite well! :)

Fix up the one nit, and then I'll push this to our test infrastructure.

I've given r+ since there's just the one nit.  You should "carry over" the r+ to your updated patch by setting it yourself.

Then, I'll push to Try for tests.

::: browser/devtools/webide/test/test_import.html
@@ +50,5 @@
> +            hostedAppManifest = TEST_BASE + "/app";
> +            yield win.Cmds.importHostedApp(hostedAppManifest);
> +
> +            project = win.AppManager.selectedProject;
> +            //yield win.AppManager.validateProject(project);

Nit: Remove this if it's not needed

        Attachment #8490473 -
        Flags: review?(jryans) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        913711.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Done!

I hope this can land on FF without problem :)

Thanks for the feedbacks, the review and the mentorship! I'll try to work on others devtools related bugs on the following days.

Please, let me know If there's anything else I have to do in order to see this shipped.

        Attachment #8490473 -
        Attachment is obsolete: true

        Attachment #8491203 -
        Flags: review+

    
    

    
  
It looks like the "alsoValid" test manifest file got dropped from your last patch, can you re-add it?  Currently the validator test fails without it.
Flags: needinfo?(tehsis)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        913711.patch
        — 
          — Splinter Review
      

    


  
oops!

Sorry for that, as I've said, I'm not quite confident with mercurial yet :)

As you have said, I forgot to add the manifest under the alsoValid directory.

Thanks again!

        Attachment #8491203 -
        Attachment is obsolete: true

        Attachment #8491541 -
        Flags: review+
Flags: needinfo?(tehsis)

    
    

    
  
Thanks, I've pushed it to Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2af80fa81fc2

If you'd rather use Git for future work, there are some tools to help do that with Mozilla processes[1].  It uses hg under the covers, but you don't have to think about it yourself.

[1]: https://github.com/mozilla/moz-git-tools

    
    

    
  
Try looks good, marking for checkin.
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/integration/fx-team/rev/4a0a256c54e2
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/4a0a256c54e2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 35

    
    

    
  
Hooray!  Thanks again for working on this.

Please check out our list of mentored bugs[1] or just look around Bugzilla for other DevTools work that interests you.

[1]: https://wiki.mozilla.org/DevTools/GetInvolved#Mentored_and_Good_First_Bugs

    
    

    
  
Thank you!

I'm on that :)

    
  
Product: Firefox → DevTools

    
  
Product: DevTools → DevTools Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: