Use ordered arguments in nativeWindow.deprecated

RESOLVED FIXED in Firefox 36

Status

()

Firefox for Android
General
P5
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: flod, Assigned: Gordon Loery, Mentored)

Tracking

unspecified
Firefox 36
Points:
---

Firefox Tracking Flags

(fennec+)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
# LOCALIZATION NOTE (nativeWindow.deprecated):
# This string is shown in the console when someone uses deprecated NativeWindow apis.
# %1$S=name of the api that's deprecated, %2$S=New API to use. This may be a url to
# a file they should import or the name of an api.
nativeWindow.deprecated=%S is deprecated. Please use %S instead

Please use ordered variables in the string, not just in the localization comment.

Also: no need to change string ID in this case (localizers could already use %1$S and %2$S in their strings).

Updated

4 years ago
Assignee: nobody → wjohnston
tracking-fennec: --- → 34+
tracking-fennec: 34+ → +
filter on [mass-p5]
Priority: -- → P5

Comment 2

4 years ago
This bug involves updating the en-US string here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/locales/en-US/chrome/browser.properties#395

And then you should make a build and test with an add-on that uses the old NativeWindow.pageactions API to make sure this warning appears correctly:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2826
Assignee: wjohnston → nobody
Mentor: margaret.leibovic
Whiteboard: [good first bug][lang=properties]
(Assignee)

Comment 3

4 years ago
Would it be ok if I worked on this?
(Assignee)

Comment 4

4 years ago
Created attachment 8509216 [details] [diff] [review]
Attaching first patch attempt

I think I implemented the change correctly (patch file attached). How would I go about building with an add-on that uses the old NativeWindow.pageactions API in order to test? I don't quite follow that part.

Comment 5

4 years ago
(In reply to Gordon Loery from comment #4)

> I think I implemented the change correctly (patch file attached). 

Thanks for taking a stab at this!

> How would I go about building with an add-on that uses the old
> NativeWindow.pageactions API in order to test? I don't quite follow that
> part.

I'm sorry, that comment wasn't totally clear. To make an add-on testcase, you can just grab our boilerplate add-on code here:
https://github.com/mozilla/firefox-for-android-addons/tree/master/boilerplate/native-ui-boilerplate

And then you can just try making a NativeWindow.pageactions call somewhere in here:
https://github.com/mozilla/firefox-for-android-addons/blob/master/boilerplate/native-ui-boilerplate/bootstrap.js#L50

You'll see examples in there of how to use NativeWindow. Since this add-on would just be used for testing, it doesn't actually need to do anything useful :)

Alternately, you can try testing an old add-on that's written with this old API. It looks like wesj's refresher add-on does this:
https://github.com/wesj/refresher/blob/master/bootstrap.js#L22
Assignee: nobody → gloery945
(Assignee)

Comment 6

4 years ago
All right, was successfully able to test it out on wesj's refresher add-on! Here's the error that displayed:
JavaScript Error: "NativeWindow.pageactions is deprecated. Please use resource://gre/modules/PageActions.jsm instead"
(I think that's the error I should be getting, right?)

Comment 7

4 years ago
(In reply to Gordon Loery from comment #6)
> All right, was successfully able to test it out on wesj's refresher add-on!
> Here's the error that displayed:
> JavaScript Error: "NativeWindow.pageactions is deprecated. Please use
> resource://gre/modules/PageActions.jsm instead"
> (I think that's the error I should be getting, right?)

Yes, looks like your patch is working! :)

I'm going to go ahead and give this an r+, so we can land it, but in the future, when you upload a patch, you can set the review? flag to an appropriate reviewer to request approval. If you upload a work-in-progress patch, you can also set the feedback? flag for more general feedback.

Thanks for the patch!

Comment 8

4 years ago
Comment on attachment 8509216 [details] [diff] [review]
Attaching first patch attempt

You should upload a new version of the patch that's formatted for checkin. Here are some instructions:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Then we can set the checkin-needed keyword on the bug, and someone will come along to check it in.
Attachment #8509216 - Flags: review+
(Assignee)

Comment 9

4 years ago
Created attachment 8510521 [details] [diff] [review]
Reformatted patch file

Ok, I followed the instructions on that page and I think I updated the patch format correctly!
Attachment #8509216 - Attachment is obsolete: true
Attachment #8510521 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 10

4 years ago
Also, for a second bug to tackle, I'm currently looking through the "mentored bugs" section of Bugzilla; would that be the right way to go?

Thank you for your help! :)

Comment 11

4 years ago
Comment on attachment 8510521 [details] [diff] [review]
Reformatted patch file

Looks great, thanks!
Attachment #8510521 - Flags: review?(margaret.leibovic) → review+

Comment 12

4 years ago
(In reply to Gordon Loery from comment #10)
> Also, for a second bug to tackle, I'm currently looking through the
> "mentored bugs" section of Bugzilla; would that be the right way to go?

Yes, that's the perfect place to look! Once you find a bug you'd like to try, you can just comment in it to express interest, and the mentor should be able to answer any questions you have.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/805c0b62d318
Keywords: checkin-needed
Whiteboard: [good first bug][lang=properties] → [good first bug][lang=properties][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/805c0b62d318
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=properties][fixed-in-fx-team] → [good first bug][lang=properties]
Target Milestone: --- → Firefox 36
You need to log in before you can comment on or make changes to this bug.