Closed
Bug 1188787
Opened 10 years ago
Closed 9 years ago
Use AppConstants instead of preprocessing in browser/components/places/
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox 43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: mak, Assigned: ma.steiman, Mentored)
Details
(Whiteboard: [good first bug][lang=js])
Attachments
(1 file, 6 obsolete files)
17.24 KB,
patch
|
Details | Diff | Splinter Review |
We should use AppConstants.platform and stop preprocessing PlacesUIUtils.jsm, places.js, browserPlacesViews.js, sidebarUtils.js, menu.xml.
Assignee | ||
Comment 1•10 years ago
|
||
Hey Marco,
I would love to work on this if I could get some more information on the problem and what needs to be fixed here. If it's not too abstract I'll assign it to myself and get to work however I'm still learning and want to make sure I can handle it. Any info you could post would be great, thanks!!
Flags: needinfo?(mak77)
Reporter | ||
Comment 2•10 years ago
|
||
Hello.
The above files are marked as preprocessed (either in moz.build or in jar.mn) cause they contain #ifdef directives.
We are trying to reduce the amount of preprocessing to speed up build time, so it would be nice to replace those #ifdefs with actual ifs in javascript.
Those ifs can be implemented using AppConstants.jsm module (it implements properties that let you know whether the current platform is macosx, or windows, for example).
You can find an example of such a change here: https://hg.mozilla.org/mozilla-central/rev/5fdc7a8a4269
Flags: needinfo?(mak77)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → ma.steiman
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•10 years ago
|
||
This patch uses AppConstants.jsm instead of preprocessing for all files except for PlacesUIUtils.jsm. For PlacesUIUtils, how should this be done? The preprocessing is importing modules:
>#ifdef MOZ_SERVICES_CLOUDSYNC
>XPCOMUtils.defineLazyModuleGetter(this, "CloudSync",
> "resource://gre/modules/CloudSync.jsm");
>#else
>let CloudSync = null;
>#endif
>#ifdef MOZ_SERVICES_SYNC
>XPCOMUtils.defineLazyModuleGetter(this, "Weave",
> "resource://services-sync/main.js");
>#endif
I don't think AppConstants.jsm can be used instead of these, and I couldn't seem to determine where else I could look. Any feedback would be appreciated!
Attachment #8642823 -
Flags: review?(mak77)
Reporter | ||
Comment 4•10 years ago
|
||
(In reply to Muhsin A. Steiman from comment #3)
> Created attachment 8642823 [details] [diff] [review]
> bug1188787_AppConstants.diff
>
> This patch uses AppConstants.jsm instead of preprocessing for all files
> except for PlacesUIUtils.jsm. For PlacesUIUtils, how should this be done?
> The preprocessing is importing modules:
>
> >#ifdef MOZ_SERVICES_CLOUDSYNC
yes, sorry, I'm asking someone with more Sync knowledge about those.
From confvars.sh looks like that is always defined in Firefox (http://mxr.mozilla.org/mozilla-central/source/browser/confvars.sh#36) from at least version 35, so I'm not sure why we have these ifdefs at all in browser/...
It's possible these are only leftover from old code changes, if it's unlikely we might disable cloudsync in the future, we should just remove them.
Mark, any idea?
Flags: needinfo?(markh)
Reporter | ||
Comment 5•10 years ago
|
||
Comment on attachment 8642823 [details] [diff] [review]
bug1188787_AppConstants.diff
Review of attachment 8642823 [details] [diff] [review]:
-----------------------------------------------------------------
you should also remove the asterisk from /browser/components/places/jar.mn from browserPlacesViews.js, places.js, menu.xml and sidebarUtils.js
The asterisk in a jar.mn file indicates the file needs preprocessing, since we removed the ifdefs that's no more needed (it would warn during the build otherwise)
PlacesUIUtils preprocessing is instead handled in the moz.build file in the same folder, but let's first wait for Mark's feedback.
::: browser/components/places/content/menu.xml
@@ +307,3 @@
> // XXX: The following check is a temporary hack until bug 420033 is
> // resolved.
> + Components.utils.import("resource://gre/modules/AppConstants.jsm");
please move the comment inside the if
The other problem here is that we don't want to retry to import again every time, we need to cache it.
See how tabbrowser.xml is doing that: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#81
Attachment #8642823 -
Flags: review?(mak77) → feedback+
Comment 6•10 years ago
|
||
(In reply to Marco Bonardo [::mak] from comment #4)
> It's possible these are only leftover from old code changes, if it's
> unlikely we might disable cloudsync in the future, we should just remove
> them.
Yeah, neither of those sync's are going away any time soon and a simple search will find the code to remove should that ever happen, so making them unconditional sgtm.
Flags: needinfo?(markh)
Assignee | ||
Comment 7•10 years ago
|
||
I've updated the patch with the corrections you mentioned. I hope I changed menu.xml correctly in terms of the caching issue.
Is PlacesUIUtils.jsm going to stay as it is for now or what needs to be done?
Attachment #8642823 -
Attachment is obsolete: true
Attachment #8643168 -
Flags: review?(mak77)
Reporter | ||
Comment 8•10 years ago
|
||
As Mark said, we can remove the ifdef on cloudsync and assume it's always true, since it's unlikely someone will disable it, if not for removing it (and at that point this is a non-issue)
Assignee | ||
Comment 9•10 years ago
|
||
This removes PlacesUIUtils.jsm's preprocessing directives for both syncs according to Mark's message. Let me know if this doesn't work, thanks!!
Attachment #8643168 -
Attachment is obsolete: true
Attachment #8643168 -
Flags: review?(mak77)
Attachment #8643607 -
Flags: review?(mak77)
Reporter | ||
Comment 10•10 years ago
|
||
Comment on attachment 8643607 [details] [diff] [review]
bug1188787_AppConstants.diff
Review of attachment 8643607 [details] [diff] [review]:
-----------------------------------------------------------------
thanks, there's still something to fix here but it's mostly good.
::: browser/components/places/content/menu.xml
@@ +305,5 @@
> return;
>
> + <field name="AppConstants" readonly="true">
> + (Components.utils.import("resource://gre/modules/AppConstants.jsm", {})).AppConstants;
> + </field>
you need to move this inside the <implementation> tag above, possibly at the top with the other fields like _indicatorBar
xul bindings have an implementation tag for fields and methods, and an handlers tag for event handlers
::: browser/components/places/content/sidebarUtils.js
@@ +34,5 @@
>
> + if (AppConstants.platform === "macosx")
> + var modifKey = aEvent.metaKey || aEvent.shiftKey;
> + else
> + var modifKey = aEvent.ctrlKey || aEvent.shiftKey;
this cannot work cause you are defining the variable inside the if else now, while before it was in scope.
you need something like
var metaKey = AppConstants.platform === "macosx" ? aEvent.metaKey
: aEvent.ctrlKey;
var modifKey = metaKey || aEvent.shiftKey;
::: browser/components/places/moz.build
@@ +13,1 @@
> with Files('**'):
you removed PlacesUIUtils.jsm completely from the build, and that won't do.
You just need to replace EXTRA_PP_MODULES with EXTRA_MODULES instead (the PP indicates preprocessed)
Attachment #8643607 -
Flags: review?(mak77)
Assignee | ||
Comment 11•10 years ago
|
||
Marco,
This patch reflects your latest comments. Thank you for your patience.
Attachment #8643607 -
Attachment is obsolete: true
Attachment #8645346 -
Flags: review?(mak77)
Reporter | ||
Comment 12•10 years ago
|
||
Comment on attachment 8645346 [details] [diff] [review]
bug1188787_AppConstants.diff
Review of attachment 8645346 [details] [diff] [review]:
-----------------------------------------------------------------
No worries for the patience, we do mentoring to help people like you understanding how the patching process works, which kind of code quality we look for, and grow new contributors. If you find this interesting and you learn new things along the way, we all win.
Please add the bug number to the commit message, it should look like:
"Bug 123456 - Description. r=reviewer"
Btw, looks like I didn't explain well how to stop preprocessing PlacesUIUtils.jsm
in the moz.build file you currently can find:
EXTRA_PP_JS_MODULES += [
'PlacesUIUtils.jsm',
]
this means "add PlacesUIUtils.jsm to the list of preprocessed modules". we need to change this to:
EXTRA_JS_MODULES += [
'PlacesUIUtils.jsm',
]
that means "add PlacesUIUtils.jsm to the list of modules"
::: browser/components/places/content/menu.xml
@@ +307,5 @@
> let elt = event.target;
> if (elt.parentNode != this)
> return;
>
> + if (AppConstants.platform === "macosx") {
should be this.AppConstants.platform, cause you are adding the AppConstants field to the current xul object (this).
::: browser/components/places/content/places.js
@@ +124,5 @@
> + findKey.setAttribute("command", "OrganizerCommand_find:all");
> +
> + // 2. Disable some keybindings from browser.xul
> + var elements = ["cmd_handleBackspace", "cmd_handleShiftBackspace"];
> + for (var i=0; i < elements.length; i++) {
nit: while here, could you please add white space around the "=" in "i=0", and convert the vars in this method from "var" to "let"? When we touch code we usually also try to fix coding style issues that come from the past.
Attachment #8645346 -
Flags: review?(mak77) → feedback+
Assignee | ||
Comment 13•10 years ago
|
||
Patch updated. I could have sworn I updated moz.build upon reading your comment, I guess a perfect example of how important attention to detail is.
Attachment #8645346 -
Attachment is obsolete: true
Attachment #8645357 -
Flags: review?(mak77)
Reporter | ||
Comment 14•10 years ago
|
||
Comment on attachment 8645357 [details] [diff] [review]
bug1188787_AppConstants.diff
Review of attachment 8645357 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/components/places/content/places.js
@@ +124,5 @@
> + findKey.setAttribute("command", "OrganizerCommand_find:all");
> +
> + // 2. Disable some keybindings from browser.xul
> + var elements = ["cmd_handleBackspace", "cmd_handleShiftBackspace"];
> + let (var i = 0; i < elements.length; i++) {
ehr sorry I didn't mean to replace "for" with "let", this is not going to work.
my comment was about replacing "var" with "let" just in this method, cause in new code we use let, old code used var all around
Attachment #8645357 -
Flags: review?(mak77) → feedback+
Assignee | ||
Comment 15•10 years ago
|
||
Updated ... don't know what's wrong with me today :\
Attachment #8645357 -
Attachment is obsolete: true
Attachment #8645358 -
Flags: review?(mak77)
Reporter | ||
Comment 16•10 years ago
|
||
Comment on attachment 8645358 [details] [diff] [review]
bug1188787_AppConstants.diff
Review of attachment 8645358 [details] [diff] [review]:
-----------------------------------------------------------------
Thank you for the patch! I'm pushing to the try server, if this will pass someone will then merge to the main repo.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f3e5e815f3c
Attachment #8645358 -
Flags: review?(mak77) → review+
Reporter | ||
Updated•10 years ago
|
Keywords: checkin-needed
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(mak77)
Reporter | ||
Comment 18•9 years ago
|
||
the problem were the license headers, we were removing them through the preprocessor.
this should do, I just changed them to be comments.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=51aa861216b5
Attachment #8645358 -
Attachment is obsolete: true
Flags: needinfo?(mak77)
Reporter | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 19•9 years ago
|
||
Keywords: checkin-needed
Comment 20•9 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
You need to log in
before you can comment on or make changes to this bug.
Description
•