Closed Bug 934274 Opened 6 years ago Closed 6 years ago

[app manager] validator.invalidLaunchPath=Unable to access to app starting document '%S': second "to" should be "the"

Categories

(DevTools :: WebIDE, defect, trivial)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 28

People

(Reporter: aryx, Assigned: cpt.at.work)

Details

(Whiteboard: [good first bug][lang=html][mentor=jryans])

Attachments

(1 file, 2 obsolete files)

/mozilla/browser/locales/en-US/chrome/browser/devtools/app-manager.properties

validator.invalidLaunchPath=Unable to access to app starting document '%S'
validator.invalidLaunchPathBadHttpCode=Unable to access to app starting document '%1$S', got HTTP code %2$S

The second "to"s should be "the"s. The entity IDs should be changed (e.g. by appending a 2), of course also where they are being called.
Whiteboard: [good first bug] → [good first bug][lang=html][mentor=jryans]
I am ready to fix this bug. Please assign it to me.
Great!  I've assigned the bug to you.  I believe comment 0 explains what's needed pretty clearly, but let me know if you have any questions.

Also, if you've never worked with the Firefox code base before, check out our Hacking[1] page for help with that.

[1]: https://wiki.mozilla.org/DevTools/Hacking
Assignee: nobody → cpt.at.work
Status: NEW → ASSIGNED
Attached patch bug (obsolete) — Splinter Review
Attachment #827235 - Flags: review?(jryans)
Comment on attachment 827235 [details] [diff] [review]
bug

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

Overall, looks like you've got the right idea!  Just a few naming improvements and reverting an unrelated file is needed.

Once you've made these changes, flag me for review on the updated patch.

::: browser/locales/en-US/chrome/browser/devtools/app-manager.properties
@@ +34,4 @@
>  validator.invalidHostedPriviledges=Hosted App can't be type '%S'.
>  validator.noCertifiedSupport='certified' apps are not fully supported on the App manager.
>  validator.nonAbsoluteLaunchPath=Launch path has to be an absolute path starting with '/': '%S'
> +validator.invalidLaunchPath1=Unable to access the app starting document '%S'

If you are going to add a number, then using "2" tends to be the convention, as in "invalidLaunchPath2".  The thinking is that the original is #1, so the fix would be #2.

Slightly rewording the key can work too, just to avoid the somewhat unsightly "2", which I think reads a bit better as well. :) Here we could go with "accessFailedLaunchPath", so something like that would be a good choice.

@@ +39,2 @@
>  # the launch document, %2$S is the http error code.
> +validator.invalidLaunchPathBadHttpCode1=Unable to access the app starting document '%1$S', got HTTP code %2$S

As above, we could use a "2", but let's make it "accessFailedLaunchPathBadHttpCode" to skip the number entirely.

::: dom/tests/mochitest/webapps/test_install_errors.xul
@@ +24,4 @@
>    invalidManifest,
>    permissionDenied,
>    invalidContent,
> +  invalidLaunchPath1,

This file is unrelated to what we are changing here, so please revert this change.

@@ +103,4 @@
>    };
>  }
>  
> +function invalidLaunchPath1(next) {

Revert this too.
Attachment #827235 - Flags: review?(jryans)
Also, you'll need to use proper Mercurial formatting on your patch, with a correctly formatted commit message.  See the guide[1] for more info.

If you are using Git, the same page has info about tools to make it generate patch in the right format.

[1]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Attached patch bug (obsolete) — Splinter Review
Attachment #827235 - Attachment is obsolete: true
Attachment #827265 - Flags: review?(jryans)
Sorry. I didn't see comment5. I will make the required changes and submit it again.
Attached patch bug934274.patchSplinter Review
Attachment #827265 - Attachment is obsolete: true
Attachment #827265 - Flags: review?(jryans)
Attachment #827281 - Flags: review?(jryans)
Comment on attachment 827281 [details] [diff] [review]
bug934274.patch

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

Great!  This looks good to me.

I've pushed this change to our Try server to run tests on various platforms, which will take several hours to run:

https://tbpl.mozilla.org/?tree=Try&rev=6cb47afb0f52

Assuming the tests come back green, then you can add the keyword "checkin-needed" on this bug, and someone will land the patch for you.

Thanks again for your contribution! :D
Attachment #827281 - Flags: review?(jryans) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/7a2feb9904f0
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [good first bug][lang=html][mentor=jryans] → [good first bug][lang=html][mentor=jryans][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/7a2feb9904f0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=html][mentor=jryans][fixed-in-fx-team] → [good first bug][lang=html][mentor=jryans]
Target Milestone: --- → Firefox 28
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.