Open
Bug 1119229
Opened 10 years ago
Updated 2 years ago
Remove onsync(to|from)preference attributes from in-content prefs
Categories
(Firefox :: Settings UI, defect)
Firefox
Settings UI
Tracking
()
NEW
People
(Reporter: Gijs, Unassigned)
References
(Blocks 1 open bug)
Details
(Whiteboard: [lang=js][lang=xul])
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
See bug 1016300 comment 13/14. AFAICT we could fix preferences.xml to always synthesize the events or use the event listener manager to check if the element has event handlers.
Reporter | ||
Updated•10 years ago
|
Mentor: gijskruitbosch+bugs
Whiteboard: [lang=js][lang=xul]
Comment 1•10 years ago
|
||
i would like to work on this bug
Comment 2•10 years ago
|
||
Since I'm new to mozilla community, i need some help in getting started. can you please explain me the bug.
Reporter | ||
Comment 3•10 years ago
|
||
(In reply to nithin from comment #2)
> Since I'm new to mozilla community, i need some help in getting started. can
> you please explain me the bug.
Hey nithin,
Great!
Are you familiar with JavaScript and DOM events? If so, you could have a look at fixing this bug. Please see https://developer.mozilla.org/en-US/docs/Introduction about how to get a Firefox build set up. That will be the first thing to sort out. You can use the "Need more information" checkbox and field to ping me again once you've done that.
Basically, you will need to change the XUL files under browser/components/preferences/in-content/ and move the inline "onsyncfrompreference" and "onsynctopreference" event handlers into the JS files that accompany the XUL files (they have the same names, apart from the extensions). You'll also need to update toolkit/content/widgets/preferences.xml to not make the event dispatch of these events conditional on the attributes being there.
If that sounds like it's a bit difficult because you're not that familiar with events and JS, that's not a problem, but I would suggest you give bug 1098517 a shot first, as it will be a bit simpler to get started with. You'll still need a Firefox build though - once you've gotten a build, comment on the bug and Matt or I can help you further there.
Reporter | ||
Comment 4•10 years ago
|
||
Oh, and if you have any questions about how to build Firefox itself, it's probably best to ask in the #introduction chat room on irc.mozilla.org.
Thanks!
Reporter | ||
Comment 5•10 years ago
|
||
Nithin, are you still working on this? Do you need more help getting going?
Flags: needinfo?(imnmfotmal)
Comment 6•10 years ago
|
||
i thought i will start working on this once i have finished the bug you referred https://bugzilla.mozilla.org/show_bug.cgi?id=1098517 . now i have submitted a patch for that(yet to be reviewed ), i will start working on this bug.
Flags: needinfo?(imnmfotmal)
Comment 7•10 years ago
|
||
using window.addEventListener(){}
Attachment #8566432 -
Flags: review?(gijskruitbosch+bugs)
Reporter | ||
Comment 8•10 years ago
|
||
Comment on attachment 8566432 [details] [diff] [review]
onsync_v1.patch
Review of attachment 8566432 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/components/preferences/in-content/main.js
@@ +138,5 @@
> Components.classes["@mozilla.org/observer-service;1"]
> .getService(Components.interfaces.nsIObserverService)
> .notifyObservers(window, "main-pane-loaded", null);
> +
> + window.addEventListener("onsyncfrompreference", function() {
I don't understand why you're adding an event listener to the window?
@@ +139,5 @@
> .getService(Components.interfaces.nsIObserverService)
> .notifyObservers(window, "main-pane-loaded", null);
> +
> + window.addEventListener("onsyncfrompreference", function() {
> + document.getElementById("browserHomePage").addEventListener("onsyncfrompreference", "return gMainPane.syncFromHomePref();");
This should be "syncfrompreference", and the same below, also for "synctopreference". The second parameter to "addEventListener" is a function, not a string, so these should be functions, not strings. Same for all the other calls.
Also, you will need to adapt all the other .xul files in this directory, and probably "languages.xul" in the directory above this one as well.
After that, you will need to change http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/preferences.xml#433 and friends so that the relevant events always get fired.
Then you should run the tests for this code (or at least rebuild Firefox's browser/components directory and open the preferences in your updated build) to see if things actually still work... :-)
::: browser/components/preferences/in-content/preferences.xul
@@ -148,5 @@
> <image class="category-icon"/>
> <label class="category-name" flex="1">&paneSecurity.title;</label>
> </richlistitem>
>
> -#ifdef MOZ_SERVICES_SYNC
You shouldn't remove the #ifdefs here.
Attachment #8566432 -
Flags: review?(gijskruitbosch+bugs)
Comment 9•10 years ago
|
||
Hi Nithin, any update on this?
Assignee: nobody → imnmfotmal
Status: NEW → ASSIGNED
Flags: needinfo?(imnmfotmal)
Comment 10•10 years ago
|
||
im having semester exams. and its GSoC application period, a bit busy. Will fix it as soon as internals are over :)
Flags: needinfo?(imnmfotmal)
Comment 11•10 years ago
|
||
it seems working. i don't know how to run tests for this piece of code. so i haven't tested it.
Attachment #8566432 -
Attachment is obsolete: true
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8589858 -
Flags: review?(gijskruitbosch+bugs)
Reporter | ||
Comment 12•10 years ago
|
||
Comment on attachment 8589858 [details] [diff] [review]
onsync_v2.patch
Review of attachment 8589858 [details] [diff] [review]:
-----------------------------------------------------------------
There are a lot of typoes here, and some changes that I'd recommend in preferences.xml (noted below). I would recommend you go through the diff and review it carefully yourself:
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1119229&attachment=8589858
Then adjust your patch accordingly and run some tests.
Basically, I think you should run at least:
./mach build browser/components toolkit/content
to rebuild, and then:
./mach mochitest-chrome toolkit/content/tests/test_preferences*
(note the *)
and once you have that passing, run:
./mach mochitest-browser browser/components/preferences
If you need help as to how to adjust code/tests in order to get this to pass, please attach an updated patch and needinfo me or ask for "feedback". Either one (not both) will be enough. :-)
::: browser/components/preferences/in-content/privacy.js
@@ +102,5 @@
> setEventListener("showCookiesButton", "command",
> gPrivacyPane.showCookies);
> setEventListener("clearDataSettings", "command",
> gPrivacyPane.showClearPrivateDataSettings);
> + setEventListener("acceptCookies", "syncfromprefrence",
All of these in this file are typoed.
::: browser/components/preferences/languages.js
@@ +14,5 @@
> {
> + function setEventListener(aId, aEventType, aCallback)
> + {
> + document.getElementById(aId)
> + .addEventListener(aEventType, aCallback.bind(gAdvancedPane));
copy-paste mistake? This shouldn't be gAdvancedPane.
@@ +21,4 @@
> if (!this._availableLanguagesList.length)
> this._loadAvailableLanguages();
> +
> + setEventListener("activeLanguages", "synctoprefrence",
Typo.
@@ +22,5 @@
> this._loadAvailableLanguages();
> +
> + setEventListener("activeLanguages", "synctoprefrence",
> + gLanguagesDialog.writeAcceptLanguages);
> + setEventListener("activeLanguages", "syncfromprefrence",
And another typo.
::: toolkit/content/widgets/preferences.xml
@@ +433,4 @@
> // Value changed, synthesize an event
> try {
> var event = document.createEvent("Events");
> + event.initEvent("syncfrompreference", true, true);
Nit: trailing space.
@@ +433,5 @@
> // Value changed, synthesize an event
> try {
> var event = document.createEvent("Events");
> + event.initEvent("syncfrompreference", true, true);
> + aElement.dispatchEvent(event);
I'm pretty sure you should assign to 'rv' here.
@@ +482,1 @@
> // Value changed, synthesize an event
Please reindent things now that the if statement is gone (ie remove two spaces in front of the lines that used to be in the if block).
@@ +482,5 @@
> // Value changed, synthesize an event
> try {
> var event = document.createEvent("Events");
> event.initEvent("synctopreference", true, true);
> + aElement.dispatchEvent(event);
You should check the result of this, too.
@@ +773,5 @@
> // Panel loaded, synthesize a load event.
> try {
> var event = document.createEvent("Events");
> event.initEvent(aEventName, true, true);
> +
Please undo this extra newline in the unrelated code.
Attachment #8589858 -
Flags: review?(gijskruitbosch+bugs)
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Reporter | ||
Comment 13•10 years ago
|
||
Hey nithin, are you still working on this? Do you need more help here? Please don't hesitate to ask.
Flags: needinfo?(imnmfotmal)
Comment 14•10 years ago
|
||
no, i am not working on this anymore. sorry for the delay.
Status: ASSIGNED → NEW
Flags: needinfo?(imnmfotmal)
Updated•10 years ago
|
Assignee: imnmfotmal → nobody
Reporter | ||
Comment 15•8 years ago
|
||
Rakhi will take this! :-)
Assignee: nobody → Rakhish1994
Status: NEW → ASSIGNED
Reporter | ||
Comment 16•8 years ago
|
||
While Rakhi did a bunch of good work in this bug, we ultimately realized that no event dispatch is currently in use. Instead, the attributes get 'eval'd to provide a return value (other than just a plain true/false rv, so not equivalent to e.g. event.isDefaultPrevented) and so converting these to use "proper" events is quite involved, and we won't be pursuing this further for the moment. As a result, it probably also isn't a good mentored bug. Unassigning Rakhi and removing myself as a mentor to clear that up.
Rakhi, would you mind attaching a diff of these changes in case we decide to work on this again in the future?
Assignee: Rakhish1994 → nobody
Mentor: gijskruitbosch+bugs
Status: ASSIGNED → NEW
Flags: needinfo?(Rakhish1994)
Comment 17•8 years ago
|
||
Sure Gijs. Here is the diff in case anyone wants to work on this bug- https://pastebin.mozilla.org/8890901
Thanks :)
Flags: needinfo?(Rakhish1994)
Comment 18•8 years ago
|
||
Adding Rakhi's patch as an attachment to make sure it is persisted.
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•